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

[Marek Vasut <marex@denx.de>]

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