Re: [PATCH 1/2] sd: sdhci: Implement basic vendor specific register support

[Guenter Roeck]

On 6/2/20 11:37 PM, Philippe Mathieu-Daudé wrote:
> Hi Guenter,
> 
> On 6/3/20 7:24 AM, Guenter Roeck wrote:
>> The Linux kernel's IMX code now uses vendor specific commands.
>> This results in endless warnings when booting the Linux kernel.
>>
>> sdhci-esdhc-imx 2194000.usdhc: esdhc_wait_for_card_clock_gate_off:
>>      card clock still not gate off in 100us!.
>>
>> Implement support for the vendor specific command implemented in IMX hardware
>> to be able to avoid this warning.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  hw/sd/sdhci-internal.h |  5 +++++
>>  hw/sd/sdhci.c          | 18 +++++++++++++++++-
>>  include/hw/sd/sdhci.h  |  5 +++++
>>  3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>> index e7c8a523b5..e8c753d6d1 100644
>> --- a/hw/sd/sdhci-internal.h
>> +++ b/hw/sd/sdhci-internal.h
>> @@ -75,6 +75,7 @@
>>  #define SDHC_CMD_INHIBIT               0x00000001
>>  #define SDHC_DATA_INHIBIT              0x00000002
>>  #define SDHC_DAT_LINE_ACTIVE           0x00000004
>> +#define SDHC_IMX_CLOCK_GATE_OFF        0x00000080
>>  #define SDHC_DOING_WRITE               0x00000100
>>  #define SDHC_DOING_READ                0x00000200
>>  #define SDHC_SPACE_AVAILABLE           0x00000400
>> @@ -289,7 +290,10 @@ extern const VMStateDescription sdhci_vmstate;
>>  
>>  
>>  #define ESDHC_MIX_CTRL                  0x48
>> +
>>  #define ESDHC_VENDOR_SPEC               0xc0
>> +#define ESDHC_IMX_FRC_SDCLK_ON          (1 << 8)
> 
> I searched for the datasheet but couldn't find any, so I suppose it is
> only available under NDA. I can not review much (in particular I wanted
> to check the register sizes), anyway the overall looks OK:
> 

Actually, I only had to register an account to be able to download
the datasheets from NXP. Register width is 32 bit.

> Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Also:
> 
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
Thanks!

Guenter

>> +
>>  #define ESDHC_DLL_CTRL                  0x60
>>  
>>  #define ESDHC_TUNING_CTRL               0xcc
>> @@ -326,6 +330,7 @@ extern const VMStateDescription sdhci_vmstate;
>>  #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>      DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>>      DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>> +    DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
>>      \
>>      /* Capabilities registers provide information on supported
>>       * features of this specific host controller implementation */ \
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 1b75d7bab9..eb2be6529e 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1569,11 +1569,13 @@ static uint64_t usdhc_read(void *opaque, hwaddr 
>> offset, unsigned size)
>>          }
>>          break;
>>  
>> +    case ESDHC_VENDOR_SPEC:
>> +        ret = s->vendor_spec;
>> +        break;
>>      case ESDHC_DLL_CTRL:
>>      case ESDHC_TUNE_CTRL_STATUS:
>>      case ESDHC_UNDOCUMENTED_REG27:
>>      case ESDHC_TUNING_CTRL:
>> -    case ESDHC_VENDOR_SPEC:
>>      case ESDHC_MIX_CTRL:
>>      case ESDHC_WTMK_LVL:
>>          ret = 0;
>> @@ -1596,7 +1598,21 @@ usdhc_write(void *opaque, hwaddr offset, uint64_t 
>> val, unsigned size)
>>      case ESDHC_UNDOCUMENTED_REG27:
>>      case ESDHC_TUNING_CTRL:
>>      case ESDHC_WTMK_LVL:
>> +        break;
>> +
>>      case ESDHC_VENDOR_SPEC:
>> +        s->vendor_spec = value;
>> +        switch (s->vendor) {
>> +        case SDHCI_VENDOR_IMX:
>> +            if (value & ESDHC_IMX_FRC_SDCLK_ON) {
>> +                s->prnsts &= ~SDHC_IMX_CLOCK_GATE_OFF;
>> +            } else {
>> +                s->prnsts |= SDHC_IMX_CLOCK_GATE_OFF;
>> +            }
>> +            break;
>> +        default:
>> +            break;
>> +        }
>>          break;
>>  
>>      case SDHC_HOSTCTL:
>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>> index c6868c9699..5d9275f3d6 100644
>> --- a/include/hw/sd/sdhci.h
>> +++ b/include/hw/sd/sdhci.h
>> @@ -74,6 +74,7 @@ typedef struct SDHCIState {
>>      uint16_t acmd12errsts; /* Auto CMD12 error status register */
>>      uint16_t hostctl2;     /* Host Control 2 */
>>      uint64_t admasysaddr;  /* ADMA System Address Register */
>> +    uint16_t vendor_spec;  /* Vendor specific register */
>>  
>>      /* Read-only registers */
>>      uint64_t capareg;      /* Capabilities Register */
>> @@ -96,8 +97,12 @@ typedef struct SDHCIState {
>>      uint32_t quirks;
>>      uint8_t sd_spec_version;
>>      uint8_t uhs_mode;
>> +    uint8_t vendor;        /* For vendor specific functionality */
>>  } SDHCIState;
>>  
>> +#define SDHCI_VENDOR_NONE       0
>> +#define SDHCI_VENDOR_IMX        1
>> +
>>  /*
>>   * Controller does not provide transfer-complete interrupt when not
>>   * busy.
>>
>