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

[Luc Verhaegen]

On Thu, Oct 22, 2009 at 03:50:53AM +0200, Carl-Daniel Hailfinger wrote:
> On 22.10.2009 02:56, Luc Verhaegen wrote:
> > Ok, big change here which will require several of the people on the list 
> > below to test and Ack this patch. But one has to admit; the result is 
> > worth it.
> >   
> 
> Before anybody tests:
> AFAICS the patch has a bug in the bitmask code which was already present
> in the old code, but the new code makes it harder to fix. Besides that,
> duplicating pci_dev_find in every board function strikes me as something
> that was better in the old code.
> I do agree that the old code required too much chipset knowledge into
> the board functions, so this patch does improve the situation in that
> regard.
> Luc, if you want me to go in detail I can do that after I've gotten some
> sleep.
> 
> So it is pretty likely that we'll see another iteration of the patch
> which has to be tested as well...
> 
> Regards,
> Carl-Daniel

Well, i prefer it this way with the pci_find_dev.

It keeps individual board enables formulaic enough to be instantly
acceptable to the eye, especially since there are 20 or so of these in 
there.

I have dabbled with scheme half a decade ago and was amazed at its power 
and its abilities to solve very complex problem with a handful of lines 
of code. And then i also noticed how detrimental this abstractional 
power can be; with this power, developers get rewarded to write as 
little lines of code as possible while spending a lot of mental work on 
it. So much so, that a short while after this mental work has been done, 
and the developer only remembers some of the things he did, and now has 
become totally unable to read, let alone correct, the code he wrote 
without redoing a significant part of the mental work from scratch.
  
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 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");
		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;
}

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;     
}

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

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

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?

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

Luc Verhaegen.