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

[PATCH v2 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>
---
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

---
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 | 221 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 178 insertions(+), 43 deletions(-)
---
base-commit: f9a7e1f3eff519e88c91d87701d05cfd968d89de
change-id: 20250814-k1-i2c-atomic-f1a90cd34364

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

[PATCH v2 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")
Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
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.51.0

[PATCH v2 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 | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index ee08811f4087c8e709d25dd314854ed643cc5a47..84f132d0504dc992f2f961253b011b86afcc2bbc 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,16 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
 	}
 
 	if (i2c->state != SPACEMIT_STATE_IDLE) {
+		val |= SPACEMIT_CR_TB | 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.51.0

[PATCH v2 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.
---
There are the restart waveforms before [1] and after [2] disabling the fix.

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

Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
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 84f132d0504dc992f2f961253b011b86afcc2bbc..9bf9f01aa68bde6460e50c6983edc3f705b12eea 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.51.0

[PATCH v2 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")
Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
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 9bf9f01aa68bde6460e50c6983edc3f705b12eea..848dfaf634f63021bc565f2c0a1c93f9f33665dd 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.51.0

[PATCH v2 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 | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index 848dfaf634f63021bc565f2c0a1c93f9f33665dd..6b918770e612e098b8ad17418f420d87c94df166 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,8 @@
 					SPACEMIT_SR_ALD)
 
 #define SPACEMIT_RCR_SDA_GLITCH_NOFIX		BIT(7)		/* bypass the SDA glitch fix */
+/* the cycles of SCL during bus reset */
+#define SPACEMIT_RCR_FIELD_RST_CYC		GENMASK(3, 0)
 
 /* SPACEMIT_IBMR register fields */
 #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
@@ -91,6 +95,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 +169,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 +179,18 @@ 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)
+			return;
+
+		/* 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);
+	}
+
 	/* check sda again here */
 	status = readl(i2c->base + SPACEMIT_IBMR);
 	if (!(status & SPACEMIT_BMR_SDA))

-- 
2.51.0

[PATCH v2 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 | 164 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 140 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -97,6 +97,9 @@
 
 #define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
 
+/* Constants */
+#define SPACEMIT_WAIT_TIMEOUT      1000 /* ms */
+
 enum spacemit_i2c_state {
 	SPACEMIT_STATE_IDLE,
 	SPACEMIT_STATE_START,
@@ -125,6 +128,7 @@ struct spacemit_i2c_dev {
 
 	enum spacemit_i2c_state state;
 	bool read;
+	bool is_pio;
 	struct completion complete;
 	u32 status;
 };
@@ -206,9 +210,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->is_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);
 
@@ -226,7 +235,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:
@@ -234,7 +243,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:
@@ -244,7 +254,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;
@@ -256,7 +267,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);
 
@@ -293,10 +307,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;
@@ -310,10 +368,28 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
 
 		reinit_completion(&i2c->complete);
 
-		spacemit_i2c_start(i2c);
+		if (i2c->is_pio) {
+			/* We disable the interrupt to avoid unintended spurious triggers. */
+			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);
@@ -341,6 +417,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 +432,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 */
@@ -373,7 +458,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)
@@ -408,7 +495,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)
@@ -416,13 +505,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;
@@ -445,7 +541,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->is_pio)
+			val |= SPACEMIT_CR_ALDIE;
+
 
 		if (spacemit_i2c_is_last_msg(i2c)) {
 			/* trigger next byte with stop */
@@ -479,15 +578,21 @@ 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;
 
-	spacemit_i2c_calc_timeout(i2c);
+	if (!i2c->is_pio)
+		spacemit_i2c_calc_timeout(i2c);
+	else
+		i2c->adapt.timeout = SPACEMIT_WAIT_TIMEOUT;
 
 	spacemit_i2c_init(i2c);
 
@@ -506,18 +611,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.51.0

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

[Aurelien Jarno]

On 2025-09-25 10:02, 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 | 26 +++++++-------------------
>  1 file changed, 7 insertions(+), 19 deletions(-)

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

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

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

[Aurelien Jarno]

On 2025-09-25 10:02, 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 | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)

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

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

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

[Wolfram Sang]

On Thu, Sep 25, 2025 at 10:02:24AM +0800, 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>

Applied patches 1-5 to for-mergewindow, thanks!

Waiting for a possible review for patch 6.

Troy, maybe you want to add yourself as a maintainer for this driver?

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

[Troy Mitchell]

Hi Wolfram,

On Thu, Sep 25, 2025 at 11:50:33PM +0200, Wolfram Sang wrote:
> On Thu, Sep 25, 2025 at 10:02:24AM +0800, 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>
> 
> Applied patches 1-5 to for-mergewindow, thanks!
Thanks.
> 
> Waiting for a possible review for patch 6.
> 
> Troy, maybe you want to add yourself as a maintainer for this driver?
Yeah, I'd be happy to.
I'll add myself to the MAINTAINERS file ASAP.

                - Troy
> 
> 

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

[Yixun Lan]

Hi Troy,

On 10:02 Thu 25 Sep     , 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 | 164 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 140 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -97,6 +97,9 @@
>  
>  #define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
>  
> +/* Constants */
> +#define SPACEMIT_WAIT_TIMEOUT      1000 /* ms */
> +
>  enum spacemit_i2c_state {
>  	SPACEMIT_STATE_IDLE,
>  	SPACEMIT_STATE_START,
> @@ -125,6 +128,7 @@ struct spacemit_i2c_dev {
>  
>  	enum spacemit_i2c_state state;
>  	bool read;
> +	bool is_pio;
using_pio_mode or simply use_pio, but have to say..

I feel it's improper to have this flag here, since it's not a controller
level feature, I understand it was introduced to support aotmic operation

Personally, I'd suggest to pass the flag in xfer(), then propagate down to
whatever needed, so it limit to single transmission which more flexible

>  	struct completion complete;
>  	u32 status;
>  };
> @@ -206,9 +210,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->is_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);
question, since you have already used state-machine to track the I2C process,
can you not poke hardware ISR register in a scatter way? I'd rather see it handled
more closely in a interrupt context or related?

btw, does some bits of the ISR register have read-then-clear feature?
which may require special attention to handle..

>  	if (ret)
>  		spacemit_i2c_reset(i2c);
>  
> @@ -226,7 +235,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:
> @@ -234,7 +243,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:
> @@ -244,7 +254,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;
> @@ -256,7 +267,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;
can you converge all assignment under one if?
>  
>  	writel(val, i2c->base + SPACEMIT_ICR);
>  
> @@ -293,10 +307,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);

can you refactor and construct a new function? that can be reused between
irq() and pio() cases, it makes people confused..

> +
> +		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;
> +}
> +

[snip]..

>  static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> @@ -416,13 +505,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;
> @@ -445,7 +541,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->is_pio)
> +			val |= SPACEMIT_CR_ALDIE;
> +
>  
>  		if (spacemit_i2c_is_last_msg(i2c)) {
>  			/* trigger next byte with stop */
> @@ -479,15 +578,21 @@ 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)
s/spacemit_i2c_xfer/spacemit_i2c_xfer_common/

s/is_pio/atomic/, I'd suggest to distinguish 'pio' vs 'atomic'

>  {
>  	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
>  	int ret;
>  
> +	i2c->is_pio = is_pio;
so, check my previous comment, you use this member to cache the flag..

> +
>  	i2c->msgs = msgs;
>  	i2c->msg_num = num;
>  
> -	spacemit_i2c_calc_timeout(i2c);
> +	if (!i2c->is_pio)
> +		spacemit_i2c_calc_timeout(i2c);
> +	else
> +		i2c->adapt.timeout = SPACEMIT_WAIT_TIMEOUT;
>  
>  	spacemit_i2c_init(i2c);
>  
> @@ -506,18 +611,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,
I'd suggest to align with core function's prototype,
s/spacemit_i2c_int_xfer/spacemit_i2c_xfer/
s/spacemit_i2c_pio_xfer/spacemit_i2c_pio_xfer_atomic /

>  	.functionality = spacemit_i2c_func,
>  };
>  
> 
> -- 
> 2.51.0
> 

-- 
Yixun Lan (dlan)

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

[Troy Mitchell]

On Fri, Sep 26, 2025 at 07:10:55PM +0800, Yixun Lan wrote:
> Hi Troy,
Hi Yixun,
Thanks for your review.

I have a question regarding the patch series.
Since patches 1–5 have already been merged,
should I keep the current version number and just send this single patch ?

About reply, see below.
> 
> On 10:02 Thu 25 Sep     , 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 | 164 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 140 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -97,6 +97,9 @@
> >  
> >  #define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
> >  
> > +/* Constants */
> > +#define SPACEMIT_WAIT_TIMEOUT      1000 /* ms */
> > +
> >  enum spacemit_i2c_state {
> >  	SPACEMIT_STATE_IDLE,
> >  	SPACEMIT_STATE_START,
> > @@ -125,6 +128,7 @@ struct spacemit_i2c_dev {
> >  
> >  	enum spacemit_i2c_state state;
> >  	bool read;
> > +	bool is_pio;
> using_pio_mode or simply use_pio, but have to say..
> 
> I feel it's improper to have this flag here, since it's not a controller
> level feature, I understand it was introduced to support aotmic operation
> 
> Personally, I'd suggest to pass the flag in xfer(), then propagate down to
> whatever needed, so it limit to single transmission which more flexible
I agree. I'll move the flag to xfer().

> 
> >  	struct completion complete;
> >  	u32 status;
> >  };
> > @@ -206,9 +210,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->is_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);
> question, since you have already used state-machine to track the I2C process,
> can you not poke hardware ISR register in a scatter way?

>
> I'd rather see it handled
> more closely in a interrupt context or related?
> 
> btw, does some bits of the ISR register have read-then-clear feature?
> which may require special attention to handle..
I’m not reading the ISR state all over the place. It is only accessed in
spacemit_i2c_wait_bus_idle() and spacemit_i2c_check_bus_release().
The first one is used before a transfer starts, and the second one is used
after a transfer error. In these cases I don’t see a way to rely on the
irq_handler to get the information in time.

In addition, with PIO support, the ISR needs to be read inside
wait_pio_xfer(). This is required, otherwise we can’t know when to
proceed with RX/TX.

As you mentioned, some ISR bits are indeed read-to-clear. I’ve already
added handling for that in the irq_handler to avoid losing events.
> 
> >  	if (ret)
> >  		spacemit_i2c_reset(i2c);
> >  
> > @@ -226,7 +235,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:
> > @@ -234,7 +243,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:
> > @@ -244,7 +254,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;
> > @@ -256,7 +267,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;
> can you converge all assignment under one if?
Yes, I'll fix it in the next version.

> >  
> >  	writel(val, i2c->base + SPACEMIT_ICR);
> >  
> > @@ -293,10 +307,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);
> 
> can you refactor and construct a new function? that can be reused between
> irq() and pio() cases, it makes people confused..
It makes sense. I will.
> 
> > +
> > +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> > +
> > +		/*
[...]
> >  
> > -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)
> s/spacemit_i2c_xfer/spacemit_i2c_xfer_common/
Great suggesstion!

> 
> s/is_pio/atomic/, I'd suggest to distinguish 'pio' vs 'atomic'
Uh, But I wanna keep the original name 'pio' to avoid confusion.
In the public doc, It is named PIO..

> 
> >  {
> >  	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
> >  	int ret;
> >  
> > +	i2c->is_pio = is_pio;
> so, check my previous comment, you use this member to cache the flag..
> 
> > +
[...]
> >  
> >  static const struct i2c_algorithm spacemit_i2c_algo = {
> > -	.xfer = spacemit_i2c_xfer,
> > +	.xfer = spacemit_i2c_int_xfer,
> > +	.xfer_atomic = spacemit_i2c_pio_xfer,
> I'd suggest to align with core function's prototype,
> s/spacemit_i2c_int_xfer/spacemit_i2c_xfer/
> s/spacemit_i2c_pio_xfer/spacemit_i2c_pio_xfer_atomic /
Thanks.
                - Troy
> 
> >  	.functionality = spacemit_i2c_func,
> >  };
> >  
> > 
> > -- 
> > 2.51.0
> > 
> 
> -- 
> Yixun Lan (dlan)
> 

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

[Wolfram Sang]

> Since patches 1–5 have already been merged,
> should I keep the current version number and just send this single patch ?

Yes, please.

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

[Aurelien Jarno]

Hi Troy,

On 2025-09-25 10:02, 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 | 164 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 140 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c

[snip]

> @@ -293,10 +307,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);

This needs to be readl_poll_timeout_atomic(), like you changed 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;

[snip]

> @@ -479,15 +578,21 @@ 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;
>  
> -	spacemit_i2c_calc_timeout(i2c);
> +	if (!i2c->is_pio)
> +		spacemit_i2c_calc_timeout(i2c);
> +	else
> +		i2c->adapt.timeout = SPACEMIT_WAIT_TIMEOUT;

Thanks for fixing that, however i2c->adapt.timeout needs to be in 
jiffies, so you want to use msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT).

>  	spacemit_i2c_init(i2c);
>  

Regards
Aurelien

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

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

[Yixun Lan]

Hi Troy,

On 16:28 Fri 26 Sep     , Wolfram Sang wrote:
> 
> > Since patches 1–5 have already been merged,
> > should I keep the current version number and just send this single patch ?
> 
> Yes, please.
> 
I agree, please do increase the patch version, have a formal cover letter,
document the changes, and add old patch URL link..

-- 
Yixun Lan (dlan)

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

[Yixun Lan]

Hi Troy,

On 10:02 Thu 25 Sep     , Troy Mitchell wrote:
> This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> enabling the use of I2C with interrupts disabled.
I feel it's more proper to say
s/with interrupts disabled/in atomic context/

I've noticed that K1 I2C controller support three different transmission mode:
 non-fifo, fifo mode, dma mode

while you are trying to implement pio support, I'd suggest to think one
further step in the long run - support more fifo/dma + normal/atomic features,

Personally, I'd like to see fifo mode implemented before adding pio
support, as it will bring quite significant code changes, require heavy
code review effort. And yes, this will put more demanding work on your side
and may slow things a bit..

-- 
Yixun Lan (dlan)

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

[Troy Mitchell]

On Sat, Sep 27, 2025 at 09:24:29AM +0800, Yixun Lan wrote:
> Hi Troy,
> 
> On 16:28 Fri 26 Sep     , Wolfram Sang wrote:
> > 
> > > Since patches 1–5 have already been merged,
> > > should I keep the current version number and just send this single patch ?
> > 
> > Yes, please.
> > 
> I agree, please do increase the patch version, have a formal cover letter,
> document the changes, and add old patch URL link..
Thanks. I will.
                - Troy
> 
> -- 
> Yixun Lan (dlan)
> 

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

[Troy Mitchell]

On Sat, Sep 27, 2025 at 09:45:47AM +0800, Yixun Lan wrote:
> Hi Troy,
> 
> On 10:02 Thu 25 Sep     , Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C with interrupts disabled.
> I feel it's more proper to say
> s/with interrupts disabled/in atomic context/
Good point.

> 
> I've noticed that K1 I2C controller support three different transmission mode:
>  non-fifo, fifo mode, dma mode
> 
> while you are trying to implement pio support, I'd suggest to think one
> further step in the long run - support more fifo/dma + normal/atomic features,
I understand your concern, but I have reviewed the code structure,
and adding FIFO support would not significantly affect the current PIO implementation.

> 
> Personally, I'd like to see fifo mode implemented before adding pio
> support, as it will bring quite significant code changes, require heavy
> code review effort. And yes, this will put more demanding work on your side
> and may slow things a bit..
That said, we don’t have any plans to support FIFO at this point,
and PIO functionality is something we need urgently.
So I will continue to push this patch (tomorrow).
Thanks for your understanding. For details on the code changes, please see above.

In addition, if anyone is interested in adding FIFO support, I’d be happy to help.

                                            - Troy
>
> 
> -- 
> Yixun Lan (dlan)
> 

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

[Troy Mitchell]

On Fri, Sep 26, 2025 at 06:47:14PM +0200, Aurelien Jarno wrote:
> Hi Troy,
> 
> On 2025-09-25 10:02, 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 | 164 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 140 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> 
> [snip]
> 
> > @@ -293,10 +307,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);
> 
> This needs to be readl_poll_timeout_atomic(), like you changed in 
> spacemit_i2c_wait_bus_idle().
Yes, nice catch!
That's my mistake... I forgot this.
> 
> > +			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;
> 
> [snip]
> 
> > @@ -479,15 +578,21 @@ 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;
> >  
> > -	spacemit_i2c_calc_timeout(i2c);
> > +	if (!i2c->is_pio)
> > +		spacemit_i2c_calc_timeout(i2c);
> > +	else
> > +		i2c->adapt.timeout = SPACEMIT_WAIT_TIMEOUT;
> 
> Thanks for fixing that, however i2c->adapt.timeout needs to be in 
> jiffies, so you want to use msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT).
Thanks for pointing this out!

                - Troy
> 
> >  	spacemit_i2c_init(i2c);
> >  
> 
> Regards
> Aurelien
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
> 

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

[Yixun Lan]

Hi Troy,

On 12:04 Sat 27 Sep     , Troy Mitchell wrote:
> On Sat, Sep 27, 2025 at 09:45:47AM +0800, Yixun Lan wrote:
> > Hi Troy,
> > 
> > On 10:02 Thu 25 Sep     , Troy Mitchell wrote:
> > > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > > enabling the use of I2C with interrupts disabled.
> > I feel it's more proper to say
> > s/with interrupts disabled/in atomic context/
> Good point.
> 
> > 
> > I've noticed that K1 I2C controller support three different transmission mode:
> >  non-fifo, fifo mode, dma mode
> > 
> > while you are trying to implement pio support, I'd suggest to think one
> > further step in the long run - support more fifo/dma + normal/atomic features,
> I understand your concern, but I have reviewed the code structure,
> and adding FIFO support would not significantly affect the current PIO implementation.
we will see..

> 
> > 
> > Personally, I'd like to see fifo mode implemented before adding pio
> > support, as it will bring quite significant code changes, require heavy
> > code review effort. And yes, this will put more demanding work on your side
> > and may slow things a bit..
> That said, we don’t have any plans to support FIFO at this point,
> and PIO functionality is something we need urgently.
> So I will continue to push this patch (tomorrow).
please do not flush the mailing list and give people some time, I haven't seen 
enough reviews coming, please wait for ~one week before sending next version,
or make sure you've accumulated enough changes..

> Thanks for your understanding. For details on the code changes, please see above.
> 
> In addition, if anyone is interested in adding FIFO support, I’d be happy to help.
> 
do you want to push the work (support fifo mode) to community?

-- 
Yixun Lan (dlan)

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

[Troy Mitchell]

On Sat, Sep 27, 2025 at 06:16:48PM +0800, Yixun Lan wrote:
> Hi Troy,
> 
> On 12:04 Sat 27 Sep     , Troy Mitchell wrote:
> > On Sat, Sep 27, 2025 at 09:45:47AM +0800, Yixun Lan wrote:
> > > Hi Troy,
> > > 
> > > On 10:02 Thu 25 Sep     , Troy Mitchell wrote:
> > > > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > > > enabling the use of I2C with interrupts disabled.
> > > I feel it's more proper to say
> > > s/with interrupts disabled/in atomic context/
> > Good point.
> > 
> > > 
> > > I've noticed that K1 I2C controller support three different transmission mode:
> > >  non-fifo, fifo mode, dma mode
> > > 
> > > while you are trying to implement pio support, I'd suggest to think one
> > > further step in the long run - support more fifo/dma + normal/atomic features,
> > I understand your concern, but I have reviewed the code structure,
> > and adding FIFO support would not significantly affect the current PIO implementation.
> we will see..
> 
> > 
> > > 
> > > Personally, I'd like to see fifo mode implemented before adding pio
> > > support, as it will bring quite significant code changes, require heavy
> > > code review effort. And yes, this will put more demanding work on your side
> > > and may slow things a bit..
> > That said, we don’t have any plans to support FIFO at this point,
> > and PIO functionality is something we need urgently.
> > So I will continue to push this patch (tomorrow).
> please do not flush the mailing list and give people some time, I haven't seen 
> enough reviews coming, please wait for ~one week before sending next version,
> or make sure you've accumulated enough changes..
Okay.. I will wait for more review

> 
> > Thanks for your understanding. For details on the code changes, please see above.
> > 
> > In addition, if anyone is interested in adding FIFO support, I’d be happy to help.
> > 
> do you want to push the work (support fifo mode) to community?
Personally, I want.
Since K3 is going to reuse the upstream driver,
PIO support has high priority (needed as P1, e.g. for shutdown and reboot).
FIFO support, is not considered a high priority at this point.

That's why I want to push PIO ASAP
Does this make sense?

                - Troy
> 
> -- 
> Yixun Lan (dlan)
> 

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

[Yixun Lan]

On 10:02 Thu 25 Sep     , 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 | 164 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 140 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -97,6 +97,9 @@
>  
..
>  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>  {
>  	unsigned long time_left;
> @@ -310,10 +368,28 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>  
>  		reinit_completion(&i2c->complete);
>  
> -		spacemit_i2c_start(i2c);
> +		if (i2c->is_pio) {
> +			/* We disable the interrupt to avoid unintended spurious triggers. */
the comment is suspicious, and seems wrong..
> +			disable_irq(i2c->irq);
> +
I guess this doesn't disable interrupt in the hardware layer, it will still
fire interrupt once enabled, so instead of calling disable_irq(), why not
dealing with ISR setting of the controller? I mean those "IE bits"(interrupt
enableing) of ICR REGISTER, disabling them should prevent the interrupt
triggered?

> +			/*
> +			 * 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);
this also doesn't seem correct to me, the irq status bit should be cleared once
interrupt occur, not dealing it here before sending msg, this seems a
wrong procedure

> +
> +			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);
> @@ -341,6 +417,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)

-- 
Yixun Lan (dlan)

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

[Troy Mitchell]

On Sat, Sep 27, 2025 at 06:56:16PM +0800, Yixun Lan wrote:
> On 10:02 Thu 25 Sep     , 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 | 164 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 140 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -97,6 +97,9 @@
> >  
> ..
> >  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> >  {
> >  	unsigned long time_left;
> > @@ -310,10 +368,28 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> >  
> >  		reinit_completion(&i2c->complete);
> >  
> > -		spacemit_i2c_start(i2c);
> > +		if (i2c->is_pio) {
> > +			/* We disable the interrupt to avoid unintended spurious triggers. */
> the comment is suspicious, and seems wrong..
> > +			disable_irq(i2c->irq);
> > +
> I guess this doesn't disable interrupt in the hardware layer, it will still
> fire interrupt once enabled, so instead of calling disable_irq(), why not
> dealing with ISR setting of the controller? I mean those "IE bits"(interrupt
> enableing) of ICR REGISTER, disabling them should prevent the interrupt
> triggered?
For example, take MSD (master stop detect).
If we disable this interrupt, even the interrupt status bit will never be triggered.
Then how are we supposed to know when the transfer has completed?
That’s why we disable the global interrupt here, but still keep the pending bit.

> 
> > +			/*
> > +			 * 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);
> this also doesn't seem correct to me, the irq status bit should be cleared once
> interrupt occur,
> not dealing it here before sending msg, this seems a
> wrong procedure
I'll double check it, as I recall that if it’s not cleared here,
the second message will inevitably fail.

                - Troy
> 
> > +
> > +			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);
> > @@ -341,6 +417,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
> 
> -- 
> Yixun Lan (dlan)
> 

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

[Yixun Lan]

Hi Troy,

On 09:17 Sun 28 Sep     , Troy Mitchell wrote:
> On Sat, Sep 27, 2025 at 06:56:16PM +0800, Yixun Lan wrote:
> > On 10:02 Thu 25 Sep     , 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 | 164 +++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 140 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > > index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> > > --- a/drivers/i2c/busses/i2c-k1.c
> > > +++ b/drivers/i2c/busses/i2c-k1.c
> > > @@ -97,6 +97,9 @@
> > >  
> > ..
> > >  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> > >  {
> > >  	unsigned long time_left;
> > > @@ -310,10 +368,28 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> > >  
> > >  		reinit_completion(&i2c->complete);
> > >  
> > > -		spacemit_i2c_start(i2c);
> > > +		if (i2c->is_pio) {
> > > +			/* We disable the interrupt to avoid unintended spurious triggers. */
> > the comment is suspicious, and seems wrong..
> > > +			disable_irq(i2c->irq);
> > > +
> > I guess this doesn't disable interrupt in the hardware layer, it will still
> > fire interrupt once enabled, so instead of calling disable_irq(), why not
> > dealing with ISR setting of the controller? I mean those "IE bits"(interrupt
> > enableing) of ICR REGISTER, disabling them should prevent the interrupt
> > triggered?
> For example, take MSD (master stop detect).
> If we disable this interrupt, even the interrupt status bit will never be triggered.
No, this is not something I understand..
> Then how are we supposed to know when the transfer has completed?
> That’s why we disable the global interrupt here, but still keep the pending bit.
checking 18.1.4.1 ICR REGISTER, there is Master Stop Deteced Enable - MSDE (Bit[26]) and
Master Stop Deteced Interrupt Enable - MSDIE (BIT[25])
you can disable MSDIE but still enable MSDE, and check status of MSD (BIT[26]) of ISR REGISTER?
it should still work with interrupt disabled, otherwise I'd consider the hardware is broken.

-- 
Yixun Lan (dlan)

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

[Troy Mitchell]

On Fri, Sep 26, 2025 at 07:10:55PM +0800, Yixun Lan wrote:
> Hi Troy,
> 
> On 10:02 Thu 25 Sep     , 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 | 164 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 140 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -97,6 +97,9 @@
> >  
> >  #define SPACEMIT_BUS_RESET_CLK_CNT_MAX		9
> >  
> > +/* Constants */
> > +#define SPACEMIT_WAIT_TIMEOUT      1000 /* ms */
> > +
> >  enum spacemit_i2c_state {
> >  	SPACEMIT_STATE_IDLE,
> >  	SPACEMIT_STATE_START,
> > @@ -125,6 +128,7 @@ struct spacemit_i2c_dev {
> >  
> >  	enum spacemit_i2c_state state;
> >  	bool read;
> > +	bool is_pio;
> using_pio_mode or simply use_pio, but have to say..
> 
> I feel it's improper to have this flag here, since it's not a controller
> level feature, I understand it was introduced to support aotmic operation
> 
> Personally, I'd suggest to pass the flag in xfer(), then propagate down to
> whatever needed, so it limit to single transmission which more flexible
I no longer agree with moving the flag into xfer.
Keeping it as a global variable is better,
otherwise it would affect several functions:
wait_bus_idle(), start(), init(), handle_write(), handle_read(), etc.

                - Troy

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

[Troy Mitchell]

On Sun, Sep 28, 2025 at 10:54:00AM +0800, Yixun Lan wrote:
> Hi Troy,
> 
> On 09:17 Sun 28 Sep     , Troy Mitchell wrote:
> > On Sat, Sep 27, 2025 at 06:56:16PM +0800, Yixun Lan wrote:
> > > On 10:02 Thu 25 Sep     , 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 | 164 +++++++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 140 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > > > index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> > > > --- a/drivers/i2c/busses/i2c-k1.c
> > > > +++ b/drivers/i2c/busses/i2c-k1.c
> > > > @@ -97,6 +97,9 @@
> > > >  
> > > ..
> > > >  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> > > >  {
> > > >  	unsigned long time_left;
> > > > @@ -310,10 +368,28 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> > > >  
> > > >  		reinit_completion(&i2c->complete);
> > > >  
> > > > -		spacemit_i2c_start(i2c);
> > > > +		if (i2c->is_pio) {
> > > > +			/* We disable the interrupt to avoid unintended spurious triggers. */
> > > the comment is suspicious, and seems wrong..
> > > > +			disable_irq(i2c->irq);
> > > > +
> > > I guess this doesn't disable interrupt in the hardware layer, it will still
> > > fire interrupt once enabled, so instead of calling disable_irq(), why not
> > > dealing with ISR setting of the controller? I mean those "IE bits"(interrupt
> > > enableing) of ICR REGISTER, disabling them should prevent the interrupt
> > > triggered?
> > For example, take MSD (master stop detect).
> > If we disable this interrupt, even the interrupt status bit will never be triggered.
> No, this is not something I understand..
> > Then how are we supposed to know when the transfer has completed?
> > That’s why we disable the global interrupt here, but still keep the pending bit.
> checking 18.1.4.1 ICR REGISTER, there is Master Stop Deteced Enable - MSDE (Bit[26]) and
> Master Stop Deteced Interrupt Enable - MSDIE (BIT[25])
> you can disable MSDIE but still enable MSDE, and check status of MSD (BIT[26]) of ISR REGISTER?
> it should still work with interrupt disabled, otherwise I'd consider the hardware is broken.

Okay..maybe I misunderstood before.
I'll remove disable/enable the controller IRQ around PIO transfers.

BTW, the K1 I2C patch [1] for ILCR as the SCL clock hasn't gotten
any replies in ages. Could you take a look?

Link:
https://lore.kernel.org/all/20250814-k1-i2c-ilcr-v3-1-317723e74bcd@linux.spacemit.com/ [1]
> 
> -- 
> Yixun Lan (dlan)
> 

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

[Yixun Lan]

Hi Troy,

On 16:09 Sun 28 Sep     , Troy Mitchell wrote:
> 
..[snip]
> Okay..maybe I misunderstood before.
> I'll remove disable/enable the controller IRQ around PIO transfers.
> 
> BTW, the K1 I2C patch [1] for ILCR as the SCL clock hasn't gotten
> any replies in ages. Could you take a look?
stop complain, you have to understand people are voluntary to review the code..
and maintainers are busy, unfortunately..

btw, you've ignored v2's review, no response, not addressed in v3
https://lore.kernel.org/all/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com/


> 
> Link:
> https://lore.kernel.org/all/20250814-k1-i2c-ilcr-v3-1-317723e74bcd@linux.spacemit.com/ [1]
> > 

-- 
Yixun Lan (dlan)