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

[Bough Chen <haibo.chen@nxp.com>]

[-- Attachment #1: Type: text/plain, Size: 4019 bytes --]

> -----Original Message-----
> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> Sent: 2021年5月11日 20:41
> To: Lucas Stach <l.stach@pengutronix.de>; Bough Chen
> <haibo.chen@nxp.com>; Ulf Hansson <ulf.hansson@linaro.org>
> 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: Re: [PATCH v2 2/3] mmc: sdhci-esdhc-imx: advertise HS400 mode
> through MMC caps
> 
> 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

Okay, I see.
For this patch:
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

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


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9571 bytes --]