[flashrom] [PATCH] Boards: Unify all intel ICH GPIO raising.

[Carl-Daniel Hailfinger]

On 22.10.2009 15:16, Luc Verhaegen wrote:
> We are writing flashrom here. We are dealing with bits and pci-ids. Lots 
> of them. If we cannot see the important things because we abstracted too 
> far or too little, then we fail.
>   

I agree that finding the right abstraction level is key.


> 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.


> 		return -1;
> 	}
>
> 	ich_gpio_raise(dev, 34); /* #TBL */
> 	ich_gpio_raise(dev, 35); /* #WP */
>
> 	return 0;
> }
>
> Over the completely impossible:
>
> {
> 	int ret;
>  	ret = ich_gpio_raise(name, 0x8086, 0x27B8, 0x48, 0x0c, 0xffc0, 34);
> 	if (!ret)
> 		ret = ich_gpio_raise(name, 0x8086, 0x27B8, 0x48, 0x0c, 0xffc0, 35);
>
> 	return ret;
> }
>   

I won't claim the old interface was perfect. It definitely had too many
parameters.


> Or even this, which still does not make me comfortable, and which 
> requires the big switch statement in the ich_gpio_raise function anyway.
>
> {
>         int ret;
>         ret = ich_gpio_raise(name, 0x27B8, 34);
>         if (!ret)
>                 ret = ich_gpio_raise(name, 0x27B8, 35);
>
>         return ret;     
> }
>   

The example above has pretty readable code, though we could argue about
whether it is a good that you have to specify the ID twice.


> 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).


> 	if (!dev)
> 		return -1;
>
>         ich_gpio_raise(dev, 34); /* #TBL */
>         ich_gpio_raise(dev, 35); /* #WP */
>
>         return 0;
> }
>
> It saves us from a pair of {} and an fprintf, and pretty much takes the 
> comment in as an argument. But this is as far as it goes in a productive 
> and maintainable manner, and the only further reduction in codesize i 
> would accept. Going any further will make the whole thing less 
> immediately readable. But since we are going to make this saving on 
> 19 board enables already (not counting the pending ones), we probably 
> should consider this change.
>
> We really have enough of these board enables today to be able to see
> some lines in there today. We can see the most important one now, and we
> are slowly acquiring the routines for touching gpio lines on common
> LPC/ISA bridges for most vendors now; we have via, nvidia and intel now,
> and the board enables for those are pretty much all the same formula:
>
> * find device.
> * vendor_devicefamily_gpio_set(device, gpio)
>
> And we have many of those sitting in our board_enable.c with many more
> probably following. And a few of them already touch multiple gpio lines.
>
> When i originally set up this table, i intended for the first device 
> in the list to be passed as the device that should be poked for such 
> things. But things evolved differently for several reasons:
> * not all board enables touch the pci devices or their io areas.
> * one did not get a pretty print message when the device was not found.
> * people felt safer with an extra check in the board enable.
> * it was not clear enough from the onset of this table that this first 
>   device was meant to be the passed and used when needed. But then, 
>   quite a few of the earlier board enables treated the pciids rather 
>   stepmotherly anyway.
> * the ISA/LPC bridge was not always a good entry for matching a board
>   uniquely.
>   

Yes, the table evolved in unexpected ways.


> If we move to a three device table (let's postpone that until actually 
> completely unavoidable), then maybe we should reconsider using a fixed
> entry as to avoid having to find the device inside the board enable, but 
> not before.
>
> About the bug, would this be endianness in the longs?

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).


> 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?


> Also, we are no longer fully on par with the previous board enables 
> which used to touch one bit only per gpio line. Now we touch three. 
> Idwer has a board which requires us to set the first bit (signal to 
> gpio) and the third (gpio level), and it can be forseen that the 
> input/output also needs to be set somewhere in future. Since we need to 
> go through the pain of getting this code tested on a representative 
> subset of this hardware anyway, why not go for a fuller test which will 
> hopefully stand the test of time better?
>   

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.


> Flashrom is maturing, and here is one of the growing pains :)
>   

I'm very happy that we have the chance to fix this up before the code
becomes too ugly to touch.

Thank you for tackling this issue!

Regards,
Carl-Daniel

-- 
Developer quote of the week: 
"We are juggling too many chainsaws and flaming arrows and tigers."