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

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

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