* [PATCH v6 0/2] riscv: spacemit: add i2c support to K1 SoC
@ 2025-03-07 13:13 Troy Mitchell
2025-03-07 13:13 ` [PATCH v6 1/2] dt-bindings: i2c: spacemit: add support for " Troy Mitchell
2025-03-07 13:13 ` [PATCH v6 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell
0 siblings, 2 replies; 6+ messages in thread
From: Troy Mitchell @ 2025-03-07 13:13 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Yixun Lan, Troy Mitchell
Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit,
Conor Dooley
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]
---
Change in v6:
- Patch #1:
- Give a brief clock description and a clear clock name
- Changed the clock numbers in the example to match those
in the mainline clock driver
- Patch #2:
- Fix incorrect comments in macro definitions
- Fix code style
- Merge `spacemit_i2c_xfer_core` into `spacemit_i2c_xfer`
- Remove `reset` and `enable_irq` before each transfer
and place `reset` in the probe function.
- SPACEMIT_I2C_GET_ERR -> SPACEMIT_I2C_SR_ERR
- spacemit_i2c_recover_bus_busy -> spacemit_i2c_wait_bus_busy
- Support standard mode
Link to v5:
https://lore.kernel.org/all/20250303-k1-i2c-master-v5-0-21dfc7adfe37@gmail.com/
Change in v5:
- Patch #1:
- Add `clock-names` property
- Modify the clock property into two
- Patch #2:
- Enable the APB clock
- Fix comment and code styles
- Fix typo and drop unnecessary description in Kconfig
- Prefix all macro definitions with SPACEMIT_
- Rename `spacemit_i2c_bus_reset` to `spacemit_i2c_conditionally_reset_bus`
- Remove all `unlikely` and `likely`
- Remove unused register and bit macros
- Remove the "err" field, as it only contains a subset of the status field
- Retrieve `clock-frequency` from the device tree instead of using a macro
- Use a local variable to track the current message
- Use `i2c->read` to represent read and write statuses instead of `i2c->dir`
Link to v4:
https://lore.kernel.org/all/20241125-k1-i2c-master-v4-0-0f3d5886336b@gmail.com/
Change in v4:
- Patch #1:
- Change the default value of clock-frequency from 100000 to
400000. This is to correspond to the driver's default value.
- Drop the minimum of clock-frequency
- Modify the description of clock-frequency
- Patch #2:
- Drop the `inline` qualifier from the `spacemit_i2c_xfer_core` function
- Drop the initialization of `ret` to 0 in `spacemit_i2c_xfer_core` function
- Drop useless wrap
Link to v3:
https://lore.kernel.org/all/20241112-k1-i2c-master-v3-0-5005b70dc208@gmail.com/
Change in v3:
- Patch #1:
- Change the maxItems of reg from 2 to 1 in properties
- Modify reg in dts example
- Changed the enum selection for clock-frequency to a range,
setting a minimum value of 1 and a maximum value of 3,300,000.
- Patch #2:
- Drop unused judgement in `spacemit_i2c_xfer_msg`
- Fix the dangling else warning in `spacemit_i2c_is_last_msg`
- Fix the error check for `i2c->base`
- Modify Kconfig dependencies
Link to v2:
https://lore.kernel.org/all/20241028053220.346283-1-TroyMitchell988@gmail.com/
Change in v2:
- Patch #1:
- 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
- Patch #2:
- 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
- Modify Kconfig dependencies
- Use `PTR_ERR(i2c->base)` directly as the `dev_err_probe` parameter instead of
the intermediate variable
Link to v1:
https://lore.kernel.org/all/20241015075134.1449458-1-TroyMitchell988@gmail.com/
---
Troy Mitchell (2):
dt-bindings: i2c: spacemit: add support for K1 SoC
i2c: spacemit: add support for SpacemiT K1 SoC
.../devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 61 +++
drivers/i2c/busses/Kconfig | 17 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-k1.c | 601 +++++++++++++++++++++
4 files changed, 680 insertions(+)
---
base-commit: 8e929cb546ee42c9a61d24fae60605e9e3192354
change-id: 20241031-k1-i2c-master-fe7f7b0dce93
prerequisite-change-id: 20240626-k1-01-basic-dt-1aa31eeebcd2:v5
prerequisite-patch-id: 47dcf6861f7d434d25855b379e6d7ef4ce369c9c
prerequisite-patch-id: 77787fe82911923aff15ccf565e8fa451538c3a6
prerequisite-patch-id: b0bdb1742d96c5738f05262c3b0059102761390b
prerequisite-patch-id: 3927d39d8d77e35d5bfe53d9950da574ff8f2054
prerequisite-patch-id: a98039136a4796252a6029e474f03906f2541643
prerequisite-patch-id: c95f6dc0547a2a63a76e3cba0cf5c623b212b4e6
prerequisite-patch-id: 66e750e438ee959ddc2a6f0650814a2d8c989139
prerequisite-patch-id: 29a0fd8c36c1a4340f0d0b68a4c34d2b8abfb1ab
prerequisite-patch-id: 0bdfff661c33c380d1cf00a6c68688e05f88c0b3
prerequisite-patch-id: 99f15718e0bfbb7ed1a96dfa19f35841b004dae9
Best regards,
--
Troy Mitchell <troymitchell988@gmail.com>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v6 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC
2025-03-07 13:13 [PATCH v6 0/2] riscv: spacemit: add i2c support to K1 SoC Troy Mitchell
@ 2025-03-07 13:13 ` Troy Mitchell
2025-03-07 13:13 ` [PATCH v6 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell
1 sibling, 0 replies; 6+ messages in thread
From: Troy Mitchell @ 2025-03-07 13:13 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Yixun Lan, Troy Mitchell
Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit,
Conor Dooley
The I2C of K1 supports fast-speed-mode and high-speed-mode,
and supports FIFO transmission.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
---
.../devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 61 ++++++++++++++++++++++
1 file changed, 61 insertions(+)
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 0000000000000000000000000000000000000000..3d6aefb0d0f185ba64e414ac7f5b96cd18659fd3
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
@@ -0,0 +1,61 @@
+# 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: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: I2C Functional Clock
+ - description: APB Bus Clock
+
+ clock-names:
+ items:
+ - const: func
+ - const: bus
+
+ clock-frequency:
+ description: |
+ K1 support three different modes which running different frequencies
+ standard speed mode: up to 100000 (100Hz)
+ fast speed mode : up to 400000 (400Hz)
+ high speed mode : up to 3300000 (3.3Mhz)
+ default: 400000
+ maximum: 3300000
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ i2c@d4010800 {
+ compatible = "spacemit,k1-i2c";
+ reg = <0xd4010800 0x38>;
+ interrupt-parent = <&plic>;
+ interrupts = <36>;
+ clocks =<&ccu 32>, <&ccu 84>;
+ clock-names = "func", "bus";
+ clock-frequency = <100000>;
+ };
+
+...
--
2.34.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v6 2/2] i2c: spacemit: add support for SpacemiT K1 SoC
2025-03-07 13:13 [PATCH v6 0/2] riscv: spacemit: add i2c support to K1 SoC Troy Mitchell
2025-03-07 13:13 ` [PATCH v6 1/2] dt-bindings: i2c: spacemit: add support for " Troy Mitchell
@ 2025-03-07 13:13 ` Troy Mitchell
2025-03-12 1:46 ` Alex Elder
1 sibling, 1 reply; 6+ messages in thread
From: Troy Mitchell @ 2025-03-07 13:13 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Yixun Lan, Troy Mitchell
Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit
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 | 17 ++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-k1.c | 601 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 619 insertions(+)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fc438f4457713d5559d163840a7b11e8cdbb8f58..12d0e99566d8625aa374279956b71a4034ded4ac 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -783,6 +783,23 @@ config I2C_JZ4780
If you don't know what to do here, say N.
+config I2C_K1
+ tristate "Spacemit K1 I2C adapter"
+ depends on ARCH_SPACEMIT || COMPILE_TEST
+ depends on OF
+ 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 can
+ 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 built as a module. If so, the
+ module will be called `i2c-k1`.
+
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 1c2a4510abe44a689dfe67d2d64cf0cf3434f510..c1252e2b779e2e47492d66179b442f2202ec0416 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -74,6 +74,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 0000000000000000000000000000000000000000..1ef98a303fb14ce10d283225ce0617a0b9f405f7
--- /dev/null
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -0,0 +1,601 @@
+// 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 SPACEMIT_ICR 0x0 /* Control Register */
+#define SPACEMIT_ISR 0x4 /* Status Register */
+#define SPACEMIT_IDBR 0xc /* Data Buffer Register */
+#define SPACEMIT_IBMR 0x1c /* Bus monitor register */
+
+/* SPACEMIT_ICR register fields */
+#define SPACEMIT_CR_START BIT(0) /* start bit */
+#define SPACEMIT_CR_STOP BIT(1) /* stop bit */
+#define SPACEMIT_CR_ACKNAK BIT(2) /* send ACK(0) or NAK(1) */
+#define SPACEMIT_CR_TB BIT(3) /* transfer byte bit */
+/* Bits 4-7 are reserved */
+#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_SCLE BIT(13) /* master clock enable */
+#define SPACEMIT_CR_IUE BIT(14) /* unit enable */
+/* Bits 15-17 are reserved */
+#define SPACEMIT_CR_ALDIE BIT(18) /* enable arbitration interrupt */
+#define SPACEMIT_CR_DTEIE BIT(19) /* enable tx interrupts */
+#define SPACEMIT_CR_DRFIE BIT(20) /* enable rx interrupts */
+#define SPACEMIT_CR_GCD BIT(21) /* general call disable */
+#define SPACEMIT_CR_BEIE BIT(22) /* enable bus error ints */
+/* Bits 23-24 are reserved */
+#define SPACEMIT_CR_MSDIE BIT(25) /* master STOP detected int enable */
+#define SPACEMIT_CR_MSDE BIT(26) /* master STOP detected enable */
+#define SPACEMIT_CR_TXDONEIE BIT(27) /* transaction done int enable */
+#define SPACEMIT_CR_TXEIE BIT(28) /* transmit FIFO empty int enable */
+#define SPACEMIT_CR_RXHFIE BIT(29) /* receive FIFO half-full int enable */
+#define SPACEMIT_CR_RXFIE BIT(30) /* receive FIFO full int enable */
+#define SPACEMIT_CR_RXOVIE BIT(31) /* receive FIFO overrun int enable */
+
+#define SPACEMIT_I2C_INT_CTRL_MASK (SPACEMIT_CR_ALDIE | SPACEMIT_CR_DTEIE | \
+ SPACEMIT_CR_DRFIE | SPACEMIT_CR_BEIE | \
+ SPACEMIT_CR_TXDONEIE | SPACEMIT_CR_TXEIE | \
+ SPACEMIT_CR_RXHFIE | SPACEMIT_CR_RXFIE | \
+ SPACEMIT_CR_RXOVIE | SPACEMIT_CR_MSDIE)
+
+/* SPACEMIT_ISR register fields */
+/* Bits 0-13 are reserved */
+#define SPACEMIT_SR_ACKNAK BIT(14) /* ACK/NACK status */
+#define SPACEMIT_SR_UB BIT(15) /* unit busy */
+#define SPACEMIT_SR_IBB BIT(16) /* i2c bus busy */
+#define SPACEMIT_SR_EBB BIT(17) /* early bus busy */
+#define SPACEMIT_SR_ALD BIT(18) /* arbitration loss detected */
+#define SPACEMIT_SR_ITE BIT(19) /* tx buffer empty */
+#define SPACEMIT_SR_IRF BIT(20) /* rx buffer full */
+#define SPACEMIT_SR_GCAD BIT(21) /* general call address detected */
+#define SPACEMIT_SR_BED BIT(22) /* bus error no ACK/NAK */
+#define SPACEMIT_SR_SAD BIT(23) /* slave address detected */
+#define SPACEMIT_SR_SSD BIT(24) /* slave stop detected */
+/* Bit 25 is reserved */
+#define SPACEMIT_SR_MSD BIT(26) /* master stop detected */
+#define SPACEMIT_SR_TXDONE BIT(27) /* transaction done */
+#define SPACEMIT_SR_TXE BIT(28) /* tx FIFO empty */
+#define SPACEMIT_SR_RXHF BIT(29) /* rx FIFO half-full */
+#define SPACEMIT_SR_RXF BIT(30) /* rx FIFO full */
+#define SPACEMIT_SR_RXOV BIT(31) /* RX FIFO overrun */
+
+#define SPACEMIT_I2C_INT_STATUS_MASK (SPACEMIT_SR_RXOV | SPACEMIT_SR_RXF | SPACEMIT_SR_RXHF | \
+ SPACEMIT_SR_TXE | SPACEMIT_SR_TXDONE | SPACEMIT_SR_MSD | \
+ SPACEMIT_SR_SSD | SPACEMIT_SR_SAD | SPACEMIT_SR_BED | \
+ SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \
+ SPACEMIT_SR_ALD)
+
+/* SPACEMIT_IBMR register fields */
+#define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
+#define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
+
+/* i2c bus recover timeout: us */
+#define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000
+
+#define SPACEMIT_SR_ERR (SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD)
+
+#define SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ 100000
+#define SPACEMIT_I2C_MAX_FAST_MODE_FREQ 400000
+
+enum spacemit_i2c_state {
+ STATE_IDLE,
+ STATE_START,
+ STATE_READ,
+ STATE_WRITE,
+};
+
+/* i2c-spacemit driver's main struct */
+struct spacemit_i2c_dev {
+ struct device *dev;
+ struct i2c_adapter adapt;
+
+ /* hardware resources */
+ void __iomem *base;
+ int irq;
+ u32 clock_freq;
+
+ struct i2c_msg *msgs;
+ int msg_num;
+
+ /* 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;
+ bool read;
+ struct completion complete;
+ u32 status;
+};
+
+static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
+{
+ u32 val;
+
+ val = readl(i2c->base + SPACEMIT_ICR);
+ val |= SPACEMIT_CR_IUE;
+ writel(val, i2c->base + SPACEMIT_ICR);
+}
+
+static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
+{
+ u32 val;
+
+ val = readl(i2c->base + SPACEMIT_ICR);
+ val &= ~SPACEMIT_CR_IUE;
+ writel(val, i2c->base + SPACEMIT_ICR);
+}
+
+static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
+{
+ writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
+ udelay(5);
+ writel(0, i2c->base + SPACEMIT_ICR);
+}
+
+static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
+{
+ dev_dbg(i2c->dev, "i2c error status: 0x%08x\n", i2c->status);
+
+ if (i2c->status & (SPACEMIT_SR_BED | SPACEMIT_SR_ALD)) {
+ spacemit_i2c_reset(i2c);
+ return -EAGAIN;
+ }
+
+ return i2c->status & SPACEMIT_SR_ACKNAK ? -ENXIO : -EIO;
+}
+
+static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
+{
+ u32 status;
+
+ /* if bus is locked, reset unit. 0: locked */
+ status = readl(i2c->base + SPACEMIT_IBMR);
+ if ((status & SPACEMIT_BMR_SDA) && (status & SPACEMIT_BMR_SCL))
+ return;
+
+ spacemit_i2c_reset(i2c);
+ usleep_range(10, 20);
+
+ /* check scl status again */
+ status = readl(i2c->base + SPACEMIT_IBMR);
+ if (!(status & SPACEMIT_BMR_SCL))
+ dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
+}
+
+static int spacemit_i2c_wait_bus_busy(struct spacemit_i2c_dev *i2c)
+{
+ int ret;
+ u32 val;
+
+ val = readl(i2c->base + SPACEMIT_ISR);
+ 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 (ret)
+ spacemit_i2c_reset(i2c);
+
+ return ret;
+}
+
+static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
+{
+ /* in case bus is not released after transfer completes */
+ if (readl(i2c->base + SPACEMIT_ISR) & SPACEMIT_SR_EBB) {
+ spacemit_i2c_conditionally_reset_bus(i2c);
+ usleep_range(90, 150);
+ }
+}
+
+static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
+{
+ u32 val;
+
+ /*
+ * Unmask interrupt bits for all xfer mode:
+ * bus error, arbitration loss detected.
+ * For transaction complete signal, we use master stop
+ * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
+ */
+ val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
+
+ /*
+ * Unmask interrupt bits for interrupt xfer mode:
+ * DBR rx full.
+ * For tx empty interrupt SPACEMIT_CR_DTEIE, we only
+ * need to enable when trigger byte transfer to start
+ * data sending.
+ */
+ val |= SPACEMIT_CR_DRFIE;
+
+ if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
+ val |= SPACEMIT_CR_MODE_FAST;
+
+ /* disable response to general call */
+ val |= SPACEMIT_CR_GCD;
+
+ /* enable SCL clock output */
+ val |= SPACEMIT_CR_SCLE;
+
+ /* enable master stop detected */
+ val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
+
+ writel(val, i2c->base + SPACEMIT_ICR);
+}
+
+static inline void
+spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
+{
+ writel(mask & SPACEMIT_I2C_INT_STATUS_MASK, i2c->base + SPACEMIT_ISR);
+}
+
+static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
+{
+ u32 slave_addr_rw, val;
+ struct i2c_msg *cur_msg = i2c->msgs + i2c->msg_idx;
+
+ i2c->read = !!(cur_msg->flags & I2C_M_RD);
+
+ i2c->state = STATE_START;
+
+ slave_addr_rw = (cur_msg->addr & 0x7f) << 1;
+ if (cur_msg->flags & I2C_M_RD)
+ slave_addr_rw |= 1;
+
+ writel(slave_addr_rw, i2c->base + SPACEMIT_IDBR);
+
+ /* send start pulse */
+ val = readl(i2c->base + SPACEMIT_ICR);
+ val &= ~SPACEMIT_CR_STOP;
+ val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
+ 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;
+
+ for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
+ i2c->msg_buf = (i2c->msgs + i2c->msg_idx)->buf;
+ i2c->status = 0;
+ i2c->unprocessed = (i2c->msgs + i2c->msg_idx)->len;
+
+ reinit_completion(&i2c->complete);
+
+ spacemit_i2c_start(i2c);
+
+ time_left = wait_for_completion_timeout(&i2c->complete,
+ i2c->adapt.timeout);
+ if (time_left == 0) {
+ dev_err(i2c->dev, "msg completion timeout\n");
+ spacemit_i2c_conditionally_reset_bus(i2c);
+ spacemit_i2c_reset(i2c);
+ return -ETIMEDOUT;
+ }
+
+ if (i2c->status & SPACEMIT_SR_ERR)
+ return spacemit_i2c_handle_err(i2c);
+ }
+
+ return 0;
+}
+
+static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
+{
+ if (i2c->msg_idx != i2c->msg_num - 1)
+ return false;
+
+ if (i2c->read)
+ return i2c->unprocessed == 1;
+
+ return !i2c->unprocessed;
+}
+
+static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
+{
+ /* if transfer completes, SPACEMIT_ISR will handle it */
+ if (i2c->status & SPACEMIT_SR_MSD)
+ return;
+
+ if (i2c->unprocessed) {
+ writel(*i2c->msg_buf++, i2c->base + SPACEMIT_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 + SPACEMIT_IDBR);
+ i2c->unprocessed--;
+ }
+
+ /* if transfer completes, SPACEMIT_ISR will handle it */
+ if (i2c->status & (SPACEMIT_SR_MSD | SPACEMIT_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)
+{
+ i2c->state = i2c->read ? STATE_READ : STATE_WRITE;
+ if (i2c->state == STATE_WRITE)
+ spacemit_i2c_handle_write(i2c);
+}
+
+static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
+{
+ u32 val;
+
+ /*
+ * send transaction complete signal:
+ * error happens, detect master stop
+ */
+ if (!(i2c->status & (SPACEMIT_SR_ERR | SPACEMIT_SR_MSD)))
+ return;
+
+ /*
+ * 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 + SPACEMIT_ICR);
+ val &= ~SPACEMIT_I2C_INT_CTRL_MASK;
+ writel(val, i2c->base + SPACEMIT_ICR);
+
+ spacemit_i2c_clear_int_status(i2c, SPACEMIT_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 + SPACEMIT_ISR);
+ if (!status)
+ return IRQ_HANDLED;
+
+ i2c->status = status;
+
+ spacemit_i2c_clear_int_status(i2c, status);
+
+ if (i2c->status & SPACEMIT_SR_ERR)
+ goto err_out;
+
+ 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 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 |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
+ writel(val, i2c->base + SPACEMIT_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;
+
+ while (idx < i2c->msg_num) {
+ cnt += (i2c->msgs + idx)->len + 1;
+ idx++;
+ }
+
+ /*
+ * 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 / i2c->clock_freq;
+
+ i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 2) / i2c->msg_num;
+}
+
+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;
+
+ spacemit_i2c_calc_timeout(i2c);
+
+ spacemit_i2c_init(i2c);
+
+ spacemit_i2c_enable(i2c);
+
+ ret = spacemit_i2c_wait_bus_busy(i2c);
+ if (!ret)
+ spacemit_i2c_xfer_msg(i2c);
+
+ if (ret < 0)
+ dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);
+ else if (ret)
+ spacemit_i2c_check_bus_release(i2c);
+
+ spacemit_i2c_disable(i2c);
+
+ if (ret == -ETIMEDOUT || ret == -EAGAIN)
+ dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%lx\n",
+ ret, i2c->status & SPACEMIT_SR_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 clk *clk;
+ struct device *dev = &pdev->dev;
+ struct device_node *of_node = pdev->dev.of_node;
+ struct spacemit_i2c_dev *i2c;
+ int ret;
+
+ i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to read clock-frequency property");
+
+ /* For now, this driver doesn't support high-speed. */
+ if (i2c->clock_freq < 1 || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
+ dev_warn(dev, "unsupport clock frequency: %d, default: %d",
+ i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
+ i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
+ } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
+ i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
+ }
+
+ i2c->dev = &pdev->dev;
+
+ i2c->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(i2c->base))
+ return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap");
+
+ i2c->irq = platform_get_irq(pdev, 0);
+ if (i2c->irq < 0)
+ return dev_err_probe(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(dev, ret, "failed to request irq");
+
+ clk = devm_clk_get_enabled(dev, "func");
+ if (IS_ERR(clk))
+ return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
+
+ clk = devm_clk_get_enabled(dev, "bus");
+ if (IS_ERR(clk))
+ return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
+
+ spacemit_i2c_reset(i2c);
+
+ 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);
+
+ platform_set_drvdata(pdev, i2c);
+
+ ret = i2c_add_numbered_adapter(&i2c->adapt);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
+
+ 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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v6 2/2] i2c: spacemit: add support for SpacemiT K1 SoC
2025-03-07 13:13 ` [PATCH v6 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell
@ 2025-03-12 1:46 ` Alex Elder
2025-03-13 0:10 ` Andi Shyti
0 siblings, 1 reply; 6+ messages in thread
From: Alex Elder @ 2025-03-12 1:46 UTC (permalink / raw)
To: Troy Mitchell, Andi Shyti, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Yixun Lan
Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit,
Guodong Xu
On 3/7/25 7:13 AM, 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>
I have a bunch of really small suggestions. Please consider
them, but whatever you choose to do this looks good to me.
Reviewed-by: Alex Elder <elder@riscstar.com>
> ---
> drivers/i2c/busses/Kconfig | 17 ++
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-k1.c | 601 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 619 insertions(+)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index fc438f4457713d5559d163840a7b11e8cdbb8f58..12d0e99566d8625aa374279956b71a4034ded4ac 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -783,6 +783,23 @@ config I2C_JZ4780
>
> If you don't know what to do here, say N.
>
> +config I2C_K1
> + tristate "Spacemit K1 I2C adapter"
> + depends on ARCH_SPACEMIT || COMPILE_TEST
> + depends on OF
> + 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 can
> + be used to manage I2C bus transactions, which are necessary for
> + interfacing with I2C peripherals such as sensors, EEPROMs, and other
> + devices.
Does SpacemiT prefer to have the T at the end of their name
capitalized?
> +
> + This driver can also be built as a module. If so, the
> + module will be called `i2c-k1`.
> +
> 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 1c2a4510abe44a689dfe67d2d64cf0cf3434f510..c1252e2b779e2e47492d66179b442f2202ec0416 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -74,6 +74,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 0000000000000000000000000000000000000000..1ef98a303fb14ce10d283225ce0617a0b9f405f7
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -0,0 +1,601 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
Maybe update your copyright date(s) to include this year before
your final version of the patch.
> + */
> +
> + #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 SPACEMIT_ICR 0x0 /* Control Register */
s/Register/register/
> +#define SPACEMIT_ISR 0x4 /* Status Register */
s/Register/register/
> +#define SPACEMIT_IDBR 0xc /* Data Buffer Register */
s/Buffer Re/buffer re/
> +#define SPACEMIT_IBMR 0x1c /* Bus monitor register */
> +
> +/* SPACEMIT_ICR register fields */
> +#define SPACEMIT_CR_START BIT(0) /* start bit */
> +#define SPACEMIT_CR_STOP BIT(1) /* stop bit */
> +#define SPACEMIT_CR_ACKNAK BIT(2) /* send ACK(0) or NAK(1) */
> +#define SPACEMIT_CR_TB BIT(3) /* transfer byte bit */
> +/* Bits 4-7 are reserved */
> +#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_SCLE BIT(13) /* master clock enable */
> +#define SPACEMIT_CR_IUE BIT(14) /* unit enable */
> +/* Bits 15-17 are reserved */
> +#define SPACEMIT_CR_ALDIE BIT(18) /* enable arbitration interrupt */
> +#define SPACEMIT_CR_DTEIE BIT(19) /* enable tx interrupts */
> +#define SPACEMIT_CR_DRFIE BIT(20) /* enable rx interrupts */
> +#define SPACEMIT_CR_GCD BIT(21) /* general call disable */
> +#define SPACEMIT_CR_BEIE BIT(22) /* enable bus error ints */
> +/* Bits 23-24 are reserved */
> +#define SPACEMIT_CR_MSDIE BIT(25) /* master STOP detected int enable */
> +#define SPACEMIT_CR_MSDE BIT(26) /* master STOP detected enable */
> +#define SPACEMIT_CR_TXDONEIE BIT(27) /* transaction done int enable */
> +#define SPACEMIT_CR_TXEIE BIT(28) /* transmit FIFO empty int enable */
> +#define SPACEMIT_CR_RXHFIE BIT(29) /* receive FIFO half-full int enable */
> +#define SPACEMIT_CR_RXFIE BIT(30) /* receive FIFO full int enable */
> +#define SPACEMIT_CR_RXOVIE BIT(31) /* receive FIFO overrun int enable */
> +
> +#define SPACEMIT_I2C_INT_CTRL_MASK (SPACEMIT_CR_ALDIE | SPACEMIT_CR_DTEIE | \
> + SPACEMIT_CR_DRFIE | SPACEMIT_CR_BEIE | \
> + SPACEMIT_CR_TXDONEIE | SPACEMIT_CR_TXEIE | \
> + SPACEMIT_CR_RXHFIE | SPACEMIT_CR_RXFIE | \
> + SPACEMIT_CR_RXOVIE | SPACEMIT_CR_MSDIE)
> +
> +/* SPACEMIT_ISR register fields */
> +/* Bits 0-13 are reserved */
> +#define SPACEMIT_SR_ACKNAK BIT(14) /* ACK/NACK status */
> +#define SPACEMIT_SR_UB BIT(15) /* unit busy */
> +#define SPACEMIT_SR_IBB BIT(16) /* i2c bus busy */
> +#define SPACEMIT_SR_EBB BIT(17) /* early bus busy */
> +#define SPACEMIT_SR_ALD BIT(18) /* arbitration loss detected */
> +#define SPACEMIT_SR_ITE BIT(19) /* tx buffer empty */
> +#define SPACEMIT_SR_IRF BIT(20) /* rx buffer full */
> +#define SPACEMIT_SR_GCAD BIT(21) /* general call address detected */
> +#define SPACEMIT_SR_BED BIT(22) /* bus error no ACK/NAK */
> +#define SPACEMIT_SR_SAD BIT(23) /* slave address detected */
> +#define SPACEMIT_SR_SSD BIT(24) /* slave stop detected */
> +/* Bit 25 is reserved */
> +#define SPACEMIT_SR_MSD BIT(26) /* master stop detected */
> +#define SPACEMIT_SR_TXDONE BIT(27) /* transaction done */
> +#define SPACEMIT_SR_TXE BIT(28) /* tx FIFO empty */
> +#define SPACEMIT_SR_RXHF BIT(29) /* rx FIFO half-full */
> +#define SPACEMIT_SR_RXF BIT(30) /* rx FIFO full */
> +#define SPACEMIT_SR_RXOV BIT(31) /* RX FIFO overrun */
s/RX FIFO/rx FIFO/
Or preferably, use "TX" and "RX" an all the comments (and anywhere
else you use "tx" and "rx" in the file).
> +
> +#define SPACEMIT_I2C_INT_STATUS_MASK (SPACEMIT_SR_RXOV | SPACEMIT_SR_RXF | SPACEMIT_SR_RXHF | \
> + SPACEMIT_SR_TXE | SPACEMIT_SR_TXDONE | SPACEMIT_SR_MSD | \
> + SPACEMIT_SR_SSD | SPACEMIT_SR_SAD | SPACEMIT_SR_BED | \
> + SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \
> + SPACEMIT_SR_ALD)
> +
> +/* SPACEMIT_IBMR register fields */
> +#define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
> +#define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
> +
> +/* i2c bus recover timeout: us */
> +#define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000
Align the 100000 above with the frequency values below.
> +
> +#define SPACEMIT_SR_ERR (SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD)
> +
Maybe it's obvious but you could comment that the unit
for these values is Hz.
> +#define SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ 100000
> +#define SPACEMIT_I2C_MAX_FAST_MODE_FREQ 400000
> +
> +enum spacemit_i2c_state {
> + STATE_IDLE,
> + STATE_START,
> + STATE_READ,
> + STATE_WRITE,
> +};
> +
> +/* i2c-spacemit driver's main struct */
> +struct spacemit_i2c_dev {
> + struct device *dev;
> + struct i2c_adapter adapt;
> +
> + /* hardware resources */
> + void __iomem *base;
> + int irq;
> + u32 clock_freq;
> +
> + struct i2c_msg *msgs;
> + int msg_num;
You'll never have a negative number of messages. (I see
it's assigned an int value, but my statement is still true.)
So this can be unsigned (u32).
> +
> + /* index of the current message being processed */
> + int msg_idx;
msg_idx can also be unsigned.
> + u8 *msg_buf;
> + /* the number of unprocessed bytes remaining in each message */
s/each/the current/
> + size_t unprocessed;
Why is unprocessed a size_t? It's OK, but msg_idx is a
simple (unsigned) int.
> +
> + enum spacemit_i2c_state state;
> + bool read;
> + struct completion complete;
> + u32 status;
> +};
> +
> +static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
> +{
> + u32 val;
> +
> + val = readl(i2c->base + SPACEMIT_ICR);
> + val |= SPACEMIT_CR_IUE;
> + writel(val, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
> +{
> + u32 val;
> +
> + val = readl(i2c->base + SPACEMIT_ICR);
> + val &= ~SPACEMIT_CR_IUE;
> + writel(val, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> +{
> + writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> + udelay(5);
> + writel(0, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
> +{
> + dev_dbg(i2c->dev, "i2c error status: 0x%08x\n", i2c->status);
> +
> + if (i2c->status & (SPACEMIT_SR_BED | SPACEMIT_SR_ALD)) {
> + spacemit_i2c_reset(i2c);
> + return -EAGAIN;
> + }
> +
> + return i2c->status & SPACEMIT_SR_ACKNAK ? -ENXIO : -EIO;
> +}
> +
> +static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
> +{
> + u32 status;
> +
> + /* if bus is locked, reset unit. 0: locked */
> + status = readl(i2c->base + SPACEMIT_IBMR);
> + if ((status & SPACEMIT_BMR_SDA) && (status & SPACEMIT_BMR_SCL))
> + return;
> +
> + spacemit_i2c_reset(i2c);
> + usleep_range(10, 20);
> +
> + /* check scl status again */
> + status = readl(i2c->base + SPACEMIT_IBMR);
> + if (!(status & SPACEMIT_BMR_SCL))
> + dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
> +}
> +
I think this is waiting for the bus to *not* be busy.
So spacemit_i2c_wait_bus_free() (or maybe _idle()) would
make more sense.
> +static int spacemit_i2c_wait_bus_busy(struct spacemit_i2c_dev *i2c)
> +{
> + int ret;
> + u32 val;
> +
> + val = readl(i2c->base + SPACEMIT_ISR);
> + 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 (ret)
> + spacemit_i2c_reset(i2c);
> +
> + return ret;
> +}
> +
> +static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
> +{
> + /* in case bus is not released after transfer completes */
> + if (readl(i2c->base + SPACEMIT_ISR) & SPACEMIT_SR_EBB) {
> + spacemit_i2c_conditionally_reset_bus(i2c);
> + usleep_range(90, 150);
> + }
> +}
> +
> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> +{
> + u32 val;
> +
> + /*
> + * Unmask interrupt bits for all xfer mode:
> + * bus error, arbitration loss detected.
> + * For transaction complete signal, we use master stop
> + * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
> + */
> + val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> +
> + /*
> + * Unmask interrupt bits for interrupt xfer mode:
> + * DBR rx full.
> + * For tx empty interrupt SPACEMIT_CR_DTEIE, we only
> + * need to enable when trigger byte transfer to start
> + * data sending.
> + */
> + val |= SPACEMIT_CR_DRFIE;
> +
> + if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> + val |= SPACEMIT_CR_MODE_FAST;
> +
> + /* disable response to general call */
> + val |= SPACEMIT_CR_GCD;
> +
> + /* enable SCL clock output */
> + val |= SPACEMIT_CR_SCLE;
> +
> + /* enable master stop detected */
> + val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
> +
> + writel(val, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static inline void
s/inline //
> +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
> +{
> + writel(mask & SPACEMIT_I2C_INT_STATUS_MASK, i2c->base + SPACEMIT_ISR);
> +}
> +
> +static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
> +{
> + u32 slave_addr_rw, val;
> + struct i2c_msg *cur_msg = i2c->msgs + i2c->msg_idx;
> +
> + i2c->read = !!(cur_msg->flags & I2C_M_RD);
> +
> + i2c->state = STATE_START;
> +
> + slave_addr_rw = (cur_msg->addr & 0x7f) << 1;
> + if (cur_msg->flags & I2C_M_RD)
> + slave_addr_rw |= 1;
> +
> + writel(slave_addr_rw, i2c->base + SPACEMIT_IDBR);
> +
> + /* send start pulse */
> + val = readl(i2c->base + SPACEMIT_ICR);
> + val &= ~SPACEMIT_CR_STOP;
> + val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
> + 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;
> +
> + for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
> + i2c->msg_buf = (i2c->msgs + i2c->msg_idx)->buf;
> + i2c->status = 0;
> + i2c->unprocessed = (i2c->msgs + i2c->msg_idx)->len;
I think this might be more understandable. The array syntax
shows it's really an array; and the local variable simplifies
the right-hand sides.
struct i2c_msg *msg = &i2c->msgs[i2c->msg_idx];
i2c->msg_buf = msg->buf;
i2c->unprocessed = msg->len;
i2c->status = 0;
> +
> + reinit_completion(&i2c->complete);
> +
> + spacemit_i2c_start(i2c);
> +
> + time_left = wait_for_completion_timeout(&i2c->complete,
> + i2c->adapt.timeout);
> + if (time_left == 0) {
if (!time_left) {
> + dev_err(i2c->dev, "msg completion timeout\n");
> + spacemit_i2c_conditionally_reset_bus(i2c);
> + spacemit_i2c_reset(i2c);
> + return -ETIMEDOUT;
> + }
> +
> + if (i2c->status & SPACEMIT_SR_ERR)
> + return spacemit_i2c_handle_err(i2c);
> + }
> +
> + return 0;
> +}
. . .
> +static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
> +{
> + unsigned long timeout;
> + int idx = 0, cnt = 0;
> +
> + while (idx < i2c->msg_num) {
> + cnt += (i2c->msgs + idx)->len + 1;
> + idx++;
> + }
> +
> + /*
> + * 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 / i2c->clock_freq;
> +
> + i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 2) / i2c->msg_num;
I think the half second pad on your timeout will essentially
always dominate the timeout period. If you transfer a
maximally-sized message at 100000 Hz it'll take almost
6 seconds. But in practice I'm sure transfers are under
32 bytes (maybe 256?). At 256 bytes you're looking at
23 milliseconds (plus 500000) at 100 KHz.
I looked around and the timeouts used for I2C adapters are
all over the map. 50 seconds, 1 second, 100 milliseconds,
and so on, so I guess what you're doing is fine.
That said, it might not be very important to be as precise
as you are here.
> +}
> +
. . .
> +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 clk *clk;
> + struct device *dev = &pdev->dev;
> + struct device_node *of_node = pdev->dev.of_node;
> + struct spacemit_i2c_dev *i2c;
> + int ret;
> +
> + i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
> + if (!i2c)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to read clock-frequency property");
Your binding doesn't say that the "clock-frequency" property is
required. If it's not found, you should default to 400000 Hz.
There should be no error here.
> +
> + /* For now, this driver doesn't support high-speed. */
The clock frequency is unsigned, so:
if (!i2c->clock_freq || ...) {
> + if (i2c->clock_freq < 1 || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> + dev_warn(dev, "unsupport clock frequency: %d, default: %d",
dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> + i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> + i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> + } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
I would issue the same warning message here.
> + i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
> + }
> +
> + i2c->dev = &pdev->dev;
> +
> + i2c->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(i2c->base))
> + return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap");
> +
> + i2c->irq = platform_get_irq(pdev, 0);
> + if (i2c->irq < 0)
> + return dev_err_probe(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(dev, ret, "failed to request irq");
> +
> + clk = devm_clk_get_enabled(dev, "func");
> + if (IS_ERR(clk))
> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
> +
> + clk = devm_clk_get_enabled(dev, "bus");
> + if (IS_ERR(clk))
> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
> +
> + spacemit_i2c_reset(i2c);
> +
> + 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;
I don't think you ever use i2c->adapt.algo_data. All you
need is the i2c pointer you set with i2c_set_adapdata().
That's it!
-Alex
> +
> + strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name));
> +
> + init_completion(&i2c->complete);
> +
> + platform_set_drvdata(pdev, i2c);
> +
> + ret = i2c_add_numbered_adapter(&i2c->adapt);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
> +
> + 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");
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6 2/2] i2c: spacemit: add support for SpacemiT K1 SoC
2025-03-12 1:46 ` Alex Elder
@ 2025-03-13 0:10 ` Andi Shyti
2025-03-13 1:19 ` Wolfram Sang
0 siblings, 1 reply; 6+ messages in thread
From: Andi Shyti @ 2025-03-13 0:10 UTC (permalink / raw)
To: Alex Elder
Cc: Troy Mitchell, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Yixun Lan, linux-riscv, linux-i2c, devicetree, linux-kernel,
spacemit, Guodong Xu
Hi,
On Tue, Mar 11, 2025 at 08:46:31PM -0500, Alex Elder wrote:
> On 3/7/25 7:13 AM, 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>
>
> I have a bunch of really small suggestions. Please consider
> them, but whatever you choose to do this looks good to me.
>
> Reviewed-by: Alex Elder <elder@riscstar.com>
thanks Alex for reviewing, I will wait for a v7 with your
suggestions.
Overall the patch looks good to me, as well, even though I have
to give some credit to Wolfram's previous comments about the
i2c-pxa similarities.
I checked the two drivers last night and I can see some
similarities, on the other hand I also understand that it might
require some extra effort.
I want to check it again, though, before applying.
Andi
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6 2/2] i2c: spacemit: add support for SpacemiT K1 SoC
2025-03-13 0:10 ` Andi Shyti
@ 2025-03-13 1:19 ` Wolfram Sang
0 siblings, 0 replies; 6+ messages in thread
From: Wolfram Sang @ 2025-03-13 1:19 UTC (permalink / raw)
To: Andi Shyti
Cc: Alex Elder, Troy Mitchell, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Yixun Lan, linux-riscv, linux-i2c, devicetree,
linux-kernel, spacemit, Guodong Xu
[-- Attachment #1.1: Type: text/plain, Size: 305 bytes --]
> I checked the two drivers last night and I can see some
> similarities, on the other hand I also understand that it might
> require some extra effort.
I think it is okay that it is a seperate driver. If register sets are
too different, the resulting combined driver would be a maintenance
nightmare.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-13 1:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-07 13:13 [PATCH v6 0/2] riscv: spacemit: add i2c support to K1 SoC Troy Mitchell
2025-03-07 13:13 ` [PATCH v6 1/2] dt-bindings: i2c: spacemit: add support for " Troy Mitchell
2025-03-07 13:13 ` [PATCH v6 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell
2025-03-12 1:46 ` Alex Elder
2025-03-13 0:10 ` Andi Shyti
2025-03-13 1:19 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).