RE: [PATCH v9 2/2] i3c: master: Add driver for AMD AXI I3C master controller

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/[email protected]/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 <[email protected]>
Sent: Tuesday, June 23, 2026 10:16 PM
To: Patil, Shubham Sanjay <[email protected]>
Cc: git (AMD-Xilinx) <[email protected]>; Simek, Michal <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; S-k, Shyam-sundar <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Pandey, Radhey Shyam <[email protected]>; Goud, Srinivas <[email protected]>; Datta, Shubhrajyoti <[email protected]>; Guntupalli, Manikanta <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Shubhrajyoti Datta <[email protected]>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> Co-developed-by: Shubham Patil <[email protected]>
> Signed-off-by: Shubham Patil <[email protected]>
> ---
...
> +#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/[email protected]/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;
> +}

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c