Re: [PATCH V7 4/4] i3c: master: Add AMD I3C bus controller driver

[Frank Li <Frank.li@nxp.com>]

On Tue, Sep 23, 2025 at 09:15:51PM +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@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.
>
> 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.
> + */
> +

...

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

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

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

> +
> +	xfer = xi3c_master_alloc_xfer(master, 1);

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

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

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

Frank
> +
> +	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;
> +}
> +
...
> +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 <manikanta.guntupalli@amd.com>");
> +MODULE_DESCRIPTION("AXI I3C master driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>