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; > +}
