[flashrom] [PATCH] Boards: Unify all intel ICH GPIO raising.

[Idwer Vollering]

2009/10/24 Luc Verhaegen <libv at skynet.be>

> 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 <vidwer at gmail.com>


>
> 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
> flashrom at flashrom.org
> http://www.flashrom.org/mailman/listinfo/flashrom
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20091024/41df2734/attachment.html>