[U-Boot] [PATCH V3 05/11] I2C: mxc_i2c: address failure with mx35 processor

[Heiko Schocher]

Hello Stefano,

just a question ...

Stefano Babic wrote:
> There is sporadic failures when more as one I2C slave
> is on the bus and the processor tries to communicate
> with more as one slave.
> The problem was seen on a mx35pdk (two I2C slaves,
> PMIC controller and CAN/RTC chip).
> 
> The current driver uses the IIF bit in the status register
> to check if the bus is busy or not. According to the manual,
> this is not correct, because the IIB bit should be checked.
> Not only, to check if a transfer is finished must be checked
> the ICF bit, and this is not tested at all.
> 
> This patch comes from analyse with a corresponding driver
> provided by Freescale as part of the LTIB tool. Comparing
> the two drivers, it appears that the current u-boot driver checks
> the wrong bits, and depending on race condition, the transfer
> can be successful or not.
> 
> The patch gets rid also of own debug function (DPRINTF),
> replaced with the general debug().
> 
> Tested on Freescale mx35pdk.
> 
> Signed-off-by: Stefano Babic <sbabic at denx.de>
> CC: Heiko Schocher <hs at denx.de>
> ---
> Changes:
> 
> Wolfgang Denk:
> 	- change commit message explaining the problem
> 	and the changes
> 	- describe in commit message the drop of DPRINTF
> 
>  drivers/i2c/mxc_i2c.c |   86 ++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 68 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index fd6db18..c5ec486 100755
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
[...]
> @@ -116,31 +113,61 @@ void i2c_init(int speed, int unused)
>  	i2c_reset();
>  }
>  
> +static int wait_idle(void)
> +{
> +	int timeout = I2C_MAX_TIMEOUT;
> +
> +	while ((readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout) {
> +		writew(0, I2C_BASE + I2SR);
> +		udelay(1);
> +	}
> +	return timeout ? timeout : (!(readw(I2C_BASE + I2SR) & I2SR_IBB));
> +}
> +
>  static int wait_busy(void)
>  {
> -	int timeout = 10000;
> +	int timeout = I2C_MAX_TIMEOUT;
>  
> -	while (!(readw(I2C_BASE + I2SR) & I2SR_IIF) && --timeout)
> +	while (!(readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout)
>  		udelay(1);
>  	writew(0, I2C_BASE + I2SR); /* clear interrupt */
>  
>  	return timeout;
>  }
>  
> +static int wait_complete(void)
> +{
> +	int timeout = I2C_MAX_TIMEOUT;
> +
> +	while ((!(readw(I2C_BASE + I2SR) & I2SR_ICF)) && (--timeout)) {
> +		writew(0, I2C_BASE + I2SR);
> +		udelay(1);
> +	}
> +	udelay(200);

Why is this delay necessary? Why exactly 200? Is this documented
somewhere in the doc?

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