fsl-i2c: fix writes to data segment before relocation

PATCH New Timur Tabi 5 participants 9 messages Jul 21, 2008 – Jul 29, 2008 I2C

Share & tools

Timur Tabi

author

2008-07-21 19:26 UTC #0 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

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 <[email protected]>

---

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(-)

drivers/i2c/fsl_i2c.c +7 -2

index 9f2c1ec..9d5df8a 100644
@@ -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 */

Signature · 1 line

1.5.5

Jean-Christophe PLAGNIOL-VILLARD

2008-07-21 20:13 UTC #1 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

Quoted 21 lines · 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 <[email protected]>
---
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(-)
drivers/i2c/fsl_i2c.c +0 -0
index 9f2c1ec..9d5df8a 100644
@@ -143,12 +143,15 @@ void
 i2c_init(int speed, int slaveadd)
 {

drivers/i2c/fsl_i2c.c void · line 153

Quoted 10 lines

drivers/i2c/fsl_i2c.c +3 -1

 	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.

Timur Tabi

author

2008-07-21 20:21 UTC #2 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

Quoted 3 lines · Jean-Christophe PLAGNIOL-VILLARD

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.

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale

Wolfgang Denk

2008-07-21 20:44 UTC #3 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

In reply to Jean-Christophe PLAGNIOL-VILLARD

Quoted 4 lines · In message <[email protected]> you wrote:

Review
Quoted 2 lines
(quoted diff) +1 -0
+	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

Signature · 4 lines

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.

David Hawkins

2008-07-21 20:29 UTC #4 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

In reply to Timur Tabi

Hi Timur,

drivers/i2c/fsl_i2c.c void · line 154

Quoted 4 lines

drivers/i2c/fsl_i2c.c +3 -1

-	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

Timur Tabi

author

2008-07-21 20:31 UTC #5 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

Quoted 8 lines · David Hawkins

Hi Timur,
drivers/i2c/fsl_i2c.c void · line 154
Quoted 4 lines
drivers/i2c/fsl_i2c.c +3 -1
-	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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale

Jerry Van Baren

2008-07-21 20:39 UTC #6 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

Quoted 2 lines · Timur Tabi

Quoted 1 line · David Hawkins
Hi Timur,

[snip]

Quoted 5 lines

Quoted 3 lines
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

Wolfgang Denk

2008-07-21 20:45 UTC #7 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

In reply to David Hawkins

Quoted 4 lines · In message <[email protected]> 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

Signature · 6 lines

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

Wolfgang Denk

Applied

2008-07-29 22:10 UTC #8 lore raw

PATCH 1/1 fsl-i2c: fix writes to data segment before relocation

In reply to Timur Tabi

Quoted 6 lines · In message <[email protected]> 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[].

Quoted 7 lines

Signed-off-by Timur Tabi <[email protected]>
---
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

Signature · 5 lines

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