| Message ID | 20240807052359.290046-2-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce initial AMD I3C HCI driver support | expand |
Hi I Cc'ed Andy and Rafael because of ACPI ID allocation question that came to my mind below which I'm not expert enough to answer. On 8/7/24 8:23 AM, 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 MIPI0100 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..24dd4603d6c6 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[] = { > + {"MIPI0100"}, > + {} > +}; I started thinking that because of quirks would AMD need to allocate an own ACPI ID for each of your HW version and not use generic MIPI ID? Then passing AMD specific quirks would be easy via driver_data here.
On Fri, Aug 09, 2024 at 04:54:18PM +0300, Jarkko Nikula wrote: > Hi > > I Cc'ed Andy and Rafael because of ACPI ID allocation question that came to > my mind below which I'm not expert enough to answer. > > On 8/7/24 8:23 AM, 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 MIPI0100 ACPI ID to the list of supported IDs. When adding a new ACPI ID, always provide the following information: 1) link (in some form) to the official confirmation / documentation for the allocated ID by the vendor (MIPI in this case) _OR_ (very exceptional!) why the bad ID had been allocated; 2) are there devices in the wild (on the market) that use the being added ID(s)? 3) excerpt from the device (independently if it's public already, see above, or not) DSDT ACPI table. With the given patch it looks to me that you most likely need a local, AMD specific ID as well. So, in my ideal world the DSDT should be like Device (I3CC) { Name (_HID, "...") // AMD specific _HID Name (_CID, "MIPI0100") // Compatible ID for generic I3C controller ... } Is this the case? Why not? P.S. Make sure you Cc me on ACPI ID matters in the future.
On 8/9/2024 19:48, Andy Shevchenko wrote: > On Fri, Aug 09, 2024 at 04:54:18PM +0300, Jarkko Nikula wrote: >> Hi >> >> I Cc'ed Andy and Rafael because of ACPI ID allocation question that came to >> my mind below which I'm not expert enough to answer. >> >> On 8/7/24 8:23 AM, 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 MIPI0100 ACPI ID to the list of supported IDs. > > When adding a new ACPI ID, always provide the following information: > > 1) link (in some form) to the official confirmation / documentation for > the allocated ID by the vendor (MIPI in this case) _OR_ (very exceptional!) > why the bad ID had been allocated; Member version: https://members.mipi.org/wg/All-Members/document/previewpdf/89465 Public version: https://www.mipi.org/mipi-disco-for-i3c-download (this requires a signup). Since there is no direct link available for preview, I did not include them in the commit-msg. But left a note that the MIPI ID is the one as specified in the MIPI DisCo spec. > > 2) are there devices in the wild (on the market) that use the being added ID(s)? > Not in the wild. But the latest platform will have this support included. So, these device IDs are crucial for the i3c-hci to be supported on AMD platforms. > 3) excerpt from the device (independently if it's public already, see above, > or not) DSDT ACPI table. > > With the given patch it looks to me that you most likely need a local, AMD > specific ID as well. > > So, in my ideal world the DSDT should be like > > Device (I3CC) > { > Name (_HID, "...") // AMD specific _HID > Name (_CID, "MIPI0100") // Compatible ID for generic I3C controller > ... > } > > Is this the case? Why not? Please refer to the MIPI HCI I3C DisCo specification (https://members.mipi.org/wg/All-Members/document/previewpdf/89465) section 5.4. The ASL looks the same in case of AMD. MSFT says that they want to use MIPI0100 as mentioned in the specification. What would you advise? > > P.S. Make sure you Cc me on ACPI ID matters in the future. > Sure, will do. Thanks, Shyam
On Fri, Aug 09, 2024 at 09:02:35PM +0530, Shyam Sundar S K wrote: > On 8/9/2024 19:48, Andy Shevchenko wrote: > > On Fri, Aug 09, 2024 at 04:54:18PM +0300, Jarkko Nikula wrote: > >> On 8/7/24 8:23 AM, Shyam Sundar S K wrote: ... > > When adding a new ACPI ID, always provide the following information: > > > > 1) link (in some form) to the official confirmation / documentation for > > the allocated ID by the vendor (MIPI in this case) _OR_ (very exceptional!) > > why the bad ID had been allocated; > > Member version: > https://members.mipi.org/wg/All-Members/document/previewpdf/89465 > > Public version: https://www.mipi.org/mipi-disco-for-i3c-download (this > requires a signup). > > Since there is no direct link available for preview, I did not include > them in the commit-msg. But left a note that the MIPI ID is the one as > specified in the MIPI DisCo spec. It's fine. > > 2) are there devices in the wild (on the market) that use the being added ID(s)? > > Not in the wild. But the latest platform will have this support > included. So, these device IDs are crucial for the i3c-hci to be > supported on AMD platforms. Good, let's do it right then! > > 3) excerpt from the device (independently if it's public already, see above, > > or not) DSDT ACPI table. > > > > With the given patch it looks to me that you most likely need a local, AMD > > specific ID as well. > > > > So, in my ideal world the DSDT should be like > > > > Device (I3CC) > > { > > Name (_HID, "...") // AMD specific _HID > > Name (_CID, "MIPI0100") // Compatible ID for generic I3C controller > > ... > > } > > > > Is this the case? Why not? > > Please refer to the MIPI HCI I3C DisCo specification > (https://members.mipi.org/wg/All-Members/document/previewpdf/89465) > section 5.4. The ASL looks the same in case of AMD. > > MSFT says that they want to use MIPI0100 as mentioned in the > specification. MIPI doesn't know how to assign the ACPI ID correctly. But again, what I put in the above is the correct way of approaching. > What would you advise? Since my intuition and experience tells me that the two devices even based on the same IP are not the same (see word 'quirk' or '.driver_data' or alike in the kernel sources) the generic ID may not be used for the specific vendor unless it's _the only_ vendor for the certain IP. So, please do as I suggested above. And file a error report (and correction proposal) to the MIPI, so in "5.1 I3C Host Controller ACPI Hardware ID (_HID)" they should use _CID instead of _HID and add some text like "Each vendor should dedicate it's own _HID for the platform in question. The same _HID as _CID may be used if and only if vendor guarantees that there 100% compatibility with MIPI as described in this and other related documents." I.o.w. do you 100% guarantee that MIPI HCI I3C DisCo covers all necessary properties that you need for _your_ hardware? If not, use my approach, if yes, use the same _HID *and* _CID. Microsoft should know this as well and much better than MIPI.
On 8/9/2024 21:27, Andy Shevchenko wrote: > On Fri, Aug 09, 2024 at 09:02:35PM +0530, Shyam Sundar S K wrote: >> On 8/9/2024 19:48, Andy Shevchenko wrote: >>> On Fri, Aug 09, 2024 at 04:54:18PM +0300, Jarkko Nikula wrote: >>>> On 8/7/24 8:23 AM, Shyam Sundar S K wrote: > > ... > >>> When adding a new ACPI ID, always provide the following information: >>> >>> 1) link (in some form) to the official confirmation / documentation for >>> the allocated ID by the vendor (MIPI in this case) _OR_ (very exceptional!) >>> why the bad ID had been allocated; >> >> Member version: >> https://members.mipi.org/wg/All-Members/document/previewpdf/89465 >> >> Public version: https://www.mipi.org/mipi-disco-for-i3c-download (this >> requires a signup). >> >> Since there is no direct link available for preview, I did not include >> them in the commit-msg. But left a note that the MIPI ID is the one as >> specified in the MIPI DisCo spec. > > It's fine. > >>> 2) are there devices in the wild (on the market) that use the being added ID(s)? >> >> Not in the wild. But the latest platform will have this support >> included. So, these device IDs are crucial for the i3c-hci to be >> supported on AMD platforms. > > Good, let's do it right then! > >>> 3) excerpt from the device (independently if it's public already, see above, >>> or not) DSDT ACPI table. >>> >>> With the given patch it looks to me that you most likely need a local, AMD >>> specific ID as well. >>> >>> So, in my ideal world the DSDT should be like >>> >>> Device (I3CC) >>> { >>> Name (_HID, "...") // AMD specific _HID >>> Name (_CID, "MIPI0100") // Compatible ID for generic I3C controller >>> ... >>> } >>> >>> Is this the case? Why not? >> >> Please refer to the MIPI HCI I3C DisCo specification >> (https://members.mipi.org/wg/All-Members/document/previewpdf/89465) >> section 5.4. The ASL looks the same in case of AMD. >> >> MSFT says that they want to use MIPI0100 as mentioned in the >> specification. > > MIPI doesn't know how to assign the ACPI ID correctly. But again, what I put in > the above is the correct way of approaching. > >> What would you advise? > > Since my intuition and experience tells me that the two devices even based on > the same IP are not the same (see word 'quirk' or '.driver_data' or alike in > the kernel sources) the generic ID may not be used for the specific vendor > unless it's _the only_ vendor for the certain IP. > > So, please do as I suggested above. And file a error report (and correction > proposal) to the MIPI, so in "5.1 I3C Host Controller ACPI Hardware ID (_HID)" Thank you. I shall file a error report soon to get it corrected. > they should use _CID instead of _HID and add some text like > "Each vendor should dedicate it's own _HID for the platform in question. The > same _HID as _CID may be used if and only if vendor guarantees that there 100% > compatibility with MIPI as described in this and other related documents." > > I.o.w. do you 100% guarantee that MIPI HCI I3C DisCo covers all necessary > properties that you need for _your_ hardware? If not, use my approach, if yes, > use the same _HID *and* _CID. > > Microsoft should know this as well and much better than MIPI. > Agree with your thoughts. I will respin based on your remarks after internal discussion. If it requires _HID to be MIPI0100, I will come back with answers. Thanks, Shyam
On 09/08/2024 18:57:04+0300, Andy Shevchenko wrote: > > Please refer to the MIPI HCI I3C DisCo specification > > (https://members.mipi.org/wg/All-Members/document/previewpdf/89465) > > section 5.4. The ASL looks the same in case of AMD. > > > > MSFT says that they want to use MIPI0100 as mentioned in the > > specification. > > MIPI doesn't know how to assign the ACPI ID correctly. But again, what I put in > the above is the correct way of approaching. > > > What would you advise? > > Since my intuition and experience tells me that the two devices even based on > the same IP are not the same (see word 'quirk' or '.driver_data' or alike in > the kernel sources) the generic ID may not be used for the specific vendor > unless it's _the only_ vendor for the certain IP. Just to be clear, the HCI defines the register interface to the IP but not the IP itself, this is just like the various USB and SD HCIs. So we will definitively see quirks as implementers will interpret the interface differently (and so I agree with everything that was said ;) ) > > So, please do as I suggested above. And file a error report (and correction > proposal) to the MIPI, so in "5.1 I3C Host Controller ACPI Hardware ID (_HID)" > they should use _CID instead of _HID and add some text like > "Each vendor should dedicate it's own _HID for the platform in question. The > same _HID as _CID may be used if and only if vendor guarantees that there 100% > compatibility with MIPI as described in this and other related documents." > > I.o.w. do you 100% guarantee that MIPI HCI I3C DisCo covers all necessary > properties that you need for _your_ hardware? If not, use my approach, if yes, > use the same _HID *and* _CID. > > Microsoft should know this as well and much better than MIPI. > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Aug 09, 2024 at 08:39:40PM +0200, Alexandre Belloni wrote: > On 09/08/2024 18:57:04+0300, Andy Shevchenko wrote: > > > Please refer to the MIPI HCI I3C DisCo specification > > > (https://members.mipi.org/wg/All-Members/document/previewpdf/89465) > > > section 5.4. The ASL looks the same in case of AMD. > > > > > > MSFT says that they want to use MIPI0100 as mentioned in the > > > specification. > > > > MIPI doesn't know how to assign the ACPI ID correctly. But again, what I put in > > the above is the correct way of approaching. > > > > > What would you advise? > > > > Since my intuition and experience tells me that the two devices even based on > > the same IP are not the same (see word 'quirk' or '.driver_data' or alike in > > the kernel sources) the generic ID may not be used for the specific vendor > > unless it's _the only_ vendor for the certain IP. > > Just to be clear, the HCI defines the register interface to the IP but > not the IP itself, this is just like the various USB and SD HCIs. Thank you for this elaboration, it's a crucial aspect! > So we will definitively see quirks as implementers will interpret the > interface differently With the above remark this makes a lot of sense. > (and so I agree with everything that was said ;) ) > > So, please do as I suggested above. And file a error report (and correction > > proposal) to the MIPI, so in "5.1 I3C Host Controller ACPI Hardware ID (_HID)" > > they should use _CID instead of _HID and add some text like > > "Each vendor should dedicate it's own _HID for the platform in question. The > > same _HID as _CID may be used if and only if vendor guarantees that there 100% > > compatibility with MIPI as described in this and other related documents." > > > > I.o.w. do you 100% guarantee that MIPI HCI I3C DisCo covers all necessary > > properties that you need for _your_ hardware? If not, use my approach, if yes, > > use the same _HID *and* _CID. > > > > Microsoft should know this as well and much better than MIPI.
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c index 4e7d6a43ee9b..24dd4603d6c6 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[] = { + {"MIPI0100"}, + {} +}; +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 MIPI0100 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(+)