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@xxxxxxx>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>> ---
>> Cc: linux-acpi@xxxxxxxxxxxxxxx
>>
>> 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,
>
