[flashrom] [PATCH] Boards: Unify all intel ICH GPIO raising.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri Oct 23 18:49:18 CEST 2009
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 at gmx.net>
Could you repost the patch so everyone knows the final state? Thanks.
Regards,
Carl-Daniel
--
Developer quote of the week:
"We are juggling too many chainsaws and flaming arrows and tigers."
More information about the flashrom
mailing list