| Message ID | 20210510190400.105162-2-l.stach@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | 2991ad76d2537a4ebe7132d087cdbc76377da302 |
| Headers | show |
| Series | [v2,1/3] dt-bindings: mmc: add no-mmc-hs400 flag | expand |
> -----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. Adrian, what's your comment? Best Regards Haibo > > if (imx_data->socdata->flags & ESDHC_FLAG_BROKEN_AUTO_CMD23) > host->quirks2 |= SDHCI_QUIRK2_ACMD23_BROKEN; > -- > 2.31.1
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 >
> -----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 > >
On Mon, 10 May 2021 at 21:04, Lucas Stach <l.stach@pengutronix.de> wrote: > > 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> Applied for next, thanks! Kind regards Uffe > --- > 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; > > if (imx_data->socdata->flags & ESDHC_FLAG_BROKEN_AUTO_CMD23) > host->quirks2 |= SDHCI_QUIRK2_ACMD23_BROKEN; > -- > 2.31.1 >
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; if (imx_data->socdata->flags & ESDHC_FLAG_BROKEN_AUTO_CMD23) host->quirks2 |= SDHCI_QUIRK2_ACMD23_BROKEN;
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(-)