On 23.10.2009 16:27, Luc Verhaegen wrote:
On Fri, Oct 23, 2009 at 01:41:41AM +0200, Carl-Daniel Hailfinger wrote:
On 22.10.2009 15:16, Luc Verhaegen wrote:
I prefer to see this:
{ struct pci_dev *dev;
dev = pci_dev_find(0x8086, 0x27b8); /* Intel ICH7 LPC */ if (!dev) { fprintf(stderr, "\nERROR: Intel ICH7 LPC bridge not found.\n");
The line above is chipset specific and does not belong in board code.
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.
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.
No, the size of the BAR differs depending on the chipset generation. One of the parameters of the old function was the mask for the BAR address and that parameter was wrong in some cases. Since your patch also has the same mask for all generations, the bug has been carried forward (I won't blame you for this).
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.
I changed this only later on, as it really made the whole thing clearer. Working with a gpio_byte = gpio / 8; gpio_bit = gpio % 8; and then using INB/OUTB is just 3 lines more, and another + gpio_byte in each INB/OUTB, but this is of course quite acceptable.
Do the new and old chipset generations handle byte accesses gracefully or do they expect dword accesses?
I do not think that such detail is given in the datasheets or whether it really matters. Since i do not have any intel hw, i also cannot go and find out and i will have to depend on people testing this. I expect both to be just fine.
If we get reports from our testers that everything is fine indeed, I'm OK with either byte or word or dword access.
Since we're already in the process of designing this function in a way that is future-proof, I'd like to have undo (restore) ability for board enables which will be called from programmer_shutdown. If this was just a matter of touching at most one GPIO per flashrom execution, it would be simple to use static variables for saving the old state. Since we do touch more than one GPIO line for some boards, this is a bit more complicated. Redesigning the code yet another time later would make this whole discussion obsolete and waste some of the effort which has been put in. We need a way to store the signal/direction/level tuple for every GPIO this function is called for, and enable per-GPIO restore. One way would be to have this function keep a list of GPIOs it touched and for every touched GPIO keep the state tuple in a locally allocated data structure. The function would take an additional parameter enum "action": set_bit/clear_bit/restore_bit.
Oh, please don't. This is a lot of hassle and not really necessary. I can understand if you want this to happen for chipset enable, there it is just fine, and the overhead is little. For the board specific enable i would not like to see this at all.
- only some boards need a board enable. only some manufacturers
implement this, while many don't bother. This means that it really is not that necessary to do this.
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.
- If we use coreboot, then the general idea is that we bring up the
lpc/superio in such a way that a board enable is no longer necessary.
This means that we really should not care too much about board enable undoing if it is just a gpio line which is tied to the write protect pin.
There could be situations where this becomes necessary, and then we could add another callback hook. But only when it becomes necessary, not before.
In any case, i would like to see this patch being accepted pretty much as is, with the following changes:
- concern about INL/OUTL addressed if that is still considered an issue.
If it works, no objection from my side.
- function renamed to intel_ich_gpio_raise
OK.
- 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."
We can worry about reducing code duplication later on, right now we need to get the gpio setting code verified.
OK.
With the agreed upon changes, this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Could you repost the patch so everyone knows the final state? Thanks.
Regards, Carl-Daniel