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

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