[RESEND,v3,3/6] i3c: mipi-i3c-hci: Add a quirk to set PIO mode

Commit Message

Shyam Sundar S K Aug. 7, 2024, 5:23 a.m. UTC

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 <[e-mail]>
Signed-off-by: Krishnamoorthi M <[e-mail]>
Co-developed-by: Guruvendra Punugupati <[e-mail]>
Signed-off-by: Guruvendra Punugupati <[e-mail]>
Signed-off-by: Shyam Sundar S K <[e-mail]>
---
 drivers/i3c/master/mipi-i3c-hci/Makefile     |  3 ++-
 drivers/i3c/master/mipi-i3c-hci/core.c       |  7 +++++++
 drivers/i3c/master/mipi-i3c-hci/hci.h        |  2 ++
 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 20 ++++++++++++++++++++
 4 files changed, 31 insertions(+), 1 deletion(-)
 create mode 100644 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c

Comments

Jarkko Nikula Aug. 9, 2024, 1:55 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Krishnamoorthi M <[e-mail]>
> Co-developed-by: Guruvendra Punugupati <[e-mail]>
> Signed-off-by: Guruvendra Punugupati <[e-mail]>
> Signed-off-by: Shyam Sundar S K <[e-mail]>
> ---

...

> +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);

Shyam Sundar S K Aug. 9, 2024, 3:44 p.m. UTC | #2

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 <[e-mail]>
>> Signed-off-by: Krishnamoorthi M <[e-mail]>
>> Co-developed-by: Guruvendra Punugupati <[e-mail]>
>> Signed-off-by: Guruvendra Punugupati <[e-mail]>
>> Signed-off-by: Shyam Sundar S K <[e-mail]>
>> ---
> 
> ...
> 
>> +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

Jarkko Nikula Aug. 12, 2024, 9:17 a.m. UTC | #3

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 :-)

Shyam Sundar S K Aug. 12, 2024, 9:32 a.m. UTC | #4

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

Shyam Sundar S K Aug. 19, 2024, 6:41 a.m. UTC | #5

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
> 
> 
>

Jarkko Nikula Aug. 19, 2024, 11:10 a.m. UTC | #6

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.

Shyam Sundar S K Aug. 19, 2024, 4:35 p.m. UTC | #7

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

Patch

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 <[e-mail]>
+ *	    Guruvendra Punugupati <[e-mail]>
+ */
+
+#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
+}