Message ID | 20240807052359.290046-4-Shyam-sundar.S-k@amd.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Introduce initial AMD I3C HCI driver support | expand |
Hi On 8/7/24 8:23 AM, Shyam Sundar S K wrote: > The AMD HCI controller currently only supports PIO mode but exposes DMA > rings to the OS, which leads to the controller being configured in DMA > mode. To address this, add a quirk to avoid configuring the controller in > DMA mode and default to PIO mode. > > Additionally, introduce a generic quirk infrastructure to the mipi-i3c-hci > driver to facilitate seamless future quirk additions. > > Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com> > Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com> > Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> > Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- ... > +void amd_i3c_hci_quirks_init(struct i3c_hci *hci) > +{ > +#if defined(CONFIG_X86) > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > + hci->quirks |= HCI_QUIRK_PIO_MODE; > +#endif > +} I was thinking these quirks can be passed as driver_data more cleanly and be specific only to affected HW if AMD HW would have an unique ACPI ID for each HW version. Above X86_VENDOR_AMD might be too generic if and when quirks are fixed in the future HW :-) So something like: static const struct acpi_device_id i3c_hci_acpi_match[] = { {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | HCI_QUIRK_RESP_BUF_THLD}, {} }; and set them in the i3c_hci_probe() as: hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
On 8/9/2024 19:25, Jarkko Nikula wrote: > Hi > > On 8/7/24 8:23 AM, Shyam Sundar S K wrote: >> The AMD HCI controller currently only supports PIO mode but exposes DMA >> rings to the OS, which leads to the controller being configured in DMA >> mode. To address this, add a quirk to avoid configuring the >> controller in >> DMA mode and default to PIO mode. >> >> Additionally, introduce a generic quirk infrastructure to the >> mipi-i3c-hci >> driver to facilitate seamless future quirk additions. >> >> Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com> >> Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com> >> Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> >> Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- > > ... > >> +void amd_i3c_hci_quirks_init(struct i3c_hci *hci) >> +{ >> +#if defined(CONFIG_X86) >> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) >> + hci->quirks |= HCI_QUIRK_PIO_MODE; >> +#endif >> +} > > I was thinking these quirks can be passed as driver_data more cleanly > and be specific only to affected HW if AMD HW would have an unique > ACPI ID for each HW version. > > Above X86_VENDOR_AMD might be too generic if and when quirks are fixed > in the future HW :-) > > So something like: > > static const struct acpi_device_id i3c_hci_acpi_match[] = { > {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | > HCI_QUIRK_RESP_BUF_THLD}, > {} > }; > > and set them in the i3c_hci_probe() as: > > hci->quirks = (unsigned long)device_get_match_data(&pdev->dev); Nice idea. But only problem is that MSFT wants to have the same ACPI ID present in the specification. I have replied to Andy on patch 1/6. Can you please put your remarks there? Yeah, agreed that having X86_VENDOR_AMD is too generic, but felt its good to have additional checks only after the HW is fixed, rather than being speculative now.. :-) What would you advise? Thanks, Shyam
Hi On 8/9/24 6:44 PM, Shyam Sundar S K wrote: >> I was thinking these quirks can be passed as driver_data more cleanly >> and be specific only to affected HW if AMD HW would have an unique >> ACPI ID for each HW version. >> >> Above X86_VENDOR_AMD might be too generic if and when quirks are fixed >> in the future HW :-) >> >> So something like: >> >> static const struct acpi_device_id i3c_hci_acpi_match[] = { >> {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | >> HCI_QUIRK_RESP_BUF_THLD}, >> {} >> }; >> >> and set them in the i3c_hci_probe() as: >> >> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev); > > Nice idea. But only problem is that MSFT wants to have the same ACPI > ID present in the specification. > > I have replied to Andy on patch 1/6. Can you please put your remarks > there? > Well this is implementation detail later in the series and I found it better to focus ACPI ID discussion in 1/6. > Yeah, agreed that having X86_VENDOR_AMD is too generic, but felt its > good to have additional checks only after the HW is fixed, rather than > being speculative now.. :-) > > What would you advise? > Most probably there will be future HW with either exactly same set of quirks, reduced quirks or new quirks and X86_VENDOR_AMD test will work only with the first case :-)
Hi, On 8/12/2024 14:47, Jarkko Nikula wrote: > Hi > > On 8/9/24 6:44 PM, Shyam Sundar S K wrote: >>> I was thinking these quirks can be passed as driver_data more cleanly >>> and be specific only to affected HW if AMD HW would have an unique >>> ACPI ID for each HW version. >>> >>> Above X86_VENDOR_AMD might be too generic if and when quirks are fixed >>> in the future HW :-) >>> >>> So something like: >>> >>> static const struct acpi_device_id i3c_hci_acpi_match[] = { >>> {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | >>> HCI_QUIRK_RESP_BUF_THLD}, >>> {} >>> }; >>> >>> and set them in the i3c_hci_probe() as: >>> >>> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev); >> >> Nice idea. But only problem is that MSFT wants to have the same ACPI >> ID present in the specification. >> >> I have replied to Andy on patch 1/6. Can you please put your remarks >> there? >> > Well this is implementation detail later in the series and I found it > better to focus ACPI ID discussion in 1/6. > >> Yeah, agreed that having X86_VENDOR_AMD is too generic, but felt its >> good to have additional checks only after the HW is fixed, rather than >> being speculative now.. :-) >> >> What would you advise? >> Most probably there will be future HW with either exactly same set of > quirks, reduced quirks or new quirks and X86_VENDOR_AMD test will work > only with the first case :-) I can add an additional check with the CPU ID and distinguish them(so the quirk gets applied to the affected HW versions) and just not restrict to X86_VENDOR_AMD, would that be fine with you? OTOH, Since these are quirks (where its a broken hardware problems) and the idea you suggested is related to driver data (where driver data is meant to store private information about the device) static const struct acpi_device_id i3c_hci_acpi_match[] = { {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | HCI_QUIRK_RESP_BUF_THLD}, {} }; does that not conflict? quirk vs driver data? I am OK to implement it the way you prefer :-) Thanks, Shyam
On 8/12/2024 15:02, Shyam Sundar S K wrote: > Hi, > > On 8/12/2024 14:47, Jarkko Nikula wrote: >> Hi >> >> On 8/9/24 6:44 PM, Shyam Sundar S K wrote: >>>> I was thinking these quirks can be passed as driver_data more cleanly >>>> and be specific only to affected HW if AMD HW would have an unique >>>> ACPI ID for each HW version. >>>> >>>> Above X86_VENDOR_AMD might be too generic if and when quirks are fixed >>>> in the future HW :-) >>>> >>>> So something like: >>>> >>>> static const struct acpi_device_id i3c_hci_acpi_match[] = { >>>> {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | >>>> HCI_QUIRK_RESP_BUF_THLD}, >>>> {} >>>> }; >>>> >>>> and set them in the i3c_hci_probe() as: >>>> >>>> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev); >>> >>> Nice idea. But only problem is that MSFT wants to have the same ACPI >>> ID present in the specification. >>> >>> I have replied to Andy on patch 1/6. Can you please put your remarks >>> there? >>> >> Well this is implementation detail later in the series and I found it >> better to focus ACPI ID discussion in 1/6. >> >>> Yeah, agreed that having X86_VENDOR_AMD is too generic, but felt its >>> good to have additional checks only after the HW is fixed, rather than >>> being speculative now.. :-) >>> >>> What would you advise? >>> Most probably there will be future HW with either exactly same set of >> quirks, reduced quirks or new quirks and X86_VENDOR_AMD test will work >> only with the first case :-) > > I can add an additional check with the CPU ID and distinguish them(so > the quirk gets applied to the affected HW versions) and just not > restrict to X86_VENDOR_AMD, would that be fine with you? > > OTOH, Since these are quirks (where its a broken hardware problems) > and the idea you suggested is related to driver data (where driver > data is meant to store private information about the device) > > static const struct acpi_device_id i3c_hci_acpi_match[] = { > {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | > HCI_QUIRK_RESP_BUF_THLD}, > {} > }; > > does that not conflict? quirk vs driver data? > > I am OK to implement it the way you prefer :-) Jarkko, any feedback on this? Thanks, Shyam > > Thanks, > Shyam > > >
On 8/19/24 9:41 AM, Shyam Sundar S K wrote: >> I can add an additional check with the CPU ID and distinguish them(so >> the quirk gets applied to the affected HW versions) and just not >> restrict to X86_VENDOR_AMD, would that be fine with you? >> >> OTOH, Since these are quirks (where its a broken hardware problems) >> and the idea you suggested is related to driver data (where driver >> data is meant to store private information about the device) >> >> static const struct acpi_device_id i3c_hci_acpi_match[] = { >> {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | >> HCI_QUIRK_RESP_BUF_THLD}, >> {} >> }; >> >> does that not conflict? quirk vs driver data? >> >> I am OK to implement it the way you prefer :-) > > Jarkko, any feedback on this? > Sorry, forgot to reply... What do you mean about conflict? So if driver data would pass quirk bits as above and set only to unique ACPI ID specific to that HW then there is no reason to check CPU ID later in the code.
On 8/19/2024 16:40, Jarkko Nikula wrote: > On 8/19/24 9:41 AM, Shyam Sundar S K wrote: >>> I can add an additional check with the CPU ID and distinguish them(so >>> the quirk gets applied to the affected HW versions) and just not >>> restrict to X86_VENDOR_AMD, would that be fine with you? >>> >>> OTOH, Since these are quirks (where its a broken hardware problems) >>> and the idea you suggested is related to driver data (where driver >>> data is meant to store private information about the device) >>> >>> static const struct acpi_device_id i3c_hci_acpi_match[] = { >>> {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | >>> HCI_QUIRK_RESP_BUF_THLD}, >>> {} >>> }; >>> >>> does that not conflict? quirk vs driver data? >>> >>> I am OK to implement it the way you prefer :-) >> >> Jarkko, any feedback on this? >> > Sorry, forgot to reply... What do you mean about conflict? So if > driver data would pass quirk bits as above and set only to unique ACPI > ID specific to that HW then there is no reason to check CPU ID later > in the code. OK. Let me respin a new version. Thanks, Shyam
diff --git a/drivers/i3c/master/mipi-i3c-hci/Makefile b/drivers/i3c/master/mipi-i3c-hci/Makefile index a658e7b8262c..1f8cd5c48fde 100644 --- a/drivers/i3c/master/mipi-i3c-hci/Makefile +++ b/drivers/i3c/master/mipi-i3c-hci/Makefile @@ -3,4 +3,5 @@ obj-$(CONFIG_MIPI_I3C_HCI) += mipi-i3c-hci.o mipi-i3c-hci-y := core.o ext_caps.o pio.o dma.o \ cmd_v1.o cmd_v2.o \ - dat_v1.o dct_v1.o + dat_v1.o dct_v1.o \ + hci_quirks.o diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c index a16da70bdfe1..4926fde6087d 100644 --- a/drivers/i3c/master/mipi-i3c-hci/core.c +++ b/drivers/i3c/master/mipi-i3c-hci/core.c @@ -754,6 +754,13 @@ static int i3c_hci_init(struct i3c_hci *hci) return -EINVAL; } + /* Initialize quirks for AMD platforms */ + amd_i3c_hci_quirks_init(hci); + + regval = reg_read(HCI_VERSION); + if (hci->quirks & HCI_QUIRK_PIO_MODE) + hci->RHS_regs = NULL; + /* Try activating DMA operations first */ if (hci->RHS_regs) { reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE); diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h index f94d95e024be..a7ea37f8f8e0 100644 --- a/drivers/i3c/master/mipi-i3c-hci/hci.h +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h @@ -135,11 +135,13 @@ struct i3c_hci_dev_data { /* list of quirks */ #define HCI_QUIRK_RAW_CCC BIT(1) /* CCC framing must be explicit */ +#define HCI_QUIRK_PIO_MODE BIT(2) /* Set PIO mode for AMD platforms */ /* global functions */ void mipi_i3c_hci_resume(struct i3c_hci *hci); void mipi_i3c_hci_pio_reset(struct i3c_hci *hci); void mipi_i3c_hci_dct_index_reset(struct i3c_hci *hci); +void amd_i3c_hci_quirks_init(struct i3c_hci *hci); #endif diff --git a/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c new file mode 100644 index 000000000000..897c0231f585 --- /dev/null +++ b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * I3C HCI Quirks + * + * Copyright 2024 Advanced Micro Devices, Inc. + * + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> + * Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> + */ + +#include <linux/i3c/master.h> +#include "hci.h" + +void amd_i3c_hci_quirks_init(struct i3c_hci *hci) +{ +#if defined(CONFIG_X86) + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) + hci->quirks |= HCI_QUIRK_PIO_MODE; +#endif +}