[PATCH] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[PATCH] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[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-vQo90/bmh3WzQB+pC5nmwQ@public.gmane.org>
Cc: Huang Shijie <b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-imx.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index a5474ef..53bd85b 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -948,8 +948,6 @@ 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);
 	spi_master_put(master);
 
 	return 0;
-- 
1.7.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[Huang Shijie]

于 2014年02月27日 16:51, Philippe De Muyter 写道:
> -	clk_disable_unprepare(spi_imx->clk_ipg);
> -	clk_disable_unprepare(spi_imx->clk_per);
yes. we do not need to disable the clocks,
but we'd better to unprepare these two clocks.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[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-vQo90/bmh3WzQB+pC5nmwQ@public.gmane.org>
Cc: Huang Shijie <b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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;
-- 
1.7.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[Huang Shijie]

于 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-vQo90/bmh3WzQB+pC5nmwQ@public.gmane.org>
> Cc: Huang Shijie <b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[Huang Shijie]

于 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[Huang Shijie]

于 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] spi: spi-imx: spi_imx_remove: do not disable disabled clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 151 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]