[PATCH v4] i2c: spacemit: introduce pio for k1

[PATCH v4] i2c: spacemit: introduce pio for k1

[Troy Mitchell]

This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
enabling the use of I2C in atomic context.

Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Changes in v4:
- refactor for better readability: simplify condition check and moving if/else (timeout/
  wait_xfer_complete) logic into a function
- remove irrelevant changes
- remove the status clear call in spacemit_i2c_xfer_common()
- sort functions to avoid forward declarations,
  move unavoidable ones above function definitions
- use udelay() in atomic context to avoid sleeping
- wait for MSD on the last byte in wait_pio_xfer()
- Link to v3: https://lore.kernel.org/r/20250929-k1-i2c-atomic-v3-1-f7e660c138b6@linux.spacemit.com

Changes in v3:
- drop 1-5 patches (have been merged)
- modify commit message
- use readl_poll_timeout_atomic() in wait_pio_xfer()
- use msecs_to_jiffies() when get PIO mode timeout value
- factor out transfer state handling into spacemit_i2c_handle_state().
- do not disable/enable the controller IRQ around PIO transfers.
- consolidate spacemit_i2c_init() interrupt setup
- rename is_pio -> use_pio
- rename spacemit_i2c_xfer() -> spacemit_i2c_xfer_common()
- rename spacemit_i2c_int_xfer() -> spacemit_i2c_xfer()
- rename spacemit_i2c_pio_xfer() -> spacemit_i2c_pio_xfer_atomic()
- call spacemit_i2c_err_check() in wait_pio_xfer() when write last byte
- Link to v2: https://lore.kernel.org/r/20250925-k1-i2c-atomic-v2-0-46dc13311cda@linux.spacemit.com

Changes in v2:
- Patch 1/6:
  Patch 3/6:
  Patch 4/6:
    - nothing
- Patch 2/6:
  - remove is_pio judgement in irq_handler()
- Patch 5/6:
  - fix wrong comment
  - In spacemit_i2c_conditionally_reset_bus(), once the condition is met inside the loop, it should
    return directly instead of using break.
- Patch 6/6:
  - add is_pio judgement in irq_handler()
  - use a fixed timeout value when PIO
  - use readl_poll_timeout() in  spacemit_i2c_wait_bus_idle() when PIO

- Link to v1: https://lore.kernel.org/r/20250827-k1-i2c-atomic-v1-0-e59bea02d680@linux.spacemit.com
---
 drivers/i2c/busses/i2c-k1.c | 229 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 185 insertions(+), 44 deletions(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index 6b918770e612e098b8ad17418f420d87c94df166..a13284f840ee967466a01375e6603f7568a42f86 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -97,6 +97,8 @@
 
 #define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
 
+#define SPACEMIT_WAIT_TIMEOUT      1000 /* ms */
+
 enum spacemit_i2c_state {
 	SPACEMIT_STATE_IDLE,
 	SPACEMIT_STATE_START,
@@ -125,10 +127,14 @@ struct spacemit_i2c_dev {
 
 	enum spacemit_i2c_state state;
 	bool read;
+	bool use_pio;
 	struct completion complete;
 	u32 status;
 };
 
+static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c);
+static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c);
+
 static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
 {
 	u32 val;
@@ -177,7 +183,11 @@ static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
 		return;
 
 	spacemit_i2c_reset(i2c);
-	usleep_range(10, 20);
+
+	if (!i2c->use_pio)
+		usleep_range(10, 20);
+	else
+		udelay(20);
 
 	for (clk_cnt = 0; clk_cnt < SPACEMIT_BUS_RESET_CLK_CNT_MAX; clk_cnt++) {
 		status = readl(i2c->base + SPACEMIT_IBMR);
@@ -206,9 +216,14 @@ static int spacemit_i2c_wait_bus_idle(struct spacemit_i2c_dev *i2c)
 	if (!(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)))
 		return 0;
 
-	ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
-				 val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
-				 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
+	if (!i2c->use_pio)
+		ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
+					 val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
+					 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
+	else
+		ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
+						val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
+						1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
 	if (ret)
 		spacemit_i2c_reset(i2c);
 
@@ -220,31 +235,39 @@ static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
 	/* in case bus is not released after transfer completes */
 	if (readl(i2c->base + SPACEMIT_ISR) & SPACEMIT_SR_EBB) {
 		spacemit_i2c_conditionally_reset_bus(i2c);
-		usleep_range(90, 150);
+		if (!i2c->use_pio)
+			usleep_range(90, 150);
+		else
+			udelay(150);
 	}
 }
 
 static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 {
-	u32 val;
-
-	/*
-	 * Unmask interrupt bits for all xfer mode:
-	 * bus error, arbitration loss detected.
-	 * 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;
-
-	/*
-	 * Unmask interrupt bits for interrupt xfer mode:
-	 * When IDBR receives a byte, an interrupt is triggered.
-	 *
-	 * For the tx empty interrupt, it will be enabled in the
-	 * i2c_start function.
-	 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
-	 */
-	val |= SPACEMIT_CR_DRFIE;
+	u32 val = 0;
+
+	if (!i2c->use_pio) {
+		/*
+		 * Unmask interrupt bits for all xfer mode:
+		 * bus error, arbitration loss detected.
+		 * 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;
+
+		/*
+		 * Unmask interrupt bits for interrupt xfer mode:
+		 * When IDBR receives a byte, an interrupt is triggered.
+		 *
+		 * For the tx empty interrupt, it will be enabled in the
+		 * i2c_start function.
+		 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
+		 */
+		val |= SPACEMIT_CR_DRFIE;
+
+		/* unmask master stop interrupt bit */
+		val |= SPACEMIT_CR_MSDIE;
+	}
 
 	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
 		val |= SPACEMIT_CR_MODE_FAST;
@@ -256,7 +279,7 @@ 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;
 
 	writel(val, i2c->base + SPACEMIT_ICR);
 
@@ -293,10 +316,85 @@ 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->use_pio)
+		val |= SPACEMIT_CR_DTEIE;
+
 	writel(val, i2c->base + SPACEMIT_ICR);
 }
 
+static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
+{
+	u32 mask, msec = jiffies_to_msecs(i2c->adapt.timeout);
+	ktime_t timeout = ktime_add_ms(ktime_get(), msec);
+	int ret;
+
+	mask = SPACEMIT_SR_IRF | SPACEMIT_SR_ITE;
+
+	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 & mask))
+			continue;
+
+		spacemit_i2c_handle_state(i2c);
+
+		/*
+		 * 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_atomic(i2c->base + SPACEMIT_ISR,
+							i2c->status, i2c->status & SPACEMIT_SR_ITE,
+							30, 1000);
+			if (ret)
+				return 0;
+
+			/*
+			 * In read mode, err_check() in handle_state properly handles what happens
+			 * after the MSD bit is set. For writes it is different: in interrupt mode,
+			 * an ITE (write-empty) interrupt is triggered after the last byte, and the
+			 * MSD-related handling takes place there. In PIO mode, however, we need to
+			 * explicitly call err_check() to emulate this step, otherwise the next
+			 * transfer will fail.
+			 */
+			if (i2c->msg_idx == i2c->msg_num - 1) {
+				mask = SPACEMIT_SR_MSD | SPACEMIT_SR_ERR;
+				/*
+				 * In some cases, MSD may not arrive immediately;
+				 * wait here to handle that.
+				 */
+				ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
+								i2c->status, i2c->status & mask,
+								30, 1000);
+				if (ret)
+					return 0;
+
+				spacemit_i2c_err_check(i2c);
+			}
+		}
+	}
+
+	if (i2c->unprocessed)
+		return 0;
+
+	return 1;
+}
+
+static int spacemit_i2c_wait_xfer_complete(struct spacemit_i2c_dev *i2c)
+{
+	if (i2c->use_pio)
+		return spacemit_i2c_wait_pio_xfer(i2c);
+	else
+		return wait_for_completion_timeout(&i2c->complete,
+						   i2c->adapt.timeout);
+}
+
 static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
 {
 	unsigned long time_left;
@@ -312,8 +410,8 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
 
 		spacemit_i2c_start(i2c);
 
-		time_left = wait_for_completion_timeout(&i2c->complete,
-							i2c->adapt.timeout);
+		time_left = spacemit_i2c_wait_xfer_complete(i2c);
+
 		if (!time_left) {
 			dev_err(i2c->dev, "msg completion timeout\n");
 			spacemit_i2c_conditionally_reset_bus(i2c);
@@ -341,6 +439,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;
@@ -353,14 +454,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->use_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 */
@@ -373,7 +480,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->use_pio)
+		complete(&i2c->complete);
 }
 
 static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
@@ -408,21 +517,14 @@ 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->use_pio)
+		complete(&i2c->complete);
 }
 
-static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
+static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c)
 {
-	struct spacemit_i2c_dev *i2c = devid;
-	u32 status, val;
-
-	status = readl(i2c->base + SPACEMIT_ISR);
-	if (!status)
-		return IRQ_HANDLED;
-
-	i2c->status = status;
-
-	spacemit_i2c_clear_int_status(i2c, status);
+	u32 val;
 
 	if (i2c->status & SPACEMIT_SR_ERR)
 		goto err_out;
@@ -445,7 +547,10 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
 	}
 
 	if (i2c->state != SPACEMIT_STATE_IDLE) {
-		val |= SPACEMIT_CR_TB | SPACEMIT_CR_ALDIE;
+		val |= SPACEMIT_CR_TB;
+		if (i2c->use_pio)
+			val |= SPACEMIT_CR_ALDIE;
+
 
 		if (spacemit_i2c_is_last_msg(i2c)) {
 			/* trigger next byte with stop */
@@ -459,6 +564,23 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
 
 err_out:
 	spacemit_i2c_err_check(i2c);
+}
+
+static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
+{
+	struct spacemit_i2c_dev *i2c = devid;
+	u32 status;
+
+	status = readl(i2c->base + SPACEMIT_ISR);
+	if (!status)
+		return IRQ_HANDLED;
+
+	i2c->status = status;
+
+	spacemit_i2c_clear_int_status(i2c, status);
+
+	spacemit_i2c_handle_state(i2c);
+
 	return IRQ_HANDLED;
 }
 
@@ -467,6 +589,11 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
 	unsigned long timeout;
 	int idx = 0, cnt = 0;
 
+	if (i2c->use_pio) {
+		i2c->adapt.timeout = msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT);
+		return;
+	}
+
 	for (; idx < i2c->msg_num; idx++)
 		cnt += (i2c->msgs + idx)->len + 1;
 
@@ -479,11 +606,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_common(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool use_pio)
 {
 	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
 	int ret;
 
+	i2c->use_pio = use_pio;
+
 	i2c->msgs = msgs;
 	i2c->msg_num = num;
 
@@ -511,6 +641,16 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
 	return ret < 0 ? ret : num;
 }
 
+static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
+{
+	return spacemit_i2c_xfer_common(adapt, msgs, num, false);
+}
+
+static int spacemit_i2c_pio_xfer_atomic(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
+{
+	return spacemit_i2c_xfer_common(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);
@@ -518,6 +658,7 @@ static u32 spacemit_i2c_func(struct i2c_adapter *adap)
 
 static const struct i2c_algorithm spacemit_i2c_algo = {
 	.xfer = spacemit_i2c_xfer,
+	.xfer_atomic = spacemit_i2c_pio_xfer_atomic,
 	.functionality = spacemit_i2c_func,
 };
 

---
base-commit: bc574b64121525b24d52e9bab747184181c808dc
change-id: 20250814-k1-i2c-atomic-f1a90cd34364

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

Re: [PATCH v4] i2c: spacemit: introduce pio for k1

[Aurelien Jarno]

Hi,

On 2025-10-09 17:59, Troy Mitchell wrote:
> This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> enabling the use of I2C in atomic context.
> 
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changes in v4:
> - refactor for better readability: simplify condition check and moving if/else (timeout/
>   wait_xfer_complete) logic into a function
> - remove irrelevant changes
> - remove the status clear call in spacemit_i2c_xfer_common()
> - sort functions to avoid forward declarations,
>   move unavoidable ones above function definitions
> - use udelay() in atomic context to avoid sleeping
> - wait for MSD on the last byte in wait_pio_xfer()
> - Link to v3: https://lore.kernel.org/r/20250929-k1-i2c-atomic-v3-1-f7e660c138b6@linux.spacemit.com
> 
> Changes in v3:
> - drop 1-5 patches (have been merged)
> - modify commit message
> - use readl_poll_timeout_atomic() in wait_pio_xfer()
> - use msecs_to_jiffies() when get PIO mode timeout value
> - factor out transfer state handling into spacemit_i2c_handle_state().
> - do not disable/enable the controller IRQ around PIO transfers.
> - consolidate spacemit_i2c_init() interrupt setup
> - rename is_pio -> use_pio
> - rename spacemit_i2c_xfer() -> spacemit_i2c_xfer_common()
> - rename spacemit_i2c_int_xfer() -> spacemit_i2c_xfer()
> - rename spacemit_i2c_pio_xfer() -> spacemit_i2c_pio_xfer_atomic()
> - call spacemit_i2c_err_check() in wait_pio_xfer() when write last byte
> - Link to v2: https://lore.kernel.org/r/20250925-k1-i2c-atomic-v2-0-46dc13311cda@linux.spacemit.com
> 
> Changes in v2:
> - Patch 1/6:
>   Patch 3/6:
>   Patch 4/6:
>     - nothing
> - Patch 2/6:
>   - remove is_pio judgement in irq_handler()
> - Patch 5/6:
>   - fix wrong comment
>   - In spacemit_i2c_conditionally_reset_bus(), once the condition is met inside the loop, it should
>     return directly instead of using break.
> - Patch 6/6:
>   - add is_pio judgement in irq_handler()
>   - use a fixed timeout value when PIO
>   - use readl_poll_timeout() in  spacemit_i2c_wait_bus_idle() when PIO
> 
> - Link to v1: https://lore.kernel.org/r/20250827-k1-i2c-atomic-v1-0-e59bea02d680@linux.spacemit.com
> ---
>  drivers/i2c/busses/i2c-k1.c | 229 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 185 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..a13284f840ee967466a01375e6603f7568a42f86 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -97,6 +97,8 @@
>  
>  #define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
>  
> +#define SPACEMIT_WAIT_TIMEOUT      1000 /* ms */
> +
>  enum spacemit_i2c_state {
>  	SPACEMIT_STATE_IDLE,
>  	SPACEMIT_STATE_START,
> @@ -125,10 +127,14 @@ struct spacemit_i2c_dev {
>  
>  	enum spacemit_i2c_state state;
>  	bool read;
> +	bool use_pio;
>  	struct completion complete;
>  	u32 status;
>  };
>  
> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c);
> +static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c);
> +

At least for spacemit_i2c_err_check, the forward declaration can be 
avoided by moving code, which is anyway probably better done in a 
separate patch.

Besides this nitpick, this sounds good, so:

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

Regards
Aurelien

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

Re: [PATCH v4] i2c: spacemit: introduce pio for k1

[Troy Mitchell]

On Wed, Nov 05, 2025 at 11:29:52PM +0100, Aurelien Jarno wrote:
> Hi,
> 
> On 2025-10-09 17:59, Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C in atomic context.
> > 
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
[...]
> > +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c);
> > +static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c);
> > +
> 
> At least for spacemit_i2c_err_check, the forward declaration can be 
> avoided by moving code, which is anyway probably better done in a 
> separate patch.
Maybe I'll handle it later.
> 
> Besides this nitpick, this sounds good, so:
> 
> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
Thanks!
> 
> Regards
> Aurelien
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
> 

Re: [PATCH v4] i2c: spacemit: introduce pio for k1

[Alex Elder]

On 10/9/25 4:59 AM, Troy Mitchell wrote:
> This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> enabling the use of I2C in atomic context.

(Sorry I haven't commented on your earlier versions.  They
included other changes to prepare for this; I'm looking at
this patch in isolation and haven't reviewed the others.)

An aside:  I notice the #includes are indented an additional
space in this source file;  perhaps you can get rid of those
(in a separate patch) at some point.

You really need to provide more information about how this
is implemented.  This patch makes non-trivial changes to
the logic.

> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changes in v4:
> - refactor for better readability: simplify condition check and moving if/else (timeout/
>    wait_xfer_complete) logic into a function
> - remove irrelevant changes
> - remove the status clear call in spacemit_i2c_xfer_common()
> - sort functions to avoid forward declarations,
>    move unavoidable ones above function definitions
> - use udelay() in atomic context to avoid sleeping
> - wait for MSD on the last byte in wait_pio_xfer()
> - Link to v3: https://lore.kernel.org/r/20250929-k1-i2c-atomic-v3-1-f7e660c138b6@linux.spacemit.com
> 
> Changes in v3:
> - drop 1-5 patches (have been merged)
> - modify commit message
> - use readl_poll_timeout_atomic() in wait_pio_xfer()
> - use msecs_to_jiffies() when get PIO mode timeout value
> - factor out transfer state handling into spacemit_i2c_handle_state().
> - do not disable/enable the controller IRQ around PIO transfers.
> - consolidate spacemit_i2c_init() interrupt setup
> - rename is_pio -> use_pio
> - rename spacemit_i2c_xfer() -> spacemit_i2c_xfer_common()
> - rename spacemit_i2c_int_xfer() -> spacemit_i2c_xfer()
> - rename spacemit_i2c_pio_xfer() -> spacemit_i2c_pio_xfer_atomic()
> - call spacemit_i2c_err_check() in wait_pio_xfer() when write last byte
> - Link to v2: https://lore.kernel.org/r/20250925-k1-i2c-atomic-v2-0-46dc13311cda@linux.spacemit.com
> 
> Changes in v2:
> - Patch 1/6:
>    Patch 3/6:
>    Patch 4/6:
>      - nothing
> - Patch 2/6:
>    - remove is_pio judgement in irq_handler()
> - Patch 5/6:
>    - fix wrong comment
>    - In spacemit_i2c_conditionally_reset_bus(), once the condition is met inside the loop, it should
>      return directly instead of using break.
> - Patch 6/6:
>    - add is_pio judgement in irq_handler()
>    - use a fixed timeout value when PIO
>    - use readl_poll_timeout() in  spacemit_i2c_wait_bus_idle() when PIO
> 
> - Link to v1: https://lore.kernel.org/r/20250827-k1-i2c-atomic-v1-0-e59bea02d680@linux.spacemit.com
> ---
>   drivers/i2c/busses/i2c-k1.c | 229 +++++++++++++++++++++++++++++++++++---------
>   1 file changed, 185 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..a13284f840ee967466a01375e6603f7568a42f86 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -97,6 +97,8 @@
>   
>   #define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
>   
> +#define SPACEMIT_WAIT_TIMEOUT      1000 /* ms */
> +
>   enum spacemit_i2c_state {
>   	SPACEMIT_STATE_IDLE,
>   	SPACEMIT_STATE_START,
> @@ -125,10 +127,14 @@ struct spacemit_i2c_dev {
>   
>   	enum spacemit_i2c_state state;
>   	bool read;
> +	bool use_pio;

This is fine, but you could maybe call it atomic (to match
the callback funtion name).

Actually, almost all uses of this field are negated, i.e.:
	if (!i2c->use_pio)

Maybe you could use a name that doesn't require negation,
like "can_block" for example.

>   	struct completion complete;
>   	u32 status;
>   };
>   
> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c);
> +static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c);

You should introduce a patch before this one that simply moves
one or both of these functions earlier in the file, so these
forward-declarations can be avoided.

>   static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
>   {
>   	u32 val;
> @@ -177,7 +183,11 @@ static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
>   		return;
>   
>   	spacemit_i2c_reset(i2c);
> -	usleep_range(10, 20);
> +

What determines whether use_pio is set?  The answer is that
it's set when i2c_algorithm->xfer_atomic is used; it is false
if i2c_algorithm->xfer is used.  (This would be something
you'd explain in your patch header.)

> +	if (!i2c->use_pio)
> +		usleep_range(10, 20);
> +	else
> +		udelay(20);

You could encapsulate the above four lines in a new function
spacemit_i2c_delay(unsigned long usec), since they are repeated
in two places.  It can also be used in spacemit_i2c_wait_pio_xfer()
even though the check for PIO isn't needed there.

>   
>   	for (clk_cnt = 0; clk_cnt < SPACEMIT_BUS_RESET_CLK_CNT_MAX; clk_cnt++) {
>   		status = readl(i2c->base + SPACEMIT_IBMR);
> @@ -206,9 +216,14 @@ static int spacemit_i2c_wait_bus_idle(struct spacemit_i2c_dev *i2c)
>   	if (!(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)))
>   		return 0;
>   
> -	ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
> -				 val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
> -				 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
> +	if (!i2c->use_pio)
> +		ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
> +					 val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
> +					 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
> +	else
> +		ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
> +						val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
> +						1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);

The above could also be encapsulated, to simplify this block of
code and the one below that uses this structure.  And again, it
could be used in spacemit_i2c_wait_pio_xfer() as well, despite
the check for PIO not being needed there.

>   	if (ret)
>   		spacemit_i2c_reset(i2c);
>   
> @@ -220,31 +235,39 @@ static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
>   	/* in case bus is not released after transfer completes */
>   	if (readl(i2c->base + SPACEMIT_ISR) & SPACEMIT_SR_EBB) {
>   		spacemit_i2c_conditionally_reset_bus(i2c);
> -		usleep_range(90, 150);
> +		if (!i2c->use_pio)
> +			usleep_range(90, 150);
> +		else
> +			udelay(150);
>   	}
>   }
>   
>   static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>   {
> -	u32 val;
> -
> -	/*
> -	 * Unmask interrupt bits for all xfer mode:
> -	 * bus error, arbitration loss detected.
> -	 * 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;
> -
> -	/*
> -	 * Unmask interrupt bits for interrupt xfer mode:
> -	 * When IDBR receives a byte, an interrupt is triggered.
> -	 *
> -	 * For the tx empty interrupt, it will be enabled in the
> -	 * i2c_start function.
> -	 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
> -	 */
> -	val |= SPACEMIT_CR_DRFIE;
> +	u32 val = 0;
> +
> +	if (!i2c->use_pio) {

Why does this block of initialization only need to be done
when not using PIO?  What happens in the PIO case?  I think
the answer is that you don't want any interrupts when using
PIO.  But... what if these conditions occur in PIO mode?

> +		/*
> +		 * Unmask interrupt bits for all xfer mode:
> +		 * bus error, arbitration loss detected.
> +		 * 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;
> +
> +		/*
> +		 * Unmask interrupt bits for interrupt xfer mode:
> +		 * When IDBR receives a byte, an interrupt is triggered.
> +		 *
> +		 * For the tx empty interrupt, it will be enabled in the
> +		 * i2c_start function.
> +		 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
> +		 */
> +		val |= SPACEMIT_CR_DRFIE;

It's worth explaining (somewhere, possibly in the patch
header) that this is different from what was done before.
As a reviewer I didn't realize this at first; I thought
you were simply making a block of code conditional on
the value of use_pio.

> +		/* unmask master stop interrupt bit */
> +		val |= SPACEMIT_CR_MSDIE;
> +	}

I would find it easier to follow if you set up the
common bit assignments to the SPACEMIT_ICR register
first, and then do the non-PIO specific ones afterward.

>   	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
>   		val |= SPACEMIT_CR_MODE_FAST;
> @@ -256,7 +279,7 @@ 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;
>   
>   	writel(val, i2c->base + SPACEMIT_ICR);
>   
> @@ -293,10 +316,85 @@ 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->use_pio)
> +		val |= SPACEMIT_CR_DTEIE;
> +
>   	writel(val, i2c->base + SPACEMIT_ICR);
>   }
>   

You should add a comment here indicating this returns 0
if there's a timeout/error, or non-zero (1) for success.

> +static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 mask, msec = jiffies_to_msecs(i2c->adapt.timeout);
> +	ktime_t timeout = ktime_add_ms(ktime_get(), msec);
> +	int ret;
> +
> +	mask = SPACEMIT_SR_IRF | SPACEMIT_SR_ITE;

I think this can be a do..while block instead.  Both conditions
will certainly be true initially.

> +	while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
> +		udelay(10);

Why do you delay *first*?  I think you should read the status
and only if you're not done, insert the delay.

> +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> +
> +		spacemit_i2c_clear_int_status(i2c, i2c->status);
> +
> +		if (!(i2c->status & mask))
> +			continue;
> +
> +		spacemit_i2c_handle_state(i2c);
> +
> +		/*
> +		 * This is the last byte to write of the current message.

You mean *if* this is the last byte to write in the message, ...

> +		 * If we do not wait here, control will proceed directly to start(),
> +		 * which would overwrite the current data.
> +		 */
> +		if (!i2c->read && !i2c->unprocessed) {

Instead, do this:

		if (i2c->read || i2c->unprocessed)
			continue;

> +			ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
> +							i2c->status, i2c->status & SPACEMIT_SR_ITE,
> +							30, 1000);
> +			if (ret)
> +				return 0;

I'd rather see the above be done outside the loop. It would
simplify things.  The call to spacemit_i2c_handle_state()
would be the end of the loop.

Then outside the loop:

	if (unprocessed)
		return 0;	/* timeout */

	if (!i2c->read)
		return 1;

	/*
	 * In read mode, err_check() ...
	 * ...
	 */

I might have the logic wrong here, but you might find
it's simpler if you try to handle it this way.

> +			/*
> +			 * In read mode, err_check() in handle_state properly handles what happens
You can drop the sentence about what happens in read mode.
It's a distraction.  Just explain what you have to do in write
mode.

> +			 * after the MSD bit is set. For writes it is different: in interrupt mode,
> +			 * an ITE (write-empty) interrupt is triggered after the last byte, and the
> +			 * MSD-related handling takes place there. In PIO mode, however, we need to
> +			 * explicitly call err_check() to emulate this step, otherwise the next
> +			 * transfer will fail.
> +			 */
> +			if (i2c->msg_idx == i2c->msg_num - 1) {
> +				mask = SPACEMIT_SR_MSD | SPACEMIT_SR_ERR;
> +				/*
> +				 * In some cases, MSD may not arrive immediately;
> +				 * wait here to handle that.
> +				 */
> +				ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
> +								i2c->status, i2c->status & mask,
> +								30, 1000);
> +				if (ret)
> +					return 0;
> +
> +				spacemit_i2c_err_check(i2c);
> +			}
> +		}
> +	}
> +
> +	if (i2c->unprocessed)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int spacemit_i2c_wait_xfer_complete(struct spacemit_i2c_dev *i2c)
> +{
> +	if (i2c->use_pio)
> +		return spacemit_i2c_wait_pio_xfer(i2c);
> +	else
No need for else here.

> +		return wait_for_completion_timeout(&i2c->complete,
> +						   i2c->adapt.timeout);
> +}
> +
>   static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>   {
>   	unsigned long time_left;
> @@ -312,8 +410,8 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>   
>   		spacemit_i2c_start(i2c);
>   
> -		time_left = wait_for_completion_timeout(&i2c->complete,
> -							i2c->adapt.timeout);
> +		time_left = spacemit_i2c_wait_xfer_complete(i2c);
> +
>   		if (!time_left) {
>   			dev_err(i2c->dev, "msg completion timeout\n");
>   			spacemit_i2c_conditionally_reset_bus(i2c);
> @@ -341,6 +439,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;
> @@ -353,14 +454,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->use_pio)
> +		complete(&i2c->complete);

The above two lines could be wrapped in spacemit_i2c_complete(i2c),
to avoid repeating them.

>   }
>   
>   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 */
> @@ -373,7 +480,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->use_pio)
> +		complete(&i2c->complete);
>   }
>   
>   static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
> @@ -408,21 +517,14 @@ 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->use_pio)
> +		complete(&i2c->complete);
>   }
>   
> -static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> +static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c)
>   {
> -	struct spacemit_i2c_dev *i2c = devid;
> -	u32 status, val;
> -
> -	status = readl(i2c->base + SPACEMIT_ISR);
> -	if (!status)
> -		return IRQ_HANDLED;
> -
> -	i2c->status = status;
> -
> -	spacemit_i2c_clear_int_status(i2c, status);
> +	u32 val;
>   
>   	if (i2c->status & SPACEMIT_SR_ERR)
>   		goto err_out;
> @@ -445,7 +547,10 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>   	}
>   
>   	if (i2c->state != SPACEMIT_STATE_IDLE) {
> -		val |= SPACEMIT_CR_TB | SPACEMIT_CR_ALDIE;
> +		val |= SPACEMIT_CR_TB;
> +		if (i2c->use_pio)
> +			val |= SPACEMIT_CR_ALDIE;
> +
>   
>   		if (spacemit_i2c_is_last_msg(i2c)) {
>   			/* trigger next byte with stop */
> @@ -459,6 +564,23 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>   
>   err_out:
>   	spacemit_i2c_err_check(i2c);
> +}
> +
> +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)

Introducing spacemit_i2c_handle_state() as a simple separate
initial patch would simplify review.  (Not a big deal, but
something to think about for future patch series.)

> +{
> +	struct spacemit_i2c_dev *i2c = devid;
> +	u32 status;
> +
> +	status = readl(i2c->base + SPACEMIT_ISR);
> +	if (!status)
> +		return IRQ_HANDLED;
> +
> +	i2c->status = status;
> +
> +	spacemit_i2c_clear_int_status(i2c, status);
> +
> +	spacemit_i2c_handle_state(i2c);
> +
>   	return IRQ_HANDLED;
>   }
>   
> @@ -467,6 +589,11 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
>   	unsigned long timeout;
>   	int idx = 0, cnt = 0;

Why is the timeout a fixed constant for PIO, but a fairly
precise calculation based on message length otherwise?

					-Alex

> +	if (i2c->use_pio) {
> +		i2c->adapt.timeout = msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT);
> +		return;
> +	}
> +
>   	for (; idx < i2c->msg_num; idx++)
>   		cnt += (i2c->msgs + idx)->len + 1;
>   
> @@ -479,11 +606,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_common(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool use_pio)
>   {
>   	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
>   	int ret;
>   
> +	i2c->use_pio = use_pio;
> +
>   	i2c->msgs = msgs;
>   	i2c->msg_num = num;
>   
> @@ -511,6 +641,16 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
>   	return ret < 0 ? ret : num;
>   }
>   
> +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> +{
> +	return spacemit_i2c_xfer_common(adapt, msgs, num, false);
> +}
> +
> +static int spacemit_i2c_pio_xfer_atomic(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> +{
> +	return spacemit_i2c_xfer_common(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);
> @@ -518,6 +658,7 @@ static u32 spacemit_i2c_func(struct i2c_adapter *adap)
>   
>   static const struct i2c_algorithm spacemit_i2c_algo = {
>   	.xfer = spacemit_i2c_xfer,
> +	.xfer_atomic = spacemit_i2c_pio_xfer_atomic,
>   	.functionality = spacemit_i2c_func,
>   };
>   
> 
> ---
> base-commit: bc574b64121525b24d52e9bab747184181c808dc
> change-id: 20250814-k1-i2c-atomic-f1a90cd34364
> 
> Best regards,

Re: [PATCH v4] i2c: spacemit: introduce pio for k1

[Aurelien Jarno]

Hi Troy,

On 2025-11-07 09:50, Alex Elder wrote:
> On 10/9/25 4:59 AM, Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C in atomic context.
> 
> (Sorry I haven't commented on your earlier versions.  They
> included other changes to prepare for this; I'm looking at
> this patch in isolation and haven't reviewed the others.)
> 
> An aside:  I notice the #includes are indented an additional
> space in this source file;  perhaps you can get rid of those
> (in a separate patch) at some point.
> 
> You really need to provide more information about how this
> is implemented.  This patch makes non-trivial changes to
> the logic.

What is the status on implementing the changes asked by Alex? Do you 
need some help with that?

Thanks
Aurelien

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

Re: [PATCH v4] i2c: spacemit: introduce pio for k1

[Troy Mitchell]

On Thu, Dec 11, 2025 at 10:28:24PM +0100, Aurelien Jarno wrote:
> Hi Troy,
> 
> On 2025-11-07 09:50, Alex Elder wrote:
> > On 10/9/25 4:59 AM, Troy Mitchell wrote:
> > > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > > enabling the use of I2C in atomic context.
> > 
> > (Sorry I haven't commented on your earlier versions.  They
> > included other changes to prepare for this; I'm looking at
> > this patch in isolation and haven't reviewed the others.)
> > 
> > An aside:  I notice the #includes are indented an additional
> > space in this source file;  perhaps you can get rid of those
> > (in a separate patch) at some point.
> > 
> > You really need to provide more information about how this
> > is implemented.  This patch makes non-trivial changes to
> > the logic.
> 
> What is the status on implementing the changes asked by Alex? Do you 
> need some help with that?
I've been a bit busy recently, but I expect the next version can be
posted later this week or next week.

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

Re: [PATCH v4] i2c: spacemit: introduce pio for k1

[Troy Mitchell]

On Fri, Nov 07, 2025 at 09:50:26AM -0600, Alex Elder wrote:
> On 10/9/25 4:59 AM, Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C in atomic context.
> 
> (Sorry I haven't commented on your earlier versions.  They
> included other changes to prepare for this; I'm looking at
> this patch in isolation and haven't reviewed the others.)
> 
> An aside:  I notice the #includes are indented an additional
> space in this source file;  perhaps you can get rid of those
> (in a separate patch) at some point.
Uh? Where?
I didn't do that in this patch.
> 
> You really need to provide more information about how this
> is implemented.  This patch makes non-trivial changes to
> the logic.
I'll provide more infomation in the next version.

[...]

> >   static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> >   {
> > -	u32 val;
> > -
> > -	/*
> > -	 * Unmask interrupt bits for all xfer mode:
> > -	 * bus error, arbitration loss detected.
> > -	 * 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;
> > -
> > -	/*
> > -	 * Unmask interrupt bits for interrupt xfer mode:
> > -	 * When IDBR receives a byte, an interrupt is triggered.
> > -	 *
> > -	 * For the tx empty interrupt, it will be enabled in the
> > -	 * i2c_start function.
> > -	 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
> > -	 */
> > -	val |= SPACEMIT_CR_DRFIE;
> > +	u32 val = 0;
> > +
> > +	if (!i2c->use_pio) {
> 
> Why does this block of initialization only need to be done
> when not using PIO?  What happens in the PIO case?  I think
> the answer is that you don't want any interrupts when using
> PIO.  But... what if these conditions occur in PIO mode?
We always read ISR register in the pio_xfer().
Disabling the interrupt for a error bit only prevents the interrupt from
firing; it does not prevent the error bit itself from being set in the
ISR.
And we reused handle_state() so the error check is still there.

> 
> > +		/*
> > +		 * Unmask interrupt bits for all xfer mode:
> > +		 * bus error, arbitration loss detected.
> > +		 * 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;
> > +
> > +		/*
> > +		 * Unmask interrupt bits for interrupt xfer mode:
> > +		 * When IDBR receives a byte, an interrupt is triggered.
> > +		 *
> > +		 * For the tx empty interrupt, it will be enabled in the
> > +		 * i2c_start function.
> > +		 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
> > +		 */
> > +		val |= SPACEMIT_CR_DRFIE;
> 
> It's worth explaining (somewhere, possibly in the patch
> header) that this is different from what was done before.
> As a reviewer I didn't realize this at first; I thought
> you were simply making a block of code conditional on
> the value of use_pio.
See above. And I'll put them into commit message.

> 
> > +		/* unmask master stop interrupt bit */
> > +		val |= SPACEMIT_CR_MSDIE;
> > +	}
> 
> I would find it easier to follow if you set up the
> common bit assignments to the SPACEMIT_ICR register
> first, and then do the non-PIO specific ones afterward.
I don't think so. I think it should be configured on demand; otherwise
we will need to do the same thing again when FIFO support or high-speed
transfer modes are added in the future.
Moreover, in the PIO path we would still have to disable the FIFO and
high-speed bits, which makes the approach counterproductive.

[...]

> > +static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
> > +{
> > +	u32 mask, msec = jiffies_to_msecs(i2c->adapt.timeout);
> > +	ktime_t timeout = ktime_add_ms(ktime_get(), msec);
> > +	int ret;
> > +
> > +	mask = SPACEMIT_SR_IRF | SPACEMIT_SR_ITE;
> 
> I think this can be a do..while block instead.  Both conditions
> will certainly be true initially.
> 
> > +	while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
> > +		udelay(10);
> 
> Why do you delay *first*?  I think you should read the status
> and only if you're not done, insert the delay.
> 
> > +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> > +
> > +		spacemit_i2c_clear_int_status(i2c, i2c->status);
> > +
> > +		if (!(i2c->status & mask))
> > +			continue;
> > +
> > +		spacemit_i2c_handle_state(i2c);
> > +
> > +		/*
> > +		 * This is the last byte to write of the current message.
> 
> You mean *if* this is the last byte to write in the message, ...
Yes I lost *if*.

[...]
> 
> > @@ -445,7 +547,10 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> >   	}
> >   	if (i2c->state != SPACEMIT_STATE_IDLE) {
> > -		val |= SPACEMIT_CR_TB | SPACEMIT_CR_ALDIE;
> > +		val |= SPACEMIT_CR_TB;
> > +		if (i2c->use_pio)
> > +			val |= SPACEMIT_CR_ALDIE;
> > +
> >   		if (spacemit_i2c_is_last_msg(i2c)) {
> >   			/* trigger next byte with stop */
> > @@ -459,6 +564,23 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> >   err_out:
> >   	spacemit_i2c_err_check(i2c);
> > +}
> > +
> > +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> 
> Introducing spacemit_i2c_handle_state() as a simple separate
> initial patch would simplify review.  (Not a big deal, but
> something to think about for future patch series.)
I will. Thanks.
> 
> > +{
> > +	struct spacemit_i2c_dev *i2c = devid;
> > +	u32 status;
> > +
> > +	status = readl(i2c->base + SPACEMIT_ISR);
> > +	if (!status)
> > +		return IRQ_HANDLED;
> > +
> > +	i2c->status = status;
> > +
> > +	spacemit_i2c_clear_int_status(i2c, status);
> > +
> > +	spacemit_i2c_handle_state(i2c);
> > +
> >   	return IRQ_HANDLED;
> >   }
> > @@ -467,6 +589,11 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
> >   	unsigned long timeout;
> >   	int idx = 0, cnt = 0;
> 
> Why is the timeout a fixed constant for PIO, but a fairly
> precise calculation based on message length otherwise?
We have talked about here [1]

                - Troy

Link: https://lore.kernel.org/all/2ADE8BFFC8EF40BC+aNH0fplBawrkLp3Z@LT-Guozexi/ [1]
> 
> 					-Alex
> 
> > +	if (i2c->use_pio) {
> > +		i2c->adapt.timeout = msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT);
> > +		return;
> > +	}
> > +
> >   	for (; idx < i2c->msg_num; idx++)
> >   		cnt += (i2c->msgs + idx)->len + 1;
> > @@ -479,11 +606,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_common(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool use_pio)
> >   {
> >   	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
> >   	int ret;
> > +	i2c->use_pio = use_pio;
> > +
> >   	i2c->msgs = msgs;
> >   	i2c->msg_num = num;
> > @@ -511,6 +641,16 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
> >   	return ret < 0 ? ret : num;
> >   }
> > +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > +{
> > +	return spacemit_i2c_xfer_common(adapt, msgs, num, false);
> > +}
> > +
> > +static int spacemit_i2c_pio_xfer_atomic(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > +{
> > +	return spacemit_i2c_xfer_common(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);
> > @@ -518,6 +658,7 @@ static u32 spacemit_i2c_func(struct i2c_adapter *adap)
> >   static const struct i2c_algorithm spacemit_i2c_algo = {
> >   	.xfer = spacemit_i2c_xfer,
> > +	.xfer_atomic = spacemit_i2c_pio_xfer_atomic,
> >   	.functionality = spacemit_i2c_func,
> >   };
> > 
> > ---
> > base-commit: bc574b64121525b24d52e9bab747184181c808dc
> > change-id: 20250814-k1-i2c-atomic-f1a90cd34364
> > 
> > Best regards,
> 
> 

Re: [PATCH v4] i2c: spacemit: introduce pio for k1

[Troy Mitchell]

On Fri, Nov 07, 2025 at 09:50:26AM -0600, Alex Elder wrote:
> On 10/9/25 4:59 AM, Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C in atomic context.
[...]
> > @@ -125,10 +127,14 @@ struct spacemit_i2c_dev {
> >   	enum spacemit_i2c_state state;
> >   	bool read;
> > +	bool use_pio;
> 
> This is fine, but you could maybe call it atomic (to match
> the callback funtion name).
> 
> Actually, almost all uses of this field are negated, i.e.:
> 	if (!i2c->use_pio)
> 
> Maybe you could use a name that doesn't require negation,
> like "can_block" for example.

Most of the negation is unnecessary; it's my code's behavior. I'll
modify the code behavior, not this variable. I believe this variable
directly reflects the hardware behavior. Therefore, I won't modify it
here.


                                  - Troy