[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