[PATCH V5 2/2] i3c: master: Add AMD I3C bus controller driver
Guntupalli, Manikanta
manikanta.guntupalli at amd.com
Tue Sep 9 03:50:02 PDT 2025
[Public]
Hi,
> -----Original Message-----
> From: Frank Li <Frank.li at nxp.com>
> Sent: Monday, September 8, 2025 9:34 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli at amd.com>
> Cc: git (AMD-Xilinx) <git at amd.com>; Simek, Michal <michal.simek at amd.com>;
> alexandre.belloni at bootlin.com; robh at kernel.org; krzk+dt at kernel.org;
> conor+dt at kernel.org; kees at kernel.org; gustavoars at kernel.org;
> jarkko.nikula at linux.intel.com; linux-i3c at lists.infradead.org;
> devicetree at vger.kernel.org; linux-kernel 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>; manion05gk at gmail.com
> Subject: Re: [PATCH V5 2/2] i3c: master: Add AMD I3C bus controller driver
>
> On Mon, Sep 08, 2025 at 04:51:17PM +0530, Manikanta Guntupalli wrote:
> > Add an I3C master driver and maintainers fragment for the AMD I3C bus
> > controller.
> >
> > The driver currently supports the I3C bus operating in SDR i3c 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.
> >
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli at amd.com>
> > ---
> > Changes for V2:
> > Updated commit description.
> > Added mixed mode support with clock configuration.
> > Converted smaller functions into inline functions.
> > Used FIELD_GET() in xi3c_get_response().
> > Updated xi3c_master_rd_from_rx_fifo() to use cmd->rx_buf.
> > Used parity8() for address parity calculation.
> > Added guards for locks.
> > Dropped num_targets and updated xi3c_master_do_daa().
> > Used __free(kfree) in xi3c_master_send_bdcast_ccc_cmd().
> > Dropped PM runtime support.
> > Updated xi3c_master_read() and xi3c_master_write() with
> > xi3c_is_resp_available() check.
> > Created separate functions: xi3c_master_init() and xi3c_master_reinit().
> > Used xi3c_master_init() in bus initialization and xi3c_master_reinit()
> > in error paths.
> > Added DAA structure to xi3c_master structure.
> >
> > Changes for V3:
> > Resolved merge conflicts.
> >
> > Changes for V4:
> > Updated timeout macros.
> > Removed type casting for xi3c_is_resp_available() macro.
> > Used ioread32() and iowrite32() instead of readl() and writel() to
> > keep consistency.
> > Read XI3C_RESET_OFFSET reg before udelay().
> > Removed xi3c_master_free_xfer() and directly used kfree().
> > Skipped checking return value of i3c_master_add_i3c_dev_locked().
> > Used devm_mutex_init() instead of mutex_init().
> >
> > Changes for V5:
> > Used GENMASK_ULL for PID mask as it's 64bit mask.
> > ---
> > MAINTAINERS | 7 +
> > drivers/i3c/master/Kconfig | 16 +
> > drivers/i3c/master/Makefile | 1 +
> > drivers/i3c/master/amd-i3c-master.c | 1014
> > +++++++++++++++++++++++++++
> > 4 files changed, 1038 insertions(+)
> > create mode 100644 drivers/i3c/master/amd-i3c-master.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 1af81124bba3..ff603ce5e78d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11693,6 +11693,13 @@ L: linux-i2c at vger.kernel.org
> > S: Maintained
> > F: drivers/i2c/i2c-stub.c
> >
> > +I3C DRIVER FOR AMD AXI I3C MASTER
> > +M: Manikanta Guntupalli <manikanta.guntupalli at amd.com>
> > +R: Michal Simek <michal.simek at amd.com>
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/i3c/xlnx,axi-i3c.yaml
> > +F: drivers/i3c/master/amd-i3c-master.c
> > +
> > I3C DRIVER FOR ASPEED AST2600
> > M: Jeremy Kerr <jk at codeconstruct.com.au>
> > S: Maintained
> > diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
> > index 13df2944f2ec..4b884a678893 100644
> > --- a/drivers/i3c/master/Kconfig
> > +++ b/drivers/i3c/master/Kconfig
> > @@ -1,4 +1,20 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config AMD_I3C_MASTER
> > + tristate "AMD I3C Master driver"
> > + depends on I3C
> > + depends on HAS_IOMEM
> > + help
> > + Support for AMD I3C Master Controller.
> > +
> > + This driver allows communication with I3C devices using the AMD
> > + I3C master, currently supporting Standard Data Rate (SDR) mode.
> > + Features include Dynamic Address Assignment, private transfers,
> > + and CCC transfers in both broadcast and direct modes.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called amd-i3c-master.
> > +
> > config CDNS_I3C_MASTER
> > tristate "Cadence I3C master driver"
> > depends on HAS_IOMEM
> > diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile
> > index aac74f3e3851..109bd48cb159 100644
> > --- a/drivers/i3c/master/Makefile
> > +++ b/drivers/i3c/master/Makefile
> > @@ -1,4 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_AMD_I3C_MASTER) += amd-i3c-master.o
> > obj-$(CONFIG_CDNS_I3C_MASTER) += i3c-master-cdns.o
> > obj-$(CONFIG_DW_I3C_MASTER) += dw-i3c-master.o
> > obj-$(CONFIG_AST2600_I3C_MASTER) += ast2600-i3c-master.o
> > diff --git a/drivers/i3c/master/amd-i3c-master.c
> > b/drivers/i3c/master/amd-i3c-master.c
> > new file mode 100644
> > index 000000000000..fb768680df45
> > --- /dev/null
> > +++ b/drivers/i3c/master/amd-i3c-master.c
> ...
> > +/* timeout waiting for the controller finish transfers */
> > +#define XI3C_XFER_TIMEOUT_MS 10000
> > +#define XI3C_XFER_TIMEOUT
> (msecs_to_jiffies(XI3C_XFER_TIMEOUT_MS))
>
> XI3C_XFER_TIMEOUT is not used.
XI3C_XFER_TIMEOUT is used in xi3c_master_read() and xi3c_master_write().
Please check.
>
> > +
> > +#define xi3c_getrevisionnumber(master) \
> > + ((u8)(FIELD_GET(XI3C_REV_NUM_MASK, \
> > + ioread32((master)->membase + XI3C_VERSION_OFFSET))))
> > +
> > +#define xi3c_wrfifolevel(master) \
> > + ((u16)(ioread32((master)->membase + XI3C_FIFO_LVL_STATUS_OFFSET)
> & \
> > + XI3C_WR_FIFO_LEVEL_MASK))
> > +
> > +#define xi3c_rdfifolevel(master) \
> > + ((u16)(ioread32((master)->membase +
> XI3C_FIFO_LVL_STATUS_1_OFFSET) & \
> > + XI3C_WR_FIFO_LEVEL_MASK))
>
> Are you sure that these force type convert is neccesary?
We will fix.
>
> > +
> > +#define xi3c_is_resp_available(master) \
> > + (FIELD_GET(XI3C_SR_RESP_NOT_EMPTY_MASK,
> \
> > + ioread32((master)->membase + XI3C_SR_OFFSET)))
> > +
> > +struct xi3c_cmd {
> > + 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;
> > +};
> > +
> > +struct xi3c_xfer {
> > + struct list_head node;
> > + struct completion comp;
> > + int ret;
> > + unsigned int ncmds;
> > + struct xi3c_cmd cmds[] __counted_by(ncmds); };
> > +
> ...
> > +
> > +static int xi3c_master_write(struct xi3c_master *master, struct
> > +xi3c_cmd *cmd) {
> > + unsigned long timeout;
> > + u16 wrfifo_space;
> > + u16 space_index;
> > + u16 len;
> > +
> > + len = cmd->tx_len;
> > + if (!cmd->tx_buf || cmd->tx_len > XI3C_MAXDATA_LENGTH)
> > + return -EINVAL;
> > +
> > + /* Fill Tx fifo */
> > + wrfifo_space = xi3c_wrfifolevel(master);
> > + for (space_index = 0; space_index < wrfifo_space && cmd->tx_len > 0;
> > + space_index++)
> > + xi3c_master_wr_to_tx_fifo(master, cmd);
>
> Does common helper function i3c_writel_fifo() help this? look like logic is similar.
No. The functions i3c_writel_fifo() and i3c_readl_fifo() operate using the CPU's native endianness.
In this case, the FIFO should always be accessed in big-endian format.
Therefore, we cannot use these common helpers directly.
>
> > +
> > + /* Write to command fifo */
> > + xi3c_master_write_to_cmdfifo(master, cmd, len);
> > +
> > + timeout = jiffies + XI3C_XFER_TIMEOUT;
> > + /* Fill if any remaining data to tx fifo */
> > + while (cmd->tx_len > 0 && !xi3c_is_resp_available(master)) {
> > + if (time_after(jiffies, timeout)) {
> > + dev_err(master->dev, "XI3C write timeout\n");
> > + return -EIO;
> > + }
> > +
> > + wrfifo_space = xi3c_wrfifolevel(master);
> > + for (space_index = 0; space_index < wrfifo_space && cmd->tx_len >
> 0;
> > + space_index++)
> > + xi3c_master_wr_to_tx_fifo(master, cmd);
> > + }
> > + return 0;
> > +}
> > +
> ...
> > +
> > +static int xi3c_master_do_daa(struct i3c_master_controller *m) {
> > + struct xi3c_master *master = to_xi3c_master(m);
> > + struct xi3c_cmd *daa_cmd;
> > + struct xi3c_xfer *xfer;
> > + u8 pid_bufs[XI3C_MAX_DEVS][8];
> > + u8 data, last_addr = 0;
> > + int addr, ret, i;
> > + u8 *pid_buf;
> > +
> > + u64 *pid_bcr_dcr __free(kfree) = kcalloc(XI3C_MAX_DEVS, sizeof(u64),
> > + GFP_KERNEL);
> > + if (!pid_bcr_dcr)
> > + return -ENOMEM;
> > +
> > + xfer = xi3c_master_alloc_xfer(master, 1);
> > + if (!xfer) {
> > + ret = -ENOMEM;
> > + goto err_daa_mem;
> > + }
> > +
> > + for (i = 0; i < XI3C_MAX_DEVS; i++) {
> > + addr = i3c_master_get_free_addr(m, last_addr + 1);
> > + if (addr < 0) {
> > + ret = -ENOSPC;
> > + goto err_daa;
> > + }
> > + master->daa.addrs[i] = (u8)addr;
> > + last_addr = (u8)addr;
> > + }
> > +
> > + /* Fill ENTDAA CCC */
> > + data = I3C_CCC_ENTDAA;
> > + daa_cmd = &xfer->cmds[0];
> > + daa_cmd->addr = I3C_BROADCAST_ADDR;
> > + daa_cmd->rnw = 0;
> > + daa_cmd->tx_buf = &data;
> > + daa_cmd->tx_len = 1;
> > + daa_cmd->type = XI3C_SDR_MODE;
> > + daa_cmd->tid = XI3C_SDR_TID;
> > + daa_cmd->continued = true;
> > +
> > + ret = xi3c_master_common_xfer(master, xfer);
> > + /* DAA always finishes with CE2_ERROR or NACK_RESP */
> > + if (ret && ret != I3C_ERROR_M2) {
> > + goto err_daa;
> > + } else {
> > + if (ret && ret == I3C_ERROR_M2) {
> > + ret = 0;
> > + goto err_daa;
> > + }
> > + }
> > +
> > + master->daa.index = 0;
> > +
> > + while (true) {
> > + struct xi3c_cmd *cmd = &xfer->cmds[0];
> > +
> > + pid_buf = pid_bufs[master->daa.index];
> > + addr = (master->daa.addrs[master->daa.index] << 1) |
> > + (!parity8(master->daa.addrs[master->daa.index]));
> > +
> > + cmd->tx_buf = (u8 *)&addr;
> > + cmd->tx_len = 1;
> > + cmd->addr = I3C_BROADCAST_ADDR;
> > + cmd->rnw = 1;
> > + cmd->rx_buf = pid_buf;
> > + cmd->rx_len = XI3C_DAA_SLAVEINFO_READ_BYTECOUNT;
> > + cmd->is_daa = true;
> > + cmd->type = XI3C_SDR_MODE;
> > + cmd->tid = XI3C_SDR_TID;
> > + cmd->continued = true;
> > +
> > + ret = xi3c_master_common_xfer(master, xfer);
> > +
> > + /* DAA always finishes with CE2_ERROR or NACK_RESP */
> > + if (ret && ret != I3C_ERROR_M2) {
> > + goto err_daa;
> > + } else {
> > + if (ret && ret == I3C_ERROR_M2) {
> > + xi3c_master_resume(master);
> > + master->daa.index--;
> > + ret = 0;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + kfree(xfer);
> > +
> > + for (i = 0; i < master->daa.index; i++) {
> > + i3c_master_add_i3c_dev_locked(m, master->daa.addrs[i]);
> > +
> > + pid_bcr_dcr[i] = FIELD_GET(XI3C_PID_MASK,
> > + get_unaligned_be64(pid_bufs[i]));
>
> you only use pid_bcr_dcr here, needn't dymatic alloc at all.
>
> u64 data = FIELD_GET(XI3C_PID_MASK,
> get_unaligned_be64(pid_bufs[i]));
> dev_info(master->dev, "Client %d: PID: 0x%llx\n", i, data);
We will fix.
>
> Frank
>
> > + dev_info(master->dev, "Client %d: PID: 0x%llx\n", i, pid_bcr_dcr[i]);
> > + }
> > +
> > + return 0;
> > +
> > +err_daa:
> > + kfree(xfer);
> > +err_daa_mem:
> > + xi3c_master_reinit(master);
> > + return ret;
> > +}
> > +
> ...
> > +};
> > +module_platform_driver(xi3c_master_driver);
> > +
> > +MODULE_AUTHOR("Manikanta Guntupalli <manikanta.guntupalli at amd.com>");
> > +MODULE_DESCRIPTION("AXI I3C master driver");
> MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >
Thanks,
Manikanta.
More information about the linux-i3c
mailing list