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

[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

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

[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

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.