[PATCH v9 2/2] i3c: master: Add driver for AMD AXI I3C master controller
Patil, Shubham Sanjay
ShubhamSanjay.Patil at amd.com
Wed Jul 1 02:52:04 PDT 2026
AMD General
Hi Frank,
Thanks for the review. Responses inline; all the mechanical points are addressed in v10.
> > +#define XI3C_OD_TLOW_NS 500000
> > +#define XI3C_OD_THIGH_NS 41000
> > +#define XI3C_I2C_TCASMIN_NS 600000
> > +#define XI3C_TCASMIN_NS 260000
[...]
> > +#define XI3C_THOLD_MIN_REV0 5
> > +#define XI3C_THOLD_MIN_REV1 6
> > +#define XI3C_CYCLE_ADJUST 2
> > +#define XI3C_FIFO_RESET_DELAY_US 10
> > +#define XI3C_POLL_INTERVAL_US 10
>
> Can you provide comment where these value come from, spec, datasheet ...?
Sources are now cited in the comments (MIPI I3C v1.1.1 tables and the AMD PG439 register sections/table numbers)
PG439: https://docs.amd.com/r/en-US/pg439-axi-i3c/Introduction
> > + ret = i3c_master_add_i3c_dev_locked(m, master->daa.addrs[i]);
> > + if (ret)
> > + goto err_daa;
>
> https://lore.kernel.org/linux-i3c/20260608054312.10604-7-adrian.hunter@intel.com/T/#u
> which defer add i3c device.
> And don't check error here, because one device add failure should not
> impact other following devices.
Agreed. The driver now just calls i3c_master_add_i3c_dev_locked() without checking its return, so one device's failure can't block the others. The registration deferral itself is handled by the core (Adrian Hunter's series); the driver doesn't register devices inline, so no driver-side change is needed beyond dropping the error check.
> > + while (cmd->rx_len > 0 && !xi3c_is_resp_available(master)) {
> > + ...
> > + xi3c_master_rd_from_rx_fifo(master, cmd);
> > + usleep_range(...);
> > + }
>
> can you use read_poll_timeout macro?
This loop isn't really a poll, so read_poll_timeout() doesn't map cleanly onto it. The loop body has a side effect rather than reading a status value: xi3c_master_rd_from_rx_fifo() drains the RX FIFO into the caller's buffer on every iteration (advancing rx_buf, decrementing rx_len). The exit condition is also compound - we stop when either the buffer is satisfied or a response word arrives - and there's a mandatory final drain after the loop to pull out any bytes left in the FIFO once the response shows up. Expressing that with read_poll_timeout()'s op/val/cond model would force the drain helper to return a value just to satisfy the macro, which reads worse than the explicit loop.
The genuine poll just above this - waiting for the first RD_FIFO_NOT_EMPTY / RESP_NOT_EMPTY - already uses readl_poll_timeout(). I'd prefer to keep the data-draining loop explicit, but I'm happy to revisit if you see a clean way to fit it to the macro.
wwr,
Shubham
-----Original Message-----
From: Frank Li <Frank.li at oss.nxp.com>
Sent: Tuesday, June 23, 2026 10:16 PM
To: Patil, Shubham Sanjay <ShubhamSanjay.Patil at amd.com>
Cc: git (AMD-Xilinx) <git at amd.com>; Simek, Michal <michal.simek at amd.com>; alexandre.belloni at bootlin.com; Frank.Li at nxp.com; robh at kernel.org; krzk+dt at kernel.org; conor+dt at kernel.org; pgaj at cadence.com; wsa+renesas at sang-engineering.com; tommaso.merciai.xr at bp.renesas.com; arnd at arndb.de; quic_msavaliy at quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k at amd.com>; sakari.ailus at linux.intel.com; billy_tsai at aspeedtech.com; kees at kernel.org; gustavoars at kernel.org; jarkko.nikula at linux.intel.com; jorge.marques at analog.com; linux-i3c at lists.infradead.org; devicetree at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arch at vger.kernel.org; linux-hardening at vger.kernel.org; Pandey, Radhey Shyam <radhey.shyam.pandey at amd.com>; Goud, Srinivas <srinivas.goud at amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta at amd.com>; Guntupalli, Manikanta <manikanta.guntupalli at amd.com>
Subject: Re: [PATCH v9 2/2] i3c: master: Add driver for AMD AXI I3C master controller
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
On Tue, Jun 23, 2026 at 05:14:16PM +0530, Shubham Patil wrote:
> From: Manikanta Guntupalli <manikanta.guntupalli at amd.com>
>
> Add an I3C master driver and maintainers fragment for the AMD I3C bus
> controller.
>
> The driver currently supports the I3C bus operating in SDR mode, with
> features including Dynamic Address Assignment, private data transfers,
> and CCC transfers in both broadcast and direct modes. It also supports
> operation in I2C mode.
>
> The controller's data FIFOs are accessed big-endian; the driver
> performs this conversion locally using ioread32be()/iowrite32be() with
> the helpers, so it does not depend on any core FIFO-endianness helpers.
>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli at amd.com>
> Co-developed-by: Shubhrajyoti Datta <shubhrajyoti.datta at amd.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta at amd.com>
> Co-developed-by: Shubham Patil <shubhamsanjay.patil at amd.com>
> Signed-off-by: Shubham Patil <shubhamsanjay.patil at amd.com>
> ---
...
> +#define XI3C_REV_NUM_MASK GENMASK(15, 8)
> +#define XI3C_PID1_MASK GENMASK(15, 0)
> +#define XI3C_FIFO_LEVEL_MASK GENMASK(15, 0)
> +#define XI3C_RESP_CODE_MASK GENMASK(8, 5)
> +#define XI3C_RESP_CODE_SUCCESS 0 /* Transfer completed OK */
> +#define XI3C_RESP_CODE_NO_TARGET 2 /* 7E NACK: no target on bus */
> +#define XI3C_RESP_CODE_NACK 3 /* Target NACK / CE2 / DAA end */
> +#define XI3C_ADDR_MASK GENMASK(6, 0)
> +#define XI3C_FIFOS_RST_MASK GENMASK(4, 1)
> +
> +/* Command FIFO word layout (bit ranges encoded in the GENMASK/BIT args) */
> +#define XI3C_CMD_TYPE GENMASK(3, 0) /* command type */
> +#define XI3C_CMD_TERMINATE BIT(4) /* terminate (last cmd of xfer) */
> +#define XI3C_CMD_ADDR GENMASK(15, 8) /* target address << 1 | RnW */
> +#define XI3C_CMD_LEN GENMASK(27, 16) /* payload length in bytes */
> +#define XI3C_CMD_TID GENMASK(31, 28) /* transfer ID */
> +
> +#define XI3C_OD_TLOW_NS 500000
> +#define XI3C_OD_THIGH_NS 41000
> +#define XI3C_I2C_TCASMIN_NS 600000
> +#define XI3C_TCASMIN_NS 260000
> +#define XI3C_MAXDATA_LENGTH 4095
> +#define XI3C_MAX_DEVS 32
> +#define XI3C_DAA_SLAVEINFO_READ_BYTECOUNT 8
> +
> +#define XI3C_THOLD_MIN_REV0 5 /* Min SDA hold cycles, rev 0 IP */
> +#define XI3C_THOLD_MIN_REV1 6 /* Min SDA hold cycles, rev >= 1 IP */
> +#define XI3C_CYCLE_ADJUST 2 /* SCL/SDA pre-bias for HW pipeline */
> +#define XI3C_FIFO_RESET_DELAY_US 10 /* HW settling time after FIFO reset */
> +#define XI3C_POLL_INTERVAL_US 10 /* readl_poll_timeout() sleep slice */
Can you provide comment where these value come from, spec, datasheet ...?
> +
> +#define XI3C_I2C_MODE 0
> +#define XI3C_I2C_TID 0
> +#define XI3C_SDR_MODE 1
> +#define XI3C_SDR_TID 1
> +
> +#define XI3C_WORD_LEN 4
> +
> +/*
> + * XI3C_RESP_TIMEOUT_US is in microseconds because it is passed as
> +the
> + * timeout_us argument of readl_poll_timeout(). XI3C_XFER_TIMEOUT_MS
> +is in
> + * milliseconds because it feeds msecs_to_jiffies(). Keep the two
> +units
> + * distinct in the names so callers cannot mix them up.
> + */
> +#define XI3C_RESP_TIMEOUT_US 500000
> +#define XI3C_XFER_TIMEOUT_MS 1000
the same here.
> +
> +struct xi3c_cmd {
> + const void *tx_buf;
> + void *rx_buf;
> + u16 tx_len;
> + u16 rx_len;
> + u8 addr;
> + u8 type;
> + u8 tid;
> + bool rnw;
> + bool is_daa;
> + bool continued;
> + enum i3c_error_code err;
> +};
> +
...
> +
> +static void xi3c_master_reset_fifos(struct xi3c_master *master) {
> + u32 data;
> +
> + /* Assert FIFO reset. */
> + data = ioread32(master->membase + XI3C_RESET_OFFSET);
> + data |= XI3C_FIFOS_RST_MASK;
> + iowrite32(data, master->membase + XI3C_RESET_OFFSET);
> + /* Read-back flushes the posted write before the settling delay below. */
> + ioread32(master->membase + XI3C_RESET_OFFSET);
> + udelay(XI3C_FIFO_RESET_DELAY_US);
now suggest use fsleep()
> +
> + /* De-assert FIFO reset, then wait for the FIFOs to come back up. */
> + data &= ~XI3C_FIFOS_RST_MASK;
> + iowrite32(data, master->membase + XI3C_RESET_OFFSET);
> + ioread32(master->membase + XI3C_RESET_OFFSET);
> + udelay(XI3C_FIFO_RESET_DELAY_US); }
> +
> +static inline void xi3c_master_init(struct xi3c_master *master) {
> + /* Reset fifos */
> + xi3c_master_reset_fifos(master);
> +
> + /* Enable controller */
> + xi3c_master_enable(master);
> +}
> +
> +static inline void xi3c_master_reinit(struct xi3c_master *master) {
> + /* Reset fifos */
> + xi3c_master_reset_fifos(master);
> +
> + /* Resume controller */
> + xi3c_master_resume(master);
> +}
> +
> +static struct xi3c_xfer *xi3c_master_alloc_xfer(unsigned int ncmds) {
> + struct xi3c_xfer *xfer;
> +
> + xfer = kzalloc(struct_size(xfer, cmds, ncmds), GFP_KERNEL);
ues new API, kzalloc_flex()
> + if (!xfer)
> + return NULL;
> +
> + xfer->ncmds = ncmds;
> +
> + return xfer;
> +}
> +
> +static void xi3c_master_rd_from_rx_fifo(struct xi3c_master *master,
> + struct xi3c_cmd *cmd) {
> + u16 rx_data_available;
> + u16 copy_len;
> + u16 len;
> +
> + rx_data_available = xi3c_rd_fifo_level(master);
> + len = rx_data_available * XI3C_WORD_LEN;
> +
> + if (!len)
> + return;
> +
> + copy_len = min_t(u16, len, cmd->rx_len);
now need't min_t, just min
> + xi3c_readl_fifo(master->membase + XI3C_RD_FIFO_OFFSET,
> + (u8 *)cmd->rx_buf, copy_len);
> +
> + cmd->rx_buf = (u8 *)cmd->rx_buf + copy_len;
> + cmd->rx_len -= copy_len;
> +}
> +
...
> +
> + timeout = jiffies + msecs_to_jiffies(XI3C_XFER_TIMEOUT_MS);
> +
> + /* Read data from rx fifo */
> + while (cmd->rx_len > 0 && !xi3c_is_resp_available(master)) {
> + if (time_after(jiffies, timeout)) {
> + dev_err(master->dev, "XI3C read timeout\n");
> + return -EIO;
> + }
> + xi3c_master_rd_from_rx_fifo(master, cmd);
> + usleep_range(XI3C_POLL_INTERVAL_US, 2 * XI3C_POLL_INTERVAL_US);
> + }
can you use read_poll_timeout macro?
> +
> + /* Read remaining data */
> + xi3c_master_rd_from_rx_fifo(master, cmd);
> +
> + return 0;
> +}
> +
...
> +
> + for (i = 0; i < master->daa.index; i++) {
> + u64 pid;
> +
> + ret = i3c_master_add_i3c_dev_locked(m, master->daa.addrs[i]);
> + if (ret)
> + goto err_daa;
https://lore.kernel.org/linux-i3c/20260608054312.10604-7-adrian.hunter@intel.com/T/#u
which defer add i3c device.
And don't check error here, because one device add failure should not impact other following devices.
Frank
> +
> + pid = FIELD_GET(XI3C_PID_MASK,
> + get_unaligned_be64(pid_bufs[i]));
> + dev_dbg(master->dev, "Client %d: PID: 0x%llx\n", i, pid);
> + }
> +
> + return 0;
> +
> +err_daa:
> + xi3c_master_reinit(master);
> + return ret;
> +}
> +
...
> +static int xi3c_master_send_bdcast_ccc_cmd(struct xi3c_master *master,
> + struct i3c_ccc_cmd *ccc) {
> + struct xi3c_xfer *xfer __free(kfree) = NULL;
> + u8 *buf __free(kfree) = NULL;
> + struct xi3c_cmd *cmd;
> + u16 xfer_len;
> + int ret;
> +
> + if (ccc->dests[0].payload.len >= XI3C_MAXDATA_LENGTH)
> + return -EINVAL;
> +
> + xfer_len = ccc->dests[0].payload.len + 1;
> +
> + xfer = xi3c_master_alloc_xfer(1);
> + if (!xfer)
> + return -ENOMEM;
> +
> + buf = kmalloc(xfer_len, GFP_KERNEL);
kmalloc_obj
Frank
> + if (!buf)
> + return -ENOMEM;
> +
> + buf[0] = ccc->id;
> + memcpy(&buf[1], ccc->dests[0].payload.data,
> + ccc->dests[0].payload.len);
> +
> + cmd = &xfer->cmds[0];
> + cmd->addr = ccc->dests[0].addr;
> + cmd->rnw = ccc->rnw;
> + cmd->tx_buf = buf;
> + cmd->tx_len = xfer_len;
> + cmd->type = XI3C_SDR_MODE;
> + cmd->tid = XI3C_SDR_TID;
> + cmd->continued = false;
> +
> + ret = xi3c_master_common_xfer(master, xfer);
> + ccc->err = cmd->err;
> +
> + return ret;
> +}
More information about the linux-i3c
mailing list