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.
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.