Re: [PATCH] I2C: mpc: insert DR read in i2c_fixup()

Valentin Longchamp
2014-06-03T06:49:59Z

On 06/02/2014 06:27 PM, Wolfram Sang wrote:
> On Fri, May 16, 2014 at 03:36:38PM +0200, Valentin Longchamp wrote:
>> The mpc_i2c_fixup function is called when the bus is not released by a
>> slave. The function generates 9 pulses that should lead the slave
>> to release the bus.
>>
>> The sequence that generates the pulses disables/enables the I2C module
>> that controls the blocked bus. We have found out on the P2041 SoC that
>> this could cause the CPU to hang (for a short delay).
>>
>> To avoid this, this patch introduces a read to the I2CDR register
>> between the re-enablement of the I2C module in master mode and its
>> returning to the slave mode instead of the delay (the final delay,
>> between the pulses is kept), as proposed in procedure from the P2041
>> reference manual (16.6.2.3).
>>
>> Signed-off-by: Rainer Boschung <rainer.boschung-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Valentin Longchamp <valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
>
> Have you checked other manuals if this always has been the proposed
> procedure?

I have just checked the RMs of the Freescale CPUs I am familiar with, and it is
the same procedure:
- MPC8360E (PowerQUICC II Pro family): 15.5.7
- MPC8548E (PowerQUICC III family): 11.5.6

So I think this is the correct procedure for this module according to several RMs.

>
>> ---
>>
>> drivers/i2c/busses/i2c-mpc.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
>> index f539163..3655af7 100644
>> --- a/drivers/i2c/busses/i2c-mpc.c
>> +++ b/drivers/i2c/busses/i2c-mpc.c
>> @@ -107,6 +107,7 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
>> static void mpc_i2c_fixup(struct mpc_i2c *i2c)
>> {
>> int k;
>> + u8 dr;
>
> Looks like we can drop this variable since the value read is not used?

That's correct it is not used. I have added it to make sure that this is not
"optimized" out but I guess the readb call is enough for this purpose ?

>
>> u32 delay_val = 1000000 / i2c->real_clk + 1;
>>
>> if (delay_val < 2)
>> @@ -115,7 +116,7 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
>> for (k = 9; k; k--) {
>> writeccr(i2c, 0);
>> writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
>> - udelay(delay_val);
>> + dr = readb(i2c->base + MPC_I2C_DR);
>> writeccr(i2c, CCR_MEN);
>> udelay(delay_val << 1);
>> }
>> --
>> 1.8.0.1
>>