[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 at denx.de>
> Cc: Stefano Babic<sbabic at denx.de>
> Cc: Fabio Estevam<fabio.estevam at freescale.com>
> Cc: Wolfgang Denk<wd at 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