Re: [PATCH v32 4/5] i2c: ast2600: Add controller driver for AST2600 new register set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver fails to send a NAK on the last byte of an intermediate read message, causing the slave to hold SDA low and block the subsequent Repeated Start.
- [High] The interrupt handler does not clear error status bits in the Write-1-to-Clear (W1C) ISR register, permanently wedging the controller after a single error.
- [Medium] Bus recovery failure and other error interrupts are not enabled in the Interrupt Enable Register (IER), leading to needless blocking timeouts.
--

commit 01c0588191421a92b3d96b7bc246be1fe0958d9f
Author: Ryan Chen <ryan_chen@aspeedtech.com>

i2c: ast2600: Add controller driver for AST2600 new register set

The AST2600 introduces a new I2C controller register layout, selectable
at runtime via global control registers. Compared to the legacy layout
used on AST2400/AST2500, the new layout separates controller (master)
and target (slave) registers and adds support for packet-based transfers

> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ast2600.c

[ ... ]

> +static int ast2600_i2c_setup_buff_rx(u32 cmd, struct ast2600_i2c_bus *i2c_bus)
> +{
> +	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> +	int xfer_len = msg->len - i2c_bus->controller_xfer_cnt;
> +
> +	cmd |= AST2600_I2CM_PKT_EN | AST2600_I2CM_RX_BUFF_EN | AST2600_I2CM_RX_CMD;
> +
> +	if (cmd & AST2600_I2CM_START_CMD)
> +		cmd |= AST2600_I2CM_PKT_ADDR(msg->addr);
> +
> +	if (msg->flags & I2C_M_RECV_LEN) {
> +		dev_dbg(i2c_bus->dev, "smbus read\n");
> +		xfer_len = 1;
> +	} else if (xfer_len > i2c_bus->buf_size) {
> +		xfer_len = i2c_bus->buf_size;
> +	} else if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) {
> +		cmd |= CONTROLLER_TRIGGER_LAST_STOP;
> +	}

[Severity: High]
Does this logic omit the NAK for intermediate read messages?

The CONTROLLER_TRIGGER_LAST_STOP flag (which applies the hardware NAK via 
AST2600_I2CM_RX_CMD_LAST) is strictly tied to the absolute final message in
the array. If the transfer contains an intermediate read message, such as a
read before a Repeated Start, the last byte will be ACKed.

This will cause the slave to expect another byte and hold SDA low, leading to
an arbitration loss or bus hang when attempting the Repeated Start.

[ ... ]

> +static int ast2600_i2c_controller_irq(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
> +	u32 ctrl;
> +
> +	sts &= ~AST2600_I2CM_SMBUS_ALERT;
> +
> +	if (sts & AST2600_I2CM_BUS_RECOVER_FAIL) {
> +		writel(AST2600_I2CM_BUS_RECOVER_FAIL, i2c_bus->reg_base + AST2600_I2CM_ISR);
> +		ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +		writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +		WRITE_ONCE(i2c_bus->msgs, NULL);
> +		i2c_bus->cmd_err = -EPROTO;
> +		complete(&i2c_bus->cmd_complete);
> +		return 1;
> +	}
> +
> +	if (sts & AST2600_I2CM_BUS_RECOVER) {
> +		writel(AST2600_I2CM_BUS_RECOVER, i2c_bus->reg_base + AST2600_I2CM_ISR);
> +		WRITE_ONCE(i2c_bus->msgs, NULL);
> +		i2c_bus->cmd_err = 0;
> +		complete(&i2c_bus->cmd_complete);
> +		return 1;
> +	}
> +
> +	i2c_bus->cmd_err = ast2600_i2c_irq_err_to_errno(sts);
> +	if (i2c_bus->cmd_err) {
> +		WRITE_ONCE(i2c_bus->msgs, NULL);
> +		writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base + AST2600_I2CM_ISR);
> +		complete(&i2c_bus->cmd_complete);
> +		return 1;
> +	}

[Severity: High]
Will this permanently wedge the controller after a single error?

When an error occurs, the error path explicitly writes only
AST2600_I2CM_PKT_DONE to the ISR. W1C error bits like AST2600_I2CM_ARBIT_LOSS
or AST2600_I2CM_TX_NAK are never written back and will remain set.

Because the error path calls complete() successfully, the timeout fallback
path that could reset the controller is bypassed. On the very next I2C
transaction, a new PKT_DONE interrupt will cause the handler to read the
stale error bits, instantly fail the transaction, and loop in this state.

The same issue appears to exist in ast2600_i2c_controller_packet_irq() for
bits like AST2600_I2CM_PKT_ERROR.

[ ... ]

> +static int ast2600_i2c_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
> +			       dev_name(dev), i2c_bus);
> +	if (ret < 0) {
> +		ret = dev_err_probe(dev, ret, "Unable to request irq %d\n",
> +				    i2c_bus->irq);
> +		goto err;
> +	}
> +
> +	writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> +	       i2c_bus->reg_base + AST2600_I2CM_IER);
> +
> +	ret = i2c_add_adapter(&i2c_bus->adap);

[Severity: Medium]
Are bus recovery failure interrupts intentionally left disabled?

The interrupt enable register is configured here without including the
AST2600_I2CM_BUS_RECOVER_FAIL bit. If a bus recovery attempt fails, the
hardware will assert the bit in the ISR, but the interrupt handler will
never be triggered.

This will cause the driver to needlessly block for the entire duration of
wait_for_completion_timeout() instead of handling the failure immediately.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-upstream_i2c-v32-0-b66eba921d01@aspeedtech.com?part=4