[v1] i2c: spacemit: fix detect issue

[PATCH] i2c: spacemit: fix detect issue

Posted by Troy Mitchell 3 months ago

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;
 	}
 
 	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);
+
 	spacemit_i2c_enable(i2c);
 
 	ret = spacemit_i2c_wait_bus_idle(i2c);

---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251103-fix-k1-detect-failure-0b3082c73784

Best regards,
-- 
Troy Mitchell <troy.mitchell@linux.spacemit.com>

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

Posted by Aurelien Jarno 3 months ago

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

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

Posted by Troy Mitchell 3 months ago

On Wed, Nov 05, 2025 at 11:44:00PM +0100, Aurelien Jarno wrote:
> Hi,
> 
> On 2025-11-03 15:06, Troy Mitchell wrote:
[...]
> >  	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);
> 	}
Thanks. I'll fix it in the next version.
> 
> 
> >  	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.
Uh, We can move it actually. But is it essentail?
> 
> >  	spacemit_i2c_enable(i2c);
> >  
> >  	ret = spacemit_i2c_wait_bus_idle(i2c);
> 
> Anyway:
> 
> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
Thanks!

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

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

Posted by Aurelien Jarno 3 months ago

Hi,

On 2025-11-06 09:05, Troy Mitchell wrote:
> On Wed, Nov 05, 2025 at 11:44:00PM +0100, Aurelien Jarno wrote:
> > Hi,
> > 
> > On 2025-11-03 15:06, Troy Mitchell wrote:
> [...]
> > >  	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);
> > 	}
> Thanks. I'll fix it in the next version.
> > 
> > 
> > >  	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.
> Uh, We can move it actually. But is it essentail?

For me ensuring that the interrupt status is in a clean state after 
enabling the interrupt is part of the initialization. Furthermore if 
spacemit_i2c_init() has to be called from another place, it's very 
likely that it's also needed to get interrupt status in a clean state.

Regards
Aurelien

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

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

Posted by Troy Mitchell 3 months ago

On Thu, Nov 06, 2025 at 06:34:26AM +0100, Aurelien Jarno wrote:
> Hi,
> 
> On 2025-11-06 09:05, Troy Mitchell wrote:
> > On Wed, Nov 05, 2025 at 11:44:00PM +0100, Aurelien Jarno wrote:
> > > Hi,
> > > 
> > > On 2025-11-03 15:06, Troy Mitchell wrote:
> > [...]
> > > >  	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);
> > > 	}
> > Thanks. I'll fix it in the next version.
> > > 
> > > 
> > > >  	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.
> > Uh, We can move it actually. But is it essentail?
> 
> For me ensuring that the interrupt status is in a clean state after 
> enabling the interrupt is part of the initialization.
Yes, I agree that.
> Furthermore if 
> spacemit_i2c_init() has to be called from another place, it's very 
> likely that it's also needed to get interrupt status in a clean state.
Why we need to call init() in other place?
Could you give me a cese?

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

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

Posted by Aurelien Jarno 3 months ago

Hi,

On 2025-11-06 13:38, Troy Mitchell wrote:
> On Thu, Nov 06, 2025 at 06:34:26AM +0100, Aurelien Jarno wrote:
> > Hi,
> > 
> > On 2025-11-06 09:05, Troy Mitchell wrote:
> > > On Wed, Nov 05, 2025 at 11:44:00PM +0100, Aurelien Jarno wrote:
> > > > Hi,
> > > > 
> > > > On 2025-11-03 15:06, Troy Mitchell wrote:
> > > [...]
> > > > >  	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);
> > > > 	}
> > > Thanks. I'll fix it in the next version.
> > > > 
> > > > 
> > > > >  	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.
> > > Uh, We can move it actually. But is it essentail?
> > 
> > For me ensuring that the interrupt status is in a clean state after 
> > enabling the interrupt is part of the initialization.
> Yes, I agree that.
> > Furthermore if 
> > spacemit_i2c_init() has to be called from another place, it's very 
> > likely that it's also needed to get interrupt status in a clean state.
> Why we need to call init() in other place?
> Could you give me a cese?

Currently there is no need to do that. However if the driver is extended 
(e.g. to add new features) and spacemit_i2c_init() needs to be called 
from another place, it is likely that a reset of the interrupt status 
should also be included. Therefore it's better to just include it 
directly in spacemit_i2c_init().

Regards
Aurelien

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

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

Posted by Troy Mitchell 3 months ago

On Thu, Nov 06, 2025 at 07:17:00PM +0100, Aurelien Jarno wrote:
> Hi,
> 
> On 2025-11-06 13:38, Troy Mitchell wrote:
> > On Thu, Nov 06, 2025 at 06:34:26AM +0100, Aurelien Jarno wrote:
> > > Hi,
> > > 
> > > On 2025-11-06 09:05, Troy Mitchell wrote:
> > > > On Wed, Nov 05, 2025 at 11:44:00PM +0100, Aurelien Jarno wrote:
> > > > > Hi,
> > > > > 
> > > > > On 2025-11-03 15:06, Troy Mitchell wrote:
> > > > [...]
> > > > > >  	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);
> > > > > 	}
> > > > Thanks. I'll fix it in the next version.
> > > > > 
> > > > > 
> > > > > >  	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.
> > > > Uh, We can move it actually. But is it essentail?
> > > 
> > > For me ensuring that the interrupt status is in a clean state after 
> > > enabling the interrupt is part of the initialization.
> > Yes, I agree that.
> > > Furthermore if 
> > > spacemit_i2c_init() has to be called from another place, it's very 
> > > likely that it's also needed to get interrupt status in a clean state.
> > Why we need to call init() in other place?
> > Could you give me a cese?
> 
> Currently there is no need to do that. However if the driver is extended 
> (e.g. to add new features) and spacemit_i2c_init() needs to be called 
> from another place, it is likely that a reset of the interrupt status 
> should also be included. Therefore it's better to just include it 
> directly in spacemit_i2c_init().
I'll move it in init(). But it's not for reuse.
It's not for reuse, but because the name init() should indeed ensure the
hardware is in a clean state.

Thanks!

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