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

[Luc Verhaegen]

On Fri, Oct 23, 2009 at 01:41:41AM +0200, Carl-Daniel Hailfinger wrote:
> 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.

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

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.

What problem with the board enable table? Copy/paste will not work, and 
i believe that we have become a lot more stringent already on what we 
accept and what not (and we should make the code more stringent as well, 
cfr the mail sent out a week or two ago).

> 
> Yes, the table evolved in unexpected ways.

Actually, using the first dev as the one passed never made it in even 
the first commit. I think i thought along those lines when originally 
developing this or something, or maybe first patch was like that, no 
real idea anymore; my synapses just tell me that this was one of the 
original intentions.

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

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

> 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.
* 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.
* function renamed to intel_ich_gpio_raise
* part of the dell comment restored.

We can worry about reducing code duplication later on, right now we need 
to get the gpio setting code verified.

Luc Verhaegen.