Patchwork Error out when chipset/board doesn't decode full ROM

login
register
about
Submitter Uwe Hermann
Date 2009-10-28 02:59:21
Message ID <20091028025920.GE7280@greenwood>
Download mbox | patch
Permalink /patch/502/
State Superseded
Headers show

Comments

Uwe Hermann - 2009-10-28 02:59:21
On Fri, Oct 23, 2009 at 03:33:33PM +0200, Carl-Daniel Hailfinger wrote:
> OK, completely rewritten attempt. I'm reusing Uwe's changelog.

Most of it looks good, but there are some changeѕ required.

 - The checks have to be done later in the code, after the chip has been
   detected, so that we only check the sizes for the chip we actually
   find, not for all the chips we probe for. I moved the checks and
   simplified them a bit (I hope).

 - Use printf instead of printf_debug so that users will see the error
   without having to use -V.

 - Abort if the chip is bigger than the supported ROM decode size.
   Continuing will result in a non-bootable system in almost all cases.
   Adding a --force option later is fine with me, but I think the
   default should be to abort.

Attached is an updated patch with the above changes. I tested it on the
Shuttle AK38N (all operations), output looks like this:

  $ flashrom v0.9.1-r753
  No coreboot table found.
  Found chipset "VIA VT8235", enabling flash write... OK.
  This chipset supports the following protocols: Non-SPI.
  Disabling flash write protection for board "Shuttle AK38N"... OK.
  Calibrating delay loop... OK.
  Found chip "SST SST39SF040" (512 KB, Parallel) at physical address 0xfff80000.
  Chipset/board/programmer only supports a max. size of 256 KB. ABORTING.

I also tested that with a 256KB chip all operations still work fine.

Additionally I tested a 512KB chip on ASUS P4B266 (FWH) with a (simulated)
max. decode size of 256KB and 512KB to ensure that non-parallel chips also
work OK with the code.

The updated patch is

  Acked-by: Uwe Hermann <uwe@hermann-uwe.de>

Feel free to commit.


> @@ -977,6 +1004,8 @@
>  	size = flash->total_size * 1024;
>  	buf = (uint8_t *) calloc(size, sizeof(char));
>  
> +	// FIXME: Check for decode size again?
> +
>  	if (erase_it) {
>  		if (flash->tested & TEST_BAD_ERASE) {
>  			fprintf(stderr, "Erase is not working on this chip. ");

Not sure about this one. Why would an additional check be needed?
According to my tests there's no need for additional checks.

 
Uwe.
Carl-Daniel Hailfinger - 2009-10-28 15:46:57
On 28.10.2009 03:59, Uwe Hermann wrote:
> Most of it looks good, but there are some changeѕ required.
>
>  - The checks have to be done later in the code, after the chip has been
>    detected, so that we only check the sizes for the chip we actually
>    find, not for all the chips we probe for. I moved the checks and
>    simplified them a bit (I hope).
>   

This breaks on Ron's setup where the chip is too big to be detected.
That's why I wanted to get the warning during probe time.


> Index: flashrom.c
> ===================================================================
> --- flashrom.c	(revision 753)
> +++ flashrom.c	(working copy)
> @@ -449,6 +452,25 @@
>  	       flash->vendor, flash->name, flash->total_size,
>  	       flashbuses_to_text(flash->bustype), base);
>  
> +	if ((buses_common & CHIP_BUSTYPE_PARALLEL) &&
> +	     (max_rom_decode.parallel < flash->total_size * 1024))
> +		tmpsize = max_rom_decode.parallel;
> +	if ((buses_common & CHIP_BUSTYPE_LPC) &&
> +	    (max_rom_decode.lpc < flash->total_size * 1024))
> +		tmpsize = max_rom_decode.lpc;
> +	if ((buses_common & CHIP_BUSTYPE_FWH) &&
> +	    (max_rom_decode.fwh < flash->total_size * 1024))
> +		tmpsize = max_rom_decode.fwh;
> +	if ((buses_common & CHIP_BUSTYPE_SPI) &&
> +	    (max_rom_decode.spi < flash->total_size * 1024))
> +		tmpsize = max_rom_decode.spi;
> +	if (tmpsize != 0xffffffff) {
> +		printf("Chipset/board/programmer only supports "
> +		       "a max. size of %u KB. ABORTING.\n", tmpsize / 1024);
> +		programmer_shutdown();
> +		exit(1);
> +	}
>   

I had this code in an earlier version, but decided to change it because
the chipset and the chip may have more than one bus in common (e.g.
SB600 and Pm49* can both speak LPC+FWH) and on SB600/SB7x0/SB8x0 there
are different limits for LPC and FWH. The only way to tell the user
about the exact circumstances is to spew error messages per bus. My
patch was missing the bus-specific error messages, so I can't blame you
for refactoring.

Would it be OK for you if I moved the size checking to a separate
function size_supported(buses_common, size, loglevel) which can be
called during probe (with printf_debug level) and before any
read/write/erase action (with printf_error level)? That way, we avoid
cluttering the main loop, get free checking during probe, and can abort
before any action happens.


>  - Use printf instead of printf_debug so that users will see the error
>    without having to use -V.
>   

OK. On a related note, we really need printf_warn/printf_notice/...


>  - Abort if the chip is bigger than the supported ROM decode size.
>    Continuing will result in a non-bootable system in almost all cases.
>    Adding a --force option later is fine with me, but I think the
>    default should be to abort.
>   

Yes.


> Attached is an updated patch with the above changes. I tested it on the
> Shuttle AK38N (all operations), output looks like this:
>
>   $ flashrom v0.9.1-r753
>   No coreboot table found.
>   Found chipset "VIA VT8235", enabling flash write... OK.
>   This chipset supports the following protocols: Non-SPI.
>   Disabling flash write protection for board "Shuttle AK38N"... OK.
>   Calibrating delay loop... OK.
>   Found chip "SST SST39SF040" (512 KB, Parallel) at physical address 0xfff80000.
>   Chipset/board/programmer only supports a max. size of 256 KB. ABORTING.
>
> I also tested that with a 256KB chip all operations still work fine.
>
> Additionally I tested a 512KB chip on ASUS P4B266 (FWH) with a (simulated)
> max. decode size of 256KB and 512KB to ensure that non-parallel chips also
> work OK with the code.
>
> The updated patch is
>
>   Acked-by: Uwe Hermann <uwe@hermann-uwe.de>
>
> Feel free to commit.
>   

Thanks. I hope to clarify one or two things in an additional round, then
commit.


> Index: chipset_enable.c
> ===================================================================
> --- chipset_enable.c	(revision 753)
> +++ chipset_enable.c	(working copy)
> @@ -536,6 +582,9 @@
>  	reg8 |= BIOS_ROM_POSITIVE_DECODE;
>  	pci_write_byte(dev, DECODE_CONTROL_REG2, reg8);
>  
> +	/* No idea which buses this limit applies to. */
> +	//max_rom_decode.lpc = 16 * 1024 * 1024;
> +
>  	return 0;
>  }
>   

This limit was originally added by you, but I commented out because I
was not sure if this was a LPC limit or a parallel limit or a FWH limit.
Since you are the original author, can you shed some light on this?


>> @@ -977,6 +1004,8 @@
>>  	size = flash->total_size * 1024;
>>  	buf = (uint8_t *) calloc(size, sizeof(char));
>>  
>> +	// FIXME: Check for decode size again?
>> +
>>  	if (erase_it) {
>>  		if (flash->tested & TEST_BAD_ERASE) {
>>  			fprintf(stderr, "Erase is not working on this chip. ");
>>     
>
> Not sure about this one. Why would an additional check be needed?
> According to my tests there's no need for additional checks.
>   

The idea behind this was to have a warning during probe (which does fail
for some chips) and abort before the first real read/write/erase action.
That way, a user can find out why probe might not have worked, and will
be stopped before he/she gets incorrect results.

What do you think? If you want me to post an updated patch for easier
review, just say so.

Regards,
Carl-Daniel

Patch

Add a facility to check and report to the user the maximum supported
decode size for chipsets and tested mainboards.

The rationale is to warn users when they, for example, try to flash
a 512KB parallel flash chip but their chipset only supports 256KB,
or they try to flash 512KB and the chipset _does_ theoretically
support 512KB but their special board doesn't wire all address lines
and thus supports only 256 KB ROM chips at maximum.

This has cost Uwe hours of debugging on some board already, until he
figured out what was going on, we should try warn our users where
possible about this (yes, this is "best effort", but still better than
not to warn at all).

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

Index: flash.h
===================================================================
--- flash.h	(revision 753)
+++ flash.h	(working copy)
@@ -352,9 +352,17 @@ 
 void sio_mask(uint16_t port, uint8_t reg, uint8_t data, uint8_t mask);
 int board_flash_enable(const char *vendor, const char *part);
 
+struct decode_sizes {
+	uint32_t parallel;
+	uint32_t lpc;
+	uint32_t fwh;
+	uint32_t spi;
+};
+
 /* chipset_enable.c */
 extern enum chipbustype buses_supported;
 int chipset_flash_enable(void);
+extern struct decode_sizes max_rom_decode;
 
 extern unsigned long flashbase;
 
Index: chipset_enable.c
===================================================================
--- chipset_enable.c	(revision 753)
+++ chipset_enable.c	(working copy)
@@ -41,6 +41,12 @@ 
  */
 
 enum chipbustype buses_supported = CHIP_BUSTYPE_NONSPI;
+struct decode_sizes max_rom_decode = {
+	.parallel	= 0xffffffff,
+	.lpc		= 0xffffffff,
+	.fwh		= 0xffffffff,
+	.spi		= 0xffffffff
+};
 
 extern int ichspi_lock;
 
@@ -204,34 +210,73 @@ 
 	uint32_t fwh_conf;
 	int i;
 	char *idsel = NULL;
+	int tmp;
+	int max_decode_fwh_idsel = 0;
+	int max_decode_fwh_decode = 0;
+	int contiguous = 1;
 
-	/* Ignore all legacy ranges below 1 MB. */
+	/* Ignore all legacy ranges below 1 MB.
+	 * We currently only support flashing the chip which responds to
+	 * IDSEL=0. To support IDSEL!=0, flashbase and decode size calculations
+	 * have to be adjusted.
+	 */
 	/* FWH_SEL1 */
 	fwh_conf = pci_read_long(dev, 0xd0);
-	for (i = 7; i >= 0; i--)
+	for (i = 7; i >= 0; i--) {
+		tmp = (fwh_conf >> (i * 4)) & 0xf;
 		printf_debug("\n0x%08x/0x%08x FWH IDSEL: 0x%x",
 			     (0x1ff8 + i) * 0x80000,
 			     (0x1ff0 + i) * 0x80000,
-			     (fwh_conf >> (i * 4)) & 0xf);
+			     tmp);
+		if ((tmp == 0) && contiguous) {
+			max_decode_fwh_idsel = (8 - i) * 0x80000;
+		} else {
+			contiguous = 0;
+		}
+	}
 	/* FWH_SEL2 */
 	fwh_conf = pci_read_word(dev, 0xd4);
-	for (i = 3; i >= 0; i--)
+	for (i = 3; i >= 0; i--) {
+		tmp = (fwh_conf >> (i * 4)) & 0xf;
 		printf_debug("\n0x%08x/0x%08x FWH IDSEL: 0x%x",
 			     (0xff4 + i) * 0x100000,
 			     (0xff0 + i) * 0x100000,
-			     (fwh_conf >> (i * 4)) & 0xf);
+			     tmp);
+		if ((tmp == 0) && contiguous) {
+			max_decode_fwh_idsel = (8 - i) * 0x100000;
+		} else {
+			contiguous = 0;
+		}
+	}
+	contiguous = 1;
 	/* FWH_DEC_EN1 */
 	fwh_conf = pci_read_word(dev, 0xd8);
-	for (i = 7; i >= 0; i--)
+	for (i = 7; i >= 0; i--) {
+		tmp = (fwh_conf >> (i + 0x8)) & 0x1;
 		printf_debug("\n0x%08x/0x%08x FWH decode %sabled",
 			     (0x1ff8 + i) * 0x80000,
 			     (0x1ff0 + i) * 0x80000,
-			     (fwh_conf >> (i + 0x8)) & 0x1 ? "en" : "dis");
-	for (i = 3; i >= 0; i--)
+			     tmp ? "en" : "dis");
+		if ((tmp == 0) && contiguous) {
+			max_decode_fwh_decode = (8 - i) * 0x80000;
+		} else {
+			contiguous = 0;
+		}
+	}
+	for (i = 3; i >= 0; i--) {
+		tmp = (fwh_conf >> i) & 0x1;
 		printf_debug("\n0x%08x/0x%08x FWH decode %sabled",
 			     (0xff4 + i) * 0x100000,
 			     (0xff0 + i) * 0x100000,
-			     (fwh_conf >> i) & 0x1 ? "en" : "dis");
+			     tmp ? "en" : "dis");
+		if ((tmp == 0) && contiguous) {
+			max_decode_fwh_decode = (8 - i) * 0x100000;
+		} else {
+			contiguous = 0;
+		}
+	}
+	max_rom_decode.fwh = min(max_decode_fwh_idsel, max_decode_fwh_decode);
+	printf_debug("\nMaximum FWH chip size: 0x%x bytes", max_rom_decode.fwh);
 
 	if (programmer_param)
 		idsel = strstr(programmer_param, "fwh_idsel=");
@@ -244,6 +289,7 @@ 
 		printf("\nSetting IDSEL=0x%x for top 16 MB", fwh_conf);
 		pci_write_long(dev, 0xd0, fwh_conf);
 		pci_write_word(dev, 0xd4, fwh_conf);
+		/* FIXME: Decode settings are not changed. */
 	}
 
 	return enable_flash_ich(dev, name, 0xdc);
@@ -536,6 +582,9 @@ 
 	reg8 |= BIOS_ROM_POSITIVE_DECODE;
 	pci_write_byte(dev, DECODE_CONTROL_REG2, reg8);
 
+	/* No idea which buses this limit applies to. */
+	//max_rom_decode.lpc = 16 * 1024 * 1024;
+
 	return 0;
 }
 
Index: flashrom.c
===================================================================
--- flashrom.c	(revision 753)
+++ flashrom.c	(working copy)
@@ -401,7 +401,9 @@ 
 struct flashchip *probe_flash(struct flashchip *first_flash, int force)
 {
 	struct flashchip *flash;
-	unsigned long base = 0, size;
+	unsigned long base = 0;
+	unsigned int size = 0, tmpsize = 0xffffffff;
+	enum chipbustype buses_common = 0;
 	char *tmp;
 
 	for (flash = first_flash; flash && flash->name; flash++) {
@@ -413,7 +415,8 @@ 
 			printf_debug("failed! flashrom has no probe function for this flash chip.\n");
 			continue;
 		}
-		if (!(buses_supported & flash->bustype)) {
+		buses_common = buses_supported & flash->bustype;
+		if (!buses_common) {
 			tmp = flashbuses_to_text(buses_supported);
 			printf_debug("skipped. Host bus type %s ", tmp);
 			free(tmp);
@@ -449,6 +452,25 @@ 
 	       flash->vendor, flash->name, flash->total_size,
 	       flashbuses_to_text(flash->bustype), base);
 
+	if ((buses_common & CHIP_BUSTYPE_PARALLEL) &&
+	     (max_rom_decode.parallel < flash->total_size * 1024))
+		tmpsize = max_rom_decode.parallel;
+	if ((buses_common & CHIP_BUSTYPE_LPC) &&
+	    (max_rom_decode.lpc < flash->total_size * 1024))
+		tmpsize = max_rom_decode.lpc;
+	if ((buses_common & CHIP_BUSTYPE_FWH) &&
+	    (max_rom_decode.fwh < flash->total_size * 1024))
+		tmpsize = max_rom_decode.fwh;
+	if ((buses_common & CHIP_BUSTYPE_SPI) &&
+	    (max_rom_decode.spi < flash->total_size * 1024))
+		tmpsize = max_rom_decode.spi;
+	if (tmpsize != 0xffffffff) {
+		printf("Chipset/board/programmer only supports "
+		       "a max. size of %u KB. ABORTING.\n", tmpsize / 1024);
+		programmer_shutdown();
+		exit(1);
+	}
+
 	return flash;
 }
 
@@ -977,6 +999,8 @@ 
 	size = flash->total_size * 1024;
 	buf = (uint8_t *) calloc(size, sizeof(char));
 
+	// FIXME: Check for decode size again?
+
 	if (erase_it) {
 		if (flash->tested & TEST_BAD_ERASE) {
 			fprintf(stderr, "Erase is not working on this chip. ");
Index: board_enable.c
===================================================================
--- board_enable.c	(revision 753)
+++ board_enable.c	(working copy)
@@ -804,16 +804,24 @@ 
 }
 
 /**
- * Suited for:
- *   - Shuttle AK38N: VIA KT333CF + VIA VT8235 + ITE IT8705F
- *   - Elitegroup K7VTA3: VIA Apollo KT266/A/333 + VIA VT8235 + ITE IT8705F
+ * Suited for: Elitegroup K7VTA3: VIA Apollo KT266/A/333 + VIA VT8235 + ITE IT8705F
  */
-static int it8705f_write_enable_2e(const char *name)
+static int elitegroup_k7vta3(const char *name)
 {
+	max_rom_decode.parallel = 256 * 1024;
 	return it8705f_write_enable(0x2e, name);
 }
 
 /**
+ * Suited for: Shuttle AK38N: VIA KT333CF + VIA VT8235 + ITE IT8705F
+ */
+static int shuttle_ak38n(const char *name)
+{
+	max_rom_decode.parallel = 256 * 1024;
+	return it8705f_write_enable(0x2e, name);
+}
+
+/**
  * Find the runtime registers of an SMSC Super I/O, after verifying its
  * chip ID.
  *
@@ -1079,7 +1087,7 @@ 
 	{0x10DE, 0x0030, 0x1043, 0x818a,  0x8086, 0x100E, 0x1043, 0x80EE, NULL,         NULL,          "ASUS",        "P5ND2-SLI Deluxe",   board_asus_p5nd2_sli},
 	{0x1106, 0x3149, 0x1565, 0x3206,  0x1106, 0x3344, 0x1565, 0x1202, NULL,         NULL,          "Biostar",     "P4M80-M4",           it8705_rom_write_enable},
 	{0x8086, 0x3590, 0x1028, 0x016c,  0x1000, 0x0030, 0x1028, 0x016c, NULL,         NULL,          "Dell",        "PowerEdge 1850",     ich5_gpio23_raise},
-	{0x1106, 0x3038, 0x1019, 0x0996,  0x1106, 0x3177, 0x1019, 0x0996, NULL,         NULL,          "Elitegroup",  "K7VTA3",             it8705f_write_enable_2e},
+	{0x1106, 0x3038, 0x1019, 0x0996,  0x1106, 0x3177, 0x1019, 0x0996, NULL,         NULL,          "Elitegroup",  "K7VTA3",             elitegroup_k7vta3},
 	{0x1106, 0x3177, 0x1106, 0x3177,  0x1106, 0x3059, 0x1695, 0x3005, NULL,         NULL,          "EPoX",        "EP-8K5A2",           board_epox_ep_8k5a2},
 	{0x10EC, 0x8139, 0x1695, 0x9001,  0x11C1, 0x5811, 0x1695, 0x9015, NULL,         NULL,          "EPoX",        "EP-8RDA3+",          board_epox_ep_8rda3plus},
 	{0x8086, 0x7110,      0,      0,  0x8086, 0x7190,      0,      0, "epox",       "ep-bx3",      "EPoX",        "EP-BX3",             board_epox_ep_bx3},
@@ -1105,7 +1113,7 @@ 
 	{0x1106, 0x0571, 0x1462, 0x7120,       0,      0,      0,      0, "msi",        "kt4v",        "MSI",         "MS-6712 (KT4V)",     board_msi_kt4v},
 	{0x8086, 0x2658, 0x1462, 0x7046,  0x1106, 0x3044, 0x1462, 0x046d, NULL,         NULL,          "MSI",         "MS-7046",            ich6_gpio19_raise},
 	{0x10de, 0x005e,      0,      0,       0,      0,      0,      0, "msi",        "k8n-neo3",    "MSI",         "MS-7135 (K8N Neo3)", w83627thf_gpio4_4_raise_4e},
-	{0x1106, 0x3104, 0x1297, 0xa238,  0x1106, 0x3059, 0x1297, 0xc063, NULL,         NULL,          "Shuttle",     "AK38N",              it8705f_write_enable_2e},
+	{0x1106, 0x3104, 0x1297, 0xa238,  0x1106, 0x3059, 0x1297, 0xc063, NULL,         NULL,          "Shuttle",     "AK38N",              shuttle_ak38n},
 	{0x10DE, 0x0050, 0x1297, 0x5036,  0x1412, 0x1724, 0x1297, 0x5036, NULL,         NULL,          "Shuttle",     "FN25",               board_shuttle_fn25},
 	{0x1106, 0x3038, 0x0925, 0x1234,  0x1106, 0x3058, 0x15DD, 0x7609, NULL,         NULL,          "Soyo",        "SY-7VCA",            board_soyo_sy_7vca},
 	{0x8086, 0x1076, 0x8086, 0x1176,  0x1106, 0x3059, 0x10f1, 0x2498, NULL,         NULL,          "Tyan",        "S2498 (Tomcat K7M)", board_asus_a7v8x_mx},