[U-Boot] [PATCH] I2C: Fix mxc_i2c.c problem on phycore pcm037

[U-Boot] [PATCH] I2C: Fix mxc_i2c.c problem on phycore pcm037

[Marek Vasut]

The problem was caused by a global variable being used early in the boot
process.

This patch also contains the multibyte address swapping fix by:
  Anatolij Gustschin <agust@denx.de>

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Heiko Schocher <hs@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
---
 drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index a805bf6..360c54c 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -37,6 +37,7 @@
 
 #include <asm/arch/clock.h>
 #include <asm/arch/imx-regs.h>
+#include <i2c.h>
 
 struct mxc_i2c_regs {
 	uint32_t	iadr;
@@ -95,14 +96,11 @@ static u16 i2c_clk_div[50][2] = {
 	{ 3072,	0x1E }, { 3840,	0x1F }
 };
 
-static u8 clk_idx;
-
 /*
  * Calculate and set proper clock divider
  */
-static void i2c_imx_set_clk(unsigned int rate)
+static uint8_t i2c_imx_get_clk(unsigned int rate)
 {
-	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 	unsigned int i2c_clk_rate;
 	unsigned int div;
 	int i;
@@ -127,9 +125,7 @@ static void i2c_imx_set_clk(unsigned int rate)
 		for (i = 0; i2c_clk_div[i][0] < div; i++)
 			;
 
-	/* Store divider value */
-	clk_idx = i2c_clk_div[i][1];
-	writeb(clk_idx, &i2c_regs->ifdr);
+	return i2c_clk_div[i][1];
 }
 
 /*
@@ -148,7 +144,12 @@ void i2c_reset(void)
  */
 void i2c_init(int speed, int unused)
 {
-	i2c_imx_set_clk(speed);
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	u8 idx = i2c_imx_get_clk(speed);
+
+	/* Store divider value */
+	writeb(idx, &i2c_regs->ifdr);
+
 	i2c_reset();
 }
 
@@ -217,8 +218,11 @@ int i2c_imx_start(void)
 	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 	unsigned int temp = 0;
 	int result;
+	int speed = i2c_get_bus_speed();
+	u8 idx = i2c_imx_get_clk(speed);
 
-	writeb(clk_idx, &i2c_regs->ifdr);
+	/* Store divider value */
+	writeb(idx, &i2c_regs->ifdr);
 
 	/* Enable I2C controller */
 	writeb(0, &i2c_regs->i2sr);
@@ -291,11 +295,10 @@ int i2c_imx_set_chip_addr(uchar chip, int read)
 int i2c_imx_set_reg_addr(uint addr, int alen)
 {
 	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
-	int ret;
-	int i;
+	int ret = 0;
 
-	for (i = 0; i < (8 * alen); i += 8) {
-		writeb((addr >> i) & 0xff, &i2c_regs->i2dr);
+	while (alen--) {
+		writeb((addr >> (alen * 8)) & 0xff, &i2c_regs->i2dr);
 
 		ret = i2c_imx_trx_complete();
 		if (ret)
-- 
1.7.6.3

[U-Boot] [PATCH] I2C: Fix mxc_i2c.c problem on phycore pcm037

[Anatolij Gustschin]

Hi Marek,

On Tue, 25 Oct 2011 14:09:11 +0200
Marek Vasut <marek.vasut@gmail.com> wrote:

> The problem was caused by a global variable being used early in the boot
> process.
> 
> This patch also contains the multibyte address swapping fix by:
>   Anatolij Gustschin <agust@denx.de>
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
>  1 files changed, 16 insertions(+), 13 deletions(-)

Great, this patch fixes the issue! Thanks!

Tested-by: Anatolij Gustschin <agust@denx.de>

But, I think we should slightly fix the commit title and commit
message to describe the observed problem better.

The board is named as "imx31_phycore" in the code, config
file uses this name, too. So we should use "imx31_phycore" name in
U-Boot commits instead of pcm037 to avoid confusion for other users
(pcm037 name is used in Linux code for this board).

In the commit message we should also mention what the problem was:
on imx31_phycore board reading the environment from I2C EEPROM
didn't work correctly.

Thanks,
Anatolij

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH V2] I2C: Fix mxc_i2c.c problem on imx31_phycore

[Marek Vasut]

The problem was caused by a global variable being used early in the boot
process.

The symptoms were on imx31_phycore board, reading the environment from I2C
EEPROM didn't work correctly and causes default environment to be loaded.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Heiko Schocher <hs@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Tested-by: Anatolij Gustschin <agust@denx.de>
---
 drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

V2: Fix commit message

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index a805bf6..360c54c 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -37,6 +37,7 @@
 
 #include <asm/arch/clock.h>
 #include <asm/arch/imx-regs.h>
+#include <i2c.h>
 
 struct mxc_i2c_regs {
 	uint32_t	iadr;
@@ -95,14 +96,11 @@ static u16 i2c_clk_div[50][2] = {
 	{ 3072,	0x1E }, { 3840,	0x1F }
 };
 
-static u8 clk_idx;
-
 /*
  * Calculate and set proper clock divider
  */
-static void i2c_imx_set_clk(unsigned int rate)
+static uint8_t i2c_imx_get_clk(unsigned int rate)
 {
-	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 	unsigned int i2c_clk_rate;
 	unsigned int div;
 	int i;
@@ -127,9 +125,7 @@ static void i2c_imx_set_clk(unsigned int rate)
 		for (i = 0; i2c_clk_div[i][0] < div; i++)
 			;
 
-	/* Store divider value */
-	clk_idx = i2c_clk_div[i][1];
-	writeb(clk_idx, &i2c_regs->ifdr);
+	return i2c_clk_div[i][1];
 }
 
 /*
@@ -148,7 +144,12 @@ void i2c_reset(void)
  */
 void i2c_init(int speed, int unused)
 {
-	i2c_imx_set_clk(speed);
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	u8 idx = i2c_imx_get_clk(speed);
+
+	/* Store divider value */
+	writeb(idx, &i2c_regs->ifdr);
+
 	i2c_reset();
 }
 
@@ -217,8 +218,11 @@ int i2c_imx_start(void)
 	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 	unsigned int temp = 0;
 	int result;
+	int speed = i2c_get_bus_speed();
+	u8 idx = i2c_imx_get_clk(speed);
 
-	writeb(clk_idx, &i2c_regs->ifdr);
+	/* Store divider value */
+	writeb(idx, &i2c_regs->ifdr);
 
 	/* Enable I2C controller */
 	writeb(0, &i2c_regs->i2sr);
@@ -291,11 +295,10 @@ int i2c_imx_set_chip_addr(uchar chip, int read)
 int i2c_imx_set_reg_addr(uint addr, int alen)
 {
 	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
-	int ret;
-	int i;
+	int ret = 0;
 
-	for (i = 0; i < (8 * alen); i += 8) {
-		writeb((addr >> i) & 0xff, &i2c_regs->i2dr);
+	while (alen--) {
+		writeb((addr >> (alen * 8)) & 0xff, &i2c_regs->i2dr);
 
 		ret = i2c_imx_trx_complete();
 		if (ret)
-- 
1.7.6.3

[U-Boot] [PATCH V2] I2C: Fix mxc_i2c.c problem on imx31_phycore

[Stefano Babic]

On 10/25/2011 05:25 PM, Marek Vasut wrote:
> The problem was caused by a global variable being used early in the boot
> process.
> 
> The symptoms were on imx31_phycore board, reading the environment from I2C
> EEPROM didn't work correctly and causes default environment to be loaded.
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> Tested-by: Anatolij Gustschin <agust@denx.de>
> ---
>  drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
>  1 files changed, 16 insertions(+), 13 deletions(-)
> 
> V2: Fix commit message

Tested on mx35pdk.

Tested-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V2] I2C: Fix mxc_i2c.c problem on imx31_phycore

[Stefano Babic]

On 10/26/2011 10:25 AM, Stefano Babic wrote:
> On 10/25/2011 05:25 PM, Marek Vasut wrote:
>> The problem was caused by a global variable being used early in the boot
>> process.
>>
>> The symptoms were on imx31_phycore board, reading the environment from I2C
>> EEPROM didn't work correctly and causes default environment to be loaded.
>>
>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>> Cc: Wolfgang Denk <wd@denx.de>
>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> Cc: Heiko Schocher <hs@denx.de>
>> Cc: Stefano Babic <sbabic@denx.de>
>> Tested-by: Anatolij Gustschin <agust@denx.de>
>> ---
>>  drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
>>  1 files changed, 16 insertions(+), 13 deletions(-)
>>
>> V2: Fix commit message
> 
> Tested on mx35pdk.
> 
> Tested-by: Stefano Babic <sbabic@denx.de>

I tested (and it works), but there is a problem. I supposed it was a
mistake by me, but after some checks I have found the cause.

Your patch does not apply on current u-boot-imx. You sent earlier the patch:

"I2C: Add i2c_get/set_speed() to mxc_i2c.c"

that after Heiko's ACK I have applied to u-boot-imx and then it is
already merged into u-boot-arm. The patch you have sent now does not
take care of your previous patch and does not apply anymore.

Can you rebase your patch ? You can take u-boot-imx als reference, all
imx related patches are already applied.

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V3] I2C: Fix mxc_i2c.c problem on imx31_phycore

[Marek Vasut]

The problem was caused by a global variable being used early in the boot
process.

The symptoms were on imx31_phycore board, reading the environment from I2C
EEPROM didn't work correctly and causes default environment to be loaded.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Heiko Schocher <hs@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
---
 drivers/i2c/mxc_i2c.c |   37 ++++++++++++++++++++++++++-----------
 1 files changed, 26 insertions(+), 11 deletions(-)

V2: Fix commit message
V3: Rebase on top of u-boot-imx

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index bf3ad95..c88ac7c 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -37,6 +37,7 @@
 
 #include <asm/arch/clock.h>
 #include <asm/arch/imx-regs.h>
+#include <i2c.h>
 
 struct mxc_i2c_regs {
 	uint32_t	iadr;
@@ -99,16 +100,14 @@ static u16 i2c_clk_div[50][2] = {
 	{ 3072,	0x1E }, { 3840,	0x1F }
 };
 
-static u8 clk_div;
-
 /*
  * Calculate and set proper clock divider
  */
-static void i2c_imx_set_clk(unsigned int rate)
+static uint8_t i2c_imx_get_clk(unsigned int rate)
 {
-	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 	unsigned int i2c_clk_rate;
 	unsigned int div;
+	u8 clk_div;
 
 #if defined(CONFIG_MX31)
 	struct clock_control_regs *sc_regs =
@@ -131,7 +130,7 @@ static void i2c_imx_set_clk(unsigned int rate)
 			;
 
 	/* Store divider value */
-	writeb(i2c_clk_div[clk_div][1], &i2c_regs->ifdr);
+	return clk_div;
 }
 
 /*
@@ -150,7 +149,13 @@ void i2c_reset(void)
  */
 void i2c_init(int speed, int unused)
 {
-	i2c_imx_set_clk(speed);
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	u8 clk_idx = i2c_imx_get_clk(speed);
+	u8 idx = i2c_clk_div[clk_idx][1];
+
+	/* Store divider value */
+	writeb(idx, &i2c_regs->ifdr);
+
 	i2c_reset();
 }
 
@@ -168,6 +173,13 @@ int i2c_set_bus_speed(unsigned int speed)
  */
 unsigned int i2c_get_bus_speed(void)
 {
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	u8 clk_idx = readb(&i2c_regs->ifdr);
+	u8 clk_div;
+
+	for (clk_div = 0; i2c_clk_div[clk_div][1] != clk_idx; clk_div++)
+		;
+
 	return mxc_get_clock(MXC_IPG_PERCLK) / i2c_clk_div[clk_div][0];
 }
 
@@ -236,8 +248,12 @@ int i2c_imx_start(void)
 	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 	unsigned int temp = 0;
 	int result;
+	int speed = i2c_get_bus_speed();
+	u8 clk_idx = i2c_imx_get_clk(speed);
+	u8 idx = i2c_clk_div[clk_idx][1];
 
-	writeb(i2c_clk_div[clk_div][1], &i2c_regs->ifdr);
+	/* Store divider value */
+	writeb(idx, &i2c_regs->ifdr);
 
 	/* Enable I2C controller */
 	writeb(0, &i2c_regs->i2sr);
@@ -310,11 +326,10 @@ int i2c_imx_set_chip_addr(uchar chip, int read)
 int i2c_imx_set_reg_addr(uint addr, int alen)
 {
 	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
-	int ret;
-	int i;
+	int ret = 0;
 
-	for (i = 0; i < (8 * alen); i += 8) {
-		writeb((addr >> i) & 0xff, &i2c_regs->i2dr);
+	while (alen--) {
+		writeb((addr >> (alen * 8)) & 0xff, &i2c_regs->i2dr);
 
 		ret = i2c_imx_trx_complete();
 		if (ret)
-- 
1.7.6.3

[U-Boot] [PATCH V2] I2C: Fix mxc_i2c.c problem on imx31_phycore

[Marek Vasut]

> On 10/26/2011 10:25 AM, Stefano Babic wrote:
> > On 10/25/2011 05:25 PM, Marek Vasut wrote:
> >> The problem was caused by a global variable being used early in the boot
> >> process.
> >> 
> >> The symptoms were on imx31_phycore board, reading the environment from
> >> I2C EEPROM didn't work correctly and causes default environment to be
> >> loaded.
> >> 
> >> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> >> Cc: Wolfgang Denk <wd@denx.de>
> >> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> >> Cc: Heiko Schocher <hs@denx.de>
> >> Cc: Stefano Babic <sbabic@denx.de>
> >> Tested-by: Anatolij Gustschin <agust@denx.de>
> >> ---
> >> 
> >>  drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
> >>  1 files changed, 16 insertions(+), 13 deletions(-)
> >> 
> >> V2: Fix commit message
> > 
> > Tested on mx35pdk.
> > 
> > Tested-by: Stefano Babic <sbabic@denx.de>
> 
> I tested (and it works), but there is a problem. I supposed it was a
> mistake by me, but after some checks I have found the cause.
> 
> Your patch does not apply on current u-boot-imx. You sent earlier the
> patch:
> 
> "I2C: Add i2c_get/set_speed() to mxc_i2c.c"
> 
> that after Heiko's ACK I have applied to u-boot-imx and then it is
> already merged into u-boot-arm. The patch you have sent now does not
> take care of your previous patch and does not apply anymore.
> 
> Can you rebase your patch ? You can take u-boot-imx als reference, all
> imx related patches are already applied.
> 
> Best regards,
> Stefano Babic

Hi Stefano,

done rebasing on u-boot-imx/master and tested on imx31_phycore.

Cheers

[U-Boot] [PATCH V3] I2C: Fix mxc_i2c.c problem on imx31_phycore

[Stefano Babic]

On 10/26/2011 12:05 PM, Marek Vasut wrote:
> The problem was caused by a global variable being used early in the boot
> process.
> 
> The symptoms were on imx31_phycore board, reading the environment from I2C
> EEPROM didn't work correctly and causes default environment to be loaded.
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> ---

Perfect ! As the patch war already acked and tested, it should be not a
problem if I already merge it into u-boot-imx.

Applied to u-boot-imx, thanks.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V3] I2C: Fix mxc_i2c.c problem on imx31_phycore

[Heiko Schocher]

Hello Stefano,

Stefano Babic wrote:
> On 10/26/2011 12:05 PM, Marek Vasut wrote:
>> The problem was caused by a global variable being used early in the boot
>> process.
>>
>> The symptoms were on imx31_phycore board, reading the environment from I2C
>> EEPROM didn't work correctly and causes default environment to be loaded.
>>
>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>> Cc: Wolfgang Denk <wd@denx.de>
>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> Cc: Heiko Schocher <hs@denx.de>
>> Cc: Stefano Babic <sbabic@denx.de>
>> ---
> 
> Perfect ! As the patch war already acked and tested, it should be not a
> problem if I already merge it into u-boot-imx.
> 
> Applied to u-boot-imx, thanks.

If it is not too late:

Acked-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH V3] I2C: Fix mxc_i2c.c problem on imx31_phycore

[Stefano Babic]

On 10/26/2011 01:17 PM, Heiko Schocher wrote:
> Hello Stefano,
> 
> Stefano Babic wrote:
>> On 10/26/2011 12:05 PM, Marek Vasut wrote:
>>> The problem was caused by a global variable being used early in the boot
>>> process.
>>>
>>> The symptoms were on imx31_phycore board, reading the environment from I2C
>>> EEPROM didn't work correctly and causes default environment to be loaded.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>>> Cc: Wolfgang Denk <wd@denx.de>
>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>> Cc: Heiko Schocher <hs@denx.de>
>>> Cc: Stefano Babic <sbabic@denx.de>
>>> ---
>>
>> Perfect ! As the patch war already acked and tested, it should be not a
>> problem if I already merge it into u-boot-imx.
>>
>> Applied to u-boot-imx, thanks.
> 
> If it is not too late:
> 

Not too late, I have added your acked-by.

Thanks,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================