[PATCH] tmio_mmc_pio: test TMIO_MMC_WRPROTECT_DISABLE earlier

[PATCH] tmio_mmc_pio: test TMIO_MMC_WRPROTECT_DISABLE earlier

[Sergei Shtylyov]

There seems  to be no sense in the runtime PM calls when the actual register
read is suppressed by the TMIO_MMC_WRPROTECT_DISABLE flag.  Check that flag
before trying to read the register and  thus doing the runtime PM dance...

While at it, kill useless local variable and add empty line after declarations.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
The patch is against Ulf Hansson's 'mmc.git' repo's 'next' branch.

 drivers/mmc/host/tmio_mmc_pio.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: mmc/drivers/mmc/host/tmio_mmc_pio.c
===================================================================
--- mmc.orig/drivers/mmc/host/tmio_mmc_pio.c
+++ mmc/drivers/mmc/host/tmio_mmc_pio.c
@@ -988,14 +988,16 @@ static void tmio_mmc_set_ios(struct mmc_
 static int tmio_mmc_get_ro(struct mmc_host *mmc)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
-	struct tmio_mmc_data *pdata = host->pdata;
 	int ret = mmc_gpio_get_ro(mmc);
+
 	if (ret >= 0)
 		return ret;
 
+	if (host->pdata->flags & TMIO_MMC_WRPROTECT_DISABLE)
+		return 0;
+
 	pm_runtime_get_sync(mmc_dev(mmc));
-	ret = !((pdata->flags & TMIO_MMC_WRPROTECT_DISABLE) ||
-		(sd_ctrl_read32(host, CTL_STATUS) & TMIO_STAT_WRPROTECT));
+	ret = !(sd_ctrl_read32(host, CTL_STATUS) & TMIO_STAT_WRPROTECT);
 	pm_runtime_mark_last_busy(mmc_dev(mmc));
 	pm_runtime_put_autosuspend(mmc_dev(mmc));
 

Re: [PATCH] tmio_mmc_pio: test TMIO_MMC_WRPROTECT_DISABLE earlier

[Geert Uytterhoeven]

On Thu, Sep 24, 2015 at 1:58 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> There seems  to be no sense in the runtime PM calls when the actual register
> read is suppressed by the TMIO_MMC_WRPROTECT_DISABLE flag.  Check that flag
> before trying to read the register and  thus doing the runtime PM dance...
>
> While at it, kill useless local variable and add empty line after declarations.
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Acked-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] tmio_mmc_pio: test TMIO_MMC_WRPROTECT_DISABLE earlier

[Ulf Hansson]

On 24 September 2015 at 01:58, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> There seems  to be no sense in the runtime PM calls when the actual register
> read is suppressed by the TMIO_MMC_WRPROTECT_DISABLE flag.  Check that flag
> before trying to read the register and  thus doing the runtime PM dance...
>
> While at it, kill useless local variable and add empty line after declarations.
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> The patch is against Ulf Hansson's 'mmc.git' repo's 'next' branch.
>
>  drivers/mmc/host/tmio_mmc_pio.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> Index: mmc/drivers/mmc/host/tmio_mmc_pio.c
> ===================================================================
> --- mmc.orig/drivers/mmc/host/tmio_mmc_pio.c
> +++ mmc/drivers/mmc/host/tmio_mmc_pio.c
> @@ -988,14 +988,16 @@ static void tmio_mmc_set_ios(struct mmc_
>  static int tmio_mmc_get_ro(struct mmc_host *mmc)
>  {
>         struct tmio_mmc_host *host = mmc_priv(mmc);
> -       struct tmio_mmc_data *pdata = host->pdata;
>         int ret = mmc_gpio_get_ro(mmc);
> +
>         if (ret >= 0)
>                 return ret;
>
> +       if (host->pdata->flags & TMIO_MMC_WRPROTECT_DISABLE)
> +               return 0;
> +
>         pm_runtime_get_sync(mmc_dev(mmc));
> -       ret = !((pdata->flags & TMIO_MMC_WRPROTECT_DISABLE) ||
> -               (sd_ctrl_read32(host, CTL_STATUS) & TMIO_STAT_WRPROTECT));
> +       ret = !(sd_ctrl_read32(host, CTL_STATUS) & TMIO_STAT_WRPROTECT);
>         pm_runtime_mark_last_busy(mmc_dev(mmc));
>         pm_runtime_put_autosuspend(mmc_dev(mmc));
>

Actually this change won't have the desired effect, as the mmc core
already done a pm_runtime_get_sync() since it has claimed the mmc
host[1].

I do realize that most drivers are still maintaining the
pm_runtime_get|put() calls, but in most cases that's not needed any
more.

[1]
commit 9250aea76bfc ("mmc: core: Enable runtime PM management of host devices")

Kind regards
Uffe

Re: [PATCH] tmio_mmc_pio: test TMIO_MMC_WRPROTECT_DISABLE earlier

[Sergei Shtylyov]

Hello.

On 09/25/2015 11:24 PM, Ulf Hansson wrote:

>> There seems  to be no sense in the runtime PM calls when the actual register
>> read is suppressed by the TMIO_MMC_WRPROTECT_DISABLE flag.  Check that flag
>> before trying to read the register and  thus doing the runtime PM dance...
>>
>> While at it, kill useless local variable and add empty line after declarations.
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> The patch is against Ulf Hansson's 'mmc.git' repo's 'next' branch.
>>
>>   drivers/mmc/host/tmio_mmc_pio.c |    8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> Index: mmc/drivers/mmc/host/tmio_mmc_pio.c
>> ===================================================================
>> --- mmc.orig/drivers/mmc/host/tmio_mmc_pio.c
>> +++ mmc/drivers/mmc/host/tmio_mmc_pio.c
>> @@ -988,14 +988,16 @@ static void tmio_mmc_set_ios(struct mmc_
>>   static int tmio_mmc_get_ro(struct mmc_host *mmc)
>>   {
>>          struct tmio_mmc_host *host = mmc_priv(mmc);
>> -       struct tmio_mmc_data *pdata = host->pdata;
>>          int ret = mmc_gpio_get_ro(mmc);
>> +
>>          if (ret >= 0)
>>                  return ret;
>>
>> +       if (host->pdata->flags & TMIO_MMC_WRPROTECT_DISABLE)
>> +               return 0;
>> +
>>          pm_runtime_get_sync(mmc_dev(mmc));
>> -       ret = !((pdata->flags & TMIO_MMC_WRPROTECT_DISABLE) ||
>> -               (sd_ctrl_read32(host, CTL_STATUS) & TMIO_STAT_WRPROTECT));
>> +       ret = !(sd_ctrl_read32(host, CTL_STATUS) & TMIO_STAT_WRPROTECT);
>>          pm_runtime_mark_last_busy(mmc_dev(mmc));
>>          pm_runtime_put_autosuspend(mmc_dev(mmc));
>>
>
> Actually this change won't have the desired effect, as the mmc core
> already done a pm_runtime_get_sync() since it has claimed the mmc
> host[1].

> I do realize that most drivers are still maintaining the
> pm_runtime_get|put() calls, but in most cases that's not needed any
> more.

    Hm, OK. Should be OK to remove the RPM dances from at least this 
particular driver, right?

> [1]
> commit 9250aea76bfc ("mmc: core: Enable runtime PM management of host devices")

    Looking at the code, the following fragment of mmc_attach_sd() doesn't 
make much sense to me:

         mmc_release_host(host);
         err = mmc_add_card(host->card);
         mmc_claim_host(host);
         if (err)
                 goto remove_card;

         return 0;

remove_card:
         mmc_release_host(host);
         mmc_remove_card(host->card);
         host->card = NULL;
         mmc_claim_host(host);

    Why claim the host and immediately release it on mmc_add_card() error? Can 
we only claim on success and save a call here?

> Kind regards
> Uffe

MBR, Sergei

Re: [PATCH] tmio_mmc_pio: test TMIO_MMC_WRPROTECT_DISABLE earlier

[Ulf Hansson]

[...]

>>
>> Actually this change won't have the desired effect, as the mmc core
>> already done a pm_runtime_get_sync() since it has claimed the mmc
>> host[1].
>
>
>> I do realize that most drivers are still maintaining the
>> pm_runtime_get|put() calls, but in most cases that's not needed any
>> more.
>
>
>    Hm, OK. Should be OK to remove the RPM dances from at least this
> particular driver, right?

In most cases, yes.

There may be corner cases where the host is accessed without having
the mmc core involved (and thus the host hasn't been claimed).

>
>> [1]
>> commit 9250aea76bfc ("mmc: core: Enable runtime PM management of host
>> devices")
>
>
>    Looking at the code, the following fragment of mmc_attach_sd() doesn't
> make much sense to me:
>
>         mmc_release_host(host);
>         err = mmc_add_card(host->card);
>         mmc_claim_host(host);
>         if (err)
>                 goto remove_card;
>
>         return 0;
>
> remove_card:
>         mmc_release_host(host);
>         mmc_remove_card(host->card);
>         host->card = NULL;
>         mmc_claim_host(host);
>
>    Why claim the host and immediately release it on mmc_add_card() error?
> Can we only claim on success and save a call here?

You are right, we can simplify the sequence!

Do you want to send a patch? Actually, it's similar for mmc_attach_mmc()...

Kind regards
Uffe

Re: [PATCH] tmio_mmc_pio: test TMIO_MMC_WRPROTECT_DISABLE earlier

[Sergei Shtylyov]

Hello.

On 09/29/2015 01:22 PM, Ulf Hansson wrote:

> [...]

>>> Actually this change won't have the desired effect, as the mmc core
>>> already done a pm_runtime_get_sync() since it has claimed the mmc
>>> host[1].
>>
>>> I do realize that most drivers are still maintaining the
>>> pm_runtime_get|put() calls, but in most cases that's not needed any
>>> more.
>>
>>     Hm, OK. Should be OK to remove the RPM dances from at least this
>> particular driver, right?
>
> In most cases, yes.
>
> There may be corner cases where the host is accessed without having
> the mmc core involved (and thus the host hasn't been claimed).

    OK, I'll try to look into this further...

>>> [1]
>>> commit 9250aea76bfc ("mmc: core: Enable runtime PM management of host
>>> devices")
>>
>>     Looking at the code, the following fragment of mmc_attach_sd() doesn't
>> make much sense to me:
>>
>>          mmc_release_host(host);
>>          err = mmc_add_card(host->card);
>>          mmc_claim_host(host);
>>          if (err)
>>                  goto remove_card;
>>
>>          return 0;
>>
>> remove_card:
>>          mmc_release_host(host);
>>          mmc_remove_card(host->card);
>>          host->card = NULL;
>>          mmc_claim_host(host);
>>
>>     Why claim the host and immediately release it on mmc_add_card() error?
>> Can we only claim on success and save a call here?

> You are right, we can simplify the sequence!

    OK, what about calling mmc_remove_card() on mmc_add_card() failure?
Isn't it also superfluous?

> Do you want to send a patch?

    Yes, I have it almost ready...

> Actually, it's similar for mmc_attach_mmc()...

    Hm, will look into this case as well...

> Kind regards
> Uffe

MBR, Sergei

Re: [PATCH] tmio_mmc_pio: test TMIO_MMC_WRPROTECT_DISABLE earlier

[Ulf Hansson]

[...]

>>>     Looking at the code, the following fragment of mmc_attach_sd()
>>> doesn't
>>> make much sense to me:
>>>
>>>          mmc_release_host(host);
>>>          err = mmc_add_card(host->card);
>>>          mmc_claim_host(host);
>>>          if (err)
>>>                  goto remove_card;
>>>
>>>          return 0;
>>>
>>> remove_card:
>>>          mmc_release_host(host);
>>>          mmc_remove_card(host->card);
>>>          host->card = NULL;
>>>          mmc_claim_host(host);
>>>
>>>     Why claim the host and immediately release it on mmc_add_card()
>>> error?
>>> Can we only claim on success and save a call here?
>
>
>> You are right, we can simplify the sequence!
>
>
>    OK, what about calling mmc_remove_card() on mmc_add_card() failure?
> Isn't it also superfluous?

Nope.

Kind regards
Uffe