[LinuxBIOS] [PATCH] K8T890/VT8237 A8V-E SE (pre)release!

Uwe Hermann uwe at hermann-uwe.de
Sat Oct 13 12:04:08 CEST 2007


On Sat, Oct 13, 2007 at 12:06:44AM -0400, Corey Osgood wrote:
> Few comments, please resend with these fixes and I'll ack, I need this
> to get in as well (so I can figure out how to patch against it without
> breaking things on your end).

Quick review will follow in another mail, but please don't commit this
yet, we shouldn't rush it before the code is in shape.

 
> > +Fix enable more than 512KB flash chip
> 
> Hrm, not sure about this. vt8237r supports 4Mb to 16Mb flash, but only
> if it's strapped correctly. I'm not sure why they did that, but it's
> extremely annoying. So unless your chip is strapped correctly (or you
> change the strapping) you can't use any larger than the motherboard
> manufacturer decided to (here, it's 4MB :( )

Does enabling more in the southbridge have any harmful effect? If not
I'd say enable as much as you can. Whether it'll be of any use depends
on the board, sure, but the default should be the maximum possible
value, IMO.


> > +Fix HPET acpi record to match the ACPI reported table
> > +Fix the e820 reserved region for MMCONFIG
> > +Fix PCIe startup - done
> > +
> > +Fix error conditiion when PCIe Link is not trained correctly.
> > +Disable GART in NB?
> > +
> 
> I have no objection to a TODO list, I rather like the idea. But please
> have separate lists for each part.

... and merged in existing *.c or *.h files. Please drop the extra file.

 
> > Index: src/southbridge/via/vt8237r/romstrap.inc
> > ===================================================================
> > --- src/southbridge/via/vt8237r/romstrap.inc	(revision 0)
> > +++ src/southbridge/via/vt8237r/romstrap.inc	(revision 0)
> > Index: src/southbridge/via/vt8237r/romstrap.lds
> > ===================================================================
> > --- src/southbridge/via/vt8237r/romstrap.lds    (revision 0)
> > +++ src/southbridge/via/vt8237r/romstrap.lds    (revision 0)
> 
> What are these files all about? Do they really belong in vt8237r?

Good question. They come from the NVIDIA code I think, are they really
needed here?


> > +void set_led()
> > +{
> > +	// set power led to steady now that lxbios has virtually done its job
> > +	//device_t dev;
> > +	//dev = dev_find_device(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_8237, 0);
> > +	//pci_write_config8(dev, 0x94, 0xb0);
> > +}
> 
> Please just remove this function and all calls to it.

Or preferrably, make it work :)

 
> > Index: src/southbridge/via/vt8237r/vt8237r.h
> > ===================================================================
> > --- src/southbridge/via/vt8237r/vt8237r.h    (revision 0)
> > +++ src/southbridge/via/vt8237r/vt8237r.h    (revision 0)
> > @@ -0,0 +1,2 @@
> > +//GPL here
> > +
> 
> Same here, no need for an empty file.

Yep.

 
> Aside from that, there's a bunch of whitespace issues, etc, which can be
> fixed later.

... but before the commit, preferrably.



Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071013/b4869677/attachment.sig>


More information about the coreboot mailing list