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.