Re: [PATCH v2 2/3] mmc: sdhci-esdhc-imx: advertise HS400 mode through MMC caps

[Adrian Hunter <adrian.hunter@intel.com>]

On 11/05/21 11:18 am, Lucas Stach wrote:
> Am Dienstag, dem 11.05.2021 um 03:00 +0000 schrieb Bough Chen:
>>> -----Original Message-----
>>> From: Lucas Stach [mailto:l.stach@pengutronix.de]
>>> Sent: 2021年5月11日 3:04
>>> To: Ulf Hansson <ulf.hansson@linaro.org>; Adrian Hunter
>>> <adrian.hunter@intel.com>; Bough Chen <haibo.chen@nxp.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>; dl-linux-imx <linux-imx@nxp.com>;
>>> kernel@pengutronix.de; linux-mmc@vger.kernel.org;
>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>> Subject: [PATCH v2 2/3] mmc: sdhci-esdhc-imx: advertise HS400 mode through
>>> MMC caps
>>>
>>> Instead of having an indirection through the SDHCI layer and emulating a
>>> capability bit, that isn't there in hardware, do the same same thing as
>> with
>>> HS400_ES and advertise the support for HS400 directly through the MMC
>> caps.
>>>
>>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>>> ---
>>>  drivers/mmc/host/sdhci-esdhc-imx.c | 5 +----
>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> index a20459744d21..65a52586db36 100644
>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> @@ -427,9 +427,6 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int
>>> reg)
>>>  					|
>> FIELD_PREP(SDHCI_RETUNING_MODE_MASK,
>>>  						     SDHCI_TUNING_MODE_3);
>>>
>>> -			if (imx_data->socdata->flags & ESDHC_FLAG_HS400)
>>> -				val |= SDHCI_SUPPORT_HS400;
>>> -
>>>  			/*
>>>  			 * Do not advertise faster UHS modes if there are no
>>>  			 * pinctrl states for 100MHz/200MHz.
>>> @@ -1603,7 +1600,7 @@ static int sdhci_esdhc_imx_probe(struct
>>> platform_device *pdev)
>>>  		host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
>>>
>>>  	if (imx_data->socdata->flags & ESDHC_FLAG_HS400)
>>> -		host->quirks2 |= SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400;
>>> +		host->mmc->caps2 |= MMC_CAP2_HS400;
>>
>> Hi Lucas,
>>
>> I think patch1 and patch 2 are enough to cover your requirement.
>> For this patch, I think it's unnecessary, sdhci-esdhc-imx.c need to reuse
>> sdhci.c as much as possible.
>> In sdhci.c, already contain the following logic. 
>>
>>          if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 &&
>>              (host->caps1 & SDHCI_SUPPORT_HS400))
>>                  mmc->caps2 |= MMC_CAP2_HS400;
>>
>> The reason why we directly use host->mmc->caps2 for HS400ES mode is that
>> sdhci.c do not contain the similar logic.
> 
> No, it's not enough. We call mmc_of_parse(), which clears the HS400
> flags, before sdhci_setup_host() is called, which will then add the
> HS400 flags again. So either I still need to evaluate the DT property
> in the esdhc driver to make it return the right emulated SDHCI caps bit
> for the HS400 case, or do it like in this patch.
> 
> While the way it is done here is a bit of a layering violation between

We see SDHCI as more of a library, not a layer, so this is OK

> SDHCI and MMC, it still feels like the cleaner and more straight
> forward solution.
> 
> Regards,
> Lucas
>