[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[Timur Tabi]

Prevent i2c_init() in fsl_i2c.c from writing to the data segment before
relocation.  Commit d8c82db4 added the ability for i2c_init() to program the
I2C bus speed and save the value in i2c_bus_speed[], which is a global
variable.  It is an error to write to the data segment before relocation,
which is what i2c_init() does when it stores the bus speed in i2c_bus_speed[].

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

Wolfgang, please apply this directly to fix the I2C bug we've been talking about.

 drivers/i2c/fsl_i2c.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 9f2c1ec..9d5df8a 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -143,12 +143,15 @@ void
 i2c_init(int speed, int slaveadd)
 {
 	struct fsl_i2c *dev;
+	unsigned int temp;
 
 	dev = (struct fsl_i2c *) (CFG_IMMR + CFG_I2C_OFFSET);
 
 	writeb(0, &dev->cr);			/* stop I2C controller */
 	udelay(5);				/* let it shutdown in peace */
-	i2c_bus_speed[0] = set_i2c_bus_speed(dev, gd->i2c1_clk, speed);
+	temp = set_i2c_bus_speed(dev, gd->i2c1_clk, speed);
+	if (gd->flags & GD_FLG_RELOC)
+		i2c_bus_speed[0] = temp;
 	writeb(slaveadd << 1, &dev->adr);	/* write slave address */
 	writeb(0x0, &dev->sr);			/* clear status register */
 	writeb(I2C_CR_MEN, &dev->cr);		/* start I2C controller */
@@ -158,7 +161,9 @@ i2c_init(int speed, int slaveadd)
 
 	writeb(0, &dev->cr);			/* stop I2C controller */
 	udelay(5);				/* let it shutdown in peace */
-	i2c_bus_speed[1] = set_i2c_bus_speed(dev, gd->i2c2_clk, speed);
+	temp = set_i2c_bus_speed(dev, gd->i2c2_clk, speed);
+	if (gd->flags & GD_FLG_RELOC)
+		i2c_bus_speed[1] = temp;
 	writeb(slaveadd << 1, &dev->adr);	/* write slave address */
 	writeb(0x0, &dev->sr);			/* clear status register */
 	writeb(I2C_CR_MEN, &dev->cr);		/* start I2C controller */
-- 
1.5.5

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:26 Mon 21 Jul     , Timur Tabi wrote:
> Prevent i2c_init() in fsl_i2c.c from writing to the data segment before
> relocation.  Commit d8c82db4 added the ability for i2c_init() to program the
> I2C bus speed and save the value in i2c_bus_speed[], which is a global
> variable.  It is an error to write to the data segment before relocation,
> which is what i2c_init() does when it stores the bus speed in i2c_bus_speed[].
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> 
> Wolfgang, please apply this directly to fix the I2C bug we've been talking about.
> 
>  drivers/i2c/fsl_i2c.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> index 9f2c1ec..9d5df8a 100644
> --- a/drivers/i2c/fsl_i2c.c
> +++ b/drivers/i2c/fsl_i2c.c
> @@ -143,12 +143,15 @@ void
>  i2c_init(int speed, int slaveadd)
>  {
>  	struct fsl_i2c *dev;
> +	unsigned int temp;
>  
>  	dev = (struct fsl_i2c *) (CFG_IMMR + CFG_I2C_OFFSET);
>  
>  	writeb(0, &dev->cr);			/* stop I2C controller */
>  	udelay(5);				/* let it shutdown in peace */
> -	i2c_bus_speed[0] = set_i2c_bus_speed(dev, gd->i2c1_clk, speed);
> +	temp = set_i2c_bus_speed(dev, gd->i2c1_clk, speed);
> +	if (gd->flags & GD_FLG_RELOC)
Maybe a macro will more clear like
	if(is_relacated())
		i2c_bus_speed[0] = temp;

Best Regards,
J.

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[Timur Tabi]

Jean-Christophe PLAGNIOL-VILLARD wrote:

> Maybe a macro will more clear like
> 	if(is_relacated())
> 		i2c_bus_speed[0] = temp;

I'm just following the same pattern as other code.  The code "if (gd->flags &
GD_FLG_RELOC)" is used in a number of other places.

Someone else can create that macro and update all the U-Boot code to use it, if
he wants.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[Wolfgang Denk]

In message <20080721201319.GA16567@game.jcrosoft.org> you wrote:
>
> > +	if (gd->flags & GD_FLG_RELOC)
> Maybe a macro will more clear like
> 	if(is_relacated())

Actually, I disagree here.

Also, for consistency you would have to change many other uses of
(gd->flags & GD_FLG_???) as well.

Let's keep as is.

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
All easy problems have already been solved.

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[David Hawkins]

Hi Timur,

> -	i2c_bus_speed[0] = set_i2c_bus_speed(dev, gd->i2c1_clk, speed);
> +	temp = set_i2c_bus_speed(dev, gd->i2c1_clk, speed);
> +	if (gd->flags & GD_FLG_RELOC)
> +		i2c_bus_speed[0] = temp;

Does i2c_init() get called again after relocation?

If the I2C code is only ever used during flash startup,
then this is dead code.

The I2C controller needs to get initialized to read the
I2C SPD EEPROM, so that it can then setup DDR.

I guess in some cases a board with fixed DDR would not
need to initialize the I2C controller until after
relocation.

If you need I2C speed tracking code, why not just re-read
the I2C controller registers, and determine the speed from
there? That is independent of relocation.

Cheers,
Dave

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[Timur Tabi]

David Hawkins wrote:
> Hi Timur,
> 
>> -	i2c_bus_speed[0] = set_i2c_bus_speed(dev, gd->i2c1_clk, speed);
>> +	temp = set_i2c_bus_speed(dev, gd->i2c1_clk, speed);
>> +	if (gd->flags & GD_FLG_RELOC)
>> +		i2c_bus_speed[0] = temp;
> 
> Does i2c_init() get called again after relocation?

Yes.

> I guess in some cases a board with fixed DDR would not
> need to initialize the I2C controller until after
> relocation.

We're okay then, too.

> If you need I2C speed tracking code, why not just re-read
> the I2C controller registers, and determine the speed from
> there? That is independent of relocation.

I suppose we could do that.  I was planning on rewriting the I2C subsystem one
day, anyway.


-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[Jerry Van Baren]

Timur Tabi wrote:
> David Hawkins wrote:
>> Hi Timur,

[snip]

>> If you need I2C speed tracking code, why not just re-read
>> the I2C controller registers, and determine the speed from
>> there? That is independent of relocation.
> 
> I suppose we could do that.

That won't work for soft (bit-banged) I2C.

Best regards,
gvb

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[Wolfgang Denk]

In message <4884F1A0.8000707@ovro.caltech.edu> you wrote:
> 
> I guess in some cases a board with fixed DDR would not
> need to initialize the I2C controller until after
> relocation.

If ever - let's keep in mind that U-Boot initializes only devices
which it actually uses itself.

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
If a man had a child who'd gone  anti-social,  killed  perhaps,  he'd
still tend to protect that child.
	-- McCoy, "The Ultimate Computer", stardate 4731.3

[U-Boot-Users] [PATCH] fsl-i2c: fix writes to data segment before relocation

[Wolfgang Denk]

In message <1216668383-17967-1-git-send-email-timur@freescale.com> you wrote:
> Prevent i2c_init() in fsl_i2c.c from writing to the data segment before
> relocation.  Commit d8c82db4 added the ability for i2c_init() to program the
> I2C bus speed and save the value in i2c_bus_speed[], which is a global
> variable.  It is an error to write to the data segment before relocation,
> which is what i2c_init() does when it stores the bus speed in i2c_bus_speed[].
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> 
> Wolfgang, please apply this directly to fix the I2C bug we've been talking about.
> 
>  drivers/i2c/fsl_i2c.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)

Applied, thanks.

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
"Once they go up, who cares where  they  come  down?  That's  not  my
department."                                       - Werner von Braun