* [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).