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.
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
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 use
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 <[email protected]>
---
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..63cc4bd033b9 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;
@@ -2031,6 +2032,8 @@ static int sdhci_check_ro(struct sdhci_host *host)
if (host->flags & SDHCI_DEVICE_DEAD)
is_readonly = 0;
+ else if (mmc_can_gpio_ro(mmc))
+ is_readonly = mmc_gpio_get_ro(mmc);
else if (host->ops->get_ro)
is_readonly = host->ops->get_ro(host);
else
@@ -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
The SDHCI core is know 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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, Jan 15, 2019 at 05:28:35PM +0100, 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 use
> check a WP GPIO state.
"use check" was probably meant to be just "check"?
>
> 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 <[email protected]>
> ---
> drivers/mmc/host/sdhci.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
Other than the typo in the commit message, this looks fine:
Reviewed-by: Thierry Reding <[email protected]>
On 15/01/19 6:28 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 use
> 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*.
Why not just use host->mmc
>
> Signed-off-by: Thomas Petazzoni <[email protected]>
> ---
> 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..63cc4bd033b9 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;
>
> @@ -2031,6 +2032,8 @@ static int sdhci_check_ro(struct sdhci_host *host)
>
> if (host->flags & SDHCI_DEVICE_DEAD)
> is_readonly = 0;
> + else if (mmc_can_gpio_ro(mmc))
> + is_readonly = mmc_gpio_get_ro(mmc);
Perhaps host->ops->get_ro should be checked before mmc_can_gpio_ro()?
> else if (host->ops->get_ro)
> is_readonly = host->ops->get_ro(host);
> else
> @@ -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;
> }
>
Hello Adrian,
On Wed, 16 Jan 2019 14:59:32 +0200, Adrian Hunter wrote:
> > 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*.
>
> Why not just use host->mmc
Could do that. I just found it weird that the calling function has the
mmc_host structure, does some gymnastic to find sdhci_host, and then in
the called function, we do the opposite gymnastic to find mmc_host from
sdhci_host. But if that's the preference, I'm happy to change the patch
accordingly.
> > if (host->flags & SDHCI_DEVICE_DEAD)
> > is_readonly = 0;
> > + else if (mmc_can_gpio_ro(mmc))
> > + is_readonly = mmc_gpio_get_ro(mmc);
>
> Perhaps host->ops->get_ro should be checked before mmc_can_gpio_ro()?
That is actually a good point, using ->get_ro() should come before
using the GPIO. Indeed, some drivers may potentially have a ->get_ro
with custom logic *and* a WP GPIO, and in this case, we want ->get_ro
to take precedence. I'll send a v2 with this, once you let me know your
decision about the previous point.
Thanks,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Jan 15, 2019 at 05:28:36PM +0100, Thomas Petazzoni wrote:
> The SDHCI core is know properly checking for the state of a WP GPIO,
"know" -> "now"
> 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 <[email protected]>
> ---
> 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(-)
I can't runtime test this myself, but it certainly looks like this would
do the right thing because it calls the core now call the very same
function as this used to, so:
Reviewed-by: Thierry Reding <[email protected]>
Adding Tony and linux-omap for a slightly wider audience and hopefully
find someone to test this on. For reference, the whole series is here:
https://patchwork.kernel.org/project/linux-mmc/list/?series=66905
Thierry
>
> 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
>
* Thierry Reding <[email protected]> [190116 13:28]:
> On Tue, Jan 15, 2019 at 05:28:36PM +0100, Thomas Petazzoni wrote:
> > The SDHCI core is know properly checking for the state of a WP GPIO,
>
> "know" -> "now"
>
> > 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 <[email protected]>
> > ---
> > 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(-)
>
> I can't runtime test this myself, but it certainly looks like this would
> do the right thing because it calls the core now call the very same
> function as this used to, so:
>
> Reviewed-by: Thierry Reding <[email protected]>
>
> Adding Tony and linux-omap for a slightly wider audience and hopefully
> find someone to test this on. For reference, the whole series is here:
>
> https://patchwork.kernel.org/project/linux-mmc/list/?series=66905
Thanks Kishon can you please review and test?
Regards,
Tony
> > 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
> >
Hello,
On Wed, 16 Jan 2019 07:47:18 -0800, Tony Lindgren wrote:
> > Reviewed-by: Thierry Reding <[email protected]>
> >
> > Adding Tony and linux-omap for a slightly wider audience and hopefully
> > find someone to test this on. For reference, the whole series is here:
> >
> > https://patchwork.kernel.org/project/linux-mmc/list/?series=66905
>
> Thanks Kishon can you please review and test?
I think Adrian's comment that ->get_ro() should be called first in the
core code makes sense, so I'll send a v2 with this change. Perhaps
testing should be done on the v2 instead, even though I believe it
should be functionally equivalent.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Jan 15, 2019 at 05:28:37PM +0100, 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 <[email protected]>
> ---
> 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(-)
So, if I apply this on top of next-20180116 I do get a BUG from
bio_split() on line 1833 in block/bio.c. However, that BUG also exists
without these patches.
More importantly, read-only detection with this applied still works on
for example Jetson TK1:
[ 28.547261] mmc1: new high speed SDHC card at address 59b4
[ 28.558814] mmcblk1: mmc1:59b4 SDC 14.7 GiB (ro)
So:
Tested-by: Thierry Reding <[email protected]>
and:
Acked-by: Thierry Reding <[email protected]>
Tony,
On 16/01/19 9:17 PM, Tony Lindgren wrote:
> * Thierry Reding <[email protected]> [190116 13:28]:
>> On Tue, Jan 15, 2019 at 05:28:36PM +0100, Thomas Petazzoni wrote:
>>> The SDHCI core is know properly checking for the state of a WP GPIO,
>>
>> "know" -> "now"
>>
>>> 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 <[email protected]>
>>> ---
>>> 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(-)
>>
>> I can't runtime test this myself, but it certainly looks like this would
>> do the right thing because it calls the core now call the very same
>> function as this used to, so:
>>
>> Reviewed-by: Thierry Reding <[email protected]>
>>
>> Adding Tony and linux-omap for a slightly wider audience and hopefully
>> find someone to test this on. For reference, the whole series is here:
>>
>> https://patchwork.kernel.org/project/linux-mmc/list/?series=66905
>
> Thanks Kishon can you please review and test?
>
I can help with testing once Thomas posts a v2.
Thanks,
Faiz
* Faiz Abbas <[email protected]> [190118 10:54]:
> Tony,
>
> On 16/01/19 9:17 PM, Tony Lindgren wrote:
> > * Thierry Reding <[email protected]> [190116 13:28]:
> >> On Tue, Jan 15, 2019 at 05:28:36PM +0100, Thomas Petazzoni wrote:
> >>> The SDHCI core is know properly checking for the state of a WP GPIO,
> >>
> >> "know" -> "now"
> >>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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(-)
> >>
> >> I can't runtime test this myself, but it certainly looks like this would
> >> do the right thing because it calls the core now call the very same
> >> function as this used to, so:
> >>
> >> Reviewed-by: Thierry Reding <[email protected]>
> >>
> >> Adding Tony and linux-omap for a slightly wider audience and hopefully
> >> find someone to test this on. For reference, the whole series is here:
> >>
> >> https://patchwork.kernel.org/project/linux-mmc/list/?series=66905
> >
> > Thanks Kishon can you please review and test?
> >
>
> I can help with testing once Thomas posts a v2.
OK great thanks!
Tony