| Message ID | 1393492575-32495-1-git-send-email-phdm@macqel.be (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fd40dccb1a170ba689664481a3de83617b7194d2 |
| Headers | show |
? 2014?02?27? 17:16, Philippe De Muyter ??: > Currently, at module removal, one gets the following warnings: > ------------[ cut here ]------------ > WARNING: at drivers/clk/clk.c:780 clk_disable+0x18/0x24() > Modules linked in: spi_imx(-) [last unloaded: ev76c560] > CPU: 1 PID: 16337 Comm: rmmod Tainted: G W 3.10.17-80548-g90191eb-dirty #33 > [<80013b4c>] (unwind_backtrace+0x0/0xf8) from [<800115dc>] (show_stack+0x10/0x14) > [<800115dc>] (show_stack+0x10/0x14) from [<800257b8>] (warn_slowpath_common+0x4c/0x68) > [<800257b8>] (warn_slowpath_common+0x4c/0x68) from [<800257f0>] (warn_slowpath_null+0x1c/0x24) > [<800257f0>] (warn_slowpath_null+0x1c/0x24) from [<803f60ec>] (clk_disable+0x18/0x24) > [<803f60ec>] (clk_disable+0x18/0x24) from [<7f02c9cc>] (spi_imx_remove+0x54/0x9c [spi_imx]) > [<7f02c9cc>] (spi_imx_remove+0x54/0x9c [spi_imx]) from [<8025868c>] (platform_drv_remove+0x18/0x1c) > [<8025868c>] (platform_drv_remove+0x18/0x1c) from [<80256f60>] (__device_release_driver+0x70/0xcc) > [<80256f60>] (__device_release_driver+0x70/0xcc) from [<80257770>] (driver_detach+0xcc/0xd0) > [<80257770>] (driver_detach+0xcc/0xd0) from [<80256d90>] (bus_remove_driver+0x7c/0xc0) > [<80256d90>] (bus_remove_driver+0x7c/0xc0) from [<80068668>] (SyS_delete_module+0x144/0x1f8) > [<80068668>] (SyS_delete_module+0x144/0x1f8) from [<8000e080>] (ret_fast_syscall+0x0/0x30) > ---[ end trace 1f5df9ad54996300 ]--- > ------------[ cut here ]------------ > WARNING: at drivers/clk/clk.c:780 clk_disable+0x18/0x24() > Modules linked in: spi_imx(-) [last unloaded: ev76c560] > CPU: 1 PID: 16337 Comm: rmmod Tainted: G W 3.10.17-80548-g90191eb-dirty #33 > [<80013b4c>] (unwind_backtrace+0x0/0xf8) from [<800115dc>] (show_stack+0x10/0x14) > [<800115dc>] (show_stack+0x10/0x14) from [<800257b8>] (warn_slowpath_common+0x4c/0x68) > [<800257b8>] (warn_slowpath_common+0x4c/0x68) from [<800257f0>] (warn_slowpath_null+0x1c/0x24) > [<800257f0>] (warn_slowpath_null+0x1c/0x24) from [<803f60ec>] (clk_disable+0x18/0x24) > [<803f60ec>] (clk_disable+0x18/0x24) from [<7f02c9e8>] (spi_imx_remove+0x70/0x9c [spi_imx]) > [<7f02c9e8>] (spi_imx_remove+0x70/0x9c [spi_imx]) from [<8025868c>] (platform_drv_remove+0x18/0x1c) > [<8025868c>] (platform_drv_remove+0x18/0x1c) from [<80256f60>] (__device_release_driver+0x70/0xcc) > [<80256f60>] (__device_release_driver+0x70/0xcc) from [<80257770>] (driver_detach+0xcc/0xd0) > [<80257770>] (driver_detach+0xcc/0xd0) from [<80256d90>] (bus_remove_driver+0x7c/0xc0) > [<80256d90>] (bus_remove_driver+0x7c/0xc0) from [<80068668>] (SyS_delete_module+0x144/0x1f8) > [<80068668>] (SyS_delete_module+0x144/0x1f8) from [<8000e080>] (ret_fast_syscall+0x0/0x30) > ---[ end trace 1f5df9ad54996301 ]--- > > Since commit 9e556dcc55774c9a1032f32baa0e5cfafede8b70, "spi: spi-imx: only > enable the clocks when we start to transfer a message", clocks are always > disabled except when transmitting messages. There is thus no need to > disable them at module removal. > > Signed-off-by: Philippe De Muyter <phdm@macqel.be> > Cc: Huang Shijie <b32955@freescale.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Shawn Guo <shawn.guo@linaro.org> > --- > Change since v1: > keep the 'unprepare' part (Thanks Huang) > drivers/spi/spi-imx.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > index a5474ef..47f15d9 100644 > --- a/drivers/spi/spi-imx.c > +++ b/drivers/spi/spi-imx.c > @@ -948,8 +948,8 @@ static int spi_imx_remove(struct platform_device *pdev) > spi_bitbang_stop(&spi_imx->bitbang); > > writel(0, spi_imx->base + MXC_CSPICTRL); > - clk_disable_unprepare(spi_imx->clk_ipg); > - clk_disable_unprepare(spi_imx->clk_per); > + clk_unprepare(spi_imx->clk_ipg); > + clk_unprepare(spi_imx->clk_per); > spi_master_put(master); > > return 0; Acked-by: Huang Shijie <b32955@freescale.com> -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 27, 2014 at 10:16:15AM +0100, Philippe De Muyter wrote: > ------------[ cut here ]------------ > WARNING: at drivers/clk/clk.c:780 clk_disable+0x18/0x24() > Modules linked in: spi_imx(-) [last unloaded: ev76c560] > CPU: 1 PID: 16337 Comm: rmmod Tainted: G W 3.10.17-80548-g90191eb-dirty #33 > [<80013b4c>] (unwind_backtrace+0x0/0xf8) from [<800115dc>] (show_stack+0x10/0x14) Don't paste entire backtraces into commit logs unless they're adding something, it makes the mail harder to read. Especially don't do this before the text explanation of the commit. > Since commit 9e556dcc55774c9a1032f32baa0e5cfafede8b70, "spi: spi-imx: only > enable the clocks when we start to transfer a message", clocks are always > disabled except when transmitting messages. There is thus no need to > disable them at module removal. > writel(0, spi_imx->base + MXC_CSPICTRL); > - clk_disable_unprepare(spi_imx->clk_ipg); > - clk_disable_unprepare(spi_imx->clk_per); > + clk_unprepare(spi_imx->clk_ipg); > + clk_unprepare(spi_imx->clk_per); > spi_master_put(master); Why is the fix for this not to convert the enable/disable done when tranferring to be preparing as well? The message transfer doesn't get started in atomic context so I can't see any advantage to leaving the clock prepared all the time but it's possible I'm missing something.
? 2014?02?28? 11:46, Mark Brown ??: > Why is the fix for this not to convert the enable/disable done when > tranferring to be preparing as well? The message transfer doesn't It will make the code run a litter slow if we also do the prepare/unprepare in the @->{un}prepare_message hooks. This is the only concern. thanks Huang Shijie -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 28, 2014 at 12:26:23PM +0800, Huang Shijie wrote: > ? 2014?02?28? 11:46, Mark Brown ??: > > Why is the fix for this not to convert the enable/disable done when > > tranferring to be preparing as well? The message transfer doesn't > It will make the code run a litter slow if we also do the > prepare/unprepare in the > @->{un}prepare_message hooks. > This is the only concern. If you're worried about that then how about doing the prepares in runtime PM and then using autosuspend to keep the clocks prepared for a little while after the last use has finished? That's a little more involved though and would still need this due to runtime PM being optional.
On Thu, Feb 27, 2014 at 10:16:15AM +0100, Philippe De Muyter wrote:
> Currently, at module removal, one gets the following warnings:
Applied, thanks.
? 2014?02?28? 14:13, Mark Brown ??: > little while after the last use has finished? That's a little more > involved though and would still need this due to runtime PM being yes. we can use the runtime PM too. but i am not sure which one is _better_. the runtime PM? or the current way? thanks Huang Shijie -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 28, 2014 at 02:28:04PM +0800, Huang Shijie wrote: > ? 2014?02?28? 14:13, Mark Brown ??: > > little while after the last use has finished? That's a little more > > involved though and would still need this due to runtime PM being > yes. we can use the runtime PM too. > but i am not sure which one is _better_. the runtime PM? or the current way? If you are able to unprepare the clocks then depending on the system that may allow more power savings so if runtime PM solves a performance problem it's going to be better for that. Similarly if the SoC does something with runtime PM (eg, powering down power domains) then that'd be helpful but that depends on the SoC.
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index a5474ef..47f15d9 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -948,8 +948,8 @@ static int spi_imx_remove(struct platform_device *pdev) spi_bitbang_stop(&spi_imx->bitbang); writel(0, spi_imx->base + MXC_CSPICTRL); - clk_disable_unprepare(spi_imx->clk_ipg); - clk_disable_unprepare(spi_imx->clk_per); + clk_unprepare(spi_imx->clk_ipg); + clk_unprepare(spi_imx->clk_per); spi_master_put(master); return 0;
Currently, at module removal, one gets the following warnings: ------------[ cut here ]------------ WARNING: at drivers/clk/clk.c:780 clk_disable+0x18/0x24() Modules linked in: spi_imx(-) [last unloaded: ev76c560] CPU: 1 PID: 16337 Comm: rmmod Tainted: G W 3.10.17-80548-g90191eb-dirty #33 [<80013b4c>] (unwind_backtrace+0x0/0xf8) from [<800115dc>] (show_stack+0x10/0x14) [<800115dc>] (show_stack+0x10/0x14) from [<800257b8>] (warn_slowpath_common+0x4c/0x68) [<800257b8>] (warn_slowpath_common+0x4c/0x68) from [<800257f0>] (warn_slowpath_null+0x1c/0x24) [<800257f0>] (warn_slowpath_null+0x1c/0x24) from [<803f60ec>] (clk_disable+0x18/0x24) [<803f60ec>] (clk_disable+0x18/0x24) from [<7f02c9cc>] (spi_imx_remove+0x54/0x9c [spi_imx]) [<7f02c9cc>] (spi_imx_remove+0x54/0x9c [spi_imx]) from [<8025868c>] (platform_drv_remove+0x18/0x1c) [<8025868c>] (platform_drv_remove+0x18/0x1c) from [<80256f60>] (__device_release_driver+0x70/0xcc) [<80256f60>] (__device_release_driver+0x70/0xcc) from [<80257770>] (driver_detach+0xcc/0xd0) [<80257770>] (driver_detach+0xcc/0xd0) from [<80256d90>] (bus_remove_driver+0x7c/0xc0) [<80256d90>] (bus_remove_driver+0x7c/0xc0) from [<80068668>] (SyS_delete_module+0x144/0x1f8) [<80068668>] (SyS_delete_module+0x144/0x1f8) from [<8000e080>] (ret_fast_syscall+0x0/0x30) ---[ end trace 1f5df9ad54996300 ]--- ------------[ cut here ]------------ WARNING: at drivers/clk/clk.c:780 clk_disable+0x18/0x24() Modules linked in: spi_imx(-) [last unloaded: ev76c560] CPU: 1 PID: 16337 Comm: rmmod Tainted: G W 3.10.17-80548-g90191eb-dirty #33 [<80013b4c>] (unwind_backtrace+0x0/0xf8) from [<800115dc>] (show_stack+0x10/0x14) [<800115dc>] (show_stack+0x10/0x14) from [<800257b8>] (warn_slowpath_common+0x4c/0x68) [<800257b8>] (warn_slowpath_common+0x4c/0x68) from [<800257f0>] (warn_slowpath_null+0x1c/0x24) [<800257f0>] (warn_slowpath_null+0x1c/0x24) from [<803f60ec>] (clk_disable+0x18/0x24) [<803f60ec>] (clk_disable+0x18/0x24) from [<7f02c9e8>] (spi_imx_remove+0x70/0x9c [spi_imx]) [<7f02c9e8>] (spi_imx_remove+0x70/0x9c [spi_imx]) from [<8025868c>] (platform_drv_remove+0x18/0x1c) [<8025868c>] (platform_drv_remove+0x18/0x1c) from [<80256f60>] (__device_release_driver+0x70/0xcc) [<80256f60>] (__device_release_driver+0x70/0xcc) from [<80257770>] (driver_detach+0xcc/0xd0) [<80257770>] (driver_detach+0xcc/0xd0) from [<80256d90>] (bus_remove_driver+0x7c/0xc0) [<80256d90>] (bus_remove_driver+0x7c/0xc0) from [<80068668>] (SyS_delete_module+0x144/0x1f8) [<80068668>] (SyS_delete_module+0x144/0x1f8) from [<8000e080>] (ret_fast_syscall+0x0/0x30) ---[ end trace 1f5df9ad54996301 ]--- Since commit 9e556dcc55774c9a1032f32baa0e5cfafede8b70, "spi: spi-imx: only enable the clocks when we start to transfer a message", clocks are always disabled except when transmitting messages. There is thus no need to disable them at module removal. Signed-off-by: Philippe De Muyter <phdm@macqel.be> Cc: Huang Shijie <b32955@freescale.com> Cc: Mark Brown <broonie@kernel.org> Cc: Shawn Guo <shawn.guo@linaro.org> --- Change since v1: keep the 'unprepare' part (Thanks Huang) drivers/spi/spi-imx.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)