[PATCH v2 0/2] riscv: spacemit: add i2c support to K1 SoC

[PATCH v2 0/2] riscv: spacemit: add i2c support to K1 SoC

[Troy Mitchell]

Hi all,

This patch implements I2C driver for the SpacemiT K1 SoC,
providing basic support for I2C read/write communication which
compatible with standard I2C bus specifications.

In this version, the driver defaults to use fast-speed-mode and
interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.

The docs of I2C can be found here, in chapter 16.1 I2C [1]

Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]

Troy Mitchell (2):
  dt-bindings: i2c: spacemit: add support for K1 SoC
  i2c: spacemit: add support for SpacemiT K1 SoC

 .../bindings/i2c/spacemit,k1-i2c.yaml         |  51 ++
 drivers/i2c/busses/Kconfig                    |  18 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-k1.c                   | 658 ++++++++++++++++++
 4 files changed, 728 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-k1.c

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Troy Mitchell]

The I2C of K1 supports fast-speed-mode and high-speed-mode,
and supports FIFO transmission.

Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
---
 .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
new file mode 100644
index 000000000000..57af66f494e7
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C controller embedded in SpacemiT's K1 SoC
+
+maintainers:
+  - Troy Mitchell <troymitchell988@gmail.com>
+
+properties:
+  compatible:
+    const: spacemit,k1-i2c
+
+  reg:
+    maxItems: 2
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-frequency:
+    description:
+      Desired I2C bus clock frequency in Hz. As only fast and high-speed
+      modes are supported by hardware, possible values are 100000 and 400000.
+    enum: [100000, 400000]
+    default: 100000
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c@d4010800 {
+        compatible = "spacemit,k1-i2c";
+        reg = <0x0 0xd4010800 0x0 0x38>;
+        interrupt-parent = <&plic>;
+        interrupts = <36>;
+        clocks = <&ccu 90>;
+        clock-frequency = <100000>;
+    };
+
+...
-- 
2.34.1

[PATCH v2 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Troy Mitchell]

This patch introduces basic I2C support for the SpacemiT K1 SoC,
utilizing interrupts for transfers.

The driver has been tested using i2c-tools on a Bananapi-F3 board,
and basic I2C read/write operations have been confirmed to work.

Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
---
 drivers/i2c/busses/Kconfig  |  18 +
 drivers/i2c/busses/Makefile |   1 +
 drivers/i2c/busses/i2c-k1.c | 658 ++++++++++++++++++++++++++++++++++++
 3 files changed, 677 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-k1.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 6b3ba7e5723a..38ebfd38dc41 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -779,6 +779,24 @@ config I2C_JZ4780
 
 	 If you don't know what to do here, say N.
 
+config I2C_K1
+	tristate "Spacemit K1 I2C adapter"
+	depends on HAS_IOMEM
+	help
+	  This option enables support for the I2C interface on the Spacemit K1
+	  platform.
+
+	  If you enable this configuration, the kernel will include support for
+	  the I2C adapter specific to the Spacemit K1 platform. This driver ca
+	  be used to manage I2C bus transactions, which are necessary for
+	  interfacing with I2C peripherals such as sensors, EEPROMs, and other
+	  devices.
+
+	  This driver can also be compiled as a module. If you choose to build
+	  it as a module, the resulting kernel module will be named `i2c-k1`.
+	  Loading this module will enable the I2C functionality for the K1
+	  platform dynamically, without requiring a rebuild of the kernel.
+
 config I2C_KEBA
 	tristate "KEBA I2C controller support"
 	depends on HAS_IOMEM
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index ecc07c50f2a0..9744b0e58598 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_IMX)		+= i2c-imx.o
 obj-$(CONFIG_I2C_IMX_LPI2C)	+= i2c-imx-lpi2c.o
 obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
 obj-$(CONFIG_I2C_JZ4780)	+= i2c-jz4780.o
+obj-$(CONFIG_I2C_K1)		+= i2c-k1.o
 obj-$(CONFIG_I2C_KEBA)		+= i2c-keba.o
 obj-$(CONFIG_I2C_KEMPLD)	+= i2c-kempld.o
 obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
new file mode 100644
index 000000000000..844d4a721760
--- /dev/null
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -0,0 +1,658 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
+ */
+
+ #include <linux/clk.h>
+ #include <linux/i2c.h>
+ #include <linux/iopoll.h>
+ #include <linux/module.h>
+ #include <linux/of_address.h>
+ #include <linux/platform_device.h>
+
+/* spacemit i2c registers */
+#define ICR          0x0		/* Control Register */
+#define ISR          0x4		/* Status Register */
+#define ISAR         0x8		/* Slave Address Register */
+#define IDBR         0xc		/* Data Buffer Register */
+#define ILCR         0x10		/* Load Count Register */
+#define IWCR         0x14		/* Wait Count Register */
+#define IRST_CYC     0x18		/* Bus reset cycle counter */
+#define IBMR         0x1c		/* Bus monitor register */
+#define IWFIFO       0x20		/* Write FIFO Register */
+#define IWFIFO_WPTR  0x24		/* Write FIFO Write Pointer Register */
+#define IWFIFO_RPTR  0x28		/* Write FIFO Read Pointer Register */
+#define IRFIFO       0x2c		/* Read FIFO Register */
+#define IRFIFO_WPTR  0x30		/* Read FIFO Write Pointer Register */
+#define IRFIFO_RPTR  0x34		/* Read FIFO Read Pointer Register */
+
+/* register ICR fields */
+#define CR_START        BIT(0)		/* start bit */
+#define CR_STOP         BIT(1)		/* stop bit */
+#define CR_ACKNAK       BIT(2)		/* send ACK(0) or NAK(1) */
+#define CR_TB           BIT(3)		/* transfer byte bit */
+#define CR_TXBEGIN      BIT(4)		/* transaction begin */
+#define CR_FIFOEN       BIT(5)		/* enable FIFO mode */
+#define CR_GPIOEN       BIT(6)		/* enable GPIO mode for SCL in HS */
+#define CR_DMAEN        BIT(7)		/* enable DMA for TX and RX FIFOs */
+#define CR_MODE_FAST    BIT(8)		/* bus mode (master operation) */
+#define CR_MODE_HIGH    BIT(9)		/* bus mode (master operation) */
+#define CR_UR           BIT(10)		/* unit reset */
+#define CR_RSTREQ       BIT(11)		/* i2c bus reset request */
+#define CR_MA           BIT(12)		/* master abort */
+#define CR_SCLE         BIT(13)		/* master clock enable */
+#define CR_IUE          BIT(14)		/* unit enable */
+#define CR_HS_STRETCH   BIT(16)		/* I2C hs stretch */
+#define CR_ALDIE        BIT(18)		/* enable arbitration interrupt */
+#define CR_DTEIE        BIT(19)		/* enable tx interrupts */
+#define CR_DRFIE        BIT(20)		/* enable rx interrupts */
+#define CR_GCD          BIT(21)		/* general call disable */
+#define CR_BEIE         BIT(22)		/* enable bus error ints */
+#define CR_SADIE        BIT(23)		/* slave address detected int enable */
+#define CR_SSDIE        BIT(24)		/* slave STOP detected int enable */
+#define CR_MSDIE        BIT(25)		/* master STOP detected int enable */
+#define CR_MSDE         BIT(26)		/* master STOP detected enable */
+#define CR_TXDONEIE     BIT(27)		/* transaction done int enable */
+#define CR_TXEIE        BIT(28)		/* transmit FIFO empty int enable */
+#define CR_RXHFIE       BIT(29)		/* receive FIFO half-full int enable */
+#define CR_RXFIE        BIT(30)		/* receive FIFO full int enable */
+#define CR_RXOVIE       BIT(31)		/* receive FIFO overrun int enable */
+
+/* register ISR fields */
+#define SR_RWM          BIT(13)		/* read/write mode */
+#define SR_ACKNAK       BIT(14)		/* ACK/NACK status */
+#define SR_UB           BIT(15)		/* unit busy */
+#define SR_IBB          BIT(16)		/* i2c bus busy */
+#define SR_EBB          BIT(17)		/* early bus busy */
+#define SR_ALD          BIT(18)		/* arbitration loss detected */
+#define SR_ITE          BIT(19)		/* tx buffer empty */
+#define SR_IRF          BIT(20)		/* rx buffer full */
+#define SR_GCAD         BIT(21)		/* general call address detected */
+#define SR_BED          BIT(22)		/* bus error no ACK/NAK */
+#define SR_SAD          BIT(23)		/* slave address detected */
+#define SR_SSD          BIT(24)		/* slave stop detected */
+#define SR_MSD          BIT(26)		/* master stop detected */
+#define SR_TXDONE       BIT(27)		/* transaction done */
+#define SR_TXE          BIT(28)		/* tx FIFO empty */
+#define SR_RXHF         BIT(29)		/* rx FIFO half-full */
+#define SR_RXF          BIT(30)		/* rx FIFO full */
+#define SR_RXOV         BIT(31)		/* RX FIFO overrun */
+
+/* register ILCR fields */
+#define LCR_SLV         0x000001FF	/* SLV: bit[8:0] */
+#define LCR_FLV         0x0003FE00	/* FLV: bit[17:9] */
+#define LCR_HLVH        0x07FC0000	/* HLVH: bit[26:18] */
+#define LCR_HLVL        0xF8000000	/* HLVL: bit[31:27] */
+
+/* register IWCR fields */
+#define WCR_COUNT       0x0000001F	/* COUNT: bit[4:0] */
+#define WCR_COUNT1      0x000003E0	/* HS_COUNT1: bit[9:5] */
+#define WCR_COUNT2      0x00007C00	/* HS_COUNT2: bit[14:10] */
+
+/* register IBMR fields */
+#define BMR_SDA         BIT(0)		/* SDA line level */
+#define BMR_SCL         BIT(1)		/* SCL line level */
+
+/* register IWFIFO fields */
+#define WFIFO_DATA_MSK      0x000000FF  /* data: bit[7:0] */
+#define WFIFO_CTRL_MSK      0x000003E0  /* control: bit[11:8] */
+#define WFIFO_CTRL_START    BIT(8)      /* start bit */
+#define WFIFO_CTRL_STOP     BIT(9)      /* stop bit */
+#define WFIFO_CTRL_ACKNAK   BIT(10)     /* send ACK(0) or NAK(1) */
+#define WFIFO_CTRL_TB       BIT(11)     /* transfer byte bit */
+
+/* status register init value */
+#define I2C_INT_STATUS_MASK    0xfffc0000  /* SR bits[31:18] */
+#define I2C_INT_CTRL_MASK      (CR_ALDIE | CR_DTEIE | CR_DRFIE | \
+				CR_BEIE | CR_TXDONEIE | CR_TXEIE | \
+				CR_RXHFIE | CR_RXFIE | CR_RXOVIE | \
+				CR_MSDIE)
+
+/* i2c bus recover timeout: us */
+#define I2C_BUS_RECOVER_TIMEOUT		(100000)
+
+#define I2C_FAST_MODE_FREQ		(400000)
+
+enum spacemit_i2c_state {
+	STATE_IDLE,
+	STATE_START,
+	STATE_READ,
+	STATE_WRITE,
+};
+
+enum spacemit_i2c_dir {
+	DIR_WRITE,
+	DIR_READ
+};
+
+/* i2c-spacemit driver's main struct */
+struct spacemit_i2c_dev {
+	struct device *dev;
+	struct i2c_adapter adapt;
+
+	/* hardware resources */
+	void __iomem *base;
+	int irq;
+
+	struct i2c_msg *msgs;
+	int msg_num;
+	struct i2c_msg *cur_msg;
+
+	/* index of the current message being processed */
+	int msg_idx;
+	u8 *msg_buf;
+	/* the number of unprocessed bytes remaining in each message  */
+	size_t unprocessed;
+
+	enum spacemit_i2c_state state;
+	enum spacemit_i2c_dir dir;
+	struct completion complete;
+	u32 status;
+	u32 err;
+};
+
+static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + ICR);
+	writel(val | CR_IUE, i2c->base + ICR);
+}
+
+static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + ICR);
+	val &= ~CR_IUE;
+	writel(val, i2c->base + ICR);
+}
+
+static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
+{
+	writel(CR_UR, i2c->base + ICR);
+	udelay(5);
+	writel(0, i2c->base + ICR);
+}
+
+static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c);
+
+static void spacemit_i2c_bus_reset(struct spacemit_i2c_dev *i2c)
+{
+	u32 status;
+
+	/* if bus is locked, reset unit. 0: locked */
+	status = readl(i2c->base + IBMR);
+	if ((status & BMR_SDA) && (status & BMR_SCL))
+		return;
+
+	spacemit_i2c_reset(i2c);
+	usleep_range(10, 20);
+
+	/* check scl status again */
+	status = readl(i2c->base + IBMR);
+	if (!(status & BMR_SCL))
+		dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
+}
+
+static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c)
+{
+	int ret = 0;
+	u32 val;
+
+	val = readl(i2c->base + ISR);
+	if (likely(!(val & (SR_UB | SR_IBB))))
+		return 0;
+
+	ret = readl_poll_timeout(i2c->base + ISR, val, !(val & (SR_UB | SR_IBB)),
+				 1500, I2C_BUS_RECOVER_TIMEOUT);
+	if (unlikely(ret)) {
+		spacemit_i2c_reset(i2c);
+		ret = -EAGAIN;
+	}
+
+	return ret;
+}
+
+static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
+{
+	/* in case bus is not released after transfer completes */
+	if (unlikely(readl(i2c->base + ISR) & SR_EBB)) {
+		spacemit_i2c_bus_reset(i2c);
+		usleep_range(90, 150);
+	}
+}
+
+static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
+{
+	u32 val = 0;
+
+	/*
+	 * 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 CR_TXDONEIE.
+	 */
+	val |= CR_BEIE | CR_ALDIE;
+
+	/*
+	 * Unmask interrupt bits for interrupt xfer mode:
+	 * DBR rx full.
+	 * For tx empty interrupt CR_DTEIE, we only
+	 * need to enable when trigger byte transfer to start
+	 * data sending.
+	 */
+	val |= CR_DRFIE;
+
+	/* set speed bits: default fast mode */
+	val |= CR_MODE_FAST;
+
+	/* disable response to general call */
+	val |= CR_GCD;
+
+	/* enable SCL clock output */
+	val |= CR_SCLE;
+
+	/* enable master stop detected */
+	val |= CR_MSDE | CR_MSDIE;
+
+	writel(val, i2c->base + ICR);
+}
+
+static inline void
+spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
+{
+	writel(mask & I2C_INT_STATUS_MASK, i2c->base + ISR);
+}
+
+static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
+{
+	u32 slave_addr_rw, val;
+
+	i2c->dir = i2c->cur_msg->flags & I2C_M_RD;
+	i2c->state = STATE_START;
+
+	if (i2c->cur_msg->flags & I2C_M_RD)
+		slave_addr_rw = ((i2c->cur_msg->addr & 0x7f) << 1) | 1;
+	else
+		slave_addr_rw = (i2c->cur_msg->addr & 0x7f) << 1;
+
+	writel(slave_addr_rw, i2c->base + IDBR);
+
+	val = readl(i2c->base + ICR);
+
+	/* send start pulse */
+	val &= ~CR_STOP;
+	val |= CR_START | CR_TB | CR_DTEIE;
+	writel(val, i2c->base + ICR);
+}
+
+static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + ICR);
+
+	val |= CR_STOP | CR_ALDIE | CR_TB;
+
+	if (i2c->dir == DIR_READ)
+		val |= CR_ACKNAK;
+
+	writel(val, i2c->base + ICR);
+}
+
+static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
+{
+	unsigned long time_left;
+
+	if (unlikely(i2c->err))
+		return -1;
+
+	for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
+		i2c->cur_msg = i2c->msgs + i2c->msg_idx;
+		i2c->msg_buf = i2c->cur_msg->buf;
+		i2c->err = 0;
+		i2c->status = 0;
+		i2c->unprocessed = i2c->cur_msg->len;
+
+		reinit_completion(&i2c->complete);
+
+		spacemit_i2c_start(i2c);
+
+		time_left = wait_for_completion_timeout(&i2c->complete,
+							i2c->adapt.timeout);
+		if (unlikely(time_left == 0)) {
+			dev_alert(i2c->dev, "msg completion timeout\n");
+			spacemit_i2c_bus_reset(i2c);
+			spacemit_i2c_reset(i2c);
+			return -ETIMEDOUT;
+		}
+
+		if (unlikely(i2c->err))
+			return spacemit_i2c_handle_err(i2c);
+	}
+
+	return 0;
+}
+
+static int spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
+{
+	if (i2c->dir == DIR_READ)
+		if (i2c->unprocessed == 1 && i2c->msg_idx == i2c->msg_num - 1)
+			return 1;
+	else if (i2c->dir == DIR_WRITE)
+		if (!i2c->unprocessed && i2c->msg_idx == i2c->msg_num - 1)
+			return 1;
+
+	return 0;
+}
+
+static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
+{
+	/* if transfer completes, ISR will handle it */
+	if (i2c->status & SR_MSD)
+		return;
+
+	if (i2c->unprocessed) {
+		writel(*i2c->msg_buf++, i2c->base + IDBR);
+		i2c->unprocessed--;
+		return;
+	}
+
+	/* STATE_IDLE avoids trigger next byte */
+	i2c->state = STATE_IDLE;
+	complete(&i2c->complete);
+}
+
+static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
+{
+	if (i2c->unprocessed) {
+		*i2c->msg_buf++ = readl(i2c->base + IDBR);
+		i2c->unprocessed--;
+	}
+
+	/* if transfer completes, ISR will handle it */
+	if (i2c->status & (SR_MSD | SR_ACKNAK))
+		return;
+
+	/* it has to append stop bit in icr that read last byte */
+	if (i2c->unprocessed)
+		return;
+
+	/* STATE_IDLE avoids trigger next byte */
+	i2c->state = STATE_IDLE;
+	complete(&i2c->complete);
+}
+
+static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
+{
+	if (i2c->dir == DIR_READ) {
+		i2c->state = STATE_READ;
+		return;
+	}
+
+	if (i2c->dir == DIR_WRITE) {
+		i2c->state = STATE_WRITE;
+		spacemit_i2c_handle_write(i2c);
+	}
+}
+
+static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
+{
+	if (!i2c->err)
+		return 0;
+
+	dev_dbg(i2c->dev, "i2c error status: 0x%08x\n",	i2c->status);
+
+	if (i2c->err & (SR_BED | SR_ALD))
+		spacemit_i2c_reset(i2c);
+
+	if (i2c->err & (SR_RXOV | SR_ALD))
+		return -EAGAIN;
+
+	return (i2c->status & SR_ACKNAK) ? -ENXIO : -EIO;
+}
+
+static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+	/*
+	 * send transaction complete signal:
+	 * error happens, detect master stop
+	 */
+	if (likely(i2c->err || (i2c->status & SR_MSD))) {
+		/*
+		 * Here the transaction is already done, we don't need any
+		 * other interrupt signals from now, in case any interrupt
+		 * happens before spacemit_i2c_xfer to disable irq and i2c unit,
+		 * we mask all the interrupt signals and clear the interrupt
+		 * status.
+		 */
+		val = readl(i2c->base + ICR);
+		val &= ~I2C_INT_CTRL_MASK;
+		writel(val, i2c->base + ICR);
+
+		spacemit_i2c_clear_int_status(i2c, I2C_INT_STATUS_MASK);
+
+		i2c->state = STATE_IDLE;
+		complete(&i2c->complete);
+	}
+}
+
+static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
+{
+	struct spacemit_i2c_dev *i2c = devid;
+	u32 status, val;
+
+	status = readl(i2c->base + ISR);
+	if (!status)
+		return IRQ_HANDLED;
+
+	i2c->status = status;
+
+	i2c->err = status & (SR_BED | SR_RXOV | SR_ALD);
+
+	spacemit_i2c_clear_int_status(i2c, status);
+
+	if (unlikely(i2c->err))
+		goto err_out;
+
+	val = readl(i2c->base + ICR);
+
+	val &= ~(CR_TB | CR_ACKNAK | CR_STOP | CR_START);
+	writel(val, i2c->base + ICR);
+
+	switch (i2c->state) {
+	case STATE_START:
+		spacemit_i2c_handle_start(i2c);
+		break;
+	case STATE_READ:
+		spacemit_i2c_handle_read(i2c);
+		break;
+	case STATE_WRITE:
+		spacemit_i2c_handle_write(i2c);
+		break;
+	default:
+		break;
+	}
+
+	if (i2c->state != STATE_IDLE) {
+		if (spacemit_i2c_is_last_msg(i2c)) {
+			/* trigger next byte with stop */
+			spacemit_i2c_stop(i2c);
+		} else {
+			/* trigger next byte */
+			val |= CR_ALDIE | CR_TB;
+			writel(val, i2c->base + ICR);
+		}
+	}
+
+err_out:
+	spacemit_i2c_err_check(i2c);
+	return IRQ_HANDLED;
+}
+
+static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
+{
+	unsigned long timeout;
+	int idx = 0, cnt = 0, freq;
+
+	while (idx < i2c->msg_num) {
+		cnt += (i2c->msgs + idx)->len + 1;
+		idx++;
+	}
+
+	freq = I2C_FAST_MODE_FREQ;
+	/*
+	 * multiply by 9 because each byte in I2C transmission requires
+	 * 9 clock cycles: 8 bits of data plus 1 ACK/NACK bit.
+	 */
+	timeout = cnt * 9 * USEC_PER_SEC / freq;
+
+	i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 2) / i2c->msg_num;
+}
+
+static inline int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c)
+{
+	int ret = 0;
+
+	spacemit_i2c_reset(i2c);
+
+	spacemit_i2c_calc_timeout(i2c);
+
+	spacemit_i2c_init(i2c);
+
+	spacemit_i2c_enable(i2c);
+	enable_irq(i2c->irq);
+
+	/* i2c wait for bus busy */
+	ret = spacemit_i2c_recover_bus_busy(i2c);
+	if (unlikely(ret))
+		return ret;
+
+	ret = spacemit_i2c_xfer_msg(i2c);
+	if (unlikely(ret < 0)) {
+		dev_dbg(i2c->dev, "i2c transfer error\n");
+		/* timeout error should not be overridden, and the transfer
+		 * error will be confirmed by err handle function latter,
+		 * the reset should be invalid argument error.
+		 */
+		if (ret != -ETIMEDOUT)
+			ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int
+spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg msgs[], int num)
+{
+	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
+	int ret;
+
+	i2c->msgs = msgs;
+	i2c->msg_num = num;
+
+	ret = spacemit_i2c_xfer_core(i2c);
+	if (likely(!ret))
+		spacemit_i2c_check_bus_release(i2c);
+
+	disable_irq(i2c->irq);
+
+	spacemit_i2c_disable(i2c);
+
+	if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))
+		dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, i2c->err);
+
+	return ret < 0 ? ret : num;
+}
+
+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,
+	.functionality = spacemit_i2c_func,
+};
+
+static int spacemit_i2c_probe(struct platform_device *pdev)
+{
+	struct spacemit_i2c_dev *i2c;
+	struct device_node *of_node = pdev->dev.of_node;
+	struct clk *clk;
+	int ret = 0;
+
+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->dev = &pdev->dev;
+
+	i2c->base = devm_platform_ioremap_resource(pdev, 0);
+	if (!i2c->base)
+		return dev_err_probe(&pdev->dev, PTR_ERR(i2c->base), "failed to do ioremap");
+
+	i2c->irq = platform_get_irq(pdev, 0);
+	if (i2c->irq < 0)
+		return dev_err_probe(&pdev->dev, i2c->irq, "failed to get irq resource");
+
+	ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler,
+			       IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to request irq");
+
+	disable_irq(i2c->irq);
+
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to enable clock");
+
+	i2c_set_adapdata(&i2c->adapt, i2c);
+	i2c->adapt.owner = THIS_MODULE;
+	i2c->adapt.algo = &spacemit_i2c_algo;
+	i2c->adapt.dev.parent = i2c->dev;
+	i2c->adapt.nr = pdev->id;
+
+	i2c->adapt.dev.of_node = of_node;
+	i2c->adapt.algo_data = i2c;
+
+	strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name));
+
+	init_completion(&i2c->complete);
+
+	ret = i2c_add_numbered_adapter(&i2c->adapt);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
+
+	platform_set_drvdata(pdev, i2c);
+
+	return 0;
+}
+
+static void spacemit_i2c_remove(struct platform_device *pdev)
+{
+	struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adapt);
+}
+
+static const struct of_device_id spacemit_i2c_of_match[] = {
+	{ .compatible = "spacemit,k1-i2c", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match);
+
+static struct platform_driver spacemit_i2c_driver = {
+	.probe = spacemit_i2c_probe,
+	.remove = spacemit_i2c_remove,
+	.driver = {
+		.name = "i2c-k1",
+		.of_match_table = spacemit_i2c_of_match,
+	},
+};
+module_platform_driver(spacemit_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC");
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Krzysztof Kozlowski]

On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---

Where is the changelog? Nothing here, nothing in cover letter.

I asked for several changes, so now I don't know if you implemented
them.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[kernel test robot]

Hi Troy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on robh/for-next linus/master v6.12-rc5 next-20241028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Troy-Mitchell/dt-bindings-i2c-spacemit-add-support-for-K1-SoC/20241028-133452
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20241028053220.346283-3-TroyMitchell988%40gmail.com
patch subject: [PATCH v2 2/2] i2c: spacemit: add support for SpacemiT K1 SoC
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20241028/202410282041.JgAO0LV0-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241028/202410282041.JgAO0LV0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410282041.JgAO0LV0-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/i2c/busses/i2c-k1.c: In function 'spacemit_i2c_is_last_msg':
>> drivers/i2c/busses/i2c-k1.c:340:12: warning: suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
     340 |         if (i2c->dir == DIR_READ)
         |            ^

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +/else +340 drivers/i2c/busses/i2c-k1.c

   337	
   338	static int spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
   339	{
 > 340		if (i2c->dir == DIR_READ)
   341			if (i2c->unprocessed == 1 && i2c->msg_idx == i2c->msg_num - 1)
   342				return 1;
   343		else if (i2c->dir == DIR_WRITE)
   344			if (!i2c->unprocessed && i2c->msg_idx == i2c->msg_num - 1)
   345				return 1;
   346	
   347		return 0;
   348	}
   349	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[kernel test robot]

Hi Troy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on robh/for-next linus/master v6.12-rc5 next-20241028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Troy-Mitchell/dt-bindings-i2c-spacemit-add-support-for-K1-SoC/20241028-133452
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20241028053220.346283-3-TroyMitchell988%40gmail.com
patch subject: [PATCH v2 2/2] i2c: spacemit: add support for SpacemiT K1 SoC
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20241028/202410282220.vl7podpG-lkp@intel.com/config)
compiler: clang version 19.1.2 (https://github.com/llvm/llvm-project 7ba7d8e2f7b6445b60679da826210cdde29eaf8b)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241028/202410282220.vl7podpG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410282220.vl7podpG-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/i2c/busses/i2c-k1.c:7:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/i2c/busses/i2c-k1.c:343:2: warning: add explicit braces to avoid dangling else [-Wdangling-else]
     343 |         else if (i2c->dir == DIR_WRITE)
         |         ^
   5 warnings generated.


vim +343 drivers/i2c/busses/i2c-k1.c

   337	
   338	static int spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
   339	{
   340		if (i2c->dir == DIR_READ)
   341			if (i2c->unprocessed == 1 && i2c->msg_idx == i2c->msg_num - 1)
   342				return 1;
 > 343		else if (i2c->dir == DIR_WRITE)
   344			if (!i2c->unprocessed && i2c->msg_idx == i2c->msg_num - 1)
   345				return 1;
   346	
   347		return 0;
   348	}
   349	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Troy Mitchell]

On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>> and supports FIFO transmission.
>>
>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>> ---
> 
> Where is the changelog? Nothing here, nothing in cover letter.
> 
> I asked for several changes, so now I don't know if you implemented
> them.

I deleted the FIFO property because I believe your suggestion is correct.
this should be decided by the driver, even though the FIFO is provided
by the hardware.

Apologies for missing the changelog. To correct this, should I send a v3 
version with the changelog or resend v2?
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Krzysztof Kozlowski]

On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
> > On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
> >> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> >> and supports FIFO transmission.
> >>
> >> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> >> ---
> > 
> > Where is the changelog? Nothing here, nothing in cover letter.
> > 
> > I asked for several changes, so now I don't know if you implemented
> > them.
> 
> I deleted the FIFO property because I believe your suggestion is correct.
> this should be decided by the driver, even though the FIFO is provided
> by the hardware.
> 
> Apologies for missing the changelog. To correct this, should I send a v3 
> version with the changelog or resend v2?

Reply now with changelog. Your binding has some other unrelated and
incorrect changes, which I do not understand.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Krzysztof Kozlowski]

On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---
>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> new file mode 100644
> index 000000000000..57af66f494e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C controller embedded in SpacemiT's K1 SoC
> +
> +maintainers:
> +  - Troy Mitchell <troymitchell988@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-i2c
> +
> +  reg:
> +    maxItems: 2

Provide changelog explaining why you are doing some changes.

List and describe items here instead.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description:
> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
> +      modes are supported by hardware, possible values are 100000 and 400000.
> +    enum: [100000, 400000]
> +    default: 100000
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c@d4010800 {
> +        compatible = "spacemit,k1-i2c";
> +        reg = <0x0 0xd4010800 0x0 0x38>;

Missing second item.

Best regards,
Krzysztof

Re: [PATCH v2 0/2] riscv: spacemit: add i2c support to K1 SoC

[Andi Shyti]

Hi Tony,

On Mon, Oct 28, 2024 at 01:32:18PM +0800, Troy Mitchell wrote:
> Hi all,
> 
> This patch implements I2C driver for the SpacemiT K1 SoC,
> providing basic support for I2C read/write communication which
> compatible with standard I2C bus specifications.
> 
> In this version, the driver defaults to use fast-speed-mode and
> interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.
> 
> The docs of I2C can be found here, in chapter 16.1 I2C [1]
> 
> Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]
> 
> Troy Mitchell (2):
>   dt-bindings: i2c: spacemit: add support for K1 SoC
>   i2c: spacemit: add support for SpacemiT K1 SoC

As Krzysztof has asked, please do provide the changelog, it's
important to track the progress of your series.

Thanks,
Andi

>  .../bindings/i2c/spacemit,k1-i2c.yaml         |  51 ++
>  drivers/i2c/busses/Kconfig                    |  18 +
>  drivers/i2c/busses/Makefile                   |   1 +
>  drivers/i2c/busses/i2c-k1.c                   | 658 ++++++++++++++++++
>  4 files changed, 728 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>  create mode 100644 drivers/i2c/busses/i2c-k1.c
> 
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Jesse T]

On Fri, Nov 1, 2024 at 10:24 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
> > On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
> > > On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
> > >> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> > >> and supports FIFO transmission.
> > >>
> > >> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> > >> ---

Change in v2:
- drop fifo-disable property
- unevaluatedProperties goes after required: block
- drop alias

> > >
> > > Where is the changelog? Nothing here, nothing in cover letter.
It seems like it was accidentally dropped seeing there are the dashes.

> > >
> > > I asked for several changes, so now I don't know if you implemented
> > > them.
> >
> > I deleted the FIFO property because I believe your suggestion is correct.
> > this should be decided by the driver, even though the FIFO is provided
> > by the hardware.
> >
> > Apologies for missing the changelog. To correct this, should I send a v3
> > version with the changelog or resend v2?
>
> Reply now with changelog. Your binding has some other unrelated and
> incorrect changes, which I do not understand.

...

Thanks,
Jesse Taube

>
> Best regards,
> Krzysztof
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Krzysztof Kozlowski]

On 01/11/2024 15:29, Jesse T wrote:
> On Fri, Nov 1, 2024 at 10:24 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>>> and supports FIFO transmission.
>>>>>
>>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>>> ---
> 
> Change in v2:
> - drop fifo-disable property
> - unevaluatedProperties goes after required: block
> - drop alias

b4 diff disagrees with you :/

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Samuel Holland]

Hi Troy,

On 2024-10-28 12:32 AM, Troy Mitchell wrote:
> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---
>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> new file mode 100644
> index 000000000000..57af66f494e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C controller embedded in SpacemiT's K1 SoC
> +
> +maintainers:
> +  - Troy Mitchell <troymitchell988@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-i2c
> +
> +  reg:
> +    maxItems: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1

Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
looks to be standard across the peripherals in this SoC. Please be sure that the
binding covers all resources needed to use this peripheral.

> +
> +  clock-frequency:
> +    description:
> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
> +      modes are supported by hardware, possible values are 100000 and 400000.
> +    enum: [100000, 400000]

This looks wrong. In the manual I see:

* Supports standard-mode operation up to 100 Kbps
* Supports fast-mode operation up to 400Kbps
* Supports high-speed mode (HS mode) slave operation up to 3.4Mbps(High-speed
I2C only)
* Supports high-speed mode (HS mode) master operation up to 3.3 Mbps (High-speed
I2C only)

So even ignoring HS mode, 100 kHz and 400 kHz are only the maximums, not fixed
frequencies.

Regards,
Samuel

> +    default: 100000
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c@d4010800 {
> +        compatible = "spacemit,k1-i2c";
> +        reg = <0x0 0xd4010800 0x0 0x38>;
> +        interrupt-parent = <&plic>;
> +        interrupts = <36>;
> +        clocks = <&ccu 90>;
> +        clock-frequency = <100000>;
> +    };
> +
> +...

Re: [PATCH v2 0/2] riscv: spacemit: add i2c support to K1 SoC

[Troy Mitchell]

On 2024/10/31 19:43, Andi Shyti wrote:
> Hi Tony,
> 
> On Mon, Oct 28, 2024 at 01:32:18PM +0800, Troy Mitchell wrote:
>> Hi all,
>>
>> This patch implements I2C driver for the SpacemiT K1 SoC,
>> providing basic support for I2C read/write communication which
>> compatible with standard I2C bus specifications.
>>
>> In this version, the driver defaults to use fast-speed-mode and
>> interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.
>>
>> The docs of I2C can be found here, in chapter 16.1 I2C [1]
>>
>> Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]
>>
>> Troy Mitchell (2):
>>   dt-bindings: i2c: spacemit: add support for K1 SoC
>>   i2c: spacemit: add support for SpacemiT K1 SoC
> 
> As Krzysztof has asked, please do provide the changelog, it's
> important to track the progress of your series.
I saw a compilation warning sent to me by the robot, and I've
fixed the warning. Should I resend V2 with the changelog
what I miss or send V3?

Thank for your response.
> 
> Thanks,
> Andi
> 
>>  .../bindings/i2c/spacemit,k1-i2c.yaml         |  51 ++
>>  drivers/i2c/busses/Kconfig                    |  18 +
>>  drivers/i2c/busses/Makefile                   |   1 +
>>  drivers/i2c/busses/i2c-k1.c                   | 658 ++++++++++++++++++
>>  4 files changed, 728 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>  create mode 100644 drivers/i2c/busses/i2c-k1.c
>>
>> -- 
>> 2.34.1
>>

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Troy Mitchell]

On 2024/10/31 16:00, Krzysztof Kozlowski wrote:
> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>> and supports FIFO transmission.
>>>>
>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>> ---
Change in v2:
 - Change the maxItems of reg from 1 to 2 in properties
 - Change 'i2c' to 'I2C' in the commit message.
 - Drop fifo-disable property
 - Drop alias in dts example
 - Move `unevaluatedProperties` after `required:` block

>>>
>>> Where is the changelog? Nothing here, nothing in cover letter.
>>>
>>> I asked for several changes, so now I don't know if you implemented
>>> them.
>>
>> I deleted the FIFO property because I believe your suggestion is correct.
>> this should be decided by the driver, even though the FIFO is provided
>> by the hardware.
>>
>> Apologies for missing the changelog. To correct this, should I send a v3 
>> version with the changelog or resend v2?
> 
> Reply now with changelog. Your binding has some other unrelated and
> incorrect changes, which I do not understand.
> 
> Best regards,
> Krzysztof
> 

-- 
Troy Mitchell

Re: [PATCH v2 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Troy Mitchell]

On 2024/10/28 13:32, Troy Mitchell wrote:
> This patch introduces basic I2C support for the SpacemiT K1 SoC,
> utilizing interrupts for transfers.
> 
> The driver has been tested using i2c-tools on a Bananapi-F3 board,
> and basic I2C read/write operations have been confirmed to work.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> --
Change in V2:
 - Alphabetize Makefile and Kconfig
 - Change `.remove_new` to `.remove` in `struct platform_driver`
 - Change `dev_alert` to `dev_warn_ratelimited` in `spacemit_i2c_bus_reset`
 - Change `spacemit_i2c_read/write_reg` to `read/writel`
 - Change `spacemit_i2c_dt_match` to `spacemit_i2c_of_match`
 - Clean up code flow
 - Fix unnecessary line wraps
 - Move `spacemit_i2c_handle_err` to a suitable location
 - Use `PTR_ERR(i2c->base)` directly as the `dev_err_probe` parameter instead of
   the intermediate variable
-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Krzysztof Kozlowski]

On 04/11/2024 14:01, Troy Mitchell wrote:
> 
> On 2024/10/31 16:00, Krzysztof Kozlowski wrote:
>> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>>> and supports FIFO transmission.
>>>>>
>>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>>> ---
> Change in v2:
>  - Change the maxItems of reg from 1 to 2 in properties

Why?

>  - Change 'i2c' to 'I2C' in the commit message.
>  - Drop fifo-disable property
>  - Drop alias in dts example
>  - Move `unevaluatedProperties` after `required:` block

Rest look ok.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Troy Mitchell]

On 2024/11/4 22:48, Krzysztof Kozlowski wrote:
> On 04/11/2024 14:01, Troy Mitchell wrote:
>>
>> On 2024/10/31 16:00, Krzysztof Kozlowski wrote:
>>> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>>>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>>>> and supports FIFO transmission.
>>>>>>
>>>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>>>> ---
>> Change in v2:
>>  - Change the maxItems of reg from 1 to 2 in properties
> 
> Why?
I need the address and size. In v1, I wrote it as 1, and I got the make
dt_binding_check error.
> 
>>  - Change 'i2c' to 'I2C' in the commit message.
>>  - Drop fifo-disable property
>>  - Drop alias in dts example
>>  - Move `unevaluatedProperties` after `required:` block
> 
> Rest look ok.
> 
> Best regards,
> Krzysztof
> 

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Samuel Holland]

Hi Troy,

On 2024-11-04 7:14 PM, Troy Mitchell wrote:
> 
> On 2024/11/4 22:48, Krzysztof Kozlowski wrote:
>> On 04/11/2024 14:01, Troy Mitchell wrote:
>>>
>>> On 2024/10/31 16:00, Krzysztof Kozlowski wrote:
>>>> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>>>>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>>>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>>>>> and supports FIFO transmission.
>>>>>>>
>>>>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>>>>> ---
>>> Change in v2:
>>>  - Change the maxItems of reg from 1 to 2 in properties
>>
>> Why?
> I need the address and size. In v1, I wrote it as 1, and I got the make
> dt_binding_check error.

One "<address size>" element is just one item. maxItems > 1 would be for
hardware with multiple discontiguous register ranges: <address1 size1>,
<address2 size2>.

Regards,
Samuel

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Troy Mitchell]

On 2024/11/5 10:50, Samuel Holland wrote:
> One "<address size>" element is just one item. maxItems > 1 would be for
> hardware with multiple discontiguous register ranges: <address1 size1>,
> <address2 size2>.
got it.I will fix it in next version.
> 
> Regards,
> Samuel

-- 
Troy Mitchell

Re: [PATCH v2 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Troy Mitchell]

On 2024/10/28 13:32, Troy Mitchell wrote:
> This patch introduces basic I2C support for the SpacemiT K1 SoC,
> utilizing interrupts for transfers.
> 
> The driver has been tested using i2c-tools on a Bananapi-F3 board,
> and basic I2C read/write operations have been confirmed to work.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---
>  drivers/i2c/busses/Kconfig  |  18 +
>  drivers/i2c/busses/Makefile |   1 +
>  drivers/i2c/busses/i2c-k1.c | 658 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 677 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-k1.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 6b3ba7e5723a..38ebfd38dc41 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -779,6 +779,24 @@ config I2C_JZ4780
>  
>  	 If you don't know what to do here, say N.
>  
> +config I2C_K1
> +	tristate "Spacemit K1 I2C adapter"
> +	depends on HAS_IOMEM
I think I should retain the depends on from v1, like this:
depends on ARCH_SPACEMIT || COMPILE_TEST

Krzysztof mentioned that there is no such thing as ARCH_SPACEMIT, that's because
this patch depends on the basic DT from dlan:
https://lore.kernel.org/all/20240730-k1-01-basic-dt-v5-0-98263aae83be@gentoo.org/
> +	help
> +	  This option enables support for the I2C interface on the Spacemit K1
> +	  platform.
> +
> +	  If you enable this configuration, the kernel will include support for
> +	  the I2C adapter specific to the Spacemit K1 platform. This driver ca
> +	  be used to manage I2C bus transactions, which are necessary for
> +	  interfacing with I2C peripherals such as sensors, EEPROMs, and other
> +	  devices.
> +
> +	  This driver can also be compiled as a module. If you choose to build
> +	  it as a module, the resulting kernel module will be named `i2c-k1`.
> +	  Loading this module will enable the I2C functionality for the K1
> +	  platform dynamically, without requiring a rebuild of the kernel.
> +
>  config I2C_KEBA
>  	tristate "KEBA I2C controller support"
>  	depends on HAS_IOMEM

-- 
Troy Mitchell

Re: [PATCH v2 0/2] riscv: spacemit: add i2c support to K1 SoC

[Andi Shyti]

Hy Troy,

On Mon, Nov 04, 2024 at 08:23:23PM +0800, Troy Mitchell wrote:
> On 2024/10/31 19:43, Andi Shyti wrote:
> > Hi Tony,

Sorry, I misread your name :-/

> > On Mon, Oct 28, 2024 at 01:32:18PM +0800, Troy Mitchell wrote:
> >> Hi all,
> >>
> >> This patch implements I2C driver for the SpacemiT K1 SoC,
> >> providing basic support for I2C read/write communication which
> >> compatible with standard I2C bus specifications.
> >>
> >> In this version, the driver defaults to use fast-speed-mode and
> >> interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.
> >>
> >> The docs of I2C can be found here, in chapter 16.1 I2C [1]
> >>
> >> Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]
> >>
> >> Troy Mitchell (2):
> >>   dt-bindings: i2c: spacemit: add support for K1 SoC
> >>   i2c: spacemit: add support for SpacemiT K1 SoC
> > 
> > As Krzysztof has asked, please do provide the changelog, it's
> > important to track the progress of your series.
> I saw a compilation warning sent to me by the robot, and I've
> fixed the warning. Should I resend V2 with the changelog
> what I miss or send V3?

Please send a v3. When there are compilation issues, normally
patches are less keen to be reviewed.

You can add the changelog in the Patch 0/2 to avoid editing all
the .patch files.

Thanks,
Andi

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Troy Mitchell]

On 2024/11/2 11:48, Samuel Holland wrote:
> Hi Troy,
> 
> On 2024-10-28 12:32 AM, Troy Mitchell wrote:
>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>> and supports FIFO transmission.
>>
>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>> ---
>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>> new file mode 100644
>> index 000000000000..57af66f494e7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C controller embedded in SpacemiT's K1 SoC
>> +
>> +maintainers:
>> +  - Troy Mitchell <troymitchell988@gmail.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: spacemit,k1-i2c
>> +
>> +  reg:
>> +    maxItems: 2
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
> 
> Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
> REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
> looks to be standard across the peripherals in this SoC. Please be sure that the
> binding covers all resources needed to use this peripheral.RCPU stands for Real-time CPU, which is typically used for low power consumption
applications.
We should be using the APBC_TWSIx_CLK_RST register, but it's not listed in the
user manual. However, you can find this register referenced in the K1 clock patch:
https://lore.kernel.org/all/SEYPR01MB4221AA2CA9C91A695FEFA777D7602@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/

Also, to see how to enable the I2C clock in the device tree (note that the
spacemit,apb_clock property is unused in the driver), check out the example here:
https://gitee.com/bianbu-linux/linux-6.1/blob/bl-v1.0.y/arch/riscv/boot/dts/spacemit/k1-x.dtsi#L1048

> 
>> +
>> +  clock-frequency:
>> +    description:
>> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
>> +      modes are supported by hardware, possible values are 100000 and 400000.
>> +    enum: [100000, 400000]
> 
> This looks wrong. In the manual I see:
> 
> * Supports standard-mode operation up to 100 Kbps
> * Supports fast-mode operation up to 400Kbps
> * Supports high-speed mode (HS mode) slave operation up to 3.4Mbps(High-speed
> I2C only)
> * Supports high-speed mode (HS mode) master operation up to 3.3 Mbps (High-speed
> I2C only)
> 
> So even ignoring HS mode, 100 kHz and 400 kHz are only the maximums, not fixed
> frequencies.
okay. I will fix it in next version.
and should I keep to ignore high-speed mode here?
if not, how about this:

  clock-frequency:
    description:
      Desired I2C bus clock frequency in Hz.
      K1 supports standard, fast, high-speed modes, from 1 to 3300000.
    default: 100000
    minimum: 1
    maximum: 3300000

> 
> Regards,
> Samuel
> 
>> +    default: 100000
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c@d4010800 {
>> +        compatible = "spacemit,k1-i2c";
>> +        reg = <0x0 0xd4010800 0x0 0x38>;
>> +        interrupt-parent = <&plic>;
>> +        interrupts = <36>;
>> +        clocks = <&ccu 90>;
>> +        clock-frequency = <100000>;
>> +    };
>> +
>> +...
> 

-- 
Troy Mitchell

Re: [PATCH v2 0/2] riscv: spacemit: add i2c support to K1 SoC

[Troy Mitchell]

On 2024/11/5 22:21, Andi Shyti wrote:
> Hy Troy,
> 
> On Mon, Nov 04, 2024 at 08:23:23PM +0800, Troy Mitchell wrote:
>> On 2024/10/31 19:43, Andi Shyti wrote:
>>> Hi Tony,
> 
> Sorry, I misread your name :-/
It doesn't matter
> 
>>> On Mon, Oct 28, 2024 at 01:32:18PM +0800, Troy Mitchell wrote:
>>>> Hi all,
>>>>
>>>> This patch implements I2C driver for the SpacemiT K1 SoC,
>>>> providing basic support for I2C read/write communication which
>>>> compatible with standard I2C bus specifications.
>>>>
>>>> In this version, the driver defaults to use fast-speed-mode and
>>>> interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.
>>>>
>>>> The docs of I2C can be found here, in chapter 16.1 I2C [1]
>>>>
>>>> Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]
>>>>
>>>> Troy Mitchell (2):
>>>>   dt-bindings: i2c: spacemit: add support for K1 SoC
>>>>   i2c: spacemit: add support for SpacemiT K1 SoC
>>>
>>> As Krzysztof has asked, please do provide the changelog, it's
>>> important to track the progress of your series.
>> I saw a compilation warning sent to me by the robot, and I've
>> fixed the warning. Should I resend V2 with the changelog
>> what I miss or send V3?
> 
> Please send a v3. When there are compilation issues, normally
> patches are less keen to be reviewed.
> 
> You can add the changelog in the Patch 0/2 to avoid editing all
> the .patch files.
Thanks! I'll send v3 after I get a response from Samuel.
> 
> Thanks,
> Andi

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Samuel Holland]

Hi Troy,

On 2024-11-06 1:58 AM, Troy Mitchell wrote:
> On 2024/11/2 11:48, Samuel Holland wrote:
>> On 2024-10-28 12:32 AM, Troy Mitchell wrote:
>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>> and supports FIFO transmission.
>>>
>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>> ---
>>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>>>  1 file changed, 51 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>> new file mode 100644
>>> index 000000000000..57af66f494e7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>> @@ -0,0 +1,51 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: I2C controller embedded in SpacemiT's K1 SoC
>>> +
>>> +maintainers:
>>> +  - Troy Mitchell <troymitchell988@gmail.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: spacemit,k1-i2c
>>> +
>>> +  reg:
>>> +    maxItems: 2
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>
>> Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
>> REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
>> looks to be standard across the peripherals in this SoC. Please be sure that the
>> binding covers all resources needed to use this peripheral.
>
> RCPU stands for Real-time CPU, which is typically used for low power consumption
> applications.
> We should be using the APBC_TWSIx_CLK_RST register, but it's not listed in the
> user manual. However, you can find this register referenced in the K1 clock patch:
> https://lore.kernel.org/all/SEYPR01MB4221AA2CA9C91A695FEFA777D7602@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/

Ah, well that driver is missing most of the bus clocks. For example, from a
quick comparison with the manual, the driver includes sdh_axi_aclk, but misses
all of the PWM APB clocks at APBC_PWMx_CLK_RST bit 0.

If the clock gate exists in the hardware, even if it is enabled by default, it
needs to be modeled in the devicetree.

> Also, to see how to enable the I2C clock in the device tree (note that the
> spacemit,apb_clock property is unused in the driver), check out the example here:
> https://gitee.com/bianbu-linux/linux-6.1/blob/bl-v1.0.y/arch/riscv/boot/dts/spacemit/k1-x.dtsi#L1048

The devicetree describes the hardware, irrespective of which features the driver
may or may not use.

>>> +
>>> +  clock-frequency:
>>> +    description:
>>> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
>>> +      modes are supported by hardware, possible values are 100000 and 400000.
>>> +    enum: [100000, 400000]
>>
>> This looks wrong. In the manual I see:
>>
>> * Supports standard-mode operation up to 100 Kbps
>> * Supports fast-mode operation up to 400Kbps
>> * Supports high-speed mode (HS mode) slave operation up to 3.4Mbps(High-speed
>> I2C only)
>> * Supports high-speed mode (HS mode) master operation up to 3.3 Mbps (High-speed
>> I2C only)
>>
>> So even ignoring HS mode, 100 kHz and 400 kHz are only the maximums, not fixed
>> frequencies.
> okay. I will fix it in next version.
> and should I keep to ignore high-speed mode here?
> if not, how about this:
> 
>   clock-frequency:
>     description:
>       Desired I2C bus clock frequency in Hz.
>       K1 supports standard, fast, high-speed modes, from 1 to 3300000.
>     default: 100000
>     minimum: 1
>     maximum: 3300000

I don't know if high-speed mode should be included, since it requires some extra
negotiation to use. Assuming it should be, that looks reasonable.

Regards,
Samuel

>>
>> Regards,
>> Samuel
>>
>>> +    default: 100000
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clocks
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    i2c@d4010800 {
>>> +        compatible = "spacemit,k1-i2c";
>>> +        reg = <0x0 0xd4010800 0x0 0x38>;
>>> +        interrupt-parent = <&plic>;
>>> +        interrupts = <36>;
>>> +        clocks = <&ccu 90>;
>>> +        clock-frequency = <100000>;
>>> +    };
>>> +
>>> +...
>>
> 

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Troy Mitchell]

On 2024/11/7 08:29, Samuel Holland wrote:
> Hi Troy,
> 
> On 2024-11-06 1:58 AM, Troy Mitchell wrote:
>> On 2024/11/2 11:48, Samuel Holland wrote:
>>> On 2024-10-28 12:32 AM, Troy Mitchell wrote:
>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>> and supports FIFO transmission.
>>>>
>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>> ---
>>>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>>>>  1 file changed, 51 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>>> new file mode 100644
>>>> index 000000000000..57af66f494e7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>>> @@ -0,0 +1,51 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: I2C controller embedded in SpacemiT's K1 SoC
>>>> +
>>>> +maintainers:
>>>> +  - Troy Mitchell <troymitchell988@gmail.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: spacemit,k1-i2c
>>>> +
>>>> +  reg:
>>>> +    maxItems: 2
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>
>>> Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
>>> REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
>>> looks to be standard across the peripherals in this SoC. Please be sure that the
>>> binding covers all resources needed to use this peripheral.
>>
>> RCPU stands for Real-time CPU, which is typically used for low power consumption
>> applications.
>> We should be using the APBC_TWSIx_CLK_RST register, but it's not listed in the
>> user manual. However, you can find this register referenced in the K1 clock patch:
>> https://lore.kernel.org/all/SEYPR01MB4221AA2CA9C91A695FEFA777D7602@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/
> 
> Ah, well that driver is missing most of the bus clocks. For example, from a
> quick comparison with the manual, the driver includes sdh_axi_aclk, but misses
> all of the PWM APB clocks at APBC_PWMx_CLK_RST bit 0.
> 
> If the clock gate exists in the hardware, even if it is enabled by default, it
> needs to be modeled in the devicetree.
I think you mean `APBCSCR`? It will keep enable all time.Just a difference in
frequency.

Should I add it in clockc property? If yes, how about this:
    clocks:
        maxItems: 1

and in dts example:
    i2c@d4010800 {
	...
        clocks = <&clk_apbc>, <&ccu 90>;
	clock-names = "clk_apbc", "clk_twsi";
        ...
    };
But one thing is, apbc_twsi is the sub-clock of clk_apbc, is this a duplicate
listing?
> 
>> Also, to see how to enable the I2C clock in the device tree (note that the
>> spacemit,apb_clock property is unused in the driver), check out the example here:
>> https://gitee.com/bianbu-linux/linux-6.1/blob/bl-v1.0.y/arch/riscv/boot/dts/spacemit/k1-x.dtsi#L1048
> 
> The devicetree describes the hardware, irrespective of which features the driver
> may or may not use.
> 
>>>> +
>>>> +  clock-frequency:
>>>> +    description:
>>>> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
>>>> +      modes are supported by hardware, possible values are 100000 and 400000.
>>>> +    enum: [100000, 400000]
>>>
>>> This looks wrong. In the manual I see:
>>>
>>> * Supports standard-mode operation up to 100 Kbps
>>> * Supports fast-mode operation up to 400Kbps
>>> * Supports high-speed mode (HS mode) slave operation up to 3.4Mbps(High-speed
>>> I2C only)
>>> * Supports high-speed mode (HS mode) master operation up to 3.3 Mbps (High-speed
>>> I2C only)
>>>
>>> So even ignoring HS mode, 100 kHz and 400 kHz are only the maximums, not fixed
>>> frequencies.
>> okay. I will fix it in next version.
>> and should I keep to ignore high-speed mode here?
>> if not, how about this:
>>
>>   clock-frequency:
>>     description:
>>       Desired I2C bus clock frequency in Hz.
>>       K1 supports standard, fast, high-speed modes, from 1 to 3300000.
>>     default: 100000
>>     minimum: 1
>>     maximum: 3300000
> 
> I don't know if high-speed mode should be included, since it requires some extra
> negotiation to use. Assuming it should be, that looks reasonable.
thanks.
> 
> Regards,
> Samuel
> 
>>>
>>> Regards,
>>> Samuel
>>>
>>>> +    default: 100000
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +  - clocks
>>>> +
>>>> +unevaluatedProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    i2c@d4010800 {
>>>> +        compatible = "spacemit,k1-i2c";
>>>> +        reg = <0x0 0xd4010800 0x0 0x38>;
>>>> +        interrupt-parent = <&plic>;
>>>> +        interrupts = <36>;
>>>> +        clocks = <&ccu 90>;
>>>> +        clock-frequency = <100000>;
>>>> +    };
>>>> +
>>>> +...
>>>
>>
> 

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Haylen Chu]

On Wed, Nov 06, 2024 at 06:29:56PM -0600, Samuel Holland wrote:
Hi Samuel,

I'm working on the clock driver for Spacemit K1.

> On 2024-11-06 1:58 AM, Troy Mitchell wrote:
> > On 2024/11/2 11:48, Samuel Holland wrote:
> >> On 2024-10-28 12:32 AM, Troy Mitchell wrote:
> >>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> >>> and supports FIFO transmission.
> >>>
> >>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> >>> ---
> >>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
> >>>  1 file changed, 51 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >>> new file mode 100644
> >>> index 000000000000..57af66f494e7
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >>> @@ -0,0 +1,51 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: I2C controller embedded in SpacemiT's K1 SoC
> >>> +
> >>> +maintainers:
> >>> +  - Troy Mitchell <troymitchell988@gmail.com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: spacemit,k1-i2c
> >>> +
> >>> +  reg:
> >>> +    maxItems: 2
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>
> >> Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
> >> REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
> >> looks to be standard across the peripherals in this SoC. Please be sure that the
> >> binding covers all resources needed to use this peripheral.
> >
> > RCPU stands for Real-time CPU, which is typically used for low power consumption
> > applications.
> > We should be using the APBC_TWSIx_CLK_RST register, but it's not listed in the
> > user manual.

The vendor missed the register definition of APBC_TWSIx_CLK_RST in the
documentation. There'll be an update soon.

> > However, you can find this register referenced in the K1 clock patch:
> > https://lore.kernel.org/all/SEYPR01MB4221AA2CA9C91A695FEFA777D7602@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/
> 
> Ah, well that driver is missing most of the bus clocks. For example, from a
> quick comparison with the manual, the driver includes sdh_axi_aclk, but misses
> all of the PWM APB clocks at APBC_PWMx_CLK_RST bit 0.

Thanks for pointing this out. Indeeded, the v2 of clock driver is
missing some bus clocks. I'm fixing them and working for a v3.

As for the I2C controller, I've confirmed with the vendor that both bus
and function clocks are required for normal operation. This applies for
all I2C controllers on the SoC, regardless of the region it belongs to
(RCPU/APBC).

> 
> If the clock gate exists in the hardware, even if it is enabled by default, it
> needs to be modeled in the devicetree.
> 
> > Also, to see how to enable the I2C clock in the device tree (note that the
> > spacemit,apb_clock property is unused in the driver), check out the example here:
> > https://gitee.com/bianbu-linux/linux-6.1/blob/bl-v1.0.y/arch/riscv/boot/dts/spacemit/k1-x.dtsi#L1048
> 
> The devicetree describes the hardware, irrespective of which features the driver
> may or may not use.

Best regards,
Haylen Chu