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

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

Hi,

On 2026/11/10 16:41, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Medium] i2c: amlogic-a9: transfer started before IRQ setup in
>    aml_i2c_do_start()
> - [Medium] i2c: amlogic-a9: stale irq_state and edge IRQ miss in
>    aml_i2c_irq()
> --
> 
> commit 75985e9b577bcde7a76a8a6f844a5a7564d2426a
> 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 its
> 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..3e08a0333ceea
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
> [ ... ]
> 
>> +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
>> +{
>> +     struct aml_i2c *i2c = dev_id;
>> +     u8 *buf = NULL;
>> +     u32 rd_len, i;
>> +     u32 rd_val;
>> +     u32 irq_state;
>> +
>> +     spin_lock(&i2c->lock);
>> +     irq_state = readl(i2c->regs + REG_CFG_IRQ_STATE);
>> +     if (i2c->state == A9_STATE_IDLE) {
>> +             dev_warn(i2c->dev, "i2c irq triggered abnormally, irq_state:0x%x,irq_en:0x%x\n",
>> +                      irq_state, readl(i2c->regs + REG_CFG_IRQ_ENABLE));
>> +             goto irq_trans_out;
>> +     }
>> +
>> +     if (irq_state & A9_TRANS_ERROR) {
>> +             i2c->error = -ENXIO;
>> +             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 == i2c->msg->len)
>> +                     aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_TX_THRESH_WRITE, 0);
>> +             /* Tx must clear thresh after write fifo */
>> +             clear_irq_status(i2c, A9_TX_THRESH_WRITE);
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +
>> +     if (!(irq_state & A9_TRANS_DONE) ||
>> +         (i2c->state == A9_STATE_READ && i2c->pos < i2c->msg->len)) {
>> +             spin_unlock(&i2c->lock);
>> +             return IRQ_HANDLED;
>> +     }
> [Severity: Medium]
> Can multi-chunk reads (msg->len > A9_I2C_FIFO_DEPTH) miss the completion
> interrupt and time out in aml_i2c_xfer_msg()?
> 
> Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml:
>          interrupts = <0 164 1>;
> 
> The interrupt is rising-edge-triggered (IRQ_TYPE_EDGE_RISING), so the
> controller's enabled interrupt output must drop to 0 and transition back to 1
> to latch a new interrupt in the GIC.
> 
> In aml_i2c_irq(), irq_state is sampled once before draining up to 32 bytes
> from REG_CFG_RX.  On a 64-byte read, the 32nd byte of the second chunk asserts
> A9_RX_THRESH_READ before the final NACK and STOP finish on the bus.  If
> A9_PHY_DONE asserts while the handler is reading REG_CFG_RX, prior to
> clear_irq_status(i2c, A9_RX_THRESH_READ), the interrupt output stays
> continuously high across the clear without generating a new rising edge, while
> aml_i2c_irq() checks the stale irq_state snapshot and returns without
> completing i2c->done.
> 
> Similarly, if both A9_RX_THRESH_READ and A9_PHY_DONE are already set in
> irq_state while more than 32 bytes remain to be read, aml_i2c_irq() reads 32
> bytes, clears only A9_RX_THRESH_READ, and returns early because i2c->pos <
> i2c->msg->len.  Because A9_PHY_DONE remains asserted in REG_CFG_IRQ_STATE and
> fewer than 32 tail bytes remain in the FIFO, no new rising edge is generated
> and wait_for_completion_timeout() hangs for 500 ms before returning
> -ETIMEDOUT.
> 
As described in this v2

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.
>> +
>> +complete_irq_out:
>> +     i2c->state = A9_STATE_IDLE;
>> +     reset_fifo_pos(i2c);
>> +     aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, 0);
>> +     complete(&i2c->done);
>> +irq_trans_out:
>> +     clear_irq_status(i2c, IRQ_ALL_MASK);
>> +     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]
> Does writing START_START to REG_CFG_START before clearing IRQ_ALL_MASK and
> enabling threshold interrupts race with hardware completion or NACK?
> 
> If execution is delayed between writing REG_CFG_START and calling
> clear_irq_status(i2c, IRQ_ALL_MASK) (for example due to vCPU preemption or
> PREEMPT_RT spinlock preemption), an address NACK or short transfer can assert
> A9_NCK_ERROR or A9_PHY_DONE and then have its status bits wiped out by
> clear_irq_status().  When aml_i2c_irq() then runs, it reads irq_state == 0,
> misses A9_TRANS_ERROR or A9_TRANS_DONE, and returns without completing
> i2c->done, causing wait_for_completion_timeout() to time out.  Clearing
> REG_CFG_IRQ_STATE and configuring REG_CFG_IRQ_ENABLE before asserting
> START_START avoids this window.
As described in this v3,

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


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