2009/10/24 Luc Verhaegen libv@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@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@flashrom.org http://www.flashrom.org/mailman/listinfo/flashrom