[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Anton Vorontsov]

The warning is bogus, so silence it with uninitialized_var().

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 board/freescale/common/sys_eeprom.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/board/freescale/common/sys_eeprom.c b/board/freescale/common/sys_eeprom.c
index c0fff68..a765b39 100644
--- a/board/freescale/common/sys_eeprom.c
+++ b/board/freescale/common/sys_eeprom.c
@@ -24,6 +24,7 @@
  */
 
 #include <common.h>
+#include <compiler.h>
 #include <command.h>
 #include <i2c.h>
 #include <linux/ctype.h>
@@ -204,7 +205,8 @@ static void update_crc(void)
  */
 static int prog_eeprom(void)
 {
-	int ret, i;
+	int uninitialized_var(ret);
+	int i;
 	void *p;
 #ifdef CONFIG_SYS_EEPROM_BUS_NUM
 	unsigned int bus;
-- 
1.6.3.3

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Kumar Gala]

On Sep 1, 2009, at 11:59 AM, Anton Vorontsov wrote:

> The warning is bogus, so silence it with uninitialized_var().
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> board/freescale/common/sys_eeprom.c |    4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/board/freescale/common/sys_eeprom.c b/board/freescale/ 
> common/sys_eeprom.c
> index c0fff68..a765b39 100644
> --- a/board/freescale/common/sys_eeprom.c
> +++ b/board/freescale/common/sys_eeprom.c
> @@ -24,6 +24,7 @@
>  */
>
> #include <common.h>
> +#include <compiler.h>
> #include <command.h>
> #include <i2c.h>
> #include <linux/ctype.h>
> @@ -204,7 +205,8 @@ static void update_crc(void)
>  */
> static int prog_eeprom(void)
> {
> -	int ret, i;
> +	int uninitialized_var(ret);
> +	int i;

why don't we init ret = 0;  seems like we should be doing that since  
we might not enter the for loop

- k

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Anton Vorontsov]

On Tue, Sep 01, 2009 at 12:30:53PM -0500, Kumar Gala wrote:
> 
> On Sep 1, 2009, at 11:59 AM, Anton Vorontsov wrote:
> 
> >The warning is bogus, so silence it with uninitialized_var().
> >
> >Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> >---
> >board/freescale/common/sys_eeprom.c |    4 +++-
> >1 files changed, 3 insertions(+), 1 deletions(-)
> >
> >diff --git a/board/freescale/common/sys_eeprom.c
> >b/board/freescale/common/sys_eeprom.c
> >index c0fff68..a765b39 100644
> >--- a/board/freescale/common/sys_eeprom.c
> >+++ b/board/freescale/common/sys_eeprom.c
> >@@ -24,6 +24,7 @@
> > */
> >
> >#include <common.h>
> >+#include <compiler.h>
> >#include <command.h>
> >#include <i2c.h>
> >#include <linux/ctype.h>
> >@@ -204,7 +205,8 @@ static void update_crc(void)
> > */
> >static int prog_eeprom(void)
> >{
> >-	int ret, i;
> >+	int uninitialized_var(ret);
> >+	int i;
> 
> why don't we init ret = 0;  seems like we should be doing that since
> we might not enter the for loop

No, we always enter the for loop:

for (i = 0, p = &e; i < sizeof(e); i += 8, p += 8) {

sizeof(e) always > 0 because:

#if !defined(CONFIG_SYS_I2C_EEPROM_CCID) && !defined(CONFIG_SYS_I2C_EEPROM_NXID)
#error "Please define either CONFIG_SYS_I2C_EEPROM_CCID or CONFIG_SYS_I2C_EEPROM_NXID"
#endif

static struct __attribute__ ((__packed__)) eeprom {
#ifdef CONFIG_SYS_I2C_EEPROM_CCID
        u8 id[4];         /* 0x00 - 0x03 EEPROM Tag 'CCID' */
...
#endif
#ifdef CONFIG_SYS_I2C_EEPROM_NXID
        u8 id[4];         /* 0x00 - 0x03 EEPROM Tag 'NXID' */
...
#endif
} e;

-- 
Anton Vorontsov
email: cbouatmailru at gmail.com
irc://irc.freenode.net/bd2

[U-Boot] invalid core multipliers?

[Kári Davíðsson]

Hello,

I am bootstrapping an mpc5121e board.

When the cpu speed is calculated (cpu/mpc512x/speed.c) there is a comment about
all core pll settings above 7 are invalid....

I can't see anything about that in the CPU documentation so I am wondering if
anyone has reference to where this is defined as invalid.

The board I am using is using CORE PLL index 0x8, i.e. multiplier of 4.

If this comment in the code is wrong then the following patch should be o.k...

Index: cpu/mpc512x/speed.c
===================================================================
--- cpu/mpc512x/speed.c	(revision 635)
+++ cpu/mpc512x/speed.c	(working copy)
@@ -44,7 +44,7 @@
  	{1, 1}, {3, 2},
  	{2, 1}, {5, 2},
  	{3, 1}, {7, 2},
-	{0, 1}, {0, 1}, /* and all above 7 are not valid too */
+	{4, 1}, {0, 1}, /* and all above 7 are not valid too.... I doubt this claim */
  	{0, 1}, {0, 1},
  	{0, 1}, {0, 1},
  	{0, 1}, {0, 1}

rg
kd

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Anton Vorontsov]

On Tue, Sep 01, 2009 at 09:38:14PM +0400, Anton Vorontsov wrote:
[...]
> > >static int prog_eeprom(void)
> > >{
> > >-	int ret, i;
> > >+	int uninitialized_var(ret);
> > >+	int i;
> > 
> > why don't we init ret = 0;  seems like we should be doing that since
> > we might not enter the for loop
> 
> No, we always enter the for loop:
> 
> for (i = 0, p = &e; i < sizeof(e); i += 8, p += 8) {
> 
> sizeof(e) always > 0 because:
> 
> #if !defined(CONFIG_SYS_I2C_EEPROM_CCID) && !defined(CONFIG_SYS_I2C_EEPROM_NXID)
> #error "Please define either CONFIG_SYS_I2C_EEPROM_CCID or CONFIG_SYS_I2C_EEPROM_NXID"
> #endif
> 
> static struct __attribute__ ((__packed__)) eeprom {
> #ifdef CONFIG_SYS_I2C_EEPROM_CCID
>         u8 id[4];         /* 0x00 - 0x03 EEPROM Tag 'CCID' */
> ...
> #endif
> #ifdef CONFIG_SYS_I2C_EEPROM_NXID
>         u8 id[4];         /* 0x00 - 0x03 EEPROM Tag 'NXID' */
> ...
> #endif
> } e;

Another option is to turn the for loop into do {} while, so
we can avoid unitialized_var() usage. Something like that:

diff --git a/board/freescale/common/sys_eeprom.c b/board/freescale/common/sys_eeprom.c
index c0fff68..a4e0980 100644
--- a/board/freescale/common/sys_eeprom.c
+++ b/board/freescale/common/sys_eeprom.c
@@ -204,8 +204,8 @@ static void update_crc(void)
  */
 static int prog_eeprom(void)
 {
-	int ret, i;
-	void *p;
+	int ret;
+	int i = 0;
 #ifdef CONFIG_SYS_EEPROM_BUS_NUM
 	unsigned int bus;
 #endif
@@ -224,13 +224,14 @@ static int prog_eeprom(void)
 	i2c_set_bus_num(CONFIG_SYS_EEPROM_BUS_NUM);
 #endif
 
-	for (i = 0, p = &e; i < sizeof(e); i += 8, p += 8) {
+	do {
 		ret = i2c_write(CONFIG_SYS_I2C_EEPROM_ADDR, i, CONFIG_SYS_I2C_EEPROM_ADDR_LEN,
-			p, min((sizeof(e) - i), 8));
+			(void *)&e + i, min((sizeof(e) - i), 8));
 		if (ret)
 			break;
 		udelay(5000);	/* 5ms write cycle timing */
-	}
+		i += 8;
+	} while (i < sizeof(e));
 
 #ifdef CONFIG_SYS_EEPROM_BUS_NUM
 	i2c_set_bus_num(bus);

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Timur Tabi]

Anton Vorontsov wrote:

> -	for (i = 0, p = &e; i < sizeof(e); i += 8, p += 8) {
> +	do {
>  		ret = i2c_write(CONFIG_SYS_I2C_EEPROM_ADDR, i, CONFIG_SYS_I2C_EEPROM_ADDR_LEN,
> -			p, min((sizeof(e) - i), 8));
> +			(void *)&e + i, min((sizeof(e) - i), 8));
>  		if (ret)
>  			break;
>  		udelay(5000);	/* 5ms write cycle timing */
> -	}
> +		i += 8;
> +	} while (i < sizeof(e));

Or we could remove the loop altogether and just do the write in one shot.  Is there any reason to believe that any of Freescale's 8[356]xx boards can't handle a large I2C block write of about 50 bytes or so?

-- 
Timur Tabi
Linux kernel developer@Freescale

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Wolfgang Denk]

Dear Anton Vorontsov,

In message <20090901165902.GA6435@oksana.dev.rtsoft.ru> you wrote:
> The warning is bogus, so silence it with uninitialized_var().

Which tool chain issues a warning here?

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
Change is the essential process of all existence.
	-- Spock, "Let That Be Your Last Battlefield",
	   stardate 5730.2

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warningп

[Anton Vorontsov]

On Tue, Sep 01, 2009 at 01:09:26PM -0500, Timur Tabi wrote:
> Anton Vorontsov wrote:
> 
> > -	for (i = 0, p = &e; i < sizeof(e); i += 8, p += 8) {
> > +	do {
> >  		ret = i2c_write(CONFIG_SYS_I2C_EEPROM_ADDR, i, CONFIG_SYS_I2C_EEPROM_ADDR_LEN,
> > -			p, min((sizeof(e) - i), 8));
> > +			(void *)&e + i, min((sizeof(e) - i), 8));
> >  		if (ret)
> >  			break;
> >  		udelay(5000);	/* 5ms write cycle timing */
> > -	}
> > +		i += 8;
> > +	} while (i < sizeof(e));
> 
> Or we could remove the loop altogether and just do the write in one shot.  Is there any reason to believe that any of Freescale's 8[356]xx boards can't handle a large I2C block write of about 50 bytes or so?

I guess "udelay(5000);	/* 5ms write cycle timing */" hints that
it's EEPROM chip dependant, so has nothing to do with i2c controller
capabilities.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru at gmail.com
irc://irc.freenode.net/bd2

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Anton Vorontsov]

On Tue, Sep 01, 2009 at 08:16:26PM +0200, Wolfgang Denk wrote:
> Dear Anton Vorontsov,
> 
> In message <20090901165902.GA6435@oksana.dev.rtsoft.ru> you wrote:
> > The warning is bogus, so silence it with uninitialized_var().
> 
> Which tool chain issues a warning here?

gcc version 4.0.0 (DENX ELDK 4.0 4.0.0) ;-)

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru at gmail.com
irc://irc.freenode.net/bd2

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Timur Tabi]

Anton Vorontsov wrote:
> The warning is bogus, so silence it with uninitialized_var().
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---

Acked-by: Timur Tabi <timur@freescale.com>

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Wolfgang Denk]

Dear Anton Vorontsov,

In message <20090901180613.GA10489@oksana.dev.rtsoft.ru> you wrote:
>
> Another option is to turn the for loop into do {} while, so
> we can avoid unitialized_var() usage. Something like that:

This is much more difficult to read.


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
Witch!  Witch!  They'll burn ya!
	-- Hag, "Tomorrow is Yesterday", stardate unknown

[U-Boot] [PATCH 2/2] fsl: sys_eeprom: Fix 'may be used uninitialized' warning

[Wolfgang Denk]

Dear Anton Vorontsov,

In message <20090901182914.GA19443@oksana.dev.rtsoft.ru> you wrote:
> On Tue, Sep 01, 2009 at 08:16:26PM +0200, Wolfgang Denk wrote:
> > Dear Anton Vorontsov,
> > 
> > In message <20090901165902.GA6435@oksana.dev.rtsoft.ru> you wrote:
> > > The warning is bogus, so silence it with uninitialized_var().
> > 
> > Which tool chain issues a warning here?
> 
> gcc version 4.0.0 (DENX ELDK 4.0 4.0.0) ;-)

Heh, indeed.

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
A Chairman was as necessary to a Board planet  as  the  zero  was  in
mathematics, but being a zero had big disadvantages...
                         - Terry Pratchett, _The Dark Side of the Sun_

[U-Boot] invalid core multipliers?

[Wolfgang Denk]

Dear =?ISO-8859-1?Q?K=E1ri_Dav=ED=F0sson?=,

In message <4A9D610A.3020206@marel.com> you wrote:
> 
> I am bootstrapping an mpc5121e board.
> 
> When the cpu speed is calculated (cpu/mpc512x/speed.c) there is a comment about
> all core pll settings above 7 are invalid....
> 
> I can't see anything about that in the CPU documentation so I am wondering if
> anyone has reference to where this is defined as invalid.
> 
> The board I am using is using CORE PLL index 0x8, i.e. multiplier of 4.

I tried and checked some old versions of the UM and the old errata
sheets, but I didn't keep all the very early ones, and in the ones
left I didn't find it. Guess this might have been a restriction ofthe
very first silicon.

John, do you happen to remember any details?

> If this comment in the code is wrong then the following patch should be o.k...

Guess so, ut then why not fill up the table?

And please add your SoB line, too.

> Index: cpu/mpc512x/speed.c
> ===================================================================
> --- cpu/mpc512x/speed.c	(revision 635)
> +++ cpu/mpc512x/speed.c	(working copy)
> @@ -44,7 +44,7 @@
>   	{1, 1}, {3, 2},
>   	{2, 1}, {5, 2},
>   	{3, 1}, {7, 2},
> -	{0, 1}, {0, 1}, /* and all above 7 are not valid too */
> +	{4, 1}, {0, 1}, /* and all above 7 are not valid too.... I doubt this claim */
>   	{0, 1}, {0, 1},
>   	{0, 1}, {0, 1},
>   	{0, 1}, {0, 1}

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
"It was the Law of the Sea, they said. Civilization ends at  the  wa-
terline.  Beyond  that,  we  all enter the food chain, and not always
right at the top."                               - Hunter S. Thompson