[PATCH v3 0/5] Introduce initial support for the AMD I3C (non-HCI) to DW driver

[PATCH v3 0/5] Introduce initial support for the AMD I3C (non-HCI) to DW driver

[Shyam Sundar S K]

The AMD EPYC platform design has DIMMs connected over the I3C bus, with
each DIMM containing three components: SPD, PMIC, and RCD.

To access component-specific information within the DIMMs, such as initial
dynamic address, static address, and provisional ID, ACPI support is
necessary for the I3C core. This requires adding ACPI binding to the
dw-i3c-master driver and retrieving slave information from the AMD ASL.

Currently, the code is closely tied to dt-bindings. This initial set aims
to decouple some of these bindings by adding the AMD-specific _HID,
enabling the current driver to support ACPI-enabled x86 systems.

In this series, support for following features has been added.
- X86/ACPI support to i3c core
- Support for SETAASA CCC command
- Add routines to plugin a SPD device to the i3c bus
- Workaround for AMD hardware
- Add dw-i3c-master driver with ACPI bindings


v3:
----
 - Address feedback from Jarkko
 - Drop using SHARED_IRQ flags during interrupt registration
 - Update commit message to mention about SETAASA
 - Use bits [6:0] as the static address

v2:
----
 - Address LKP reports issues

Shyam Sundar S K (5):
  i3c: dw: Add support for AMDI0015 ACPI ID
  i3c: master: Add ACPI support to i3c subsystem
  i3c: master: Add a routine to include the I3C SPD device
  i3c: master: Add support for SETAASA CCC
  i3c: dw: Add quirk to address OD/PP timing issue on AMD platform

 drivers/i3c/internals.h            |   3 +
 drivers/i3c/master.c               | 157 ++++++++++++++++++++++++++++-
 drivers/i3c/master/dw-i3c-master.c |  42 ++++++++
 drivers/i3c/master/dw-i3c-master.h |   1 +
 include/linux/i3c/ccc.h            |   1 +
 include/linux/i3c/master.h         |   2 +
 6 files changed, 205 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v3 1/5] i3c: dw: Add support for AMDI0015 ACPI ID

[Shyam Sundar S K]

Add AMDI0015 _HID for Designware I3C driver so that the dw-i3c-master
driver can be probed on AMD platforms.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master/dw-i3c-master.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 8d694672c110..1a7c300b6d45 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1748,12 +1748,19 @@ static const struct of_device_id dw_i3c_master_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
 
+static const struct acpi_device_id amd_i3c_device_match[] = {
+	{ "AMDI0015" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, amd_i3c_device_match);
+
 static struct platform_driver dw_i3c_driver = {
 	.probe = dw_i3c_probe,
 	.remove_new = dw_i3c_remove,
 	.driver = {
 		.name = "dw-i3c-master",
 		.of_match_table = dw_i3c_master_of_match,
+		.acpi_match_table = amd_i3c_device_match,
 		.pm = &dw_i3c_pm_ops,
 	},
 };
-- 
2.34.1

[PATCH v3 2/5] i3c: master: Add ACPI support to i3c subsystem

[Shyam Sundar S K]

As of now, the I3C subsystem only has ARM-specific initialization, and
there is no corresponding ACPI plumbing present. To address this, ACPI
support needs to be added to both the I3C core and DW driver.

Add support to get the ACPI handle from the _HID probed and parse the apci
object to retrieve the slave information from BIOS.

Based on the acpi object information propogated via BIOS, build the i3c
board information so that the same information can be used across the
driver to handle the slave requests.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
Cc: linux-acpi@vger.kernel.org

 drivers/i3c/internals.h            |  3 ++
 drivers/i3c/master.c               | 84 ++++++++++++++++++++++++++++++
 drivers/i3c/master/dw-i3c-master.c |  7 +++
 include/linux/i3c/master.h         |  1 +
 4 files changed, 95 insertions(+)

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 433f6088b7ce..178bc0ebe6b6 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -10,6 +10,9 @@
 
 #include <linux/i3c/master.h>
 
+#define I3C_GET_PID		0x08
+#define I3C_GET_ADDR		0x7F
+
 void i3c_bus_normaluse_lock(struct i3c_bus *bus);
 void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
 
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 6f3eb710a75d..0ceef2aa9161 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2251,6 +2251,84 @@ static int of_i3c_master_add_dev(struct i3c_master_controller *master,
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_ACPI)
+static int i3c_acpi_configure_master(struct i3c_master_controller *master)
+{
+	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
+	enum i3c_addr_slot_status addrstatus;
+	struct i3c_dev_boardinfo *boardinfo;
+	struct device *dev = &master->dev;
+	struct fwnode_handle *fwnode;
+	struct acpi_device *adev;
+	u32 slv_addr, num_dev;
+	acpi_status status;
+	u64 val;
+
+	status = acpi_evaluate_object_typed(master->ahandle, "_DSD", NULL, &buf, ACPI_TYPE_PACKAGE);
+	if (ACPI_FAILURE(status)) {
+		dev_err(&master->dev, "Error reading _DSD:%s\n", acpi_format_exception(status));
+		return -ENODEV;
+	}
+
+	num_dev = device_get_child_node_count(dev);
+	if (!num_dev) {
+		dev_err(&master->dev, "Error: no child node present\n");
+		return -EINVAL;
+	}
+
+	device_for_each_child_node(dev, fwnode) {
+		adev = to_acpi_device_node(fwnode);
+		if (!adev)
+			return -ENODEV;
+
+		status = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &val);
+		if (ACPI_FAILURE(status)) {
+			dev_err(&master->dev, "Error: eval _ADR failed\n");
+			return -EINVAL;
+		}
+		slv_addr = val & I3C_GET_ADDR;
+
+		boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
+		if (!boardinfo)
+			return -ENOMEM;
+
+		if (slv_addr) {
+			if (slv_addr > I3C_MAX_ADDR)
+				return -EINVAL;
+
+			addrstatus = i3c_bus_get_addr_slot_status(&master->bus, slv_addr);
+			if (addrstatus != I3C_ADDR_SLOT_FREE)
+				return -EINVAL;
+		}
+
+		boardinfo->static_addr = slv_addr;
+		if (boardinfo->static_addr > I3C_MAX_ADDR)
+			return -EINVAL;
+
+		addrstatus = i3c_bus_get_addr_slot_status(&master->bus,	boardinfo->static_addr);
+		if (addrstatus != I3C_ADDR_SLOT_FREE)
+			return -EINVAL;
+
+		boardinfo->pid = val >> I3C_GET_PID;
+		if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
+		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
+			return -EINVAL;
+
+		/*
+		 * According to the specification, SETDASA is not supported for DIMM slaves
+		 * during device discovery. Therefore, BIOS will populate same initial
+		 * dynamic address as the static address.
+		 */
+		boardinfo->init_dyn_addr = boardinfo->static_addr;
+		list_add_tail(&boardinfo->node, &master->boardinfo.i3c);
+	}
+
+	return 0;
+}
+#else
+static int i3c_acpi_configure_master(struct i3c_master_controller *master) { return 0; }
+#endif
+
 static int of_populate_i3c_bus(struct i3c_master_controller *master)
 {
 	struct device *dev = &master->dev;
@@ -2771,6 +2849,12 @@ int i3c_master_register(struct i3c_master_controller *master,
 	master->dev.coherent_dma_mask = parent->coherent_dma_mask;
 	master->dev.dma_parms = parent->dma_parms;
 
+	if (has_acpi_companion(master->dev.parent)) {
+		ret = i3c_acpi_configure_master(master);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = of_populate_i3c_bus(master);
 	if (ret)
 		goto err_put_dev;
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 1a7c300b6d45..d44b771d03e1 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1602,6 +1602,13 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	master->maxdevs = ret >> 16;
 	master->free_pos = GENMASK(master->maxdevs - 1, 0);
 
+#if IS_ENABLED(CONFIG_ACPI)
+	ACPI_COMPANION_SET(&master->base.dev, ACPI_COMPANION(&pdev->dev));
+	master->base.ahandle = acpi_device_handle(ACPI_COMPANION(&pdev->dev));
+	if (!master->base.ahandle)
+		dev_err(&pdev->dev, "Failed to get acpi device handle\n");
+#endif
+
 	INIT_WORK(&master->hj_work, dw_i3c_hj_work);
 	ret = i3c_master_register(&master->base, &pdev->dev,
 				  &dw_mipi_i3c_ops, false);
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 2a1ed05d5782..367faf7c4bf3 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -523,6 +523,7 @@ struct i3c_master_controller {
 	} boardinfo;
 	struct i3c_bus bus;
 	struct workqueue_struct *wq;
+	acpi_handle ahandle;
 };
 
 /**
-- 
2.34.1

[PATCH v3 3/5] i3c: master: Add a routine to include the I3C SPD device

[Shyam Sundar S K]

Implement the i3c_master_add_spd_dev() function in the I3C master to
handle SPD (Serial Presence Detect) device creation and map the board
information into the I3C device structure.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 0ceef2aa9161..8f37dcaa1efe 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1657,6 +1657,45 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 	}
 }
 
+static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
+				  struct i3c_dev_boardinfo *boardinfo)
+{
+	struct i3c_dev_desc *i3cdev;
+	int ret;
+
+	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
+		struct i3c_device_info info = {
+			.static_addr = boardinfo->static_addr,
+		};
+
+		ret = i3c_bus_get_addr_slot_status(&master->bus, boardinfo->static_addr);
+		if (ret != I3C_ADDR_SLOT_FREE)
+			return -EBUSY;
+
+		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
+		if (IS_ERR(i3cdev))
+			return -ENOMEM;
+
+		i3cdev->boardinfo = boardinfo;
+		ret = i3c_master_attach_i3c_dev(master, i3cdev);
+		if (ret)
+			goto err_free_dev;
+
+		i3cdev->info.pid = i3cdev->boardinfo->pid;
+		i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
+
+		i3c_bus_normaluse_lock(&master->bus);
+		i3c_master_register_new_i3c_devs(master);
+		i3c_bus_normaluse_unlock(&master->bus);
+	}
+
+	return 0;
+
+err_free_dev:
+	i3c_master_free_i3c_dev(i3cdev);
+	return ret;
+}
+
 /**
  * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
  * @master: master doing the DAA
@@ -1814,7 +1853,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 {
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
-	struct i3c_dev_boardinfo *i3cboardinfo;
+	struct i3c_dev_boardinfo *i3cboardinfo = NULL;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 
@@ -1868,6 +1907,8 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 		goto err_bus_cleanup;
 	}
 
+	i3c_master_add_spd_dev(master, i3cboardinfo);
+
 	if (master->ops->set_speed) {
 		ret = master->ops->set_speed(master, I3C_OPEN_DRAIN_SLOW_SPEED);
 		if (ret)
-- 
2.34.1

[PATCH v3 4/5] i3c: master: Add support for SETAASA CCC

[Shyam Sundar S K]

I3C devices like DIMMs over SPD use SETAASA[1] for bus discovery instead of
SETDASA. Add a new routine for I3C host controller drivers to use. If the
I3C slave on the bus is an SPD device, skip the regular DAA process.

According to the SPD spec[2], use SETAASA for bus discovery, and avoid
sending RSTDAA and DISEC, as they are considered illegal. Skip this entire
process if the slave is SPD-compliant, as indicated by the "jdec_spd" flag
from the BIOS.

[1] SETAASA is introduced in MIPI v1.1.1 specification.
[2] https://www.jedec.org/system/files/docs/JESD300-5B.01.pdf
(section 2.4 and 2.6.3)

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master.c               | 32 +++++++++++++++++++++++++++++-
 drivers/i3c/master/dw-i3c-master.c |  1 +
 include/linux/i3c/ccc.h            |  1 +
 include/linux/i3c/master.h         |  1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 8f37dcaa1efe..2f06f168ef82 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1657,6 +1657,21 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 	}
 }
 
+static int i3c_master_setaasa_locked(struct i3c_master_controller *master)
+{
+	struct i3c_ccc_cmd_dest dest;
+	struct i3c_ccc_cmd cmd;
+	int ret;
+
+	i3c_ccc_cmd_dest_init(&dest, I3C_BROADCAST_ADDR, 0);
+	i3c_ccc_cmd_init(&cmd, false, I3C_CCC_SETAASA, &dest, 1);
+
+	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+	i3c_ccc_cmd_dest_cleanup(&dest);
+
+	return ret;
+}
+
 static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
 				  struct i3c_dev_boardinfo *boardinfo)
 {
@@ -1684,6 +1699,10 @@ static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
 		i3cdev->info.pid = i3cdev->boardinfo->pid;
 		i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
 
+		ret = i3c_master_setaasa_locked(master);
+		if (ret)
+			goto err_free_dev;
+
 		i3c_bus_normaluse_lock(&master->bus);
 		i3c_master_register_new_i3c_devs(master);
 		i3c_bus_normaluse_unlock(&master->bus);
@@ -1907,7 +1926,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 		goto err_bus_cleanup;
 	}
 
-	i3c_master_add_spd_dev(master, i3cboardinfo);
+	/*
+	 * If the I3C slave on the bus is SPD device, then do not follow the regular
+	 * DAA process. Also, as per SPD spec SETAASA is required for the bus discovery
+	 * and sending RSTDAA and DISEC is considered as illegal. So skip the entire process
+	 * if the jdec_spd flag has been identified from the BIOS.
+	 */
+	if (master->jdec_spd)
+		return i3c_master_add_spd_dev(master, i3cboardinfo);
 
 	if (master->ops->set_speed) {
 		ret = master->ops->set_speed(master, I3C_OPEN_DRAIN_SLOW_SPEED);
@@ -2311,6 +2337,10 @@ static int i3c_acpi_configure_master(struct i3c_master_controller *master)
 		return -ENODEV;
 	}
 
+	status = acpi_evaluate_object(master->ahandle, "_STR", NULL, NULL);
+	if (ACPI_SUCCESS(status))
+		master->jdec_spd = true;
+
 	num_dev = device_get_child_node_count(dev);
 	if (!num_dev) {
 		dev_err(&master->dev, "Error: no child node present\n");
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index d44b771d03e1..dc701275485a 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -282,6 +282,7 @@ static bool dw_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
 	case I3C_CCC_GETSTATUS:
 	case I3C_CCC_GETMXDS:
 	case I3C_CCC_GETHDRCAP:
+	case I3C_CCC_SETAASA:
 		return true;
 	default:
 		return false;
diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
index ad59a4ae60d1..a145d766ab6f 100644
--- a/include/linux/i3c/ccc.h
+++ b/include/linux/i3c/ccc.h
@@ -32,6 +32,7 @@
 #define I3C_CCC_DEFSLVS			I3C_CCC_ID(0x8, true)
 #define I3C_CCC_ENTTM			I3C_CCC_ID(0xb, true)
 #define I3C_CCC_ENTHDR(x)		I3C_CCC_ID(0x20 + (x), true)
+#define I3C_CCC_SETAASA			I3C_CCC_ID(0x29, true)
 
 /* Unicast-only commands */
 #define I3C_CCC_SETDASA			I3C_CCC_ID(0x7, false)
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 367faf7c4bf3..cd8390d8b469 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -516,6 +516,7 @@ struct i3c_master_controller {
 	const struct i3c_master_controller_ops *ops;
 	unsigned int secondary : 1;
 	unsigned int init_done : 1;
+	unsigned int jdec_spd : 1;
 	unsigned int hotjoin: 1;
 	struct {
 		struct list_head i3c;
-- 
2.34.1

[PATCH v3 5/5] i3c: dw: Add quirk to address OD/PP timing issue on AMD platform

[Shyam Sundar S K]

The AMD Legacy I3C is having a problem with its IP, specifically with the
push-pull and open-drain pull-up registers. These registers need to be
manually programmed for every CCC submission to align with the duty cycle.
Therefore, add a quirk to address this issue.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master/dw-i3c-master.c | 29 ++++++++++++++++++++++++++++-
 drivers/i3c/master/dw-i3c-master.h |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index dc701275485a..029f81729333 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -220,6 +220,14 @@
 
 #define XFER_TIMEOUT (msecs_to_jiffies(1000))
 #define RPM_AUTOSUSPEND_TIMEOUT 1000 /* ms */
+
+/* Timing values to configure 12.5MHz frequency */
+#define AMD_I3C_OD_TIMING          0x4C007C
+#define AMD_I3C_PP_TIMING          0x8001A
+
+/* List of quirks */
+#define AMD_I3C_OD_PP_TIMING		BIT(1)
+
 struct dw_i3c_cmd {
 	u32 cmd_lo;
 	u32 cmd_hi;
@@ -795,6 +803,12 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
 	return ret;
 }
 
+static void amd_configure_od_pp_quirk(struct dw_i3c_master *master)
+{
+	master->i3c_od_timing = AMD_I3C_OD_TIMING;
+	master->i3c_pp_timing = AMD_I3C_PP_TIMING;
+}
+
 static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
 				      struct i3c_ccc_cmd *ccc)
 {
@@ -804,6 +818,13 @@ static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
 	if (ccc->id == I3C_CCC_ENTDAA)
 		return -EINVAL;
 
+	/* AMD platform specific OD and PP timings */
+	if (master->quirks & AMD_I3C_OD_PP_TIMING) {
+		amd_configure_od_pp_quirk(master);
+		writel(master->i3c_pp_timing, master->regs + SCL_I3C_PP_TIMING);
+		writel(master->i3c_od_timing, master->regs + SCL_I3C_OD_TIMING);
+	}
+
 	ret = pm_runtime_resume_and_get(master->dev);
 	if (ret < 0) {
 		dev_err(master->dev,
@@ -1610,6 +1631,8 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 		dev_err(&pdev->dev, "Failed to get acpi device handle\n");
 #endif
 
+	master->quirks = (unsigned long)device_get_match_data(&pdev->dev);
+
 	INIT_WORK(&master->hj_work, dw_i3c_hj_work);
 	ret = i3c_master_register(&master->base, &pdev->dev,
 				  &dw_mipi_i3c_ops, false);
@@ -1683,6 +1706,10 @@ static void dw_i3c_master_restore_addrs(struct dw_i3c_master *master)
 
 static void dw_i3c_master_restore_timing_regs(struct dw_i3c_master *master)
 {
+	/* AMD platform specific OD and PP timings */
+	if (master->quirks & AMD_I3C_OD_PP_TIMING)
+		amd_configure_od_pp_quirk(master);
+
 	writel(master->i3c_pp_timing, master->regs + SCL_I3C_PP_TIMING);
 	writel(master->bus_free_timing, master->regs + BUS_FREE_TIMING);
 	writel(master->i3c_od_timing, master->regs + SCL_I3C_OD_TIMING);
@@ -1757,7 +1784,7 @@ static const struct of_device_id dw_i3c_master_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
 
 static const struct acpi_device_id amd_i3c_device_match[] = {
-	{ "AMDI0015" },
+	{ "AMDI0015", AMD_I3C_OD_PP_TIMING },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, amd_i3c_device_match);
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index 219ff815d3a7..c5cb695c16ab 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -50,6 +50,7 @@ struct dw_i3c_master {
 	u32 bus_free_timing;
 	u32 i2c_fm_timing;
 	u32 i2c_fmp_timing;
+	u32 quirks;
 	/*
 	 * Per-device hardware data, used to manage the device address table
 	 * (DAT)
-- 
2.34.1

Re: [PATCH v3 1/5] i3c: dw: Add support for AMDI0015 ACPI ID

[Jarkko Nikula]

Hi

On 11/8/24 9:33 AM, Shyam Sundar S K wrote:
> Add AMDI0015 _HID for Designware I3C driver so that the dw-i3c-master
> driver can be probed on AMD platforms.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/i3c/master/dw-i3c-master.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 8d694672c110..1a7c300b6d45 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -1748,12 +1748,19 @@ static const struct of_device_id dw_i3c_master_of_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
>   
> +static const struct acpi_device_id amd_i3c_device_match[] = {
> +	{ "AMDI0015" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, amd_i3c_device_match);
> +
>   static struct platform_driver dw_i3c_driver = {
>   	.probe = dw_i3c_probe,
>   	.remove_new = dw_i3c_remove,
>   	.driver = {
>   		.name = "dw-i3c-master",
>   		.of_match_table = dw_i3c_master_of_match,
> +		.acpi_match_table = amd_i3c_device_match,
>   		.pm = &dw_i3c_pm_ops,
>   	},
>   };

Am I right this and patch 5/5 can be independent from rest of the series?

To me it looks these two patches enable bus communication and thus be 
useful without rest of the series while latter need more discussion 
(I'll have some notes coming) and Cc'ing linux-acpi.

Re: [PATCH v3 1/5] i3c: dw: Add support for AMDI0015 ACPI ID

[Shyam Sundar S K]

Hi Jarkko,

On 11/12/2024 13:46, Jarkko Nikula wrote:
> Hi
> 
> On 11/8/24 9:33 AM, Shyam Sundar S K wrote:
>> Add AMDI0015 _HID for Designware I3C driver so that the dw-i3c-master
>> driver can be probed on AMD platforms.
>>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>   drivers/i3c/master/dw-i3c-master.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/i3c/master/dw-i3c-master.c
>> b/drivers/i3c/master/dw-i3c-master.c
>> index 8d694672c110..1a7c300b6d45 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> @@ -1748,12 +1748,19 @@ static const struct of_device_id
>> dw_i3c_master_of_match[] = {
>>   };
>>   MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
>>   +static const struct acpi_device_id amd_i3c_device_match[] = {
>> +    { "AMDI0015" },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, amd_i3c_device_match);
>> +
>>   static struct platform_driver dw_i3c_driver = {
>>       .probe = dw_i3c_probe,
>>       .remove_new = dw_i3c_remove,
>>       .driver = {
>>           .name = "dw-i3c-master",
>>           .of_match_table = dw_i3c_master_of_match,
>> +        .acpi_match_table = amd_i3c_device_match,
>>           .pm = &dw_i3c_pm_ops,
>>       },
>>   };
> 
> Am I right this and patch 5/5 can be independent from rest of the series?

Right. 1/5 and 5/5 can be grouped. But rest of the other patches are
equally important because they drive the usecase.

> 
> To me it looks these two patches enable bus communication and thus be
> useful without rest of the series while latter need more discussion
> (I'll have some notes coming) and Cc'ing linux-acpi.

I have Cc'ed linux-acpi in this revision. Do you have any feedback for
patches 2-4 ?

Thanks,
Shyam

Re: [PATCH v3 1/5] i3c: dw: Add support for AMDI0015 ACPI ID

[Jarkko Nikula]

Hi

On 11/12/24 10:48 AM, Shyam Sundar S K wrote:
>> Am I right this and patch 5/5 can be independent from rest of the series?
> 
> Right. 1/5 and 5/5 can be grouped. But rest of the other patches are
> equally important because they drive the usecase.
> 
>>
>> To me it looks these two patches enable bus communication and thus be
>> useful without rest of the series while latter need more discussion
>> (I'll have some notes coming) and Cc'ing linux-acpi.
> 
> I have Cc'ed linux-acpi in this revision. Do you have any feedback for
> patches 2-4 ?
> 
Yes, I'm reviewing them and only the patch 2/5 was Cc'ed to linux-acpi.

Patchset split would serve better in my opinion enabling basic 
communication and have an other set concentrating more complex scenario 
we were try to get input from ACPI folks.

Re: [PATCH v3 1/5] i3c: dw: Add support for AMDI0015 ACPI ID

[Shyam Sundar S K]

On 11/12/2024 15:18, Jarkko Nikula wrote:
> Hi
> 
> On 11/12/24 10:48 AM, Shyam Sundar S K wrote:
>>> Am I right this and patch 5/5 can be independent from rest of the
>>> series?
>>
>> Right. 1/5 and 5/5 can be grouped. But rest of the other patches are
>> equally important because they drive the usecase.
>>
>>>
>>> To me it looks these two patches enable bus communication and thus be
>>> useful without rest of the series while latter need more discussion
>>> (I'll have some notes coming) and Cc'ing linux-acpi.
>>
>> I have Cc'ed linux-acpi in this revision. Do you have any feedback for
>> patches 2-4 ?
>>
> Yes, I'm reviewing them and only the patch 2/5 was Cc'ed to linux-acpi.
> 
> Patchset split would serve better in my opinion enabling basic
> communication and have an other set concentrating more complex
> scenario we were try to get input from ACPI folks.

Ok. I can copy the linux-apci for patches from 2-4 after your feedback
on the current series.

Can atleast patches 1/5 and 5/5 be independently applied for -next so
they we don't miss out this cycle?

Thanks,
Shyam

Re: [PATCH v3 2/5] i3c: master: Add ACPI support to i3c subsystem

[Jarkko Nikula]

Hi

On 11/8/24 9:33 AM, Shyam Sundar S K wrote:
> As of now, the I3C subsystem only has ARM-specific initialization, and
> there is no corresponding ACPI plumbing present. To address this, ACPI
> support needs to be added to both the I3C core and DW driver.
> 
> Add support to get the ACPI handle from the _HID probed and parse the apci
> object to retrieve the slave information from BIOS.
> 
> Based on the acpi object information propogated via BIOS, build the i3c
> board information so that the same information can be used across the
> driver to handle the slave requests.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> Cc: linux-acpi@vger.kernel.org
> 
>   drivers/i3c/internals.h            |  3 ++
>   drivers/i3c/master.c               | 84 ++++++++++++++++++++++++++++++
>   drivers/i3c/master/dw-i3c-master.c |  7 +++
>   include/linux/i3c/master.h         |  1 +
>   4 files changed, 95 insertions(+)
> 
To clarify: The DSDT example I mentioned in the previous version I meant 
it would be good to have in the patch/series itself. It helps both 
reviewing the patchset and future documentation.

First I was thinking commit log itself but I guess a full DSDT example 
showing SDP target devices becomes too large and e.g. 
Documentation/firmware-guide/acpi/enumeration.rst is better?

> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 433f6088b7ce..178bc0ebe6b6 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -10,6 +10,9 @@
>   
>   #include <linux/i3c/master.h>
>   
> +#define I3C_GET_PID		0x08
> +#define I3C_GET_ADDR		0x7F
> +

These are bit confusing. One could think these are commands while 
I3C_GET_PID is a shift and I3C_GET_ADDR is a mask in the code below.

Since these are actually defines for the ACPI _ADR I tried to find are 
there already similar defines in ACPI headers but could not find and 
e.g. drivers/ata/libata-acpi.c is interpreting itself so I guess it's ok 
to define here?

>   void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>   void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
>   
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 6f3eb710a75d..0ceef2aa9161 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2251,6 +2251,84 @@ static int of_i3c_master_add_dev(struct i3c_master_controller *master,
>   	return ret;
>   }
>   
> +#if IS_ENABLED(CONFIG_ACPI)
> +static int i3c_acpi_configure_master(struct i3c_master_controller *master)
> +{
> +	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> +	enum i3c_addr_slot_status addrstatus;
> +	struct i3c_dev_boardinfo *boardinfo;
> +	struct device *dev = &master->dev;
> +	struct fwnode_handle *fwnode;
> +	struct acpi_device *adev;
> +	u32 slv_addr, num_dev;
> +	acpi_status status;
> +	u64 val;
> +
> +	status = acpi_evaluate_object_typed(master->ahandle, "_DSD", NULL, &buf, ACPI_TYPE_PACKAGE);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&master->dev, "Error reading _DSD:%s\n", acpi_format_exception(status));
> +		return -ENODEV;
> +	}
> +
> +	num_dev = device_get_child_node_count(dev);
> +	if (!num_dev) {
> +		dev_err(&master->dev, "Error: no child node present\n");
> +		return -EINVAL;
> +	}
> +
> +	device_for_each_child_node(dev, fwnode) {
> +		adev = to_acpi_device_node(fwnode);
> +		if (!adev)
> +			return -ENODEV;
> +
> +		status = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &val);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(&master->dev, "Error: eval _ADR failed\n");
> +			return -EINVAL;
> +		}
> +		slv_addr = val & I3C_GET_ADDR;
> +
> +		boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
> +		if (!boardinfo)
> +			return -ENOMEM;
> +
> +		if (slv_addr) {
> +			if (slv_addr > I3C_MAX_ADDR)
> +				return -EINVAL;
> +
> +			addrstatus = i3c_bus_get_addr_slot_status(&master->bus, slv_addr);
> +			if (addrstatus != I3C_ADDR_SLOT_FREE)
> +				return -EINVAL;
> +		}
> +
> +		boardinfo->static_addr = slv_addr;
> +		if (boardinfo->static_addr > I3C_MAX_ADDR)
> +			return -EINVAL;
> +

slv_addr/static_addr > I3C_MAX_ADDR test is needless due masking above.

> +		boardinfo->pid = val >> I3C_GET_PID;
> +		if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> +			return -EINVAL;
> +

Bit shifting makes PID and also test incorrect?

Re: [PATCH v3 2/5] i3c: master: Add ACPI support to i3c subsystem

[Jarkko Nikula]

Hi

On 11/8/24 9:33 AM, Shyam Sundar S K wrote:
> As of now, the I3C subsystem only has ARM-specific initialization, and
> there is no corresponding ACPI plumbing present. To address this, ACPI
> support needs to be added to both the I3C core and DW driver.
> 
> Add support to get the ACPI handle from the _HID probed and parse the apci
> object to retrieve the slave information from BIOS.
> 
> Based on the acpi object information propogated via BIOS, build the i3c
> board information so that the same information can be used across the
> driver to handle the slave requests.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> Cc: linux-acpi@vger.kernel.org
> 
>   drivers/i3c/internals.h            |  3 ++
>   drivers/i3c/master.c               | 84 ++++++++++++++++++++++++++++++
>   drivers/i3c/master/dw-i3c-master.c |  7 +++
>   include/linux/i3c/master.h         |  1 +
>   4 files changed, 95 insertions(+)
> 
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 433f6088b7ce..178bc0ebe6b6 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -10,6 +10,9 @@
>   
>   #include <linux/i3c/master.h>
>   
> +#define I3C_GET_PID		0x08
> +#define I3C_GET_ADDR		0x7F
> +
>   void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>   void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
>   
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 6f3eb710a75d..0ceef2aa9161 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2251,6 +2251,84 @@ static int of_i3c_master_add_dev(struct i3c_master_controller *master,
>   	return ret;
>   }
>   
> +#if IS_ENABLED(CONFIG_ACPI)
> +static int i3c_acpi_configure_master(struct i3c_master_controller *master)
> +{
> +	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> +	enum i3c_addr_slot_status addrstatus;
> +	struct i3c_dev_boardinfo *boardinfo;
> +	struct device *dev = &master->dev;
> +	struct fwnode_handle *fwnode;
> +	struct acpi_device *adev;
> +	u32 slv_addr, num_dev;
> +	acpi_status status;
> +	u64 val;
> +
> +	status = acpi_evaluate_object_typed(master->ahandle, "_DSD", NULL, &buf, ACPI_TYPE_PACKAGE);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&master->dev, "Error reading _DSD:%s\n", acpi_format_exception(status));
> +		return -ENODEV;
> +	}
> +
> +	num_dev = device_get_child_node_count(dev);
> +	if (!num_dev) {
> +		dev_err(&master->dev, "Error: no child node present\n");
> +		return -EINVAL;
> +	}
> +
I didn't notice earlier these cause host controller registration to fail 
and thus regression on platforms where DSDT doesn't have these optional 
information for the host controller.

Re: [PATCH v3 4/5] i3c: master: Add support for SETAASA CCC

[Jarkko Nikula]

Hi

On 11/8/24 9:33 AM, Shyam Sundar S K wrote:
> @@ -1907,7 +1926,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>   		goto err_bus_cleanup;
>   	}
>   
> -	i3c_master_add_spd_dev(master, i3cboardinfo);
> +	/*
> +	 * If the I3C slave on the bus is SPD device, then do not follow the regular
> +	 * DAA process. Also, as per SPD spec SETAASA is required for the bus discovery
> +	 * and sending RSTDAA and DISEC is considered as illegal. So skip the entire process
> +	 * if the jdec_spd flag has been identified from the BIOS.
> +	 */
> +	if (master->jdec_spd)
> +		return i3c_master_add_spd_dev(master, i3cboardinfo);
>   
This looks wrong the previous patch adds unconditional call to 
i3c_master_add_spd_dev() and this patch makes it conditional. Can 
previous patch then cause a regression if applied without this one?

>   	if (master->ops->set_speed) {
>   		ret = master->ops->set_speed(master, I3C_OPEN_DRAIN_SLOW_SPEED);
> @@ -2311,6 +2337,10 @@ static int i3c_acpi_configure_master(struct i3c_master_controller *master)
>   		return -ENODEV;
>   	}
>   
> +	status = acpi_evaluate_object(master->ahandle, "_STR", NULL, NULL);
> +	if (ACPI_SUCCESS(status))
> +		master->jdec_spd = true;
> +

I'm still suspicious about this one when existence of _STR for the host 
controller causes normal bus initialization to be skipped. I.e. like below.

Device (I3C0)
{
	_STR ("My I3C Host Controller")
...

Re: [PATCH v3 2/5] i3c: master: Add ACPI support to i3c subsystem

[Heikki Krogerus]

Hi,

On Fri, Nov 08, 2024 at 01:03:20PM +0530, Shyam Sundar S K wrote:
> As of now, the I3C subsystem only has ARM-specific initialization, and
> there is no corresponding ACPI plumbing present. To address this, ACPI
> support needs to be added to both the I3C core and DW driver.
> 
> Add support to get the ACPI handle from the _HID probed and parse the apci
> object to retrieve the slave information from BIOS.
> 
> Based on the acpi object information propogated via BIOS, build the i3c
> board information so that the same information can be used across the
> driver to handle the slave requests.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> Cc: linux-acpi@vger.kernel.org
> 
>  drivers/i3c/internals.h            |  3 ++
>  drivers/i3c/master.c               | 84 ++++++++++++++++++++++++++++++
>  drivers/i3c/master/dw-i3c-master.c |  7 +++
>  include/linux/i3c/master.h         |  1 +
>  4 files changed, 95 insertions(+)
> 
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 433f6088b7ce..178bc0ebe6b6 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -10,6 +10,9 @@
>  
>  #include <linux/i3c/master.h>
>  
> +#define I3C_GET_PID		0x08
> +#define I3C_GET_ADDR		0x7F
> +
>  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
>  
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 6f3eb710a75d..0ceef2aa9161 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2251,6 +2251,84 @@ static int of_i3c_master_add_dev(struct i3c_master_controller *master,
>  	return ret;
>  }
>  
> +#if IS_ENABLED(CONFIG_ACPI)
> +static int i3c_acpi_configure_master(struct i3c_master_controller *master)
> +{
> +	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> +	enum i3c_addr_slot_status addrstatus;
> +	struct i3c_dev_boardinfo *boardinfo;
> +	struct device *dev = &master->dev;
> +	struct fwnode_handle *fwnode;
> +	struct acpi_device *adev;
> +	u32 slv_addr, num_dev;
> +	acpi_status status;
> +	u64 val;
> +
> +	status = acpi_evaluate_object_typed(master->ahandle, "_DSD", NULL, &buf, ACPI_TYPE_PACKAGE);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&master->dev, "Error reading _DSD:%s\n", acpi_format_exception(status));
> +		return -ENODEV;
> +	}

Why do you need to do that?

> +	num_dev = device_get_child_node_count(dev);
> +	if (!num_dev) {
> +		dev_err(&master->dev, "Error: no child node present\n");
> +		return -EINVAL;
> +	}

I think Jarkko already pointed out the problem with that. The whole
check should be dropped.

> +	device_for_each_child_node(dev, fwnode) {
> +		adev = to_acpi_device_node(fwnode);
> +		if (!adev)
> +			return -ENODEV;
> +
> +		status = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &val);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(&master->dev, "Error: eval _ADR failed\n");
> +			return -EINVAL;
> +		}

val = acpi_device_adr(adev);

> +		slv_addr = val & I3C_GET_ADDR;
> +
> +		boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
> +		if (!boardinfo)
> +			return -ENOMEM;
> +
> +		if (slv_addr) {
> +			if (slv_addr > I3C_MAX_ADDR)
> +				return -EINVAL;
> +
> +			addrstatus = i3c_bus_get_addr_slot_status(&master->bus, slv_addr);
> +			if (addrstatus != I3C_ADDR_SLOT_FREE)
> +				return -EINVAL;
> +		}
> +
> +		boardinfo->static_addr = slv_addr;
> +		if (boardinfo->static_addr > I3C_MAX_ADDR)
> +			return -EINVAL;
> +
> +		addrstatus = i3c_bus_get_addr_slot_status(&master->bus,	boardinfo->static_addr);
> +		if (addrstatus != I3C_ADDR_SLOT_FREE)
> +			return -EINVAL;
> +
> +		boardinfo->pid = val >> I3C_GET_PID;
> +		if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> +			return -EINVAL;
> +
> +		/*
> +		 * According to the specification, SETDASA is not supported for DIMM slaves
> +		 * during device discovery. Therefore, BIOS will populate same initial
> +		 * dynamic address as the static address.
> +		 */
> +		boardinfo->init_dyn_addr = boardinfo->static_addr;
> +		list_add_tail(&boardinfo->node, &master->boardinfo.i3c);
> +	}
> +
> +	return 0;
> +}
> +#else
> +static int i3c_acpi_configure_master(struct i3c_master_controller *master) { return 0; }
> +#endif

I think this code should be placed into a separate file.

If the goal is to add ACPI support for code that is written for DT
only, then I think the first thing to do before that really should be
to convert the existing code to use the unified device property
interface, and move all the DT-only parts to a separate file(s).

thanks,

-- 
heikki

Re: [PATCH v3 2/5] i3c: master: Add ACPI support to i3c subsystem

[Shyam Sundar S K]

On 11/13/2024 19:51, Heikki Krogerus wrote:
> Hi,
> 
> On Fri, Nov 08, 2024 at 01:03:20PM +0530, Shyam Sundar S K wrote:
>> As of now, the I3C subsystem only has ARM-specific initialization, and
>> there is no corresponding ACPI plumbing present. To address this, ACPI
>> support needs to be added to both the I3C core and DW driver.
>>
>> Add support to get the ACPI handle from the _HID probed and parse the apci
>> object to retrieve the slave information from BIOS.
>>
>> Based on the acpi object information propogated via BIOS, build the i3c
>> board information so that the same information can be used across the
>> driver to handle the slave requests.
>>
>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>> Cc: linux-acpi@vger.kernel.org
>>
>>  drivers/i3c/internals.h            |  3 ++
>>  drivers/i3c/master.c               | 84 ++++++++++++++++++++++++++++++
>>  drivers/i3c/master/dw-i3c-master.c |  7 +++
>>  include/linux/i3c/master.h         |  1 +
>>  4 files changed, 95 insertions(+)
>>
>> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
>> index 433f6088b7ce..178bc0ebe6b6 100644
>> --- a/drivers/i3c/internals.h
>> +++ b/drivers/i3c/internals.h
>> @@ -10,6 +10,9 @@
>>  
>>  #include <linux/i3c/master.h>
>>  
>> +#define I3C_GET_PID		0x08
>> +#define I3C_GET_ADDR		0x7F
>> +
>>  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>>  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
>>  
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index 6f3eb710a75d..0ceef2aa9161 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -2251,6 +2251,84 @@ static int of_i3c_master_add_dev(struct i3c_master_controller *master,
>>  	return ret;
>>  }
>>  
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +static int i3c_acpi_configure_master(struct i3c_master_controller *master)
>> +{
>> +	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
>> +	enum i3c_addr_slot_status addrstatus;
>> +	struct i3c_dev_boardinfo *boardinfo;
>> +	struct device *dev = &master->dev;
>> +	struct fwnode_handle *fwnode;
>> +	struct acpi_device *adev;
>> +	u32 slv_addr, num_dev;
>> +	acpi_status status;
>> +	u64 val;
>> +
>> +	status = acpi_evaluate_object_typed(master->ahandle, "_DSD", NULL, &buf, ACPI_TYPE_PACKAGE);
>> +	if (ACPI_FAILURE(status)) {
>> +		dev_err(&master->dev, "Error reading _DSD:%s\n", acpi_format_exception(status));
>> +		return -ENODEV;
>> +	}
> 
> Why do you need to do that?
> 
>> +	num_dev = device_get_child_node_count(dev);
>> +	if (!num_dev) {
>> +		dev_err(&master->dev, "Error: no child node present\n");
>> +		return -EINVAL;
>> +	}
> 
> I think Jarkko already pointed out the problem with that. The whole
> check should be dropped.
> 
>> +	device_for_each_child_node(dev, fwnode) {
>> +		adev = to_acpi_device_node(fwnode);
>> +		if (!adev)
>> +			return -ENODEV;
>> +
>> +		status = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &val);
>> +		if (ACPI_FAILURE(status)) {
>> +			dev_err(&master->dev, "Error: eval _ADR failed\n");
>> +			return -EINVAL;
>> +		}
> 
> val = acpi_device_adr(adev);
> 
>> +		slv_addr = val & I3C_GET_ADDR;
>> +
>> +		boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
>> +		if (!boardinfo)
>> +			return -ENOMEM;
>> +
>> +		if (slv_addr) {
>> +			if (slv_addr > I3C_MAX_ADDR)
>> +				return -EINVAL;
>> +
>> +			addrstatus = i3c_bus_get_addr_slot_status(&master->bus, slv_addr);
>> +			if (addrstatus != I3C_ADDR_SLOT_FREE)
>> +				return -EINVAL;
>> +		}
>> +
>> +		boardinfo->static_addr = slv_addr;
>> +		if (boardinfo->static_addr > I3C_MAX_ADDR)
>> +			return -EINVAL;
>> +
>> +		addrstatus = i3c_bus_get_addr_slot_status(&master->bus,	boardinfo->static_addr);
>> +		if (addrstatus != I3C_ADDR_SLOT_FREE)
>> +			return -EINVAL;
>> +
>> +		boardinfo->pid = val >> I3C_GET_PID;
>> +		if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
>> +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
>> +			return -EINVAL;
>> +
>> +		/*
>> +		 * According to the specification, SETDASA is not supported for DIMM slaves
>> +		 * during device discovery. Therefore, BIOS will populate same initial
>> +		 * dynamic address as the static address.
>> +		 */
>> +		boardinfo->init_dyn_addr = boardinfo->static_addr;
>> +		list_add_tail(&boardinfo->node, &master->boardinfo.i3c);
>> +	}
>> +
>> +	return 0;
>> +}
>> +#else
>> +static int i3c_acpi_configure_master(struct i3c_master_controller *master) { return 0; }
>> +#endif
> 
> I think this code should be placed into a separate file.
> 
> If the goal is to add ACPI support for code that is written for DT
> only, then I think the first thing to do before that really should be
> to convert the existing code to use the unified device property
> interface, and move all the DT-only parts to a separate file(s).
> 

Thank you Jarkko and Heikki. Let me work and these remarks and come
back with a new version.

Jarkko, will you be able to pick 1/5 and 5/5 without a separate series
or do you want me to send one?

Thanks,
Shyam
> thanks,
> 

Re: [PATCH v3 2/5] i3c: master: Add ACPI support to i3c subsystem

[Alexandre Belloni]

On 14/11/2024 10:03:13+0530, Shyam Sundar S K wrote:
> 
> 
> On 11/13/2024 19:51, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Fri, Nov 08, 2024 at 01:03:20PM +0530, Shyam Sundar S K wrote:
> >> As of now, the I3C subsystem only has ARM-specific initialization, and
> >> there is no corresponding ACPI plumbing present. To address this, ACPI
> >> support needs to be added to both the I3C core and DW driver.
> >>
> >> Add support to get the ACPI handle from the _HID probed and parse the apci
> >> object to retrieve the slave information from BIOS.
> >>
> >> Based on the acpi object information propogated via BIOS, build the i3c
> >> board information so that the same information can be used across the
> >> driver to handle the slave requests.
> >>
> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >> Cc: linux-acpi@vger.kernel.org
> >>
> >>  drivers/i3c/internals.h            |  3 ++
> >>  drivers/i3c/master.c               | 84 ++++++++++++++++++++++++++++++
> >>  drivers/i3c/master/dw-i3c-master.c |  7 +++
> >>  include/linux/i3c/master.h         |  1 +
> >>  4 files changed, 95 insertions(+)
> >>
> >> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> >> index 433f6088b7ce..178bc0ebe6b6 100644
> >> --- a/drivers/i3c/internals.h
> >> +++ b/drivers/i3c/internals.h
> >> @@ -10,6 +10,9 @@
> >>  
> >>  #include <linux/i3c/master.h>
> >>  
> >> +#define I3C_GET_PID		0x08
> >> +#define I3C_GET_ADDR		0x7F
> >> +
> >>  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
> >>  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> >>  
> >> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> >> index 6f3eb710a75d..0ceef2aa9161 100644
> >> --- a/drivers/i3c/master.c
> >> +++ b/drivers/i3c/master.c
> >> @@ -2251,6 +2251,84 @@ static int of_i3c_master_add_dev(struct i3c_master_controller *master,
> >>  	return ret;
> >>  }
> >>  
> >> +#if IS_ENABLED(CONFIG_ACPI)
> >> +static int i3c_acpi_configure_master(struct i3c_master_controller *master)
> >> +{
> >> +	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> >> +	enum i3c_addr_slot_status addrstatus;
> >> +	struct i3c_dev_boardinfo *boardinfo;
> >> +	struct device *dev = &master->dev;
> >> +	struct fwnode_handle *fwnode;
> >> +	struct acpi_device *adev;
> >> +	u32 slv_addr, num_dev;
> >> +	acpi_status status;
> >> +	u64 val;
> >> +
> >> +	status = acpi_evaluate_object_typed(master->ahandle, "_DSD", NULL, &buf, ACPI_TYPE_PACKAGE);
> >> +	if (ACPI_FAILURE(status)) {
> >> +		dev_err(&master->dev, "Error reading _DSD:%s\n", acpi_format_exception(status));
> >> +		return -ENODEV;
> >> +	}
> > 
> > Why do you need to do that?
> > 
> >> +	num_dev = device_get_child_node_count(dev);
> >> +	if (!num_dev) {
> >> +		dev_err(&master->dev, "Error: no child node present\n");
> >> +		return -EINVAL;
> >> +	}
> > 
> > I think Jarkko already pointed out the problem with that. The whole
> > check should be dropped.
> > 
> >> +	device_for_each_child_node(dev, fwnode) {
> >> +		adev = to_acpi_device_node(fwnode);
> >> +		if (!adev)
> >> +			return -ENODEV;
> >> +
> >> +		status = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &val);
> >> +		if (ACPI_FAILURE(status)) {
> >> +			dev_err(&master->dev, "Error: eval _ADR failed\n");
> >> +			return -EINVAL;
> >> +		}
> > 
> > val = acpi_device_adr(adev);
> > 
> >> +		slv_addr = val & I3C_GET_ADDR;
> >> +
> >> +		boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
> >> +		if (!boardinfo)
> >> +			return -ENOMEM;
> >> +
> >> +		if (slv_addr) {
> >> +			if (slv_addr > I3C_MAX_ADDR)
> >> +				return -EINVAL;
> >> +
> >> +			addrstatus = i3c_bus_get_addr_slot_status(&master->bus, slv_addr);
> >> +			if (addrstatus != I3C_ADDR_SLOT_FREE)
> >> +				return -EINVAL;
> >> +		}
> >> +
> >> +		boardinfo->static_addr = slv_addr;
> >> +		if (boardinfo->static_addr > I3C_MAX_ADDR)
> >> +			return -EINVAL;
> >> +
> >> +		addrstatus = i3c_bus_get_addr_slot_status(&master->bus,	boardinfo->static_addr);
> >> +		if (addrstatus != I3C_ADDR_SLOT_FREE)
> >> +			return -EINVAL;
> >> +
> >> +		boardinfo->pid = val >> I3C_GET_PID;
> >> +		if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> >> +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> >> +			return -EINVAL;
> >> +
> >> +		/*
> >> +		 * According to the specification, SETDASA is not supported for DIMM slaves
> >> +		 * during device discovery. Therefore, BIOS will populate same initial
> >> +		 * dynamic address as the static address.
> >> +		 */
> >> +		boardinfo->init_dyn_addr = boardinfo->static_addr;
> >> +		list_add_tail(&boardinfo->node, &master->boardinfo.i3c);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +#else
> >> +static int i3c_acpi_configure_master(struct i3c_master_controller *master) { return 0; }
> >> +#endif
> > 
> > I think this code should be placed into a separate file.
> > 
> > If the goal is to add ACPI support for code that is written for DT
> > only, then I think the first thing to do before that really should be
> > to convert the existing code to use the unified device property
> > interface, and move all the DT-only parts to a separate file(s).
> > 
> 
> Thank you Jarkko and Heikki. Let me work and these remarks and come
> back with a new version.
> 
> Jarkko, will you be able to pick 1/5 and 5/5 without a separate series
> or do you want me to send one?

Please send a new series.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 2/5] i3c: master: Add ACPI support to i3c subsystem

[Jarkko Nikula]

Hi

On 11/14/24 6:33 AM, Shyam Sundar S K wrote:
> Jarkko, will you be able to pick 1/5 and 5/5 without a separate series
> or do you want me to send one?
> 
I let Alexandre answer to that since he's the I3C subsystem maintainer. 
Some maintainers prefer a new set and others may pick individual patches.

I'll give anyway my reviewed by tags to those two patches.

Re: [PATCH v3 1/5] i3c: dw: Add support for AMDI0015 ACPI ID

[Jarkko Nikula]

On 11/8/24 9:33 AM, Shyam Sundar S K wrote:
> Add AMDI0015 _HID for Designware I3C driver so that the dw-i3c-master
> driver can be probed on AMD platforms.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/i3c/master/dw-i3c-master.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v3 5/5] i3c: dw: Add quirk to address OD/PP timing issue on AMD platform

[Jarkko Nikula]

On 11/8/24 9:33 AM, Shyam Sundar S K wrote:
> The AMD Legacy I3C is having a problem with its IP, specifically with the
> push-pull and open-drain pull-up registers. These registers need to be
> manually programmed for every CCC submission to align with the duty cycle.
> Therefore, add a quirk to address this issue.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/i3c/master/dw-i3c-master.c | 29 ++++++++++++++++++++++++++++-
>   drivers/i3c/master/dw-i3c-master.h |  1 +
>   2 files changed, 29 insertions(+), 1 deletion(-)
> 
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v3 2/5] i3c: master: Add ACPI support to i3c subsystem

[Shyam Sundar S K]

On 11/14/2024 13:26, Alexandre Belloni wrote:
> On 14/11/2024 10:03:13+0530, Shyam Sundar S K wrote:
>>
>>
>> On 11/13/2024 19:51, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Fri, Nov 08, 2024 at 01:03:20PM +0530, Shyam Sundar S K wrote:
>>>> As of now, the I3C subsystem only has ARM-specific initialization, and
>>>> there is no corresponding ACPI plumbing present. To address this, ACPI
>>>> support needs to be added to both the I3C core and DW driver.
>>>>
>>>> Add support to get the ACPI handle from the _HID probed and parse the apci
>>>> object to retrieve the slave information from BIOS.
>>>>
>>>> Based on the acpi object information propogated via BIOS, build the i3c
>>>> board information so that the same information can be used across the
>>>> driver to handle the slave requests.
>>>>
>>>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>> Cc: linux-acpi@vger.kernel.org
>>>>
>>>>  drivers/i3c/internals.h            |  3 ++
>>>>  drivers/i3c/master.c               | 84 ++++++++++++++++++++++++++++++
>>>>  drivers/i3c/master/dw-i3c-master.c |  7 +++
>>>>  include/linux/i3c/master.h         |  1 +
>>>>  4 files changed, 95 insertions(+)
>>>>
>>>> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
>>>> index 433f6088b7ce..178bc0ebe6b6 100644
>>>> --- a/drivers/i3c/internals.h
>>>> +++ b/drivers/i3c/internals.h
>>>> @@ -10,6 +10,9 @@
>>>>  
>>>>  #include <linux/i3c/master.h>
>>>>  
>>>> +#define I3C_GET_PID		0x08
>>>> +#define I3C_GET_ADDR		0x7F
>>>> +
>>>>  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>>>>  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
>>>>  
>>>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>>>> index 6f3eb710a75d..0ceef2aa9161 100644
>>>> --- a/drivers/i3c/master.c
>>>> +++ b/drivers/i3c/master.c
>>>> @@ -2251,6 +2251,84 @@ static int of_i3c_master_add_dev(struct i3c_master_controller *master,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +#if IS_ENABLED(CONFIG_ACPI)
>>>> +static int i3c_acpi_configure_master(struct i3c_master_controller *master)
>>>> +{
>>>> +	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
>>>> +	enum i3c_addr_slot_status addrstatus;
>>>> +	struct i3c_dev_boardinfo *boardinfo;
>>>> +	struct device *dev = &master->dev;
>>>> +	struct fwnode_handle *fwnode;
>>>> +	struct acpi_device *adev;
>>>> +	u32 slv_addr, num_dev;
>>>> +	acpi_status status;
>>>> +	u64 val;
>>>> +
>>>> +	status = acpi_evaluate_object_typed(master->ahandle, "_DSD", NULL, &buf, ACPI_TYPE_PACKAGE);
>>>> +	if (ACPI_FAILURE(status)) {
>>>> +		dev_err(&master->dev, "Error reading _DSD:%s\n", acpi_format_exception(status));
>>>> +		return -ENODEV;
>>>> +	}
>>>
>>> Why do you need to do that?
>>>
>>>> +	num_dev = device_get_child_node_count(dev);
>>>> +	if (!num_dev) {
>>>> +		dev_err(&master->dev, "Error: no child node present\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> I think Jarkko already pointed out the problem with that. The whole
>>> check should be dropped.
>>>
>>>> +	device_for_each_child_node(dev, fwnode) {
>>>> +		adev = to_acpi_device_node(fwnode);
>>>> +		if (!adev)
>>>> +			return -ENODEV;
>>>> +
>>>> +		status = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &val);
>>>> +		if (ACPI_FAILURE(status)) {
>>>> +			dev_err(&master->dev, "Error: eval _ADR failed\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>
>>> val = acpi_device_adr(adev);
>>>
>>>> +		slv_addr = val & I3C_GET_ADDR;
>>>> +
>>>> +		boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
>>>> +		if (!boardinfo)
>>>> +			return -ENOMEM;
>>>> +
>>>> +		if (slv_addr) {
>>>> +			if (slv_addr > I3C_MAX_ADDR)
>>>> +				return -EINVAL;
>>>> +
>>>> +			addrstatus = i3c_bus_get_addr_slot_status(&master->bus, slv_addr);
>>>> +			if (addrstatus != I3C_ADDR_SLOT_FREE)
>>>> +				return -EINVAL;
>>>> +		}
>>>> +
>>>> +		boardinfo->static_addr = slv_addr;
>>>> +		if (boardinfo->static_addr > I3C_MAX_ADDR)
>>>> +			return -EINVAL;
>>>> +
>>>> +		addrstatus = i3c_bus_get_addr_slot_status(&master->bus,	boardinfo->static_addr);
>>>> +		if (addrstatus != I3C_ADDR_SLOT_FREE)
>>>> +			return -EINVAL;
>>>> +
>>>> +		boardinfo->pid = val >> I3C_GET_PID;
>>>> +		if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
>>>> +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
>>>> +			return -EINVAL;
>>>> +
>>>> +		/*
>>>> +		 * According to the specification, SETDASA is not supported for DIMM slaves
>>>> +		 * during device discovery. Therefore, BIOS will populate same initial
>>>> +		 * dynamic address as the static address.
>>>> +		 */
>>>> +		boardinfo->init_dyn_addr = boardinfo->static_addr;
>>>> +		list_add_tail(&boardinfo->node, &master->boardinfo.i3c);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +#else
>>>> +static int i3c_acpi_configure_master(struct i3c_master_controller *master) { return 0; }
>>>> +#endif
>>>
>>> I think this code should be placed into a separate file.
>>>
>>> If the goal is to add ACPI support for code that is written for DT
>>> only, then I think the first thing to do before that really should be
>>> to convert the existing code to use the unified device property
>>> interface, and move all the DT-only parts to a separate file(s).
>>>
>>
>> Thank you Jarkko and Heikki. Let me work and these remarks and come
>> back with a new version.
>>
>> Jarkko, will you be able to pick 1/5 and 5/5 without a separate series
>> or do you want me to send one?
> 
> Please send a new series.

OK. I am spinning out two based on feedback received.

Thanks,
Shyam