| Message ID | 1513691.2yatcrs42i@wasted.cogentembedded.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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));
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html