[LinuxBIOS] [PATCH] flashrom patch for iwill dk8_htx
Luc Verhaegen
libv at skynet.be
Thu May 3 15:29:33 CEST 2007
There are some serious problems with this code. And things only become
transparent when you introduce those helper functions.
This is what your function is reduced to:
/*
* Disable the flash write protect (which is connected to the
* Winbond W83627HF GPIOs).
*
* There are some severe discrepancies in this code!
*/
static int board_iwill_dk8_htx(const char *name)
{
w836xx_ext_enter();
/* set pins to SCL or GPIO. */
wbsio_mask(0x2B, 0xD0, 0xD0); /* GPIO 21,22,24 */
wbsio_write(0x07, 0x08); /* select logical device 8: GPIO port 2. */
wbsio_mask(0x30, 0x01, 0x01); /* Activate logical device */
wbsio_mask(0xF0, 0xEF, 0xEF); /* GPIO 20,21,22,23,25,26,27 = input */
wbsio_mask(0xF1, 0x16, 0x16); /* Raise GPIO 21,22,24 */
#if 0
wbsio_mask(0xF2, 0x00, 0x00); /* GPIO inversion reg */
#endif
w836xx_ext_leave();
return 0;
}
Let me run down through things one by one.
First of all, which GPIO pin is the culprit here? Which one needs to be
raised/dropped for your rom to be writeable? Do you know this? Or are
you not sure which one it is.
Secondly, you are using | <value> all the time on all but one register
access. Clearly this is not what is wanted in all cases and it leads to
some pretty crappy results when using the helper functions.
Let me step through the code:
/* set pins to SCL or GPIO. */
wbsio_mask(0x2B, 0xD0, 0xD0); /* GPIO 21,22,24 */
This was moved up. You don't need to select a logical device for this to
work. Aruma code was correct here. But which GPIO is it?
wbsio_write(0x07, 0x08); /* select logical device 8: GPIO port 2. */
This is fully correct.
wbsio_mask(0x30, 0x01, 0x01); /* Activate logical device */
This is correct, but could've just as well been a write. I prefer this
over what the aruma code uses as it clearly marks this as a single bit
to be raised.
wbsio_mask(0xF0, 0xEF, 0xEF); /* GPIO 20,21,22,23,25,26,27 = input */
It seems as if you wanted to set GPIO24 as an output. But instead you
did | 0xEF. Which is quite the opposite of & 0xEF or & ~0x10.
This line should be replaced by:
wbsio_mask(0xF0, 0x00, 0x10); /* GPIO24 = output */
So is your gpio line GPIO24 like the aruma?
Next line:
wbsio_mask(0xF1, 0x16, 0x16); /* Raise GPIO 21,22,24 */
Which one is it?
And this:
#if 0
wbsio_mask(0xF2, 0x00, 0x00); /* GPIO inversion reg */
#endif
This was the conversion of the useless | 0x00 you did there. Were you
trying to do this:
wbsio_write(0xF2, 0x00, 0x10); /* Clear GPIO24 inversion */
All in all, the way this patch was handled was very very poor. We've all
been talking about formatting yet no-one bothered to look at what
actually was going on (and i too acked this code).
This is not how code should get handled.
I've attached the full patch here. Except for the ordering of the 0x2B
write, this is a direct translation, with just some comments added.
I will look into writing
static int W83627HFGPIORaise(int Port);
which handles the raising of GPIO20-27 on this very superIO.
This would reduce the two relevant board enables and stop mistakes like
this from happening ever again.
Luc Verhaegen.
-------------- next part --------------
Add Winbond SuperIO helper functions to clear up the board enables.
Also reveals some nastyness in the iwill dk8 htx, which is to be rectified
in one of the subsequent commits.
Properly constify the argument of all board enables: otherwise the
compiler bombs out here (warning is treated like error in this Makefile).
Signed-Off-By: Luc Verhaegen <libv at skynet.be>
Index: board_enable.c
===================================================================
--- board_enable.c (revision 2624)
+++ board_enable.c (working copy)
@@ -23,53 +23,78 @@
#include "flash.h"
#include "debug.h"
-static int board_iwill_dk8_htx(const char *name)
+/*
+ * Helper functions for many Winbond superIOs of the w836xx range.
+ */
+#define W836_INDEX 0x2E
+#define W836_DATA 0x2F
+
+/* Enter extended functions */
+static void
+w836xx_ext_enter(void)
{
- /* Extended function index register, either 0x2e or 0x4e. */
-#define EFIR 0x2e
- /* Extended function data register, one plus the index reg. */
-#define EFDR EFIR + 1
- char b;
+ outb(0x87, W836_INDEX);
+ outb(0x87, W836_INDEX);
+}
- /* Disable the flash write protect (which is connected to the
- * Winbond W83627HF GPIOs).
- */
- outb(0x87, EFIR); /* Sequence to unlock extended functions */
- outb(0x87, EFIR);
+/* Leave extended functions */
+static void
+w836xx_ext_leave(void)
+{
+ outb(0xAA, W836_INDEX);
+}
- /* Activate logical device. */
- outb(0x7, EFIR);
- outb(8, EFDR);
+/* General functions for read/writing WB SuperIOs */
+static unsigned char
+wbsio_read(unsigned char index)
+{
+ outb(index, W836_INDEX);
+ return inb(W836_DATA);
+}
- /* Set GPIO regs. */
- outb(0x2b, EFIR); /* GPIO multiplexed pin reg. */
- b = inb(EFDR) | 0xd0;
- outb(0x2b, EFIR);
- outb(b, EFDR);
+static void
+wbsio_write(unsigned char index, unsigned char data)
+{
+ outb(index, W836_INDEX);
+ outb(data, W836_DATA);
+}
- outb(0x30, EFIR); /* GPIO2 */
- b = inb(EFDR) | 0x01;
- outb(0x30, EFIR);
- outb(b, EFDR);
+static void
+wbsio_mask(unsigned char index, unsigned char data,
+ unsigned char mask)
+{
+ unsigned char tmp;
- outb(0xf0, EFIR); /* IO sel */
- b = inb(EFDR) | 0xef;
- outb(0xf0, EFIR);
- outb(b, EFDR);
+ outb(index, W836_INDEX);
+ tmp = inb(W836_DATA) & ~mask;
+ outb(tmp | (data & mask), W836_DATA);
+}
- outb(0xf1, EFIR); /* GPIO data reg */
- b = inb(EFDR) | 0x16;
- outb(0xf1, EFIR);
- outb(b, EFDR);
+/*
+ * Disable the flash write protect (which is connected to the
+ * Winbond W83627HF GPIOs).
+ */
+static int board_iwill_dk8_htx(const char *name)
+{
+ w836xx_ext_enter();
- outb(0xf2, EFIR); /* GPIO inversion reg */
- b = inb(EFDR) | 0x00;
- outb(0xf2, EFIR);
- outb(b, EFDR);
+ /* set to SCL or GPIO. */
+ wbsio_mask(0x2B, 0xD0, 0xD0); /* GPIO 21,22,24 */
- /* Lock extended functions again. */
- outb(0xaa, EFIR); /* Command to exit extended functions */
+ wbsio_write(0x07, 0x08); /* Activate logical device 8: GPIO port 2. */
+ wbsio_mask(0x30, 0x01, 0x01); /* Activate logical device */
+
+ wbsio_mask(0xF0, 0xEF, 0xEF); /* GPIO 20,21,22,23,25,26,27 = input */
+
+ wbsio_mask(0xF1, 0x16, 0x16); /* Raise GPIO 21,22,24 */
+
+#if 0 /* Surely this is wrong */
+ wbsio_mask(0xF2, 0x00, 0x00); /* GPIO inversion reg */
+#endif
+
+ w836xx_ext_leave();
+
return 0;
}
@@ -78,53 +103,36 @@
* specific code:
* main: 0x1022:0x746B, which is the SMBUS controller.
* card: 0x1022:0x36C0...
+ *
+ * Flash write protect is connected to GPIO24 of the WinBond w83627hf.
*/
-
-static int board_agami_aruma(char *name)
+static int board_agami_aruma(const char *name)
{
- /* Extended function index register, either 0x2e or 0x4e */
-#define EFIR 0x2e
- /* Extended function data register, one plus the index reg. */
-#define EFDR EFIR + 1
- char b;
+ w836xx_ext_enter();
- /* Disable the flash write protect. The flash write protect is
- * connected to the WinBond w83627hf GPIO 24.
- */
- outb(0x87, EFIR); /* sequence to unlock extended functions */
- outb(0x87, EFIR);
-
- outb(0x20, EFIR); /* SIO device ID register */
- b = inb(EFDR);
- printf_debug("\nW83627HF device ID = 0x%x\n",b);
-
- if (b != 0x52) {
- fprintf(stderr, "\nIncorrect device ID, aborting write protect disable\n");
+ if (wbsio_read(0x20) != 0x52) { /* SIO device ID register */
+ fprintf(stderr, "\n%s: Incorrect Winbond device ID (0x%02X),"
+ " aborting.\n", name, wbsio_read(0x20));
+ w836xx_ext_leave();
return -1;
}
- outb(0x2b, EFIR); /* GPIO multiplexed pin reg. */
- b = inb(EFDR) | 0x10;
- outb(0x2b, EFIR);
- outb(b, EFDR); /* select GPIO 24 instead of WDTO */
+ /* GPIO multiplexed pin reg: select GPIO24 instead of WDTO */
+ wbsio_mask(0x2B, 0x10, 0x10);
- outb(0x7, EFIR); /* logical device select */
- outb(0x8, EFDR); /* point to device 8, GPIO port 2 */
+ /* logical device select: point to device 8, GPIO port 2 */
+ wbsio_write(0x07, 0x08);
- outb(0x30, EFIR); /* logic device activation control */
- outb(0x1, EFDR); /* activate */
+ /* logic device activation control: activate */
+ wbsio_write(0x30, 0x01);
- outb(0xf0, EFIR); /* GPIO 20-27 I/O selection register */
- b = inb(EFDR) & ~0x10;
- outb(0xf0, EFIR);
- outb(b, EFDR); /* set GPIO 24 as an output */
+ /* GPIO 20-27 I/O selection register: GPIO24 = output */
+ wbsio_mask(0xF0, 0x00, 0x10);
- outb(0xf1, EFIR); /* GPIO 20-27 data register */
- b = inb(EFDR) | 0x10;
- outb(0xf1, EFIR);
- outb(b, EFDR); /* set GPIO 24 */
+ /* GPIO 20-27 data register: set GPIO24 */
+ wbsio_mask(0xF1, 0x10, 0x10);
- outb(0xaa, EFIR); /* command to exit extended functions */
+ w836xx_ext_leave();
return 0;
}
@@ -135,7 +143,7 @@
* We don't need to do this when using linuxbios, GPIO15 is never lowered there.
*/
-static int board_via_epia_m(char *name)
+static int board_via_epia_m(const char *name)
{
struct pci_dev *dev;
unsigned int base;
@@ -164,33 +172,11 @@
}
/*
- * Winbond LPC super IO.
- *
- * Raises the ROM MEMW# line.
- */
-
-static void w83697_rom_memw_enable(void)
-{
- uint8_t val;
-
- outb(0x87, 0x2E); /* enable extended functions */
- outb(0x87, 0x2E);
-
- outb(0x24, 0x2E); /* rom bits live here */
-
- val = inb(0x2F);
- if (!(val & 0x02)) /* flash rom enabled? */
- outb(val | 0x08, 0x2F); /* enable MEMW# */
-
- outb(0xAA, 0x2E); /* disable extended functions */
-}
-
-/*
* Suited for Asus A7V8X-MX SE and A7V400-MX.
*
*/
-static int board_asus_a7v8x_mx(char *name)
+static int board_asus_a7v8x_mx(const char *name)
{
struct pci_dev *dev;
uint8_t val;
@@ -206,8 +192,14 @@
val &= 0x7F;
pci_write_byte(dev, 0x59, val);
- w83697_rom_memw_enable();
+ /* Raise ROM MEMW# line on Winbond w83697 SuperIO */
+ w836xx_ext_enter();
+ if (!(wbsio_read(0x24) & 0x02)) /* flash rom enabled? */
+ wbsio_mask(0x24, 0x08, 0x08); /* enable MEMW# */
+
+ w836xx_ext_leave();
+
return 0;
}
@@ -241,7 +233,7 @@
char *lb_part;
char *name;
- int (*enable)(char *name);
+ int (*enable)(const char *name);
};
struct board_pciid_enable board_pciid_enables[] = {
More information about the coreboot
mailing list