mxc_i2c: specify i2c base address in config file

PATCH New Troy Kisky 7 participants 20 messages Apr 25, 2012 – Jul 16, 2012

Subsystems (3)

Arm (32-bit) Arm i.MX and Layerscape I2C

lore

Share & tools

lore

Troy Kisky

author

2012-04-25 03:33 UTC #0 lore raw

PATCH 1/3 mxc_i2c: specify i2c base address in config file

The following platforms had their config files changed flea3, imx31_phycore, mx35pdk, mx53ard, mx53evk, mx53smd

Signed-off-by Troy Kisky <[email protected]>

---
 arch/arm/include/asm/arch-mx31/imx-regs.h |    7 +++++++
 arch/arm/include/asm/arch-mx35/imx-regs.h |    2 +-
 drivers/i2c/mxc_i2c.c                     |   25 ++++---------------------
 include/configs/flea3.h                   |    2 +-
 include/configs/imx31_phycore.h           |    3 ++-
 include/configs/mx35pdk.h                 |    2 +-
 include/configs/mx53ard.h                 |    2 +-
 include/configs/mx53evk.h                 |    2 +-
 include/configs/mx53smd.h                 |    2 +-
 9 files changed, 19 insertions(+), 28 deletions(-)

arch/arm/include/asm/arch-mx31/imx-regs.h +7 -0

index 6454acb..7ddbbd6 100644
@@ -606,6 +606,13 @@ struct esdc_regs {
 #define UART4_BASE	0x43FB0000
 #define UART5_BASE	0x43FB4000
 
+#define I2C1_BASE_ADDR          0x43f80000
+#define I2C1_CLK_OFFSET		26
+#define I2C2_BASE_ADDR          0x43F98000
+#define I2C2_CLK_OFFSET		28
+#define I2C3_BASE_ADDR          0x43f84000
+#define I2C3_CLK_OFFSET		30
+
 #define ESDCTL_SDE			(1 << 31)
 #define ESDCTL_CMD_RW			(0 << 28)
 #define ESDCTL_CMD_PRECHARGE		(1 << 28)

arch/arm/include/asm/arch-mx35/imx-regs.h +1 -1

index e570ad1..3146006 100644
@@ -39,7 +39,7 @@
 #define MAX_BASE_ADDR           0x43F04000
 #define EVTMON_BASE_ADDR        0x43F08000
 #define CLKCTL_BASE_ADDR        0x43F0C000
-#define I2C_BASE_ADDR           0x43F80000
+#define I2C1_BASE_ADDR		0x43F80000
 #define I2C3_BASE_ADDR          0x43F84000
 #define ATA_BASE_ADDR           0x43F8C000
 #define UART1_BASE		0x43F90000

drivers/i2c/mxc_i2c.c +4 ~~-21~~

index c88ac7c..416ffee 100644
@@ -59,27 +59,10 @@ struct mxc_i2c_regs {
 #define I2SR_IIF	(1 << 1)
 #define I2SR_RX_NO_AK	(1 << 0)
 
-#if defined(CONFIG_SYS_I2C_MX31_PORT1)
-#define I2C_BASE	0x43f80000
-#define I2C_CLK_OFFSET	26
-#elif defined (CONFIG_SYS_I2C_MX31_PORT2)
-#define I2C_BASE	0x43f98000
-#define I2C_CLK_OFFSET	28
-#elif defined (CONFIG_SYS_I2C_MX31_PORT3)
-#define I2C_BASE	0x43f84000
-#define I2C_CLK_OFFSET	30
-#elif defined(CONFIG_SYS_I2C_MX53_PORT1)
-#define I2C_BASE        I2C1_BASE_ADDR
-#elif defined(CONFIG_SYS_I2C_MX53_PORT2)
-#define I2C_BASE        I2C2_BASE_ADDR
-#elif defined(CONFIG_SYS_I2C_MX35_PORT1)
-#define I2C_BASE	I2C_BASE_ADDR
-#elif defined(CONFIG_SYS_I2C_MX35_PORT2)
-#define I2C_BASE	I2C2_BASE_ADDR
-#elif defined(CONFIG_SYS_I2C_MX35_PORT3)
-#define I2C_BASE	I2C3_BASE_ADDR
+#ifdef CONFIG_SYS_I2C_BASE
+#define I2C_BASE	CONFIG_SYS_I2C_BASE
 #else
-#error "define CONFIG_SYS_I2C_MX<Processor>_PORTx to use the mx I2C driver"
+#error "define CONFIG_SYS_I2C_BASE to use the mxc_i2c driver"
 #endif
 
 #define I2C_MAX_TIMEOUT		10000

@@ -114,7 +97,7 @@ static uint8_t i2c_imx_get_clk(unsigned int rate)
 		(struct clock_control_regs *)CCM_BASE;
 
 	/* start the required I2C clock */
-	writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET),
+	writel(readl(&sc_regs->cgr0) | (3 << CONFIG_SYS_I2C_CLK_OFFSET),
 		&sc_regs->cgr0);
 #endif

include/configs/flea3.h +1 -1

index f046a58..75330c4 100644
@@ -68,7 +68,7 @@
  */
 #define CONFIG_HARD_I2C
 #define CONFIG_I2C_MXC
-#define CONFIG_SYS_I2C_MX35_PORT3
+#define CONFIG_SYS_I2C_BASE		I2C3_BASE_ADDR
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_SYS_I2C_SLAVE		0xfe
 #define CONFIG_MXC_SPI

include/configs/imx31_phycore.h +2 -1

index 3153eb5..197cefa 100644
@@ -54,7 +54,8 @@
 
 #define CONFIG_HARD_I2C
 #define CONFIG_I2C_MXC
-#define CONFIG_SYS_I2C_MX31_PORT2
+#define CONFIG_SYS_I2C_BASE		I2C2_BASE_ADDR
+#define CONFIG_SYS_I2C_CLK_OFFSET	I2C2_CLK_OFFSET
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_SYS_I2C_SLAVE		0xfe

include/configs/mx35pdk.h +1 -1

index de4b954..dfe39b8 100644
@@ -59,7 +59,7 @@
  */
 #define CONFIG_HARD_I2C
 #define CONFIG_I2C_MXC
-#define CONFIG_SYS_I2C_MX35_PORT1
+#define CONFIG_SYS_I2C_BASE		I2C1_BASE_ADDR
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_SYS_I2C_SLAVE		0xfe
 #define CONFIG_MXC_SPI

include/configs/mx53ard.h +1 -1

index f48a41e..0bdf6a3 100644
@@ -50,7 +50,7 @@
 #define CONFIG_CMD_I2C
 #define CONFIG_HARD_I2C
 #define CONFIG_I2C_MXC
-#define CONFIG_SYS_I2C_MX53_PORT2
+#define CONFIG_SYS_I2C_BASE		I2C2_BASE_ADDR
 #define CONFIG_SYS_I2C_SPEED            100000
 #define CONFIG_SYS_I2C_SLAVE            0xfe

include/configs/mx53evk.h +1 -1

index a77e5b2..3c7c329 100644
@@ -53,7 +53,7 @@
 #define CONFIG_CMD_I2C
 #define CONFIG_HARD_I2C
 #define CONFIG_I2C_MXC
-#define CONFIG_SYS_I2C_MX53_PORT2       1
+#define CONFIG_SYS_I2C_BASE		I2C2_BASE_ADDR
 #define CONFIG_SYS_I2C_SPEED            100000
 #define CONFIG_SYS_I2C_SLAVE            0xfe

include/configs/mx53smd.h +1 -1

index a04db3f..a904130 100644
@@ -50,7 +50,7 @@
 #define CONFIG_CMD_I2C
 #define CONFIG_HARD_I2C
 #define CONFIG_I2C_MXC
-#define CONFIG_SYS_I2C_MX53_PORT2
+#define CONFIG_SYS_I2C_BASE		I2C2_BASE_ADDR
 #define CONFIG_SYS_I2C_SPEED            100000
 #define CONFIG_SYS_I2C_SLAVE            0xfe

Signature · 1 line

1.7.5.4

Troy Kisky

author

2012-04-25 03:33 UTC #1 lore raw

PATCH 2/3 mx6qsabrelite: add i2c support

Signed-off-by Troy Kisky <[email protected]>

---
 board/freescale/mx6qsabrelite/mx6qsabrelite.c |   11 +++++++++++
 include/configs/mx6qsabrelite.h               |    8 ++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

board/freescale/mx6qsabrelite/mx6qsabrelite.c +11 -0

index 1d09a72..8690f22 100644
@@ -50,6 +50,11 @@ DECLARE_GLOBAL_DATA_PTR;
 	PAD_CTL_PUS_100K_DOWN | PAD_CTL_SPEED_MED |		\
 	PAD_CTL_DSE_40ohm     | PAD_CTL_SRE_FAST)
 
+#define I2C_PAD_CTRL	(PAD_CTL_PKE | PAD_CTL_PUE |		\
+	PAD_CTL_PUS_100K_UP | PAD_CTL_SPEED_MED |		\
+	PAD_CTL_DSE_40ohm | PAD_CTL_HYS |			\
+	PAD_CTL_ODE | PAD_CTL_SRE_FAST)
+
 int dram_init(void)
 {
        gd->ram_size = get_ram_size((void *)PHYS_SDRAM, PHYS_SDRAM_SIZE);

@@ -67,6 +72,11 @@ iomux_v3_cfg_t uart2_pads[] = {
        MX6Q_PAD_EIM_D27__UART2_RXD | MUX_PAD_CTRL(UART_PAD_CTRL),
 };
 
+iomux_v3_cfg_t i2c3_pads[] = {
+	MX6Q_PAD_GPIO_5__I2C3_SCL | MUX_PAD_CTRL(I2C_PAD_CTRL),
+	MX6Q_PAD_GPIO_16__I2C3_SDA | MUX_PAD_CTRL(I2C_PAD_CTRL),
+};
+
 iomux_v3_cfg_t usdhc3_pads[] = {
        MX6Q_PAD_SD3_CLK__USDHC3_CLK   | MUX_PAD_CTRL(USDHC_PAD_CTRL),
        MX6Q_PAD_SD3_CMD__USDHC3_CMD   | MUX_PAD_CTRL(USDHC_PAD_CTRL),

@@ -282,6 +292,7 @@ int board_init(void)
 #ifdef CONFIG_MXC_SPI
 	setup_spi();
 #endif
+	imx_iomux_v3_setup_multiple_pads(i2c3_pads, ARRAY_SIZE(i2c3_pads));
 
        return 0;
 }

include/configs/mx6qsabrelite.h +8 -0

index f52c3c7..311494b 100644
@@ -58,6 +58,14 @@
 #define CONFIG_SF_DEFAULT_MODE (SPI_MODE_0)
 #endif
 
+/* I2C Configs */
+#define CONFIG_CMD_I2C
+#define CONFIG_HARD_I2C
+#define CONFIG_I2C_MXC
+#define CONFIG_SYS_I2C_BASE		I2C3_BASE_ADDR
+#define CONFIG_SYS_I2C_SPEED            100000
+#define CONFIG_SYS_I2C_SLAVE            0xfe
+
 /* MMC Configs */
 #define CONFIG_FSL_ESDHC
 #define CONFIG_FSL_USDHC

Signature · 1 line

1.7.5.4

Stefano Babic

2012-05-06 15:24 UTC #2 lore raw

PATCH 2/3 mx6qsabrelite: add i2c support

Quoted 6 lines · Troy Kisky

Signed-off-by Troy Kisky <[email protected]>

---
 board/freescale/mx6qsabrelite/mx6qsabrelite.c |   11 +++++++++++
 include/configs/mx6qsabrelite.h               |    8 ++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

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

Troy Kisky

author

2012-04-25 03:33 UTC #3 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

In reply to Troy Kisky

Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.

Signed-off-by Troy Kisky <[email protected]>

---
 drivers/i2c/mxc_i2c.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

drivers/i2c/mxc_i2c.c +0 -6

index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx, &i2c_regs->ifdr);
 
 	/* Enable I2C controller */
 	writeb(0, &i2c_regs->i2sr);

Signature · 1 line

1.7.5.4

Troy Kisky

author

2012-05-04 22:52 UTC #4 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Quoted 18 lines · Troy Kisky

Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
  drivers/i2c/mxc_i2c.c |    6 ------
  1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -0
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ int i2c_imx_start(void)
  	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
  	unsigned int temp = 0;

drivers/i2c/mxc_i2c.c int i2c_imx_start(void) · line 236

Quoted 10 lines

drivers/i2c/mxc_i2c.c +0 -6

  	int result;
-	int speed = i2c_get_bus_speed();
-	u8 clk_idx = i2c_imx_get_clk(speed);
-	u8 idx = i2c_clk_div[clk_idx][1];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 
  	/* Enable I2C controller */
  	writeb(0,&i2c_regs->i2sr);

Marek would you care to ack/nak this? It is deleting code that you added.

Thanks
Troy

Marek Vasut

2012-05-05 13:05 UTC #5 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Dear Troy Kisky,

Quoted 30 lines · Troy Kisky

Quoted 29 lines
Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
  drivers/i2c/mxc_i2c.c |    6 ------
  1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -6
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 

Quoted 4 lines

Quoted 2 lines
  	/* Enable I2C controller */
  	writeb(0,&i2c_regs->i2sr);
Marek would you care to ack/nak this? It is deleting code that you added.

Lemme check

Quoted 3 lines

Thanks
Troy

Best regards,
Marek Vasut

Marek Vasut

2012-05-05 13:08 UTC #6 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

In reply to Troy Kisky

Dear Troy Kisky,

Quoted 30 lines · Troy Kisky

Quoted 29 lines
Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
  drivers/i2c/mxc_i2c.c |    6 ------
  1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -6
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 

Quoted 4 lines

Quoted 2 lines
  	/* Enable I2C controller */
  	writeb(0,&i2c_regs->i2sr);
Marek would you care to ack/nak this? It is deleting code that you added.

Ok, who will set the controller speed if you remove this?

Quoted 3 lines

Thanks
Troy

Best regards,
Marek Vasut

Troy Kisky

author

2012-05-05 20:22 UTC #7 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Quoted 32 lines · Marek Vasut

Dear Troy Kisky,
Quoted 30 lines · Troy Kisky
Quoted 29 lines
Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
   drivers/i2c/mxc_i2c.c |    6 ------
   1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -6
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 

Quoted 4 lines

Quoted 3 lines
Quoted 2 lines
   	/* Enable I2C controller */
   	writeb(0,&i2c_regs->i2sr);
Marek would you care to ack/nak this? It is deleting code that you added.
Ok, who will set the controller speed if you remove this?

i2c_init is the only function that writes the ifdr register after this patch.
This is fine because this register is not affected by a software reset. If this register were affected by a software reset, then the current code would not work either, as i2c_imx_start is reading from this register before trying(and often failing) to set it to the same value.

Quoted 6 lines

Quoted 3 lines
Thanks
Troy
Best regards,
Marek Vasut

Marek Vasut

2012-05-05 21:36 UTC #8 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Dear Troy Kisky,

Quoted 39 lines · Marek Vasut

Quoted 38 lines
Dear Troy Kisky,
Quoted 35 lines · Troy Kisky
Quoted 29 lines
Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
   drivers/i2c/mxc_i2c.c |    6 ------
   1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -6
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 
Quoted 2 lines
   	/* Enable I2C controller */
   	writeb(0,&i2c_regs->i2sr);
Marek would you care to ack/nak this? It is deleting code that you added.

Quoted 4 lines

Quoted 1 line
Ok, who will set the controller speed if you remove this?
i2c_init is the only function that writes the ifdr register after this patch.

And i2c_init() is called on every boot. Correct?

Quoted 1 line

This is fine because this register is not affected by a software reset.

I take it you verified this or that you're sure here :)

Quoted 3 lines

If this register were affected by a software reset, then the current code would not work either, as i2c_imx_start is reading from this register before trying(and often failing) to set it to the same value.

Reading from ifdr? That seems indeed wrong :-(

Well ... I have no objection then

Acked-by Marek Vasut <[email protected]>

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
Thanks
Troy
Best regards,
Marek Vasut

Best regards,
Marek Vasut

Troy Kisky

author

2012-05-05 22:26 UTC #9 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Quoted 39 lines · Marek Vasut

Dear Troy Kisky,
Quoted 37 lines · Marek Vasut
Quoted 36 lines
Dear Troy Kisky,
Quoted 34 lines · Troy Kisky
Quoted 29 lines
Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
    drivers/i2c/mxc_i2c.c |    6 ------
    1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -6
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 
Quoted 2 lines
    	/* Enable I2C controller */
    	writeb(0,&i2c_regs->i2sr);
Marek would you care to ack/nak this? It is deleting code that you added.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Ok, who will set the controller speed if you remove this?
i2c_init is the only function that writes the ifdr register after this patch.
And i2c_init() is called on every boot. Correct?
Quoted 1 line
This is fine because this register is not affected by a software reset.
I take it you verified this or that you're sure here :)

I haven't looked at every reference manual, but if some oddball chip needs it reinitialized after
a software reset, then the fix should go into the i2c_reset function, not i2c_imx_start.
And this patch would not be introducing a regression for the oddball chip anyway.

Troy

Marek Vasut

2012-05-05 23:06 UTC #10 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Dear Troy Kisky,

Quoted 51 lines · Marek Vasut

Quoted 50 lines
Dear Troy Kisky,
Quoted 39 lines · Marek Vasut
Quoted 38 lines
Dear Troy Kisky,
Quoted 35 lines · Troy Kisky
Quoted 29 lines
Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
    drivers/i2c/mxc_i2c.c |    6 ------
    1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -6
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 
Quoted 2 lines
    	/* Enable I2C controller */
    	writeb(0,&i2c_regs->i2sr);
Marek would you care to ack/nak this? It is deleting code that you added.
Quoted 4 lines
Quoted 1 line
Ok, who will set the controller speed if you remove this?
i2c_init is the only function that writes the ifdr register after this patch.
And i2c_init() is called on every boot. Correct?
Quoted 1 line
This is fine because this register is not affected by a software reset.

Quoted 8 lines

Quoted 1 line
I take it you verified this or that you're sure here :)
I haven't looked at every reference manual, but if some oddball chip needs it reinitialized after
a software reset, then the fix should go into the i2c_reset function, not i2c_imx_start.
And this patch would not be introducing a regression for the oddball chip anyway.

Agreed, I think I already acked this, sorry for the delay :)

Quoted 2 lines

Troy

Best regards,
Marek Vasut

Stefano Babic

2012-05-06 15:27 UTC #11 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Quoted 61 lines · Marek Vasut

Dear Troy Kisky,
Quoted 59 lines · Marek Vasut
Quoted 43 lines
Dear Troy Kisky,
Quoted 41 lines · Marek Vasut
Quoted 39 lines
Dear Troy Kisky,
Quoted 30 lines · Troy Kisky
Quoted 29 lines
Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
    drivers/i2c/mxc_i2c.c |    6 ------
    1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -6
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 
Quoted 5 lines
Quoted 2 lines
    	/* Enable I2C controller */
    	writeb(0,&i2c_regs->i2sr);
Marek would you care to ack/nak this? It is deleting code that you added.
Ok, who will set the controller speed if you remove this?
Quoted 8 lines
Quoted 2 lines
i2c_init is the only function that writes the ifdr register after this patch.
And i2c_init() is called on every boot. Correct?
Quoted 1 line
This is fine because this register is not affected by a software reset.
I take it you verified this or that you're sure here :)
I haven't looked at every reference manual, but if some oddball chip needs it reinitialized after
a software reset, then the fix should go into the i2c_reset function, not i2c_imx_start.
And this patch would not be introducing a regression for the oddball chip anyway.

Ok, everything was already clear, and I can also add my:

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

Dirk Behme

2012-07-11 06:27 UTC #12 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Quoted 57 lines · Stefano Babic

Quoted 55 lines · Marek Vasut
Dear Troy Kisky,
Quoted 53 lines · Marek Vasut
Quoted 39 lines
Dear Troy Kisky,
Quoted 37 lines · Marek Vasut
Quoted 36 lines
Dear Troy Kisky,
Quoted 34 lines · Troy Kisky
Quoted 29 lines
Other then being very weird, this code was also wrong. For example, say I set speed to 100K. I'll read back the speed as 85937. But the speed is really 85937.5, so we I reset the speed to 85937, I'll get 73660.7. After a couple of transactions my speed is now exactly 68750 so it will remain there.
Signed-off-by Troy Kisky<[email protected]>
---
    drivers/i2c/mxc_i2c.c |    6 ------
    1 files changed, 0 insertions(+), 6 deletions(-)
drivers/i2c/mxc_i2c.c +0 -6
index 416ffee..fc68062 100644
@@ -231,12 +231,6 @@ 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];
-
-	/* Store divider value */
-	writeb(idx,&i2c_regs->ifdr);
 
Quoted 2 lines
    	/* Enable I2C controller */
    	writeb(0,&i2c_regs->i2sr);
Marek would you care to ack/nak this? It is deleting code that you added.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Ok, who will set the controller speed if you remove this?
i2c_init is the only function that writes the ifdr register after this patch.
And i2c_init() is called on every boot. Correct?
Quoted 1 line
This is fine because this register is not affected by a software reset.
I take it you verified this or that you're sure here :)
I haven't looked at every reference manual, but if some oddball chip needs it reinitialized after
a software reset, then the fix should go into the i2c_reset function, not i2c_imx_start.
And this patch would not be introducing a regression for the oddball chip anyway.

Quoted 3 lines

Ok, everything was already clear, and I can also add my:
Acked-by Stefano Babic <[email protected]>

Same for this one: Is this applied anywhere? It doesn't seem to be part of v2012.07-rc1?

Best regards

Dirk

Marek Vasut

2012-07-13 10:38 UTC #13 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Dear Dirk Behme,

[...]

Quoted 8 lines

Quoted 3 lines
Ok, everything was already clear, and I can also add my:
Acked-by Stefano Babic <[email protected]>
Same for this one: Is this applied anywhere? It doesn't seem to be part of v2012.07-rc1?
Best regards

Stefano, once you're out of mont blanc, can you apply this please?

Best regards,
Marek Vasut

Wolfgang Denk

2012-07-13 11:56 UTC #14 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Dear Marek Vasut,

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

Quoted 9 lines
Quoted 4 lines
Ok, everything was already clear, and I can also add my:
Acked-by Stefano Babic <[email protected]>
Same for this one: Is this applied anywhere? It doesn't seem to be part of v2012.07-rc1?
Best regards
Stefano, once you're out of mont blanc, can you apply this please?

This is in mainline:

9ca37d7 2012-07-11 10:54:52 +0200 mxc_i2c: remove setting speed at each start

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
The rule on staying alive as a program manager is to give 'em a  num-
ber or give 'em a date, but never give 'em both at once.

Marek Vasut

2012-07-13 12:11 UTC #15 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

Dear Wolfgang Denk,

Quoted 10 lines

Dear Marek Vasut,
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 7 lines
Quoted 3 lines
Ok, everything was already clear, and I can also add my:
Acked-by Stefano Babic <[email protected]>
Same for this one: Is this applied anywhere? It doesn't seem to be part of v2012.07-rc1?

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Best regards
Stefano, once you're out of mont blanc, can you apply this please?
This is in mainline:
9ca37d7 2012-07-11 10:54:52 +0200 mxc_i2c: remove setting speed at each
start

Ok, sorry for the noise then.

Quoted 3 lines

Best regards,
Wolfgang Denk

Best regards,
Marek Vasut

Stefano Babic

2012-07-16 09:39 UTC #16 lore raw

PATCH 3/3 mxc_i2c: remove setting speed at each start

In reply to Marek Vasut

Quoted 6 lines · Marek Vasut

Dear Dirk Behme,
[...]
Quoted 2 lines
Quoted 2 lines
Ok, everything was already clear, and I can also add my:

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Acked-by Stefano Babic <[email protected]>
Same for this one: Is this applied anywhere? It doesn't seem to be part of v2012.07-rc1?
Best regards
Stefano, once you're out of mont blanc, can you apply this please?

It is applied into u-boot-i2c, it will be merged directly into mainline ny next Heiko's pull request.

Best regards,
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-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Stefano Babic

2012-05-06 15:27 UTC #17 lore raw

PATCH 1/3 mxc_i2c: specify i2c base address in config file

In reply to Troy Kisky

Quoted 5 lines · Troy Kisky

The following platforms had their config files changed flea3, imx31_phycore, mx35pdk, mx53ard, mx53evk, mx53smd
Signed-off-by Troy Kisky <[email protected]>
---

Hi Troy,

Quoted 11 lines

 arch/arm/include/asm/arch-mx31/imx-regs.h |    7 +++++++
 arch/arm/include/asm/arch-mx35/imx-regs.h |    2 +-
 drivers/i2c/mxc_i2c.c                     |   25 ++++---------------------
 include/configs/flea3.h                   |    2 +-
 include/configs/imx31_phycore.h           |    3 ++-
 include/configs/mx35pdk.h                 |    2 +-
 include/configs/mx53ard.h                 |    2 +-
 include/configs/mx53evk.h                 |    2 +-
 include/configs/mx53smd.h                 |    2 +-
 9 files changed, 19 insertions(+), 28 deletions(-)

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

Dirk Behme

2012-07-11 06:25 UTC #18 lore raw

PATCH 1/3 mxc_i2c: specify i2c base address in config file

Quoted 21 lines · Stefano Babic

Quoted 5 lines · Troy Kisky

The following platforms had their config files changed flea3, imx31_phycore, mx35pdk, mx53ard, mx53evk, mx53smd
Signed-off-by Troy Kisky <[email protected]>
---

Hi Troy,

Quoted 11 lines

 arch/arm/include/asm/arch-mx31/imx-regs.h |    7 +++++++
 arch/arm/include/asm/arch-mx35/imx-regs.h |    2 +-
 drivers/i2c/mxc_i2c.c                     |   25 ++++---------------------
 include/configs/flea3.h                   |    2 +-
 include/configs/imx31_phycore.h           |    3 ++-
 include/configs/mx35pdk.h                 |    2 +-
 include/configs/mx53ard.h                 |    2 +-
 include/configs/mx53evk.h                 |    2 +-
 include/configs/mx53smd.h                 |    2 +-
 9 files changed, 19 insertions(+), 28 deletions(-)

Quoted 1 line

Acked-by Stefano Babic <[email protected]>

Hmm, is this already applied anywhere? It doesn't seem to be included in v2012.07-rc1?

Best regards

Dirk

Heiko Schocher

2012-07-11 08:35 UTC #19 lore raw

PATCH 1/3 mxc_i2c: specify i2c base address in config file

Hello Dirk,

Quoted 22 lines · Dirk Behme

Quoted 21 lines · Stefano Babic
Quoted 5 lines · Troy Kisky
The following platforms had their config files changed flea3, imx31_phycore, mx35pdk, mx53ard, mx53evk, mx53smd
Signed-off-by Troy Kisky <[email protected]>
---
Hi Troy,
Quoted 11 lines
arch/arm/include/asm/arch-mx31/imx-regs.h | 7 +++++++ arch/arm/include/asm/arch-mx35/imx-regs.h | 2 +- drivers/i2c/mxc_i2c.c | 25 ++++--------------------- include/configs/flea3.h | 2 +- include/configs/imx31_phycore.h | 3 ++-
include/configs/mx35pdk.h | 2 +-
include/configs/mx53ard.h | 2 +-
include/configs/mx53evk.h | 2 +-
include/configs/mx53smd.h | 2 +-
9 files changed, 19 insertions(+), 28 deletions(-)

Quoted 3 lines

Quoted 1 line
Acked-by Stefano Babic <[email protected]>
Hmm, is this already applied anywhere? It doesn't seem to be included in v2012.07-rc1?

Yes, it is applied in:

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=summary

But I could not found my pull request to Wolfgang :-(

Currently rebased to current master, send pull reqeust soon.

Thanks for detecting this!

bye,
Heiko

Signature · 2 lines

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