[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[Timur Tabi]

Update the code which writes to the on-board EEPROM so that it can detect if
the write failed because the EEPROM is write-protected.  Most of the 8xxx-class
Freescale reference boards use an AT24C02 EEPROM to store MAC addresses and
similar information.  With this patch, if the EEPROM is protected, the
"mac save" command will display an error message indicating that the write
has not succeeded.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 board/freescale/common/sys_eeprom.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/board/freescale/common/sys_eeprom.c b/board/freescale/common/sys_eeprom.c
index 5a8f4f5..c5f1c06 100644
--- a/board/freescale/common/sys_eeprom.c
+++ b/board/freescale/common/sys_eeprom.c
@@ -204,7 +204,7 @@ static void update_crc(void)
  */
 static int prog_eeprom(void)
 {
-	int ret = 0; /* shut up gcc */
+	int ret = 0;
 	int i;
 	void *p;
 #ifdef CONFIG_SYS_EEPROM_BUS_NUM
@@ -225,6 +225,11 @@ static int prog_eeprom(void)
 	i2c_set_bus_num(CONFIG_SYS_EEPROM_BUS_NUM);
 #endif
 
+	/*
+	 * The AT24C02 datasheet says that data can only be written in page
+	 * mode, which means 8 bytes at a time, and it takes up to 5ms to
+	 * complete a given write.
+	 */
 	for (i = 0, p = &e; i < sizeof(e); i += 8, p += 8) {
 		ret = i2c_write(CONFIG_SYS_I2C_EEPROM_ADDR, i, CONFIG_SYS_I2C_EEPROM_ADDR_LEN,
 			p, min((sizeof(e) - i), 8));
@@ -233,12 +238,23 @@ static int prog_eeprom(void)
 		udelay(5000);	/* 5ms write cycle timing */
 	}
 
+	if (!ret) {
+		/* Verify the write by reading back the EEPROM and comparing */
+		struct eeprom e2;
+
+		ret = i2c_read(CONFIG_SYS_I2C_EEPROM_ADDR, 0,
+			CONFIG_SYS_I2C_EEPROM_ADDR_LEN, (void *)&e2, sizeof(e2));
+		if (!ret && memcmp(&e, &e2, sizeof(e)))
+			ret = -1;
+	}
+
 #ifdef CONFIG_SYS_EEPROM_BUS_NUM
 	i2c_set_bus_num(bus);
 #endif
 
 	if (ret) {
 		printf("Programming failed.\n");
+		has_been_read = 0;
 		return -1;
 	}
 
-- 
1.7.0.1

[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[Wolfgang Denk]

Dear Timur Tabi,

In message <1280772203-8859-1-git-send-email-timur@freescale.com> you wrote:
> Update the code which writes to the on-board EEPROM so that it can detect if
> the write failed because the EEPROM is write-protected.  Most of the 8xxx-class
> Freescale reference boards use an AT24C02 EEPROM to store MAC addresses and
> similar information.  With this patch, if the EEPROM is protected, the
> "mac save" command will display an error message indicating that the write
> has not succeeded.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  board/freescale/common/sys_eeprom.c |   18 +++++++++++++++++-
...
> +	/*
> +	 * The AT24C02 datasheet says that data can only be written in page
> +	 * mode, which means 8 bytes at a time, and it takes up to 5ms to
> +	 * complete a given write.
> +	 */
>  	for (i = 0, p = &e; i < sizeof(e); i += 8, p += 8) {
>  		ret = i2c_write(CONFIG_SYS_I2C_EEPROM_ADDR, i, CONFIG_SYS_I2C_EEPROM_ADDR_LEN,
>  			p, min((sizeof(e) - i), 8));
> @@ -233,12 +238,23 @@ static int prog_eeprom(void)
>  		udelay(5000);	/* 5ms write cycle timing */
>  	}
>  
> +	if (!ret) {
> +		/* Verify the write by reading back the EEPROM and comparing */
> +		struct eeprom e2;
> +
> +		ret = i2c_read(CONFIG_SYS_I2C_EEPROM_ADDR, 0,
> +			CONFIG_SYS_I2C_EEPROM_ADDR_LEN, (void *)&e2, sizeof(e2));
> +		if (!ret && memcmp(&e, &e2, sizeof(e)))
> +			ret = -1;
> +	}
> +

Why is this i2c_read() needed or actually useful? Should the error
return code from the i2c_write() above not be sufficient indication
that the writing failed?  If that was the case, then some other parts
of the code need fixing.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I'd rather be led to hell than managed to heaven.

[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[Timur Tabi]

Wolfgang Denk wrote:
> Why is this i2c_read() needed or actually useful? Should the error
> return code from the i2c_write() above not be sufficient indication
> that the writing failed?  If that was the case, then some other parts
> of the code need fixing.

That's just the way the EEPROM chip works.  When it's set into write-protect
mode, it cheerfully accepts all of the I2C write commands, and acknowledges
them appropriately, but it doesn't actually store the data into the EEPROM.
 The read-back is the only way I've found to verify that the write has
actually occurred.

This patch fixes a real problem with all of our boards that use this EEPROM
chip, which is almost all of our 83xx, 85xx, and 86xx boards.  Some of our
boards would be shipped with EEPROM write-protected.  The current code
caches the EEPROM contents in memory.  After using the "mac save" command,
the code would think that the EEPROM was updated.  Issuing the "mac read"
command again would *not* re-read from the EEPROM, because the code would
think that the cached value is up-to-date.  The only way you would know if
the EEPROM write failed is to reboot the board.

[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[Wolfgang Denk]

Dear Timur Tabi,

In message <4C57336E.7050704@freescale.com> you wrote:
> Wolfgang Denk wrote:
> > Why is this i2c_read() needed or actually useful? Should the error
> > return code from the i2c_write() above not be sufficient indication
> > that the writing failed?  If that was the case, then some other parts
> > of the code need fixing.
> 
> That's just the way the EEPROM chip works.  When it's set into write-protect
> mode, it cheerfully accepts all of the I2C write commands, and acknowledges
> them appropriately, but it doesn't actually store the data into the EEPROM.
>  The read-back is the only way I've found to verify that the write has
> actually occurred.

Is it correct to assume the the WP signal is connected to some GPIO
pin which can be set / unset in software?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Lots of folks confuse bad management with destiny.   -- Frank Hubbard

[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[Timur Tabi]

Wolfgang Denk wrote:
> Is it correct to assume the the WP signal is connected to some GPIO
> pin which can be set / unset in software?

No, it's a dip switch on the board.  You have to physically set the dip
switch in order write protect or unprotect the EEPROM.

[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[Wolfgang Denk]

Dear Timur Tabi,

In message <4C573530.9050004@freescale.com> you wrote:
> Wolfgang Denk wrote:
> > Is it correct to assume the the WP signal is connected to some GPIO
> > pin which can be set / unset in software?
> 
> No, it's a dip switch on the board.  You have to physically set the dip
> switch in order write protect or unprotect the EEPROM.

And the setting cannot be read back through some GPIO pin eitheer?
Arghh... what a broken design...

Thanks for the explanation.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"There's only one kind of woman ..." "Or man, for  that  matter.  You
either believe in yourself or you don't."
	-- Kirk and Harry Mudd, "Mudd's Women", stardate 1330.1

[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[Timur Tabi]

Wolfgang Denk wrote:
> And the setting cannot be read back through some GPIO pin eitheer?

Technically, the FPGA (on ngPIXIS boards, of which there are only a few)
reads all of the switches and allows software to set and read the values.
But I've found it to be unreliable.  For example, on one board, SW7 is set
to 00001101, but the ngPIXIS shows 00000000.

And even if it were reliable, it wouldn't help, because the ngPIXIS reads
the software-programmable settings only on boot time.  So in order for me to
override the dip switch, I would need to program the right value and then
reboot the board.

And that assumes that the board doesn't have a broken FPGA firmware.  I
believe the current version is 11, or maybe 13.  The version on my board is 4.

The majority of our boards that use this EEPROM don't have an ngPIXIS, they
have the old-style PIXIS, which definitely does not allow software to
read/set any dip switches.

And this isn't the worst part of our designs.  On the P1022, for example,
the same pins are used for the DVI signals and the local bus (used for NOR
flash), so it's not possible to use flash memory while the video display is
active.   That means we can't turn on the video until after relocation, and
when we do, we lose the ability to update flash.

[U-Boot] [PATCH] fsl: verify writes to the MAC address EEPROM

[Kumar Gala]

On Aug 2, 2010, at 1:03 PM, Timur Tabi wrote:

> Update the code which writes to the on-board EEPROM so that it can detect if
> the write failed because the EEPROM is write-protected.  Most of the 8xxx-class
> Freescale reference boards use an AT24C02 EEPROM to store MAC addresses and
> similar information.  With this patch, if the EEPROM is protected, the
> "mac save" command will display an error message indicating that the write
> has not succeeded.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> board/freescale/common/sys_eeprom.c |   18 +++++++++++++++++-
> 1 files changed, 17 insertions(+), 1 deletions(-)

applied to 85xx

- k