| Message ID | 1420454789-11547-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Ping? On Mon, Jan 5, 2015 at 11:46 AM, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > During system reboot, the sh-dma-engine device may be runtime-suspended, > causing a crash: > > Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c > Internal error: : 1406 [#1] SMP ARM > ... > PC is at sh_dmae_ctl_stop+0x28/0x64 > LR is at sh_dmae_ctl_stop+0x24/0x64 > > If the sh-dma-engine is runtime-suspended, its module clock is turned > off, and its registers cannot be accessed. > > To fix this, change the driver's .shutdown() callback: > - If the device is runtime suspended, do nothing, > - Else, explicitly runtime-resume the device, to avoid the device from > being suspended while sh_dmae_ctl_stop() is being executed. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v2: > - Do nothing if we're runtime suspended. > --- > drivers/dma/sh/shdmac.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c > index aec8a84784a469d7..2de30e8e7d9290b9 100644 > --- a/drivers/dma/sh/shdmac.c > +++ b/drivers/dma/sh/shdmac.c > @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) > static void sh_dmae_shutdown(struct platform_device *pdev) > { > struct sh_dmae_device *shdev = platform_get_drvdata(pdev); > + > + if (pm_runtime_suspended(&pdev->dev)) > + return; > + > + pm_runtime_get_sync(&pdev->dev); > sh_dmae_ctl_stop(shdev); > + pm_runtime_put(&pdev->dev); > } > > static int sh_dmae_runtime_suspend(struct device *dev) > -- > 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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-01-05 11:46 GMT+01:00 Geert Uytterhoeven <geert+renesas@glider.be>: > During system reboot, the sh-dma-engine device may be runtime-suspended, > causing a crash: > > Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c > Internal error: : 1406 [#1] SMP ARM > ... > PC is at sh_dmae_ctl_stop+0x28/0x64 > LR is at sh_dmae_ctl_stop+0x24/0x64 > > If the sh-dma-engine is runtime-suspended, its module clock is turned > off, and its registers cannot be accessed. > > To fix this, change the driver's .shutdown() callback: > - If the device is runtime suspended, do nothing, > - Else, explicitly runtime-resume the device, to avoid the device from > being suspended while sh_dmae_ctl_stop() is being executed. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v2: > - Do nothing if we're runtime suspended. > --- > drivers/dma/sh/shdmac.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c > index aec8a84784a469d7..2de30e8e7d9290b9 100644 > --- a/drivers/dma/sh/shdmac.c > +++ b/drivers/dma/sh/shdmac.c > @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) > static void sh_dmae_shutdown(struct platform_device *pdev) > { > struct sh_dmae_device *shdev = platform_get_drvdata(pdev); > + > + if (pm_runtime_suspended(&pdev->dev)) This still looks a little racy. What if runtime resume happens exactly here? I think safer would be disabling runtime PM before checking for suspend state. Best regards, Krzysztof > + return; > + > + pm_runtime_get_sync(&pdev->dev); > sh_dmae_ctl_stop(shdev); > + pm_runtime_put(&pdev->dev); > } > > static int sh_dmae_runtime_suspend(struct device *dev) > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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
Hi Krzysztof, On Fri, Jan 30, 2015 at 10:46 AM, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > 2015-01-05 11:46 GMT+01:00 Geert Uytterhoeven <geert+renesas@glider.be>: >> During system reboot, the sh-dma-engine device may be runtime-suspended, >> causing a crash: >> >> Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c >> Internal error: : 1406 [#1] SMP ARM >> ... >> PC is at sh_dmae_ctl_stop+0x28/0x64 >> LR is at sh_dmae_ctl_stop+0x24/0x64 >> >> If the sh-dma-engine is runtime-suspended, its module clock is turned >> off, and its registers cannot be accessed. >> >> To fix this, change the driver's .shutdown() callback: >> - If the device is runtime suspended, do nothing, >> - Else, explicitly runtime-resume the device, to avoid the device from >> being suspended while sh_dmae_ctl_stop() is being executed. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> v2: >> - Do nothing if we're runtime suspended. >> --- >> drivers/dma/sh/shdmac.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c >> index aec8a84784a469d7..2de30e8e7d9290b9 100644 >> --- a/drivers/dma/sh/shdmac.c >> +++ b/drivers/dma/sh/shdmac.c >> @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) >> static void sh_dmae_shutdown(struct platform_device *pdev) >> { >> struct sh_dmae_device *shdev = platform_get_drvdata(pdev); >> + >> + if (pm_runtime_suspended(&pdev->dev)) > > This still looks a little racy. What if runtime resume happens exactly > here? I think safer would be disabling runtime PM before checking for > suspend state. I don't think new requests can come in while .shutdown() is called, so the device cannot be resumed here. >> + return; >> + >> + pm_runtime_get_sync(&pdev->dev); >> sh_dmae_ctl_stop(shdev); >> + pm_runtime_put(&pdev->dev); >> } >> >> static int sh_dmae_runtime_suspend(struct device *dev) >> -- >> 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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 January 2015 at 11:46, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > During system reboot, the sh-dma-engine device may be runtime-suspended, > causing a crash: > > Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c > Internal error: : 1406 [#1] SMP ARM > ... > PC is at sh_dmae_ctl_stop+0x28/0x64 > LR is at sh_dmae_ctl_stop+0x24/0x64 > > If the sh-dma-engine is runtime-suspended, its module clock is turned > off, and its registers cannot be accessed. > > To fix this, change the driver's .shutdown() callback: > - If the device is runtime suspended, do nothing, > - Else, explicitly runtime-resume the device, to avoid the device from > being suspended while sh_dmae_ctl_stop() is being executed. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v2: > - Do nothing if we're runtime suspended. > --- > drivers/dma/sh/shdmac.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c > index aec8a84784a469d7..2de30e8e7d9290b9 100644 > --- a/drivers/dma/sh/shdmac.c > +++ b/drivers/dma/sh/shdmac.c > @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) > static void sh_dmae_shutdown(struct platform_device *pdev) > { > struct sh_dmae_device *shdev = platform_get_drvdata(pdev); > + > + if (pm_runtime_suspended(&pdev->dev)) > + return; > + > + pm_runtime_get_sync(&pdev->dev); > sh_dmae_ctl_stop(shdev); I can't find that sh_dmae_ctl_stop() is invoked from the runtime PM suspend callback. That means the device will be "removed" differently, depending on it's runtime PM status (due to the upper check for pm_runtime_suspended() ) . Is that really what you want? > + pm_runtime_put(&pdev->dev); > } > > static int sh_dmae_runtime_suspend(struct device *dev) > -- > 1.9.1 > 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
On Mon, Feb 02, 2015 at 10:35:10AM +0100, Ulf Hansson wrote: > On 5 January 2015 at 11:46, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > > During system reboot, the sh-dma-engine device may be runtime-suspended, > > causing a crash: > > > > Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c > > Internal error: : 1406 [#1] SMP ARM > > ... > > PC is at sh_dmae_ctl_stop+0x28/0x64 > > LR is at sh_dmae_ctl_stop+0x24/0x64 > > > > If the sh-dma-engine is runtime-suspended, its module clock is turned > > off, and its registers cannot be accessed. > > > > To fix this, change the driver's .shutdown() callback: > > - If the device is runtime suspended, do nothing, > > - Else, explicitly runtime-resume the device, to avoid the device from > > being suspended while sh_dmae_ctl_stop() is being executed. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > v2: > > - Do nothing if we're runtime suspended. > > --- > > drivers/dma/sh/shdmac.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c > > index aec8a84784a469d7..2de30e8e7d9290b9 100644 > > --- a/drivers/dma/sh/shdmac.c > > +++ b/drivers/dma/sh/shdmac.c > > @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) > > static void sh_dmae_shutdown(struct platform_device *pdev) > > { > > struct sh_dmae_device *shdev = platform_get_drvdata(pdev); > > + > > + if (pm_runtime_suspended(&pdev->dev)) > > + return; > > + > > + pm_runtime_get_sync(&pdev->dev); > > sh_dmae_ctl_stop(shdev); > > I can't find that sh_dmae_ctl_stop() is invoked from the runtime PM > suspend callback. That means the device will be "removed" differently, > depending on it's runtime PM status (due to the upper check for > pm_runtime_suspended() ) . Is that really what you want? I think the patch description is the key. "During system reboot, the sh-dma-engine device may be runtime-suspended, causing a crash" The runtime-suspended device is already idle and has removed its clock.
On 4 February 2015 at 03:08, Vinod Koul <vinod.koul@intel.com> wrote: > On Mon, Feb 02, 2015 at 10:35:10AM +0100, Ulf Hansson wrote: >> On 5 January 2015 at 11:46, Geert Uytterhoeven <geert+renesas@glider.be> wrote: >> > During system reboot, the sh-dma-engine device may be runtime-suspended, >> > causing a crash: >> > >> > Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c >> > Internal error: : 1406 [#1] SMP ARM >> > ... >> > PC is at sh_dmae_ctl_stop+0x28/0x64 >> > LR is at sh_dmae_ctl_stop+0x24/0x64 >> > >> > If the sh-dma-engine is runtime-suspended, its module clock is turned >> > off, and its registers cannot be accessed. >> > >> > To fix this, change the driver's .shutdown() callback: >> > - If the device is runtime suspended, do nothing, >> > - Else, explicitly runtime-resume the device, to avoid the device from >> > being suspended while sh_dmae_ctl_stop() is being executed. >> > >> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> > --- >> > v2: >> > - Do nothing if we're runtime suspended. >> > --- >> > drivers/dma/sh/shdmac.c | 6 ++++++ >> > 1 file changed, 6 insertions(+) >> > >> > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c >> > index aec8a84784a469d7..2de30e8e7d9290b9 100644 >> > --- a/drivers/dma/sh/shdmac.c >> > +++ b/drivers/dma/sh/shdmac.c >> > @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) >> > static void sh_dmae_shutdown(struct platform_device *pdev) >> > { >> > struct sh_dmae_device *shdev = platform_get_drvdata(pdev); >> > + >> > + if (pm_runtime_suspended(&pdev->dev)) >> > + return; >> > + >> > + pm_runtime_get_sync(&pdev->dev); >> > sh_dmae_ctl_stop(shdev); >> >> I can't find that sh_dmae_ctl_stop() is invoked from the runtime PM >> suspend callback. That means the device will be "removed" differently, >> depending on it's runtime PM status (due to the upper check for >> pm_runtime_suspended() ) . Is that really what you want? > I think the patch description is the key. "During system reboot, the > sh-dma-engine device may be runtime-suspended, causing a crash" > > The runtime-suspended device is already idle and has removed its clock. That's not my point. The device will be shutdown differently, depending if it's runtime PM suspended or not. So, if it's only about gating clocks then why do even bother invoking sh_dmae_ctl_stop() in this path. 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
Hi Ulf, On Wed, Feb 4, 2015 at 10:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 4 February 2015 at 03:08, Vinod Koul <vinod.koul@intel.com> wrote: >> On Mon, Feb 02, 2015 at 10:35:10AM +0100, Ulf Hansson wrote: >>> On 5 January 2015 at 11:46, Geert Uytterhoeven <geert+renesas@glider.be> wrote: >>> > During system reboot, the sh-dma-engine device may be runtime-suspended, >>> > causing a crash: >>> > >>> > Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c >>> > Internal error: : 1406 [#1] SMP ARM >>> > ... >>> > PC is at sh_dmae_ctl_stop+0x28/0x64 >>> > LR is at sh_dmae_ctl_stop+0x24/0x64 >>> > >>> > If the sh-dma-engine is runtime-suspended, its module clock is turned >>> > off, and its registers cannot be accessed. >>> > >>> > To fix this, change the driver's .shutdown() callback: >>> > - If the device is runtime suspended, do nothing, >>> > - Else, explicitly runtime-resume the device, to avoid the device from >>> > being suspended while sh_dmae_ctl_stop() is being executed. >>> > >>> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> > --- >>> > v2: >>> > - Do nothing if we're runtime suspended. >>> > --- >>> > drivers/dma/sh/shdmac.c | 6 ++++++ >>> > 1 file changed, 6 insertions(+) >>> > >>> > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c >>> > index aec8a84784a469d7..2de30e8e7d9290b9 100644 >>> > --- a/drivers/dma/sh/shdmac.c >>> > +++ b/drivers/dma/sh/shdmac.c >>> > @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) >>> > static void sh_dmae_shutdown(struct platform_device *pdev) >>> > { >>> > struct sh_dmae_device *shdev = platform_get_drvdata(pdev); >>> > + >>> > + if (pm_runtime_suspended(&pdev->dev)) >>> > + return; >>> > + >>> > + pm_runtime_get_sync(&pdev->dev); >>> > sh_dmae_ctl_stop(shdev); >>> >>> I can't find that sh_dmae_ctl_stop() is invoked from the runtime PM >>> suspend callback. That means the device will be "removed" differently, >>> depending on it's runtime PM status (due to the upper check for >>> pm_runtime_suspended() ) . Is that really what you want? >> I think the patch description is the key. "During system reboot, the >> sh-dma-engine device may be runtime-suspended, causing a crash" >> >> The runtime-suspended device is already idle and has removed its clock. > > That's not my point. The device will be shutdown differently, > depending if it's runtime PM suspended or not. You're right. > So, if it's only about gating clocks then why do even bother invoking > sh_dmae_ctl_stop() in this path. sh_dmae_ctl_stop() stops the DMA controller. But this is the "master stop". At this time, the individual channels should have been stopped by dmae_halt(). So you prefer my V1 patch instead, which unconditionally runtime-resumed the device, so sh_dmae_ctl_stop() can be called? http://www.spinics.net/lists/dmaengine/msg02954.html Alternatively... Both sh_dmae_resume() and sh_dmae_runtime_resume() call sh_dmae_rst(), which re-initializes the DMAOR register. To make it symmetric, we can move the call to sh_dmae_ctl_stop() to sh_dmae_suspend() and sh_dmae_runtime_suspend() instead? 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 4 February 2015 at 10:56, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ulf, > > On Wed, Feb 4, 2015 at 10:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 4 February 2015 at 03:08, Vinod Koul <vinod.koul@intel.com> wrote: >>> On Mon, Feb 02, 2015 at 10:35:10AM +0100, Ulf Hansson wrote: >>>> On 5 January 2015 at 11:46, Geert Uytterhoeven <geert+renesas@glider.be> wrote: >>>> > During system reboot, the sh-dma-engine device may be runtime-suspended, >>>> > causing a crash: >>>> > >>>> > Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c >>>> > Internal error: : 1406 [#1] SMP ARM >>>> > ... >>>> > PC is at sh_dmae_ctl_stop+0x28/0x64 >>>> > LR is at sh_dmae_ctl_stop+0x24/0x64 >>>> > >>>> > If the sh-dma-engine is runtime-suspended, its module clock is turned >>>> > off, and its registers cannot be accessed. >>>> > >>>> > To fix this, change the driver's .shutdown() callback: >>>> > - If the device is runtime suspended, do nothing, >>>> > - Else, explicitly runtime-resume the device, to avoid the device from >>>> > being suspended while sh_dmae_ctl_stop() is being executed. >>>> > >>>> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>>> > --- >>>> > v2: >>>> > - Do nothing if we're runtime suspended. >>>> > --- >>>> > drivers/dma/sh/shdmac.c | 6 ++++++ >>>> > 1 file changed, 6 insertions(+) >>>> > >>>> > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c >>>> > index aec8a84784a469d7..2de30e8e7d9290b9 100644 >>>> > --- a/drivers/dma/sh/shdmac.c >>>> > +++ b/drivers/dma/sh/shdmac.c >>>> > @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) >>>> > static void sh_dmae_shutdown(struct platform_device *pdev) >>>> > { >>>> > struct sh_dmae_device *shdev = platform_get_drvdata(pdev); >>>> > + >>>> > + if (pm_runtime_suspended(&pdev->dev)) >>>> > + return; >>>> > + >>>> > + pm_runtime_get_sync(&pdev->dev); >>>> > sh_dmae_ctl_stop(shdev); >>>> >>>> I can't find that sh_dmae_ctl_stop() is invoked from the runtime PM >>>> suspend callback. That means the device will be "removed" differently, >>>> depending on it's runtime PM status (due to the upper check for >>>> pm_runtime_suspended() ) . Is that really what you want? >>> I think the patch description is the key. "During system reboot, the >>> sh-dma-engine device may be runtime-suspended, causing a crash" >>> >>> The runtime-suspended device is already idle and has removed its clock. >> >> That's not my point. The device will be shutdown differently, >> depending if it's runtime PM suspended or not. > > You're right. > >> So, if it's only about gating clocks then why do even bother invoking >> sh_dmae_ctl_stop() in this path. > > sh_dmae_ctl_stop() stops the DMA controller. But this is the "master stop". > At this time, the individual channels should have been stopped by dmae_halt(). > > So you prefer my V1 patch instead, which unconditionally runtime-resumed > the device, so sh_dmae_ctl_stop() can be called? > http://www.spinics.net/lists/dmaengine/msg02954.html Well, that one didn't disable runtime PM, but did pm_runtime_put() when done. I guess that's because you have a PM domain controlling PM clocks which you would like to gate as well? Maybe pm_runtime_put_sync() would be better? Then disable runtime PM? > > Alternatively... > > Both sh_dmae_resume() and sh_dmae_runtime_resume() call sh_dmae_rst(), > which re-initializes the DMAOR register. > To make it symmetric, we can move the call to sh_dmae_ctl_stop() to > sh_dmae_suspend() and sh_dmae_runtime_suspend() instead? That could make sense. In principle that would enable you to remove the ->shutdown() callback, if that's possible I would have done that. As I had a look in the driver in more detail, I believe there are a similar issue to fix as @subject patch does, but for the ->remove() callback. In there, I doubt runtime PM is handled properly. 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
On Wed, Feb 04, 2015 at 10:56:00AM +0100, Geert Uytterhoeven wrote: > >>> I can't find that sh_dmae_ctl_stop() is invoked from the runtime PM > >>> suspend callback. That means the device will be "removed" differently, > >>> depending on it's runtime PM status (due to the upper check for > >>> pm_runtime_suspended() ) . Is that really what you want? > >> I think the patch description is the key. "During system reboot, the > >> sh-dma-engine device may be runtime-suspended, causing a crash" > >> > >> The runtime-suspended device is already idle and has removed its clock. > > > > That's not my point. The device will be shutdown differently, > > depending if it's runtime PM suspended or not. > > You're right. > > > So, if it's only about gating clocks then why do even bother invoking > > sh_dmae_ctl_stop() in this path. > > sh_dmae_ctl_stop() stops the DMA controller. But this is the "master stop". > At this time, the individual channels should have been stopped by dmae_halt(). > > So you prefer my V1 patch instead, which unconditionally runtime-resumed > the device, so sh_dmae_ctl_stop() can be called? > http://www.spinics.net/lists/dmaengine/msg02954.html That forces device to resume when we are doinga shutdown... > > Alternatively... > > Both sh_dmae_resume() and sh_dmae_runtime_resume() call sh_dmae_rst(), > which re-initializes the DMAOR register. > To make it symmetric, we can move the call to sh_dmae_ctl_stop() to > sh_dmae_suspend() and sh_dmae_runtime_suspend() instead? I think thats a btter idea..
diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c index aec8a84784a469d7..2de30e8e7d9290b9 100644 --- a/drivers/dma/sh/shdmac.c +++ b/drivers/dma/sh/shdmac.c @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) static void sh_dmae_shutdown(struct platform_device *pdev) { struct sh_dmae_device *shdev = platform_get_drvdata(pdev); + + if (pm_runtime_suspended(&pdev->dev)) + return; + + pm_runtime_get_sync(&pdev->dev); sh_dmae_ctl_stop(shdev); + pm_runtime_put(&pdev->dev); } static int sh_dmae_runtime_suspend(struct device *dev)
During system reboot, the sh-dma-engine device may be runtime-suspended, causing a crash: Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c Internal error: : 1406 [#1] SMP ARM ... PC is at sh_dmae_ctl_stop+0x28/0x64 LR is at sh_dmae_ctl_stop+0x24/0x64 If the sh-dma-engine is runtime-suspended, its module clock is turned off, and its registers cannot be accessed. To fix this, change the driver's .shutdown() callback: - If the device is runtime suspended, do nothing, - Else, explicitly runtime-resume the device, to avoid the device from being suspended while sh_dmae_ctl_stop() is being executed. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v2: - Do nothing if we're runtime suspended. --- drivers/dma/sh/shdmac.c | 6 ++++++ 1 file changed, 6 insertions(+)