* [PATCH v2 0/2] i2c: Add driver support Amlogic A9 I2C
@ 2026-10-22 9:26 Xianwei Zhao via B4 Relay
2026-10-22 9:26 ` [PATCH v2 1/2] dt-bindings: i2c: add Amlogic A9 I2C controller Xianwei Zhao via B4 Relay
2026-10-22 9:26 ` [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver Xianwei Zhao via B4 Relay
0 siblings, 2 replies; 4+ messages in thread
From: Xianwei Zhao via B4 Relay @ 2026-10-22 9:26 UTC (permalink / raw)
To: Junyi Zhao, Andi Shyti, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Xianwei Zhao
Cc: Junyi Zhao, linux-i2c, linux-amlogic, devicetree, linux-kernel,
Xianwei Zhao
Add a Device Tree binding and driver for the I2C controller found on the
Amlogic A9 SoC.
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
Changes in v2:
- dtbinding: make some minor modifications, suggestted by Conor and robot.
- driver: Make some adjustments, including macro definitions, format adjustments,
adding quirks, process optimization, and so on. According to Andi and robot's suggestion
- Give up MAINTAINERS file the separate submission, make it together with the driver.
- Link to v1: https://lore.kernel.org/r/20260924-a9-i2c-v1-0-b8ad9b46f4a8@amlogic.com
---
Junyi Zhao (1):
i2c: amlogic: Add Amlogic A9 I2C controller driver
Xianwei Zhao (1):
dt-bindings: i2c: add Amlogic A9 I2C controller
.../devicetree/bindings/i2c/amlogic,a9-i2c.yaml | 56 +++
MAINTAINERS | 9 +
drivers/i2c/busses/Kconfig | 8 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-amlogic-a9.c | 469 +++++++++++++++++++++
5 files changed, 543 insertions(+)
---
base-commit: 4ec2365c08de306f52650d8736aef5862baedb8d
change-id: 20260911-a9-i2c-24478e22e683
Best regards,
--
Xianwei Zhao <xianwei.zhao@amlogic.com>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v2 1/2] dt-bindings: i2c: add Amlogic A9 I2C controller 2026-10-22 9:26 [PATCH v2 0/2] i2c: Add driver support Amlogic A9 I2C Xianwei Zhao via B4 Relay @ 2026-10-22 9:26 ` Xianwei Zhao via B4 Relay 2026-10-22 9:26 ` [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver Xianwei Zhao via B4 Relay 1 sibling, 0 replies; 4+ messages in thread From: Xianwei Zhao via B4 Relay @ 2026-10-22 9:26 UTC (permalink / raw) To: Junyi Zhao, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Xianwei Zhao Cc: Junyi Zhao, linux-i2c, linux-amlogic, devicetree, linux-kernel, Xianwei Zhao From: Xianwei Zhao <xianwei.zhao@amlogic.com> Add a Device Tree binding for the I2C controller found on the Amlogic A9 SoC. Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- .../devicetree/bindings/i2c/amlogic,a9-i2c.yaml | 56 ++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml b/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml new file mode 100644 index 000000000000..31ddc6d23732 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml @@ -0,0 +1,56 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/amlogic,a9-i2c.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic A9 I2C Controller + +maintainers: + - Xianwei Zhao <xianwei.zhao@amlogic.com> + - Junyi Zhao <junyi.zhao@amlogic.com> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + const: amlogic,a9-i2c + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + dmas: + items: + - description: DMA controller phandle and request line for RX + - description: DMA controller phandle and request line for TX + + dma-names: + items: + - const: rx + - const: tx + +required: + - compatible + - reg + - interrupts + - clocks + +unevaluatedProperties: false + +examples: + - | + i2c@3a4400 { + compatible = "amlogic,a9-i2c"; + reg = <0x3a4400 0x3c>; + interrupts = <0 164 1>; + clocks = <&clkc_periphs 11>; + #address-cells = <1>; + #size-cells = <0>; + }; -- 2.52.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver 2026-10-22 9:26 [PATCH v2 0/2] i2c: Add driver support Amlogic A9 I2C Xianwei Zhao via B4 Relay 2026-10-22 9:26 ` [PATCH v2 1/2] dt-bindings: i2c: add Amlogic A9 I2C controller Xianwei Zhao via B4 Relay @ 2026-10-22 9:26 ` Xianwei Zhao via B4 Relay 2026-10-22 9:46 ` sashiko-bot 1 sibling, 1 reply; 4+ messages in thread From: Xianwei Zhao via B4 Relay @ 2026-10-22 9:26 UTC (permalink / raw) To: Junyi Zhao, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Xianwei Zhao Cc: Junyi Zhao, linux-i2c, linux-amlogic, devicetree, linux-kernel, Xianwei Zhao From: Junyi Zhao <junyi.zhao@amlogic.com> Add support for the new I2C controller used by Amlogic A9 SoCs. Add a dedicated Kconfig option and Makefile entry for the A9 I2C controller driver. And add Amlogic I2C controller entry to MAINTAINERS to clarify the maintainers. Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- MAINTAINERS | 9 + drivers/i2c/busses/Kconfig | 8 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-amlogic-a9.c | 469 ++++++++++++++++++++++++++++++++++++ 4 files changed, 487 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index a2d702919ef3..a56bdfefabad 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1371,6 +1371,15 @@ F: Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml F: drivers/perf/amlogic/ F: include/soc/amlogic/ +AMLOGIC I2C DRIVER +M: Junyi Zhao <junyi.zhao@amlogic.com> +M: Xianwei Zhao <xianwei.zhao@amlogic.com> +L: linux-i2c@vger.kernel.org +L: linux-amlogic@lists.infradead.org +S: Maintained +F: Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml +F: drivers/i2c/busses/i2c-amlogic-a9.c + AMLOGIC ISP DRIVER M: Keke Li <keke.li@amlogic.com> L: linux-media@vger.kernel.org diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 275aaa7f3583..50e031d97ed3 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -885,6 +885,14 @@ config I2C_MESON If you say yes to this option, support will be included for the I2C interface on the Amlogic Meson family of SoCs. +config I2C_AMLOGIC_A9 + tristate "Amlogic new I2C controller" + depends on ARCH_MESON || COMPILE_TEST + depends on COMMON_CLK + help + If you say yes to this option, support will be included for the + I2C interface on the new Amlogic family of SoCs. + config I2C_MICROCHIP_CORE tristate "Microchip FPGA I2C controller" depends on ARCH_MICROCHIP_POLARFIRE || COMPILE_TEST diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index e5440de0fe14..343eec4b0889 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -82,6 +82,7 @@ obj-$(CONFIG_I2C_LPC2K) += i2c-lpc2k.o obj-$(CONFIG_I2C_LS2X) += i2c-ls2x.o obj-$(CONFIG_I2C_LS2X_V2) += i2c-ls2x-v2.o obj-$(CONFIG_I2C_MESON) += i2c-meson.o +obj-$(CONFIG_I2C_AMLOGIC_A9) += i2c-amlogic-a9.o obj-$(CONFIG_I2C_MICROCHIP_CORE) += i2c-microchip-corei2c.o obj-$(CONFIG_I2C_MPC) += i2c-mpc.o obj-$(CONFIG_I2C_MT65XX) += i2c-mt65xx.o diff --git a/drivers/i2c/busses/i2c-amlogic-a9.c b/drivers/i2c/busses/i2c-amlogic-a9.c new file mode 100644 index 000000000000..d4954a4fd88e --- /dev/null +++ b/drivers/i2c/busses/i2c-amlogic-a9.c @@ -0,0 +1,469 @@ +// SPDX-License-Identifier: (GPL-2.0-only OR MIT) +/* + * Copyright (C) 2026 Amlogic, Inc. All rights reserved + */ + +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> + +/* Amlogic I2C register map */ +#define REG_CFG_RDY 0x00 +#define REG_CFG_I2C 0x04 +#define REG_CFG_START 0x08 +#define REG_CFG_BUS 0x0c +#define REG_TX_RD_ADDR 0x10 +#define REG_TX_WR_ADDR 0x14 +#define REG_RX_RD_ADDR 0x18 +#define REG_RX_WR_ADDR 0x1c +#define REG_CFG_TX 0x20 +#define REG_CFG_RX 0x24 +#define REG_CFG_IRQ_STATE 0x30 +#define REG_CFG_IRQ_ENABLE 0x34 +#define REG_SHAKE_BLK_CNT 0x38 + +/* CFG RDY fields */ +#define RDY_TEE_ONLY BIT(1) +#define RDY_IF BIT(0) + +/* CFG I2C fields */ +#define I2C_RX_THR GENMASK(23, 16) +#define I2C_TX_THR GENMASK(15, 8) + +/* CFG BUS fields */ +#define BUS_NO_STOP BIT(18) + +/* SCL = clk/b_ratio if b_ratio<=8, SCL = clk/8 */ +#define BUS_RATIO_MASK GENMASK(11, 0) + +/* CFG START fields */ +#define START_START BIT(31) +#define START_LEN GENMASK(23, 12) +#define START_LEN_SHIFT 12 +#define START_SLAVE_ADDR GENMASK(10, 1) +#define START_SADDR_SHIFT 1 +#define START_READ BIT(0) + +/* CFG TX_RX fields */ +#define TX_RX_DATA GENMASK(7, 0) + +/* CFG IRQ fields */ +#define IRQ_ALL_MASK GENMASK(15, 0) +#define A9_NCK_ERROR BIT(0) +#define A9_RX_THRESH_READ BIT(5) +#define A9_TX_THRESH_WRITE BIT(6) +#define A9_PHY_DONE BIT(7) +#define A9_TASK_DONE BIT(8) +#define A9_ALL_DONE BIT(9) + +#define A9_TRANS_DONE (A9_PHY_DONE | A9_NCK_ERROR) +#define A9_TRANS_ERROR (A9_NCK_ERROR) +#define A9_ENABLE_IRQ_BIT (A9_NCK_ERROR | A9_PHY_DONE) +#define THRESH_MODE (A9_RX_THRESH_READ | A9_TX_THRESH_WRITE) + +#define A9_I2C_FIFO_DEPTH 32 +#define A9_I2C_HALF_FIFO (A9_I2C_FIFO_DEPTH >> 1) + +#define A9_I2C_MAX_XFER_SIZE 0xfff + +#define I2C_TIMEOUT_MS 500 + +enum { + A9_STATE_IDLE, + A9_STATE_READ, + A9_STATE_WRITE, +}; + +enum fifo_fill_mode { + A9_FIFO_FILL_FULL, + A9_FIFO_FILL_HALF, +}; + +/** + * struct aml_i2c - Amlogic I2C device private data + * + * @adap: I2C adapter instance + * @dev: Pointer to device structure + * @regs: Base address of the device memory mapped registers + * @clk: Pointer to clock structure + * @msg: Pointer to the current I2C message + * @state: Current state in the driver state machine + * @count: Number of bytes to be sent/received in current transfer + * @pos: Current position in the send/receive buffer + * @error: Flag set when an error is received + * @lock: To avoid race conditions between irq handler and xfer code + * @done: Completion used to wait for transfer termination + * @clk_rate: Clock source rate of I2C module + * @frequency: Operating frequency of I2C bus clock + */ +struct aml_i2c { + struct i2c_adapter adap; + struct device *dev; + void __iomem *regs; + struct clk *clk; + + struct i2c_msg *msg; + int state; + int count; + int pos; + int error; + /* protect reg access */ + spinlock_t lock; + struct completion done; + unsigned long clk_rate; + unsigned int frequency; +}; + +static void aml_i2c_set_mask(struct aml_i2c *i2c, int reg, u32 mask, u32 val) +{ + u32 data; + + data = readl(i2c->regs + reg); + data &= ~mask; + data |= val & mask; + writel(data, i2c->regs + reg); +} + +/* + * SCL = clk/b_ratio. if b_ratio<=8, SCL = clk/8 + */ +static int aml_i2c_set_clk_div(struct aml_i2c *i2c) +{ + unsigned long clk_rate = i2c->clk_rate; + unsigned int div; + + div = DIV_ROUND_UP(clk_rate, i2c->frequency); + + /* clock divider has 12 bits */ + if (div >= (1 << 12)) { + dev_err(i2c->dev, "requested bus frequency too low\n"); + return -EINVAL; + } else if (div <= 8) { + dev_err(i2c->dev, "requested bus frequency too high\n"); + return -EINVAL; + } + /* CFG_BUS reg:11 - 0 bits */ + aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_RATIO_MASK, div); + + return 0; +} + +static void reset_fifo_pos(struct aml_i2c *i2c) +{ + writel(0x00, i2c->regs + REG_TX_RD_ADDR); + writel(0x00, i2c->regs + REG_TX_WR_ADDR); + writel(0x00, i2c->regs + REG_RX_WR_ADDR); + writel(0x00, i2c->regs + REG_RX_RD_ADDR); +} + +static int controller_is_error(struct aml_i2c *i2c) +{ + int ret = 0; + + if (readl(i2c->regs + REG_CFG_IRQ_STATE) & A9_TRANS_ERROR) + ret = -ENXIO; + + return ret; +} + +static int controller_is_done(struct aml_i2c *i2c) +{ + return readl(i2c->regs + REG_CFG_IRQ_STATE) & A9_TRANS_DONE; +} + +static void clear_irq_status(struct aml_i2c *i2c, u32 irq_bits) +{ + writel(irq_bits, i2c->regs + REG_CFG_IRQ_STATE); +} + +static int aml_i2c_init(struct aml_i2c *i2c) +{ + int ret; + + clear_irq_status(i2c, IRQ_ALL_MASK); + aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, BUS_NO_STOP); + aml_i2c_set_mask(i2c, REG_CFG_I2C, I2C_RX_THR | I2C_TX_THR, + (A9_I2C_FIFO_DEPTH << 16) | (A9_I2C_HALF_FIFO << 8)); + reset_fifo_pos(i2c); + ret = aml_i2c_set_clk_div(i2c); + + return ret; +} + +static void aml_i2c_put_data(struct aml_i2c *i2c, u8 *buf, int len) +{ + int i; + + /* When this I2C module transfers 0 bytes, it must still send at least 1 byte. */ + if (!i2c->msg->len) { + writel(0x00, i2c->regs + REG_CFG_TX); + return; + } + + for (i = 0; i < len; i++, buf++) + writel(*buf, i2c->regs + REG_CFG_TX); +} + +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode) +{ + bool write = !(i2c->msg->flags & I2C_M_RD); + + if (!write) + return; + + if (mode == A9_FIFO_FILL_FULL) + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH); + else + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_HALF_FIFO); + aml_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count); +} + +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id) +{ + struct aml_i2c *i2c = dev_id; + u8 *buf = NULL; + int ret; + u32 rd_len, i; + u32 rd_val; + + spin_lock(&i2c->lock); + ret = controller_is_error(i2c); + if (ret) { + i2c->error = ret; + goto complete_irq_out; + } + + switch (i2c->state) { + case A9_STATE_READ: + buf = i2c->msg->buf; + rd_len = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH); + for (i = 0; i < rd_len; i++) { + rd_val = readl(i2c->regs + REG_CFG_RX); + buf[i2c->pos++] = rd_val & TX_RX_DATA; + } + /* Rx must clear thresh after read fifo */ + clear_irq_status(i2c, A9_RX_THRESH_READ); + break; + case A9_STATE_WRITE: + aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_HALF); + if ((i2c->pos + A9_I2C_HALF_FIFO) >= i2c->msg->len) + aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_TX_THRESH_WRITE, 0); + i2c->pos += i2c->count; + /* Tx must clear thresh after write fifo */ + clear_irq_status(i2c, A9_TX_THRESH_WRITE); + break; + case A9_STATE_IDLE: + dev_warn(i2c->dev, "i2c irq triggered abnormally, irq_state:0x%x,irq_en:0x%x\n", + readl(i2c->regs + REG_CFG_IRQ_STATE), + readl(i2c->regs + REG_CFG_IRQ_ENABLE)); + break; + default: + break; + } + + if (!controller_is_done(i2c)) { + goto irq_trans_out; + } else { + if (i2c->state == A9_STATE_READ && (i2c->msg->len - i2c->pos)) + goto irq_trans_out; + } + +complete_irq_out: + reset_fifo_pos(i2c); + complete(&i2c->done); + i2c->state = A9_STATE_IDLE; + aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, 0); + clear_irq_status(i2c, IRQ_ALL_MASK); +irq_trans_out: + spin_unlock(&i2c->lock); + + return IRQ_HANDLED; +} + +static void aml_i2c_do_start(struct aml_i2c *i2c, struct i2c_msg *msg) +{ + unsigned int reg_val; + unsigned int trans_len; + + trans_len = msg->len; + reg_val = (unsigned int)((msg->addr << START_SADDR_SHIFT) | + ((msg->flags & I2C_M_RD) ? START_READ : 0) | + ((trans_len & 0xfff) << START_LEN_SHIFT) | START_START); + aml_i2c_set_mask(i2c, REG_CFG_START, + START_START | START_SLAVE_ADDR | START_READ | START_LEN, reg_val); + clear_irq_status(i2c, IRQ_ALL_MASK); + /* enable thresh mode */ + if (trans_len > A9_I2C_FIFO_DEPTH) { + reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE; + aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, + A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val); + } +} + +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last) +{ + unsigned long time_left, flags; + int ret = 0; + + i2c->msg = msg; + i2c->pos = 0; + i2c->count = 0; + i2c->error = 0; + + i2c->state = (msg->flags & I2C_M_RD) ? A9_STATE_READ : A9_STATE_WRITE; + spin_lock_irqsave(&i2c->lock, flags); + aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_FULL); + i2c->pos += i2c->count; + spin_unlock_irqrestore(&i2c->lock, flags); + reinit_completion(&i2c->done); + + if (last) + aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, 0); + else + aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, BUS_NO_STOP); + /* Start the transfer */ + aml_i2c_do_start(i2c, msg); + + time_left = msecs_to_jiffies(I2C_TIMEOUT_MS); + time_left = wait_for_completion_timeout(&i2c->done, time_left); + + /* + * Protect access to i2c struct and registers from interrupt + * handlers triggered by a transfer terminated after the + * timeout period + */ + + spin_lock_irqsave(&i2c->lock, flags); + if (!time_left) { + i2c->state = A9_STATE_IDLE; + clear_irq_status(i2c, IRQ_ALL_MASK); + reset_fifo_pos(i2c); + ret = -ETIMEDOUT; + } + + if (i2c->error) + ret = i2c->error; + spin_unlock_irqrestore(&i2c->lock, flags); + + return ret; +} + +static int aml_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) +{ + struct aml_i2c *i2c = adap->algo_data; + int i, ret = 0; + + for (i = 0; i < num; i++) { + ret = aml_i2c_xfer_msg(i2c, msgs + i, i == num - 1); + if (ret) + return ret; + } + + return i; +} + +static u32 aml_i2c_func(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; +} + +static const struct i2c_adapter_quirks aml_i2c_quirks = { + .max_read_len = A9_I2C_MAX_XFER_SIZE, + .max_write_len = A9_I2C_MAX_XFER_SIZE, +}; + +static const struct i2c_algorithm aml_i2c_algorithm = { + .master_xfer = aml_i2c_xfer, + .functionality = aml_i2c_func, +}; + +static int aml_i2c_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct aml_i2c *i2c; + struct i2c_timings timings; + int irq, ret; + + i2c = devm_kzalloc(&pdev->dev, sizeof(struct aml_i2c), GFP_KERNEL); + if (!i2c) + return -ENOMEM; + + i2c_parse_fw_timings(&pdev->dev, &timings, true); + i2c->frequency = timings.bus_freq_hz; + + i2c->dev = &pdev->dev; + platform_set_drvdata(pdev, i2c); + + spin_lock_init(&i2c->lock); + init_completion(&i2c->done); + + i2c->regs = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(i2c->regs)) + return PTR_ERR(i2c->regs); + + i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL); + if (IS_ERR(i2c->clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->clk), "can't get enable clock\n"); + + i2c->clk_rate = clk_get_rate(i2c->clk); + if (i2c->clk_rate == 0) + return dev_err_probe(&pdev->dev, -EINVAL, "failed to get clk rate\n"); + + ret = aml_i2c_init(i2c); + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, "fail init i2c\n"); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return dev_err_probe(&pdev->dev, irq, "can't find IRQ\n"); + + ret = devm_request_irq(&pdev->dev, irq, aml_i2c_irq, 0, dev_name(&pdev->dev), i2c); + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, "can't request IRQ\n"); + /* Enable error & done irq */ + writel(A9_ENABLE_IRQ_BIT, i2c->regs + REG_CFG_IRQ_ENABLE); + strscpy(i2c->adap.name, "Amlogic I2C adapter", + sizeof(i2c->adap.name)); + i2c->adap.owner = THIS_MODULE; + i2c->adap.algo = &aml_i2c_algorithm; + i2c->adap.quirks = &aml_i2c_quirks; + i2c->adap.dev.parent = &pdev->dev; + i2c->adap.dev.of_node = np; + i2c->adap.algo_data = i2c; + + return i2c_add_adapter(&i2c->adap); +} + +static void aml_i2c_remove(struct platform_device *pdev) +{ + struct aml_i2c *i2c = platform_get_drvdata(pdev); + + i2c_del_adapter(&i2c->adap); +} + +static const struct of_device_id aml_i2c_match[] = { + { .compatible = "amlogic,a9-i2c",}, + { } +}; + +MODULE_DEVICE_TABLE(of, aml_i2c_match); + +static struct platform_driver aml_i2c_driver = { + .probe = aml_i2c_probe, + .remove = aml_i2c_remove, + .driver = { + .name = "amlogic-i2c", + .of_match_table = aml_i2c_match, + }, +}; + +module_platform_driver(aml_i2c_driver); + +MODULE_DESCRIPTION("Amlogic I2C Bus driver"); +MODULE_AUTHOR("Junyi Zhao <junyi.zhao@amlogic.com>"); +MODULE_LICENSE("GPL"); -- 2.52.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver 2026-10-22 9:26 ` [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver Xianwei Zhao via B4 Relay @ 2026-10-22 9:46 ` sashiko-bot 0 siblings, 0 replies; 4+ messages in thread From: sashiko-bot @ 2026-10-22 9:46 UTC (permalink / raw) To: Xianwei Zhao; +Cc: linux-amlogic, devicetree, robh, conor+dt, neil.armstrong Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] i2c: amlogic-a9: unlocked state init and late IRQ setup in aml_i2c_do_start() - [High] i2c: amlogic-a9: threshold IRQs left enabled on timeout in aml_i2c_xfer_msg() - [High] i2c: amlogic-a9: unhandled IRQ and spurious complete() in aml_i2c_irq() - [High] i2c: amlogic: missing RX drain in aml_i2c_irq() when A9_PHY_DONE is set - [Medium] i2c: amlogic-a9: unmasked msg->addr in aml_i2c_do_start() corrupts START_LEN -- commit 10e276126f680fbcf2e402bbde459a690540ef33 Author: Junyi Zhao <junyi.zhao@amlogic.com> i2c: amlogic: Add Amlogic A9 I2C controller driver This commit adds a driver for the I2C controller found on Amlogic A9 SoCs, along with the corresponding Kconfig, Makefile, and MAINTAINERS entries. > diff --git a/drivers/i2c/busses/i2c-amlogic-a9.c b/drivers/i2c/busses/i2c-amlogic-a9.c > new file mode 100644 > index 0000000000000..d4954a4fd88ef > --- /dev/null > +++ b/drivers/i2c/busses/i2c-amlogic-a9.c > @@ -0,0 +1,469 @@ [ ... ] > +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id) > +{ > + struct aml_i2c *i2c = dev_id; > + u8 *buf = NULL; > + int ret; > + u32 rd_len, i; > + u32 rd_val; > + > + spin_lock(&i2c->lock); > + ret = controller_is_error(i2c); > + if (ret) { > + i2c->error = ret; > + goto complete_irq_out; > + } > + > + switch (i2c->state) { > + case A9_STATE_READ: > + buf = i2c->msg->buf; > + rd_len = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH); > + for (i = 0; i < rd_len; i++) { > + rd_val = readl(i2c->regs + REG_CFG_RX); > + buf[i2c->pos++] = rd_val & TX_RX_DATA; > + } > + /* Rx must clear thresh after read fifo */ > + clear_irq_status(i2c, A9_RX_THRESH_READ); > + break; > + case A9_STATE_WRITE: > + aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_HALF); > + if ((i2c->pos + A9_I2C_HALF_FIFO) >= i2c->msg->len) > + aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_TX_THRESH_WRITE, 0); > + i2c->pos += i2c->count; > + /* Tx must clear thresh after write fifo */ > + clear_irq_status(i2c, A9_TX_THRESH_WRITE); > + break; > + case A9_STATE_IDLE: > + dev_warn(i2c->dev, "i2c irq triggered abnormally, irq_state:0x%x,irq_en:0x%x\n", > + readl(i2c->regs + REG_CFG_IRQ_STATE), > + readl(i2c->regs + REG_CFG_IRQ_ENABLE)); > + break; > + default: > + break; > + } > + > + if (!controller_is_done(i2c)) { > + goto irq_trans_out; > + } else { > + if (i2c->state == A9_STATE_READ && (i2c->msg->len - i2c->pos)) > + goto irq_trans_out; > + } [Severity: High] Can a multi-chunk read (msg->len > A9_I2C_FIFO_DEPTH) hang in drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_irq() if the remaining bytes finish arriving before A9_RX_THRESH_READ is cleared? In aml_i2c_xfer() -> aml_i2c_xfer_msg() -> aml_i2c_do_start(), a read with msg->len > 32 enables A9_RX_THRESH_READ alongside A9_PHY_DONE, with I2C_RX_THR configured to 32. When the first 32 bytes arrive, A9_RX_THRESH_READ asserts. If interrupt handling is slightly delayed or the remaining tail bytes finish arriving while the A9_STATE_READ loop is reading the first 32 bytes from REG_CFG_RX, A9_PHY_DONE will already be set in REG_CFG_IRQ_STATE before clear_irq_status(i2c, A9_RX_THRESH_READ) runs. Because A9_STATE_READ only drains up to A9_I2C_FIFO_DEPTH bytes per call, i2c->msg->len - i2c->pos remains non-zero, and the controller_is_done(i2c) branch takes goto irq_trans_out without draining the remaining tail bytes or completing i2c->done. Since A9_PHY_DONE is already asserted and fewer than 32 bytes remain to re-trigger A9_RX_THRESH_READ, no new rising edge will occur on the interrupt line, leaving wait_for_completion_timeout() in aml_i2c_xfer_msg() to time out with -ETIMEDOUT. Should aml_i2c_irq() drain the remaining bytes in the RX FIFO when controller_is_done(i2c) is true during A9_STATE_READ before completing the transfer? > + > +complete_irq_out: > + reset_fifo_pos(i2c); > + complete(&i2c->done); > + i2c->state = A9_STATE_IDLE; > + aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, 0); > + clear_irq_status(i2c, IRQ_ALL_MASK); > +irq_trans_out: > + spin_unlock(&i2c->lock); > + > + return IRQ_HANDLED; > +} [Severity: High] What happens in drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_irq() when an interrupt arrives while i2c->state is A9_STATE_IDLE? First, controller_is_error(i2c) is checked before inspecting i2c->state. If a late A9_NCK_ERROR interrupt fires after a transfer has already timed out and transitioned to A9_STATE_IDLE, aml_i2c_irq() sets i2c->error = -ENXIO, jumps to complete_irq_out, and calls complete(&i2c->done). Second, in the switch statement, the A9_STATE_IDLE case logs a warning and breaks into the completion checks: if (!controller_is_done(i2c)) { goto irq_trans_out; } If a threshold interrupt fires while in A9_STATE_IDLE and controller_is_done() is false, aml_i2c_irq() jumps to irq_trans_out and returns IRQ_HANDLED without clearing REG_CFG_IRQ_STATE or masking the threshold interrupt in REG_CFG_IRQ_ENABLE, leaving the interrupt asserted. Conversely, if controller_is_done() is true (such as a late A9_PHY_DONE), execution falls through to complete_irq_out and spuriously signals i2c->done while idle. Should aml_i2c_irq() check for A9_STATE_IDLE up front, disable threshold interrupts, clear REG_CFG_IRQ_STATE, and return without calling complete(&i2c->done)? > + > +static void aml_i2c_do_start(struct aml_i2c *i2c, struct i2c_msg *msg) > +{ > + unsigned int reg_val; > + unsigned int trans_len; > + > + trans_len = msg->len; > + reg_val = (unsigned int)((msg->addr << START_SADDR_SHIFT) | > + ((msg->flags & I2C_M_RD) ? START_READ : 0) | > + ((trans_len & 0xfff) << START_LEN_SHIFT) | START_START); > + aml_i2c_set_mask(i2c, REG_CFG_START, > + START_START | START_SLAVE_ADDR | START_READ | START_LEN, reg_val); [Severity: Medium] In drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_do_start(), could upper bits in msg->addr corrupt the START_LEN field of REG_CFG_START? When userspace submits an I2C_RDWR ioctl on /dev/i2c-*, msg->addr is a u16 passed through i2cdev_ioctl_rdwr() -> i2c_transfer() -> aml_i2c_xfer() -> aml_i2c_xfer_msg() -> aml_i2c_do_start() without masking. Because msg->addr << START_SADDR_SHIFT is not masked to 7 bits (or START_SLAVE_ADDR) before being ORed with (trans_len & 0xfff) << START_LEN_SHIFT and written via aml_i2c_set_mask() under the combined mask START_START | START_SLAVE_ADDR | START_READ | START_LEN, any bits set in msg->addr[15:11] shift into START_LEN (GENMASK(23, 12)) and alter the hardware transfer length. Should msg->addr be masked (for example with 0x7f, since 10-bit addressing is not advertised in aml_i2c_func()) before shifting by START_SADDR_SHIFT? > + clear_irq_status(i2c, IRQ_ALL_MASK); > + /* enable thresh mode */ > + if (trans_len > A9_I2C_FIFO_DEPTH) { > + reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE; > + aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, > + A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val); > + } > +} > + > +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last) > +{ > + unsigned long time_left, flags; > + int ret = 0; > + > + i2c->msg = msg; > + i2c->pos = 0; > + i2c->count = 0; > + i2c->error = 0; > + > + i2c->state = (msg->flags & I2C_M_RD) ? A9_STATE_READ : A9_STATE_WRITE; > + spin_lock_irqsave(&i2c->lock, flags); > + aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_FULL); > + i2c->pos += i2c->count; > + spin_unlock_irqrestore(&i2c->lock, flags); > + reinit_completion(&i2c->done); > + > + if (last) > + aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, 0); > + else > + aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, BUS_NO_STOP); > + /* Start the transfer */ > + aml_i2c_do_start(i2c, msg); [Severity: High] Can starting the hardware transfer before clearing IRQ status and enabling threshold interrupts in drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_do_start(), without holding i2c->lock across transfer setup in aml_i2c_xfer_msg(), race with aml_i2c_irq()? In aml_i2c_do_start(), setting START_START in REG_CFG_START kicks off the hardware transaction before clear_irq_status(i2c, IRQ_ALL_MASK) and before enabling A9_RX_THRESH_READ or A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE: aml_i2c_set_mask(i2c, REG_CFG_START, START_START | START_SLAVE_ADDR | START_READ | START_LEN, reg_val); clear_irq_status(i2c, IRQ_ALL_MASK); /* enable thresh mode */ if (trans_len > A9_I2C_FIFO_DEPTH) { reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE; aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val); } If the task is preempted or interrupted after writing REG_CFG_START, or if the controller quickly asserts A9_NCK_ERROR (such as on an address NACK) or A9_PHY_DONE before clear_irq_status() runs, clear_irq_status(i2c, IRQ_ALL_MASK) can wipe out the newly asserted IRQ status bits and cause wait_for_completion_timeout() to time out. Similarly, if aml_i2c_irq() runs and completes on an early NACK before aml_i2c_do_start() updates REG_CFG_IRQ_ENABLE, the unlocked read-modify-write in aml_i2c_do_start() can re-enable A9_RX_THRESH_READ or A9_TX_THRESH_WRITE after complete_irq_out already cleared them. In addition, in aml_i2c_xfer_msg(), i2c->msg, i2c->pos, i2c->count, i2c->error, and i2c->state are initialized outside i2c->lock, and i2c->lock is released before reinit_completion(&i2c->done), REG_CFG_BUS configuration, and aml_i2c_do_start(). If a late interrupt from a previous timed-out transfer arrives while i2c->state is already set to A9_STATE_READ or A9_STATE_WRITE, aml_i2c_irq() can modify i2c->pos and access the new i2c->msg->buf before the new transfer begins (and if i2c->pos exceeds the new msg->len in A9_STATE_READ, the signed subtraction i2c->msg->len - i2c->pos in min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH) will wrap to a large u32 in rd_len and overflow buf[i2c->pos++]). Could the state initialization, reinit_completion(), bus configuration, IRQ status clearing, threshold IRQ setup, and START_START trigger all be performed under a single spin_lock_irqsave(&i2c->lock, flags) critical section, with clear_irq_status() and REG_CFG_IRQ_ENABLE configured before asserting START_START? > + > + time_left = msecs_to_jiffies(I2C_TIMEOUT_MS); > + time_left = wait_for_completion_timeout(&i2c->done, time_left); > + > + /* > + * Protect access to i2c struct and registers from interrupt > + * handlers triggered by a transfer terminated after the > + * timeout period > + */ > + > + spin_lock_irqsave(&i2c->lock, flags); > + if (!time_left) { > + i2c->state = A9_STATE_IDLE; > + clear_irq_status(i2c, IRQ_ALL_MASK); > + reset_fifo_pos(i2c); > + ret = -ETIMEDOUT; > + } [Severity: High] When a transfer with msg->len > A9_I2C_FIFO_DEPTH times out in drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_xfer_msg(), aren't the threshold interrupts in REG_CFG_IRQ_ENABLE and the active transfer in REG_CFG_START left enabled? In aml_i2c_do_start(), transfers with trans_len > A9_I2C_FIFO_DEPTH enable A9_RX_THRESH_READ or A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE, and only update REG_CFG_IRQ_ENABLE when trans_len > A9_I2C_FIFO_DEPTH. On normal completion in aml_i2c_irq(), complete_irq_out clears A9_RX_THRESH_READ | A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE, but the !time_left timeout path in aml_i2c_xfer_msg() does not disable those threshold IRQ bits or stop the transfer in REG_CFG_START. If a >32-byte write times out and the next transfer is a short read with msg->len <= 32, wouldn't A9_TX_THRESH_WRITE remain enabled and fire during A9_STATE_READ, causing aml_i2c_irq() to read from an empty REG_CFG_RX FIFO into msg->buf? [ ... ] > +MODULE_DESCRIPTION("Amlogic I2C Bus driver"); > +MODULE_AUTHOR("Junyi Zhao <junyi.zhao@amlogic.com>"); > +MODULE_LICENSE("GPL"); -- Sashiko AI review · https://sashiko.dev/#/patchset/20261022-a9-i2c-v2-0-45e3cef175bf@amlogic.com?part=2 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-10-22 9:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-10-22 9:26 [PATCH v2 0/2] i2c: Add driver support Amlogic A9 I2C Xianwei Zhao via B4 Relay 2026-10-22 9:26 ` [PATCH v2 1/2] dt-bindings: i2c: add Amlogic A9 I2C controller Xianwei Zhao via B4 Relay 2026-10-22 9:26 ` [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver Xianwei Zhao via B4 Relay 2026-10-22 9:46 ` sashiko-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®