| Message ID | 20240821133554.391937-2-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce initial AMD I3C HCI driver support | expand |
+ Andy On 8/21/2024 19:05, Shyam Sundar S K wrote: > The current driver code lacks the necessary plumbing for ACPI IDs, > preventing the mipi-i3c-hci driver from being loaded on x86 > platforms that advertise I3C ACPI support. > > This update adds the AMDI5017 ACPI ID to the list of supported IDs. > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/i3c/master/mipi-i3c-hci/core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c > index 4e7d6a43ee9b..b02fbd7882f8 100644 > --- a/drivers/i3c/master/mipi-i3c-hci/core.c > +++ b/drivers/i3c/master/mipi-i3c-hci/core.c > @@ -834,12 +834,19 @@ static const __maybe_unused struct of_device_id i3c_hci_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, i3c_hci_of_match); > > +static const struct acpi_device_id i3c_hci_acpi_match[] = { > + {"AMDI5017"}, > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match); > + > static struct platform_driver i3c_hci_driver = { > .probe = i3c_hci_probe, > .remove_new = i3c_hci_remove, > .driver = { > .name = "mipi-i3c-hci", > .of_match_table = of_match_ptr(i3c_hci_of_match), > + .acpi_match_table = i3c_hci_acpi_match, > }, > }; > module_platform_driver(i3c_hci_driver);
On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote: > + Andy Thank you! > On 8/21/2024 19:05, Shyam Sundar S K wrote: > > The current driver code lacks the necessary plumbing for ACPI IDs, > > preventing the mipi-i3c-hci driver from being loaded on x86 > > platforms that advertise I3C ACPI support. > > > > This update adds the AMDI5017 ACPI ID to the list of supported IDs. Please, provide a DSDT excerpt to show how it will look like in the ACPI tables. ... > > +static const struct acpi_device_id i3c_hci_acpi_match[] = { > > + {"AMDI5017"}, Spaces are missing { "AMDI5017" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match); ... Otherwise LGTM, thanks!
On 8/21/2024 19:26, Andy Shevchenko wrote: > On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote: >> + Andy > > Thank you! > >> On 8/21/2024 19:05, Shyam Sundar S K wrote: >>> The current driver code lacks the necessary plumbing for ACPI IDs, >>> preventing the mipi-i3c-hci driver from being loaded on x86 >>> platforms that advertise I3C ACPI support. >>> >>> This update adds the AMDI5017 ACPI ID to the list of supported IDs. > > Please, provide a DSDT excerpt to show how it will look like in the ACPI > tables. Scope (_SB) { ... Name (HCID, "AMDI5017") Device (I3CA) { Method (_HID, 0, Serialized) // _HID: Hardware ID { If ((I30M == Zero)) { If (CondRefOf (HCIB)) { Return (HCID) /* \_SB_.HCID */ } Else { Return (I3ID) /* \_SB_.I3ID */ } } Else { Return (I2ID) /* \_SB_.I2ID */ } } ... } > > ... > >>> +static const struct acpi_device_id i3c_hci_acpi_match[] = { >>> + {"AMDI5017"}, > > Spaces are missing > > { "AMDI5017" }, OK. Will change it. > >>> + {} >>> +}; >>> +MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match); > > ... > > Otherwise LGTM, thanks! > Thanks, Shyam
On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote: > On 8/21/2024 19:26, Andy Shevchenko wrote: > > On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote: > >> On 8/21/2024 19:05, Shyam Sundar S K wrote: ... > >>> This update adds the AMDI5017 ACPI ID to the list of supported IDs. s/This update adds/Add/ > > Please, provide a DSDT excerpt to show how it will look like in the ACPI > > tables. > > Scope (_SB) > { > ... > > Name (HCID, "AMDI5017") > Device (I3CA) > { > Method (_HID, 0, Serialized) // _HID: Hardware ID > { > If ((I30M == Zero)) > { > If (CondRefOf (HCIB)) > { > Return (HCID) /* \_SB_.HCID */ > } > Else > { > Return (I3ID) /* \_SB_.I3ID */ Do I understand correctly that I3ID is the old one (as per another path I have seen last week or so), i.o.w. *not* a MIPI allocated one? If it's the case, feel free to add Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> from ACPI ID perspective. > } > } > Else > { > Return (I2ID) /* \_SB_.I2ID */ > } > } > > ... > }
On Wed, Aug 21, 2024 at 06:39:04PM +0300, Andy Shevchenko wrote: > On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote: > > On 8/21/2024 19:26, Andy Shevchenko wrote: > > > On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote: > > >> On 8/21/2024 19:05, Shyam Sundar S K wrote: ... > > >>> This update adds the AMDI5017 ACPI ID to the list of supported IDs. > > s/This update adds/Add/ > > > > Please, provide a DSDT excerpt to show how it will look like in the ACPI > > > tables. > > > > Scope (_SB) > > { > > ... > > > > Name (HCID, "AMDI5017") > > Device (I3CA) > > { > > Method (_HID, 0, Serialized) // _HID: Hardware ID > > { > > If ((I30M == Zero)) > > { > > If (CondRefOf (HCIB)) > > { > > Return (HCID) /* \_SB_.HCID */ > > } > > Else > > { > > Return (I3ID) /* \_SB_.I3ID */ > > Do I understand correctly that I3ID is the old one (as per another path I have > seen last week or so), i.o.w. *not* a MIPI allocated one? > > If it's the case, feel free to add > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > from ACPI ID perspective. Regarding MIPI ID and using it as a _CID is kinda unsolved now, in any case _CID *requires* _HID to be present, and hence _HID has a priority in enumeration. It doesn't matter if it's absent now (it's even more flexible in case MIPI decides to use _another_ ID for _CID) as long as software uses _HID for enumeration. > > } > > } > > Else > > { > > Return (I2ID) /* \_SB_.I2ID */ > > } > > } > > > > ... > > }
On Wed, Aug 21, 2024 at 06:42:13PM +0300, Andy Shevchenko wrote: > On Wed, Aug 21, 2024 at 06:39:04PM +0300, Andy Shevchenko wrote: > > On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote: > > > On 8/21/2024 19:26, Andy Shevchenko wrote: > > > > On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote: > > > >> On 8/21/2024 19:05, Shyam Sundar S K wrote: ... > > > >>> This update adds the AMDI5017 ACPI ID to the list of supported IDs. > > > > s/This update adds/Add/ > > > > > > Please, provide a DSDT excerpt to show how it will look like in the ACPI > > > > tables. > > > > > > Scope (_SB) > > > { > > > ... > > > > > > Name (HCID, "AMDI5017") > > > Device (I3CA) > > > { > > > Method (_HID, 0, Serialized) // _HID: Hardware ID > > > { > > > If ((I30M == Zero)) > > > { > > > If (CondRefOf (HCIB)) > > > { > > > Return (HCID) /* \_SB_.HCID */ > > > } > > > Else > > > { > > > Return (I3ID) /* \_SB_.I3ID */ > > > > Do I understand correctly that I3ID is the old one (as per another path I have > > seen last week or so), i.o.w. *not* a MIPI allocated one? > > > > If it's the case, feel free to add > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > from ACPI ID perspective. > > Regarding MIPI ID and using it as a _CID is kinda unsolved now, in any case > _CID *requires* _HID to be present, and hence _HID has a priority in > enumeration. It doesn't matter if it's absent now (it's even more flexible in Just to be crystal clear "It doesn't matter if _CID is absent now..." > case MIPI decides to use _another_ ID for _CID) as long as software uses > _HID for enumeration. > > > > } > > > } > > > Else > > > { > > > Return (I2ID) /* \_SB_.I2ID */ > > > } > > > } > > > > > > ... > > > }
On 8/21/2024 21:09, Andy Shevchenko wrote: > On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote: >> On 8/21/2024 19:26, Andy Shevchenko wrote: >>> On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote: >>>> On 8/21/2024 19:05, Shyam Sundar S K wrote: > > ... > >>>>> This update adds the AMDI5017 ACPI ID to the list of supported IDs. > > s/This update adds/Add/ > >>> Please, provide a DSDT excerpt to show how it will look like in the ACPI >>> tables. >> >> Scope (_SB) >> { >> ... >> >> Name (HCID, "AMDI5017") >> Device (I3CA) >> { >> Method (_HID, 0, Serialized) // _HID: Hardware ID >> { >> If ((I30M == Zero)) >> { >> If (CondRefOf (HCIB)) >> { >> Return (HCID) /* \_SB_.HCID */ >> } >> Else >> { >> Return (I3ID) /* \_SB_.I3ID */ > > Do I understand correctly that I3ID is the old one (as per another path I have > seen last week or so), i.o.w. *not* a MIPI allocated one? Yes. That's right. > > If it's the case, feel free to add > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > from ACPI ID perspective. > >> } >> } >> Else >> { >> Return (I2ID) /* \_SB_.I2ID */ >> } >> } >> >> ... >> } >
On 8/21/2024 21:13, Andy Shevchenko wrote: > On Wed, Aug 21, 2024 at 06:42:13PM +0300, Andy Shevchenko wrote: >> On Wed, Aug 21, 2024 at 06:39:04PM +0300, Andy Shevchenko wrote: >>> On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote: >>>> On 8/21/2024 19:26, Andy Shevchenko wrote: >>>>> On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote: >>>>>> On 8/21/2024 19:05, Shyam Sundar S K wrote: > > ... > >>>>>>> This update adds the AMDI5017 ACPI ID to the list of supported IDs. >>> >>> s/This update adds/Add/ >>> >>>>> Please, provide a DSDT excerpt to show how it will look like in the ACPI >>>>> tables. >>>> >>>> Scope (_SB) >>>> { >>>> ... >>>> >>>> Name (HCID, "AMDI5017") >>>> Device (I3CA) >>>> { >>>> Method (_HID, 0, Serialized) // _HID: Hardware ID >>>> { >>>> If ((I30M == Zero)) >>>> { >>>> If (CondRefOf (HCIB)) >>>> { >>>> Return (HCID) /* \_SB_.HCID */ >>>> } >>>> Else >>>> { >>>> Return (I3ID) /* \_SB_.I3ID */ >>> >>> Do I understand correctly that I3ID is the old one (as per another path I have >>> seen last week or so), i.o.w. *not* a MIPI allocated one? >>> >>> If it's the case, feel free to add >>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> from ACPI ID perspective. >> >> Regarding MIPI ID and using it as a _CID is kinda unsolved now, in any case >> _CID *requires* _HID to be present, and hence _HID has a priority in >> enumeration. It doesn't matter if it's absent now (it's even more flexible in > > Just to be crystal clear > > "It doesn't matter if _CID is absent now..." Thanks! yeah, and that's right. I have left this decision to my BIOS peers. btw, I have notified MIPI about this error in the MIPI I3C DisCo Specification. Thanks, Shyam > >> case MIPI decides to use _another_ ID for _CID) as long as software uses >> _HID for enumeration. >> >>>> } >>>> } >>>> Else >>>> { >>>> Return (I2ID) /* \_SB_.I2ID */ >>>> } >>>> } >>>> >>>> ... >>>> } >
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c index 4e7d6a43ee9b..b02fbd7882f8 100644 --- a/drivers/i3c/master/mipi-i3c-hci/core.c +++ b/drivers/i3c/master/mipi-i3c-hci/core.c @@ -834,12 +834,19 @@ static const __maybe_unused struct of_device_id i3c_hci_of_match[] = { }; MODULE_DEVICE_TABLE(of, i3c_hci_of_match); +static const struct acpi_device_id i3c_hci_acpi_match[] = { + {"AMDI5017"}, + {} +}; +MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match); + static struct platform_driver i3c_hci_driver = { .probe = i3c_hci_probe, .remove_new = i3c_hci_remove, .driver = { .name = "mipi-i3c-hci", .of_match_table = of_match_ptr(i3c_hci_of_match), + .acpi_match_table = i3c_hci_acpi_match, }, }; module_platform_driver(i3c_hci_driver);
The current driver code lacks the necessary plumbing for ACPI IDs, preventing the mipi-i3c-hci driver from being loaded on x86 platforms that advertise I3C ACPI support. This update adds the AMDI5017 ACPI ID to the list of supported IDs. Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> --- drivers/i3c/master/mipi-i3c-hci/core.c | 7 +++++++ 1 file changed, 7 insertions(+)