I2C: Fix mxc_i2c.c problem on phycore pcm037

PATCH Superseded Marek Vasut 4 participants 10 messages Oct 25, 2011 – Oct 26, 2011

Subsystems (2)

Arm i.MX and Layerscape I2C

lore

Share & tools

lore

Marek Vasut

author

2011-10-25 12:09 UTC #0 lore raw

PATCH 1/1 I2C: Fix mxc_i2c.c problem on phycore pcm037

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

Signed-off-by Marek Vasut <[email protected]>

Cc Anatolij Gustschin <[email protected]>

Cc Wolfgang Denk <[email protected]>

Cc Albert ARIBAUD <[email protected]>

Cc Heiko Schocher <[email protected]>

Cc Stefano Babic <[email protected]>

---
 drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

drivers/i2c/mxc_i2c.c +16 ~~-13~~

index a805bf6..360c54c 100644
@@ -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)

Signature · 1 line

1.7.6.3

Anatolij Gustschin

2011-10-25 14:59 UTC #1 lore raw

PATCH 1/1 I2C: Fix mxc_i2c.c problem on phycore pcm037

Hi Marek,

Quoted 12 lines · On Tue, 25 Oct 2011 14:09:11 +0200 Marek Vasut <[email protected]> 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 <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Albert ARIBAUD <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Stefano Babic <[email protected]>

Quoted 3 lines

---
 drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

Great, this patch fixes the issue! Thanks!

Tested-by Anatolij Gustschin <[email protected]>

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

Marek Vasut

author

2011-10-25 15:25 UTC #2 lore raw

PATCH v2 1/1 I2C: Fix mxc_i2c.c problem on imx31_phycore

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

Cc Wolfgang Denk <[email protected]>

Cc Albert ARIBAUD <[email protected]>

Cc Heiko Schocher <[email protected]>

Cc Stefano Babic <[email protected]>

Tested-by Anatolij Gustschin <[email protected]>

---
 drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

V2: Fix commit message

drivers/i2c/mxc_i2c.c +16 ~~-13~~

index a805bf6..360c54c 100644
@@ -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)

Signature · 1 line

1.7.6.3

Stefano Babic

2011-10-26 08:25 UTC #3 lore raw

PATCH v2 1/1 I2C: Fix mxc_i2c.c problem on imx31_phycore

Quoted 12 lines · 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 <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Albert ARIBAUD <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Stefano Babic <[email protected]>
Tested-by Anatolij Gustschin <[email protected]>

Quoted 5 lines

---
 drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

V2: Fix commit message

Tested on mx35pdk.

Tested-by Stefano Babic <[email protected]>

Best regards,
Stefano Babic

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-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

Stefano Babic

2011-10-26 08:59 UTC #4 lore raw

PATCH v2 1/1 I2C: Fix mxc_i2c.c problem on imx31_phycore

Quoted 17 lines · Stefano Babic

Quoted 16 lines · 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 <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Albert ARIBAUD <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Stefano Babic <[email protected]>
Tested-by Anatolij Gustschin <[email protected]>
---
 drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

Quoted 5 lines

Quoted 1 line
V2: Fix commit message
Tested on mx35pdk.
Tested-by Stefano Babic <[email protected]>

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

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-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

Marek Vasut

author

2011-10-26 10:08 UTC #5 lore raw

PATCH v2 1/1 I2C: Fix mxc_i2c.c problem on imx31_phycore

Quoted 38 lines · Stefano Babic

Quoted 16 lines · Marek Vasut
Quoted 15 lines
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 <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Albert ARIBAUD <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Stefano Babic <[email protected]>
Tested-by Anatolij Gustschin <[email protected]>
---
Quoted 8 lines
Quoted 4 lines
 drivers/i2c/mxc_i2c.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)
V2: Fix commit message
Tested on mx35pdk.
Tested-by Stefano Babic <[email protected]>
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.

Quoted 5 lines

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

Marek Vasut

author

2011-10-26 10:05 UTC #6 lore raw

PATCH v3 1/1 I2C: Fix mxc_i2c.c problem on imx31_phycore

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

Cc Wolfgang Denk <[email protected]>

Cc Albert ARIBAUD <[email protected]>

Cc Heiko Schocher <[email protected]>

Cc Stefano Babic <[email protected]>

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

drivers/i2c/mxc_i2c.c +26 ~~-11~~

index bf3ad95..c88ac7c 100644
@@ -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)

Signature · 1 line

1.7.6.3

Stefano Babic

2011-10-26 10:53 UTC #7 lore raw

PATCH v3 1/1 I2C: Fix mxc_i2c.c problem on imx31_phycore

Quoted 6 lines · 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.

Quoted 6 lines

Signed-off-by Marek Vasut <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Albert ARIBAUD <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Stefano Babic <[email protected]>
---

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

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-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

Heiko Schocher

2011-10-26 11:17 UTC #8 lore raw

PATCH v3 1/1 I2C: Fix mxc_i2c.c problem on imx31_phycore

Hello Stefano,

Quoted 14 lines · Stefano Babic

Quoted 6 lines · 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.
Quoted 6 lines
Signed-off-by Marek Vasut <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Albert ARIBAUD <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Stefano Babic <[email protected]>
---

Quoted 4 lines

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

bye,
Heiko

Signature · 2 lines

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

Stefano Babic

2011-10-26 13:26 UTC #9 lore raw

PATCH v3 1/1 I2C: Fix mxc_i2c.c problem on imx31_phycore

Quoted 17 lines · Heiko Schocher

Hello Stefano,
Quoted 14 lines · Stefano Babic
Quoted 12 lines · 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 <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Albert ARIBAUD <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Stefano Babic <[email protected]>
---

Quoted 7 lines

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

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-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================