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

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

[Valentin Longchamp]

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>

---

 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;
 	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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

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?

> ---
> 
>  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?

>  	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
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Valentin Longchamp]

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
>>

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]


> > 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.

Thanks.

> >> --- 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 ?

I'd think so, but it needs your test :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Valentin Longchamp]

On 06/03/2014 09:11 AM, Wolfram Sang wrote:
>>>> --- 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 ?
> 
> I'd think so, but it needs your test :)
> 

I have just tested it and it's OK without the dr variable as well. Sending V2
right now.

Valentin