[coreboot] SST25VF016B (2MB) flash on m57sli (IT8716F).

Ronald Hoogenboom ronald at zonnet.nl
Wed Feb 20 23:21:10 CET 2008


Hi Ward,

Thanks for reviewing my patch. I'm glad to hear that it got you a
booting mainboard too.

Most of the seemingly unrelated changes are a result of my quest to get
my nvidia VGA to work. Also I'm quite new submitting patches and I'm not
yet very familiar with what stuff is allowed to be in a patch like this
and what not. Thanks for the advise in this.

 Please find my comments below.


On Tue, 2008-02-19 at 23:34 -0500, Ward Vandewege wrote:
> Hi Ronald,
<snip>
> > Not so nice: hardcoded 0x820 SPI-IO port.
> 
> Is that superio specific? Maybe some sort of lookup table based on the
> superio in use would be a solution? At this point there would be just the one
> line I presume.
> 

I was thinking this port number should be snarfed from the
mainboard/gigabyte/m57sli/Config.lb file (see line 293: io 0x64 =
0x820), but I couldn't figure out how, so I hardcoded it to get going.

> > -	unsigned long index;
> > +	//unsigned long index;
> 
> What's this change for? Unrelated to the rest of the patch? If it's general
> cleanup, please do this in a separate patch.
> 
This was only to stop vim from going to that file every compile ;-).
Just to get rid of an 'unused variable' warning.

<snip>
> > +uses CONFIG_VGA_ROM_RUN
<snip>
> > -default CONFIG_PCI_ROM_RUN=1
> > +default CONFIG_VGA_ROM_RUN=1
> > +default CONFIG_PCI_ROM_RUN=0
> 
> Unrelated to the rest of the patch? I'm not sure I want this default to
> change - it's nice to have any PCI roms run by default.
> 
You are right. But it might be good to at least give the user the option
to choose to run only VGA ROM (get display going) and leave the rest to
the OS.

> > -default MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID=0x1022
> > -default MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID=0x2b80
> > +default MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID=0x1458
> > +default MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID=0xe000
> 
> Also unrelated to the rest of the patch? This does not seem to have any
> effect on my box. Also; the proprietary bios uses 0x1022; why are you
> changing this?
> 
My prop. BIOS does put the gigabyte subvendor ID in. When I changed it,
I was only looking at MCP55 only (which gets the e000 DID from the prop.
BIOS), I didn't realize the setting inpacted EVERY PCI card that can set
subsystem ID's. Indeed it didn't have any effect with me either, but I
do think that the 1458 vendor is more appropriate (for what that's
worth...)

> This is great work; if you can respond to the comments above I think I can
> ack this soon.
> 
Thanks, please let me know what to leave out definitively or what to put
in a separate patch and I'll send (a) new patch(es).

> Thanks,
> Ward.
> 
Regards,
Ronald.





More information about the coreboot mailing list