| Message ID | 1408029833-6970-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 14 Aug 2014 17:23:53 +0200 Geert Uytterhoeven <geert+renesas@glider.be> wrote: > On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI > module clock is disabled. Doing so will cause a lock-up. > > When suspending, enable the module clock before disabling the SDHI > interrupts, and disable the clock again afterwards. This feels wrong. Why can't interrupts be disabled prior to turning the clock off? -Ian > This fixes suspend and resume on r8a7791/koelsch. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > index faf0924e71cb..83192420a7e3 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev) > { > struct mmc_host *mmc = dev_get_drvdata(dev); > struct tmio_mmc_host *host = mmc_priv(mmc); > + struct tmio_mmc_data *pdata = host->pdata; > + > + if (pdata->clk_enable) { > + unsigned int f; > + pdata->clk_enable(host->pdev, &f); > + } > > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); > + > + if (pdata->clk_disable) > + pdata->clk_disable(host->pdev); > + > return 0; > } > EXPORT_SYMBOL(tmio_mmc_host_suspend); > -- > 1.9.1 >
On 14 August 2014 17:23, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI > module clock is disabled. Doing so will cause a lock-up. > > When suspending, enable the module clock before disabling the SDHI > interrupts, and disable the clock again afterwards. > > This fixes suspend and resume on r8a7791/koelsch. Out of curiosity, are you using CONFIG_PM_RUNTIME to trigger this bug? Kind regards Uffe > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > index faf0924e71cb..83192420a7e3 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev) > { > struct mmc_host *mmc = dev_get_drvdata(dev); > struct tmio_mmc_host *host = mmc_priv(mmc); > + struct tmio_mmc_data *pdata = host->pdata; > + > + if (pdata->clk_enable) { > + unsigned int f; > + pdata->clk_enable(host->pdev, &f); > + } > > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); > + > + if (pdata->clk_disable) > + pdata->clk_disable(host->pdev); > + > return 0; > } > EXPORT_SYMBOL(tmio_mmc_host_suspend); > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On Fri, Aug 15, 2014 at 4:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 14 August 2014 17:23, Geert Uytterhoeven <geert+renesas@glider.be> wrote: >> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI >> module clock is disabled. Doing so will cause a lock-up. >> >> When suspending, enable the module clock before disabling the SDHI >> interrupts, and disable the clock again afterwards. >> >> This fixes suspend and resume on r8a7791/koelsch. > > Out of curiosity, are you using CONFIG_PM_RUNTIME to trigger this bug? Yes I am: $ grep CONFIG_PM .config CONFIG_PM_SLEEP=y CONFIG_PM_SLEEP_SMP=y # CONFIG_PM_AUTOSLEEP is not set # CONFIG_PM_WAKELOCKS is not set CONFIG_PM_RUNTIME=y CONFIG_PM=y CONFIG_PM_DEBUG=y CONFIG_PM_ADVANCED_DEBUG=y # CONFIG_PM_TEST_SUSPEND is not set CONFIG_PM_SLEEP_DEBUG=y CONFIG_PM_OPP=y CONFIG_PM_CLK=y # CONFIG_PMIC_ADP5520 is not set # CONFIG_PMIC_DA903X is not set # CONFIG_PM_DEVFREQ is not set $ 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-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ian, On Fri, Aug 15, 2014 at 2:49 PM, Ian Molton <ian.molton@codethink.co.uk> wrote: > On Thu, 14 Aug 2014 17:23:53 +0200 > Geert Uytterhoeven <geert+renesas@glider.be> wrote: > >> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI >> module clock is disabled. Doing so will cause a lock-up. >> >> When suspending, enable the module clock before disabling the SDHI >> interrupts, and disable the clock again afterwards. > > This feels wrong. Why can't interrupts be disabled prior to turning the clock off? The clock is handled by runtime PM. So if SDHI becomes idle, the clock is turned off. However, the card insertion interrupt is still enabled. If all interrupts would be disabled when the clock is turned off, I believe card insertion can no longer be detected. BTW, I'm still wondering why tmio_mmc_host_resume() works without enabling the clock, as it calls tmio_mmc_enable_dma(), which also writes to an SDHI register (CTL_DMA_ENABLE). >> This fixes suspend and resume on r8a7791/koelsch. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c >> index faf0924e71cb..83192420a7e3 100644 >> --- a/drivers/mmc/host/tmio_mmc_pio.c >> +++ b/drivers/mmc/host/tmio_mmc_pio.c >> @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev) >> { >> struct mmc_host *mmc = dev_get_drvdata(dev); >> struct tmio_mmc_host *host = mmc_priv(mmc); >> + struct tmio_mmc_data *pdata = host->pdata; >> + >> + if (pdata->clk_enable) { >> + unsigned int f; >> + pdata->clk_enable(host->pdev, &f); >> + } >> >> tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); >> + >> + if (pdata->clk_disable) >> + pdata->clk_disable(host->pdev); >> + >> return 0; >> } >> EXPORT_SYMBOL(tmio_mmc_host_suspend); >> -- >> 1.9.1 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-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18 August 2014 14:36, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ian, > > On Fri, Aug 15, 2014 at 2:49 PM, Ian Molton <ian.molton@codethink.co.uk> wrote: >> On Thu, 14 Aug 2014 17:23:53 +0200 >> Geert Uytterhoeven <geert+renesas@glider.be> wrote: >> >>> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI >>> module clock is disabled. Doing so will cause a lock-up. >>> >>> When suspending, enable the module clock before disabling the SDHI >>> interrupts, and disable the clock again afterwards. >> >> This feels wrong. Why can't interrupts be disabled prior to turning the clock off? > > The clock is handled by runtime PM. So if SDHI becomes idle, the clock > is turned off. > However, the card insertion interrupt is still enabled. > If all interrupts would be disabled when the clock is turned off, I believe card > insertion can no longer be detected. > > BTW, I'm still wondering why tmio_mmc_host_resume() works without > enabling the clock, as it calls tmio_mmc_enable_dma(), which also writes to > an SDHI register (CTL_DMA_ENABLE). A while ago, I tried to rework the runtime PM handling of tmio. http://www.spinics.net/lists/linux-mmc/msg23177.html The hole set never got merged, due to that we couldn't get them tested on hardware and there were a lack in review. We might want to pick them up to see if those may solve our problem. :-) Kind regards Uffe > >>> This fixes suspend and resume on r8a7791/koelsch. >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> --- >>> drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c >>> index faf0924e71cb..83192420a7e3 100644 >>> --- a/drivers/mmc/host/tmio_mmc_pio.c >>> +++ b/drivers/mmc/host/tmio_mmc_pio.c >>> @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev) >>> { >>> struct mmc_host *mmc = dev_get_drvdata(dev); >>> struct tmio_mmc_host *host = mmc_priv(mmc); >>> + struct tmio_mmc_data *pdata = host->pdata; >>> + >>> + if (pdata->clk_enable) { >>> + unsigned int f; >>> + pdata->clk_enable(host->pdev, &f); >>> + } >>> >>> tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); >>> + >>> + if (pdata->clk_disable) >>> + pdata->clk_disable(host->pdev); >>> + >>> return 0; >>> } >>> EXPORT_SYMBOL(tmio_mmc_host_suspend); >>> -- >>> 1.9.1 > > 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-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On Mon, Aug 18, 2014 at 3:10 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 18 August 2014 14:36, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Fri, Aug 15, 2014 at 2:49 PM, Ian Molton <ian.molton@codethink.co.uk> wrote: >>> On Thu, 14 Aug 2014 17:23:53 +0200 >>> Geert Uytterhoeven <geert+renesas@glider.be> wrote: >>>> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI >>>> module clock is disabled. Doing so will cause a lock-up. >>>> >>>> When suspending, enable the module clock before disabling the SDHI >>>> interrupts, and disable the clock again afterwards. >>> >>> This feels wrong. Why can't interrupts be disabled prior to turning the clock off? >> >> The clock is handled by runtime PM. So if SDHI becomes idle, the clock >> is turned off. >> However, the card insertion interrupt is still enabled. >> If all interrupts would be disabled when the clock is turned off, I believe card >> insertion can no longer be detected. >> >> BTW, I'm still wondering why tmio_mmc_host_resume() works without >> enabling the clock, as it calls tmio_mmc_enable_dma(), which also writes to >> an SDHI register (CTL_DMA_ENABLE). > > A while ago, I tried to rework the runtime PM handling of tmio. > > http://www.spinics.net/lists/linux-mmc/msg23177.html > > The hole set never got merged, due to that we couldn't get them tested > on hardware and there were a lack in review. > > We might want to pick them up to see if those may solve our problem. :-) Thanks, I applied patches 4-8 (1-3 did get merged), and tried them on SDHI1 of r8a7791/koelsch. It still seems to work as before. However, I still need my patch to fix suspend. Your "[PATCH 8/8] mmc: tmio: Handle clock gating from runtime PM functions" manages the SDHI interface clock, while my patch manages the platform-specific SDHI module clock. >>>> This fixes suspend and resume on r8a7791/koelsch. >>>> >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>>> --- >>>> drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c >>>> index faf0924e71cb..83192420a7e3 100644 >>>> --- a/drivers/mmc/host/tmio_mmc_pio.c >>>> +++ b/drivers/mmc/host/tmio_mmc_pio.c >>>> @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev) >>>> { >>>> struct mmc_host *mmc = dev_get_drvdata(dev); >>>> struct tmio_mmc_host *host = mmc_priv(mmc); >>>> + struct tmio_mmc_data *pdata = host->pdata; >>>> + >>>> + if (pdata->clk_enable) { >>>> + unsigned int f; >>>> + pdata->clk_enable(host->pdev, &f); >>>> + } >>>> >>>> tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); >>>> + >>>> + if (pdata->clk_disable) >>>> + pdata->clk_disable(host->pdev); >>>> + >>>> return 0; >>>> } >>>> EXPORT_SYMBOL(tmio_mmc_host_suspend); 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-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index faf0924e71cb..83192420a7e3 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); struct tmio_mmc_host *host = mmc_priv(mmc); + struct tmio_mmc_data *pdata = host->pdata; + + if (pdata->clk_enable) { + unsigned int f; + pdata->clk_enable(host->pdev, &f); + } tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); + + if (pdata->clk_disable) + pdata->clk_disable(host->pdev); + return 0; } EXPORT_SYMBOL(tmio_mmc_host_suspend);
On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI module clock is disabled. Doing so will cause a lock-up. When suspending, enable the module clock before disabling the SDHI interrupts, and disable the clock again afterwards. This fixes suspend and resume on r8a7791/koelsch. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++ 1 file changed, 10 insertions(+)