Re: [PATCH] i2c: spacemit: fix detect issue

Aurelien Jarno
2025-11-05T22:44:08Z

Hi,

On 2025-11-03 15:06, Troy Mitchell wrote:
> This commit addresses two issues causing i2c detect to fail.
>
> The identified issues are:
>
> 1. Incorrect error handling for BED (Bus Error No ACK/NAK):
> Before this commit, Both ALD (Arbitration Loss Detected) and
> BED returned -EAGAIN.
> 2. Missing interrupt status clear after initialization in xfer():
> On the K1 SoC, simply fixing the first issue changed the error
> from -EAGAIN to -ETIMEOUT. Through tracing, it was determined that
> this is likely due to MSD (Master Stop Detected) latency issues.
>
> That means the MSD bit in the ISR may still be set on the next transfer.
> As a result, the controller won't work — we can see from the scope that
> it doesn't issue any signal.
> (This only occurs during rapid consecutive I2C transfers.
> That explains why the issue only shows up with i2cdetect.)
>
> With these two fixes, i2c device detection now functions correctly on the K1 SoC.
>
> Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> I checked the vendor version driver and tried commenting out
> spacemit_i2c_clear_int_status() that runs before xfer starts.
> Surprisingly, i2cdetect stopped working as well.
> ---
> drivers/i2c/busses/i2c-k1.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..37828323317770ae2f0522d213dca67342ae166f 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -160,7 +160,8 @@ static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
>
> if (i2c->status & (SPACEMIT_SR_BED | SPACEMIT_SR_ALD)) {
> spacemit_i2c_reset(i2c);
> - return -EAGAIN;
> + if (i2c->status & SPACEMIT_SR_ALD)
> + return -EAGAIN;
> }

This makes the resulting code, while correct, complex to understand as
it is now two really different errors, as you explained well in the
commit message.

I therefore suggest to organize the code as:

/* Arbitration Loss Detected */
if (i2c->status & SPACEMIT_SR_ALD) {
spacemit_i2c_reset(i2c);
return -EAGAIN;
}

/* Bus Error No ACK/NAK */
if (i2c->status & SPACEMIT_SR_BED) {
spacemit_i2c_reset(i2c);
}

> return i2c->status & SPACEMIT_SR_ACKNAK ? -ENXIO : -EIO;
> @@ -491,6 +492,8 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
>
> spacemit_i2c_init(i2c);
>
> + spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> +

This sounds good to start the transfer with a clean interrupt state. I
just wonder if it should be moved to spacemit_i2c_init(), ie where the
corresponding interrupts are enabled.

> spacemit_i2c_enable(i2c);
>
> ret = spacemit_i2c_wait_bus_idle(i2c);

Anyway:

Tested-by: Aurelien Jarno <aurelien@aurel32.net>

Regards
Aurelien

--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://aurel32.net