[Public] Hi,
> -----Original Message----- > From: Frank Li <[email protected]> > Sent: Monday, September 8, 2025 9:34 PM > To: Guntupalli, Manikanta <[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]; linux- > [email protected]; Pandey, Radhey Shyam > <[email protected]>; Goud, Srinivas <[email protected]>; > Datta, Shubhrajyoti <[email protected]>; [email protected] > 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 <[email protected]> > > --- > > 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: [email protected] > > S: Maintained > > F: drivers/i2c/i2c-stub.c > > > > +I3C DRIVER FOR AMD AXI I3C MASTER > > +M: Manikanta Guntupalli <[email protected]> > > +R: Michal Simek <[email protected]> > > +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 <[email protected]> > > 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 <[email protected]>"); > > +MODULE_DESCRIPTION("AXI I3C master driver"); > MODULE_LICENSE("GPL"); > > -- > > 2.34.1 > > Thanks, Manikanta.
