[PATCH] i2c: mpc: Use atomic read and fix break condition

[PATCH] i2c: mpc: Use atomic read and fix break condition

[Chris Packham]

Maxime points out that the polling code in mpc_i2c_isr should use the
_atomic API because it is called in an irq context and that the
behaviour of the MCF bit is that it is 1 when the byte transfer is
complete. All of this means the original code was effectively a
udelay(100).

Fix this by using readb_poll_timeout_atomic() and removing the negation
of the break condition.

Fixes: 4a8ac5e45cda ("i2c: mpc: Poll for MCF")
Reported-by: Maxime Bizon <mbizon@freebox.fr>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Maxime,

Can you give this a test on your setup. I've tried it on the setup where
I had the original problem that led to 4a8ac5e45cda and it seems OK so
far (I'll leave my test running overnight).

 drivers/i2c/busses/i2c-mpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index a6ea1eb1394e..53b8da6dbb23 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -636,7 +636,7 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
 	status = readb(i2c->base + MPC_I2C_SR);
 	if (status & CSR_MIF) {
 		/* Wait up to 100us for transfer to properly complete */
-		readb_poll_timeout(i2c->base + MPC_I2C_SR, status, !(status & CSR_MCF), 0, 100);
+		readb_poll_timeout_atomic(i2c->base + MPC_I2C_SR, status, status & CSR_MCF, 0, 100);
 		writeb(0, i2c->base + MPC_I2C_SR);
 		mpc_i2c_do_intr(i2c, status);
 		return IRQ_HANDLED;
-- 
2.34.1

Re: [PATCH] i2c: mpc: Use atomic read and fix break condition

[Maxime Bizon]

On Tue, 2021-12-07 at 17:21 +1300, Chris Packham wrote:

> Can you give this a test on your setup. I've tried it on the setup
> where I had the original problem that led to 4a8ac5e45cda and it
> seems OK so far (I'll leave my test running overnight).

Tested-by: Maxime Bizon <mbizon@freebox.fr>

Small reservation though, it does not seem to be understood why this
polling is needed.

Reading the driver history, the theory is that the controller will
trigger an interrupt at the end of transfer just after the last SCL
cycle, but irrespective of whether SCL goes high, which happens if a
slave "stretch" the clock until it's ready to answer.

Supposedly when that happen, CSR_MCF bit would be 0 at interrupt time,
meaning bus is busy, and we have to poll until it goes to 1 meaning the
slave has released SCL.

I have no slave that does clock stretching on my board so I cannot test
the theory. On my mpc8347 device, i2c clock speed set to 90kHz, I've
never seen a case where MCR was 0 at interrupt time.

For i2c experts here, is 100us enough in that case ? I could not any
maximum stretch time in i2c specification.

My CPU user manual is IMO vague on this precise topic, hopefully an NXP
knowledgeable employee will read this and enlighten us.

Thanks,

-- 
Maxime
 

Re: [PATCH] i2c: mpc: Use atomic read and fix break condition

[Chris Packham]

On 7/12/21 11:13 pm, Maxime Bizon wrote:
> On Tue, 2021-12-07 at 17:21 +1300, Chris Packham wrote:
>
>> Can you give this a test on your setup. I've tried it on the setup
>> where I had the original problem that led to 4a8ac5e45cda and it
>> seems OK so far (I'll leave my test running overnight).
> Tested-by: Maxime Bizon <mbizon@freebox.fr>
My testing overnight also looks good.
> Small reservation though, it does not seem to be understood why this
> polling is needed.
>
> Reading the driver history, the theory is that the controller will
> trigger an interrupt at the end of transfer just after the last SCL
> cycle, but irrespective of whether SCL goes high, which happens if a
> slave "stretch" the clock until it's ready to answer.
>
> Supposedly when that happen, CSR_MCF bit would be 0 at interrupt time,
> meaning bus is busy, and we have to poll until it goes to 1 meaning the
> slave has released SCL.

I share your reservation. The original re-read pre-dates git (I do 
recall looking in the historical repo as well and finding nothing 
enlightening). All I can say is that the original code thought I2C_SR 
needed some time to "stabilize".

Both MIF and MCF should be set at the falling edge of the ninth clock. 
In theory we could end up with MIF=1 MCF=0 if MAL is set (in which case 
we'd hit the 100us timeout in the poll). But I see no evidence of that 
actually happening (and no idea what arbitration lost means w.r.t i2c).

The root interrupts for I2C1 and I2C2 are shared so it may be possible 
for MIF to be in the process of being set for I2C1 but the actual mpic 
interrupt be raised for a different transfer on I2C2. The isr will look 
at both I2C adapters and attempt to handle the interrupt if MIF is set. 
I'd expect a spurious interrupt to be counted in this case as by the 
time I2C1 raises the interrupt with the mpic we'd have already serviced 
it (but maybe the fiddling with MEIN prevents that).

My best guess is that even if the host adapter has sent the ninth clock 
it doesn't mean that the remote device will release SCL (e.g. in the 
case of clock stretching or my slightly dodgy hardware). So I think the 
act of polling for MCF (or prior to this what was effectively a 
udelay(100)) allows the remote device a bit of time to release SCL.

> I have no slave that does clock stretching on my board so I cannot test
> the theory. On my mpc8347 device, i2c clock speed set to 90kHz, I've
> never seen a case where MCR was 0 at interrupt time.
>
> For i2c experts here, is 100us enough in that case ? I could not any
> maximum stretch time in i2c specification.

I don't know that there is a maximum clock stretch time (we certainly 
know there are misbehaving devices that hold SCL low forever). The SMBUS 
protocol adds some timeouts but as far as I know i2c says nothing about 
how long a remote device can hold SCL.

> My CPU user manual is IMO vague on this precise topic, hopefully an NXP
> knowledgeable employee will read this and enlighten us.

That would be nice (but there is a reason I've ended up being listed as 
the maintainer for this driver).

>
> Thanks,
>

Re: [PATCH] i2c: mpc: Use atomic read and fix break condition

[wsa]

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


> we'd hit the 100us timeout in the poll). But I see no evidence of that 
> actually happening (and no idea what arbitration lost means w.r.t i2c).

On a bus with multiple masters, it means the other master has won the
arbitration because the address it wants to talk to contains more 0 bits.

> I don't know that there is a maximum clock stretch time (we certainly 
> know there are misbehaving devices that hold SCL low forever). The SMBUS 
> protocol adds some timeouts but as far as I know i2c says nothing about 
> how long a remote device can hold SCL.

The above is all correct.

Even with the unclear situation about the 100us, I think this should go
to for-current soon, right?



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: mpc: Use atomic read and fix break condition

[Chris Packham]

On 9/12/21 10:21 pm, wsa@kernel.org wrote:
>> we'd hit the 100us timeout in the poll). But I see no evidence of that
>> actually happening (and no idea what arbitration lost means w.r.t i2c).
> On a bus with multiple masters, it means the other master has won the
> arbitration because the address it wants to talk to contains more 0 bits.
>
>> I don't know that there is a maximum clock stretch time (we certainly
>> know there are misbehaving devices that hold SCL low forever). The SMBUS
>> protocol adds some timeouts but as far as I know i2c says nothing about
>> how long a remote device can hold SCL.
> The above is all correct.
>
> Even with the unclear situation about the 100us, I think this should go
> to for-current soon, right?
Please and thank you.