diff mbox series

[v2,1/3] dt-bindings: mmc: add no-mmc-hs400 flag

Message ID 20210510190400.105162-1-l.stach@pengutronix.de
State Not Applicable, archived
Headers show
Series [v2,1/3] dt-bindings: mmc: add no-mmc-hs400 flag | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success
robh/dtbs-check success

Commit Message

Lucas Stach May 10, 2021, 7:03 p.m. UTC 
From: Lucas Stach <dev@lynxeye.de>

HS400 requires a data strobe line in addition to the usual MMC signal
lines. If a board design neglects to wire up this signal, HS400 mode is
not available, even if both the controller and the eMMC are claiming to
support this mode. Add a DT flag to allow boards to disable the HS400
support in this case.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Bough Chen May 11, 2021, 3 a.m. UTC | #1 
> -----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
Lucas Stach May 11, 2021, 8:18 a.m. UTC | #2 
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
SDHCI and MMC, it still feels like the cleaner and more straight
forward solution.

Regards,
Lucas
Ulf Hansson May 11, 2021, 11:14 a.m. UTC | #3 
+ Chris Ruehl

On Mon, 10 May 2021 at 21:04, Lucas Stach <l.stach@pengutronix.de> wrote:
>
> From: Lucas Stach <dev@lynxeye.de>
>
> On some boards the data strobe line isn't wired up, rendering HS400
> support broken, even if both the controller and the eMMC claim to
> support it. Allow to disable HS400 mode via DT.

Before I review the series, I just wanted to highlight that quite
recently we got a related series posted from Chris [1]. I made some
comments, but he hasn't replied yet.

In any case, if I understood it correctly, it looks like some
controllers may support HS400 ES, but not HS200. Could that be the
case here as well? Or is this a different problem?

Kind regards
Uffe

[1]
https://patchwork.kernel.org/project/linux-mmc/patch/20201208061839.21163-7-chris.ruehl@gtsys.com.hk/

>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
> v2:
> - move to core
> - actually disable all HS400 modes
> ---
>  drivers/mmc/core/host.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 9b89a91b6b47..0e066c5f5243 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -351,6 +351,9 @@ int mmc_of_parse(struct mmc_host *host)
>                 host->caps2 |= MMC_CAP2_NO_SD;
>         if (device_property_read_bool(dev, "no-mmc"))
>                 host->caps2 |= MMC_CAP2_NO_MMC;
> +       if (device_property_read_bool(dev, "no-mmc-hs400"))
> +               host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
> +                                MMC_CAP2_HS400_ES);
>
>         /* Must be after "non-removable" check */
>         if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
> --
> 2.31.1
>
Lucas Stach May 11, 2021, 11:54 a.m. UTC | #4 
Hi Ulf,

Am Dienstag, dem 11.05.2021 um 13:14 +0200 schrieb Ulf Hansson:
> + Chris Ruehl
> 
> On Mon, 10 May 2021 at 21:04, Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > From: Lucas Stach <dev@lynxeye.de>
> > 
> > On some boards the data strobe line isn't wired up, rendering HS400
> > support broken, even if both the controller and the eMMC claim to
> > support it. Allow to disable HS400 mode via DT.
> 
> Before I review the series, I just wanted to highlight that quite
> recently we got a related series posted from Chris [1]. I made some
> comments, but he hasn't replied yet.
> 
> In any case, if I understood it correctly, it looks like some
> controllers may support HS400 ES, but not HS200. Could that be the
> case here as well? Or is this a different problem?
> 
> 
That's not the issue I'm trying to solve here. HS400 modes, whether ES
nor not, require the data strobe line to work. ES mode just defines how
this line is used. I know for a fact that the board I'm dealing with
here, just hasn't wired up this line between the SoC and the eMMC. Thus
HS400 modes fail to work, even though both controller and eMMC support
this mode.

When HS400 is disabled, like in this series, communication falls back
to HS200 mode and works fine this way.

Regards,
Lucas

> Kind regards
> Uffe
> 
> [1]
> https://patchwork.kernel.org/project/linux-mmc/patch/20201208061839.21163-7-chris.ruehl@gtsys.com.hk/
> 
> > 
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> > v2:
> > - move to core
> > - actually disable all HS400 modes
> > ---
> >  drivers/mmc/core/host.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > index 9b89a91b6b47..0e066c5f5243 100644
> > --- a/drivers/mmc/core/host.c
> > +++ b/drivers/mmc/core/host.c
> > @@ -351,6 +351,9 @@ int mmc_of_parse(struct mmc_host *host)
> >                 host->caps2 |= MMC_CAP2_NO_SD;
> >         if (device_property_read_bool(dev, "no-mmc"))
> >                 host->caps2 |= MMC_CAP2_NO_MMC;
> > +       if (device_property_read_bool(dev, "no-mmc-hs400"))
> > +               host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
> > +                                MMC_CAP2_HS400_ES);
> > 
> >         /* Must be after "non-removable" check */
> >         if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
> > --
> > 2.31.1
> >
Ulf Hansson May 11, 2021, 12:19 p.m. UTC | #5 
On Tue, 11 May 2021 at 13:54, Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Ulf,
>
> Am Dienstag, dem 11.05.2021 um 13:14 +0200 schrieb Ulf Hansson:
> > + Chris Ruehl
> >
> > On Mon, 10 May 2021 at 21:04, Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > From: Lucas Stach <dev@lynxeye.de>
> > >
> > > On some boards the data strobe line isn't wired up, rendering HS400
> > > support broken, even if both the controller and the eMMC claim to
> > > support it. Allow to disable HS400 mode via DT.
> >
> > Before I review the series, I just wanted to highlight that quite
> > recently we got a related series posted from Chris [1]. I made some
> > comments, but he hasn't replied yet.
> >
> > In any case, if I understood it correctly, it looks like some
> > controllers may support HS400 ES, but not HS200. Could that be the
> > case here as well? Or is this a different problem?
> >
> >
> That's not the issue I'm trying to solve here. HS400 modes, whether ES
> nor not, require the data strobe line to work. ES mode just defines how
> this line is used. I know for a fact that the board I'm dealing with
> here, just hasn't wired up this line between the SoC and the eMMC. Thus
> HS400 modes fail to work, even though both controller and eMMC support
> this mode.
>
> When HS400 is disabled, like in this series, communication falls back
> to HS200 mode and works fine this way.

Alright, thanks for clarifying. I will look into the series soon.

Kind regards
Uffe

>
> Regards,
> Lucas
>
> > Kind regards
> > Uffe
> >
> > [1]
> > https://patchwork.kernel.org/project/linux-mmc/patch/20201208061839.21163-7-chris.ruehl@gtsys.com.hk/
> >
> > >
> > > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > > ---
> > > v2:
> > > - move to core
> > > - actually disable all HS400 modes
> > > ---
> > >  drivers/mmc/core/host.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > > index 9b89a91b6b47..0e066c5f5243 100644
> > > --- a/drivers/mmc/core/host.c
> > > +++ b/drivers/mmc/core/host.c
> > > @@ -351,6 +351,9 @@ int mmc_of_parse(struct mmc_host *host)
> > >                 host->caps2 |= MMC_CAP2_NO_SD;
> > >         if (device_property_read_bool(dev, "no-mmc"))
> > >                 host->caps2 |= MMC_CAP2_NO_MMC;
> > > +       if (device_property_read_bool(dev, "no-mmc-hs400"))
> > > +               host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
> > > +                                MMC_CAP2_HS400_ES);
> > >
> > >         /* Must be after "non-removable" check */
> > >         if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
> > > --
> > > 2.31.1
> > >
>
>
Adrian Hunter May 11, 2021, 12:40 p.m. UTC | #6 
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
>
Bough Chen May 12, 2021, 2:23 a.m. UTC | #7 
> -----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
> >
Ulf Hansson May 24, 2021, 2:10 p.m. UTC | #8 
On Mon, 10 May 2021 at 21:04, Lucas Stach <l.stach@pengutronix.de> wrote:
>
> From: Lucas Stach <dev@lynxeye.de>
>
> HS400 requires a data strobe line in addition to the usual MMC signal
> lines. If a board design neglects to wire up this signal, HS400 mode is
> not available, even if both the controller and the eMMC are claiming to
> support this mode. Add a DT flag to allow boards to disable the HS400
> support in this case.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> Acked-by: Rob Herring <robh@kernel.org>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> index e141330c1114..ac80d09df3a9 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> @@ -220,6 +220,11 @@ properties:
>      description:
>        eMMC HS400 enhanced strobe mode is supported
>
> +  no-mmc-hs400:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      All eMMC HS400 modes are not supported.
> +
>    dsr:
>      description:
>        Value the card Driver Stage Register (DSR) should be programmed
> --
> 2.31.1
>
Ulf Hansson May 24, 2021, 2:10 p.m. UTC | #9 
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
>
Ulf Hansson May 24, 2021, 2:10 p.m. UTC | #10 
On Mon, 10 May 2021 at 21:04, Lucas Stach <l.stach@pengutronix.de> wrote:
>
> From: Lucas Stach <dev@lynxeye.de>
>
> On some boards the data strobe line isn't wired up, rendering HS400
> support broken, even if both the controller and the eMMC claim to
> support it. Allow to disable HS400 mode via DT.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>

Applied for next, thanks!

Kind regards
Uffe


> ---
> v2:
> - move to core
> - actually disable all HS400 modes
> ---
>  drivers/mmc/core/host.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 9b89a91b6b47..0e066c5f5243 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -351,6 +351,9 @@ int mmc_of_parse(struct mmc_host *host)
>                 host->caps2 |= MMC_CAP2_NO_SD;
>         if (device_property_read_bool(dev, "no-mmc"))
>                 host->caps2 |= MMC_CAP2_NO_MMC;
> +       if (device_property_read_bool(dev, "no-mmc-hs400"))
> +               host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
> +                                MMC_CAP2_HS400_ES);
>
>         /* Must be after "non-removable" check */
>         if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
> --
> 2.31.1
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
index e141330c1114..ac80d09df3a9 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
@@ -220,6 +220,11 @@  properties:
     description:
       eMMC HS400 enhanced strobe mode is supported
 
+  no-mmc-hs400:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      All eMMC HS400 modes are not supported.
+
   dsr:
     description:
       Value the card Driver Stage Register (DSR) should be programmed