[LinuxBIOS] [PATCH] Flashrom -- Add W83627THF and K8N-Neo3 support

[David Hendricks]

Hello again everyone,
    Another patch we hacked up today to get flashrom working on the MSI
K8N-Neo3 board. Let us know what you think!

Signed-off-by: David Hendricks

[LinuxBIOS] [PATCH] Flashrom -- Add W83627THF and K8N-Neo3 support

[Uwe Hermann]

Hi,

some comments below:

On Sat, Oct 06, 2007 at 07:15:13PM -0700, David Hendricks wrote:
> Index: board_enable.c
>
> --- board_enable.c	(revision 2763)
> +++ board_enable.c	(working copy)
> @@ -32,43 +32,43 @@
>  /*
> Helper functions for many Winbond Super I/Os of the W836xx range.
>
> */
> -#define W836_INDEX 0x2E
> -#define W836_DATA  0x2F
> +#define W836_INDEX_2E 0x2E
> +#define W836_INDEX_4E 0x4E

Hm, not sure the #define makes much sense anymore if the value is
encoded in the name anyway(?) I think we can drop the #defines
(but the code should still be able to cope with both 0x2e and 0x4e).
> /* Enter extended functions */
> -static void w836xx_ext_enter(void)
> +static void w836xx_ext_enter(unsigned char index)

'unsigned char' -> 'uint16_t'

Maybe also 'index' -> 'port'.
> {
> outb(0x87, W836_INDEX);
> outb(0x87, W836_INDEX);
> outb(0x87, index);
> outb(0x87, index);
>
> }
>
> /* Leave extended functions */
> -static void w836xx_ext_leave(void)
> +static void w836xx_ext_leave(unsigned char index)
>  {
> outb(0xAA, W836_INDEX);
> outb(0xAA, index);
>
> }
>
> /* General functions for reading/writing Winbond Super I/Os. */
> -static unsigned char wbsio_read(unsigned char index)
> +static unsigned char wbsio_read(unsigned char index, unsigned char reg)

index -> uint16_t
reg -> uint8_t
> {
> outb(index, W836_INDEX);
> return inb(W836_DATA);
> outb(reg, index);
> return inb(index+1);
>
> }
>
> -static void wbsio_write(unsigned char index, unsigned char data)
> +static void wbsio_write(unsigned char index, unsigned char reg, unsigned char data)

Same here. data is uint8_t.
> {
> outb(index, W836_INDEX);
> outb(data, W836_DATA);
> outb(reg, index);
> outb(data, index+1);
>
> }
>
> -static void wbsio_mask(unsigned char index, unsigned char data,
> +static void wbsio_mask(unsigned char index, unsigned char reg, unsigned char data,
>    	       unsigned char mask)

Same here.
> {
>    unsigned char tmp;
> outb(index, W836_INDEX);
> tmp = inb(W836_DATA) & ~mask;
> outb(tmp | (data & mask), W836_DATA);
> outb(reg, index);
> tmp = inb(index+1) & ~mask;
> outb(tmp | (data & mask), index+1);
>
> }
>
> /**
> @@ -80,35 +80,69 @@
>   */
>  static int w83627hf_gpio24_raise(const char *name)
>  {
> w836xx_ext_enter();
> w836xx_ext_enter(W836_INDEX_2E);

This won't work if the Super I/O config port is a 0x4e, correct?
Can the code be made generic enough to handle both cases?
> /* Is this the w83627hf? */
> if (wbsio_read(0x20) != 0x52) {	/* SIO device ID register */
> if (wbsio_read(W836_INDEX_2E, 0x20) != 0x52) {	/* SIO device ID register */

SIO -> Super I/O. "SIO" might be confused with "serial I/O" or so.
> fprintf(stderr, "\nERROR: %s: W83627HF: Wrong ID: 0x%02X.\n",
> 	name, wbsio_read(0x20));
> w836xx_ext_leave();
> 	name, wbsio_read(W836_INDEX_2E, 0x20));
> w836xx_ext_leave(W836_INDEX_2E);
>
> return -1;
> }
>
> /* PIN89S: WDTO/GP24 multiplex -> GPIO24 */
> wbsio_mask(0x2B, 0x10, 0x10);
> wbsio_mask(W836_INDEX_2E, 0x2B, 0x10, 0x10);
> wbsio_write(0x07, 0x08);	/* Select logical device 8: GPIO port 2 */
> wbsio_write(W836_INDEX_2E, 0x07, 0x08);	/* Select logical device 8: GPIO port 2 */
> wbsio_mask(0x30, 0x01, 0x01);	/* Activate logical device. */
> wbsio_mask(W836_INDEX_2E, 0x30, 0x01, 0x01);	/* Activate logical device. */
> wbsio_mask(0xF0, 0x00, 0x10);	/* GPIO24 -> output */
> wbsio_mask(W836_INDEX_2E, 0xF0, 0x00, 0x10);	/* GPIO24 -> output */
> wbsio_mask(0xF2, 0x00, 0x10);	/* Clear GPIO24 inversion */
> wbsio_mask(W836_INDEX_2E, 0xF2, 0x00, 0x10);	/* Clear GPIO24 inversion */
> wbsio_mask(0xF1, 0x10, 0x10);	/* Raise GPIO24 */
> wbsio_mask(W836_INDEX_2E, 0xF1, 0x10, 0x10);	/* Raise GPIO24 */

Just use 0x2e here (not W836_INDEX_2E).
> w836xx_ext_leave();
>
> w836xx_ext_leave(W836_INDEX_2E);
>
> return 0;
>
> }
>
> /**
> Winbond W83627THF: GPIO 4, bit 4
> Suited for:
> MSI K8N-NEO3
> */
>
> +static int w83627thf_gpio4_4_raise(const char *name)
> +{
> w836xx_ext_enter(W836_INDEX_4E);

Same here, this is 0x4e specific and won't work for other boards which
have the (same) Super I/O at 0x2e?

I'd pass the port as a parameter to the function. The respective
mainboard-specific function should contain the specific 0x2e or 0x4e.
> /* Is this the w83627thf? */
> if (wbsio_read(W836_INDEX_4E, 0x20) != 0x82) {	/* SIO device ID register */
> fprintf(stderr, "\nERROR: %s: W83627THF: Wrong ID: 0x%02X.\n",
> 	name, wbsio_read(W836_INDEX_4E, 0x20));
> w836xx_ext_leave(W836_INDEX_4E);
> return -1;
> }
> /* PINxxxxS: GPIO4/bit 4 multiplex -> GPIOXXX */
> wbsio_write(W836_INDEX_4E, 0x07, 0x09);	/* Select logical device 9: GPIO port 4 */
> wbsio_mask(W836_INDEX_4E, 0x30, 0x02, 0x02);	/* Activate logical device. */
> wbsio_mask(W836_INDEX_4E, 0xF4, 0x00, 0x10);	/* GPIO4 bit 4 -> output */
> wbsio_mask(W836_INDEX_4E, 0xF6, 0x00, 0x10);	/* Clear GPIO4 bit 4 inversion */
> wbsio_mask(W836_INDEX_4E, 0xF5, 0x10, 0x10);	/* Raise GPIO4 bit 4 */
> w836xx_ext_leave(W836_INDEX_4E);
> return 0;
>
> +}
>
> +/**
> Suited for VIAs EPIA M and MII, and maybe other CLE266 based EPIAs.
> We don't need to do this when using LinuxBIOS, GPIO15 is never lowered there.
>
> @@ -165,12 +199,12 @@
>    pci_write_byte(dev, 0x59, val);
>
> /* Raise ROM MEMW# line on Winbond w83697 SuperIO */
> w836xx_ext_enter();
> w836xx_ext_enter(W836_INDEX_2E);
> if (!(wbsio_read(0x24) & 0x02))	/* flash rom enabled? */
> wbsio_mask(0x24, 0x08, 0x08);	/* enable MEMW# */
> if (!(wbsio_read(W836_INDEX_2E, 0x24) & 0x02))	/* flash rom enabled? */
> wbsio_mask(W836_INDEX_2E, 0x24, 0x08, 0x08);	/* enable MEMW# */
>
> w836xx_ext_leave();
>
> w836xx_ext_leave(W836_INDEX_2E);
>
> return 0;
>
> }
> @@ -316,6 +350,8 @@
>  struct board_pciid_enable board_pciid_enables[] = {
>    {0x1022, 0x7468, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
>     "iwill", "dk8_htx", "IWILL DK8-HTX", w83627hf_gpio24_raise},
> {0x10de, 0x005e, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
> "MSI", "K8N-NEO3", "MSI K8N-NEO3", w83627thf_gpio4_4_raise},

Small spelling fixes:

"msi", "k8n-neo3", "MSI K8N Neo3", w83627thf_gpio4_4_raise},

Are you sure this is not the MSI K8N Neo3-F (attached -F)?

Uwe.

[LinuxBIOS] [PATCH] Flashrom -- Add W83627THF and K8N-Neo3 support

[ron minnich]

My changes based on comments. NOT tested.

ron

[LinuxBIOS] [PATCH] Flashrom -- Add W83627THF and K8N-Neo3 support

[Uwe Hermann]

On Wed, Oct 10, 2007 at 02:41:14PM -0700, ron minnich wrote:
> My changes based on comments. NOT tested.

Can someone please test the last patch before commiting on hardware?
> David's changes plus suggested changes from uwe.

Add Signed-off-by: David Hendricks 
here please (in addition to your sign-off), so we don't forget to put
both in the commit log (as David signed-off the first revision of
the patch)...
> Signed-off-by: Ronald G. Minnich
> Index: jedec.c
>
> --- jedec.c	(revision 2847)
> +++ jedec.c	(working copy)
> @@ -281,7 +281,7 @@
>    // dumb check if erase was successful.
>    for (i = 0; i < total_size; i++) {
>    	if (bios[i] != (uint8_t) 0xff) {
> 	printf("ERASE FAILED\n");
> 	printf("ERASE FAILED@%d, val %02x\n", i, bios[i]);

^
                space between FAILED and "@", otherwise it looks strange
> 	return -1;
> }
>
> }
> Index: board_enable.c
> ===================================================================
> --- board_enable.c	(revision 2847)
> +++ board_enable.c	(working copy)
> @@ -202,43 +202,39 @@
>  /*
> Helper functions for many Winbond Super I/Os of the W836xx range.
>
> */
> -#define W836_INDEX 0x2E
> -#define W836_DATA  0x2F
>
> /* Enter extended functions */
> -static void w836xx_ext_enter(void)
> +static void w836xx_ext_enter(uint16_t port)
>  {
> outb(0x87, W836_INDEX);
> outb(0x87, W836_INDEX);
> outb(0x87, port);
> outb(0x87, port);
>
> }
>
> /* Leave extended functions */
> -static void w836xx_ext_leave(void)
> +static void w836xx_ext_leave(uint16_t port)
>  {
> outb(0xAA, W836_INDEX);
> outb(0xAA, port);
>
> }
>
> /* General functions for reading/writing Winbond Super I/Os. */
> -static unsigned char wbsio_read(unsigned char index)
> +static unsigned char wbsio_read(uint16_t index, uint8_t reg)
>  {
> outb(index, W836_INDEX);
> return inb(W836_DATA);
> outb(reg, index);
> return inb(index+1);

^^
                      spaces
> }
>
> -static void wbsio_write(unsigned char index, unsigned char data)
> +static void wbsio_write(uint16_t index, uint8_t reg, uint8_t data)
>  {
> outb(index, W836_INDEX);
> outb(data, W836_DATA);
> outb(reg, index);
> outb(data, index+1);

Ditto.
> }
>
> -static void wbsio_mask(unsigned char index, unsigned char data,
>        unsigned char mask)
>
> +static void wbsio_mask(uint16_t index, uint8_t reg, uint8_t data, uint8_t mask)
>  {
> unsigned char tmp;
> uint8_t tmp;
> outb(index, W836_INDEX);
> tmp = inb(W836_DATA) & ~mask;
> outb(tmp | (data & mask), W836_DATA);
> outb(reg, index);
> tmp = inb(index+1) & ~mask;

Ditto.
> outb(tmp | (data & mask), index+1);
>
> }
>
> /**
> @@ -248,37 +244,80 @@
> Agami Aruma
> IWILL DK8-HTX
>
> */
> -static int w83627hf_gpio24_raise(const char *name)
> +static int w83627hf_gpio24_raise(uint16_t index, const char *name)
>  {
> w836xx_ext_enter();
>
> w836xx_ext_enter(index);
>
> /* Is this the w83627hf? */
> if (wbsio_read(0x20) != 0x52) {	/* SIO device ID register */
> if (wbsio_read(index, 0x20) != 0x52) {	/* Super I/O device ID register */
> fprintf(stderr, "\nERROR: %s: W83627HF: Wrong ID: 0x%02X.\n",
> 	name, wbsio_read(0x20));
> w836xx_ext_leave();
> 	name, wbsio_read(0x2E, 0x20));
> w836xx_ext_leave(index);

Why 0x2e in the first line and index in the second? Shouldn't both be index?
> return -1;
>
> }
>
> /* PIN89S: WDTO/GP24 multiplex -> GPIO24 */
> wbsio_mask(0x2B, 0x10, 0x10);
> wbsio_mask(index, 0x2B, 0x10, 0x10);
> wbsio_write(0x07, 0x08);	/* Select logical device 8: GPIO port 2 */
> wbsio_write(index, 0x07, 0x08);	/* Select logical device 8: GPIO port 2 */
> wbsio_mask(0x30, 0x01, 0x01);	/* Activate logical device. */
> wbsio_mask(index, 0x30, 0x01, 0x01);	/* Activate logical device. */
> wbsio_mask(0xF0, 0x00, 0x10);	/* GPIO24 -> output */
> wbsio_mask(index, 0xF0, 0x00, 0x10);	/* GPIO24 -> output */
> wbsio_mask(0xF2, 0x00, 0x10);	/* Clear GPIO24 inversion */
> wbsio_mask(index, 0xF2, 0x00, 0x10);	/* Clear GPIO24 inversion */
> wbsio_mask(0xF1, 0x10, 0x10);	/* Raise GPIO24 */
> wbsio_mask(index, 0xF1, 0x10, 0x10);	/* Raise GPIO24 */
> w836xx_ext_leave();
>
> w836xx_ext_leave(index);
>
> return 0;
>
> }
>
> +static int w83627hf_gpio24_raise_2E(const char *name)

Make the function name all-lowercase as usual, please
(lowercase E in the _2E).
> +{
> return w83627hf_gpio24_raise(0x2d, name);
>
> +}
>
> /**
> Winbond W83627THF: GPIO 4, bit 4
> Suited for:
> MSI K8N-NEO3
> */
>
> +static int w83627thf_gpio4_4_raise(uint16_t index, const char *name)
> +{
> w836xx_ext_enter(index);
> /* Is this the w83627thf? */
> if (wbsio_read(index, 0x20) != 0x82) {	/* SIO device ID register */

SIO -> Super I/O
> fprintf(stderr, "\nERROR: %s: W83627THF: Wrong ID: 0x%02X.\n",
> 	name, wbsio_read(index, 0x20));
> w836xx_ext_leave(index);
> return -1;
> }
> /* PINxxxxS: GPIO4/bit 4 multiplex -> GPIOXXX */
> wbsio_write(index, 0x07, 0x09);	/* Select logical device 9: GPIO port 4 */
> wbsio_mask(index, 0x30, 0x02, 0x02);	/* Activate logical device. */
> wbsio_mask(index, 0xF4, 0x00, 0x10);	/* GPIO4 bit 4 -> output */
> wbsio_mask(index, 0xF6, 0x00, 0x10);	/* Clear GPIO4 bit 4 inversion */
> wbsio_mask(index, 0xF5, 0x10, 0x10);	/* Raise GPIO4 bit 4 */
> w836xx_ext_leave(index);
> return 0;
>
> +}
>
> +static int w83627thf_gpio4_4_raise_4E(const char *name)

4E -> 4e
> +{
> return  w83627thf_gpio4_4_raise(0x4E, name);

^^
        Only one space should be here
> +}
> +/**
> Suited for VIAs EPIA M and MII, and maybe other CLE266 based EPIAs.
> We don't need to do this when using LinuxBIOS, GPIO15 is never lowered there.
>
> @@ -335,12 +374,12 @@
>    pci_write_byte(dev, 0x59, val);
>
> /* Raise ROM MEMW# line on Winbond w83697 SuperIO */
> w836xx_ext_enter();
> w836xx_ext_enter(0x2E);
> if (!(wbsio_read(0x24) & 0x02))	/* flash rom enabled? */
> wbsio_mask(0x24, 0x08, 0x08);	/* enable MEMW# */
> if (!(wbsio_read(0x2E, 0x24) & 0x02))	/* flash rom enabled? */
> wbsio_mask(0x2E, 0x24, 0x08, 0x08);	/* enable MEMW# */
>
> w836xx_ext_leave();
>
> w836xx_ext_leave(0x2E);
>
> return 0;
>
> }
> @@ -487,9 +526,11 @@
>    {0x10de, 0x0360, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
>     "gigabyte", "m57sli", "GIGABYTE GA-M57SLI", it87xx_probe_serial_flash},
>    {0x1022, 0x7468, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
> "iwill", "dk8_htx", "IWILL DK8-HTX", w83627hf_gpio24_raise},
> "iwill", "dk8_htx", "IWILL DK8-HTX", w83627hf_gpio24_raise_2E},
> {0x10de, 0x005e, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
> "msr", "k8n-neo3", "MSI K8N Neo3", w83627thf_gpio4_4_raise_4E},

^^^
          msi
> {0x1022, 0x746B, 0x1022, 0x36C0, 0x0000, 0x0000, 0x0000, 0x0000,
> "AGAMI", "ARUMA", "agami Aruma", w83627hf_gpio24_raise},
> "AGAMI", "ARUMA", "agami Aruma", w83627hf_gpio24_raise_2E},
> {0x1106, 0x3177, 0x1106, 0xAA01, 0x1106, 0x3123, 0x1106, 0xAA01,
>   NULL, NULL, "VIA EPIA M/MII/...", board_via_epia_m},
>  {0x1106, 0x3177, 0x1043, 0x80A1, 0x1106, 0x3205, 0x1043, 0x8118,
>
> @@ -529,6 +570,7 @@
>    		continue;
>    	return board;
>    }
> printf("NOT FOUND %s:%s\n", vendor, part);

Will this be shown always or only if you specify -m?

With the above fixes this is ready to be committed, IMO.

Uwe.

[LinuxBIOS] [PATCH] Flashrom -- Add W83627THF and K8N-Neo3 support

[ron minnich]

OK, try this one and david, can you test.

ron

[LinuxBIOS] [PATCH] Flashrom -- Add W83627THF and K8N-Neo3 support

[Stefan Reinauer]

* ron minnich  [071012 21:41]:
> OK, try this one and david, can you test.
>
> ron
> Changes to flashrom to support the K8N-NEO3, first tested at Google on GSOC day :-)
>
> Also minor changes to remove tab-space combinations where possible.
>
> Signed-off-by: Ronald G. Minnich 
> Signed-off-by: David Hendricks

Acked-by: Stefan Reinauer

[LinuxBIOS] [PATCH] Flashrom -- Add W83627THF and K8N-Neo3 support

[David Hendricks]

This one worked perfectly on my hardware.

Signed-off-by: David Hendricks

On 10/12/07, ron minnich  wrote:
> OK, try this one and david, can you test.
>
> ron

[LinuxBIOS] [PATCH] Flashrom -- Add W83627THF and K8N-Neo3 support

[ron minnich]

Committed revision 2850.

On 10/12/07, David Hendricks  wrote:
> This one worked perfectly on my hardware.
>
> Signed-off-by: David Hendricks