2009/10/24 Luc Verhaegen <[email protected]> > On Fri, Oct 23, 2009 at 06:49:18PM +0200, Carl-Daniel Hailfinger wrote: > > On 23.10.2009 16:27, Luc Verhaegen wrote: > > > On Fri, Oct 23, 2009 at 01:41:41AM +0200, Carl-Daniel Hailfinger wrote: > > > > > > Boards are very heavily tied to chipsets. Quite often, southbridge > > > manufacturers do not provide complete drop-in replacements for new > > > chipsets, and boards come with the same chipset even if they run a bit > > > longer, just with different revisions. > > > > > > > Yes, but the way you suggested keeps the description around in a comment > > and an error message. That's duplication. > > Yeah, we can do away with this duplication by turning that into one > function which takes the name as the argument. > > > > > > >>> Once you have seen a few of those pci dev finding things, and they > all > > >>> get copy/pasted around anyway, you quickly see what is important to > > >>> them, and they are all very formulaic anyway. You learn to ignore the > > >>> rest. > > >>> > > >>> What could be done is the following: > > >>> > > >>> /** > > >>> * Saves a few lines per board enable routine. > > >>> */ > > >>> struct pci_dev *pci_dev_find_board(uint8_t vendor, uint8_t device, > char *name) > > >>> { > > >>> struct pci_dev *dev; > > >>> > > >>> dev = pci_dev_find(vendor, device); > > >>> if (!dev) > > >>> fprintf(stderr, "\nERROR: %s not found.\n"); > > >>> > > >>> return dev; > > >>> } > > >>> > > >>> Which then turns the first board enable into the following: > > >>> > > >>> { > > >>> struct pci_dev *dev; > > >>> > > >>> dev = pci_dev_find_board(0x8086, 0x27b8, "Intel ICH7 LPC bridge"); > > >>> > > >>> > > >> Two ways to make the above acceptable: > > >> - Call it pci_dev_find_intel and remove the vendorid+name parameter. > > >> - Call it pci_dev_find_name and remove the name parameter. > > >> > > >> As I wrote above, my major objection is having the chipset name in a > > >> board function. This does not scale and leads to exactly those > problems > > >> we have now with the board enable table (copy and paste without > thinking). > > >> > > > > > > Well, if we provide bare pciids, people will add comments or would want > > > to see them anyway. This argument list there provides documentation in > > > the same go. > > > > > > > Let's use your original pci_dev_find variant and refactor it later. > > Right now this discussion is leading nowhere and I want the important > > parts of the patch merged. > > Yeah, this way it is at least in the same shape as the other functions, > and this refactoring (of all) can be done later without much danger, the > touching of extra gpio bits now is much more likely to break things. > > > > This i have checked. All of the intel ICHs have the lower bits zeroed > > > per definition (except bit 0 which is 1 for this being an io bar). > > > Everyone is having 5:1 zeroed, except the very latest (9 & 10) which > > > have 6:1 zeroed. So this mask thing does not matter. > > > > > > > Ah ok. But please add your explanation above as a comment to the generic > > ICH function. > > Done. > > > If we get reports from our testers that everything is fine indeed, I'm > > OK with either byte or word or dword access. > > Wait and see. I will try to get this one board enable resent, on top of > this patch here. And will get idwer his board enable out (finally). This > is two more testcases. > > > The pain is in boards with multiple flash chips where flashrom may > > toggle the chip select lines in the future (DualBIOS). Anyway, we can > > handle this later. > > Right, solve it when it occurs and when we know what it is exactly that > we need to do. > > > If it works, no objection from my side. > > So then we are just waiting for testers ;) > > > > * part of the dell comment restored. > > > > > > > That one is crucial. For referrence, here's the rewritten comment which > > would be OK for me: > > "Suited for the Dell PowerEdge 1850. The GPIO number has to be in the > > range [16,31] according to the public Intel 82801EB ICH5 / 82801ER ICH5R > > datasheet and was found by exhaustive search." > > Done. >
Acked-by: Idwer Vollering <[email protected]> > > Latest patch attached, difference is just those two comments and the > intel_ prependage (and the fact that i now went to git-svn so i can > juggle the patches more easily). INL/OUTL was kept while waiting for > testing. > > Luc Verhaegen. > > _______________________________________________ > flashrom mailing list > [email protected] > http://www.flashrom.org/mailman/listinfo/flashrom >
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
