* [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-10-22 9:26 ` Xianwei Zhao
0 siblings, 0 replies; 21+ 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 related [flat|nested] 21+ messages in thread* [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-10-22 9:26 ` Xianwei Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Xianwei Zhao @ 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-10-22 9:26 ` Xianwei Zhao
@ 2026-10-22 9:46 ` sashiko-bot
-1 siblings, 0 replies; 21+ 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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-10-22 9:46 ` sashiko-bot
0 siblings, 0 replies; 21+ 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] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-10-22 9:46 ` sashiko-bot
@ 2026-10-28 11:21 ` Xianwei Zhao
-1 siblings, 0 replies; 21+ messages in thread
From: Xianwei Zhao @ 2026-10-28 11:21 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-amlogic, devicetree, robh, conor+dt, neil.armstrong
On 2026/10/22 17:46, sashiko-bot@kernel.org wrote:
> 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?
>
No. According to the code logic, if the data has not been read
completely, the condition for completing the operation is not met, so
the interrupt status bit will not be cleared. Once the data transfer is
complete, the interrupt will immediately be triggered again, allowing
the handler to continue reading the data. Finally, once the read
operation is complete and the condition is satisfied, the completion
interrupt will be 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)?
>
Later, I will check the status at the very beginning. If an abnormal
interrupt occurs, I can skip the processing and clear irq state.
>> +
>> +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?
>
Will add mask for address.
>> + 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?
>
I will lock start. This way, there will be no conflict that causes
interruption.
>> +
>> + 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?
>
I will take corresponding measures to prevent this from happening.
> [ ... ]
>
>> +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] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-10-28 11:21 ` Xianwei Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Xianwei Zhao @ 2026-10-28 11:21 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-amlogic, devicetree, robh, conor+dt, neil.armstrong
On 2026/10/22 17:46, sashiko-bot@kernel.org wrote:
> 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?
>
No. According to the code logic, if the data has not been read
completely, the condition for completing the operation is not met, so
the interrupt status bit will not be cleared. Once the data transfer is
complete, the interrupt will immediately be triggered again, allowing
the handler to continue reading the data. Finally, once the read
operation is complete and the condition is satisfied, the completion
interrupt will be 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)?
>
Later, I will check the status at the very beginning. If an abnormal
interrupt occurs, I can skip the processing and clear irq state.
>> +
>> +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?
>
Will add mask for address.
>> + 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?
>
I will lock start. This way, there will be no conflict that causes
interruption.
>> +
>> + 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?
>
I will take corresponding measures to prevent this from happening.
> [ ... ]
>
>> +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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-10-22 9:26 ` Xianwei Zhao
@ 2026-10-26 15:22 ` Andi Shyti
-1 siblings, 0 replies; 21+ messages in thread
From: Andi Shyti @ 2026-10-26 15:22 UTC (permalink / raw)
To: Xianwei Zhao
Cc: Junyi Zhao, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Xianwei Zhao, Junyi Zhao, linux-i2c, linux-amlogic, devicetree,
linux-kernel
Hi Junyi,
Sashiko's comments look legit to me, can you please take a look
at those?
Besides...
> +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;
> + }
No need for brackets here:
if (...)
goto ...;
else if (...)
goto ...;
But perhaps the two can be even merged.
(i2c->msg->len - i2c->pos) is equal to (i2c->pos < i2c->msg->len)
> +
> +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);
...
> + 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);
I think this should also be under spinlock in order to avoid that
the interrupt handler accesses a partially initialized state.
> +
> + 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;
the next instruction overwrites this error. Perhaps you could
use a goto to spin_unlock and return.
Andi
> + }
> +
> + if (i2c->error)
> + ret = i2c->error;
> + spin_unlock_irqrestore(&i2c->lock, flags);
> +
> + return ret;
> +}
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-10-26 15:22 ` Andi Shyti
0 siblings, 0 replies; 21+ messages in thread
From: Andi Shyti @ 2026-10-26 15:22 UTC (permalink / raw)
To: Xianwei Zhao
Cc: Junyi Zhao, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Xianwei Zhao, Junyi Zhao, linux-i2c, linux-amlogic, devicetree,
linux-kernel
Hi Junyi,
Sashiko's comments look legit to me, can you please take a look
at those?
Besides...
> +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;
> + }
No need for brackets here:
if (...)
goto ...;
else if (...)
goto ...;
But perhaps the two can be even merged.
(i2c->msg->len - i2c->pos) is equal to (i2c->pos < i2c->msg->len)
> +
> +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);
...
> + 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);
I think this should also be under spinlock in order to avoid that
the interrupt handler accesses a partially initialized state.
> +
> + 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;
the next instruction overwrites this error. Perhaps you could
use a goto to spin_unlock and return.
Andi
> + }
> +
> + if (i2c->error)
> + ret = i2c->error;
> + spin_unlock_irqrestore(&i2c->lock, flags);
> +
> + return ret;
> +}
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-10-26 15:22 ` Andi Shyti
@ 2026-10-27 2:59 ` Xianwei Zhao
-1 siblings, 0 replies; 21+ messages in thread
From: Xianwei Zhao @ 2026-10-27 2:59 UTC (permalink / raw)
To: Andi Shyti
Cc: Junyi Zhao, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Xianwei Zhao, Junyi Zhao, linux-i2c, linux-amlogic, devicetree,
linux-kernel
Hi Andi,
Thanks for your review.
On 2026/10/26 23:22, Andi Shyti wrote:
> Hi Junyi,
>
> Sashiko's comments look legit to me, can you please take a look
> at those?
>
I'm currently looking at this part. Some of them don't have relatively
good solutions yet, so I haven't replied.
> Besides...
>
>> +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;
>> + }
> No need for brackets here:
>
> if (...)
> goto ...;
> else if (...)
> goto ...;
>
Will do.
> But perhaps the two can be even merged.
I will try to merged them.
> (i2c->msg->len - i2c->pos) is equal to (i2c->pos < i2c->msg->len)
>
Will do.
>> +
>> +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);
> ...
>
>> + 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);
> I think this should also be under spinlock in order to avoid that
> the interrupt handler accesses a partially initialized state.
>
I will lock it.
>> +
>> + 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;
> the next instruction overwrites this error. Perhaps you could
> use a goto to spin_unlock and return.
>
> Andi
>
Will do.
>> + }
>> +
>> + if (i2c->error)
>> + ret = i2c->error;
>> + spin_unlock_irqrestore(&i2c->lock, flags);
>> +
>> + return ret;
>> +}
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-10-27 2:59 ` Xianwei Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Xianwei Zhao @ 2026-10-27 2:59 UTC (permalink / raw)
To: Andi Shyti
Cc: Junyi Zhao, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Xianwei Zhao, Junyi Zhao, linux-i2c, linux-amlogic, devicetree,
linux-kernel
Hi Andi,
Thanks for your review.
On 2026/10/26 23:22, Andi Shyti wrote:
> Hi Junyi,
>
> Sashiko's comments look legit to me, can you please take a look
> at those?
>
I'm currently looking at this part. Some of them don't have relatively
good solutions yet, so I haven't replied.
> Besides...
>
>> +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;
>> + }
> No need for brackets here:
>
> if (...)
> goto ...;
> else if (...)
> goto ...;
>
Will do.
> But perhaps the two can be even merged.
I will try to merged them.
> (i2c->msg->len - i2c->pos) is equal to (i2c->pos < i2c->msg->len)
>
Will do.
>> +
>> +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);
> ...
>
>> + 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);
> I think this should also be under spinlock in order to avoid that
> the interrupt handler accesses a partially initialized state.
>
I will lock it.
>> +
>> + 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;
> the next instruction overwrites this error. Perhaps you could
> use a goto to spin_unlock and return.
>
> Andi
>
Will do.
>> + }
>> +
>> + if (i2c->error)
>> + ret = i2c->error;
>> + spin_unlock_irqrestore(&i2c->lock, flags);
>> +
>> + return ret;
>> +}
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-10-22 9:26 ` Xianwei Zhao
@ 2026-10-26 17:28 ` Markus Elfring
-1 siblings, 0 replies; 21+ messages in thread
From: Markus Elfring @ 2026-10-26 17:28 UTC (permalink / raw)
To: Junyi Zhao, Xianwei Zhao, linux-amlogic, linux-i2c, devicetree,
Andi Shyti, Conor Dooley, Krzysztof Kozlowski, Rob Herring
Cc: Junyi Zhao, Xianwei Zhao, LKML
…
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
> @@ -0,0 +1,469 @@
…
How do you think about to avoid a bit of duplicate source code?
…
> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
> +{
…
> + 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);
…
+ i2c->count = min(i2c->msg->len - i2c->pos,
+ (mode == A9_FIFO_FILL_FULL ? A9_I2C_FIFO_DEPTH : A9_I2C_HALF_FIFO));
…
> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
> +{
…
> + 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);
…
+ aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, (last ? 0 : BUS_NO_STOP));
Regards,
Markus
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-10-26 17:28 ` Markus Elfring
0 siblings, 0 replies; 21+ messages in thread
From: Markus Elfring @ 2026-10-26 17:28 UTC (permalink / raw)
To: Junyi Zhao, Xianwei Zhao, linux-amlogic, linux-i2c, devicetree,
Andi Shyti, Conor Dooley, Krzysztof Kozlowski, Rob Herring
Cc: Junyi Zhao, Xianwei Zhao, LKML
…
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
> @@ -0,0 +1,469 @@
…
How do you think about to avoid a bit of duplicate source code?
…
> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
> +{
…
> + 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);
…
+ i2c->count = min(i2c->msg->len - i2c->pos,
+ (mode == A9_FIFO_FILL_FULL ? A9_I2C_FIFO_DEPTH : A9_I2C_HALF_FIFO));
…
> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
> +{
…
> + 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);
…
+ aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, (last ? 0 : BUS_NO_STOP));
Regards,
Markus
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-10-26 17:28 ` Markus Elfring
@ 2026-10-27 3:18 ` Xianwei Zhao
-1 siblings, 0 replies; 21+ messages in thread
From: Xianwei Zhao @ 2026-10-27 3:18 UTC (permalink / raw)
To: Markus Elfring, Junyi Zhao, linux-amlogic, linux-i2c, devicetree,
Andi Shyti, Conor Dooley, Krzysztof Kozlowski, Rob Herring
Cc: Junyi Zhao, Xianwei Zhao, LKML
Hi Markus,
Thanks for your review.
On 2026/10/27 01:28, Markus Elfring wrote:
> …
>> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
>> @@ -0,0 +1,469 @@
> …
>
> How do you think about to avoid a bit of duplicate source code?
>
This one has a different IP from the existing controller. They only
share some similarities in register access, and most other logics are
incompatible. Therefore, it is not considered for the time being.
> > …
>> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
>> +{
> …
>> + 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);
> …
>
> + i2c->count = min(i2c->msg->len - i2c->pos,
> + (mode == A9_FIFO_FILL_FULL ? A9_I2C_FIFO_DEPTH : A9_I2C_HALF_FIFO));
>
>
Will do.
> …
>> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
>> +{
> …
>> + 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);
> …
>
> + aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, (last ? 0 : BUS_NO_STOP));
>
Will do.
>
> Regards,
> Markus
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-10-27 3:18 ` Xianwei Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Xianwei Zhao @ 2026-10-27 3:18 UTC (permalink / raw)
To: Markus Elfring, Junyi Zhao, linux-amlogic, linux-i2c, devicetree,
Andi Shyti, Conor Dooley, Krzysztof Kozlowski, Rob Herring
Cc: Junyi Zhao, Xianwei Zhao, LKML
Hi Markus,
Thanks for your review.
On 2026/10/27 01:28, Markus Elfring wrote:
> …
>> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
>> @@ -0,0 +1,469 @@
> …
>
> How do you think about to avoid a bit of duplicate source code?
>
This one has a different IP from the existing controller. They only
share some similarities in register access, and most other logics are
incompatible. Therefore, it is not considered for the time being.
> > …
>> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
>> +{
> …
>> + 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);
> …
>
> + i2c->count = min(i2c->msg->len - i2c->pos,
> + (mode == A9_FIFO_FILL_FULL ? A9_I2C_FIFO_DEPTH : A9_I2C_HALF_FIFO));
>
>
Will do.
> …
>> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
>> +{
> …
>> + 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);
> …
>
> + aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, (last ? 0 : BUS_NO_STOP));
>
Will do.
>
> Regards,
> Markus
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 21+ messages in thread