[PATCH v3 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Thu Oct 29 01:48:39 PDT 2026
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: amlogic-a9: missing threshold IRQ disable on timeout in aml_i2c_xfer_msg()
- [Medium] i2c: amlogic-a9: transfer started before IRQ setup in aml_i2c_do_start()
- [Medium] i2c: amlogic-a9: spurious TX FIFO write on completion in aml_i2c_irq()
--
commit ab8bf627f35c4931f8163555b700d4cca01277db
Author: Junyi Zhao <junyi.zhao at amlogic.com>
i2c: amlogic: Add Amlogic A9 I2C controller driver
This commit adds an I2C controller driver for Amlogic A9 SoCs, along with
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..503511734fe26
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
[ ... ]
> +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;
> +
> + i2c->count = min(i2c->msg->len - i2c->pos,
> + mode == A9_FIFO_FILL_FULL ? A9_I2C_FIFO_DEPTH : 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)
> +{
[ ... ]
> + 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;
[Severity: Medium]
When a zero-length write message (i2c->msg->len == 0) is submitted via
aml_i2c_xfer() -> aml_i2c_xfer_msg(), aml_i2c_prepare_xfer() calls
aml_i2c_put_data() and writes 0x00 to REG_CFG_TX before starting the
transfer.
When the transfer finishes and A9_PHY_DONE triggers aml_i2c_irq(), the
A9_STATE_WRITE case runs unconditionally without checking if bytes remain to
be written or if A9_TX_THRESH_WRITE is set. Because i2c->msg->len is still 0,
aml_i2c_prepare_xfer() -> aml_i2c_put_data() writes a second dummy 0x00 byte
to REG_CFG_TX during completion.
Also, given the comment in aml_i2c_put_data() noting that the hardware must
send at least 1 byte when transferring 0 bytes, does a 0-byte transfer
actually transmit a data byte on the bus? If the controller cannot perform
true zero-length transfers, should aml_i2c_quirks set I2C_AQ_NO_ZERO_LEN
since aml_i2c_func() advertises I2C_FUNC_SMBUS_EMUL (which includes SMBus
Quick commands)?
> + default:
> + break;
> + }
> +
> + if (!(irq_state & A9_TRANS_DONE) ||
> + (i2c->state == A9_STATE_READ && i2c->pos < i2c->msg->len))
> + goto irq_trans_out;
> +
> +complete_irq_out:
> + i2c->state = A9_STATE_IDLE;
> + reset_fifo_pos(i2c);
> + complete(&i2c->done);
> + 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 & 0x7f) << 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);
> + }
> +}
[Severity: Medium]
In aml_i2c_do_start(), START_START is written to REG_CFG_START before
clear_irq_status(i2c, IRQ_ALL_MASK) is called and before the threshold
interrupts are enabled in REG_CFG_IRQ_ENABLE.
If execution is delayed between writing REG_CFG_START and clearing
REG_CFG_IRQ_STATE (for instance on PREEMPT_RT where spin_lock_irqsave() does
not disable preemption, or due to an NMI or hypervisor preemption), can the
hardware assert A9_NCK_ERROR or A9_PHY_DONE for the new transfer only for
clear_irq_status(i2c, IRQ_ALL_MASK) to immediately wipe it out, causing
aml_i2c_irq() to miss A9_TRANS_DONE and wait_for_completion_timeout() in
aml_i2c_xfer_msg() to time out?
> +
> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
> +{
[ ... ]
> + if (!time_left) {
> + spin_lock_irqsave(&i2c->lock, flags);
> + i2c->state = A9_STATE_IDLE;
> + clear_irq_status(i2c, IRQ_ALL_MASK);
> + reset_fifo_pos(i2c);
> + spin_unlock_irqrestore(&i2c->lock, flags);
> + return -ETIMEDOUT;
> + }
[Severity: High]
When a transfer with msg->len > A9_I2C_FIFO_DEPTH (32 bytes) is started,
aml_i2c_do_start() enables A9_RX_THRESH_READ or A9_TX_THRESH_WRITE in
REG_CFG_IRQ_ENABLE.
If that transfer times out in aml_i2c_xfer_msg(), this timeout path resets
i2c->state, clears REG_CFG_IRQ_STATE, and resets the FIFO pointers, but
unlike complete_irq_out in aml_i2c_irq(), it does not disable
A9_RX_THRESH_READ | A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE, nor does it
clear START_START in REG_CFG_START to halt the controller.
When a subsequent transfer with msg->len <= A9_I2C_FIFO_DEPTH runs,
aml_i2c_do_start() skips updating REG_CFG_IRQ_ENABLE:
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);
}
Because aml_i2c_irq() dispatches strictly on i2c->state without checking
which bit in irq_state triggered the interrupt, won't a stale
A9_TX_THRESH_WRITE interrupt left enabled from a timed-out write fire during
a subsequent <= 32-byte read (i2c->state == A9_STATE_READ), causing
aml_i2c_irq() to read from an empty REG_CFG_RX FIFO into i2c->msg->buf while
leaving A9_TX_THRESH_WRITE uncleared?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-a9-i2c-v3-0-7b30ccc00533@amlogic.com?part=2
More information about the linux-amlogic
mailing list