Hi,
I'm not a skilled programmer so this patch may look rather ugly. (Suggestions to improve it are very welcome.)
Anyways, this changes the intel_piix4_gpo_set() function to always check the GENCFG and XBCS register for the availability of the requested GPO line (unless called with safe=0) before raising/lowering it. This behaviour was suggested by Michael Karcher on the flashrom mailing list and this is an attempt implementing it.
Also adds a board enable for Asus P2B-N. Probe/Read/Write/Erease tested and works with this board.
Signed-off-by: Mattias Mattsson vitplister@gmail.com
Index: board_enable.c =================================================================== --- board_enable.c (revision 1091) +++ board_enable.c (working copy) @@ -947,13 +947,67 @@ }
/** + * Helper function to check if a given gpo line is available on Intel PIIX4{,E,M}. + */ +static int intel_piix4_gpo_available(unsigned int gpo, struct pci_dev *pci_dev) +{ + + uint32_t gencfg; + uint16_t xbcs; + + gencfg = pci_read_long(pci_dev, 0xB0); /* GENCFG */ + xbcs = pci_read_word(pci_dev, 0x4E); /* XBCS */ + + if ((gpo == 0)) { + return 1; /* GPO0 is always available */ + + } else if ((gpo >= 1) && (gpo <= 7)) { + return ((gencfg & 1) == 0); /* GENCFG bit 0 */ + + } else if ((gpo == 8)) { + return -1; /* GPO8 is never available */ + + } else if ((gpo >= 9) && (gpo <= 11)) { + return ((gencfg & (1 << (gpo - 1))) == 0); /* GENCFG bit 8-10 */ + + } else if ((gpo >= 12) && (gpo <= 14)) { + return ((xbcs & (1 << 8)) == 0); /* XBCS bit 8 */ + + } else if ((gpo >= 15) && (gpo <= 16)) { + return ((gencfg & (1 << 17)) == 1); /* GENCFG bit 17 */ + + } else if ((gpo >= 17) && (gpo <= 21)) { + return ((gencfg & (1 << (gpo + 1))) == 1); /* GENCFG bit 18-22 */ + + } else if ((gpo >= 22) && (gpo <= 23)) { + return ((gencfg & (1 << 28)) == 1); /* GENCFG bit 28 */ + + } else if ((gpo >= 24) && (gpo <= 26)) { + return ((gencfg & (1 << (gpo + 5))) == 1); /* GENCFG bit 29-31 */ + + } else if ((gpo >= 27) && (gpo <= 28)) { + return 1; /* GPO27-28 is always available */ + + } else if ((gpo == 29)) { + return ((xbcs & (1 << 8)) == 0); /* XBCS bit 8 */ + + } else if ((gpo == 30)) { + return 1; /* GPO30 is always available */ + + } else { + return -1; + } +} + +/** * Helper function to raise/drop a given gpo line on Intel PIIX4{,E,M}. */ -static int intel_piix4_gpo_set(unsigned int gpo, int raise) +static int intel_piix4_gpo_set(unsigned int gpo, int raise, int safe) { unsigned int gpo_byte, gpo_bit; struct pci_dev *dev; uint32_t tmp, base; + uint16_t tmp2;
dev = pci_dev_find(0x8086, 0x7110); /* Intel PIIX4 ISA bridge */ if (!dev) { @@ -967,36 +1021,86 @@ return -1; }
- /* these are dual function pins which are most likely in use already */ - if (((gpo >= 1) && (gpo <= 7)) || - ((gpo >= 9) && (gpo <= 21)) || (gpo == 29)) { - msg_perr("\nERROR: Unsupported PIIX4 GPO%d.\n", gpo); - return -1; - } + if (safe) { + if (intel_piix4_gpo_available(gpo, dev) == -1) { + msg_perr("\nERROR: Intel PIIX4 GPO%d not available.\n", gpo); + return -1; + } + } else { + /* dual function that need special enable. */
- /* dual function that need special enable. */ - if ((gpo >= 22) && (gpo <= 26)) { - tmp = pci_read_long(dev, 0xB0); /* GENCFG */ + tmp = pci_read_long(dev, 0xB0); /* GENCFG */ + tmp2 = pci_read_word(dev, 0x4E); /* XBCS */ switch (gpo) { - case 22: /* XBUS: XDIR#/GPO22 */ - case 23: /* XBUS: XOE#/GPO23 */ - tmp |= 1 << 28; + case 0: break; - case 24: /* RTCSS#/GPO24 */ - tmp |= 1 << 29; + case 1: + case 2: + case 3: + case 4: + case 5: + case 6: + case 7: + tmp &= ~(1 << 0); /* GENCFG bit 0: 0 = GPI0+GPO1-7 / 1 = IOCHK#+LA17-23 */ break; - case 25: /* RTCALE/GPO25 */ - tmp |= 1 << 30; + case 8: break; - case 26: /* KBCSS#/GPO26 */ - tmp |= 1 << 31; + case 9: + tmp &= ~(1 << 8); /* GENCFG bit 8: 0 = GPI2+GPO9 / 1 = PC/PCI REQA+GNTA */ break; - } - pci_write_long(dev, 0xB0, tmp); - } + case 10: + tmp &= ~(1 << 9); /* GENCFG bit 9: 0 = GPI3+GPO10 / 1 = PC/PCI REQB+GNTB */ + break; + case 11: + tmp &= ~(1 << 10); /* GENCFG bit 10: 0 = GPI4+GPO11 / 1 = PC/PCI REQC+GNTC */ + break; + case 12: + case 13: + case 14: + case 29: + tmp2 &= ~(1 << 8); /* XBCS bit 8: 0 = GPI5+GPO12-14+GPO29 / 1 = APIC Chip select */ + break; + case 15: + case 16: + tmp |= 1 << 17; /* GENCFG bit 17: 0 = SUSB#+SUSC# / 1 = GPO15+GPO16 */ + break; + case 17: + tmp |= 1 << 18; /* GENCFG bit 18: 0 = CPU_STP# / 1 = GPO17 */ + break; + case 18: + tmp |= 1 << 19; /* GENCFG bit 19: 0 = PCI_STP# / 1 = GPO18 */ + break; + case 19: + tmp |= 1 << 20; /* GENCFG bit 20: 0 = ZZ / 1 = GPO19 */ + break; + case 20: + tmp |= 1 << 21; /* GENCFG bit 21: 0 = SUS_STAT1# / 1 = GPO20 */ + break; + case 21: + tmp |= 1 << 22; /* GENCFG bit 22: 0 = SUS_STAT2# / 1 = GPO21 */ + break; + case 22: + case 23: + tmp |= 1 << 28; /* GENCFG bit 28: 0 = XDIR#+XDIR# / 1 = GPO22+GPO23 */ + break; + case 24: + tmp |= 1 << 29; /* GENCFG bit 29: 0 = RTCSS# / 1 = GPO24 */ + break; + case 25: + tmp |= 1 << 30; /* GENCFG bit 30: 0 = RTCALE / 1 = GPO25 */ + break; + case 26: + tmp |= 1 << 31; /* GENCFG bit 31: 0 = KBCSS# / 1 = GPO26 */ + break; + case 27: + case 28: + case 30: + break; + } + pci_write_long(dev, 0xB0, tmp); + pci_write_word(dev, 0x4E, tmp2); + }
- /* GPO {0,8,27,28,30} are always available. */ - dev = pci_dev_find(0x8086, 0x7113); /* Intel PIIX4 PM */ if (!dev) { msg_perr("\nERROR: Intel PIIX4 PM not found.\n"); @@ -1023,15 +1127,23 @@ */ static int board_epox_ep_bx3(void) { - return intel_piix4_gpo_set(22, 1); + return intel_piix4_gpo_set(22, 1, 1); }
/** + * Suited for Asus P2B-N + */ +static int intel_piix4_gpo18_lower(void) +{ + return intel_piix4_gpo_set(18, 0, 1); +} + +/** * Suited for Intel SE440BX-2 */ static int intel_piix4_gpo27_lower(void) { - return intel_piix4_gpo_set(27, 0); + return intel_piix4_gpo_set(27, 0, 1); }
/** @@ -1672,6 +1784,7 @@ {0x10de, 0x0264, 0x1043, 0x81bc, 0x10de, 0x02f0, 0x1043, 0x81cd, NULL, NULL, NULL, "ASUS", "A8N-VM CSM", 0, NT, w83627ehf_gpio24_raise_2e}, {0x10DE, 0x0264, 0x1043, 0x81C0, 0x10DE, 0x0260, 0x1043, 0x81C0, NULL, NULL, NULL, "ASUS", "M2NBP-VM CSM", 0, OK, nvidia_mcp_gpio0_raise}, {0x1106, 0x1336, 0x1043, 0x80ed, 0x1106, 0x3288, 0x1043, 0x8249, NULL, NULL, NULL, "ASUS", "M2V-MX", 0, OK, via_vt823x_gpio5_raise}, + {0x8086, 0x7190, 0, 0, 0x8086, 0x7191, 0, 0, "^P2B-N$", NULL, NULL, "ASUS", "P2B-N", 0, OK, intel_piix4_gpo18_lower}, {0x8086, 0x1A30, 0x1043, 0x8025, 0x8086, 0x244B, 0x104D, 0x80F0, NULL, NULL, NULL, "ASUS", "P4B266-LM", 0, OK, intel_ich_gpio21_raise}, {0x8086, 0x1a30, 0x1043, 0x8070, 0x8086, 0x244b, 0x1043, 0x8028, NULL, NULL, NULL, "ASUS", "P4B266", 0, OK, intel_ich_gpio22_raise}, {0x8086, 0x1A30, 0x1043, 0x8088, 0x8086, 0x24C3, 0x1043, 0x8089, NULL, NULL, NULL, "ASUS", "P4B533-E", 0, NT, intel_ich_gpio22_raise},
Am Sonntag, den 18.07.2010, 06:15 +0200 schrieb Mattias Mattsson:
Hi,
I'm not a skilled programmer so this patch may look rather ugly. (Suggestions to improve it are very welcome.)
I think using a table-based approach instead of a switch-based approach is sensible. Something like
static const unsigned int forbidden_gpios = 0x00000100; /* never available */ static const unsigned int nonmuxed_gpios = 0x58000001; /* always available */ static const struct { unsigned int reg, bitmask, bitvalue } piix4_gpio = {0,0}, {0xB0, 0x0001, 0x0000}, /* GPO1... */ {0xB0, 0x0001, 0x0000}, {0xB0, 0x0001, 0x0000}, {0xB0, 0x0001, 0x0000}, {0xB0, 0x0001, 0x0000}, {0xB0, 0x0001, 0x0000}, {0xB0, 0x0001, 0x0000}, /* ...GPO7: GENCFG bit 0 (IOCHK#+LA17-23) ... {0xB2, 0x0004, 0x0004}, /* GPO18: GENCFG bit 18 */ ...
Use word reads for GENCFG lower/upper part, just as you use for
This will avoid that you have to put the info about the GPO configuration bits both to the safety-checking procedure and the setting procedure.
Also, your code uses "== 1" where it should use "!= 0".
Regards, Michael Karcher
Many thanks to Michael Karcher for the useful comments. Here is a new try at this patch using the table-based approach as he suggested.
This patch changes the intel_piix4_gpo_set() function to always check the GENCFG and XBCS registers for the availability of the requested GPO line before raising/lowering it and fails otherwise. It makes no attempt to bypass the values in these configuration registers.
The old flashrom code did consider it safe to reprogram (multiplexed) GPO:s 22-26 without checking the value of the controlling register (GENCFG). I do not really know why.
I have tested this patch on an Asus P2B-N (needs GPO18 low) and MSI MS-6163 Pro (needs GPO14 high).
The information for these registers are from the Intel "82371AB PCI-TO-ISA / IDE XCELERATOR (PIIX4)" datasheet available here: http://www.intel.com/design/intarch/datashts/29056201.pdf
Thanks!
Signed-off-by: Mattias Mattsson vitplister@gmail.com
Am Sonntag, den 15.08.2010, 01:00 +0200 schrieb Mattias Mattsson:
This patch changes the intel_piix4_gpo_set() function to always check the GENCFG and XBCS registers for the availability of the requested GPO line before raising/lowering it and fails otherwise. It makes no attempt to bypass the values in these configuration registers.
Please mark all board enables using intel_piix4_gpo_set() untested then, because we don't know whether the reconfiguration to output is needed. It shouldn't be that much.
The old flashrom code did consider it safe to reprogram (multiplexed) GPO:s 22-26 without checking the value of the controlling register (GENCFG). I do not really know why.
Probably because vendor BIOS board enables (which have been reverse engineered to make the flashrom code) uses boilerplate code that makes a pin GPO and sets it at the same time.
Signed-off-by: Mattias Mattsson vitplister@gmail.com
One question remaining: Why do you think GPO8 is "never available". The only special behaviour about GPO8 I could find in the data sheet is that GPO8 has a defined low level in standby mode, but it should work as usual in all other modes. Otherwise the patch looks great. I verified the table against the datasheet, looks correct.
Thanks for your work, Michael Karcher
On Sun, Aug 15, 2010 at 11:37, Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de wrote:
One question remaining: Why do you think GPO8 is "never available". The only special behaviour about GPO8 I could find in the data sheet is that GPO8 has a defined low level in standby mode, but it should work as usual in all other modes.
While experimenting I found that some board immediately powered off when lowering GPO8 (think it was a Dell, unfortunately it's dead now). Therefore I made the assumption that it was unsafe to fiddle with. Today I did some more testing with a modified flashrom. When lowering GPO8 on these boards (all with PIIX4) the following happens:
Asus P2B-N: Nothing Asus P2E-M: Nothing Intel SE440BX: Kernel crash MSI MS-6163 Pro: Nothing
It seems safe in most cases so I decided to just remove the forbidden_gpos variable and mark GPO8 as always available.
Also I marked all board enables using intel_piix4_gpo_set as untested (and renamed the function to the more generic form).
New patch attached
Thanks! -mattias
Am Sonntag, den 15.08.2010, 20:26 +0200 schrieb Mattias Mattsson:
One question remaining: Why do you think GPO8 is "never available". The only special behaviour about GPO8 I could find in the data sheet is that GPO8 has a defined low level in standby mode, but it should work as usual in all other modes.
While experimenting I found that some board immediately powered off when lowering GPO8 (think it was a Dell, unfortunately it's dead now).
Strange behaviour on randomly touching GPIO pins is quite common, don't worry about that. There is a reason we don't run untested board enables by default.
It seems safe in most cases so I decided to just remove the forbidden_gpos variable and mark GPO8 as always available.
Right. Flashrom can't know what GPO8 is wired to on the specific mainboard it runs on, so it has no chance to know whether it should better not touch it.
Also I marked all board enables using intel_piix4_gpo_set as untested (and renamed the function to the more generic form).
Great.
New patch attached
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de and committed as r1142.
Thanks for your contribution!
Regards, Michael Karcher
-
Mattias Mattsson -
Michael Karcher