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@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?
>
> Please send a new series.
OK. I am spinning out two based on feedback received.
Thanks,
Shyam
