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