[PATCH 0/3] mmc: sdhci-esdhc-imx: fix no UHS modes

[PATCH 0/3] mmc: sdhci-esdhc-imx: fix no UHS modes

[Stefan Agner]

Currently sdhci-esdhc-imx.c sets SDHCI_QUIRK2_NO_1_8_V if no
100MHz/200MHz pinctrl configurations are present in order to
prevent the stack from choosing high speed modes.

This does work for SD cards quite well, since 1.8V matches all
higher speed modes, and all lower speed modes run with 3.3V.

However, when running a eMMC on 1.8V only, this causes issues:
If vqmmc-supply is set to 1.8V only, and the driver at the same
time sets SDHCI_QUIRK2_NO_1_8_V, the stack has troubls selecting
a valid mode and continuously prints:
  mmc1: Switching to 3.3V signalling voltage failed

There are already board device trees which work around this by
not setting vqmmc-supply, e.g.
arch/arm/boot/dts/imx6qdl-sr-som-ti.dtsi.

Introducing a new quirk was the only way which I came up with,
but maybe there is a better way to prevent higher speed modes
while allowing 1.8V eMMC?

Stefan Agner (3):
  mmc: sdhci-esdhc-imx: get rid of support_vsel
  mmc: sdhci: add quirk to prevent higher speed modes
  mmc: sdhci-esdhc-imx: prevent stack from using higher speed modes

 drivers/mmc/host/sdhci-esdhc-imx.c          | 12 ++++--------
 drivers/mmc/host/sdhci.c                    |  8 ++++++++
 drivers/mmc/host/sdhci.h                    |  2 ++
 include/linux/platform_data/mmc-esdhc-imx.h |  2 --
 4 files changed, 14 insertions(+), 10 deletions(-)

-- 
2.18.0

[PATCH 1/3] mmc: sdhci-esdhc-imx: get rid of support_vsel

[Stefan Agner]

The field support_vsel is currently only used in the device tree
case. Get rid of it. No change in behavior.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mmc/host/sdhci-esdhc-imx.c          | 8 ++------
 include/linux/platform_data/mmc-esdhc-imx.h | 2 --
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 6f444731754d..20a420b765b3 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1145,18 +1145,14 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 			     &boarddata->tuning_start_tap);
 
 	if (of_find_property(np, "no-1-8-v", NULL))
-		boarddata->support_vsel = false;
-	else
-		boarddata->support_vsel = true;
+		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
 
 	if (of_property_read_u32(np, "fsl,delay-line", &boarddata->delay_line))
 		boarddata->delay_line = 0;
 
 	mmc_of_parse_voltage(np, &host->ocr_mask);
 
-	/* sdr50 and sdr104 need work on 1.8v signal voltage */
-	if ((boarddata->support_vsel) && esdhc_is_usdhc(imx_data) &&
-	    !IS_ERR(imx_data->pins_default)) {
+	if (esdhc_is_usdhc(imx_data) && !IS_ERR(imx_data->pins_default)) {
 		imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
 						ESDHC_PINCTRL_STATE_100MHZ);
 		imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
index 7daa78a2f342..640dec8b5b0c 100644
--- a/include/linux/platform_data/mmc-esdhc-imx.h
+++ b/include/linux/platform_data/mmc-esdhc-imx.h
@@ -34,7 +34,6 @@ enum cd_types {
  * @cd_gpio:	gpio for card_detect interrupt
  * @wp_type:	type of write_protect method (see wp_types enum above)
  * @cd_type:	type of card_detect method (see cd_types enum above)
- * @support_vsel:  indicate it supports 1.8v switching
  */
 
 struct esdhc_platform_data {
@@ -43,7 +42,6 @@ struct esdhc_platform_data {
 	enum wp_types wp_type;
 	enum cd_types cd_type;
 	int max_bus_width;
-	bool support_vsel;
 	unsigned int delay_line;
 	unsigned int tuning_step;       /* The delay cell steps in tuning procedure */
 	unsigned int tuning_start_tap;	/* The start delay cell point in tuning procedure */
-- 
2.18.0

[PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

[Stefan Agner]

Some hosts are capable of running higher speed modes but do not
have the board support for it. Introduce a quirk which prevents
the stack from using modes running at 100MHz or faster.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mmc/host/sdhci.c | 8 ++++++++
 drivers/mmc/host/sdhci.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1c828e0e9905..8ac257dfaab3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3749,6 +3749,14 @@ int sdhci_setup_host(struct sdhci_host *host)
 		}
 	}
 
+	if (host->quirks2 & SDHCI_QUIRK2_NO_UHS_HS200_HS400) {
+		host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
+				 SDHCI_SUPPORT_DDR50);
+
+		mmc->caps2 &= ~(MMC_CAP2_HSX00_1_8V | MMC_CAP2_HSX00_1_2V |
+				MMC_CAP2_HS400_ES);
+	}
+
 	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
 		host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
 				 SDHCI_SUPPORT_DDR50);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 23966f887da6..cb2433d6d61f 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -450,6 +450,8 @@ struct sdhci_host {
  * obtainable timeout.
  */
 #define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT			(1<<17)
+/* Do not support any higher speeds (>50MHz) */
+#define SDHCI_QUIRK2_NO_UHS_HS200_HS400			(1<<18)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.18.0

[PATCH 3/3] mmc: sdhci-esdhc-imx: prevent stack from using higher speed modes

[Stefan Agner]

If pinctrl configurations for higher speed modes are missing, the
stack currently uses the no 1.8V quirk. This comes close to what
we need but not exactly: E.g. if a eMMC chip uses 1.8V signaling
(by specifying a 1.8V only vqmmc-supply) while not providing any
100MHz/200MHz pinctrl configurations then the SDHCI_QUIRK2_NO_1_8_V
leads the stack to print signaling voltage switch failed errors
continuously:
  mmc1: Switching to 3.3V signalling voltage failed

Presumably because the stack tries to use 3.3V signaling:

  # cat /sys/kernel/debug/mmc1/ios
  ...
  timing spec:    8 (mmc DDR52)
  signal voltage: 0 (3.30 V)
  ...

With using SDHCI_QUIRK2_NO_UHS_HS200_HS400 we prevent the stack
from choosing any modes require speeds higher than 52MHz while
still allowing to select modes using 1.8V at lower speeds (e.g.
DDR52):

  # cat /sys/kernel/debug/mmc1/ios
  ...
  timing spec:    8 (mmc DDR52)
  signal voltage: 1 (1.80 V)
  ...

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 20a420b765b3..4a1c33018072 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1165,10 +1165,10 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 			 * fall back to not supporting uhs by specifying no
 			 * 1.8v quirk
 			 */
-			host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
+			host->quirks2 |= SDHCI_QUIRK2_NO_UHS_HS200_HS400;
 		}
 	} else {
-		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
+		host->quirks2 |= SDHCI_QUIRK2_NO_UHS_HS200_HS400;
 	}
 
 	/* call to generic mmc_of_parse to support additional capabilities */
-- 
2.18.0

Re: [PATCH 1/3] mmc: sdhci-esdhc-imx: get rid of support_vsel

[Ulf Hansson]

On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote:
> The field support_vsel is currently only used in the device tree
> case. Get rid of it. No change in behavior.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Thanks, applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c          | 8 ++------
>  include/linux/platform_data/mmc-esdhc-imx.h | 2 --
>  2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 6f444731754d..20a420b765b3 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1145,18 +1145,14 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>                              &boarddata->tuning_start_tap);
>
>         if (of_find_property(np, "no-1-8-v", NULL))
> -               boarddata->support_vsel = false;
> -       else
> -               boarddata->support_vsel = true;
> +               host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>
>         if (of_property_read_u32(np, "fsl,delay-line", &boarddata->delay_line))
>                 boarddata->delay_line = 0;
>
>         mmc_of_parse_voltage(np, &host->ocr_mask);
>
> -       /* sdr50 and sdr104 need work on 1.8v signal voltage */
> -       if ((boarddata->support_vsel) && esdhc_is_usdhc(imx_data) &&
> -           !IS_ERR(imx_data->pins_default)) {
> +       if (esdhc_is_usdhc(imx_data) && !IS_ERR(imx_data->pins_default)) {
>                 imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
>                                                 ESDHC_PINCTRL_STATE_100MHZ);
>                 imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
> index 7daa78a2f342..640dec8b5b0c 100644
> --- a/include/linux/platform_data/mmc-esdhc-imx.h
> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
> @@ -34,7 +34,6 @@ enum cd_types {
>   * @cd_gpio:   gpio for card_detect interrupt
>   * @wp_type:   type of write_protect method (see wp_types enum above)
>   * @cd_type:   type of card_detect method (see cd_types enum above)
> - * @support_vsel:  indicate it supports 1.8v switching
>   */
>
>  struct esdhc_platform_data {
> @@ -43,7 +42,6 @@ struct esdhc_platform_data {
>         enum wp_types wp_type;
>         enum cd_types cd_type;
>         int max_bus_width;
> -       bool support_vsel;
>         unsigned int delay_line;
>         unsigned int tuning_step;       /* The delay cell steps in tuning procedure */
>         unsigned int tuning_start_tap;  /* The start delay cell point in tuning procedure */
> --
> 2.18.0
>

Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

[Ulf Hansson]

On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote:
> Some hosts are capable of running higher speed modes but do not
> have the board support for it. Introduce a quirk which prevents
> the stack from using modes running at 100MHz or faster.

To cap the freq, use the DT property "max-frequency". To enable
certain speed modes, use the corresponding speed mode binding. For
example "sd-uhs-sdr*" and "mmc-hs200*".
Documented in Documentation/devicetree/bindings/mmc/mmc.txt

In case the sdhci cap register, doesn't reflect the board support
properly, such that you may want to disable some speed modes, then you
may benefit from using the DT properties "sdhci-caps*.
Documented in Documentation/devicetree/bindings/mmc/sdhci.txt

Kind regards
Uffe

>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mmc/host/sdhci.c | 8 ++++++++
>  drivers/mmc/host/sdhci.h | 2 ++
>  2 files changed, 10 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1c828e0e9905..8ac257dfaab3 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3749,6 +3749,14 @@ int sdhci_setup_host(struct sdhci_host *host)
>                 }
>         }
>
> +       if (host->quirks2 & SDHCI_QUIRK2_NO_UHS_HS200_HS400) {
> +               host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> +                                SDHCI_SUPPORT_DDR50);
> +
> +               mmc->caps2 &= ~(MMC_CAP2_HSX00_1_8V | MMC_CAP2_HSX00_1_2V |
> +                               MMC_CAP2_HS400_ES);
> +       }
> +
>         if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
>                 host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
>                                  SDHCI_SUPPORT_DDR50);
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 23966f887da6..cb2433d6d61f 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -450,6 +450,8 @@ struct sdhci_host {
>   * obtainable timeout.
>   */
>  #define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT                        (1<<17)
> +/* Do not support any higher speeds (>50MHz) */
> +#define SDHCI_QUIRK2_NO_UHS_HS200_HS400                        (1<<18)
>
>         int irq;                /* Device IRQ */
>         void __iomem *ioaddr;   /* Mapped address */
> --
> 2.18.0
>

Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

[Stefan Agner]

On 02.07.2018 16:36, Ulf Hansson wrote:
> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote:
>> Some hosts are capable of running higher speed modes but do not
>> have the board support for it. Introduce a quirk which prevents
>> the stack from using modes running at 100MHz or faster.
> 
> To cap the freq, use the DT property "max-frequency". To enable
> certain speed modes, use the corresponding speed mode binding. For
> example "sd-uhs-sdr*" and "mmc-hs200*".
> Documented in Documentation/devicetree/bindings/mmc/mmc.txt

I had bad experience with max-frequency: Some higher speed modes seem
not to work reliably if constraint to low frequencies. E.g. we had lots
of devices fail in practise with HS400@100MHz... So it is doing what it
should, but it just seems that higher speed modes do not necessarily run
well with lower frequencies...

So I'd rather prefer to limit speed modes as it is done right now.

> 
> In case the sdhci cap register, doesn't reflect the board support
> properly, such that you may want to disable some speed modes, then you
> may benefit from using the DT properties "sdhci-caps*.
> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt

Hm, yeah I guess something like

sdhci-caps-mask =  /bits/ 64 <((SDHCI_SUPPORT_SDR104 |
SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)>

would come close.

But it does not restrict MMC modes such as HS200/HS400. There seem to be
no mmc-caps...


My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not
restrict modes correctly. Currently the driver checks whether >=100MHz
pinctrl settings are available, and if not uses the quirk to restrict
higher speed modes. Removing that would break device tree backward
compatibility... 

We probably could do something like this:
if (!100mhzpinctrl) {
     if (!sdhci-caps) {
          /*
           * If no 100MHz/200MHz pinctrl are available, SDHC caps should
be used to restrict 
           * modes. Falling back to old behavior...
           */
         pr_warn(...)
         host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
     }
}


--
Stefan

> 
> Kind regards
> Uffe
> 
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/mmc/host/sdhci.c | 8 ++++++++
>>  drivers/mmc/host/sdhci.h | 2 ++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 1c828e0e9905..8ac257dfaab3 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -3749,6 +3749,14 @@ int sdhci_setup_host(struct sdhci_host *host)
>>                 }
>>         }
>>
>> +       if (host->quirks2 & SDHCI_QUIRK2_NO_UHS_HS200_HS400) {
>> +               host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
>> +                                SDHCI_SUPPORT_DDR50);
>> +
>> +               mmc->caps2 &= ~(MMC_CAP2_HSX00_1_8V | MMC_CAP2_HSX00_1_2V |
>> +                               MMC_CAP2_HS400_ES);
>> +       }
>> +
>>         if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
>>                 host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
>>                                  SDHCI_SUPPORT_DDR50);
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 23966f887da6..cb2433d6d61f 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -450,6 +450,8 @@ struct sdhci_host {
>>   * obtainable timeout.
>>   */
>>  #define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT                        (1<<17)
>> +/* Do not support any higher speeds (>50MHz) */
>> +#define SDHCI_QUIRK2_NO_UHS_HS200_HS400                        (1<<18)
>>
>>         int irq;                /* Device IRQ */
>>         void __iomem *ioaddr;   /* Mapped address */
>> --
>> 2.18.0
>>

Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

[Ulf Hansson]

On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote:
> On 02.07.2018 16:36, Ulf Hansson wrote:
>> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote:
>>> Some hosts are capable of running higher speed modes but do not
>>> have the board support for it. Introduce a quirk which prevents
>>> the stack from using modes running at 100MHz or faster.
>>
>> To cap the freq, use the DT property "max-frequency". To enable
>> certain speed modes, use the corresponding speed mode binding. For
>> example "sd-uhs-sdr*" and "mmc-hs200*".
>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt
>
> I had bad experience with max-frequency: Some higher speed modes seem
> not to work reliably if constraint to low frequencies. E.g. we had lots
> of devices fail in practise with HS400@100MHz... So it is doing what it
> should, but it just seems that higher speed modes do not necessarily run
> well with lower frequencies...
>
> So I'd rather prefer to limit speed modes as it is done right now.
>
>>
>> In case the sdhci cap register, doesn't reflect the board support
>> properly, such that you may want to disable some speed modes, then you
>> may benefit from using the DT properties "sdhci-caps*.
>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt
>
> Hm, yeah I guess something like
>
> sdhci-caps-mask =  /bits/ 64 <((SDHCI_SUPPORT_SDR104 |
> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)>
>
> would come close.
>
> But it does not restrict MMC modes such as HS200/HS400. There seem to be
> no mmc-caps...

Right.

The solution to fix this, should be to *not* set those DT properties,
like "mmc-hs*" for example. That should work, no?

>
>
> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not
> restrict modes correctly. Currently the driver checks whether >=100MHz
> pinctrl settings are available, and if not uses the quirk to restrict
> higher speed modes. Removing that would break device tree backward
> compatibility...

Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather
how the pinctrl setting becomes interpreted when setting the quirk.

>
> We probably could do something like this:
> if (!100mhzpinctrl) {
>      if (!sdhci-caps) {
>           /*
>            * If no 100MHz/200MHz pinctrl are available, SDHC caps should
> be used to restrict
>            * modes. Falling back to old behavior...
>            */
>          pr_warn(...)
>          host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>      }
> }
>

I am not sure what makes best sense here. Let me have a look at patch 3 as well.

[...]

Kind regards
Uffe

Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

[Stefan Agner]

On 04.07.2018 12:07, Ulf Hansson wrote:
> On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote:
>> On 02.07.2018 16:36, Ulf Hansson wrote:
>>> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote:
>>>> Some hosts are capable of running higher speed modes but do not
>>>> have the board support for it. Introduce a quirk which prevents
>>>> the stack from using modes running at 100MHz or faster.
>>>
>>> To cap the freq, use the DT property "max-frequency". To enable
>>> certain speed modes, use the corresponding speed mode binding. For
>>> example "sd-uhs-sdr*" and "mmc-hs200*".
>>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt
>>
>> I had bad experience with max-frequency: Some higher speed modes seem
>> not to work reliably if constraint to low frequencies. E.g. we had lots
>> of devices fail in practise with HS400@100MHz... So it is doing what it
>> should, but it just seems that higher speed modes do not necessarily run
>> well with lower frequencies...
>>
>> So I'd rather prefer to limit speed modes as it is done right now.
>>
>>>
>>> In case the sdhci cap register, doesn't reflect the board support
>>> properly, such that you may want to disable some speed modes, then you
>>> may benefit from using the DT properties "sdhci-caps*.
>>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt
>>
>> Hm, yeah I guess something like
>>
>> sdhci-caps-mask =  /bits/ 64 <((SDHCI_SUPPORT_SDR104 |
>> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)>
>>
>> would come close.
>>
>> But it does not restrict MMC modes such as HS200/HS400. There seem to be
>> no mmc-caps...
> 
> Right.
> 
> The solution to fix this, should be to *not* set those DT properties,
> like "mmc-hs*" for example. That should work, no?
> 

The controller does not make use of the dt modes so far, so I can't not
set those properties...

>>
>>
>> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not
>> restrict modes correctly. Currently the driver checks whether >=100MHz
>> pinctrl settings are available, and if not uses the quirk to restrict
>> higher speed modes. Removing that would break device tree backward
>> compatibility...
> 
> Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather
> how the pinctrl setting becomes interpreted when setting the quirk.
> 

Yes, sorry for the confusion. SDHCI_QUIRK2_NO_1_8_V is fine, it is just
not the quirk this driver needs.

I argue that commit ad93220de7da ("mmc: sdhci-esdhc-imx: change pinctrl
state according to uhs mode") chose the wrong quirk from the
beginning...

Afaict, the quirk needed here does not exist.

--
Stefan

>>
>> We probably could do something like this:
>> if (!100mhzpinctrl) {
>>      if (!sdhci-caps) {
>>           /*
>>            * If no 100MHz/200MHz pinctrl are available, SDHC caps should
>> be used to restrict
>>            * modes. Falling back to old behavior...
>>            */
>>          pr_warn(...)
>>          host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>      }
>> }
>>
> 
> I am not sure what makes best sense here. Let me have a look at patch 3 as well.
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

[Ulf Hansson]

On 4 July 2018 at 12:55, Stefan Agner <stefan@agner.ch> wrote:
> On 04.07.2018 12:07, Ulf Hansson wrote:
>> On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote:
>>> On 02.07.2018 16:36, Ulf Hansson wrote:
>>>> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote:
>>>>> Some hosts are capable of running higher speed modes but do not
>>>>> have the board support for it. Introduce a quirk which prevents
>>>>> the stack from using modes running at 100MHz or faster.
>>>>
>>>> To cap the freq, use the DT property "max-frequency". To enable
>>>> certain speed modes, use the corresponding speed mode binding. For
>>>> example "sd-uhs-sdr*" and "mmc-hs200*".
>>>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt
>>>
>>> I had bad experience with max-frequency: Some higher speed modes seem
>>> not to work reliably if constraint to low frequencies. E.g. we had lots
>>> of devices fail in practise with HS400@100MHz... So it is doing what it
>>> should, but it just seems that higher speed modes do not necessarily run
>>> well with lower frequencies...
>>>
>>> So I'd rather prefer to limit speed modes as it is done right now.
>>>
>>>>
>>>> In case the sdhci cap register, doesn't reflect the board support
>>>> properly, such that you may want to disable some speed modes, then you
>>>> may benefit from using the DT properties "sdhci-caps*.
>>>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt
>>>
>>> Hm, yeah I guess something like
>>>
>>> sdhci-caps-mask =  /bits/ 64 <((SDHCI_SUPPORT_SDR104 |
>>> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)>
>>>
>>> would come close.
>>>
>>> But it does not restrict MMC modes such as HS200/HS400. There seem to be
>>> no mmc-caps...
>>
>> Right.
>>
>> The solution to fix this, should be to *not* set those DT properties,
>> like "mmc-hs*" for example. That should work, no?
>>
>
> The controller does not make use of the dt modes so far, so I can't not
> set those properties...

Then where are the corresponding caps for the eMMC speed modes being set?

Can't you just avoid setting them?

>
>>>
>>>
>>> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not
>>> restrict modes correctly. Currently the driver checks whether >=100MHz
>>> pinctrl settings are available, and if not uses the quirk to restrict
>>> higher speed modes. Removing that would break device tree backward
>>> compatibility...
>>
>> Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather
>> how the pinctrl setting becomes interpreted when setting the quirk.
>>
>
> Yes, sorry for the confusion. SDHCI_QUIRK2_NO_1_8_V is fine, it is just
> not the quirk this driver needs.
>
> I argue that commit ad93220de7da ("mmc: sdhci-esdhc-imx: change pinctrl
> state according to uhs mode") chose the wrong quirk from the
> beginning...

That's seems reasonable! But it's been there since 3.13, so I guess we
have to think about backwards compatibility issues, as you stated.

>
> Afaict, the quirk needed here does not exist.

[...]

Kind regards
Uffe

Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

[Stefan Agner]

On 04.07.2018 13:16, Ulf Hansson wrote:
> On 4 July 2018 at 12:55, Stefan Agner <stefan@agner.ch> wrote:
>> On 04.07.2018 12:07, Ulf Hansson wrote:
>>> On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote:
>>>> On 02.07.2018 16:36, Ulf Hansson wrote:
>>>>> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote:
>>>>>> Some hosts are capable of running higher speed modes but do not
>>>>>> have the board support for it. Introduce a quirk which prevents
>>>>>> the stack from using modes running at 100MHz or faster.
>>>>>
>>>>> To cap the freq, use the DT property "max-frequency". To enable
>>>>> certain speed modes, use the corresponding speed mode binding. For
>>>>> example "sd-uhs-sdr*" and "mmc-hs200*".
>>>>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt
>>>>
>>>> I had bad experience with max-frequency: Some higher speed modes seem
>>>> not to work reliably if constraint to low frequencies. E.g. we had lots
>>>> of devices fail in practise with HS400@100MHz... So it is doing what it
>>>> should, but it just seems that higher speed modes do not necessarily run
>>>> well with lower frequencies...
>>>>
>>>> So I'd rather prefer to limit speed modes as it is done right now.
>>>>
>>>>>
>>>>> In case the sdhci cap register, doesn't reflect the board support
>>>>> properly, such that you may want to disable some speed modes, then you
>>>>> may benefit from using the DT properties "sdhci-caps*.
>>>>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt
>>>>
>>>> Hm, yeah I guess something like
>>>>
>>>> sdhci-caps-mask =  /bits/ 64 <((SDHCI_SUPPORT_SDR104 |
>>>> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)>
>>>>
>>>> would come close.
>>>>
>>>> But it does not restrict MMC modes such as HS200/HS400. There seem to be
>>>> no mmc-caps...
>>>
>>> Right.
>>>
>>> The solution to fix this, should be to *not* set those DT properties,
>>> like "mmc-hs*" for example. That should work, no?
>>>
>>
>> The controller does not make use of the dt modes so far, so I can't not
>> set those properties...
> 
> Then where are the corresponding caps for the eMMC speed modes being set?
> 
> Can't you just avoid setting them?
> 

That was the right question:

On closer look, the higher speed MMC modes are actually not set at all!

So just using sdhci-caps-mask = <0x7 0x0>; (which masks
SDR104/SDR50/DDR50) is doing the job as far as I can tell.

>>
>>>>
>>>>
>>>> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not
>>>> restrict modes correctly. Currently the driver checks whether >=100MHz
>>>> pinctrl settings are available, and if not uses the quirk to restrict
>>>> higher speed modes. Removing that would break device tree backward
>>>> compatibility...
>>>
>>> Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather
>>> how the pinctrl setting becomes interpreted when setting the quirk.
>>>
>>
>> Yes, sorry for the confusion. SDHCI_QUIRK2_NO_1_8_V is fine, it is just
>> not the quirk this driver needs.
>>
>> I argue that commit ad93220de7da ("mmc: sdhci-esdhc-imx: change pinctrl
>> state according to uhs mode") chose the wrong quirk from the
>> beginning...
> 
> That's seems reasonable! But it's been there since 3.13, so I guess we
> have to think about backwards compatibility issues, as you stated.
> 

It seems that the driver already fakes
SDHCI_CAPABILITIES/SDHCI_CAPABILITIES_1 in esdhc_readl_le, so instead
using the sdhci-caps-mask we could implement the work around there. Not
very clean, but backward compatible and everything self contained in the
driver.

--
Stefan

>>
>> Afaict, the quirk needed here does not exist.
> 
> [...]
> 
> Kind regards
> Uffe

RE: [PATCH 1/3] mmc: sdhci-esdhc-imx: get rid of support_vsel

[A.s. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Thursday, June 28, 2018 4:13 PM
> To: adrian.hunter@intel.com; ulf.hansson@linaro.org
> Cc: Fabio Estevam <fabio.estevam@nxp.com>; Bough Chen
> <haibo.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>;
> michael@amarulasolutions.com; rmk+kernel@armlinux.org.uk; linux-
> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Stefan Agner
> <stefan@agner.ch>
> Subject: [PATCH 1/3] mmc: sdhci-esdhc-imx: get rid of support_vsel
> 
> The field support_vsel is currently only used in the device tree case. Get rid
> of it. No change in behavior.
> 

I'm not sure if it's quite necessary to remove it as it's used to bypass
100Mhz above pad settings look up which is meaningless if user claims
no 1-8 v support.

If you remove it, probably you still need better check the quirk for
Pad state look up.

Regards
Dong Aisheng

> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c          | 8 ++------
>  include/linux/platform_data/mmc-esdhc-imx.h | 2 --
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-
> esdhc-imx.c
> index 6f444731754d..20a420b765b3 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1145,18 +1145,14 @@ sdhci_esdhc_imx_probe_dt(struct
> platform_device *pdev,
>  			     &boarddata->tuning_start_tap);
> 
>  	if (of_find_property(np, "no-1-8-v", NULL))
> -		boarddata->support_vsel = false;
> -	else
> -		boarddata->support_vsel = true;
> +		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
> 
>  	if (of_property_read_u32(np, "fsl,delay-line", &boarddata-
> >delay_line))
>  		boarddata->delay_line = 0;
> 
>  	mmc_of_parse_voltage(np, &host->ocr_mask);
> 
> -	/* sdr50 and sdr104 need work on 1.8v signal voltage */
> -	if ((boarddata->support_vsel) && esdhc_is_usdhc(imx_data) &&
> -	    !IS_ERR(imx_data->pins_default)) {
> +	if (esdhc_is_usdhc(imx_data) && !IS_ERR(imx_data->pins_default))
> {
>  		imx_data->pins_100mhz = pinctrl_lookup_state(imx_data-
> >pinctrl,
> 
> 	ESDHC_PINCTRL_STATE_100MHZ);
>  		imx_data->pins_200mhz = pinctrl_lookup_state(imx_data-
> >pinctrl,
> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h
> b/include/linux/platform_data/mmc-esdhc-imx.h
> index 7daa78a2f342..640dec8b5b0c 100644
> --- a/include/linux/platform_data/mmc-esdhc-imx.h
> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
> @@ -34,7 +34,6 @@ enum cd_types {
>   * @cd_gpio:	gpio for card_detect interrupt
>   * @wp_type:	type of write_protect method (see wp_types enum above)
>   * @cd_type:	type of card_detect method (see cd_types enum above)
> - * @support_vsel:  indicate it supports 1.8v switching
>   */
> 
>  struct esdhc_platform_data {
> @@ -43,7 +42,6 @@ struct esdhc_platform_data {
>  	enum wp_types wp_type;
>  	enum cd_types cd_type;
>  	int max_bus_width;
> -	bool support_vsel;
>  	unsigned int delay_line;
>  	unsigned int tuning_step;       /* The delay cell steps in tuning
> procedure */
>  	unsigned int tuning_start_tap;	/* The start delay cell point in tuning
> procedure */
> --
> 2.18.0

Re: [PATCH 1/3] mmc: sdhci-esdhc-imx: get rid of support_vsel

[Stefan Agner]

On 05.07.2018 04:52, A.s. Dong wrote:
>> -----Original Message-----
>> From: Stefan Agner [mailto:stefan@agner.ch]
>> Sent: Thursday, June 28, 2018 4:13 PM
>> To: adrian.hunter@intel.com; ulf.hansson@linaro.org
>> Cc: Fabio Estevam <fabio.estevam@nxp.com>; Bough Chen
>> <haibo.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>;
>> michael@amarulasolutions.com; rmk+kernel@armlinux.org.uk; linux-
>> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Stefan Agner
>> <stefan@agner.ch>
>> Subject: [PATCH 1/3] mmc: sdhci-esdhc-imx: get rid of support_vsel
>>
>> The field support_vsel is currently only used in the device tree case. Get rid
>> of it. No change in behavior.
>>
> 
> I'm not sure if it's quite necessary to remove it as it's used to bypass
> 100Mhz above pad settings look up which is meaningless if user claims
> no 1-8 v support.
> 
> If you remove it, probably you still need better check the quirk for
> Pad state look up.

There should be really no change in behavior, just simplifying code:

If no-1-8-v is set, SDHCI_QUIRK2_NO_1_8_V will be set in any case.

If no-1-8-v is not set, SDHCI_QUIRK2_NO_1_8_V will be set depending on
whether pinctrl states are available

This has been the case before and after this change.

--
Stefan

> 
> Regards
> Dong Aisheng
> 
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/mmc/host/sdhci-esdhc-imx.c          | 8 ++------
>>  include/linux/platform_data/mmc-esdhc-imx.h | 2 --
>>  2 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-
>> esdhc-imx.c
>> index 6f444731754d..20a420b765b3 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -1145,18 +1145,14 @@ sdhci_esdhc_imx_probe_dt(struct
>> platform_device *pdev,
>>  			     &boarddata->tuning_start_tap);
>>
>>  	if (of_find_property(np, "no-1-8-v", NULL))
>> -		boarddata->support_vsel = false;
>> -	else
>> -		boarddata->support_vsel = true;
>> +		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>
>>  	if (of_property_read_u32(np, "fsl,delay-line", &boarddata-
>> >delay_line))
>>  		boarddata->delay_line = 0;
>>
>>  	mmc_of_parse_voltage(np, &host->ocr_mask);
>>
>> -	/* sdr50 and sdr104 need work on 1.8v signal voltage */
>> -	if ((boarddata->support_vsel) && esdhc_is_usdhc(imx_data) &&
>> -	    !IS_ERR(imx_data->pins_default)) {
>> +	if (esdhc_is_usdhc(imx_data) && !IS_ERR(imx_data->pins_default))
>> {
>>  		imx_data->pins_100mhz = pinctrl_lookup_state(imx_data-
>> >pinctrl,
>>
>> 	ESDHC_PINCTRL_STATE_100MHZ);
>>  		imx_data->pins_200mhz = pinctrl_lookup_state(imx_data-
>> >pinctrl,
>> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h
>> b/include/linux/platform_data/mmc-esdhc-imx.h
>> index 7daa78a2f342..640dec8b5b0c 100644
>> --- a/include/linux/platform_data/mmc-esdhc-imx.h
>> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
>> @@ -34,7 +34,6 @@ enum cd_types {
>>   * @cd_gpio:	gpio for card_detect interrupt
>>   * @wp_type:	type of write_protect method (see wp_types enum above)
>>   * @cd_type:	type of card_detect method (see cd_types enum above)
>> - * @support_vsel:  indicate it supports 1.8v switching
>>   */
>>
>>  struct esdhc_platform_data {
>> @@ -43,7 +42,6 @@ struct esdhc_platform_data {
>>  	enum wp_types wp_type;
>>  	enum cd_types cd_type;
>>  	int max_bus_width;
>> -	bool support_vsel;
>>  	unsigned int delay_line;
>>  	unsigned int tuning_step;       /* The delay cell steps in tuning
>> procedure */
>>  	unsigned int tuning_start_tap;	/* The start delay cell point in tuning
>> procedure */
>> --
>> 2.18.0