[Public]

Hi,

-----Original Message-----
From: Frank Li <Frank.li@nxp.com>
Sent: Wednesday, September 24, 2025 12:53 AM
To: Guntupalli, Manikanta <redacted>
Cc: git (AMD-Xilinx) <redacted>; Simek, Michal <michal.simek@amd.com>;
alexandre.belloni@bootlin.com; robh@kernel.org; krzk+dt@kernel.org;
conor+dt@kernel.org; pgaj@cadence.com; wsa+renesas@sang-engineering.com;
tommaso.merciai.xr@bp.renesas.com; arnd@arndb.de;
quic_msavaliy@quicinc.com; S-k, Shyam-sundar [off-list ref];
sakari.ailus@linux.intel.com; billy_tsai@aspeedtech.com; kees@kernel.org;
gustavoars@kernel.org; jarkko.nikula@linux.intel.com; jorge.marques@analog.com;
linux-i3c@lists.infradead.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-arch@vger.kernel.org; linux-
hardening@vger.kernel.org; Pandey, Radhey Shyam
[off-list ref]; Goud, Srinivas [off-list ref];
Datta, Shubhrajyoti [off-list ref]; manion05gk@gmail.com
Subject: Re: [PATCH V7 4/4] i3c: master: Add AMD I3C bus controller driver

On Tue, Sep 23, 2025 at 09:15:51PM +0530, Manikanta Guntupalli wrote:

quoted

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 <redacted>
---
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.

Changes for V6:
Removed typecast for xi3c_getrevisionnumber(), xi3c_wrfifolevel(), and
xi3c_rdfifolevel().
Replaced dynamic allocation with a static variable for pid_bcr_dcr.
Fixed sparse warning in do_daa by typecasting the address parity value
to u8.
Fixed sparse warning in xi3c_master_bus_init by typecasting the pid
value to u64 in info.pid calculation.

Changes for V7:
Updated timeout macro name.
Updated xi3c_master_wr_to_tx_fifo() and xi3c_master_rd_from_rx_fifo()
to use i3c_writel_fifo() and i3c_readl_fifo().
---
 MAINTAINERS                         |    7 +
 drivers/i3c/master/Kconfig          |   16 +
 drivers/i3c/master/Makefile         |    1 +
 drivers/i3c/master/amd-i3c-master.c | 1012
+++++++++++++++++++++++++++
 4 files changed, 1036 insertions(+)
 create mode 100644 drivers/i3c/master/amd-i3c-master.c

diff --git a/MAINTAINERS b/MAINTAINERS index

8886d66bd824..fe88efb41f5b 100644

--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -11782,6 +11782,13 @@ L: linux-i2c@vger.kernel.org
 S: Maintained
 F: drivers/i2c/i2c-stub.c

+I3C DRIVER FOR AMD AXI I3C MASTER
+M: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
+R: Michal Simek <michal.simek@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@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..c89f7de85635

--- /dev/null
+++ b/drivers/i3c/master/amd-i3c-master.c

@@ -0,0 +1,1012 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * I3C master driver for the AMD I3C controller.
+ *
+ * Copyright (C) 2025, Advanced Micro Devices, Inc.
+ */
+

...

quoted

+
+static inline int xi3c_master_common_xfer(struct xi3c_master *master,
+                                     struct xi3c_xfer *xfer)
+{
+   int ret, timeout;
+
+   guard(mutex)(&master->lock);
+
+   xi3c_master_enqueue_xfer(master, xfer);
+   timeout = wait_for_completion_timeout(&xfer->comp,
+                                         XI3C_XFER_TIMEOUT_JIFFIES);

Recent patch use time_left instead of timeout.

We will update.

quoted

+   if (!timeout)
+           ret = -ETIMEDOUT;
+   else
+           ret = xfer->ret;
+
+   if (ret)
+           xi3c_master_dequeue_xfer(master, xfer);
+
+   return ret;
+}
+
+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;

struct xi3c_xfer __free(kfree) *xfer;
will simple err path.

We will fix.

quoted

+   u8 pid_bufs[XI3C_MAX_DEVS][8];
+   u8 data, last_addr = 0;
+   int addr, ret, i;
+   u8 *pid_buf;

try keep reverise christmas tree order

We will fix.

quoted

+
+   xfer = xi3c_master_alloc_xfer(master, 1);

only 1 xfer, needn't alloc from heap. Just use stack varible should be enough.

xi3c_master_alloc_xfer() handles both memory allocation and structure initialization. If we replace it with a stack variable in this case, we would need to duplicate the initialization logic in several places. Using the helper keeps the code consistent and avoids repetition.

quoted

+   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) |
+                  (u8)(!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]);
+
+           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);
+   }
+
+   return 0;
+
+err_daa:
+   kfree(xfer);
+err_daa_mem:
+   xi3c_master_reinit(master);
+   return ret;
+}
+

quoted

+static int xi3c_master_send_bdcast_ccc_cmd(struct xi3c_master *master,
+                                      struct i3c_ccc_cmd *ccc)
+{
+   u16 xfer_len = ccc->dests[0].payload.len + 1;
+   struct xi3c_xfer *xfer;
+   struct xi3c_cmd *cmd;
+   int ret;
+
+   xfer = xi3c_master_alloc_xfer(master, 1);
+   if (!xfer)
+           return -ENOMEM;

the same here. use struct xi3c_xfer xfer should be more simple.

Same reasoning as mentioned above - xi3c_master_alloc_xfer() handles both allocation and initialization, avoiding code duplication.

quoted

+
+   u8 *buf __free(kfree) = kmalloc(xfer_len, GFP_KERNEL);
+   if (!buf) {
+           kfree(xfer);
+           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);
+   kfree(xfer);
+
+   return ret;
+}
+

...

quoted

+static struct platform_driver xi3c_master_driver = {
+   .probe = xi3c_master_probe,
+   .remove = xi3c_master_remove,
+   .driver = {
+           .name = "axi-i3c-master",
+           .of_match_table = xi3c_master_of_ids,
+   },
+};
+module_platform_driver(xi3c_master_driver);
+
+MODULE_AUTHOR("Manikanta Guntupalli [off-list ref]");
+MODULE_DESCRIPTION("AXI I3C master driver");

MODULE_LICENSE("GPL");

quoted

--
2.34.1

Thanks,
Manikanta.