Re: [RFC 2/2] mmc: sdhci: Ignore capability register when it comes to speeds and use DT binding instead when sdhci-cap-speed-modes-broken is set.

[Ulf Hansson]

On 18 October 2016 at 23:05, Zach Brown  wrote:
> When the sdhci-cap-speed-modes-broken DT property is set, the driver
> will ignore the bits of the capability registers that correspond to
> speed modes and will read the of properties of the device to determine
> which speeds are supported.

To me this seems like a great idea. Yeah, I proposed it so I guess
that's why. :-)

Anyway, it's Adrian call to decide how he want to go with this. He
might consider the other option [1] to be better.

Some more comments below.

>
> Signed-off-by: Zach Brown 
> ---
>  drivers/mmc/host/sdhci.c | 31 +++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1e25b01..100649a 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>
> @@ -3020,6 +3021,32 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 
> *ver, u32 *caps, u32 *caps1)
>  }
>  EXPORT_SYMBOL_GPL(__sdhci_read_caps);
>
> +void sdhci_get_speed_caps_from_of(struct sdhci_host *host)
> +{
> +   struct mmc_host *mmc = host->mmc;
> +
> +   host->caps &= ~SDHCI_CAN_DO_HISPD;
> +
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node, "cap-sd-highspeed"))
> +   host->caps |= SDHCI_CAN_DO_HISPD;
> +
> +   if (host->version < SDHCI_SPEC_300)
> +   return;
> +
> +   host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
> +SDHCI_SUPPORT_DDR50);
> +
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-sdr50"))
> +   host->caps1 |= SDHCI_SUPPORT_SDR50;
> +
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-sdr104"))
> +   host->caps1 |= SDHCI_SUPPORT_SDR104;
> +
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-ddr50"))
> +   host->caps1 |= SDHCI_SUPPORT_DDR50;
> +

I don't think you need a separate OF parsing function. Instead the
SDHCI variant drivers may call mmc_of_parse() to parse the generic mmc
OF properties and then read the SDHCI caps registers (in some way or
the other).

As reading the SDHCI caps registers is done in __sdhci_read_caps(),
you could instead check for the OF property
"sdhci-cap-speed-modes-broken" in there,  and if it's set, clear the
related bits. I think that's all you need.

Note, the above code considers only SD speed modes, I assume we should
include eMMC speed modes to be broken as well!?

> +}
> +
>  int sdhci_setup_host(struct sdhci_host *host)
>  {
> struct mmc_host *mmc;
> @@ -3046,6 +3073,10 @@ int sdhci_setup_host(struct sdhci_host *host)
> return ret;
>
> sdhci_read_caps(host);
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node,
> + "sdhci-cap-speed-modes-broken"))
> +   sdhci_get_speed_caps_from_of(host);
> +
>
> override_timeout_clk = host->timeout_clk;
>
> --
> 2.7.4
>

Kind regards
Uffe

[1]
http://www.spinics.net/lists/devicetree/msg146401.html

Re: [RFC 2/2] mmc: sdhci: Ignore capability register when it comes to speeds and use DT binding instead when sdhci-cap-speed-modes-broken is set.

[Ulf Hansson]

On 18 October 2016 at 23:05, Zach Brown  wrote:
> When the sdhci-cap-speed-modes-broken DT property is set, the driver
> will ignore the bits of the capability registers that correspond to
> speed modes and will read the of properties of the device to determine
> which speeds are supported.

To me this seems like a great idea. Yeah, I proposed it so I guess
that's why. :-)

Anyway, it's Adrian call to decide how he want to go with this. He
might consider the other option [1] to be better.

Some more comments below.

>
> Signed-off-by: Zach Brown 
> ---
>  drivers/mmc/host/sdhci.c | 31 +++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1e25b01..100649a 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>
> @@ -3020,6 +3021,32 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 
> *ver, u32 *caps, u32 *caps1)
>  }
>  EXPORT_SYMBOL_GPL(__sdhci_read_caps);
>
> +void sdhci_get_speed_caps_from_of(struct sdhci_host *host)
> +{
> +   struct mmc_host *mmc = host->mmc;
> +
> +   host->caps &= ~SDHCI_CAN_DO_HISPD;
> +
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node, "cap-sd-highspeed"))
> +   host->caps |= SDHCI_CAN_DO_HISPD;
> +
> +   if (host->version < SDHCI_SPEC_300)
> +   return;
> +
> +   host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
> +SDHCI_SUPPORT_DDR50);
> +
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-sdr50"))
> +   host->caps1 |= SDHCI_SUPPORT_SDR50;
> +
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-sdr104"))
> +   host->caps1 |= SDHCI_SUPPORT_SDR104;
> +
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-ddr50"))
> +   host->caps1 |= SDHCI_SUPPORT_DDR50;
> +

I don't think you need a separate OF parsing function. Instead the
SDHCI variant drivers may call mmc_of_parse() to parse the generic mmc
OF properties and then read the SDHCI caps registers (in some way or
the other).

As reading the SDHCI caps registers is done in __sdhci_read_caps(),
you could instead check for the OF property
"sdhci-cap-speed-modes-broken" in there,  and if it's set, clear the
related bits. I think that's all you need.

Note, the above code considers only SD speed modes, I assume we should
include eMMC speed modes to be broken as well!?

> +}
> +
>  int sdhci_setup_host(struct sdhci_host *host)
>  {
> struct mmc_host *mmc;
> @@ -3046,6 +3073,10 @@ int sdhci_setup_host(struct sdhci_host *host)
> return ret;
>
> sdhci_read_caps(host);
> +   if (of_property_read_bool(mmc_dev(mmc)->of_node,
> + "sdhci-cap-speed-modes-broken"))
> +   sdhci_get_speed_caps_from_of(host);
> +
>
> override_timeout_clk = host->timeout_clk;
>
> --
> 2.7.4
>

Kind regards
Uffe

[1]
http://www.spinics.net/lists/devicetree/msg146401.html

[RFC 2/2] mmc: sdhci: Ignore capability register when it comes to speeds and use DT binding instead when sdhci-cap-speed-modes-broken is set.

[Zach Brown]

When the sdhci-cap-speed-modes-broken DT property is set, the driver
will ignore the bits of the capability registers that correspond to
speed modes and will read the of properties of the device to determine
which speeds are supported.

Signed-off-by: Zach Brown 
---
 drivers/mmc/host/sdhci.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1e25b01..100649a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -3020,6 +3021,32 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 
*ver, u32 *caps, u32 *caps1)
 }
 EXPORT_SYMBOL_GPL(__sdhci_read_caps);
 
+void sdhci_get_speed_caps_from_of(struct sdhci_host *host)
+{
+   struct mmc_host *mmc = host->mmc;
+
+   host->caps &= ~SDHCI_CAN_DO_HISPD;
+
+   if (of_property_read_bool(mmc_dev(mmc)->of_node, "cap-sd-highspeed"))
+   host->caps |= SDHCI_CAN_DO_HISPD;
+
+   if (host->version < SDHCI_SPEC_300)
+   return;
+
+   host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
+SDHCI_SUPPORT_DDR50);
+
+   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-sdr50"))
+   host->caps1 |= SDHCI_SUPPORT_SDR50;
+
+   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-sdr104"))
+   host->caps1 |= SDHCI_SUPPORT_SDR104;
+
+   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-ddr50"))
+   host->caps1 |= SDHCI_SUPPORT_DDR50;
+
+}
+
 int sdhci_setup_host(struct sdhci_host *host)
 {
struct mmc_host *mmc;
@@ -3046,6 +3073,10 @@ int sdhci_setup_host(struct sdhci_host *host)
return ret;
 
sdhci_read_caps(host);
+   if (of_property_read_bool(mmc_dev(mmc)->of_node,
+ "sdhci-cap-speed-modes-broken"))
+   sdhci_get_speed_caps_from_of(host);
+
 
override_timeout_clk = host->timeout_clk;
 
-- 
2.7.4

[RFC 2/2] mmc: sdhci: Ignore capability register when it comes to speeds and use DT binding instead when sdhci-cap-speed-modes-broken is set.

[Zach Brown]

When the sdhci-cap-speed-modes-broken DT property is set, the driver
will ignore the bits of the capability registers that correspond to
speed modes and will read the of properties of the device to determine
which speeds are supported.

Signed-off-by: Zach Brown 
---
 drivers/mmc/host/sdhci.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1e25b01..100649a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -3020,6 +3021,32 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 
*ver, u32 *caps, u32 *caps1)
 }
 EXPORT_SYMBOL_GPL(__sdhci_read_caps);
 
+void sdhci_get_speed_caps_from_of(struct sdhci_host *host)
+{
+   struct mmc_host *mmc = host->mmc;
+
+   host->caps &= ~SDHCI_CAN_DO_HISPD;
+
+   if (of_property_read_bool(mmc_dev(mmc)->of_node, "cap-sd-highspeed"))
+   host->caps |= SDHCI_CAN_DO_HISPD;
+
+   if (host->version < SDHCI_SPEC_300)
+   return;
+
+   host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
+SDHCI_SUPPORT_DDR50);
+
+   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-sdr50"))
+   host->caps1 |= SDHCI_SUPPORT_SDR50;
+
+   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-sdr104"))
+   host->caps1 |= SDHCI_SUPPORT_SDR104;
+
+   if (of_property_read_bool(mmc_dev(mmc)->of_node, "sd-uhs-ddr50"))
+   host->caps1 |= SDHCI_SUPPORT_DDR50;
+
+}
+
 int sdhci_setup_host(struct sdhci_host *host)
 {
struct mmc_host *mmc;
@@ -3046,6 +3073,10 @@ int sdhci_setup_host(struct sdhci_host *host)
return ret;
 
sdhci_read_caps(host);
+   if (of_property_read_bool(mmc_dev(mmc)->of_node,
+ "sdhci-cap-speed-modes-broken"))
+   sdhci_get_speed_caps_from_of(host);
+
 
override_timeout_clk = host->timeout_clk;
 
-- 
2.7.4