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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8ec23c64-0b55-4a71-8199-14cb6061ad45@amd.com>
Date: Mon, 5 Aug 2024 14:56:01 +0530
From: Shyam Sundar S K <Shyam-sundar.S-k@....com>
To: Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: Guruvendra Punugupati <Guruvendra.Punugupati@....com>,
 Krishnamoorthi M <krishnamoorthi.m@....com>, linux-i3c@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/5] i3c: mipi-i3c-hci: Add a quirk to set PIO mode

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@....com>
>> Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@....com>
>> Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@....com>
>> Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@....com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@....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.