[PATCH v2 0/3] mmc: Introduce support for WP GPIO in the core SDHCI

[PATCH v2 0/3] mmc: Introduce support for WP GPIO in the core SDHCI

[Thomas Petazzoni]

Hello,

While doing the bring up of a Zynq 7000 platform where the WP signal
of a SD slot is connected to a regular GPIO rather than through the
SDHCI WP pin, I realized that the GPIO described by wp-gpios was
properly requested, but it was in fact not used at all.

Indeed, the SDHCI core implements sdhci_check_ro() by:

 - Calling a controller-specific ->get_ro() callback if it exists. A
   few controller-specific drivers implement this, but not
   sdhci-of-arasan, which is used on Zynq 7000.

 - Using the SDHCI_PRESENT_STATE register, which reports the state of
   the SDHCI interface WP pin, and obvisouly not the state of a
   separate WP GPIO.

This patch series therefore changes sdhci_check_ro() to behave like
sdhci_get_cd(): use a GPIO first if available, and if not, fallback to
using the SDHCI_PRESENT_STATE register. Indeed, if there's a wp-gpios
described in the DT, it quite certainly indicates that the SDHCI WP
signal is not used, and the WP GPIO should be used instead.

As part of this series, two SDHCI drivers are modified to no longer
implement their custom ->get_ro() hook, since the core SDHCI now does
the right thing with the WP GPIO.

Changes since v1:
- Call the ->get_ro() callback before using the WP GPIO in the core,
  as suggested by Adrian Hunter.
- Fix typoes in commit logs.
- Collect Reviewed-by/Tested-by/Acked-by tags.

Best regards,

Thomas

Thomas Petazzoni (3):
  mmc: sdhci: use WP GPIO in sdhci_check_ro()
  mmc: sdhci-omap: drop ->get_ro() implementation
  mmc: sdhci-tegra: drop ->get_ro() implementation

 drivers/mmc/host/sdhci-omap.c  | 1 -
 drivers/mmc/host/sdhci-tegra.c | 9 ---------
 drivers/mmc/host/sdhci.c       | 9 ++++++---
 3 files changed, 6 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] mmc: sdhci: use WP GPIO in sdhci_check_ro()

[Thomas Petazzoni]

Even though SDHCI controllers may have a dedicated WP pin that can be
queried using the SDHCI_PRESENT_STATE register, some platforms may
chose to use a separate regular GPIO to route the WP signal. Such a
GPIO is typically represented using the wp-gpios property in the
Device Tree.

Unfortunately, the current sdhci_check_ro() function does not make use
of such GPIO when available: it either uses a host controller specific
->get_ro() operation, or uses the SDHCI_PRESENT_STATE. Several host
controller specific ->get_ro() functions are implemented just to check
a WP GPIO state.

Instead of pushing this to more controller-specific implementations,
let's handle this in the core SDHCI code, just like it is already done
for the CD GPIO in sdhci_get_cd().

The below patch simply changes sdhci_check_ro() to use the value of
the WP GPIO if available. We need to adjust the prototype of the
function to use a mmc_host* as argument instead of sdhci_host*, since
the mmc_can_gpio_ro() and mmc_gpio_get_ro() helpers take a mmc_host*.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
Changes since v1:

 - As suggested by Adrian Hunter, call the ->get_ro() if it exists
   before falling back to using mmc_gpio_get_ro(). Indeed, if the
   controller-specific code has implemented a ->get_ro() callback, it
   should take precedence over what the SDHCI core does.

   Due to this change, I have not added Thierry Redding Reviewed-by.

 - Fix typo in the commit log noticed by Thierry Redding.
---
 drivers/mmc/host/sdhci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index df05352b6a4a..e6682ea5f7c0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2022,8 +2022,9 @@ static int sdhci_get_cd(struct mmc_host *mmc)
 	return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
 }
 
-static int sdhci_check_ro(struct sdhci_host *host)
+static int sdhci_check_ro(struct mmc_host *mmc)
 {
+	struct sdhci_host *host = mmc_priv(mmc);
 	unsigned long flags;
 	int is_readonly;
 
@@ -2033,6 +2034,8 @@ static int sdhci_check_ro(struct sdhci_host *host)
 		is_readonly = 0;
 	else if (host->ops->get_ro)
 		is_readonly = host->ops->get_ro(host);
+	else if (mmc_can_gpio_ro(mmc))
+		is_readonly = mmc_gpio_get_ro(mmc);
 	else
 		is_readonly = !(sdhci_readl(host, SDHCI_PRESENT_STATE)
 				& SDHCI_WRITE_PROTECT);
@@ -2052,11 +2055,11 @@ static int sdhci_get_ro(struct mmc_host *mmc)
 	int i, ro_count;
 
 	if (!(host->quirks & SDHCI_QUIRK_UNSTABLE_RO_DETECT))
-		return sdhci_check_ro(host);
+		return sdhci_check_ro(mmc);
 
 	ro_count = 0;
 	for (i = 0; i < SAMPLE_COUNT; i++) {
-		if (sdhci_check_ro(host)) {
+		if (sdhci_check_ro(mmc)) {
 			if (++ro_count > SAMPLE_COUNT / 2)
 				return 1;
 		}
-- 
2.20.1

Re: [PATCH v2 1/3] mmc: sdhci: use WP GPIO in sdhci_check_ro()

[Adrian Hunter]

On 11/02/19 3:23 PM, Thomas Petazzoni wrote:
> Even though SDHCI controllers may have a dedicated WP pin that can be
> queried using the SDHCI_PRESENT_STATE register, some platforms may
> chose to use a separate regular GPIO to route the WP signal. Such a
> GPIO is typically represented using the wp-gpios property in the
> Device Tree.
> 
> Unfortunately, the current sdhci_check_ro() function does not make use
> of such GPIO when available: it either uses a host controller specific
> ->get_ro() operation, or uses the SDHCI_PRESENT_STATE. Several host
> controller specific ->get_ro() functions are implemented just to check
> a WP GPIO state.
> 
> Instead of pushing this to more controller-specific implementations,
> let's handle this in the core SDHCI code, just like it is already done
> for the CD GPIO in sdhci_get_cd().
> 
> The below patch simply changes sdhci_check_ro() to use the value of
> the WP GPIO if available. We need to adjust the prototype of the
> function to use a mmc_host* as argument instead of sdhci_host*, since
> the mmc_can_gpio_ro() and mmc_gpio_get_ro() helpers take a mmc_host*.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
> Changes since v1:
> 
>  - As suggested by Adrian Hunter, call the ->get_ro() if it exists
>    before falling back to using mmc_gpio_get_ro(). Indeed, if the
>    controller-specific code has implemented a ->get_ro() callback, it
>    should take precedence over what the SDHCI core does.
> 
>    Due to this change, I have not added Thierry Redding Reviewed-by.
> 
>  - Fix typo in the commit log noticed by Thierry Redding.
> ---
>  drivers/mmc/host/sdhci.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index df05352b6a4a..e6682ea5f7c0 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2022,8 +2022,9 @@ static int sdhci_get_cd(struct mmc_host *mmc)
>  	return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
>  }
>  
> -static int sdhci_check_ro(struct sdhci_host *host)
> +static int sdhci_check_ro(struct mmc_host *mmc)

Please let's not change the parameter.

>  {
> +	struct sdhci_host *host = mmc_priv(mmc);
>  	unsigned long flags;
>  	int is_readonly;
>  
> @@ -2033,6 +2034,8 @@ static int sdhci_check_ro(struct sdhci_host *host)
>  		is_readonly = 0;
>  	else if (host->ops->get_ro)
>  		is_readonly = host->ops->get_ro(host);
> +	else if (mmc_can_gpio_ro(mmc))
> +		is_readonly = mmc_gpio_get_ro(mmc);

Just make it 'host->mmc' instead of 'mmc'

>  	else
>  		is_readonly = !(sdhci_readl(host, SDHCI_PRESENT_STATE)
>  				& SDHCI_WRITE_PROTECT);
> @@ -2052,11 +2055,11 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>  	int i, ro_count;
>  
>  	if (!(host->quirks & SDHCI_QUIRK_UNSTABLE_RO_DETECT))
> -		return sdhci_check_ro(host);
> +		return sdhci_check_ro(mmc);
>  
>  	ro_count = 0;
>  	for (i = 0; i < SAMPLE_COUNT; i++) {
> -		if (sdhci_check_ro(host)) {
> +		if (sdhci_check_ro(mmc)) {
>  			if (++ro_count > SAMPLE_COUNT / 2)
>  				return 1;
>  		}
> 

[PATCH v2 2/3] mmc: sdhci-omap: drop ->get_ro() implementation

[Thomas Petazzoni]

The SDHCI core is now properly checking for the state of a WP GPIO,
so there is no longer any need for the sdhci-omap code to implement
->get_ro() using mmc_gpio_get_ro().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Reviewed-by: Thierry Reding <treding@nvidia.com>
---
Changes since v1:
- Added Reviewed-by from Thierry Reding
- Fix typo in commit log s/know/now/ noticed by Thierry Reding

Note: this patch has only been compiled tested, as I don't have the
hardware to test it.
---
 drivers/mmc/host/sdhci-omap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index d264391616f9..c2a28930086f 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -987,7 +987,6 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 		goto err_put_sync;
 	}
 
-	host->mmc_host_ops.get_ro = mmc_gpio_get_ro;
 	host->mmc_host_ops.start_signal_voltage_switch =
 					sdhci_omap_start_signal_voltage_switch;
 	host->mmc_host_ops.set_ios = sdhci_omap_set_ios;
-- 
2.20.1

Re: [PATCH v2 2/3] mmc: sdhci-omap: drop ->get_ro() implementation

[Adrian Hunter]

On 11/02/19 3:23 PM, Thomas Petazzoni wrote:
> The SDHCI core is now properly checking for the state of a WP GPIO,
> so there is no longer any need for the sdhci-omap code to implement
> ->get_ro() using mmc_gpio_get_ro().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Reviewed-by: Thierry Reding <treding@nvidia.com>

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

> ---
> Changes since v1:
> - Added Reviewed-by from Thierry Reding
> - Fix typo in commit log s/know/now/ noticed by Thierry Reding
> 
> Note: this patch has only been compiled tested, as I don't have the
> hardware to test it.
> ---
>  drivers/mmc/host/sdhci-omap.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index d264391616f9..c2a28930086f 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -987,7 +987,6 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>  		goto err_put_sync;
>  	}
>  
> -	host->mmc_host_ops.get_ro = mmc_gpio_get_ro;
>  	host->mmc_host_ops.start_signal_voltage_switch =
>  					sdhci_omap_start_signal_voltage_switch;
>  	host->mmc_host_ops.set_ios = sdhci_omap_set_ios;
> 

Re: [PATCH v2 2/3] mmc: sdhci-omap: drop ->get_ro() implementation

[Kishon Vijay Abraham I]

On 12/02/19 1:41 PM, Adrian Hunter wrote:
> On 11/02/19 3:23 PM, Thomas Petazzoni wrote:
>> The SDHCI core is now properly checking for the state of a WP GPIO,
>> so there is no longer any need for the sdhci-omap code to implement
>> ->get_ro() using mmc_gpio_get_ro().
>>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> 
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> 
>> ---
>> Changes since v1:
>> - Added Reviewed-by from Thierry Reding
>> - Fix typo in commit log s/know/now/ noticed by Thierry Reding
>>
>> Note: this patch has only been compiled tested, as I don't have the
>> hardware to test it.
>> ---
>>  drivers/mmc/host/sdhci-omap.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
>> index d264391616f9..c2a28930086f 100644
>> --- a/drivers/mmc/host/sdhci-omap.c
>> +++ b/drivers/mmc/host/sdhci-omap.c
>> @@ -987,7 +987,6 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>>  		goto err_put_sync;
>>  	}
>>  
>> -	host->mmc_host_ops.get_ro = mmc_gpio_get_ro;
>>  	host->mmc_host_ops.start_signal_voltage_switch =
>>  					sdhci_omap_start_signal_voltage_switch;
>>  	host->mmc_host_ops.set_ios = sdhci_omap_set_ios;
>>
> 

[PATCH v2 3/3] mmc: sdhci-tegra: drop ->get_ro() implementation

[Thomas Petazzoni]

The SDHCI core is know properly checking for the state of a WP GPIO,
so there is no longer any need for the sdhci-tegra code to implement
->get_ro() using mmc_gpio_get_ro().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Tested-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
Changes since v1:
 - Added Tested-by/Acked-by from Thierry Reding

Note: this patch has only been compiled tested, as I don't have the
hardware to test it.
---
 drivers/mmc/host/sdhci-tegra.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index e6ace31e2a41..6ed7323bf030 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -237,11 +237,6 @@ static void tegra210_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
 	}
 }
 
-static unsigned int tegra_sdhci_get_ro(struct sdhci_host *host)
-{
-	return mmc_gpio_get_ro(host->mmc);
-}
-
 static bool tegra_sdhci_is_pad_and_regulator_valid(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -837,7 +832,6 @@ static void tegra_sdhci_voltage_switch(struct sdhci_host *host)
 }
 
 static const struct sdhci_ops tegra_sdhci_ops = {
-	.get_ro     = tegra_sdhci_get_ro,
 	.read_w     = tegra_sdhci_readw,
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
@@ -893,7 +887,6 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
 };
 
 static const struct sdhci_ops tegra114_sdhci_ops = {
-	.get_ro     = tegra_sdhci_get_ro,
 	.read_w     = tegra_sdhci_readw,
 	.write_w    = tegra_sdhci_writew,
 	.write_l    = tegra_sdhci_writel,
@@ -947,7 +940,6 @@ static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
 };
 
 static const struct sdhci_ops tegra210_sdhci_ops = {
-	.get_ro     = tegra_sdhci_get_ro,
 	.read_w     = tegra_sdhci_readw,
 	.write_w    = tegra210_sdhci_writew,
 	.write_l    = tegra_sdhci_writel,
@@ -980,7 +972,6 @@ static const struct sdhci_tegra_soc_data soc_data_tegra210 = {
 };
 
 static const struct sdhci_ops tegra186_sdhci_ops = {
-	.get_ro     = tegra_sdhci_get_ro,
 	.read_w     = tegra_sdhci_readw,
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
-- 
2.20.1

Re: [PATCH v2 3/3] mmc: sdhci-tegra: drop ->get_ro() implementation

[Adrian Hunter]

On 11/02/19 3:23 PM, Thomas Petazzoni wrote:
> The SDHCI core is know properly checking for the state of a WP GPIO,
> so there is no longer any need for the sdhci-tegra code to implement
> ->get_ro() using mmc_gpio_get_ro().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Tested-by: Thierry Reding <treding@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>

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

> ---
> Changes since v1:
>  - Added Tested-by/Acked-by from Thierry Reding
> 
> Note: this patch has only been compiled tested, as I don't have the
> hardware to test it.
> ---
>  drivers/mmc/host/sdhci-tegra.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index e6ace31e2a41..6ed7323bf030 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -237,11 +237,6 @@ static void tegra210_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
>  	}
>  }
>  
> -static unsigned int tegra_sdhci_get_ro(struct sdhci_host *host)
> -{
> -	return mmc_gpio_get_ro(host->mmc);
> -}
> -
>  static bool tegra_sdhci_is_pad_and_regulator_valid(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -837,7 +832,6 @@ static void tegra_sdhci_voltage_switch(struct sdhci_host *host)
>  }
>  
>  static const struct sdhci_ops tegra_sdhci_ops = {
> -	.get_ro     = tegra_sdhci_get_ro,
>  	.read_w     = tegra_sdhci_readw,
>  	.write_l    = tegra_sdhci_writel,
>  	.set_clock  = tegra_sdhci_set_clock,
> @@ -893,7 +887,6 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
>  };
>  
>  static const struct sdhci_ops tegra114_sdhci_ops = {
> -	.get_ro     = tegra_sdhci_get_ro,
>  	.read_w     = tegra_sdhci_readw,
>  	.write_w    = tegra_sdhci_writew,
>  	.write_l    = tegra_sdhci_writel,
> @@ -947,7 +940,6 @@ static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
>  };
>  
>  static const struct sdhci_ops tegra210_sdhci_ops = {
> -	.get_ro     = tegra_sdhci_get_ro,
>  	.read_w     = tegra_sdhci_readw,
>  	.write_w    = tegra210_sdhci_writew,
>  	.write_l    = tegra_sdhci_writel,
> @@ -980,7 +972,6 @@ static const struct sdhci_tegra_soc_data soc_data_tegra210 = {
>  };
>  
>  static const struct sdhci_ops tegra186_sdhci_ops = {
> -	.get_ro     = tegra_sdhci_get_ro,
>  	.read_w     = tegra_sdhci_readw,
>  	.write_l    = tegra_sdhci_writel,
>  	.set_clock  = tegra_sdhci_set_clock,
>