[PATCH 0/2] mmc: sh_mobile_sdhi: use SDR & card_busy only on Gen2+

[PATCH 0/2] mmc: sh_mobile_sdhi: use SDR & card_busy only on Gen2+

[Wolfram Sang]

This series hopefully fixes a performance regression for r8a73a4 (pre Gen2)
caused by a not working card_busy. Because we don't have documentation for its
SDHI (and I don't have the hardware), just enable card_busy and SDR only for
machines known to have hardware support for it.

Geert, can you test, please?

Wolfram Sang (2):
  mmc: host: sh_mobile_sdhi: move card_busy from tmio to sdhi
  mmc: host: sh_mobile_sdhi: don't populate unneeded functions

 drivers/mmc/host/sh_mobile_sdhi.c | 16 +++++++++++++++-
 drivers/mmc/host/tmio_mmc.h       |  1 +
 drivers/mmc/host/tmio_mmc_pio.c   |  9 +--------
 3 files changed, 17 insertions(+), 9 deletions(-)

-- 
2.9.3

[PATCH 1/2] mmc: host: sh_mobile_sdhi: move card_busy from tmio to sdhi

[Wolfram Sang]

card_busy is only used/tested on SDHI for R-Car Gen2 and later.
Move it to the SDHI driver, so we can then activate it conditionally
depending on the SDHI type.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 8 ++++++++
 drivers/mmc/host/tmio_mmc.h       | 1 +
 drivers/mmc/host/tmio_mmc_pio.c   | 9 +--------
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index c3b651bf89cb4a..c4e63b7790d719 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -213,6 +213,13 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 	clk_disable_unprepare(priv->clk);
 }
 
+static int sh_mobile_sdhi_card_busy(struct mmc_host *mmc)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+
+	return !(sd_ctrl_read16_and_16_as_32(host, CTL_STATUS) & TMIO_STAT_DAT0);
+}
+
 static int sh_mobile_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
 						      struct mmc_ios *ios)
 {
@@ -369,6 +376,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	host->clk_update	= sh_mobile_sdhi_clk_update;
 	host->clk_disable	= sh_mobile_sdhi_clk_disable;
 	host->multi_io_quirk	= sh_mobile_sdhi_multi_io_quirk;
+	host->card_busy	= sh_mobile_sdhi_card_busy;
 	host->start_signal_voltage_switch = sh_mobile_sdhi_start_signal_voltage_switch;
 
 	/* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 7f63ec05bdf41d..1f1cb2628f673b 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -158,6 +158,7 @@ struct tmio_mmc_host {
 	void (*clk_disable)(struct tmio_mmc_host *host);
 	int (*multi_io_quirk)(struct mmc_card *card,
 			      unsigned int direction, int blk_size);
+	int (*card_busy)(struct mmc_host *mmc);
 	int (*start_signal_voltage_switch)(struct mmc_host *mmc,
 					   struct mmc_ios *ios);
 };
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 92467efc4e2c9e..192817850dffac 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -960,20 +960,12 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
 	return blk_size;
 }
 
-static int tmio_mmc_card_busy(struct mmc_host *mmc)
-{
-	struct tmio_mmc_host *host = mmc_priv(mmc);
-
-	return !(sd_ctrl_read16_and_16_as_32(host, CTL_STATUS) & TMIO_STAT_DAT0);
-}
-
 static struct mmc_host_ops tmio_mmc_ops = {
 	.request	= tmio_mmc_request,
 	.set_ios	= tmio_mmc_set_ios,
 	.get_ro         = tmio_mmc_get_ro,
 	.get_cd		= mmc_gpio_get_cd,
 	.enable_sdio_irq = tmio_mmc_enable_sdio_irq,
-	.card_busy	= tmio_mmc_card_busy,
 	.multi_io_quirk	= tmio_multi_io_quirk,
 };
 
@@ -1072,6 +1064,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 		goto host_free;
 	}
 
+	tmio_mmc_ops.card_busy = _host->card_busy;
 	tmio_mmc_ops.start_signal_voltage_switch = _host->start_signal_voltage_switch;
 	mmc->ops = &tmio_mmc_ops;
 
-- 
2.9.3

[PATCH 2/2] mmc: host: sh_mobile_sdhi: don't populate unneeded functions

[Wolfram Sang]

Populating card_busy caused a side-effect on a chip variant we don't
have documentation for (r8a73a4). So, enable it and voltage switching
only on devices known to support those features.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index c4e63b7790d719..d679c8a533b6e5 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -376,8 +376,14 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	host->clk_update	= sh_mobile_sdhi_clk_update;
 	host->clk_disable	= sh_mobile_sdhi_clk_disable;
 	host->multi_io_quirk	= sh_mobile_sdhi_multi_io_quirk;
-	host->card_busy	= sh_mobile_sdhi_card_busy;
-	host->start_signal_voltage_switch = sh_mobile_sdhi_start_signal_voltage_switch;
+
+	/* SDR speeds are only available on Gen2+ */
+	if (mmc_data->flags & TMIO_MMC_MIN_RCAR2) {
+		/* card_busy caused issues on r8a73a4 (pre-Gen2) CD-less SDHI */
+		host->card_busy	= sh_mobile_sdhi_card_busy;
+		host->start_signal_voltage_switch =
+			sh_mobile_sdhi_start_signal_voltage_switch;
+	}
 
 	/* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */
 	if (!host->bus_shift && resource_size(res) > 0x100) /* old way to determine the shift */
-- 
2.9.3

Re: [PATCH 2/2] mmc: host: sh_mobile_sdhi: don't populate unneeded functions

[Magnus Damm]

On Wed, Aug 24, 2016 at 6:34 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Populating card_busy caused a side-effect on a chip variant we don't
> have documentation for (r8a73a4). So, enable it and voltage switching
> only on devices known to support those features.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/host/sh_mobile_sdhi.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index c4e63b7790d719..d679c8a533b6e5 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -376,8 +376,14 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>         host->clk_update        = sh_mobile_sdhi_clk_update;
>         host->clk_disable       = sh_mobile_sdhi_clk_disable;
>         host->multi_io_quirk    = sh_mobile_sdhi_multi_io_quirk;
> -       host->card_busy = sh_mobile_sdhi_card_busy;
> -       host->start_signal_voltage_switch = sh_mobile_sdhi_start_signal_voltage_switch;
> +
> +       /* SDR speeds are only available on Gen2+ */
> +       if (mmc_data->flags & TMIO_MMC_MIN_RCAR2) {
> +               /* card_busy caused issues on r8a73a4 (pre-Gen2) CD-less SDHI */
> +               host->card_busy = sh_mobile_sdhi_card_busy;
> +               host->start_signal_voltage_switch =
> +                       sh_mobile_sdhi_start_signal_voltage_switch;
> +       }
>
>         /* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */
>         if (!host->bus_shift && resource_size(res) > 0x100) /* old way to determine the shift */
> --
> 2.9.3
>

Hi Wolfram,

Thanks for your efforts improving the code. The patch title says
something about unneeded functions, and then comments in the code
mentions SDR which makes me interested to dig a bit deeper. I know
that the SDHI hardware is supported on a wide range of hardware so I
wonder how you handle all the callbacks for such a wide range of
hardware with varying level of documentation.

Many years ago when I initially prototyped the SDHI driver we had no
access to any documentation - only the reverse engineered TMIO driver
and the register definitions in that driver existed. I remember being
very happy being able to remove some firmware binary used for register
access and fully rely on open source to access the hardware. These
days I believe developers have access to documentation for at least
the most recent hardware, however older SDHI hardware may still be
undocumented.

To my eye it looks like this patch might be adding a fix for a bug
introduced by another patch. R-Car Gen2 and later are quite recent
SoCs but I believe support for other mobile chips and earlier R-Car
SoCs also also covered by the SDHI driver. Also there are theTMIO
chips that share some software and are related but a bit different. So
in my opinion we need to thread lightly to avoid breakage.

I'm currently looking at this commit included in
renesas-drivers-2016-08-30-v4.8-rc4:
d11b9b7 mmc: host: sh_mobile_sdhi: don't populate unneeded functions

The code adds a feature flag check and in case of more recent hardware
the following callbacks are installed:
host->card_busy()
host->start_signal_voltage_switch()
Also the comment "/* SDR speeds are only available on Gen2+ */" was added.

My concern is what happened before this patch was applied. It looks
like the callbacks were installed for all hardware types which makes
me wonder because UHS/SDR support is only available for quite recent
hardware.

The ->card_busy() callback is not yet in mainline or -next. It was
introduced in:
0157290 mmc: host: sh_mobile_sdhi: move card_busy from tmio to sdhi

However the ->start_signal_voltage_switch() is included in mainline.
It was introduced in:
057a459 mmc: sh_mobile_sdhi: Add UHS-I mode support

If this patch is fixing a recent commit then perhaps some patches
should be squashed together to prevent bisection breakage or if a
patch is already part of mainline then a "Fixes:" tag might be
suitable.

Thanks,

/ magnus

Re: [PATCH 2/2] mmc: host: sh_mobile_sdhi: don't populate unneeded functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

Magnus,

> To my eye it looks like this patch might be adding a fix for a bug
> introduced by another patch. R-Car Gen2 and later are quite recent
> SoCs but I believe support for other mobile chips and earlier R-Car
> SoCs also also covered by the SDHI driver. Also there are theTMIO
> chips that share some software and are related but a bit different. So
> in my opinion we need to thread lightly to avoid breakage.

I'm very much with you. This is the very much reason I introduced
TMIO_MMC_MIN_RCAR2 in 3d376fb2ea907f ("mmc: tmio/sdhi: introduce flag
for RCar 2+ specific features") in the first place to be able to
"protect" old hardware from new features. It was not done before! This
is also the reason why we have commits like a21553c9e0c236 ("mmc:
tmio/sdhi: distinguish between SCLKDIVEN and ILL_FUNC") documenting a
difference between some old TMIO variant and our current SDHI. I spent
quite some time finding old TMIO information somewhere for that.

> My concern is what happened before this patch was applied. It looks
> like the callbacks were installed for all hardware types which makes
> me wonder because UHS/SDR support is only available for quite recent
> hardware.

I didn't protect these callbacks before because I assumed they are only
called when SDR support is enabled via devicetree or platform data.
Which is not the case for all the old platforms. I sadly missed that
card_busy() is used when polling an SDIO card in case "broken-cd" is
used. That was a detail I overlooked, sorry. Given my work outlined
above I don't think one can deduce that I don't care about regressing
old hardware, though.

> The ->card_busy() callback is not yet in mainline or -next. It was
> introduced in:
> 0157290 mmc: host: sh_mobile_sdhi: move card_busy from tmio to sdhi

Not quite, it was introduced with 452e5eef6d311e ("mmc: tmio: Add UHS-I
mode support"). The commit you mentioned moved it from tmio*.c to
sdhi*.c

> If this patch is fixing a recent commit then perhaps some patches
> should be squashed together to prevent bisection breakage or if a
> patch is already part of mainline then a "Fixes:" tag might be
> suitable.

It can be argued that this tag could be added:

Fixes: 452e5eef6d311e ("mmc: tmio: Add UHS-I mode support")

I don't know how well it applies, though, because the code has been
modified a lot recently. But we can try.

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] mmc: host: sh_mobile_sdhi: don't populate unneeded functions

[Ulf Hansson]

On 2 September 2016 at 07:23, Wolfram Sang <wsa@the-dreams.de> wrote:
> Magnus,
>
>> To my eye it looks like this patch might be adding a fix for a bug
>> introduced by another patch. R-Car Gen2 and later are quite recent
>> SoCs but I believe support for other mobile chips and earlier R-Car
>> SoCs also also covered by the SDHI driver. Also there are theTMIO
>> chips that share some software and are related but a bit different. So
>> in my opinion we need to thread lightly to avoid breakage.
>
> I'm very much with you. This is the very much reason I introduced
> TMIO_MMC_MIN_RCAR2 in 3d376fb2ea907f ("mmc: tmio/sdhi: introduce flag
> for RCar 2+ specific features") in the first place to be able to
> "protect" old hardware from new features. It was not done before! This
> is also the reason why we have commits like a21553c9e0c236 ("mmc:
> tmio/sdhi: distinguish between SCLKDIVEN and ILL_FUNC") documenting a
> difference between some old TMIO variant and our current SDHI. I spent
> quite some time finding old TMIO information somewhere for that.
>
>> My concern is what happened before this patch was applied. It looks
>> like the callbacks were installed for all hardware types which makes
>> me wonder because UHS/SDR support is only available for quite recent
>> hardware.
>
> I didn't protect these callbacks before because I assumed they are only
> called when SDR support is enabled via devicetree or platform data.
> Which is not the case for all the old platforms. I sadly missed that
> card_busy() is used when polling an SDIO card in case "broken-cd" is
> used. That was a detail I overlooked, sorry. Given my work outlined
> above I don't think one can deduce that I don't care about regressing
> old hardware, though.
>
>> The ->card_busy() callback is not yet in mainline or -next. It was
>> introduced in:
>> 0157290 mmc: host: sh_mobile_sdhi: move card_busy from tmio to sdhi
>
> Not quite, it was introduced with 452e5eef6d311e ("mmc: tmio: Add UHS-I
> mode support"). The commit you mentioned moved it from tmio*.c to
> sdhi*.c
>
>> If this patch is fixing a recent commit then perhaps some patches
>> should be squashed together to prevent bisection breakage or if a
>> patch is already part of mainline then a "Fixes:" tag might be
>> suitable.
>
> It can be argued that this tag could be added:
>
> Fixes: 452e5eef6d311e ("mmc: tmio: Add UHS-I mode support")
>
> I don't know how well it applies, though, because the code has been
> modified a lot recently. But we can try.

Please tell me if you would like me to drop any of the changes I
queued up in this series.

Of course, I can also amend the change log, just tell me.

Kind regards
Uffe

Re: [PATCH 2/2] mmc: host: sh_mobile_sdhi: don't populate unneeded functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]


> > It can be argued that this tag could be added:
> >
> > Fixes: 452e5eef6d311e ("mmc: tmio: Add UHS-I mode support")
> >
> > I don't know how well it applies, though, because the code has been
> > modified a lot recently. But we can try.
> 
> Please tell me if you would like me to drop any of the changes I
> queued up in this series.
> 
> Of course, I can also amend the change log, just tell me.

If you could add the Fixes: tag, this would be welcome. Thanks, Ulf!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] mmc: host: sh_mobile_sdhi: don't populate unneeded functions

[Ulf Hansson]

On 5 September 2016 at 10:18, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > It can be argued that this tag could be added:
>> >
>> > Fixes: 452e5eef6d311e ("mmc: tmio: Add UHS-I mode support")
>> >
>> > I don't know how well it applies, though, because the code has been
>> > modified a lot recently. But we can try.
>>
>> Please tell me if you would like me to drop any of the changes I
>> queued up in this series.
>>
>> Of course, I can also amend the change log, just tell me.
>
> If you could add the Fixes: tag, this would be welcome. Thanks, Ulf!
>

Thanks, done!

Kind regards
Uffe

Re: [PATCH 0/2] mmc: sh_mobile_sdhi: use SDR & card_busy only on Gen2+

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Aug 24, 2016 at 11:34 AM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> This series hopefully fixes a performance regression for r8a73a4 (pre Gen2)
> caused by a not working card_busy. Because we don't have documentation for its
> SDHI (and I don't have the hardware), just enable card_busy and SDR only for
> machines known to have hardware support for it.

Thanks, ape6evm is much more rensponsive with your 2 patches applied.

> Geert, can you test, please?

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/2] mmc: sh_mobile_sdhi: use SDR & card_busy only on Gen2+

[Ulf Hansson]

On 24 August 2016 at 11:34, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> This series hopefully fixes a performance regression for r8a73a4 (pre Gen2)
> caused by a not working card_busy. Because we don't have documentation for its
> SDHI (and I don't have the hardware), just enable card_busy and SDR only for
> machines known to have hardware support for it.
>
> Geert, can you test, please?
>
> Wolfram Sang (2):
>   mmc: host: sh_mobile_sdhi: move card_busy from tmio to sdhi
>   mmc: host: sh_mobile_sdhi: don't populate unneeded functions
>
>  drivers/mmc/host/sh_mobile_sdhi.c | 16 +++++++++++++++-
>  drivers/mmc/host/tmio_mmc.h       |  1 +
>  drivers/mmc/host/tmio_mmc_pio.c   |  9 +--------
>  3 files changed, 17 insertions(+), 9 deletions(-)
>

Thanks, applied for next!

Kind regards
Uffe