[U-Boot-Users] [PATCH] 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 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 */
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.
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.
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
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
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 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
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
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
participants (5)
-
David Hawkins -
Jean-Christophe PLAGNIOL-VILLARD -
Jerry Van Baren -
Timur Tabi -
Wolfgang Denk