1. Patchew
  2. linux
  3. [PATCH v2 0/5] Introduce initial AMD I3C HCI driver support
  4. View patch

[PATCH v2 2/5] i3c: mipi-i3c-hci: Add a quirk to set PIO mode

Shyam Sundar S K posted 5 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH v2 2/5] i3c: mipi-i3c-hci: Add a quirk to set PIO mode
Posted by Shyam Sundar S K 1 month, 3 weeks ago 
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>
---
 drivers/i3c/master/mipi-i3c-hci/Makefile |  3 ++-
 drivers/i3c/master/mipi-i3c-hci/core.c   | 15 ++++++++++++++-
 drivers/i3c/master/mipi-i3c-hci/hci.h    |  3 +++
 3 files changed, 19 insertions(+), 2 deletions(-)

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 dbc8c38bd962..8bb422ab1d01 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -33,6 +33,7 @@
 #define reg_clear(r, v)		reg_write(r, reg_read(r) & ~(v))
 
 #define HCI_VERSION			0x00	/* HCI Version (in BCD) */
+#define HCI_VERSION_V1			0x100	/* MIPI HCI Version number V1.0 */
 
 #define HC_CONTROL			0x04
 #define HC_CONTROL_BUS_ENABLE		BIT(31)
@@ -745,6 +746,14 @@ 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_AMD_PIO_MODE)
+		hci->RHS_regs = NULL;
+
 	/* Try activating DMA operations first */
 	if (hci->RHS_regs) {
 		reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE);
@@ -760,7 +769,11 @@ static int i3c_hci_init(struct i3c_hci *hci)
 	/* If no DMA, try PIO */
 	if (!hci->io && hci->PIO_regs) {
 		reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE);
-		if (!(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
+		/*
+		 * HC_CONTROL_PIO_MODE bit not present in HC_CONTROL register w.r.t V1.0
+		 * specification. So skip checking PIO_MODE bit status
+		 */
+		if (regval != HCI_VERSION_V1 && !(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
 			dev_err(&hci->master.dev, "DMA mode is stuck\n");
 			ret = -EIO;
 		} else {
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index f94d95e024be..046b65d43e63 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -135,6 +135,7 @@ struct i3c_hci_dev_data {
 
 /* list of quirks */
 #define HCI_QUIRK_RAW_CCC	BIT(1)	/* CCC framing must be explicit */
+#define HCI_QUIRK_AMD_PIO_MODE		BIT(2)  /* Set PIO mode for AMD platforms */
 
 
 /* global functions */
@@ -142,4 +143,6 @@ 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
-- 
2.25.1
Re: [PATCH v2 2/5] i3c: mipi-i3c-hci: Add a quirk to set PIO mode
Posted by Jarkko Nikula 1 month, 2 weeks ago 
Hi

On 7/24/24 10:12 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>
> ---
>   drivers/i3c/master/mipi-i3c-hci/Makefile |  3 ++-
>   drivers/i3c/master/mipi-i3c-hci/core.c   | 15 ++++++++++++++-
>   drivers/i3c/master/mipi-i3c-hci/hci.h    |  3 +++
>   3 files changed, 19 insertions(+), 2 deletions(-)
> 
> 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

This doesn't build since hci_quirks.c is added by the patch 4/5. One 
idea below.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index dbc8c38bd962..8bb422ab1d01 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -33,6 +33,7 @@
>   #define reg_clear(r, v)		reg_write(r, reg_read(r) & ~(v))
>   
>   #define HCI_VERSION			0x00	/* HCI Version (in BCD) */
> +#define HCI_VERSION_V1			0x100	/* MIPI HCI Version number V1.0 */
>   
>   #define HC_CONTROL			0x04
>   #define HC_CONTROL_BUS_ENABLE		BIT(31)
> @@ -745,6 +746,14 @@ 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_AMD_PIO_MODE)
> +		hci->RHS_regs = NULL;
> +
>   	/* Try activating DMA operations first */
>   	if (hci->RHS_regs) {
>   		reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE);
> @@ -760,7 +769,11 @@ static int i3c_hci_init(struct i3c_hci *hci)
>   	/* If no DMA, try PIO */
>   	if (!hci->io && hci->PIO_regs) {
>   		reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE);
> -		if (!(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
> +		/*
> +		 * HC_CONTROL_PIO_MODE bit not present in HC_CONTROL register w.r.t V1.0
> +		 * specification. So skip checking PIO_MODE bit status
> +		 */
> +		if (regval != HCI_VERSION_V1 && !(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
>   			dev_err(&hci->master.dev, "DMA mode is stuck\n");
>   			ret = -EIO;
>   		} else {

This is true, I see this now from pre-v1.0, v1.0. v1.1 and v1.2 specs 
too, HC_CONTROL_PIO_MODE bit is present only after v1.0. And therefore 
version != HCI_VERSION_V1 check is not fully correct since bit is not 
present in pre-v1.0 HW versions either.

I'd split this patch and do version check alone here (perhaps as a first 
patch) and do quirk stuff later where hci_quirks.c is added.
Re: [PATCH v2 2/5] i3c: mipi-i3c-hci: Add a quirk to set PIO mode
Posted by Shyam Sundar S K 1 month, 1 week ago 
Hi,

On 8/2/2024 19:28, Jarkko Nikula wrote:
> Hi
> 
> On 7/24/24 10:12 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>
>> ---
>>   drivers/i3c/master/mipi-i3c-hci/Makefile |  3 ++-
>>   drivers/i3c/master/mipi-i3c-hci/core.c   | 15 ++++++++++++++-
>>   drivers/i3c/master/mipi-i3c-hci/hci.h    |  3 +++
>>   3 files changed, 19 insertions(+), 2 deletions(-)
>>
>> 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
> 
> This doesn't build since hci_quirks.c is added by the patch 4/5. One
> idea below.
> 

Ah! will fix this in next revision.

>> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c
>> b/drivers/i3c/master/mipi-i3c-hci/core.c
>> index dbc8c38bd962..8bb422ab1d01 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
>> @@ -33,6 +33,7 @@
>>   #define reg_clear(r, v)        reg_write(r, reg_read(r) & ~(v))
>>     #define HCI_VERSION            0x00    /* HCI Version (in BCD) */
>> +#define HCI_VERSION_V1            0x100    /* MIPI HCI Version
>> number V1.0 */
>>     #define HC_CONTROL            0x04
>>   #define HC_CONTROL_BUS_ENABLE        BIT(31)
>> @@ -745,6 +746,14 @@ 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_AMD_PIO_MODE)
>> +        hci->RHS_regs = NULL;
>> +
>>       /* Try activating DMA operations first */
>>       if (hci->RHS_regs) {
>>           reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE);
>> @@ -760,7 +769,11 @@ static int i3c_hci_init(struct i3c_hci *hci)
>>       /* If no DMA, try PIO */
>>       if (!hci->io && hci->PIO_regs) {
>>           reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE);
>> -        if (!(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
>> +        /*
>> +         * HC_CONTROL_PIO_MODE bit not present in HC_CONTROL
>> register w.r.t V1.0
>> +         * specification. So skip checking PIO_MODE bit status
>> +         */
>> +        if (regval != HCI_VERSION_V1 && !(reg_read(HC_CONTROL) &
>> HC_CONTROL_PIO_MODE)) {
>>               dev_err(&hci->master.dev, "DMA mode is stuck\n");
>>               ret = -EIO;
>>           } else {
> 
> This is true, I see this now from pre-v1.0, v1.0. v1.1 and v1.2 specs
> too, HC_CONTROL_PIO_MODE bit is present only after v1.0. And therefore
> version != HCI_VERSION_V1 check is not fully correct since bit is not
> present in pre-v1.0 HW versions either.
> 

Agreed. HC_CONTROL_PIO_MODE is only present in v1.1 and v1.2.

anything below v1.0, the version check handling is already present;

        switch (regval & ~0xf) {
        case 0x100:     /* version 1.0 */
        case 0x110:     /* version 1.1 */
        case 0x200:     /* version 2.0 */
                break;
        default:
                dev_err(&hci->master.dev, "unsupported HCI version\n");
                return -EPROTONOSUPPORT;
        }

Let me know your thoughts.

Thanks,
Shyam

> I'd split this patch and do version check alone here (perhaps as a
> first patch) and do quirk stuff later where hci_quirks.c is added.
Re: [PATCH v2 2/5] i3c: mipi-i3c-hci: Add a quirk to set PIO mode
Posted by Jarkko Nikula 1 month, 1 week ago 
On 8/5/24 12:26 PM, Shyam Sundar S K wrote:
> Hi,
> 
> On 8/2/2024 19:28, Jarkko Nikula wrote:
>>
>> This is true, I see this now from pre-v1.0, v1.0. v1.1 and v1.2 specs
>> too, HC_CONTROL_PIO_MODE bit is present only after v1.0. And therefore
>> version != HCI_VERSION_V1 check is not fully correct since bit is not
>> present in pre-v1.0 HW versions either.
>>
> 
> Agreed. HC_CONTROL_PIO_MODE is only present in v1.1 and v1.2.
> 
> anything below v1.0, the version check handling is already present;
> 
>          switch (regval & ~0xf) {
>          case 0x100:     /* version 1.0 */
>          case 0x110:     /* version 1.1 */
>          case 0x200:     /* version 2.0 */
>                  break;
>          default:
>                  dev_err(&hci->master.dev, "unsupported HCI version\n");
>                  return -EPROTONOSUPPORT;
>          }
> 
> Let me know your thoughts.
> 
Yeah, true. I'd still use version > v1.0 check because that reflects the 
current situation. Mostly to avoid confusion when reading the code but 
also if someone adds support for pre-v1.0 HW (perhaps unlikely) and then 
doesn't need to change version check here.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.