[U-Boot] [PATCH] I2C: mxc_i2c rework

[Marek Vasut]

On Wednesday, July 13, 2011 03:56:45 PM Stefano Babic wrote:
> On 07/13/2011 11:53 AM, Marek Vasut wrote:
> > Rewrite the mxc_i2c driver.
> > 
> >  * This version is much closer to Linux implementation.
> >  * Fixes IPG_PERCLK being incorrectly used as clock source
> >  * Fixes behaviour of the driver on iMX51
> >  * Clean up coding style a bit ;-)
> > 
> > Signed-off-by: Marek Vasut <marek.vasut at gmail.com>
> > ---
> 
> Hi Marek,
> 
> I have added Heiko in CC. He is the Maintainer for I2C.
> 
> >  #define I2C_MAX_TIMEOUT		10000
> > 
> > -#define I2C_MAX_RETRIES		3
> > 
> > -static u16 div[] = { 30, 32, 36, 42, 48, 52, 60, 72, 80, 88, 104, 128,
> > 144, -	             160, 192, 240, 288, 320, 384, 480, 576, 640, 768,
> > 960, -	             1152, 1280, 1536, 1920, 2304, 2560, 3072, 3840};
> > +static u16 i2c_clk_div[50][2] = {
> > +	{ 22,	0x20 }, { 24,	0x21 }, { 26,	0x22 }, { 28,	0x23 },
> > +	{ 30,	0x00 }, { 32,	0x24 }, { 36,	0x25 }, { 40,	0x26 },
> > +	{ 42,	0x03 }, { 44,	0x27 }, { 48,	0x28 }, { 52,	0x05 },
> > +	{ 56,	0x29 }, { 60,	0x06 }, { 64,	0x2A }, { 72,	0x2B },
> > +	{ 80,	0x2C }, { 88,	0x09 }, { 96,	0x2D }, { 104,	0x0A },
> > +	{ 112,	0x2E }, { 128,	0x2F }, { 144,	0x0C }, { 160,	0x30 },
> > +	{ 192,	0x31 }, { 224,	0x32 }, { 240,	0x0F }, { 256,	0x33 },
> > +	{ 288,	0x10 }, { 320,	0x34 }, { 384,	0x35 }, { 448,	0x36 },
> > +	{ 480,	0x13 }, { 512,	0x37 }, { 576,	0x14 }, { 640,	0x38 },
> > +	{ 768,	0x39 }, { 896,	0x3A }, { 960,	0x17 }, { 1024,	0x3B },
> > +	{ 1152,	0x18 }, { 1280,	0x3C }, { 1536,	0x3D }, { 1792,	0x3E },
> > +	{ 1920,	0x1B }, { 2048,	0x3F }, { 2304,	0x1C }, { 2560,	0x1D },
> > +	{ 3072,	0x1E }, { 3840,	0x1F }
> > +};
> 
> You have added an array with fixed values as indicated in the
> Freescale's manual (Table 26-7 for i.MX31, Table 40-7 for MX51, Table
> 41-12 for MX53). What about to add also some comments about these changes ?

I stole this from linux kernel, but sure ... as this is nearly a complete 
rewrite, I didn't describe all the changes.
> 
> > -void i2c_init(int speed, int unused)
> > +/*
> > + * Calculate and set proper clock divider
> > + *
> > + * FIXME: remove #ifdefs !
> > + */
> 
> Agree. I have prepared a patch to get rid of this mx31_ specialties. I
> will post soon. Then we can use mxc_get_clock(MXC_IPG_CLK) for all i.MX
> processors.

Yes, thanks. I saw the patch you posted already.
> 
> > +	div = (i2c_clk_rate + rate - 1) / rate;
> > +	if (div < i2c_clk_div[0][0])
> > +		i = 0;
> > +	else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
> > +		i = ARRAY_SIZE(i2c_clk_div) - 1;
> > +	else
> > +		for (i = 0; i2c_clk_div[i][0] < div; i++);
> > +
> > +	/* Store divider value */
> > +	writeb(div, I2C_BASE + IFDR);
> > +	clk_idx = div;
> 
> It seems to me ok - you replaced a computed value, that does not obtain
> exactly the value indicated in the manual, with the closest value of the
> table.

This thing is from linux kernel, nearly exact copy.
> 
> > +int i2c_imx_bus_busy(int for_busy)
> > 
> >  {
> > 
> > +	unsigned int temp;
> > +
> > 
> >  	int timeout = I2C_MAX_TIMEOUT;
> > 
> > -	while ((readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout) {
> > -		writew(0, I2C_BASE + I2SR);
> > +	while (timeout--) {
> > +		temp = readb(I2C_BASE + I2SR);
> > +
> > +		if (for_busy && (temp & I2SR_IBB))
> > +			return 0;
> > +		if (!for_busy && !(temp & I2SR_IBB))
> > +			return 0;
> > +
> > 
> >  		udelay(1);
> >  	
> >  	}
> > 
> > -	return timeout ? timeout : (!(readw(I2C_BASE + I2SR) & I2SR_IBB));
> > +
> > +	return 1;
> > 
> >  }
> 
> It is not clear to me why you add a way to go out from the function. If
> it is busy, should we not wait at least until the timeout variable
> becomes zero ?

You are waiting until it times out ... you can use thing to either wait for the 
bus to become busy or to become free, check the description of this function :)

> 
> > -static int wait_busy(void)
> > +/*
> > + * Wait for transaction to complete
> > + */
> > +int i2c_imx_trx_complete(void)
> > 
> >  {
> >  
> >  	int timeout = I2C_MAX_TIMEOUT;
> > 
> > -	while (!(readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout)
> > +	while (timeout--) {
> > +		if (readb(I2C_BASE + I2SR) & I2SR_IIF) {
> 
> If we wait for completion, should we not check the ICF bit instead of
> IIF, as done before your patch ?

That's how this is done in linux kernel, and TBH the only possible occurance of 
IIF is exactly when ICF is set too (in our case at least). IIRC when I last 
tried this, I had some trouble with ICF, but this is a valid point.

I'll investigate further here.

> 
> > +/*
> > + * Start the controller
> > + */
> > +int i2c_imx_start(void)
> > +{
> > +	unsigned int temp = 0;
> > +	int result;
> > 
> > -	writew(0, I2C_BASE + I2SR);	/* clear interrupt */
> > +	writeb(clk_idx, I2C_BASE + IFDR);
> 
> Well, as you talk about cleaning up the code, what about to replace the
> direct access to the registers with a C structure, as part of your clean
> up ?

Very good point!
> 
> > +/*
> > + * Write register address
> > + */
> > +int i2c_imx_set_reg_addr(uint addr, int alen)
> > 
> >  {
> > 
> > -	int i, retry = 0;
> > -	for (retry = 0; retry < 3; retry++) {
> > -		if (wait_idle())
> > +	int ret;
> > +	int i;
> > +
> 
> mmmhh...it seems to me you change completely the logic here. Heiko, waht
> do you think about ?
> 
> Best regards,
> Stefano Babic

Cheers