linux-mmc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] sdhci-of-arasan: card initialization failure -84
@ 2018-06-04  6:42 Helmut Grohne
  2018-06-14  7:14 ` Adrian Hunter
  2018-06-14  7:19 ` Adrian Hunter
  0 siblings, 2 replies; 11+ messages in thread
From: Helmut Grohne @ 2018-06-04  6:42 UTC (permalink / raw)
  To: Adrian Hunter, Michal Simek, Sören Brinkmann, Ulf Hansson,
	linux-mmc
  Cc: linux-arm-kernel

Work around repeated:

    mmc0: error -84 whilst initialising SD card
---
 drivers/mmc/host/sdhci.c | 1 +
 1 file changed, 1 insertion(+)

I'm seeing the above error -84 with a series of Kingston cards of 4GB capacity
when used with an arasan host controller. Using the cards with other hosts or
using other cards with the host is not problematic. The host controller has the
following device tree:

	ps7-sdio@e0100000 {
		compatible = "arasan,sdhci-8.9a";
		reg = <0xe0100000 0x1000>;
		clock-names = "clk_xin", "clk_ahb";
		clock-frequency = <50000000>;
		clocks = <&clkc 21>, <&clkc 32>;
		interrupt-parent = <&ps7_scugic_0>;
		interrupts = <0 24 4>;
	};

When enabling sufficient debugging (and thus slowing the kernel down), the
problem goes away. Without debug features, the insertion of this delay suffices
for my particular instance. Inserting the same delay before the
SDHCI_CLOCK_CARD_EN write also makes the error go away.

While instrumenting the code I found that the loop waiting for the
SDHCI_CLOCK_INT_STABLE would always terminate immediately (regardless of the
card being used). Only when the card is removed, the loop would spin once. This
hints that potentially the stable checking is broken for this host controller.

During my testing I checked 7 Kingston cards, 2 Transcend Cards, 1 Apacer card,
3 arasn hosts, 3 non-arasan hosts and had consistent reports for a similar
number of devices from others.

If my hypothesis is reasonable, I can turn this patch into a new quirk that
replaces the stable polling with msleep(20), which is the poll timeout. Is
there anything else I should investigate before doing so?

Helmut

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 90cc1977b792..5d2809aeb4eb 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1430,6 +1430,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
 
 	clk |= SDHCI_CLOCK_CARD_EN;
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+	udelay(80);
 }
 EXPORT_SYMBOL_GPL(sdhci_enable_clk);
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] sdhci-of-arasan: card initialization failure -84
  2018-06-04  6:42 [RFC PATCH] sdhci-of-arasan: card initialization failure -84 Helmut Grohne
@ 2018-06-14  7:14 ` Adrian Hunter
  2018-06-14  7:19 ` Adrian Hunter
  1 sibling, 0 replies; 11+ messages in thread
From: Adrian Hunter @ 2018-06-14  7:14 UTC (permalink / raw)
  To: Helmut Grohne, Michal Simek, Sören Brinkmann, Ulf Hansson,
	linux-mmc
  Cc: linux-arm-kernel, Atul Garg

On 04/06/18 09:42, Helmut Grohne wrote:
> Work around repeated:
> 
>     mmc0: error -84 whilst initialising SD card
> ---
>  drivers/mmc/host/sdhci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> I'm seeing the above error -84 with a series of Kingston cards of 4GB capacity
> when used with an arasan host controller. Using the cards with other hosts or
> using other cards with the host is not problematic. The host controller has the
> following device tree:
> 
> 	ps7-sdio@e0100000 {
> 		compatible = "arasan,sdhci-8.9a";
> 		reg = <0xe0100000 0x1000>;
> 		clock-names = "clk_xin", "clk_ahb";
> 		clock-frequency = <50000000>;
> 		clocks = <&clkc 21>, <&clkc 32>;
> 		interrupt-parent = <&ps7_scugic_0>;
> 		interrupts = <0 24 4>;
> 	};
> 
> When enabling sufficient debugging (and thus slowing the kernel down), the
> problem goes away. Without debug features, the insertion of this delay suffices
> for my particular instance. Inserting the same delay before the
> SDHCI_CLOCK_CARD_EN write also makes the error go away.
> 
> While instrumenting the code I found that the loop waiting for the
> SDHCI_CLOCK_INT_STABLE would always terminate immediately (regardless of the
> card being used). Only when the card is removed, the loop would spin once. This
> hints that potentially the stable checking is broken for this host controller.
> 
> During my testing I checked 7 Kingston cards, 2 Transcend Cards, 1 Apacer card,
> 3 arasn hosts, 3 non-arasan hosts and had consistent reports for a similar
> number of devices from others.
> 
> If my hypothesis is reasonable, I can turn this patch into a new quirk that
> replaces the stable polling with msleep(20), which is the poll timeout. Is
> there anything else I should investigate before doing so?

Please avoid quirks.  Perhaps use the ->set_clock() callback.

> 
> Helmut
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 90cc1977b792..5d2809aeb4eb 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1430,6 +1430,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>  
>  	clk |= SDHCI_CLOCK_CARD_EN;
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +	udelay(80);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_enable_clk);
>  
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH] sdhci-of-arasan: card initialization failure -84
  2018-06-04  6:42 [RFC PATCH] sdhci-of-arasan: card initialization failure -84 Helmut Grohne
  2018-06-14  7:14 ` Adrian Hunter
@ 2018-06-14  7:19 ` Adrian Hunter
  2018-06-15  8:18   ` [PATCH v2] mmc: sdhci-of-arasan: Add quirk for unstable clocks Helmut Grohne
  1 sibling, 1 reply; 11+ messages in thread
From: Adrian Hunter @ 2018-06-14  7:19 UTC (permalink / raw)
  To: Helmut Grohne, Michal Simek, Sören Brinkmann, Ulf Hansson,
	linux-mmc
  Cc: Atul Garg, linux-arm-kernel

And again without mangling Atul's email address.

On 04/06/18 09:42, Helmut Grohne wrote:
> Work around repeated:
> 
>     mmc0: error -84 whilst initialising SD card
> ---
>  drivers/mmc/host/sdhci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> I'm seeing the above error -84 with a series of Kingston cards of 4GB capacity
> when used with an arasan host controller. Using the cards with other hosts or
> using other cards with the host is not problematic. The host controller has the
> following device tree:
> 
> 	ps7-sdio@e0100000 {
> 		compatible = "arasan,sdhci-8.9a";
> 		reg = <0xe0100000 0x1000>;
> 		clock-names = "clk_xin", "clk_ahb";
> 		clock-frequency = <50000000>;
> 		clocks = <&clkc 21>, <&clkc 32>;
> 		interrupt-parent = <&ps7_scugic_0>;
> 		interrupts = <0 24 4>;
> 	};
> 
> When enabling sufficient debugging (and thus slowing the kernel down), the
> problem goes away. Without debug features, the insertion of this delay suffices
> for my particular instance. Inserting the same delay before the
> SDHCI_CLOCK_CARD_EN write also makes the error go away.
> 
> While instrumenting the code I found that the loop waiting for the
> SDHCI_CLOCK_INT_STABLE would always terminate immediately (regardless of the
> card being used). Only when the card is removed, the loop would spin once. This
> hints that potentially the stable checking is broken for this host controller.
> 
> During my testing I checked 7 Kingston cards, 2 Transcend Cards, 1 Apacer card,
> 3 arasn hosts, 3 non-arasan hosts and had consistent reports for a similar
> number of devices from others.
> 
> If my hypothesis is reasonable, I can turn this patch into a new quirk that
> replaces the stable polling with msleep(20), which is the poll timeout. Is
> there anything else I should investigate before doing so?

Please avoid quirks.  Perhaps use the ->set_clock() callback.

> 
> Helmut
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 90cc1977b792..5d2809aeb4eb 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1430,6 +1430,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>  
>  	clk |= SDHCI_CLOCK_CARD_EN;
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +	udelay(80);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_enable_clk);
>  
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] mmc: sdhci-of-arasan: Add quirk for unstable clocks
  2018-06-14  7:19 ` Adrian Hunter
@ 2018-06-15  8:18   ` Helmut Grohne
  2018-06-18  8:59     ` Ulf Hansson
                       ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Helmut Grohne @ 2018-06-15  8:18 UTC (permalink / raw)
  To: Adrian Hunter, Michal Simek, Sören Brinkmann, Ulf Hansson,
	linux-mmc
  Cc: Atul Garg, linux-arm-kernel

Some controllers immediately report SDHCI_CLOCK_INT_STABLE after
enabling the clock even when the clock is not stable. When used in
conjunction with older/slower cards, this can result in:

    mmc0: error -84 whilst initialising SD card

When the stable reporting is broken, we simply wait for the maximum
stabilization period.

Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt |  2 ++
 drivers/mmc/host/sdhci-of-arasan.c                     | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

Changes since v1 (RFC):
 * Use an arasan-specific quirk in the ->set_clock() callback as requested by
   Adrian Hunter.

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 60481bfc3d31..c0e0f04a8504 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -39,6 +39,8 @@ Optional Properties:
   - xlnx,fails-without-test-cd: when present, the controller doesn't work when
     the CD line is not connected properly, and the line is not connected
     properly. Test mode can be used to force the controller to function.
+  - xlnx,int-clock-stable-broken: when present, the controller always reports
+    that the internal clock is stable even when it is not.
 
 Example:
 	sdhci@e0100000 {
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7393bd..f7fe26c75150 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -102,6 +102,9 @@ struct sdhci_arasan_data {
 
 /* Controller does not have CD wired and will not function normally without */
 #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST	BIT(0)
+/* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
+ * internal clock even when the clock isn't stable */
+#define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
 };
 
 static const struct sdhci_arasan_soc_ctl_map rk3399_soc_ctl_map = {
@@ -207,6 +210,16 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	sdhci_set_clock(host, clock);
 
+	if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE)
+		/*
+		 * Some controllers immediately report SDHCI_CLOCK_INT_STABLE
+		 * after enabling the clock even though the clock is not
+		 * stable. Trying to use a clock without waiting here results
+		 * in EILSEQ while detecting some older/slower cards. The
+		 * chosen delay is the maximum delay from sdhci_set_clock.
+		 */
+		msleep(20);
+
 	if (ctrl_phy) {
 		phy_power_on(sdhci_arasan->phy);
 		sdhci_arasan->is_phy_on = true;
@@ -759,6 +772,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
 		sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
 
+	if (of_property_read_bool(np, "xlnx,int-clock-stable-broken"))
+		sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE;
+
 	pltfm_host->clk = clk_xin;
 
 	if (of_device_is_compatible(pdev->dev.of_node,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] mmc: sdhci-of-arasan: Add quirk for unstable clocks
  2018-06-15  8:18   ` [PATCH v2] mmc: sdhci-of-arasan: Add quirk for unstable clocks Helmut Grohne
@ 2018-06-18  8:59     ` Ulf Hansson
  2018-06-19  9:09     ` Adrian Hunter
  2018-06-20 11:32     ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Helmut Grohne
  2 siblings, 0 replies; 11+ messages in thread
From: Ulf Hansson @ 2018-06-18  8:59 UTC (permalink / raw)
  To: Helmut Grohne
  Cc: Atul Garg, linux-mmc@vger.kernel.org, Adrian Hunter, Michal Simek,
	Sören Brinkmann, Linux ARM

On 15 June 2018 at 10:18, Helmut Grohne <h.grohne@intenta.de> wrote:
> Some controllers immediately report SDHCI_CLOCK_INT_STABLE after
> enabling the clock even when the clock is not stable. When used in
> conjunction with older/slower cards, this can result in:
>
>     mmc0: error -84 whilst initialising SD card
>
> When the stable reporting is broken, we simply wait for the maximum
> stabilization period.
>
> Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt |  2 ++

Please split this. DT doc should be a separate patch.

Otherwise this looks good to me.

Kind regards
Uffe

>  drivers/mmc/host/sdhci-of-arasan.c                     | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
>
> Changes since v1 (RFC):
>  * Use an arasan-specific quirk in the ->set_clock() callback as requested by
>    Adrian Hunter.
>
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 60481bfc3d31..c0e0f04a8504 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -39,6 +39,8 @@ Optional Properties:
>    - xlnx,fails-without-test-cd: when present, the controller doesn't work when
>      the CD line is not connected properly, and the line is not connected
>      properly. Test mode can be used to force the controller to function.
> +  - xlnx,int-clock-stable-broken: when present, the controller always reports
> +    that the internal clock is stable even when it is not.
>
>  Example:
>         sdhci@e0100000 {
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index c33a5f7393bd..f7fe26c75150 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -102,6 +102,9 @@ struct sdhci_arasan_data {
>
>  /* Controller does not have CD wired and will not function normally without */
>  #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST        BIT(0)
> +/* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
> + * internal clock even when the clock isn't stable */
> +#define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
>  };
>
>  static const struct sdhci_arasan_soc_ctl_map rk3399_soc_ctl_map = {
> @@ -207,6 +210,16 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>
>         sdhci_set_clock(host, clock);
>
> +       if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE)
> +               /*
> +                * Some controllers immediately report SDHCI_CLOCK_INT_STABLE
> +                * after enabling the clock even though the clock is not
> +                * stable. Trying to use a clock without waiting here results
> +                * in EILSEQ while detecting some older/slower cards. The
> +                * chosen delay is the maximum delay from sdhci_set_clock.
> +                */
> +               msleep(20);
> +
>         if (ctrl_phy) {
>                 phy_power_on(sdhci_arasan->phy);
>                 sdhci_arasan->is_phy_on = true;
> @@ -759,6 +772,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>         if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
>                 sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>
> +       if (of_property_read_bool(np, "xlnx,int-clock-stable-broken"))
> +               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE;
> +
>         pltfm_host->clk = clk_xin;
>
>         if (of_device_is_compatible(pdev->dev.of_node,
> --
> 2.11.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] mmc: sdhci-of-arasan: Add quirk for unstable clocks
  2018-06-15  8:18   ` [PATCH v2] mmc: sdhci-of-arasan: Add quirk for unstable clocks Helmut Grohne
  2018-06-18  8:59     ` Ulf Hansson
@ 2018-06-19  9:09     ` Adrian Hunter
  2018-06-20 11:32     ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Helmut Grohne
  2 siblings, 0 replies; 11+ messages in thread
From: Adrian Hunter @ 2018-06-19  9:09 UTC (permalink / raw)
  To: Helmut Grohne, Michal Simek, Sören Brinkmann, Ulf Hansson,
	linux-mmc
  Cc: Atul Garg, linux-arm-kernel

On 15/06/18 11:18, Helmut Grohne wrote:
> Some controllers immediately report SDHCI_CLOCK_INT_STABLE after
> enabling the clock even when the clock is not stable. When used in
> conjunction with older/slower cards, this can result in:
> 
>     mmc0: error -84 whilst initialising SD card
> 
> When the stable reporting is broken, we simply wait for the maximum
> stabilization period.
> 
> Signed-off-by: Helmut Grohne <h.grohne@intenta.de>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt |  2 ++
>  drivers/mmc/host/sdhci-of-arasan.c                     | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> Changes since v1 (RFC):
>  * Use an arasan-specific quirk in the ->set_clock() callback as requested by
>    Adrian Hunter.
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 60481bfc3d31..c0e0f04a8504 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -39,6 +39,8 @@ Optional Properties:
>    - xlnx,fails-without-test-cd: when present, the controller doesn't work when
>      the CD line is not connected properly, and the line is not connected
>      properly. Test mode can be used to force the controller to function.
> +  - xlnx,int-clock-stable-broken: when present, the controller always reports
> +    that the internal clock is stable even when it is not.
>  
>  Example:
>  	sdhci@e0100000 {
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index c33a5f7393bd..f7fe26c75150 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -102,6 +102,9 @@ struct sdhci_arasan_data {
>  
>  /* Controller does not have CD wired and will not function normally without */
>  #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST	BIT(0)
> +/* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
> + * internal clock even when the clock isn't stable */
> +#define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
>  };
>  
>  static const struct sdhci_arasan_soc_ctl_map rk3399_soc_ctl_map = {
> @@ -207,6 +210,16 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  	sdhci_set_clock(host, clock);
>  
> +	if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE)
> +		/*
> +		 * Some controllers immediately report SDHCI_CLOCK_INT_STABLE
> +		 * after enabling the clock even though the clock is not
> +		 * stable. Trying to use a clock without waiting here results
> +		 * in EILSEQ while detecting some older/slower cards. The
> +		 * chosen delay is the maximum delay from sdhci_set_clock.
> +		 */
> +		msleep(20);
> +
>  	if (ctrl_phy) {
>  		phy_power_on(sdhci_arasan->phy);
>  		sdhci_arasan->is_phy_on = true;
> @@ -759,6 +772,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  	if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
>  		sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>  
> +	if (of_property_read_bool(np, "xlnx,int-clock-stable-broken"))
> +		sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE;
> +
>  	pltfm_host->clk = clk_xin;
>  
>  	if (of_device_is_compatible(pdev->dev.of_node,
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks
  2018-06-15  8:18   ` [PATCH v2] mmc: sdhci-of-arasan: Add quirk for unstable clocks Helmut Grohne
  2018-06-18  8:59     ` Ulf Hansson
  2018-06-19  9:09     ` Adrian Hunter
@ 2018-06-20 11:32     ` Helmut Grohne
  2018-06-20 11:32       ` [PATCH v3 1/2] dt-bindings: mmc: broken clock stable indicator on arasan controllers Helmut Grohne
                         ` (2 more replies)
  2 siblings, 3 replies; 11+ messages in thread
From: Helmut Grohne @ 2018-06-20 11:32 UTC (permalink / raw)
  To: Adrian Hunter, Michal Simek, Sören Brinkmann, Ulf Hansson,
	Rob Herring, Mark Rutland, linux-mmc, devicetree
  Cc: Atul Garg, linux-arm-kernel

This patch series works around the internal clock indicating stability too
quickly to operate. It manifests itself as:

    mmc0: error -84 whilst initialising SD card

Changes since v2:
 * Ulf Hansson requested splitting the patch in accordance with
   Documentation/devicetree/bindings/submitting-patches.txt. Also update
   recipient list accordingly.
 * Added Acked-by from Adrian Hunter.

Helmut Grohne (2):
  dt-bindings: mmc: broken clock stable indicator on arasan controllers
  mmc: sdhci-of-arasan: Add quirk for unstable clocks

 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt |  2 ++
 drivers/mmc/host/sdhci-of-arasan.c                     | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/2] dt-bindings: mmc: broken clock stable indicator on arasan controllers
  2018-06-20 11:32     ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Helmut Grohne
@ 2018-06-20 11:32       ` Helmut Grohne
  2018-06-20 19:08         ` Rob Herring
  2018-06-20 11:33       ` [PATCH v3 2/2] mmc: sdhci-of-arasan: Add quirk for unstable clocks Helmut Grohne
  2018-07-02 13:17       ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Ulf Hansson
  2 siblings, 1 reply; 11+ messages in thread
From: Helmut Grohne @ 2018-06-20 11:32 UTC (permalink / raw)
  To: Adrian Hunter, Michal Simek, Sören Brinkmann, Ulf Hansson,
	Rob Herring, Mark Rutland, linux-mmc, devicetree
  Cc: Atul Garg, linux-arm-kernel

Some controllers immediately report that their internal clock is stable
after activating it even when the clock is not stable. When used in
conjunction with older/slower cards, this can result in:

    mmc0: error -84 whilst initialising SD card

This flag allows documenting and thus working around such a hardware
defect.

Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 60481bfc3d31..28332b655d2e 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -39,6 +39,8 @@ Optional Properties:
   - xlnx,fails-without-test-cd: when present, the controller doesn't work when
     the CD line is not connected properly, and the line is not connected
     properly. Test mode can be used to force the controller to function.
+  - xlnx,int-clock-stable-broken: when present, the controller always reports
+    that the internal clock is stable even when it is not.
 
 Example:
 	sdhci@e0100000 {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/2] mmc: sdhci-of-arasan: Add quirk for unstable clocks
  2018-06-20 11:32     ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Helmut Grohne
  2018-06-20 11:32       ` [PATCH v3 1/2] dt-bindings: mmc: broken clock stable indicator on arasan controllers Helmut Grohne
@ 2018-06-20 11:33       ` Helmut Grohne
  2018-07-02 13:17       ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Ulf Hansson
  2 siblings, 0 replies; 11+ messages in thread
From: Helmut Grohne @ 2018-06-20 11:33 UTC (permalink / raw)
  To: Adrian Hunter, Michal Simek, Sören Brinkmann, Ulf Hansson,
	Rob Herring, Mark Rutland, linux-mmc, devicetree
  Cc: Atul Garg, linux-arm-kernel

Some controllers immediately report SDHCI_CLOCK_INT_STABLE after
enabling the clock even when the clock is not stable. When used in
conjunction with older/slower cards, this can result in:

    mmc0: error -84 whilst initialising SD card

When the stable reporting is known to be broken, we simply wait for the
maximum stabilization period.

Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index e3332a522a5d..a40bcc27f187 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -102,6 +102,9 @@ struct sdhci_arasan_data {
 
 /* Controller does not have CD wired and will not function normally without */
 #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST	BIT(0)
+/* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
+ * internal clock even when the clock isn't stable */
+#define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
 };
 
 static const struct sdhci_arasan_soc_ctl_map rk3399_soc_ctl_map = {
@@ -207,6 +210,16 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	sdhci_set_clock(host, clock);
 
+	if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE)
+		/*
+		 * Some controllers immediately report SDHCI_CLOCK_INT_STABLE
+		 * after enabling the clock even though the clock is not
+		 * stable. Trying to use a clock without waiting here results
+		 * in EILSEQ while detecting some older/slower cards. The
+		 * chosen delay is the maximum delay from sdhci_set_clock.
+		 */
+		msleep(20);
+
 	if (ctrl_phy) {
 		phy_power_on(sdhci_arasan->phy);
 		sdhci_arasan->is_phy_on = true;
@@ -758,6 +771,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
 		sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
 
+	if (of_property_read_bool(np, "xlnx,int-clock-stable-broken"))
+		sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE;
+
 	pltfm_host->clk = clk_xin;
 
 	if (of_device_is_compatible(pdev->dev.of_node,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: mmc: broken clock stable indicator on arasan controllers
  2018-06-20 11:32       ` [PATCH v3 1/2] dt-bindings: mmc: broken clock stable indicator on arasan controllers Helmut Grohne
@ 2018-06-20 19:08         ` Rob Herring
  0 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2018-06-20 19:08 UTC (permalink / raw)
  To: Helmut Grohne
  Cc: Mark Rutland, devicetree, Ulf Hansson, Atul Garg, linux-mmc,
	Adrian Hunter, Michal Simek, linux-arm-kernel,
	Sören Brinkmann

On Wed, Jun 20, 2018 at 01:32:51PM +0200, Helmut Grohne wrote:
> Some controllers immediately report that their internal clock is stable
> after activating it even when the clock is not stable. When used in
> conjunction with older/slower cards, this can result in:
> 
>     mmc0: error -84 whilst initialising SD card
> 
> This flag allows documenting and thus working around such a hardware
> defect.
> 
> Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks
  2018-06-20 11:32     ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Helmut Grohne
  2018-06-20 11:32       ` [PATCH v3 1/2] dt-bindings: mmc: broken clock stable indicator on arasan controllers Helmut Grohne
  2018-06-20 11:33       ` [PATCH v3 2/2] mmc: sdhci-of-arasan: Add quirk for unstable clocks Helmut Grohne
@ 2018-07-02 13:17       ` Ulf Hansson
  2 siblings, 0 replies; 11+ messages in thread
From: Ulf Hansson @ 2018-07-02 13:17 UTC (permalink / raw)
  To: Helmut Grohne
  Cc: Mark Rutland, devicetree, Atul Garg, linux-mmc@vger.kernel.org,
	Adrian Hunter, Michal Simek, Rob Herring, Linux ARM,
	Sören Brinkmann

On 20 June 2018 at 13:32, Helmut Grohne <h.grohne@intenta.de> wrote:
> This patch series works around the internal clock indicating stability too
> quickly to operate. It manifests itself as:
>
>     mmc0: error -84 whilst initialising SD card
>
> Changes since v2:
>  * Ulf Hansson requested splitting the patch in accordance with
>    Documentation/devicetree/bindings/submitting-patches.txt. Also update
>    recipient list accordingly.
>  * Added Acked-by from Adrian Hunter.
>
> Helmut Grohne (2):
>   dt-bindings: mmc: broken clock stable indicator on arasan controllers
>   mmc: sdhci-of-arasan: Add quirk for unstable clocks
>
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt |  2 ++
>  drivers/mmc/host/sdhci-of-arasan.c                     | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
>
> --
> 2.11.0
>

Thanks, applied for next!

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-07-02 13:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-04  6:42 [RFC PATCH] sdhci-of-arasan: card initialization failure -84 Helmut Grohne
2018-06-14  7:14 ` Adrian Hunter
2018-06-14  7:19 ` Adrian Hunter
2018-06-15  8:18   ` [PATCH v2] mmc: sdhci-of-arasan: Add quirk for unstable clocks Helmut Grohne
2018-06-18  8:59     ` Ulf Hansson
2018-06-19  9:09     ` Adrian Hunter
2018-06-20 11:32     ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Helmut Grohne
2018-06-20 11:32       ` [PATCH v3 1/2] dt-bindings: mmc: broken clock stable indicator on arasan controllers Helmut Grohne
2018-06-20 19:08         ` Rob Herring
2018-06-20 11:33       ` [PATCH v3 2/2] mmc: sdhci-of-arasan: Add quirk for unstable clocks Helmut Grohne
2018-07-02 13:17       ` [PATCH v3 0/2] mmc: sdhci-of-arasan: workaround for broken clocks Ulf Hansson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).