Re: [PATCH v3 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

hi,

On 2026/10/29 16:48, sashiko-bot@kernel.org wrote:
> 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@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)?
> 

Will add flag I2C_AQ_NO_ZERO_LEN in aml_i2c_quirks.

>> +     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?
> 

This is a hardware-related issue. Some status bits can only be cleared 
during startup. Changing the initialization sequence may cause data 
transmission issues.

>> +
>> +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?

Will add clear A9_RX_THRESH_READ | A9_TX_THRESH_WRITE enable bit after 
timeout.

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic