[U-Boot] [PATCH] mxs: i2c: Implement algorithm to set up arbitrary i2c speed

[U-Boot] [PATCH] mxs: i2c: Implement algorithm to set up arbitrary i2c speed

[Marek Vasut]

This algorithm computes the values of TIMING{0,1,2} registers for the
MX28 I2C block. This algorithm was derived by using a scope, but the
result seems correct.

The resulting values programmed into the registers do not correlate
with the contents of the datasheet. When using the values from the
datasheet, the I2C clock were completely wrong.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Heiko Schocher <hs@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/i2c/mxs_i2c.c |   73 ++++++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 52 deletions(-)

NOTE: Heiko, please apply it onto your u-boot-i2c tree, it's dependent on the
patches that are already there.

diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c
index 006fb91..7b3eec5 100644
--- a/drivers/i2c/mxs_i2c.c
+++ b/drivers/i2c/mxs_i2c.c
@@ -28,6 +28,7 @@
 
 #include <common.h>
 #include <malloc.h>
+#include <i2c.h>
 #include <asm/errno.h>
 #include <asm/io.h>
 #include <asm/arch/clock.h>
@@ -40,6 +41,7 @@ void mxs_i2c_reset(void)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
 	int ret;
+	int speed = i2c_get_bus_speed();
 
 	ret = mxs_reset_block(&i2c_regs->hw_i2c_ctrl0_reg);
 	if (ret) {
@@ -53,6 +55,8 @@ void mxs_i2c_reset(void)
 		&i2c_regs->hw_i2c_ctrl1_clr);
 
 	writel(I2C_QUEUECTRL_PIO_QUEUE_MODE, &i2c_regs->hw_i2c_queuectrl_set);
+
+	i2c_set_bus_speed(speed);
 }
 
 void mxs_i2c_setup_read(uint8_t chip, int len)
@@ -210,62 +214,28 @@ int i2c_probe(uchar chip)
 	return ret;
 }
 
-static struct mxs_i2c_speed_table {
-	uint32_t	speed;
-	uint32_t	timing0;
-	uint32_t	timing1;
-} mxs_i2c_tbl[] = {
-	{
-		100000,
-		(0x0078 << I2C_TIMING0_HIGH_COUNT_OFFSET) |
-		(0x0030 << I2C_TIMING0_RCV_COUNT_OFFSET),
-		(0x0080 << I2C_TIMING1_LOW_COUNT_OFFSET) |
-		(0x0030 << I2C_TIMING1_XMIT_COUNT_OFFSET)
-	},
-	{
-		400000,
-		(0x000f << I2C_TIMING0_HIGH_COUNT_OFFSET) |
-		(0x0007 << I2C_TIMING0_RCV_COUNT_OFFSET),
-		(0x001f << I2C_TIMING1_LOW_COUNT_OFFSET) |
-		(0x000f << I2C_TIMING1_XMIT_COUNT_OFFSET),
-	}
-};
-
-static struct mxs_i2c_speed_table *mxs_i2c_speed_to_cfg(uint32_t speed)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(mxs_i2c_tbl); i++)
-		if (mxs_i2c_tbl[i].speed == speed)
-			return &mxs_i2c_tbl[i];
-	return NULL;
-}
-
-static uint32_t mxs_i2c_cfg_to_speed(uint32_t timing0, uint32_t timing1)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(mxs_i2c_tbl); i++) {
-		if (mxs_i2c_tbl[i].timing0 != timing0)
-			continue;
-		if (mxs_i2c_tbl[i].timing1 != timing1)
-			continue;
-		return mxs_i2c_tbl[i].speed;
-	}
-
-	return 0;
-}
-
 int i2c_set_bus_speed(unsigned int speed)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
-	struct mxs_i2c_speed_table *spd = mxs_i2c_speed_to_cfg(speed);
 
-	if (!spd) {
-		printf("MXS I2C: Invalid speed selected (%d Hz)\n", speed);
+	uint32_t base = ((24000000 / speed) - 38) / 2;
+	uint16_t high_count = base + 3;
+	uint16_t low_count = base - 3;
+	uint16_t rcv_count = (high_count * 3) / 4;
+	uint16_t xmit_count = low_count / 4;
+
+	if (speed > 540000) {
+		printf("MXS I2C: Speed too high (%d Hz)\n", speed);
+		return -EINVAL;
+	}
+
+	if (speed < 12000) {
+		printf("MXS I2C: Speed too low (%d Hz)\n", speed);
 		return -EINVAL;
 	}
 
-	writel(spd->timing0, &i2c_regs->hw_i2c_timing0);
-	writel(spd->timing1, &i2c_regs->hw_i2c_timing1);
+	writel((high_count << 16) | rcv_count, &i2c_regs->hw_i2c_timing0);
+	writel((low_count << 16) | xmit_count, &i2c_regs->hw_i2c_timing1);
 
 	writel((0x0030 << I2C_TIMING2_BUS_FREE_OFFSET) |
 		(0x0030 << I2C_TIMING2_LEADIN_COUNT_OFFSET),
@@ -277,12 +247,11 @@ int i2c_set_bus_speed(unsigned int speed)
 unsigned int i2c_get_bus_speed(void)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
-	uint32_t timing0, timing1;
+	uint32_t timing0;
 
 	timing0 = readl(&i2c_regs->hw_i2c_timing0);
-	timing1 = readl(&i2c_regs->hw_i2c_timing1);
 
-	return mxs_i2c_cfg_to_speed(timing0, timing1);
+	return 24000000 / ((((timing0 >> 16) - 3) * 2) + 38);
 }
 
 void i2c_init(int speed, int slaveadd)
-- 
1.7.10.4

[U-Boot] [PATCH] mxs: i2c: Implement algorithm to set up arbitrary i2c speed

[Wolfgang Denk]

Dear Marek Vasut,

In message <1354280910-17539-1-git-send-email-marex@denx.de> you wrote:
> This algorithm computes the values of TIMING{0,1,2} registers for the
> MX28 I2C block. This algorithm was derived by using a scope, but the
> result seems correct.

Thanks!  I like that!

...
> +	uint32_t base = ((24000000 / speed) - 38) / 2;
...
> +	return 24000000 / ((((timing0 >> 16) - 3) * 2) + 38);

But we should get rid of this magic constant.  On other i.MX systems
that would probably be MXC_HCLK ?

For iMX28, we have CONFIG_PL011_CLOCK in the board config files
instead.

Hm... grepping the source tree for 24000000 I get the feeling that
this really needs some cleanup....

Best regards,

Wolfgang Denk

-- 
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
Vulcans worship peace above all.
	-- McCoy, "Return to Tomorrow", stardate 4768.3

[U-Boot] [PATCH] mxs: i2c: Implement algorithm to set up arbitrary i2c speed

[Marek Vasut]

Dear Wolfgang Denk,

> Dear Marek Vasut,
> 
> In message <1354280910-17539-1-git-send-email-marex@denx.de> you wrote:
> > This algorithm computes the values of TIMING{0,1,2} registers for the
> > MX28 I2C block. This algorithm was derived by using a scope, but the
> > result seems correct.
> 
> Thanks!  I like that!
> 
> ...
> 
> > +	uint32_t base = ((24000000 / speed) - 38) / 2;
> 
> ...
> 
> > +	return 24000000 / ((((timing0 >> 16) - 3) * 2) + 38);
> 
> But we should get rid of this magic constant.  On other i.MX systems
> that would probably be MXC_HCLK ?

It's APBX clock ... you are so right ... I rolled the patch out too fast.
[...]

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] mxs: i2c: Implement algorithm to set up arbitrary i2c speed

[Marek Vasut]

This algorithm computes the values of TIMING{0,1,2} registers for the
MX28 I2C block. This algorithm was derived by using a scope, but the
result seems correct.

The resulting values programmed into the registers do not correlate
with the contents in datasheet. When using the values from the datasheet,
the I2C clock were completely wrong.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Wolfgang Denk <wd@denx.de>
---
 arch/arm/cpu/arm926ejs/mxs/clock.c    |    2 +
 arch/arm/include/asm/arch-mxs/clock.h |    1 +
 drivers/i2c/mxs_i2c.c                 |   75 ++++++++++-----------------------
 3 files changed, 26 insertions(+), 52 deletions(-)

v2: Properly implement XTAL clock retrieval. The i2c clock are derived from the
    24MHz XTAL clock.

diff --git a/arch/arm/cpu/arm926ejs/mxs/clock.c b/arch/arm/cpu/arm926ejs/mxs/clock.c
index bfea6ab..4ff19c3 100644
--- a/arch/arm/cpu/arm926ejs/mxs/clock.c
+++ b/arch/arm/cpu/arm926ejs/mxs/clock.c
@@ -333,6 +333,8 @@ uint32_t mxc_get_clock(enum mxc_clock clk)
 		return mx28_get_sspclk(MXC_SSPCLK2);
 	case MXC_SSP3_CLK:
 		return mx28_get_sspclk(MXC_SSPCLK3);
+	case MXC_XTAL_CLK:
+		return XTAL_FREQ_KHZ * 1000;
 	}
 
 	return 0;
diff --git a/arch/arm/include/asm/arch-mxs/clock.h b/arch/arm/include/asm/arch-mxs/clock.h
index 1700fe3..3d39ef2 100644
--- a/arch/arm/include/asm/arch-mxs/clock.h
+++ b/arch/arm/include/asm/arch-mxs/clock.h
@@ -35,6 +35,7 @@ enum mxc_clock {
 	MXC_SSP1_CLK,
 	MXC_SSP2_CLK,
 	MXC_SSP3_CLK,
+	MXC_XTAL_CLK,
 };
 
 enum mxs_ioclock {
diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c
index 006fb91..b040535 100644
--- a/drivers/i2c/mxs_i2c.c
+++ b/drivers/i2c/mxs_i2c.c
@@ -28,6 +28,7 @@
 
 #include <common.h>
 #include <malloc.h>
+#include <i2c.h>
 #include <asm/errno.h>
 #include <asm/io.h>
 #include <asm/arch/clock.h>
@@ -40,6 +41,7 @@ void mxs_i2c_reset(void)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
 	int ret;
+	int speed = i2c_get_bus_speed();
 
 	ret = mxs_reset_block(&i2c_regs->hw_i2c_ctrl0_reg);
 	if (ret) {
@@ -53,6 +55,8 @@ void mxs_i2c_reset(void)
 		&i2c_regs->hw_i2c_ctrl1_clr);
 
 	writel(I2C_QUEUECTRL_PIO_QUEUE_MODE, &i2c_regs->hw_i2c_queuectrl_set);
+
+	i2c_set_bus_speed(speed);
 }
 
 void mxs_i2c_setup_read(uint8_t chip, int len)
@@ -210,62 +214,29 @@ int i2c_probe(uchar chip)
 	return ret;
 }
 
-static struct mxs_i2c_speed_table {
-	uint32_t	speed;
-	uint32_t	timing0;
-	uint32_t	timing1;
-} mxs_i2c_tbl[] = {
-	{
-		100000,
-		(0x0078 << I2C_TIMING0_HIGH_COUNT_OFFSET) |
-		(0x0030 << I2C_TIMING0_RCV_COUNT_OFFSET),
-		(0x0080 << I2C_TIMING1_LOW_COUNT_OFFSET) |
-		(0x0030 << I2C_TIMING1_XMIT_COUNT_OFFSET)
-	},
-	{
-		400000,
-		(0x000f << I2C_TIMING0_HIGH_COUNT_OFFSET) |
-		(0x0007 << I2C_TIMING0_RCV_COUNT_OFFSET),
-		(0x001f << I2C_TIMING1_LOW_COUNT_OFFSET) |
-		(0x000f << I2C_TIMING1_XMIT_COUNT_OFFSET),
-	}
-};
-
-static struct mxs_i2c_speed_table *mxs_i2c_speed_to_cfg(uint32_t speed)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(mxs_i2c_tbl); i++)
-		if (mxs_i2c_tbl[i].speed == speed)
-			return &mxs_i2c_tbl[i];
-	return NULL;
-}
-
-static uint32_t mxs_i2c_cfg_to_speed(uint32_t timing0, uint32_t timing1)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(mxs_i2c_tbl); i++) {
-		if (mxs_i2c_tbl[i].timing0 != timing0)
-			continue;
-		if (mxs_i2c_tbl[i].timing1 != timing1)
-			continue;
-		return mxs_i2c_tbl[i].speed;
-	}
-
-	return 0;
-}
-
 int i2c_set_bus_speed(unsigned int speed)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
-	struct mxs_i2c_speed_table *spd = mxs_i2c_speed_to_cfg(speed);
 
-	if (!spd) {
-		printf("MXS I2C: Invalid speed selected (%d Hz)\n", speed);
+	uint32_t clk = mxc_get_clock(MXC_XTAL_CLK);
+	uint32_t base = ((clk / speed) - 38) / 2;
+	uint16_t high_count = base + 3;
+	uint16_t low_count = base - 3;
+	uint16_t rcv_count = (high_count * 3) / 4;
+	uint16_t xmit_count = low_count / 4;
+
+	if (speed > 540000) {
+		printf("MXS I2C: Speed too high (%d Hz)\n", speed);
+		return -EINVAL;
+	}
+
+	if (speed < 12000) {
+		printf("MXS I2C: Speed too low (%d Hz)\n", speed);
 		return -EINVAL;
 	}
 
-	writel(spd->timing0, &i2c_regs->hw_i2c_timing0);
-	writel(spd->timing1, &i2c_regs->hw_i2c_timing1);
+	writel((high_count << 16) | rcv_count, &i2c_regs->hw_i2c_timing0);
+	writel((low_count << 16) | xmit_count, &i2c_regs->hw_i2c_timing1);
 
 	writel((0x0030 << I2C_TIMING2_BUS_FREE_OFFSET) |
 		(0x0030 << I2C_TIMING2_LEADIN_COUNT_OFFSET),
@@ -277,12 +248,12 @@ int i2c_set_bus_speed(unsigned int speed)
 unsigned int i2c_get_bus_speed(void)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
-	uint32_t timing0, timing1;
+	uint32_t clk = mxc_get_clock(MXC_XTAL_CLK);
+	uint32_t timing0;
 
 	timing0 = readl(&i2c_regs->hw_i2c_timing0);
-	timing1 = readl(&i2c_regs->hw_i2c_timing1);
 
-	return mxs_i2c_cfg_to_speed(timing0, timing1);
+	return clk / ((((timing0 >> 16) - 3) * 2) + 38);
 }
 
 void i2c_init(int speed, int slaveadd)
-- 
1.7.10.4

[U-Boot] [PATCH v2] mxs: i2c: Implement algorithm to set up arbitrary i2c speed

[Heiko Schocher]

Hello Marek,

On 30.11.2012 16:28, Marek Vasut wrote:
> This algorithm computes the values of TIMING{0,1,2} registers for the
> MX28 I2C block. This algorithm was derived by using a scope, but the
> result seems correct.
>
> The resulting values programmed into the registers do not correlate
> with the contents in datasheet. When using the values from the datasheet,
> the I2C clock were completely wrong.
>
> Signed-off-by: Marek Vasut<marex@denx.de>
> Cc: Stefano Babic<sbabic@denx.de>
> Cc: Fabio Estevam<fabio.estevam@freescale.com>
> Cc: Wolfgang Denk<wd@denx.de>
> ---
>   arch/arm/cpu/arm926ejs/mxs/clock.c    |    2 +
>   arch/arm/include/asm/arch-mxs/clock.h |    1 +
>   drivers/i2c/mxs_i2c.c                 |   75 ++++++++++-----------------------
>   3 files changed, 26 insertions(+), 52 deletions(-)
>
> v2: Properly implement XTAL clock retrieval. The i2c clock are derived from the
>      24MHz XTAL clock.
>
[...]
> diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c
> index 006fb91..b040535 100644
> --- a/drivers/i2c/mxs_i2c.c
> +++ b/drivers/i2c/mxs_i2c.c
> @@ -28,6 +28,7 @@
>
>   #include<common.h>
>   #include<malloc.h>
> +#include<i2c.h>
>   #include<asm/errno.h>
>   #include<asm/io.h>
>   #include<asm/arch/clock.h>
> @@ -40,6 +41,7 @@ void mxs_i2c_reset(void)
>   {
>   	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
>   	int ret;
> +	int speed = i2c_get_bus_speed();
>
>   	ret = mxs_reset_block(&i2c_regs->hw_i2c_ctrl0_reg);
>   	if (ret) {
> @@ -53,6 +55,8 @@ void mxs_i2c_reset(void)
>   		&i2c_regs->hw_i2c_ctrl1_clr);
>
>   	writel(I2C_QUEUECTRL_PIO_QUEUE_MODE,&i2c_regs->hw_i2c_queuectrl_set);
> +
> +	i2c_set_bus_speed(speed);
>   }

This change is not described in the patch description, please fix.

>   void mxs_i2c_setup_read(uint8_t chip, int len)
> @@ -210,62 +214,29 @@ int i2c_probe(uchar chip)
>   	return ret;
>   }
>
> -static struct mxs_i2c_speed_table {
[...]
>   int i2c_set_bus_speed(unsigned int speed)
>   {
>   	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
> -	struct mxs_i2c_speed_table *spd = mxs_i2c_speed_to_cfg(speed);
>
> -	if (!spd) {
> -		printf("MXS I2C: Invalid speed selected (%d Hz)\n", speed);
> +	uint32_t clk = mxc_get_clock(MXC_XTAL_CLK);
> +	uint32_t base = ((clk / speed) - 38) / 2;
> +	uint16_t high_count = base + 3;
> +	uint16_t low_count = base - 3;
> +	uint16_t rcv_count = (high_count * 3) / 4;
> +	uint16_t xmit_count = low_count / 4;

a lot of magic constants ...

> +
> +	if (speed>  540000) {
> +		printf("MXS I2C: Speed too high (%d Hz)\n", speed);
> +		return -EINVAL;
> +	}
> +
> +	if (speed<  12000) {
> +		printf("MXS I2C: Speed too low (%d Hz)\n", speed);
>   		return -EINVAL;
>   	}
>
> -	writel(spd->timing0,&i2c_regs->hw_i2c_timing0);
> -	writel(spd->timing1,&i2c_regs->hw_i2c_timing1);
> +	writel((high_count<<  16) | rcv_count,&i2c_regs->hw_i2c_timing0);
> +	writel((low_count<<  16) | xmit_count,&i2c_regs->hw_i2c_timing1);
                          ^                    ^
                          spaces

>
>   	writel((0x0030<<  I2C_TIMING2_BUS_FREE_OFFSET) |
>   		(0x0030<<  I2C_TIMING2_LEADIN_COUNT_OFFSET),
> @@ -277,12 +248,12 @@ int i2c_set_bus_speed(unsigned int speed)
>   unsigned int i2c_get_bus_speed(void)
>   {
>   	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
> -	uint32_t timing0, timing1;
> +	uint32_t clk = mxc_get_clock(MXC_XTAL_CLK);
> +	uint32_t timing0;
>
>   	timing0 = readl(&i2c_regs->hw_i2c_timing0);
> -	timing1 = readl(&i2c_regs->hw_i2c_timing1);
>
> -	return mxs_i2c_cfg_to_speed(timing0, timing1);
> +	return clk / ((((timing0>>  16) - 3) * 2) + 38);
                                 ^
                                 spaces

... and here a lot of magic constants too ... as this is a result of
measuring ... we should at least note here, that we have this values
from a scope session and the values in the manual seem to be not
correct ...

Hmm... I am a little confused ... you write the i2c_regs->hw_i2c_timing{0,1,2}
registers in i2c_set_bus_speed() but you return in i2c_get_bus_speed()
just results from reading the i2c_regs->hw_i2c_timing0 register only
and do some funny calculations ... is this correct?

>   }
>
>   void i2c_init(int speed, int slaveadd)

Thanks!

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

[U-Boot] [PATCH v2] mxs: i2c: Implement algorithm to set up arbitrary i2c speed

[Marek Vasut]

Dear Heiko Schocher,

> Hello Marek,
[...]
> > diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c
> > index 006fb91..b040535 100644
> > --- a/drivers/i2c/mxs_i2c.c
> > +++ b/drivers/i2c/mxs_i2c.c
> > @@ -28,6 +28,7 @@
> > 
> >   #include<common.h>
> >   #include<malloc.h>
> > 
> > +#include<i2c.h>
> > 
> >   #include<asm/errno.h>
> >   #include<asm/io.h>
> >   #include<asm/arch/clock.h>
> > 
> > @@ -40,6 +41,7 @@ void mxs_i2c_reset(void)
> > 
> >   {
> >   
> >   	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
> >   	int ret;
> > 
> > +	int speed = i2c_get_bus_speed();
> > 
> >   	ret = mxs_reset_block(&i2c_regs->hw_i2c_ctrl0_reg);
> >   	if (ret) {
> > 
> > @@ -53,6 +55,8 @@ void mxs_i2c_reset(void)
> > 
> >   		&i2c_regs->hw_i2c_ctrl1_clr);
> >   	
> >   	writel(I2C_QUEUECTRL_PIO_QUEUE_MODE,&i2c_regs->hw_i2c_queuectrl_set);
> > 
> > +
> > +	i2c_set_bus_speed(speed);
> > 
> >   }
> 
> This change is not described in the patch description, please fix.

I suspect I'll move this to a separate patch.

> >   void mxs_i2c_setup_read(uint8_t chip, int len)
> > 
> > @@ -210,62 +214,29 @@ int i2c_probe(uchar chip)
> > 
> >   	return ret;
> >   
> >   }
> > 
> > -static struct mxs_i2c_speed_table {
> 
> [...]
> 
> >   int i2c_set_bus_speed(unsigned int speed)
> >   {
> >   
> >   	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
> > 
> > -	struct mxs_i2c_speed_table *spd = mxs_i2c_speed_to_cfg(speed);
> > 
> > -	if (!spd) {
> > -		printf("MXS I2C: Invalid speed selected (%d Hz)\n", speed);
> > +	uint32_t clk = mxc_get_clock(MXC_XTAL_CLK);
> > +	uint32_t base = ((clk / speed) - 38) / 2;
> > +	uint16_t high_count = base + 3;
> > +	uint16_t low_count = base - 3;
> > +	uint16_t rcv_count = (high_count * 3) / 4;
> > +	uint16_t xmit_count = low_count / 4;
> 
> a lot of magic constants ...

True ... and they have no other meaning than to align stuff on the scope ;-)

> > +
> > +	if (speed>  540000) {
> > +		printf("MXS I2C: Speed too high (%d Hz)\n", speed);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (speed<  12000) {
> > +		printf("MXS I2C: Speed too low (%d Hz)\n", speed);
> > 
> >   		return -EINVAL;
> >   	
> >   	}
> > 
> > -	writel(spd->timing0,&i2c_regs->hw_i2c_timing0);
> > -	writel(spd->timing1,&i2c_regs->hw_i2c_timing1);
> > +	writel((high_count<<  16) | rcv_count,&i2c_regs->hw_i2c_timing0);
> > +	writel((low_count<<  16) | xmit_count,&i2c_regs->hw_i2c_timing1);
> 
>                           ^                    ^
>                           spaces

Could this be your mailer's issue?

[...]

> ... and here a lot of magic constants too ... as this is a result of
> measuring ... we should at least note here, that we have this values
> from a scope session and the values in the manual seem to be not
> correct ...

True.

> Hmm... I am a little confused ... you write the
> i2c_regs->hw_i2c_timing{0,1,2} registers in i2c_set_bus_speed() but you
> return in i2c_get_bus_speed() just results from reading the
> i2c_regs->hw_i2c_timing0 register only and do some funny calculations ...
> is this correct?

Yes, the speed is configured upon boot anyway, so the timing0 register contains 
result of previous call to i2c_set_bus_speed(). And if it doesn't, we can't 
properly determine the speed anyway :(

> >   }
> >   
> >   void i2c_init(int speed, int slaveadd)
> 
> Thanks!
> 
> bye,
> Heiko

[U-Boot] [PATCH 1/2] mxs: i2c: Restore speed setting after block reset

[Marek Vasut]

The I2C block reset configures the I2C bus speed to strange value.
Read the I2C speed from the block before reseting the block and
restore it afterwards, so the I2C operates correctly. This issue
can be replicated by doing unsuccessful I2C transfer, after such
transfer finishes, the I2C block clock speed is misconfigured.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Heiko Schocher <hs@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/i2c/mxs_i2c.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c
index 006fb91..73a6659 100644
--- a/drivers/i2c/mxs_i2c.c
+++ b/drivers/i2c/mxs_i2c.c
@@ -40,6 +40,7 @@ void mxs_i2c_reset(void)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
 	int ret;
+	int speed = i2c_get_bus_speed();
 
 	ret = mxs_reset_block(&i2c_regs->hw_i2c_ctrl0_reg);
 	if (ret) {
@@ -53,6 +54,8 @@ void mxs_i2c_reset(void)
 		&i2c_regs->hw_i2c_ctrl1_clr);
 
 	writel(I2C_QUEUECTRL_PIO_QUEUE_MODE, &i2c_regs->hw_i2c_queuectrl_set);
+
+	i2c_set_bus_speed(speed);
 }
 
 void mxs_i2c_setup_read(uint8_t chip, int len)
-- 
1.7.10.4

[U-Boot] [PATCH 2/2 V3] mxs: i2c: Implement algorithm to set up arbitrary i2c speed

[Marek Vasut]

This algorithm computes the values of TIMING{0,1,2} registers for the
MX28 I2C block. This algorithm was derived by using a scope, but the
result seems correct.

The resulting values programmed into the registers do not correlate
with the contents in datasheet. When using the values from the datasheet,
the I2C clock were completely wrong.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Wolfgang Denk <wd@denx.de>
---
 arch/arm/cpu/arm926ejs/mxs/clock.c    |    2 +
 arch/arm/include/asm/arch-mxs/clock.h |    1 +
 drivers/i2c/mxs_i2c.c                 |   87 +++++++++++++--------------------
 3 files changed, 37 insertions(+), 53 deletions(-)

V2: Properly implement XTAL clock retrieval. The i2c clock are derived from the
    24MHz XTAL clock.

V3: Split away reset speed fix
    Add comment about the funny algorithm

diff --git a/arch/arm/cpu/arm926ejs/mxs/clock.c b/arch/arm/cpu/arm926ejs/mxs/clock.c
index bfea6ab..4ff19c3 100644
--- a/arch/arm/cpu/arm926ejs/mxs/clock.c
+++ b/arch/arm/cpu/arm926ejs/mxs/clock.c
@@ -333,6 +333,8 @@ uint32_t mxc_get_clock(enum mxc_clock clk)
 		return mx28_get_sspclk(MXC_SSPCLK2);
 	case MXC_SSP3_CLK:
 		return mx28_get_sspclk(MXC_SSPCLK3);
+	case MXC_XTAL_CLK:
+		return XTAL_FREQ_KHZ * 1000;
 	}
 
 	return 0;
diff --git a/arch/arm/include/asm/arch-mxs/clock.h b/arch/arm/include/asm/arch-mxs/clock.h
index 1700fe3..3d39ef2 100644
--- a/arch/arm/include/asm/arch-mxs/clock.h
+++ b/arch/arm/include/asm/arch-mxs/clock.h
@@ -35,6 +35,7 @@ enum mxc_clock {
 	MXC_SSP1_CLK,
 	MXC_SSP2_CLK,
 	MXC_SSP3_CLK,
+	MXC_XTAL_CLK,
 };
 
 enum mxs_ioclock {
diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c
index 73a6659..b907f7b 100644
--- a/drivers/i2c/mxs_i2c.c
+++ b/drivers/i2c/mxs_i2c.c
@@ -28,6 +28,7 @@
 
 #include <common.h>
 #include <malloc.h>
+#include <i2c.h>
 #include <asm/errno.h>
 #include <asm/io.h>
 #include <asm/arch/clock.h>
@@ -213,62 +214,39 @@ int i2c_probe(uchar chip)
 	return ret;
 }
 
-static struct mxs_i2c_speed_table {
-	uint32_t	speed;
-	uint32_t	timing0;
-	uint32_t	timing1;
-} mxs_i2c_tbl[] = {
-	{
-		100000,
-		(0x0078 << I2C_TIMING0_HIGH_COUNT_OFFSET) |
-		(0x0030 << I2C_TIMING0_RCV_COUNT_OFFSET),
-		(0x0080 << I2C_TIMING1_LOW_COUNT_OFFSET) |
-		(0x0030 << I2C_TIMING1_XMIT_COUNT_OFFSET)
-	},
-	{
-		400000,
-		(0x000f << I2C_TIMING0_HIGH_COUNT_OFFSET) |
-		(0x0007 << I2C_TIMING0_RCV_COUNT_OFFSET),
-		(0x001f << I2C_TIMING1_LOW_COUNT_OFFSET) |
-		(0x000f << I2C_TIMING1_XMIT_COUNT_OFFSET),
-	}
-};
-
-static struct mxs_i2c_speed_table *mxs_i2c_speed_to_cfg(uint32_t speed)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(mxs_i2c_tbl); i++)
-		if (mxs_i2c_tbl[i].speed == speed)
-			return &mxs_i2c_tbl[i];
-	return NULL;
-}
-
-static uint32_t mxs_i2c_cfg_to_speed(uint32_t timing0, uint32_t timing1)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(mxs_i2c_tbl); i++) {
-		if (mxs_i2c_tbl[i].timing0 != timing0)
-			continue;
-		if (mxs_i2c_tbl[i].timing1 != timing1)
-			continue;
-		return mxs_i2c_tbl[i].speed;
-	}
-
-	return 0;
-}
-
 int i2c_set_bus_speed(unsigned int speed)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
-	struct mxs_i2c_speed_table *spd = mxs_i2c_speed_to_cfg(speed);
+	/*
+	 * The timing derivation algorithm. There is no documentation for this
+	 * algorithm available, it was derived by using the scope and fiddling
+	 * with constants until the result observed on the scope was good enough
+	 * for 20kHz, 50kHz, 100kHz, 200kHz, 300kHz and 400kHz. It should be
+	 * possible to assume the algorithm works for other frequencies as well.
+	 *
+	 * Note it was necessary to cap the frequency on both ends as it's not
+	 * possible to configure completely arbitrary frequency for the I2C bus
+	 * clock.
+	 */
+	uint32_t clk = mxc_get_clock(MXC_XTAL_CLK);
+	uint32_t base = ((clk / speed) - 38) / 2;
+	uint16_t high_count = base + 3;
+	uint16_t low_count = base - 3;
+	uint16_t rcv_count = (high_count * 3) / 4;
+	uint16_t xmit_count = low_count / 4;
+
+	if (speed > 540000) {
+		printf("MXS I2C: Speed too high (%d Hz)\n", speed);
+		return -EINVAL;
+	}
 
-	if (!spd) {
-		printf("MXS I2C: Invalid speed selected (%d Hz)\n", speed);
+	if (speed < 12000) {
+		printf("MXS I2C: Speed too low (%d Hz)\n", speed);
 		return -EINVAL;
 	}
 
-	writel(spd->timing0, &i2c_regs->hw_i2c_timing0);
-	writel(spd->timing1, &i2c_regs->hw_i2c_timing1);
+	writel((high_count << 16) | rcv_count, &i2c_regs->hw_i2c_timing0);
+	writel((low_count << 16) | xmit_count, &i2c_regs->hw_i2c_timing1);
 
 	writel((0x0030 << I2C_TIMING2_BUS_FREE_OFFSET) |
 		(0x0030 << I2C_TIMING2_LEADIN_COUNT_OFFSET),
@@ -280,12 +258,15 @@ int i2c_set_bus_speed(unsigned int speed)
 unsigned int i2c_get_bus_speed(void)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
-	uint32_t timing0, timing1;
+	uint32_t clk = mxc_get_clock(MXC_XTAL_CLK);
+	uint32_t timing0;
 
 	timing0 = readl(&i2c_regs->hw_i2c_timing0);
-	timing1 = readl(&i2c_regs->hw_i2c_timing1);
-
-	return mxs_i2c_cfg_to_speed(timing0, timing1);
+	/*
+	 * This is a reverse version of the algorithm presented in
+	 * i2c_set_bus_speed(). Please refer there for details.
+	 */
+	return clk / ((((timing0 >> 16) - 3) * 2) + 38);
 }
 
 void i2c_init(int speed, int slaveadd)
-- 
1.7.10.4