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

[Philippe Mathieu-Daudé]

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:

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

Also:

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

> +
>  #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.
>