| Message ID | 20241108073323.523805-3-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce initial support for the AMD I3C (non-HCI) to DW driver | expand |
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?
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.
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, >
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; }; /**