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.
Jonathan, can you verify this patch on your acorp 6A815EPD (preferably also with the previous patch)? Thanks for having replied that swiftly last time :)
Uwe, it seems that the Asus P4B266 is yours, the code was committed as r247. Can you send in an lspci -vvnnxxx to the list as well?
Carl-Daniel, the Dell is from your commit, r728. Any lspci on that?
David, i would guess that the MSI MS-7046 is your board (original commit r414), can you go and verify this code on this hardware? We would like to have an lspci -vvnnxxx for this board too.
Stepan, it seems that you committed the kontron 986LCD-M board enable. Can you check whether this code still works with your board, and can you also provide an lspci -vvnnxxx? I would also like to get rid of the entry which just matches one pci devices, and the coreboot ids in the table.
Richie, can you give this code a spin on your Abit IP35? (original commit r642)
Bojan? Asus P4P800-E seems to be yours and it was added just under two months ago in r682. Can you give this code a run too?
I would like to see at least 4 of these boards get an Ack before this can be committed.
Thanks all, now i can send in at least two more board enables which go on top of this stuff.
Luc Verhaegen.
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
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.
On Thu, Oct 22, 2009 at 03:16:16PM +0200, 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.
Yes, I think so too, but see below for one change request.
Will test the patch on ASUS P4B266 soon.
Besides that, duplicating pci_dev_find in every board function strikes me as something that was better in the old code.
Ack.
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; }
I agree we should drop the totally useless 0x48 register index, 0x0c offset, and 0xffc0 bitmask from the function prototype. Also, IMHO "name" is pretty useless, it's on my TODO list to drop that anyway.
However, I very much think that we _do_ want the PCI IDs to stay in there.
Example call would look like this:
ich_gpio_raise(name, 0x8086, 0x27B8, 34);
I'm fine with also dropping 0x8086 (PCI vendor ID) as that should always be the same for the ich_* function. If we later also drop "name" this becomes:
ich_gpio_raise(0x27B8, 34);
Which is perfect.
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;}
This is perfectly fine if you ask me. Many board enables only raise one pin and thus are as simple as:
{ return ich_gpio_raise(name, 0x27B8, 34); }
Which I like a lot. And the above example is very nice and readable,too.
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.
No need to copy any of it around, that's just useless code duplication. Just put them in the ich_gpio_raise() function, they're fine in there.
I'll report back on the P4B266 status when I tested that, but I don't expect breakage, the code looks good!
Thanks, Uwe.
On 22.10.2009 23:44, Uwe Hermann wrote:
On Thu, Oct 22, 2009 at 03:16:16PM +0200, Luc Verhaegen wrote:
I agree we should drop the totally useless 0x48 register index, 0x0c offset, and 0xffc0 bitmask from the function prototype.
Absolutely.
Also, IMHO "name" is pretty useless, it's on my TODO list to drop that anyway.
Indeed.
However, I very much think that we _do_ want the PCI IDs to stay in there.
Example call would look like this:
ich_gpio_raise(name, 0x8086, 0x27B8, 34);
I'm fine with also dropping 0x8086 (PCI vendor ID) as that should always be the same for the ich_* function. If we later also drop "name" this becomes:
ich_gpio_raise(0x27B8, 34);
Which is perfect.
Hm yes. It is a bit ugly in the case of multiple GPIOs per board, but other than that... please see my mail from a few minutes ago for more commentary.
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;}
This is perfectly fine if you ask me. Many board enables only raise one pin and thus are as simple as:
{ return ich_gpio_raise(name, 0x27B8, 34); }
Which I like a lot. And the above example is very nice and readable,too.
Well yes.
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.
No need to copy any of it around, that's just useless code duplication. Just put them in the ich_gpio_raise() function, they're fine in there.
We should strive to keep the board enables 1. short and 2. easy to understand.
The old code clearly fails #2, while some of the new proposals fail #1.
Apart from that, this discussion really helps us define our goals for the future.
Regards, Carl-Daniel
On Fri, Oct 23, 2009 at 01:46:52AM +0200, Carl-Daniel Hailfinger wrote:
On 22.10.2009 23:44, Uwe Hermann wrote:
On Thu, Oct 22, 2009 at 03:16:16PM +0200, Luc Verhaegen wrote:
I agree we should drop the totally useless 0x48 register index, 0x0c offset, and 0xffc0 bitmask from the function prototype.
Absolutely.
Also, IMHO "name" is pretty useless, it's on my TODO list to drop that anyway.
Indeed.
However, I very much think that we _do_ want the PCI IDs to stay in there.
Example call would look like this:
ich_gpio_raise(name, 0x8086, 0x27B8, 34);
I'm fine with also dropping 0x8086 (PCI vendor ID) as that should always be the same for the ich_* function. If we later also drop "name" this becomes:
ich_gpio_raise(0x27B8, 34);
Which is perfect.
Hm yes. It is a bit ugly in the case of multiple GPIOs per board, but other than that... please see my mail from a few minutes ago for more commentary.
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;}
This is perfectly fine if you ask me. Many board enables only raise one pin and thus are as simple as:
{ return ich_gpio_raise(name, 0x27B8, 34); }
Which I like a lot. And the above example is very nice and readable,too.
Well yes.
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.
No need to copy any of it around, that's just useless code duplication. Just put them in the ich_gpio_raise() function, they're fine in there.
We should strive to keep the board enables
- short and
- easy to understand.
The old code clearly fails #2, while some of the new proposals fail #1.
Apart from that, this discussion really helps us define our goals for the future.
Regards, Carl-Daniel
As said before, do not abstract away too much, then people will end up working around the abstraction later on when they find they need to.
The dev finding can be improved, but this is not the sort of improvement that should alter the behaviour of the gpio setting itself, and therefor is of less importance than the harder to test things this patch does.
Luc Verhaegen.
On Thu, Oct 22, 2009 at 11:44:24PM +0200, Uwe Hermann wrote:
On Thu, Oct 22, 2009 at 03:16:16PM +0200, 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.
Yes, I think so too, but see below for one change request.
Will test the patch on ASUS P4B266 soon.
Besides that, duplicating pci_dev_find in every board function strikes me as something that was better in the old code.
Ack.
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; }
I agree we should drop the totally useless 0x48 register index, 0x0c offset, and 0xffc0 bitmask from the function prototype. Also, IMHO "name" is pretty useless, it's on my TODO list to drop that anyway.
However, I very much think that we _do_ want the PCI IDs to stay in there.
Example call would look like this:
ich_gpio_raise(name, 0x8086, 0x27B8, 34);
I'm fine with also dropping 0x8086 (PCI vendor ID) as that should always be the same for the ich_* function. If we later also drop "name" this becomes:
ich_gpio_raise(0x27B8, 34);
Which is perfect.
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;}
This is perfectly fine if you ask me. Many board enables only raise one pin and thus are as simple as:
{ return ich_gpio_raise(name, 0x27B8, 34); }
Which I like a lot. And the above example is very nice and readable,too.
People will naturally add a comment here with the chipset name. If they don't, then people who come in afterwards will want to see such a comment.
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.
No need to copy any of it around, that's just useless code duplication. Just put them in the ich_gpio_raise() function, they're fine in there.
I do not agree.
One issue with the "stick the discovery of the pci device in the gpio set routine" is the multiple gpio pin poking. Another is when you need to touch some other bits of that same pci device.
It is also the pci device finding which can fail on a badly matched board here (or some programming issue which we should not encounter if we have board enables tested). So intel_ich_gpio_raise itself, later on, should not fail anymore.
Getting the dev, and then working from there is atomic enough, without overly abstracting or keeping things.
Also, retrieving the dev, can be made a tiny bit cleaner, at a later stage, which will then be uniform for all, even the non-ich board enables. Then you really can apply a more general formula.
If you no longer have to pass the name, then you have the situation where people put comments in there which state the name, so that is not an improvement either.
Let's get the bugs out of this patch, and then worry about the looks of it in further patches.
Luc Verhaegen.
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
On 23.10.2009 01:41, Carl-Daniel Hailfinger wrote:
On 22.10.2009 15:16, Luc Verhaegen wrote:
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?
[...] 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.
One more thought: Instead of writing the whole tuple without checking the old value, can we add some debugging instead? I.e. printf_debug("Changing signal to GPIO") if the signal was previously set to an alternate function, and being silent in the case where everything is already set up correctly?
Regards, Carl-Daniel
On Fri, Oct 23, 2009 at 02:42:16AM +0200, Carl-Daniel Hailfinger wrote:
On 23.10.2009 01:41, Carl-Daniel Hailfinger wrote:
On 22.10.2009 15:16, Luc Verhaegen wrote:
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?
[...] 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.
One more thought: Instead of writing the whole tuple without checking the old value, can we add some debugging instead? I.e. printf_debug("Changing signal to GPIO") if the signal was previously set to an alternate function, and being silent in the case where everything is already set up correctly?
Regards, Carl-Daniel
Let's do this afterwards, like some of the other changes suggested which i will go through now.
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.
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@gmx.net
Could you repost the patch so everyone knows the final state? Thanks.
Regards, Carl-Daniel
On Fri, Oct 23, 2009 at 06:49:18PM +0200, Carl-Daniel Hailfinger wrote:
On 23.10.2009 16:27, Luc Verhaegen wrote:
On Fri, Oct 23, 2009 at 01:41:41AM +0200, Carl-Daniel Hailfinger wrote:
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.
Yeah, we can do away with this duplication by turning that into one function which takes the name as the argument.
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.
Yeah, this way it is at least in the same shape as the other functions, and this refactoring (of all) can be done later without much danger, the touching of extra gpio bits now is much more likely to break things.
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.
Done.
If we get reports from our testers that everything is fine indeed, I'm OK with either byte or word or dword access.
Wait and see. I will try to get this one board enable resent, on top of this patch here. And will get idwer his board enable out (finally). This is two more testcases.
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.
Right, solve it when it occurs and when we know what it is exactly that we need to do.
If it works, no objection from my side.
So then we are just waiting for testers ;)
- 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."
Done.
Latest patch attached, difference is just those two comments and the intel_ prependage (and the fact that i now went to git-svn so i can juggle the patches more easily). INL/OUTL was kept while waiting for testing.
Luc Verhaegen.
On 24.10.2009 00:42, Luc Verhaegen wrote:
On Fri, Oct 23, 2009 at 06:49:18PM +0200, Carl-Daniel Hailfinger wrote:
On 23.10.2009 16:27, Luc Verhaegen wrote:
On Fri, Oct 23, 2009 at 01:41:41AM +0200, Carl-Daniel Hailfinger wrote:
Yes, but the way you suggested keeps the description around in a comment and an error message. That's duplication.
Yeah, we can do away with this duplication by turning that into one function which takes the name as the argument.
If we're trying to rebuild the device name printing of lspci, we might as well have a function which looks up the name.
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.
Yeah, this way it is at least in the same shape as the other functions, and this refactoring (of all) can be done later without much danger, the touching of extra gpio bits now is much more likely to break things.
Right.
If we get reports from our testers that everything is fine indeed, I'm OK with either byte or word or dword access.
Wait and see. I will try to get this one board enable resent, on top of this patch here. And will get idwer his board enable out (finally). This is two more testcases.
Good.
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.
Right, solve it when it occurs and when we know what it is exactly that we need to do.
Just wanted to mention it. We'll probably deal with this later.
- part of the dell comment restored.
Done.
Thanks.
Latest patch attached, difference is just those two comments and the intel_ prependage (and the fact that i now went to git-svn so i can juggle the patches more easily). INL/OUTL was kept while waiting for testing.
I have one more comment, but feel free to address this in a followup patch: You're not checking the GPIO number against the upper limit of available GPIOs per chipset. This can lead to unpleasant accidents if someone specifies a GPIO bitmask instead of a GPIO number (like it happened ~2 weeks ago).
If you're going to send a followup with the GPIO number checking mentioned above, this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
2009/10/24 Luc Verhaegen libv@skynet.be
On Fri, Oct 23, 2009 at 06:49:18PM +0200, Carl-Daniel Hailfinger wrote:
On 23.10.2009 16:27, Luc Verhaegen wrote:
On Fri, Oct 23, 2009 at 01:41:41AM +0200, Carl-Daniel Hailfinger wrote:
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.
Yeah, we can do away with this duplication by turning that into one function which takes the name as the argument.
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.
Yeah, this way it is at least in the same shape as the other functions, and this refactoring (of all) can be done later without much danger, the touching of extra gpio bits now is much more likely to break things.
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.
Done.
If we get reports from our testers that everything is fine indeed, I'm OK with either byte or word or dword access.
Wait and see. I will try to get this one board enable resent, on top of this patch here. And will get idwer his board enable out (finally). This is two more testcases.
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.
Right, solve it when it occurs and when we know what it is exactly that we need to do.
If it works, no objection from my side.
So then we are just waiting for testers ;)
- 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."
Done.
Acked-by: Idwer Vollering vidwer@gmail.com
Latest patch attached, difference is just those two comments and the intel_ prependage (and the fact that i now went to git-svn so i can juggle the patches more easily). INL/OUTL was kept while waiting for testing.
Luc Verhaegen.
flashrom mailing list flashrom@flashrom.org http://www.flashrom.org/mailman/listinfo/flashrom
On Sat, Oct 24, 2009 at 12:42:21AM +0200, Luc Verhaegen wrote:
Let's use your original pci_dev_find variant and refactor it later.
Yep.
Acked-by: Uwe Hermann uwe@hermann-uwe.de
See below for smaller fixes though.
/**
- Set the specified GPIO on the specified ICHx southbridge to high.
- Raises a GPIO line on intel ICH, ICH0 and ICH2 through ICH10 southbridges.
s/intel/Intel/
- In hardware, the GPIO BARs have the lower bits (5:1) zeroed, starting
- with a mobile ich9 version bits 6:1 are zeroed. The 0xFFC0 mask should
s/ich9/ICH9/
+/**
- Suited for ASUS P4B266: socket478 + intel 845D + ICH2.
s/intel/Intel/
+/**
- Suited for Acorp 6A815EPD: socket 370 + intel 815 + ICH2.
s/intel/Intel/
*/
- Suited for Asus P4P800-E Deluxe: Intel socket478 + 865PE + ICH5R.
-static int ich2_gpio22_raise(const char *name) +static int board_asus_p4p800(const char *name) {
- return ich_gpio_raise(name, 0x8086, 0x2440, 0x58, 0x0c, 0xffc0, 22);
- struct pci_dev *dev;
- dev = pci_dev_find(0x8086, 0x24D0); /* Intel ICH5R ISA Bridge */
- if (!dev) {
fprintf(stderr, "\nERROR: Intel ICH5 ISA Bridge not found.\n");
One comment says ICH5R, one says ICH5.
/**
- Suited for the Dell PowerEdge 1850. All parameters except the last one are
- documented in the public Intel 82801EB ICH5 / 82801ER ICH5R datasheet. The
- last parameter (GPIO number) has to be in the range [16,31] according to
- said Intel datasheet and was found by exhaustive search.
- Suited for Dell Poweredge 1850: Intel PPGA604 + E7520 + ICH5R.
s/Poweredge/PowerEdge/ as per vendor website.
- Suited for MSI MS-7046.
*/
- Suited for MSI MS-7046: LGA775 + 915P + ICH6.
-static int ich6_gpio19_raise(const char *name) +static int board_msi_ms7046(const char *name) {
- return ich_gpio_raise(name, 0x8086, 0x2640, 0x48, 0x0c, 0xffc0, 19);
- struct pci_dev *dev;
- dev = pci_dev_find(0x8086, 0x2640); /* Intel ICH6 LPC Interface */
- if (!dev) {
fprintf(stderr, "\nERROR: Intel ICH9R LPC not found.\n");
One comment says ICH6, the other ICH9R. One of them is wrong.
Uwe.
On Sun, Oct 25, 2009 at 02:50:03AM +0200, Uwe Hermann wrote:
On Sat, Oct 24, 2009 at 12:42:21AM +0200, Luc Verhaegen wrote:
Let's use your original pci_dev_find variant and refactor it later.
Yep.
Acked-by: Uwe Hermann uwe@hermann-uwe.de
See below for smaller fixes though.
/**
- Set the specified GPIO on the specified ICHx southbridge to high.
- Raises a GPIO line on intel ICH, ICH0 and ICH2 through ICH10 southbridges.
s/intel/Intel/
- In hardware, the GPIO BARs have the lower bits (5:1) zeroed, starting
- with a mobile ich9 version bits 6:1 are zeroed. The 0xFFC0 mask should
s/ich9/ICH9/
+/**
- Suited for ASUS P4B266: socket478 + intel 845D + ICH2.
s/intel/Intel/
+/**
- Suited for Acorp 6A815EPD: socket 370 + intel 815 + ICH2.
s/intel/Intel/
*/
- Suited for Asus P4P800-E Deluxe: Intel socket478 + 865PE + ICH5R.
-static int ich2_gpio22_raise(const char *name) +static int board_asus_p4p800(const char *name) {
- return ich_gpio_raise(name, 0x8086, 0x2440, 0x58, 0x0c, 0xffc0, 22);
- struct pci_dev *dev;
- dev = pci_dev_find(0x8086, 0x24D0); /* Intel ICH5R ISA Bridge */
- if (!dev) {
fprintf(stderr, "\nERROR: Intel ICH5 ISA Bridge not found.\n");One comment says ICH5R, one says ICH5.
/**
- Suited for the Dell PowerEdge 1850. All parameters except the last one are
- documented in the public Intel 82801EB ICH5 / 82801ER ICH5R datasheet. The
- last parameter (GPIO number) has to be in the range [16,31] according to
- said Intel datasheet and was found by exhaustive search.
- Suited for Dell Poweredge 1850: Intel PPGA604 + E7520 + ICH5R.
s/Poweredge/PowerEdge/ as per vendor website.
- Suited for MSI MS-7046.
*/
- Suited for MSI MS-7046: LGA775 + 915P + ICH6.
-static int ich6_gpio19_raise(const char *name) +static int board_msi_ms7046(const char *name) {
- return ich_gpio_raise(name, 0x8086, 0x2640, 0x48, 0x0c, 0xffc0, 19);
- struct pci_dev *dev;
- dev = pci_dev_find(0x8086, 0x2640); /* Intel ICH6 LPC Interface */
- if (!dev) {
fprintf(stderr, "\nERROR: Intel ICH9R LPC not found.\n");One comment says ICH6, the other ICH9R. One of them is wrong.
Uwe.
Oh please.
Luc Verhaegen.
On 25.10.2009 03:48, Luc Verhaegen wrote:
On Sun, Oct 25, 2009 at 02:50:03AM +0200, Uwe Hermann wrote:
On Sat, Oct 24, 2009 at 12:42:21AM +0200, Luc Verhaegen wrote:
- return ich_gpio_raise(name, 0x8086, 0x2640, 0x48, 0x0c, 0xffc0, 19);
- struct pci_dev *dev;
- dev = pci_dev_find(0x8086, 0x2640); /* Intel ICH6 LPC Interface */
- if (!dev) {
fprintf(stderr, "\nERROR: Intel ICH9R LPC not found.\n");One comment says ICH6, the other ICH9R. One of them is wrong.
Oh please.
Actually, this is the reason why I didn't want chipset names in board enable functions. It's so easy to get them wrong when using cut-n-paste.
Regards, Carl-Daniel
On Sun, Oct 25, 2009 at 11:59:53AM +0100, Carl-Daniel Hailfinger wrote:
On 25.10.2009 03:48, Luc Verhaegen wrote:
On Sun, Oct 25, 2009 at 02:50:03AM +0200, Uwe Hermann wrote:
On Sat, Oct 24, 2009 at 12:42:21AM +0200, Luc Verhaegen wrote:
- return ich_gpio_raise(name, 0x8086, 0x2640, 0x48, 0x0c, 0xffc0, 19);
- struct pci_dev *dev;
- dev = pci_dev_find(0x8086, 0x2640); /* Intel ICH6 LPC Interface */
- if (!dev) {
fprintf(stderr, "\nERROR: Intel ICH9R LPC not found.\n");One comment says ICH6, the other ICH9R. One of them is wrong.
Oh please.
Actually, this is the reason why I didn't want chipset names in board enable functions. It's so easy to get them wrong when using cut-n-paste.
Regards, Carl-Daniel
It's the fact that only the last one is worth actually caring for that is bothering me here. I really can not believe that capitalisation is something anyone can care for like this.
I prefer to do real work.
Luc Verhaegen.
-
Carl-Daniel Hailfinger -
Idwer Vollering -
Luc Verhaegen -
Uwe Hermann