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

[Philippe Mathieu-Daudé]

On 6/3/20 8:58 AM, Guenter Roeck wrote:
> 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.

Yes, thanks for the tip!

"10.3.8.28 Vendor Specific Register (uSDHCx_VEND_SPEC)"

> 
>> Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

^ this can be changed by:

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