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

[Aurelien Jarno <aurelien@aurel32.net>]

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