[PATCH 0/6] i2c: spacemit: fix and introduce pio

[PATCH 0/6] i2c: spacemit: fix and introduce pio

[Troy Mitchell]

Previously, there were a few latent issues in the I2C driver.

These did not manifest under interrupt mode, but they were
still present and could be triggered when running in PIO mode.

This series addresses those issues and adds support for PIO
mode transfers.

Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Troy Mitchell (6):
      i2c: spacemit: ensure bus release check runs when wait_bus_idle() fails
      i2c: spacemit: remove stop function to avoid bus error
      i2c: spacemit: disable SDA glitch fix to avoid restart delay
      i2c: spacemit: check SDA instead of SCL after bus reset
      i2c: spacemit: ensure SDA is released after bus reset
      i2c: spacemit: introduce pio for k1

 drivers/i2c/busses/i2c-k1.c | 202 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 163 insertions(+), 39 deletions(-)
---
base-commit: 5d05b05c7086628473f01e860c63110d1b42cd29
change-id: 20250814-k1-i2c-atomic-f1a90cd34364

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

[PATCH 1/6] i2c: spacemit: ensure bus release check runs when wait_bus_idle() fails

[Troy Mitchell]

spacemit_i2c_wait_bus_idle() only returns 0 on success or a negative
error code on failure.

Since 'ret' can never be positive, the final 'else' branch was
unreachable, and spacemit_i2c_check_bus_release() was never called.

This commit guarantees we attempt to release the bus whenever waiting for
an idle bus fails.

Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/i2c/busses/i2c-k1.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index b68a21fff0b56b59fe2032ccb7ca6953423aad32..ee08811f4087c8e709d25dd314854ed643cc5a47 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -476,12 +476,13 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
 	spacemit_i2c_enable(i2c);
 
 	ret = spacemit_i2c_wait_bus_idle(i2c);
-	if (!ret)
+	if (!ret) {
 		ret = spacemit_i2c_xfer_msg(i2c);
-	else if (ret < 0)
-		dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);
-	else
+		if (ret < 0)
+			dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);
+	} else {
 		spacemit_i2c_check_bus_release(i2c);
+	}
 
 	spacemit_i2c_disable(i2c);
 

-- 
2.50.1

Re: [PATCH 1/6] i2c: spacemit: ensure bus release check runs when wait_bus_idle() fails

[Aurelien Jarno]

On 2025-08-27 15:39, Troy Mitchell wrote:
> spacemit_i2c_wait_bus_idle() only returns 0 on success or a negative
> error code on failure.
> 
> Since 'ret' can never be positive, the final 'else' branch was
> unreachable, and spacemit_i2c_check_bus_release() was never called.
> 
> This commit guarantees we attempt to release the bus whenever waiting for
> an idle bus fails.
> 
> Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/i2c/busses/i2c-k1.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index b68a21fff0b56b59fe2032ccb7ca6953423aad32..ee08811f4087c8e709d25dd314854ed643cc5a47 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -476,12 +476,13 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
>  	spacemit_i2c_enable(i2c);
>  
>  	ret = spacemit_i2c_wait_bus_idle(i2c);
> -	if (!ret)
> +	if (!ret) {
>  		ret = spacemit_i2c_xfer_msg(i2c);
> -	else if (ret < 0)
> -		dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);
> -	else
> +		if (ret < 0)
> +			dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);
> +	} else {
>  		spacemit_i2c_check_bus_release(i2c);
> +	}
>  
>  	spacemit_i2c_disable(i2c);

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

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

[PATCH 2/6] i2c: spacemit: remove stop function to avoid bus error

[Troy Mitchell]

Previously, STOP handling was split into two separate steps:
  1) clear TB/STOP/START/ACK bits
  2) issue STOP by calling spacemit_i2c_stop()

This left a small window where the control register was updated
twice, which can confuse the controller. While this race has not
been observed with interrupt-driven transfers, it reliably causes
bus errors in PIO mode.

Inline the STOP sequence into the IRQ handler and ensure that
control register bits are updated atomically in a single writel().

Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/i2c/busses/i2c-k1.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index ee08811f4087c8e709d25dd314854ed643cc5a47..d342752030d077953adf84a2886211de96e843c4 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -267,19 +267,6 @@ static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
 	writel(val, i2c->base + SPACEMIT_ICR);
 }
 
-static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
-{
-	u32 val;
-
-	val = readl(i2c->base + SPACEMIT_ICR);
-	val |= SPACEMIT_CR_STOP | SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
-
-	if (i2c->read)
-		val |= SPACEMIT_CR_ACKNAK;
-
-	writel(val, i2c->base + SPACEMIT_ICR);
-}
-
 static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
 {
 	unsigned long time_left;
@@ -412,7 +399,6 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
 
 	val = readl(i2c->base + SPACEMIT_ICR);
 	val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START);
-	writel(val, i2c->base + SPACEMIT_ICR);
 
 	switch (i2c->state) {
 	case SPACEMIT_STATE_START:
@@ -429,14 +415,18 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
 	}
 
 	if (i2c->state != SPACEMIT_STATE_IDLE) {
+		val |= SPACEMIT_CR_TB;
+		if (i2c->is_pio)
+			val |= SPACEMIT_CR_ALDIE;
+
 		if (spacemit_i2c_is_last_msg(i2c)) {
 			/* trigger next byte with stop */
-			spacemit_i2c_stop(i2c);
-		} else {
-			/* trigger next byte */
-			val |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
-			writel(val, i2c->base + SPACEMIT_ICR);
+			val |= SPACEMIT_CR_STOP;
+
+			if (i2c->read)
+				val |= SPACEMIT_CR_ACKNAK;
 		}
+		writel(val, i2c->base + SPACEMIT_ICR);
 	}
 
 err_out:

-- 
2.50.1

Re: [PATCH 2/6] i2c: spacemit: remove stop function to avoid bus error

[Aurelien Jarno]

On 2025-08-27 15:39, Troy Mitchell wrote:
> Previously, STOP handling was split into two separate steps:
>   1) clear TB/STOP/START/ACK bits
>   2) issue STOP by calling spacemit_i2c_stop()
> 
> This left a small window where the control register was updated
> twice, which can confuse the controller. While this race has not
> been observed with interrupt-driven transfers, it reliably causes
> bus errors in PIO mode.
> 
> Inline the STOP sequence into the IRQ handler and ensure that
> control register bits are updated atomically in a single writel().
> 
> Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/i2c/busses/i2c-k1.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index ee08811f4087c8e709d25dd314854ed643cc5a47..d342752030d077953adf84a2886211de96e843c4 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -267,19 +267,6 @@ static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
>  	writel(val, i2c->base + SPACEMIT_ICR);
>  }
>  
> -static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
> -{
> -	u32 val;
> -
> -	val = readl(i2c->base + SPACEMIT_ICR);
> -	val |= SPACEMIT_CR_STOP | SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
> -
> -	if (i2c->read)
> -		val |= SPACEMIT_CR_ACKNAK;
> -
> -	writel(val, i2c->base + SPACEMIT_ICR);
> -}
> -
>  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>  {
>  	unsigned long time_left;
> @@ -412,7 +399,6 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>  
>  	val = readl(i2c->base + SPACEMIT_ICR);
>  	val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START);
> -	writel(val, i2c->base + SPACEMIT_ICR);
>  
>  	switch (i2c->state) {
>  	case SPACEMIT_STATE_START:
> @@ -429,14 +415,18 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>  	}
>  
>  	if (i2c->state != SPACEMIT_STATE_IDLE) {
> +		val |= SPACEMIT_CR_TB;
> +		if (i2c->is_pio)
> +			val |= SPACEMIT_CR_ALDIE;
> +

This needs to be moved to the last patch introducing PIO support.

>  		if (spacemit_i2c_is_last_msg(i2c)) {
>  			/* trigger next byte with stop */
> -			spacemit_i2c_stop(i2c);
> -		} else {
> -			/* trigger next byte */
> -			val |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
> -			writel(val, i2c->base + SPACEMIT_ICR);
> +			val |= SPACEMIT_CR_STOP;
> +
> +			if (i2c->read)
> +				val |= SPACEMIT_CR_ACKNAK;
>  		}
> +		writel(val, i2c->base + SPACEMIT_ICR);
>  	}
>  
>  err_out:

Otherwise sounds good.

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

Re: [PATCH 2/6] i2c: spacemit: remove stop function to avoid bus error

[Troy Mitchell]

On Mon, Sep 22, 2025 at 10:55:57PM +0200, Aurelien Jarno wrote:
> On 2025-08-27 15:39, Troy Mitchell wrote:
> > Previously, STOP handling was split into two separate steps:
> >   1) clear TB/STOP/START/ACK bits
> >   2) issue STOP by calling spacemit_i2c_stop()
> > 
> > This left a small window where the control register was updated
> > twice, which can confuse the controller. While this race has not
> > been observed with interrupt-driven transfers, it reliably causes
> > bus errors in PIO mode.
> > 
> > Inline the STOP sequence into the IRQ handler and ensure that
> > control register bits are updated atomically in a single writel().
> > 
> > Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> >  drivers/i2c/busses/i2c-k1.c | 28 +++++++++-------------------
> >  1 file changed, 9 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index ee08811f4087c8e709d25dd314854ed643cc5a47..d342752030d077953adf84a2886211de96e843c4 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -429,14 +415,18 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> >  	}
> >  
> >  	if (i2c->state != SPACEMIT_STATE_IDLE) {
> > +		val |= SPACEMIT_CR_TB;
> > +		if (i2c->is_pio)
> > +			val |= SPACEMIT_CR_ALDIE;
> > +
> 
> This needs to be moved to the last patch introducing PIO support.
Nice catch!
I'll fix it and send v2 today.
> 
> >  		if (spacemit_i2c_is_last_msg(i2c)) {
> >  			/* trigger next byte with stop */
> > -			spacemit_i2c_stop(i2c);
> > -		} else {
> > -			/* trigger next byte */
> > -			val |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
> > -			writel(val, i2c->base + SPACEMIT_ICR);
> > +			val |= SPACEMIT_CR_STOP;
> > +
> > +			if (i2c->read)
> > +				val |= SPACEMIT_CR_ACKNAK;
> >  		}
> > +		writel(val, i2c->base + SPACEMIT_ICR);
> >  	}
> >  
> >  err_out:
> 
> Otherwise sounds good.
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
> 

[PATCH 3/6] i2c: spacemit: disable SDA glitch fix to avoid restart delay

[Troy Mitchell]

The K1 I2C controller has an SDA glitch fix that introduces a small
delay on restart signals. While this feature can suppress glitches
on SDA when SCL = 0, it also delays the restart signal, which may
cause unexpected behavior in some transfers.

The glitch itself does not affect normal I2C operation, because
the I2C specification allows SDA to change while SCL is low.

To ensure correct transmission for every message, we disable the
SDA glitch fix by setting the RCR.SDA_GLITCH_NOFIX bit during
initialization.

This guarantees that restarts are issued promptly without
unintended delays.

Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/i2c/busses/i2c-k1.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index d342752030d077953adf84a2886211de96e843c4..c1656b78f1681729ccc2ebca6e290259d14889d9 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -14,6 +14,7 @@
 #define SPACEMIT_ICR		 0x0		/* Control register */
 #define SPACEMIT_ISR		 0x4		/* Status register */
 #define SPACEMIT_IDBR		 0xc		/* Data buffer register */
+#define SPACEMIT_IRCR		 0x18		/* Reset cycle counter */
 #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
 
 /* SPACEMIT_ICR register fields */
@@ -76,6 +77,8 @@
 					SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \
 					SPACEMIT_SR_ALD)
 
+#define SPACEMIT_RCR_SDA_GLITCH_NOFIX		BIT(7)		/* bypass the SDA glitch fix */
+
 /* SPACEMIT_IBMR register fields */
 #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
 #define SPACEMIT_BMR_SCL         BIT(1)		/* SCL line level */
@@ -237,6 +240,14 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
 
 	writel(val, i2c->base + SPACEMIT_ICR);
+
+	/*
+	 * The glitch fix in the K1 I2C controller introduces a delay
+	 * on restart signals, so we disable the fix here.
+	 */
+	val = readl(i2c->base + SPACEMIT_IRCR);
+	val |= SPACEMIT_RCR_SDA_GLITCH_NOFIX;
+	writel(val, i2c->base + SPACEMIT_IRCR);
 }
 
 static inline void

-- 
2.50.1

Re: [PATCH 3/6] i2c: spacemit: disable SDA glitch fix to avoid restart delay

[Troy Mitchell]

On Wed, Aug 27, 2025 at 03:39:10PM +0800, Troy Mitchell wrote:
> The K1 I2C controller has an SDA glitch fix that introduces a small
> delay on restart signals. While this feature can suppress glitches
> on SDA when SCL = 0, it also delays the restart signal, which may
> cause unexpected behavior in some transfers.
> 
> The glitch itself does not affect normal I2C operation, because
> the I2C specification allows SDA to change while SCL is low.
> 
> To ensure correct transmission for every message, we disable the
> SDA glitch fix by setting the RCR.SDA_GLITCH_NOFIX bit during
> initialization.
> 
> This guarantees that restarts are issued promptly without
> unintended delays.
These are the restart waveforms before [1] and after [2] disabling the fix.

Link:
https://psce.pw/839rzq [1]
https://psce.pw/839s4q [2]

---
And I'll put above information into the commit message in the next version.

                 - Troy
> 
> Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/i2c/busses/i2c-k1.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index d342752030d077953adf84a2886211de96e843c4..c1656b78f1681729ccc2ebca6e290259d14889d9 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -14,6 +14,7 @@
>  #define SPACEMIT_ICR		 0x0		/* Control register */
>  #define SPACEMIT_ISR		 0x4		/* Status register */
>  #define SPACEMIT_IDBR		 0xc		/* Data buffer register */
> +#define SPACEMIT_IRCR		 0x18		/* Reset cycle counter */
>  #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
>  
>  /* SPACEMIT_ICR register fields */
> @@ -76,6 +77,8 @@
>  					SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \
>  					SPACEMIT_SR_ALD)
>  
> +#define SPACEMIT_RCR_SDA_GLITCH_NOFIX		BIT(7)		/* bypass the SDA glitch fix */
> +
>  /* SPACEMIT_IBMR register fields */
>  #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
>  #define SPACEMIT_BMR_SCL         BIT(1)		/* SCL line level */
> @@ -237,6 +240,14 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>  	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
>  
>  	writel(val, i2c->base + SPACEMIT_ICR);
> +
> +	/*
> +	 * The glitch fix in the K1 I2C controller introduces a delay
> +	 * on restart signals, so we disable the fix here.
> +	 */
> +	val = readl(i2c->base + SPACEMIT_IRCR);
> +	val |= SPACEMIT_RCR_SDA_GLITCH_NOFIX;
> +	writel(val, i2c->base + SPACEMIT_IRCR);
>  }
>  
>  static inline void
> 
> -- 
> 2.50.1
> 

Re: [PATCH 3/6] i2c: spacemit: disable SDA glitch fix to avoid restart delay

[Aurelien Jarno]

On 2025-08-27 15:39, Troy Mitchell wrote:
> The K1 I2C controller has an SDA glitch fix that introduces a small
> delay on restart signals. While this feature can suppress glitches
> on SDA when SCL = 0, it also delays the restart signal, which may
> cause unexpected behavior in some transfers.
> 
> The glitch itself does not affect normal I2C operation, because
> the I2C specification allows SDA to change while SCL is low.
> 
> To ensure correct transmission for every message, we disable the
> SDA glitch fix by setting the RCR.SDA_GLITCH_NOFIX bit during
> initialization.
> 
> This guarantees that restarts are issued promptly without
> unintended delays.
> 
> Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/i2c/busses/i2c-k1.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index d342752030d077953adf84a2886211de96e843c4..c1656b78f1681729ccc2ebca6e290259d14889d9 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -14,6 +14,7 @@
>  #define SPACEMIT_ICR		 0x0		/* Control register */
>  #define SPACEMIT_ISR		 0x4		/* Status register */
>  #define SPACEMIT_IDBR		 0xc		/* Data buffer register */
> +#define SPACEMIT_IRCR		 0x18		/* Reset cycle counter */
>  #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
>  
>  /* SPACEMIT_ICR register fields */
> @@ -76,6 +77,8 @@
>  					SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \
>  					SPACEMIT_SR_ALD)
>  
> +#define SPACEMIT_RCR_SDA_GLITCH_NOFIX		BIT(7)		/* bypass the SDA glitch fix */
> +

The datasheet seems to use FIX_BYPASS instead of NOFIX, but maybe you 
have chosen the later because it's shorter.

>  /* SPACEMIT_IBMR register fields */
>  #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
>  #define SPACEMIT_BMR_SCL         BIT(1)		/* SCL line level */
> @@ -237,6 +240,14 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>  	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
>  
>  	writel(val, i2c->base + SPACEMIT_ICR);
> +
> +	/*
> +	 * The glitch fix in the K1 I2C controller introduces a delay
> +	 * on restart signals, so we disable the fix here.
> +	 */
> +	val = readl(i2c->base + SPACEMIT_IRCR);
> +	val |= SPACEMIT_RCR_SDA_GLITCH_NOFIX;
> +	writel(val, i2c->base + SPACEMIT_IRCR);
>  }
>  
>  static inline void

I trust you on the waveform captures, with that:

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

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

Re: [PATCH 3/6] i2c: spacemit: disable SDA glitch fix to avoid restart delay

[Troy Mitchell]

On Mon, Sep 22, 2025 at 10:56:00PM +0200, Aurelien Jarno wrote:
> On 2025-08-27 15:39, Troy Mitchell wrote:
> > The K1 I2C controller has an SDA glitch fix that introduces a small
> > delay on restart signals. While this feature can suppress glitches
> > on SDA when SCL = 0, it also delays the restart signal, which may
> > cause unexpected behavior in some transfers.
> > 
> > The glitch itself does not affect normal I2C operation, because
> > the I2C specification allows SDA to change while SCL is low.
> > 
> > To ensure correct transmission for every message, we disable the
> > SDA glitch fix by setting the RCR.SDA_GLITCH_NOFIX bit during
> > initialization.
> > 
> > This guarantees that restarts are issued promptly without
> > unintended delays.
> > 
> > Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> >  drivers/i2c/busses/i2c-k1.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index d342752030d077953adf84a2886211de96e843c4..c1656b78f1681729ccc2ebca6e290259d14889d9 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -14,6 +14,7 @@
> >  #define SPACEMIT_ICR		 0x0		/* Control register */
> >  #define SPACEMIT_ISR		 0x4		/* Status register */
> >  #define SPACEMIT_IDBR		 0xc		/* Data buffer register */
> > +#define SPACEMIT_IRCR		 0x18		/* Reset cycle counter */
> >  #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
> >  
> >  /* SPACEMIT_ICR register fields */
> > @@ -76,6 +77,8 @@
> >  					SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \
> >  					SPACEMIT_SR_ALD)
> >  
> > +#define SPACEMIT_RCR_SDA_GLITCH_NOFIX		BIT(7)		/* bypass the SDA glitch fix */
> > +
> 
> The datasheet seems to use FIX_BYPASS instead of NOFIX, but maybe you 
> have chosen the later because it's shorter.
Yes, it is shorter so I chosen that one.

> 
> >  /* SPACEMIT_IBMR register fields */
> >  #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
> >  #define SPACEMIT_BMR_SCL         BIT(1)		/* SCL line level */
> > @@ -237,6 +240,14 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> >  	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
> >  
> >  	writel(val, i2c->base + SPACEMIT_ICR);
> > +
> > +	/*
> > +	 * The glitch fix in the K1 I2C controller introduces a delay
> > +	 * on restart signals, so we disable the fix here.
> > +	 */
> > +	val = readl(i2c->base + SPACEMIT_IRCR);
> > +	val |= SPACEMIT_RCR_SDA_GLITCH_NOFIX;
> > +	writel(val, i2c->base + SPACEMIT_IRCR);
> >  }
> >  
> >  static inline void
> 
> I trust you on the waveform captures, with that:
Tnx!
                - Troy
> 
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
> 

[PATCH 4/6] i2c: spacemit: check SDA instead of SCL after bus reset

[Troy Mitchell]

After calling spacemit_i2c_conditionally_reset_bus(),
the controller should ensure that the SDA line is release
before proceeding.

Previously, the driver checked the SCL line instead,
which does not guarantee that the bus is truly idle.

This patch changes the check to verify SDA. This ensures
proper bus recovery and avoids potential communication errors
after a conditional reset.

Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/i2c/busses/i2c-k1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index c1656b78f1681729ccc2ebca6e290259d14889d9..4d78ee7b6929ee43771e500d4f85d9e55e68b221 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -172,9 +172,9 @@ static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
 	spacemit_i2c_reset(i2c);
 	usleep_range(10, 20);
 
-	/* check scl status again */
+	/* check sda again here */
 	status = readl(i2c->base + SPACEMIT_IBMR);
-	if (!(status & SPACEMIT_BMR_SCL))
+	if (!(status & SPACEMIT_BMR_SDA))
 		dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
 }
 

-- 
2.50.1

Re: [PATCH 4/6] i2c: spacemit: check SDA instead of SCL after bus reset

[Aurelien Jarno]

On 2025-08-27 15:39, Troy Mitchell wrote:
> After calling spacemit_i2c_conditionally_reset_bus(),
> the controller should ensure that the SDA line is release
> before proceeding.
> 
> Previously, the driver checked the SCL line instead,
> which does not guarantee that the bus is truly idle.
> 
> This patch changes the check to verify SDA. This ensures
> proper bus recovery and avoids potential communication errors
> after a conditional reset.
> 
> Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/i2c/busses/i2c-k1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index c1656b78f1681729ccc2ebca6e290259d14889d9..4d78ee7b6929ee43771e500d4f85d9e55e68b221 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -172,9 +172,9 @@ static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
>  	spacemit_i2c_reset(i2c);
>  	usleep_range(10, 20);
>  
> -	/* check scl status again */
> +	/* check sda again here */
>  	status = readl(i2c->base + SPACEMIT_IBMR);
> -	if (!(status & SPACEMIT_BMR_SCL))
> +	if (!(status & SPACEMIT_BMR_SDA))
>  		dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
>  }

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

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

Re: [PATCH 4/6] i2c: spacemit: check SDA instead of SCL after bus reset

[Troy Mitchell]

On Mon, Sep 22, 2025 at 10:56:04PM +0200, Aurelien Jarno wrote:
> On 2025-08-27 15:39, Troy Mitchell wrote:
> > After calling spacemit_i2c_conditionally_reset_bus(),
> > the controller should ensure that the SDA line is release
> > before proceeding.
> > 
> > Previously, the driver checked the SCL line instead,
> > which does not guarantee that the bus is truly idle.
> > 
> > This patch changes the check to verify SDA. This ensures
> > proper bus recovery and avoids potential communication errors
> > after a conditional reset.
> > 
> > Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> >  drivers/i2c/busses/i2c-k1.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index c1656b78f1681729ccc2ebca6e290259d14889d9..4d78ee7b6929ee43771e500d4f85d9e55e68b221 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -172,9 +172,9 @@ static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
> >  	spacemit_i2c_reset(i2c);
> >  	usleep_range(10, 20);
> >  
> > -	/* check scl status again */
> > +	/* check sda again here */
> >  	status = readl(i2c->base + SPACEMIT_IBMR);
> > -	if (!(status & SPACEMIT_BMR_SCL))
> > +	if (!(status & SPACEMIT_BMR_SDA))
> >  		dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
> >  }
> 
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
Thanks for the RB!
                - Troy
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
> 

[PATCH 5/6] i2c: spacemit: ensure SDA is released after bus reset

[Troy Mitchell]

After performing a conditional bus reset, the controller must ensure
that the SDA line is actually released.

Previously, the reset routine only performed a single check,
which could leave the bus in a locked state in some situations.

This patch introduces a loop that toggles the reset cycle and issues
a reset request up to SPACEMIT_BUS_RESET_CLK_CNT_MAX times, checking
SDA after each attempt. If SDA is released before the maximum count,
the function returns early. Otherwise, a warning is emitted.

This change improves bus recovery reliability.

Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/i2c/busses/i2c-k1.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index 4d78ee7b6929ee43771e500d4f85d9e55e68b221..d2c0d20d19ba73baa8b2e9a6acb02b2cc3b7243f 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2024-2025 Troy Mitchell <troymitchell988@gmail.com>
  */
 
+#include <linux/bitfield.h>
  #include <linux/clk.h>
  #include <linux/i2c.h>
  #include <linux/iopoll.h>
@@ -26,7 +27,8 @@
 #define SPACEMIT_CR_MODE_FAST    BIT(8)		/* bus mode (master operation) */
 /* Bit 9 is reserved */
 #define SPACEMIT_CR_UR           BIT(10)	/* unit reset */
-/* Bits 11-12 are reserved */
+#define SPACEMIT_CR_RSTREQ	 BIT(11)	/* i2c bus reset request */
+/* Bit 12 is reserved */
 #define SPACEMIT_CR_SCLE         BIT(13)	/* master clock enable */
 #define SPACEMIT_CR_IUE          BIT(14)	/* unit enable */
 /* Bits 15-17 are reserved */
@@ -78,6 +80,7 @@
 					SPACEMIT_SR_ALD)
 
 #define SPACEMIT_RCR_SDA_GLITCH_NOFIX		BIT(7)		/* bypass the SDA glitch fix */
+#define SPACEMIT_RCR_FIELD_RST_CYC		GENMASK(3, 0)	/* bypass the SDA glitch fix */
 
 /* SPACEMIT_IBMR register fields */
 #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
@@ -91,6 +94,8 @@
 
 #define SPACEMIT_SR_ERR	(SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD)
 
+#define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
+
 enum spacemit_i2c_state {
 	SPACEMIT_STATE_IDLE,
 	SPACEMIT_STATE_START,
@@ -163,6 +168,7 @@ static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
 static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
 {
 	u32 status;
+	u8 clk_cnt;
 
 	/* if bus is locked, reset unit. 0: locked */
 	status = readl(i2c->base + SPACEMIT_IBMR);
@@ -172,6 +178,21 @@ static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
 	spacemit_i2c_reset(i2c);
 	usleep_range(10, 20);
 
+	for (clk_cnt = 0; clk_cnt < SPACEMIT_BUS_RESET_CLK_CNT_MAX; clk_cnt++) {
+		status = readl(i2c->base + SPACEMIT_IBMR);
+		if (status & SPACEMIT_BMR_SDA)
+			break;
+
+		/* There's nothing left to save here, we are about to exit */
+		writel(FIELD_PREP(SPACEMIT_RCR_FIELD_RST_CYC, 1),
+		       i2c->base + SPACEMIT_IRCR);
+		writel(SPACEMIT_CR_RSTREQ, i2c->base + SPACEMIT_ICR);
+		usleep_range(20, 30);
+	}
+
+	if (clk_cnt < SPACEMIT_BUS_RESET_CLK_CNT_MAX)
+		return;
+
 	/* check sda again here */
 	status = readl(i2c->base + SPACEMIT_IBMR);
 	if (!(status & SPACEMIT_BMR_SDA))

-- 
2.50.1

Re: [PATCH 5/6] i2c: spacemit: ensure SDA is released after bus reset

[Aurelien Jarno]

On 2025-08-27 15:39, Troy Mitchell wrote:
> After performing a conditional bus reset, the controller must ensure
> that the SDA line is actually released.
> 
> Previously, the reset routine only performed a single check,
> which could leave the bus in a locked state in some situations.
> 
> This patch introduces a loop that toggles the reset cycle and issues
> a reset request up to SPACEMIT_BUS_RESET_CLK_CNT_MAX times, checking
> SDA after each attempt. If SDA is released before the maximum count,
> the function returns early. Otherwise, a warning is emitted.
> 
> This change improves bus recovery reliability.
> 
> Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/i2c/busses/i2c-k1.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 4d78ee7b6929ee43771e500d4f85d9e55e68b221..d2c0d20d19ba73baa8b2e9a6acb02b2cc3b7243f 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2024-2025 Troy Mitchell <troymitchell988@gmail.com>
>   */
>  
> +#include <linux/bitfield.h>
>   #include <linux/clk.h>
>   #include <linux/i2c.h>
>   #include <linux/iopoll.h>
> @@ -26,7 +27,8 @@
>  #define SPACEMIT_CR_MODE_FAST    BIT(8)		/* bus mode (master operation) */
>  /* Bit 9 is reserved */
>  #define SPACEMIT_CR_UR           BIT(10)	/* unit reset */
> -/* Bits 11-12 are reserved */
> +#define SPACEMIT_CR_RSTREQ	 BIT(11)	/* i2c bus reset request */
> +/* Bit 12 is reserved */
>  #define SPACEMIT_CR_SCLE         BIT(13)	/* master clock enable */
>  #define SPACEMIT_CR_IUE          BIT(14)	/* unit enable */
>  /* Bits 15-17 are reserved */
> @@ -78,6 +80,7 @@
>  					SPACEMIT_SR_ALD)
>  
>  #define SPACEMIT_RCR_SDA_GLITCH_NOFIX		BIT(7)		/* bypass the SDA glitch fix */
> +#define SPACEMIT_RCR_FIELD_RST_CYC		GENMASK(3, 0)	/* bypass the SDA glitch fix */

The comment here seems wrong, datasheet says "The cycles of SCL during 
bus reset."

>  /* SPACEMIT_IBMR register fields */
>  #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
> @@ -91,6 +94,8 @@
>  
>  #define SPACEMIT_SR_ERR	(SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD)
>  
> +#define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
> +
>  enum spacemit_i2c_state {
>  	SPACEMIT_STATE_IDLE,
>  	SPACEMIT_STATE_START,
> @@ -163,6 +168,7 @@ static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
>  static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
>  {
>  	u32 status;
> +	u8 clk_cnt;
>  
>  	/* if bus is locked, reset unit. 0: locked */
>  	status = readl(i2c->base + SPACEMIT_IBMR);
> @@ -172,6 +178,21 @@ static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
>  	spacemit_i2c_reset(i2c);
>  	usleep_range(10, 20);
>  
> +	for (clk_cnt = 0; clk_cnt < SPACEMIT_BUS_RESET_CLK_CNT_MAX; clk_cnt++) {
> +		status = readl(i2c->base + SPACEMIT_IBMR);
> +		if (status & SPACEMIT_BMR_SDA)
> +			break;

What about just adding the return here instead of checking clk_cnt 
below?

> +
> +		/* There's nothing left to save here, we are about to exit */
> +		writel(FIELD_PREP(SPACEMIT_RCR_FIELD_RST_CYC, 1),
> +		       i2c->base + SPACEMIT_IRCR);
> +		writel(SPACEMIT_CR_RSTREQ, i2c->base + SPACEMIT_ICR);
> +		usleep_range(20, 30);
> +	}
> +
> +	if (clk_cnt < SPACEMIT_BUS_RESET_CLK_CNT_MAX)
> +		return;
> +
>  	/* check sda again here */
>  	status = readl(i2c->base + SPACEMIT_IBMR);
>  	if (!(status & SPACEMIT_BMR_SDA))

Once we have exited the loop, I am not sure we should check SDA once 
more, maybe just display the error message directly.

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

Re: [PATCH 5/6] i2c: spacemit: ensure SDA is released after bus reset

[Troy Mitchell]

On Mon, Sep 22, 2025 at 10:56:10PM +0200, Aurelien Jarno wrote:
> On 2025-08-27 15:39, Troy Mitchell wrote:
> > After performing a conditional bus reset, the controller must ensure
> > that the SDA line is actually released.
> > 
> > Previously, the reset routine only performed a single check,
> > which could leave the bus in a locked state in some situations.
> > 
> > This patch introduces a loop that toggles the reset cycle and issues
> > a reset request up to SPACEMIT_BUS_RESET_CLK_CNT_MAX times, checking
> > SDA after each attempt. If SDA is released before the maximum count,
> > the function returns early. Otherwise, a warning is emitted.
> > 
> > This change improves bus recovery reliability.
> > 
> > Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> >  drivers/i2c/busses/i2c-k1.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index 4d78ee7b6929ee43771e500d4f85d9e55e68b221..d2c0d20d19ba73baa8b2e9a6acb02b2cc3b7243f 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -3,6 +3,7 @@
> >   * Copyright (C) 2024-2025 Troy Mitchell <troymitchell988@gmail.com>
> >   */
> >  
> > +#include <linux/bitfield.h>
> >   #include <linux/clk.h>
> >   #include <linux/i2c.h>
> >   #include <linux/iopoll.h>
> > @@ -26,7 +27,8 @@
> >  #define SPACEMIT_CR_MODE_FAST    BIT(8)		/* bus mode (master operation) */
> >  /* Bit 9 is reserved */
> >  #define SPACEMIT_CR_UR           BIT(10)	/* unit reset */
> > -/* Bits 11-12 are reserved */
> > +#define SPACEMIT_CR_RSTREQ	 BIT(11)	/* i2c bus reset request */
> > +/* Bit 12 is reserved */
> >  #define SPACEMIT_CR_SCLE         BIT(13)	/* master clock enable */
> >  #define SPACEMIT_CR_IUE          BIT(14)	/* unit enable */
> >  /* Bits 15-17 are reserved */
> > @@ -78,6 +80,7 @@
> >  					SPACEMIT_SR_ALD)
> >  
> >  #define SPACEMIT_RCR_SDA_GLITCH_NOFIX		BIT(7)		/* bypass the SDA glitch fix */
> > +#define SPACEMIT_RCR_FIELD_RST_CYC		GENMASK(3, 0)	/* bypass the SDA glitch fix */
> 
> The comment here seems wrong, datasheet says "The cycles of SCL during 
> bus reset."
Yes, that's my mistake.
I'll fix it.

> 
> >  /* SPACEMIT_IBMR register fields */
> >  #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
> > @@ -91,6 +94,8 @@
> >  
> >  #define SPACEMIT_SR_ERR	(SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD)
> >  
> > +#define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
> > +
> >  enum spacemit_i2c_state {
> >  	SPACEMIT_STATE_IDLE,
> >  	SPACEMIT_STATE_START,
> > @@ -163,6 +168,7 @@ static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
> >  static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
> >  {
> >  	u32 status;
> > +	u8 clk_cnt;
> >  
> >  	/* if bus is locked, reset unit. 0: locked */
> >  	status = readl(i2c->base + SPACEMIT_IBMR);
> > @@ -172,6 +178,21 @@ static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
> >  	spacemit_i2c_reset(i2c);
> >  	usleep_range(10, 20);
> >  
> > +	for (clk_cnt = 0; clk_cnt < SPACEMIT_BUS_RESET_CLK_CNT_MAX; clk_cnt++) {
> > +		status = readl(i2c->base + SPACEMIT_IBMR);
> > +		if (status & SPACEMIT_BMR_SDA)
> > +			break;
> 
> What about just adding the return here instead of checking clk_cnt 
> below?
> 
See below.
> > +
> > +		/* There's nothing left to save here, we are about to exit */
> > +		writel(FIELD_PREP(SPACEMIT_RCR_FIELD_RST_CYC, 1),
> > +		       i2c->base + SPACEMIT_IRCR);
> > +		writel(SPACEMIT_CR_RSTREQ, i2c->base + SPACEMIT_ICR);
> > +		usleep_range(20, 30);
> > +	}
> > +
> > +	if (clk_cnt < SPACEMIT_BUS_RESET_CLK_CNT_MAX)
> > +		return;
> > +
> >  	/* check sda again here */
> >  	status = readl(i2c->base + SPACEMIT_IBMR);
> >  	if (!(status & SPACEMIT_BMR_SDA))
> 
> Once we have exited the loop, I am not sure we should check SDA once 
> more, maybe just display the error message directly.
I'll double check it.
If it's unnecessary, I'll just return directly.

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

[PATCH 6/6] i2c: spacemit: introduce pio for k1

[Troy Mitchell]

This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
enabling the use of I2C with interrupts disabled.

Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/i2c/busses/i2c-k1.c | 139 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 120 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index d2c0d20d19ba73baa8b2e9a6acb02b2cc3b7243f..e558fe4cbd5a78b5b53b0c02cbbca818b6495d4a 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -124,6 +124,7 @@ struct spacemit_i2c_dev {
 
 	enum spacemit_i2c_state state;
 	bool read;
+	bool is_pio;
 	struct completion complete;
 	u32 status;
 };
@@ -228,7 +229,7 @@ static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
 
 static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 {
-	u32 val;
+	u32 val = 0;
 
 	/*
 	 * Unmask interrupt bits for all xfer mode:
@@ -236,7 +237,8 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 	 * For transaction complete signal, we use master stop
 	 * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
 	 */
-	val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
+	if (!i2c->is_pio)
+		val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
 
 	/*
 	 * Unmask interrupt bits for interrupt xfer mode:
@@ -246,7 +248,8 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 	 * i2c_start function.
 	 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
 	 */
-	val |= SPACEMIT_CR_DRFIE;
+	if (!i2c->is_pio)
+		val |= SPACEMIT_CR_DRFIE;
 
 	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
 		val |= SPACEMIT_CR_MODE_FAST;
@@ -258,7 +261,10 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 	val |= SPACEMIT_CR_SCLE;
 
 	/* enable master stop detected */
-	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
+	val |= SPACEMIT_CR_MSDE;
+
+	if (!i2c->is_pio)
+		val |= SPACEMIT_CR_MSDIE;
 
 	writel(val, i2c->base + SPACEMIT_ICR);
 
@@ -295,10 +301,54 @@ static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
 	/* send start pulse */
 	val = readl(i2c->base + SPACEMIT_ICR);
 	val &= ~SPACEMIT_CR_STOP;
-	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
+	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB;
+
+	if (!i2c->is_pio)
+		val |= SPACEMIT_CR_DTEIE;
+
 	writel(val, i2c->base + SPACEMIT_ICR);
 }
 
+static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid);
+static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
+{
+	u32 msec = jiffies_to_msecs(i2c->adapt.timeout);
+	ktime_t timeout = ktime_add_ms(ktime_get(), msec);
+	int ret;
+
+	while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
+		udelay(10);
+		i2c->status = readl(i2c->base + SPACEMIT_ISR);
+
+		spacemit_i2c_clear_int_status(i2c, i2c->status);
+
+		if (!(i2c->status & SPACEMIT_SR_IRF) && !(i2c->status & SPACEMIT_SR_ITE))
+			continue;
+
+		spacemit_i2c_irq_handler(0, i2c);
+
+		i2c->status = readl(i2c->base + SPACEMIT_ISR);
+
+		/*
+		 * This is the last byte to write of the current message.
+		 * If we do not wait here, control will proceed directly to start(),
+		 * which would overwrite the current data.
+		 */
+		if (!i2c->read && !i2c->unprocessed) {
+			ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
+						i2c->status, i2c->status & SPACEMIT_SR_ITE,
+						30, 1000);
+			if (ret)
+				return 0;
+		}
+	}
+
+	if (i2c->unprocessed)
+		return 0;
+
+	return 1;
+}
+
 static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
 {
 	unsigned long time_left;
@@ -312,10 +362,27 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
 
 		reinit_completion(&i2c->complete);
 
-		spacemit_i2c_start(i2c);
+		if (i2c->is_pio) {
+			disable_irq(i2c->irq);
+
+			/*
+			 * The K1 I2C controller has a quirk:
+			 * when doing PIO transfers, the status register must be cleared
+			 * of all other bits before issuing a START.
+			 * Failing to do so will prevent the transfer from working properly.
+			 */
+			spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
+
+			spacemit_i2c_start(i2c);
+			time_left = spacemit_i2c_wait_pio_xfer(i2c);
+
+			enable_irq(i2c->irq);
+		} else {
+			spacemit_i2c_start(i2c);
+			time_left = wait_for_completion_timeout(&i2c->complete,
+								i2c->adapt.timeout);
+		}
 
-		time_left = wait_for_completion_timeout(&i2c->complete,
-							i2c->adapt.timeout);
 		if (!time_left) {
 			dev_err(i2c->dev, "msg completion timeout\n");
 			spacemit_i2c_conditionally_reset_bus(i2c);
@@ -343,6 +410,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
 
 static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
 {
+	if (!(i2c->status & SPACEMIT_SR_ITE))
+		return;
+
 	/* if transfer completes, SPACEMIT_ISR will handle it */
 	if (i2c->status & SPACEMIT_SR_MSD)
 		return;
@@ -355,14 +425,20 @@ static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
 
 	/* SPACEMIT_STATE_IDLE avoids trigger next byte */
 	i2c->state = SPACEMIT_STATE_IDLE;
-	complete(&i2c->complete);
+
+	if (!i2c->is_pio)
+		complete(&i2c->complete);
 }
 
 static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
 {
+	if (!(i2c->status & SPACEMIT_SR_IRF))
+		return;
+
 	if (i2c->unprocessed) {
 		*i2c->msg_buf++ = readl(i2c->base + SPACEMIT_IDBR);
 		i2c->unprocessed--;
+		return;
 	}
 
 	/* if transfer completes, SPACEMIT_ISR will handle it */
@@ -375,7 +451,9 @@ static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
 
 	/* SPACEMIT_STATE_IDLE avoids trigger next byte */
 	i2c->state = SPACEMIT_STATE_IDLE;
-	complete(&i2c->complete);
+
+	if (!i2c->is_pio)
+		complete(&i2c->complete);
 }
 
 static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
@@ -410,7 +488,9 @@ static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
 	spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
 
 	i2c->state = SPACEMIT_STATE_IDLE;
-	complete(&i2c->complete);
+
+	if (!i2c->is_pio)
+		complete(&i2c->complete);
 }
 
 static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
@@ -418,13 +498,20 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
 	struct spacemit_i2c_dev *i2c = devid;
 	u32 status, val;
 
-	status = readl(i2c->base + SPACEMIT_ISR);
-	if (!status)
-		return IRQ_HANDLED;
+	/*
+	 * In PIO mode, do not read status again.
+	 * It has already been read in wait_pio_xfer(),
+	 * and reading it clears some bits.
+	 */
+	if (!i2c->is_pio) {
+		status = readl(i2c->base + SPACEMIT_ISR);
+		if (!status)
+			return IRQ_HANDLED;
 
-	i2c->status = status;
+		i2c->status = status;
 
-	spacemit_i2c_clear_int_status(i2c, status);
+		spacemit_i2c_clear_int_status(i2c, status);
+	}
 
 	if (i2c->status & SPACEMIT_SR_ERR)
 		goto err_out;
@@ -483,11 +570,14 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
 	i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 10) / i2c->msg_num;
 }
 
-static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
+static inline int
+spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool is_pio)
 {
 	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
 	int ret;
 
+	i2c->is_pio = is_pio;
+
 	i2c->msgs = msgs;
 	i2c->msg_num = num;
 
@@ -510,18 +600,29 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
 
 	if (ret == -ETIMEDOUT || ret == -EAGAIN)
 		dev_err(i2c->dev, "i2c transfer failed, ret %d err 0x%lx\n",
-			  ret, i2c->status & SPACEMIT_SR_ERR);
+			ret, i2c->status & SPACEMIT_SR_ERR);
 
 	return ret < 0 ? ret : num;
 }
 
+static int spacemit_i2c_int_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
+{
+	return spacemit_i2c_xfer(adapt, msgs, num, false);
+}
+
+static int spacemit_i2c_pio_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
+{
+	return spacemit_i2c_xfer(adapt, msgs, num, true);
+}
+
 static u32 spacemit_i2c_func(struct i2c_adapter *adap)
 {
 	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
 }
 
 static const struct i2c_algorithm spacemit_i2c_algo = {
-	.xfer = spacemit_i2c_xfer,
+	.xfer = spacemit_i2c_int_xfer,
+	.xfer_atomic = spacemit_i2c_pio_xfer,
 	.functionality = spacemit_i2c_func,
 };
 

-- 
2.50.1

Re: [PATCH 6/6] i2c: spacemit: introduce pio for k1

[Aurelien Jarno]

On 2025-08-27 15:39, Troy Mitchell wrote:
> This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> enabling the use of I2C with interrupts disabled.
> 
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/i2c/busses/i2c-k1.c | 139 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 120 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index d2c0d20d19ba73baa8b2e9a6acb02b2cc3b7243f..e558fe4cbd5a78b5b53b0c02cbbca818b6495d4a 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -124,6 +124,7 @@ struct spacemit_i2c_dev {
>  
>  	enum spacemit_i2c_state state;
>  	bool read;
> +	bool is_pio;
>  	struct completion complete;
>  	u32 status;
>  };
> @@ -228,7 +229,7 @@ static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
>  
>  static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>  {
> -	u32 val;
> +	u32 val = 0;
>  
>  	/*
>  	 * Unmask interrupt bits for all xfer mode:
> @@ -236,7 +237,8 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>  	 * For transaction complete signal, we use master stop
>  	 * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
>  	 */
> -	val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> +	if (!i2c->is_pio)
> +		val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
>  
>  	/*
>  	 * Unmask interrupt bits for interrupt xfer mode:
> @@ -246,7 +248,8 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>  	 * i2c_start function.
>  	 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
>  	 */
> -	val |= SPACEMIT_CR_DRFIE;
> +	if (!i2c->is_pio)
> +		val |= SPACEMIT_CR_DRFIE;
>  
>  	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
>  		val |= SPACEMIT_CR_MODE_FAST;
> @@ -258,7 +261,10 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>  	val |= SPACEMIT_CR_SCLE;
>  
>  	/* enable master stop detected */
> -	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
> +	val |= SPACEMIT_CR_MSDE;
> +
> +	if (!i2c->is_pio)
> +		val |= SPACEMIT_CR_MSDIE;
>  
>  	writel(val, i2c->base + SPACEMIT_ICR);
>  
> @@ -295,10 +301,54 @@ static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
>  	/* send start pulse */
>  	val = readl(i2c->base + SPACEMIT_ICR);
>  	val &= ~SPACEMIT_CR_STOP;
> -	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
> +	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB;
> +
> +	if (!i2c->is_pio)
> +		val |= SPACEMIT_CR_DTEIE;
> +
>  	writel(val, i2c->base + SPACEMIT_ICR);
>  }

For all the above is_pio conditions, I have a stupid question, and the
answer probably depends on the controller behaviour. 

Given all the individual interrupts are kept disabled, does it make 
sense to disable the controller interrupt in spacemit_i2c_xfer_msg() 
below? Or maybe the reverse question, does it make sense to disable the 
controller interrupt in spacemit_i2c_xfer_msg() given all individual 
interrupts are kept disabled?

> +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid);
> +static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 msec = jiffies_to_msecs(i2c->adapt.timeout);

The i2c->adapt.timeout value is computed without a lot of margin, so I 
wonder if it is also valid in PIO mode where there is more overhead?

Note that I haven't encountered any issue, but OTOH I only tried writing 
one register of the P1 chip.

Maybe this whole computation could be simplified, other adapters seems 
to use a fixed value independent of the message, of between 200 ms to 
6s. That could also fix a timeout if the SCL clock is slowed down by the 
slave (again that's not something I have tried or experienced).

> +	ktime_t timeout = ktime_add_ms(ktime_get(), msec);
> +	int ret;
> +
> +	while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
> +		udelay(10);
> +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> +
> +		spacemit_i2c_clear_int_status(i2c, i2c->status);
> +
> +		if (!(i2c->status & SPACEMIT_SR_IRF) && !(i2c->status & SPACEMIT_SR_ITE))
> +			continue;
> +
> +		spacemit_i2c_irq_handler(0, i2c);
> +
> +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> +
> +		/*
> +		 * This is the last byte to write of the current message.
> +		 * If we do not wait here, control will proceed directly to start(),
> +		 * which would overwrite the current data.
> +		 */
> +		if (!i2c->read && !i2c->unprocessed) {
> +			ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
> +						i2c->status, i2c->status & SPACEMIT_SR_ITE,
> +						30, 1000);

You can't use readl_poll_timeout() in an atomic context. You should use 
readl_poll_timeout_atomic() instead. Note that there is another one to 
fix in spacemit_i2c_wait_bus_idle.

> +			if (ret)
> +				return 0;
> +		}
> +	}
> +
> +	if (i2c->unprocessed)
> +		return 0;
> +
> +	return 1;
> +}
> +
>  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>  {
>  	unsigned long time_left;
> @@ -312,10 +362,27 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>  
>  		reinit_completion(&i2c->complete);
>  
> -		spacemit_i2c_start(i2c);
> +		if (i2c->is_pio) {
> +			disable_irq(i2c->irq);
> +
> +			/*
> +			 * The K1 I2C controller has a quirk:
> +			 * when doing PIO transfers, the status register must be cleared
> +			 * of all other bits before issuing a START.
> +			 * Failing to do so will prevent the transfer from working properly.
> +			 */
> +			spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> +
> +			spacemit_i2c_start(i2c);
> +			time_left = spacemit_i2c_wait_pio_xfer(i2c);
> +
> +			enable_irq(i2c->irq);
> +		} else {
> +			spacemit_i2c_start(i2c);
> +			time_left = wait_for_completion_timeout(&i2c->complete,
> +								i2c->adapt.timeout);
> +		}
>  
> -		time_left = wait_for_completion_timeout(&i2c->complete,
> -							i2c->adapt.timeout);
>  		if (!time_left) {
>  			dev_err(i2c->dev, "msg completion timeout\n");
>  			spacemit_i2c_conditionally_reset_bus(i2c);
> @@ -343,6 +410,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
>  
>  static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
>  {
> +	if (!(i2c->status & SPACEMIT_SR_ITE))
> +		return;
> +
>  	/* if transfer completes, SPACEMIT_ISR will handle it */
>  	if (i2c->status & SPACEMIT_SR_MSD)
>  		return;
> @@ -355,14 +425,20 @@ static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
>  
>  	/* SPACEMIT_STATE_IDLE avoids trigger next byte */
>  	i2c->state = SPACEMIT_STATE_IDLE;
> -	complete(&i2c->complete);
> +
> +	if (!i2c->is_pio)
> +		complete(&i2c->complete);
>  }
>  
>  static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
>  {
> +	if (!(i2c->status & SPACEMIT_SR_IRF))
> +		return;
> +
>  	if (i2c->unprocessed) {
>  		*i2c->msg_buf++ = readl(i2c->base + SPACEMIT_IDBR);
>  		i2c->unprocessed--;
> +		return;
>  	}
>  
>  	/* if transfer completes, SPACEMIT_ISR will handle it */
> @@ -375,7 +451,9 @@ static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
>  
>  	/* SPACEMIT_STATE_IDLE avoids trigger next byte */
>  	i2c->state = SPACEMIT_STATE_IDLE;
> -	complete(&i2c->complete);
> +
> +	if (!i2c->is_pio)
> +		complete(&i2c->complete);
>  }
>  
>  static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
> @@ -410,7 +488,9 @@ static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
>  	spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
>  
>  	i2c->state = SPACEMIT_STATE_IDLE;
> -	complete(&i2c->complete);
> +
> +	if (!i2c->is_pio)
> +		complete(&i2c->complete);
>  }
>  
>  static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> @@ -418,13 +498,20 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>  	struct spacemit_i2c_dev *i2c = devid;
>  	u32 status, val;
>  
> -	status = readl(i2c->base + SPACEMIT_ISR);
> -	if (!status)
> -		return IRQ_HANDLED;
> +	/*
> +	 * In PIO mode, do not read status again.
> +	 * It has already been read in wait_pio_xfer(),
> +	 * and reading it clears some bits.
> +	 */
> +	if (!i2c->is_pio) {
> +		status = readl(i2c->base + SPACEMIT_ISR);
> +		if (!status)
> +			return IRQ_HANDLED;
>  
> -	i2c->status = status;
> +		i2c->status = status;
>  
> -	spacemit_i2c_clear_int_status(i2c, status);
> +		spacemit_i2c_clear_int_status(i2c, status);
> +	}
>  
>  	if (i2c->status & SPACEMIT_SR_ERR)
>  		goto err_out;
> @@ -483,11 +570,14 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
>  	i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 10) / i2c->msg_num;
>  }
>  
> -static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> +static inline int
> +spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool is_pio)
>  {
>  	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
>  	int ret;
>  
> +	i2c->is_pio = is_pio;
> +
>  	i2c->msgs = msgs;
>  	i2c->msg_num = num;
>  
> @@ -510,18 +600,29 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
>  
>  	if (ret == -ETIMEDOUT || ret == -EAGAIN)
>  		dev_err(i2c->dev, "i2c transfer failed, ret %d err 0x%lx\n",
> -			  ret, i2c->status & SPACEMIT_SR_ERR);
> +			ret, i2c->status & SPACEMIT_SR_ERR);
>  
>  	return ret < 0 ? ret : num;
>  }
>  
> +static int spacemit_i2c_int_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> +{
> +	return spacemit_i2c_xfer(adapt, msgs, num, false);
> +}
> +
> +static int spacemit_i2c_pio_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> +{
> +	return spacemit_i2c_xfer(adapt, msgs, num, true);
> +}
> +
>  static u32 spacemit_i2c_func(struct i2c_adapter *adap)
>  {
>  	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
>  }
>  
>  static const struct i2c_algorithm spacemit_i2c_algo = {
> -	.xfer = spacemit_i2c_xfer,
> +	.xfer = spacemit_i2c_int_xfer,
> +	.xfer_atomic = spacemit_i2c_pio_xfer,
>  	.functionality = spacemit_i2c_func,
>  };
>  
> 
> -- 
> 2.50.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

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

Re: [PATCH 6/6] i2c: spacemit: introduce pio for k1

[Troy Mitchell]

On Mon, Sep 22, 2025 at 10:56:41PM +0200, Aurelien Jarno wrote:
> On 2025-08-27 15:39, Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C with interrupts disabled.
> > 
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> >  drivers/i2c/busses/i2c-k1.c | 139 ++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 120 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index d2c0d20d19ba73baa8b2e9a6acb02b2cc3b7243f..e558fe4cbd5a78b5b53b0c02cbbca818b6495d4a 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -124,6 +124,7 @@ struct spacemit_i2c_dev {
> >  
> >  	enum spacemit_i2c_state state;
> >  	bool read;
> > +	bool is_pio;
> >  	struct completion complete;
> >  	u32 status;
> >  };
> > @@ -228,7 +229,7 @@ static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
> >  
> >  static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> >  {
> > -	u32 val;
> > +	u32 val = 0;
> >  
> >  	/*
> >  	 * Unmask interrupt bits for all xfer mode:
> > @@ -236,7 +237,8 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> >  	 * For transaction complete signal, we use master stop
> >  	 * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
> >  	 */
> > -	val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> > +	if (!i2c->is_pio)
> > +		val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> >  
> >  	/*
> >  	 * Unmask interrupt bits for interrupt xfer mode:
> > @@ -246,7 +248,8 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> >  	 * i2c_start function.
> >  	 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
> >  	 */
> > -	val |= SPACEMIT_CR_DRFIE;
> > +	if (!i2c->is_pio)
> > +		val |= SPACEMIT_CR_DRFIE;
> >  
> >  	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> >  		val |= SPACEMIT_CR_MODE_FAST;
> > @@ -258,7 +261,10 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> >  	val |= SPACEMIT_CR_SCLE;
> >  
> >  	/* enable master stop detected */
> > -	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
> > +	val |= SPACEMIT_CR_MSDE;
> > +
> > +	if (!i2c->is_pio)
> > +		val |= SPACEMIT_CR_MSDIE;
> >  
> >  	writel(val, i2c->base + SPACEMIT_ICR);
> >  
> > @@ -295,10 +301,54 @@ static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
> >  	/* send start pulse */
> >  	val = readl(i2c->base + SPACEMIT_ICR);
> >  	val &= ~SPACEMIT_CR_STOP;
> > -	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
> > +	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB;
> > +
> > +	if (!i2c->is_pio)
> > +		val |= SPACEMIT_CR_DTEIE;
> > +
> >  	writel(val, i2c->base + SPACEMIT_ICR);
> >  }
> 
> For all the above is_pio conditions, I have a stupid question, and the
> answer probably depends on the controller behaviour. 
> 
> Given all the individual interrupts are kept disabled, does it make 
> sense to disable the controller interrupt in spacemit_i2c_xfer_msg() 
> below? Or maybe the reverse question, does it make sense to disable the 
> controller interrupt in spacemit_i2c_xfer_msg() given all individual 
> interrupts are kept disabled?
I'll double check this.
If disabling the individual interrupt bits is not really necessary,
I'll drop them and just disable the controller interrupt in
spacemit_i2c_xfer_msg().

Good catch, thanks!
> 
> > +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid);
> > +static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
> > +{
> > +	u32 msec = jiffies_to_msecs(i2c->adapt.timeout);
> 
> The i2c->adapt.timeout value is computed without a lot of margin, so I 
> wonder if it is also valid in PIO mode where there is more overhead?
> 
> Note that I haven't encountered any issue, but OTOH I only tried writing 
> one register of the P1 chip.
> 
> Maybe this whole computation could be simplified, other adapters seems 
> to use a fixed value independent of the message, of between 200 ms to 
> 6s. That could also fix a timeout if the SCL clock is slowed down by the 
> slave (again that's not something I have tried or experienced).
>
Thanks for pointing this out. You are right that the current timeout is
derived quite tightly from the message length and bus clock, and I only
tested simple register writes so far. In PIO mode the additional CPU
overhead, or in case a slave stretches SCL, this margin may indeed not
be sufficient.

I agree that using a fixed timeout as done in other adapters could make
the behaviour more robust. I can update the driver to use a fixed value
in PIO mode, or at least increase the margin, to avoid spurious
timeouts.
> 
> > +	ktime_t timeout = ktime_add_ms(ktime_get(), msec);
> > +	int ret;
> > +
> > +	while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
> > +		udelay(10);
> > +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> > +
> > +		spacemit_i2c_clear_int_status(i2c, i2c->status);
> > +
> > +		if (!(i2c->status & SPACEMIT_SR_IRF) && !(i2c->status & SPACEMIT_SR_ITE))
> > +			continue;
> > +
> > +		spacemit_i2c_irq_handler(0, i2c);
> > +
> > +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> > +
> > +		/*
> > +		 * This is the last byte to write of the current message.
> > +		 * If we do not wait here, control will proceed directly to start(),
> > +		 * which would overwrite the current data.
> > +		 */
> > +		if (!i2c->read && !i2c->unprocessed) {
> > +			ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
> > +						i2c->status, i2c->status & SPACEMIT_SR_ITE,
> > +						30, 1000);
> 
> You can't use readl_poll_timeout() in an atomic context. You should use 
> readl_poll_timeout_atomic() instead. Note that there is another one to 
> fix in spacemit_i2c_wait_bus_idle.
will fix it. :0

Thanks!

                - Troy
> 
> > +			if (ret)
> > +				return 0;
> > +		}
> > +	}
> > +
> > +	if (i2c->unprocessed)
> > +		return 0;
> > +
> > +	return 1;
> > +}
> > +
> >  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> >  {
> >  	unsigned long time_left;
> > @@ -312,10 +362,27 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> >  
> >  		reinit_completion(&i2c->complete);
> >  
> > -		spacemit_i2c_start(i2c);
> > +		if (i2c->is_pio) {
> > +			disable_irq(i2c->irq);
> > +
> > +			/*
> > +			 * The K1 I2C controller has a quirk:
> > +			 * when doing PIO transfers, the status register must be cleared
> > +			 * of all other bits before issuing a START.
> > +			 * Failing to do so will prevent the transfer from working properly.
> > +			 */
> > +			spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> > +
> > +			spacemit_i2c_start(i2c);
> > +			time_left = spacemit_i2c_wait_pio_xfer(i2c);
> > +
> > +			enable_irq(i2c->irq);
> > +		} else {
> > +			spacemit_i2c_start(i2c);
> > +			time_left = wait_for_completion_timeout(&i2c->complete,
> > +								i2c->adapt.timeout);
> > +		}
> >  
> > -		time_left = wait_for_completion_timeout(&i2c->complete,
> > -							i2c->adapt.timeout);
> >  		if (!time_left) {
> >  			dev_err(i2c->dev, "msg completion timeout\n");
> >  			spacemit_i2c_conditionally_reset_bus(i2c);
> > @@ -343,6 +410,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
> >  
> >  static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
> >  {
> > +	if (!(i2c->status & SPACEMIT_SR_ITE))
> > +		return;
> > +
> >  	/* if transfer completes, SPACEMIT_ISR will handle it */
> >  	if (i2c->status & SPACEMIT_SR_MSD)
> >  		return;
> > @@ -355,14 +425,20 @@ static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
> >  
> >  	/* SPACEMIT_STATE_IDLE avoids trigger next byte */
> >  	i2c->state = SPACEMIT_STATE_IDLE;
> > -	complete(&i2c->complete);
> > +
> > +	if (!i2c->is_pio)
> > +		complete(&i2c->complete);
> >  }
> >  
> >  static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
> >  {
> > +	if (!(i2c->status & SPACEMIT_SR_IRF))
> > +		return;
> > +
> >  	if (i2c->unprocessed) {
> >  		*i2c->msg_buf++ = readl(i2c->base + SPACEMIT_IDBR);
> >  		i2c->unprocessed--;
> > +		return;
> >  	}
> >  
> >  	/* if transfer completes, SPACEMIT_ISR will handle it */
> > @@ -375,7 +451,9 @@ static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
> >  
> >  	/* SPACEMIT_STATE_IDLE avoids trigger next byte */
> >  	i2c->state = SPACEMIT_STATE_IDLE;
> > -	complete(&i2c->complete);
> > +
> > +	if (!i2c->is_pio)
> > +		complete(&i2c->complete);
> >  }
> >  
> >  static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
> > @@ -410,7 +488,9 @@ static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
> >  	spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> >  
> >  	i2c->state = SPACEMIT_STATE_IDLE;
> > -	complete(&i2c->complete);
> > +
> > +	if (!i2c->is_pio)
> > +		complete(&i2c->complete);
> >  }
> >  
> >  static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> > @@ -418,13 +498,20 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> >  	struct spacemit_i2c_dev *i2c = devid;
> >  	u32 status, val;
> >  
> > -	status = readl(i2c->base + SPACEMIT_ISR);
> > -	if (!status)
> > -		return IRQ_HANDLED;
> > +	/*
> > +	 * In PIO mode, do not read status again.
> > +	 * It has already been read in wait_pio_xfer(),
> > +	 * and reading it clears some bits.
> > +	 */
> > +	if (!i2c->is_pio) {
> > +		status = readl(i2c->base + SPACEMIT_ISR);
> > +		if (!status)
> > +			return IRQ_HANDLED;
> >  
> > -	i2c->status = status;
> > +		i2c->status = status;
> >  
> > -	spacemit_i2c_clear_int_status(i2c, status);
> > +		spacemit_i2c_clear_int_status(i2c, status);
> > +	}
> >  
> >  	if (i2c->status & SPACEMIT_SR_ERR)
> >  		goto err_out;
> > @@ -483,11 +570,14 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
> >  	i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 10) / i2c->msg_num;
> >  }
> >  
> > -static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > +static inline int
> > +spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool is_pio)
> >  {
> >  	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
> >  	int ret;
> >  
> > +	i2c->is_pio = is_pio;
> > +
> >  	i2c->msgs = msgs;
> >  	i2c->msg_num = num;
> >  
> > @@ -510,18 +600,29 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
> >  
> >  	if (ret == -ETIMEDOUT || ret == -EAGAIN)
> >  		dev_err(i2c->dev, "i2c transfer failed, ret %d err 0x%lx\n",
> > -			  ret, i2c->status & SPACEMIT_SR_ERR);
> > +			ret, i2c->status & SPACEMIT_SR_ERR);
> >  
> >  	return ret < 0 ? ret : num;
> >  }
> >  
> > +static int spacemit_i2c_int_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > +{
> > +	return spacemit_i2c_xfer(adapt, msgs, num, false);
> > +}
> > +
> > +static int spacemit_i2c_pio_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > +{
> > +	return spacemit_i2c_xfer(adapt, msgs, num, true);
> > +}
> > +
> >  static u32 spacemit_i2c_func(struct i2c_adapter *adap)
> >  {
> >  	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> >  }
> >  
> >  static const struct i2c_algorithm spacemit_i2c_algo = {
> > -	.xfer = spacemit_i2c_xfer,
> > +	.xfer = spacemit_i2c_int_xfer,
> > +	.xfer_atomic = spacemit_i2c_pio_xfer,
> >  	.functionality = spacemit_i2c_func,
> >  };
> >  
> > 
> > -- 
> > 2.50.1
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > 
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
> 

Re: [PATCH 6/6] i2c: spacemit: introduce pio for k1

[Troy Mitchell]

On Tue, Sep 23, 2025 at 09:14:38AM +0800, Troy Mitchell wrote:
> On Mon, Sep 22, 2025 at 10:56:41PM +0200, Aurelien Jarno wrote:
> > On 2025-08-27 15:39, Troy Mitchell wrote:
> > > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > > enabling the use of I2C with interrupts disabled.
> > > 
> > > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > > ---
> > >  drivers/i2c/busses/i2c-k1.c | 139 ++++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 120 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > > index d2c0d20d19ba73baa8b2e9a6acb02b2cc3b7243f..e558fe4cbd5a78b5b53b0c02cbbca818b6495d4a 100644
> > > --- a/drivers/i2c/busses/i2c-k1.c
> > > +++ b/drivers/i2c/busses/i2c-k1.c
> > > @@ -124,6 +124,7 @@ struct spacemit_i2c_dev {
> > >  
> > >  	enum spacemit_i2c_state state;
> > >  	bool read;
> > > +	bool is_pio;
> > >  	struct completion complete;
> > >  	u32 status;
> > >  };
> > > @@ -228,7 +229,7 @@ static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
> > >  
> > >  static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> > >  {
> > > -	u32 val;
> > > +	u32 val = 0;
> > >  
> > >  	/*
> > >  	 * Unmask interrupt bits for all xfer mode:
> > > @@ -236,7 +237,8 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> > >  	 * For transaction complete signal, we use master stop
> > >  	 * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
> > >  	 */
> > > -	val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> > > +	if (!i2c->is_pio)
> > > +		val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> > >  
> > >  	/*
> > >  	 * Unmask interrupt bits for interrupt xfer mode:
> > > @@ -246,7 +248,8 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> > >  	 * i2c_start function.
> > >  	 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
> > >  	 */
> > > -	val |= SPACEMIT_CR_DRFIE;
> > > +	if (!i2c->is_pio)
> > > +		val |= SPACEMIT_CR_DRFIE;
> > >  
> > >  	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> > >  		val |= SPACEMIT_CR_MODE_FAST;
> > > @@ -258,7 +261,10 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> > >  	val |= SPACEMIT_CR_SCLE;
> > >  
> > >  	/* enable master stop detected */
> > > -	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
> > > +	val |= SPACEMIT_CR_MSDE;
> > > +
> > > +	if (!i2c->is_pio)
> > > +		val |= SPACEMIT_CR_MSDIE;
> > >  
> > >  	writel(val, i2c->base + SPACEMIT_ICR);
> > >  
> > > @@ -295,10 +301,54 @@ static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
> > >  	/* send start pulse */
> > >  	val = readl(i2c->base + SPACEMIT_ICR);
> > >  	val &= ~SPACEMIT_CR_STOP;
> > > -	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
> > > +	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB;
> > > +
> > > +	if (!i2c->is_pio)
> > > +		val |= SPACEMIT_CR_DTEIE;
> > > +
> > >  	writel(val, i2c->base + SPACEMIT_ICR);
> > >  }
> > 
> > For all the above is_pio conditions, I have a stupid question, and the
> > answer probably depends on the controller behaviour. 
> > 
> > Given all the individual interrupts are kept disabled, does it make 
> > sense to disable the controller interrupt in spacemit_i2c_xfer_msg() 
> > below? Or maybe the reverse question, does it make sense to disable the 
> > controller interrupt in spacemit_i2c_xfer_msg() given all individual 
> > interrupts are kept disabled?
> I'll double check this.
> If disabling the individual interrupt bits is not really necessary,
> I'll drop them and just disable the controller interrupt in
> spacemit_i2c_xfer_msg().
> 
> Good catch, thanks!
I'll disable the controller interrupt in xfer_msg().

                - Troy
> > 
> > > +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid);
> > > +static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
> > > +{
> > > +	u32 msec = jiffies_to_msecs(i2c->adapt.timeout);
> > 
> > The i2c->adapt.timeout value is computed without a lot of margin, so I 
> > wonder if it is also valid in PIO mode where there is more overhead?
> > 
> > Note that I haven't encountered any issue, but OTOH I only tried writing 
> > one register of the P1 chip.
> > 
> > Maybe this whole computation could be simplified, other adapters seems 
> > to use a fixed value independent of the message, of between 200 ms to 
> > 6s. That could also fix a timeout if the SCL clock is slowed down by the 
> > slave (again that's not something I have tried or experienced).
> >
> Thanks for pointing this out. You are right that the current timeout is
> derived quite tightly from the message length and bus clock, and I only
> tested simple register writes so far. In PIO mode the additional CPU
> overhead, or in case a slave stretches SCL, this margin may indeed not
> be sufficient.
> 
> I agree that using a fixed timeout as done in other adapters could make
> the behaviour more robust. I can update the driver to use a fixed value
> in PIO mode, or at least increase the margin, to avoid spurious
> timeouts.
> > 
> > > +	ktime_t timeout = ktime_add_ms(ktime_get(), msec);
> > > +	int ret;
> > > +
> > > +	while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
> > > +		udelay(10);
> > > +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> > > +
> > > +		spacemit_i2c_clear_int_status(i2c, i2c->status);
> > > +
> > > +		if (!(i2c->status & SPACEMIT_SR_IRF) && !(i2c->status & SPACEMIT_SR_ITE))
> > > +			continue;
> > > +
> > > +		spacemit_i2c_irq_handler(0, i2c);
> > > +
> > > +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> > > +
> > > +		/*
> > > +		 * This is the last byte to write of the current message.
> > > +		 * If we do not wait here, control will proceed directly to start(),
> > > +		 * which would overwrite the current data.
> > > +		 */
> > > +		if (!i2c->read && !i2c->unprocessed) {
> > > +			ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
> > > +						i2c->status, i2c->status & SPACEMIT_SR_ITE,
> > > +						30, 1000);
> > 
> > You can't use readl_poll_timeout() in an atomic context. You should use 
> > readl_poll_timeout_atomic() instead. Note that there is another one to 
> > fix in spacemit_i2c_wait_bus_idle.
> will fix it. :0
> 
> Thanks!
> 
>                 - Troy
> > 
> > > +			if (ret)
> > > +				return 0;
> > > +		}
> > > +	}
> > > +
> > > +	if (i2c->unprocessed)
> > > +		return 0;
> > > +
> > > +	return 1;
> > > +}
> > > +
> > >  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> > >  {
> > >  	unsigned long time_left;
> > > @@ -312,10 +362,27 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> > >  
> > >  		reinit_completion(&i2c->complete);
> > >  
> > > -		spacemit_i2c_start(i2c);
> > > +		if (i2c->is_pio) {
> > > +			disable_irq(i2c->irq);
> > > +
> > > +			/*
> > > +			 * The K1 I2C controller has a quirk:
> > > +			 * when doing PIO transfers, the status register must be cleared
> > > +			 * of all other bits before issuing a START.
> > > +			 * Failing to do so will prevent the transfer from working properly.
> > > +			 */
> > > +			spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> > > +
> > > +			spacemit_i2c_start(i2c);
> > > +			time_left = spacemit_i2c_wait_pio_xfer(i2c);
> > > +
> > > +			enable_irq(i2c->irq);
> > > +		} else {
> > > +			spacemit_i2c_start(i2c);
> > > +			time_left = wait_for_completion_timeout(&i2c->complete,
> > > +								i2c->adapt.timeout);
> > > +		}
> > >  
> > > -		time_left = wait_for_completion_timeout(&i2c->complete,
> > > -							i2c->adapt.timeout);
> > >  		if (!time_left) {
> > >  			dev_err(i2c->dev, "msg completion timeout\n");
> > >  			spacemit_i2c_conditionally_reset_bus(i2c);
> > > @@ -343,6 +410,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
> > >  
> > >  static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
> > >  {
> > > +	if (!(i2c->status & SPACEMIT_SR_ITE))
> > > +		return;
> > > +
> > >  	/* if transfer completes, SPACEMIT_ISR will handle it */
> > >  	if (i2c->status & SPACEMIT_SR_MSD)
> > >  		return;
> > > @@ -355,14 +425,20 @@ static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
> > >  
> > >  	/* SPACEMIT_STATE_IDLE avoids trigger next byte */
> > >  	i2c->state = SPACEMIT_STATE_IDLE;
> > > -	complete(&i2c->complete);
> > > +
> > > +	if (!i2c->is_pio)
> > > +		complete(&i2c->complete);
> > >  }
> > >  
> > >  static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
> > >  {
> > > +	if (!(i2c->status & SPACEMIT_SR_IRF))
> > > +		return;
> > > +
> > >  	if (i2c->unprocessed) {
> > >  		*i2c->msg_buf++ = readl(i2c->base + SPACEMIT_IDBR);
> > >  		i2c->unprocessed--;
> > > +		return;
> > >  	}
> > >  
> > >  	/* if transfer completes, SPACEMIT_ISR will handle it */
> > > @@ -375,7 +451,9 @@ static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
> > >  
> > >  	/* SPACEMIT_STATE_IDLE avoids trigger next byte */
> > >  	i2c->state = SPACEMIT_STATE_IDLE;
> > > -	complete(&i2c->complete);
> > > +
> > > +	if (!i2c->is_pio)
> > > +		complete(&i2c->complete);
> > >  }
> > >  
> > >  static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
> > > @@ -410,7 +488,9 @@ static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
> > >  	spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> > >  
> > >  	i2c->state = SPACEMIT_STATE_IDLE;
> > > -	complete(&i2c->complete);
> > > +
> > > +	if (!i2c->is_pio)
> > > +		complete(&i2c->complete);
> > >  }
> > >  
> > >  static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> > > @@ -418,13 +498,20 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> > >  	struct spacemit_i2c_dev *i2c = devid;
> > >  	u32 status, val;
> > >  
> > > -	status = readl(i2c->base + SPACEMIT_ISR);
> > > -	if (!status)
> > > -		return IRQ_HANDLED;
> > > +	/*
> > > +	 * In PIO mode, do not read status again.
> > > +	 * It has already been read in wait_pio_xfer(),
> > > +	 * and reading it clears some bits.
> > > +	 */
> > > +	if (!i2c->is_pio) {
> > > +		status = readl(i2c->base + SPACEMIT_ISR);
> > > +		if (!status)
> > > +			return IRQ_HANDLED;
> > >  
> > > -	i2c->status = status;
> > > +		i2c->status = status;
> > >  
> > > -	spacemit_i2c_clear_int_status(i2c, status);
> > > +		spacemit_i2c_clear_int_status(i2c, status);
> > > +	}
> > >  
> > >  	if (i2c->status & SPACEMIT_SR_ERR)
> > >  		goto err_out;
> > > @@ -483,11 +570,14 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
> > >  	i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 10) / i2c->msg_num;
> > >  }
> > >  
> > > -static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > > +static inline int
> > > +spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool is_pio)
> > >  {
> > >  	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
> > >  	int ret;
> > >  
> > > +	i2c->is_pio = is_pio;
> > > +
> > >  	i2c->msgs = msgs;
> > >  	i2c->msg_num = num;
> > >  
> > > @@ -510,18 +600,29 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
> > >  
> > >  	if (ret == -ETIMEDOUT || ret == -EAGAIN)
> > >  		dev_err(i2c->dev, "i2c transfer failed, ret %d err 0x%lx\n",
> > > -			  ret, i2c->status & SPACEMIT_SR_ERR);
> > > +			ret, i2c->status & SPACEMIT_SR_ERR);
> > >  
> > >  	return ret < 0 ? ret : num;
> > >  }
> > >  
> > > +static int spacemit_i2c_int_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > > +{
> > > +	return spacemit_i2c_xfer(adapt, msgs, num, false);
> > > +}
> > > +
> > > +static int spacemit_i2c_pio_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > > +{
> > > +	return spacemit_i2c_xfer(adapt, msgs, num, true);
> > > +}
> > > +
> > >  static u32 spacemit_i2c_func(struct i2c_adapter *adap)
> > >  {
> > >  	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> > >  }
> > >  
> > >  static const struct i2c_algorithm spacemit_i2c_algo = {
> > > -	.xfer = spacemit_i2c_xfer,
> > > +	.xfer = spacemit_i2c_int_xfer,
> > > +	.xfer_atomic = spacemit_i2c_pio_xfer,
> > >  	.functionality = spacemit_i2c_func,
> > >  };
> > >  
> > > 
> > > -- 
> > > 2.50.1
> > > 
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > 
> > 
> > -- 
> > Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> > aurelien@aurel32.net                     http://aurel32.net
> > 

Re: [PATCH 0/6] i2c: spacemit: fix and introduce pio

[Aurelien Jarno]

Hi,

On 2025-08-27 15:39, Troy Mitchell wrote:
> Previously, there were a few latent issues in the I2C driver.
> 
> These did not manifest under interrupt mode, but they were
> still present and could be triggered when running in PIO mode.
> 
> This series addresses those issues and adds support for PIO
> mode transfers.
> 
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>

Thanks for this series. Based on it and with the few fixes suggested in 
the individual patches, I have been able to write a small power reset 
driver for the P1 chip, supporting reset and shutdown. I'll post it on 
the list in the next days.

Regards
Aurelien

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

Re: [PATCH 0/6] i2c: spacemit: fix and introduce pio

[Troy Mitchell]

On Mon, Sep 22, 2025 at 10:55:33PM +0200, Aurelien Jarno wrote:
> Hi,
> 
> On 2025-08-27 15:39, Troy Mitchell wrote:
> > Previously, there were a few latent issues in the I2C driver.
> > 
> > These did not manifest under interrupt mode, but they were
> > still present and could be triggered when running in PIO mode.
> > 
> > This series addresses those issues and adds support for PIO
> > mode transfers.
> > 
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> 
> Thanks for this series. Based on it and with the few fixes suggested in 
> the individual patches, I have been able to write a small power reset 
> driver for the P1 chip, supporting reset and shutdown. 
>
Nice job. I'll follow yours. That's what we need.

                - Troy
> I'll post it on 
> the list in the next days.
> 
> Regards
> Aurelien
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
>