All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] clk: ti: CLK_SET_RATE_NO_REPARENT for ti,mux-clock
@ 2014-06-17  8:04 Tomi Valkeinen
  2014-06-17  8:04 ` [RFC PATCH] clk: ti: set " Tomi Valkeinen
  0 siblings, 1 reply; 16+ messages in thread
From: Tomi Valkeinen @ 2014-06-17  8:04 UTC (permalink / raw)
  To: Tero Kristo, Paul Walmsley, Mike Turquette, linux-omap
  Cc: Nishanth Menon, Felipe Balbi, Tomi Valkeinen

While implementing DSS support for AM43xx I encountered a problem with clock
mux: clk mux may change the parent clock automatically, and with
set-rate-parent this leads to changing clocks for other devices. The problem is
described in more detail in the actual patch.

The problem doesn't seem to happen in -rc1, even if it was there in linux-next
(20140527). However, in -rc1, if I add debug prints to clk.c, I can see that
the code is going through the parents, but it decides to use the first one (the
correct one). So I presume it's just a matter of right clock rates or such
which will cause the parent to be changed.

Thus I think the problem is still there and should be fixed until it happens
again, either for DSS or for some other device.

This patch changes the behavior for all TI mux-clocks. I think it's much better
to default to CLK_SET_RATE_NO_REPARENT as changing the parent automatically
feels rather dangerouns to me.

Another option could be to set CLK_SET_RATE_NO_REPARENT only when the mux has
"ti,set-rate-parent" set, as (maybe) it's safe to change the parent if the
parent's rate will not be changed.

However, I'm fine with doing this only for the DSS, if someone has ideas how to
do that. A DT property would be easy, but I guess the argument against it is
the same as for the PLL rounding: it's not a hardware property.

Tomi Valkeinen (1):
  clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock

 drivers/clk/ti/mux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.9.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:04 [RFC PATCH] clk: ti: CLK_SET_RATE_NO_REPARENT for ti,mux-clock Tomi Valkeinen
@ 2014-06-17  8:04 ` Tomi Valkeinen
  2014-06-17  8:11   ` Tero Kristo
                     ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Tomi Valkeinen @ 2014-06-17  8:04 UTC (permalink / raw)
  To: Tero Kristo, Paul Walmsley, Mike Turquette, linux-omap
  Cc: Nishanth Menon, Felipe Balbi, Tomi Valkeinen

When setting the rate of a clock, by default the clock framework will
change the parent of the clock to the most suitable one in
__clk_mux_determine_rate() (most suitable by looking at the clock rate).

This is a rather dangerous default, and causes problems on AM43x when
using display and ethernet. There are multiple ways to select the clock
muxes on AM43x, and some of those clock paths have the same source
clocks for display and ethernet. When changing the clock rate for the
display subsystem, the clock framework decides to change the display mux
from the dedicated display PLL to a shared PLL which is used by the
ethernet, and then changes the rate of the shared PLL, breaking the
ethernet.

As I don't think there ever is a case where we want the clock framework
to automatically change the parent clock of a clock mux, this patch sets
the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/clk/ti/mux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
index 0197a478720c..e9d650e51287 100644
--- a/drivers/clk/ti/mux.c
+++ b/drivers/clk/ti/mux.c
@@ -160,7 +160,7 @@ static void of_mux_clk_setup(struct device_node *node)
 	u8 clk_mux_flags = 0;
 	u32 mask = 0;
 	u32 shift = 0;
-	u32 flags = 0;
+	u32 flags = CLK_SET_RATE_NO_REPARENT;
 
 	num_parents = of_clk_get_parent_count(node);
 	if (num_parents < 2) {
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:04 ` [RFC PATCH] clk: ti: set " Tomi Valkeinen
@ 2014-06-17  8:11   ` Tero Kristo
  2014-06-17  8:19     ` Paul Walmsley
  2014-06-17  8:15   ` Paul Walmsley
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Tero Kristo @ 2014-06-17  8:11 UTC (permalink / raw)
  To: Tomi Valkeinen, Paul Walmsley, Mike Turquette, linux-omap
  Cc: Nishanth Menon, Felipe Balbi

On 06/17/2014 11:04 AM, Tomi Valkeinen wrote:
> When setting the rate of a clock, by default the clock framework will
> change the parent of the clock to the most suitable one in
> __clk_mux_determine_rate() (most suitable by looking at the clock rate).
>
> This is a rather dangerous default, and causes problems on AM43x when
> using display and ethernet. There are multiple ways to select the clock
> muxes on AM43x, and some of those clock paths have the same source
> clocks for display and ethernet. When changing the clock rate for the
> display subsystem, the clock framework decides to change the display mux
> from the dedicated display PLL to a shared PLL which is used by the
> ethernet, and then changes the rate of the shared PLL, breaking the
> ethernet.
>
> As I don't think there ever is a case where we want the clock framework
> to automatically change the parent clock of a clock mux, this patch sets
> the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/clk/ti/mux.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
> index 0197a478720c..e9d650e51287 100644
> --- a/drivers/clk/ti/mux.c
> +++ b/drivers/clk/ti/mux.c
> @@ -160,7 +160,7 @@ static void of_mux_clk_setup(struct device_node *node)
>   	u8 clk_mux_flags = 0;
>   	u32 mask = 0;
>   	u32 shift = 0;
> -	u32 flags = 0;
> +	u32 flags = CLK_SET_RATE_NO_REPARENT;

I am fine with this approach, as it seems pretty much all the other 
mux-clock users are setting this flag also. The TI clocks have had this 
way of using mux clocks from the legacy times... might just be a design 
flaw.

-Tero

>
>   	num_parents = of_clk_get_parent_count(node);
>   	if (num_parents < 2) {
>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:04 ` [RFC PATCH] clk: ti: set " Tomi Valkeinen
  2014-06-17  8:11   ` Tero Kristo
@ 2014-06-17  8:15   ` Paul Walmsley
  2014-06-17 21:34     ` Mike Turquette
  2014-06-17 13:23   ` Felipe Balbi
  2014-06-19 11:33   ` Tero Kristo
  3 siblings, 1 reply; 16+ messages in thread
From: Paul Walmsley @ 2014-06-17  8:15 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Tero Kristo, Mike Turquette, linux-omap, Nishanth Menon, Felipe Balbi

On Tue, 17 Jun 2014, Tomi Valkeinen wrote:

> When setting the rate of a clock, by default the clock framework will
> change the parent of the clock to the most suitable one in
> __clk_mux_determine_rate() (most suitable by looking at the clock rate).

That is just insane.

> This is a rather dangerous default, and causes problems on AM43x when
> using display and ethernet. There are multiple ways to select the clock
> muxes on AM43x, and some of those clock paths have the same source
> clocks for display and ethernet. When changing the clock rate for the
> display subsystem, the clock framework decides to change the display mux
> from the dedicated display PLL to a shared PLL which is used by the
> ethernet, and then changes the rate of the shared PLL, breaking the
> ethernet.
> 
> As I don't think there ever is a case where we want the clock framework
> to automatically change the parent clock of a clock mux, this patch sets
> the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Paul Walmsley <paul@pwsan.com>


- Paul

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:11   ` Tero Kristo
@ 2014-06-17  8:19     ` Paul Walmsley
  2014-06-17  8:23       ` Tero Kristo
  0 siblings, 1 reply; 16+ messages in thread
From: Paul Walmsley @ 2014-06-17  8:19 UTC (permalink / raw)
  To: Tero Kristo
  Cc: Tomi Valkeinen, Mike Turquette, linux-omap, Nishanth Menon, Felipe Balbi

On Tue, 17 Jun 2014, Tero Kristo wrote:

> I am fine with this approach, as it seems pretty much all the other mux-clock
> users are setting this flag also. The TI clocks have had this way of using mux
> clocks from the legacy times... might just be a design flaw.

The non-CCF clock framework never automatically switched parents on rate 
changes, AFAIK.

The only case that approached this was with PLLs.  PLLs would 
automatically be placed into bypass if the PLL rate was set to the bypass 
rate.


- Paul

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:19     ` Paul Walmsley
@ 2014-06-17  8:23       ` Tero Kristo
  2014-06-17 21:08         ` Mike Turquette
  2014-06-18  6:33         ` Paul Walmsley
  0 siblings, 2 replies; 16+ messages in thread
From: Tero Kristo @ 2014-06-17  8:23 UTC (permalink / raw)
  To: Paul Walmsley
  Cc: Tomi Valkeinen, Mike Turquette, linux-omap, Nishanth Menon, Felipe Balbi

On 06/17/2014 11:19 AM, Paul Walmsley wrote:
> On Tue, 17 Jun 2014, Tero Kristo wrote:
>
>> I am fine with this approach, as it seems pretty much all the other mux-clock
>> users are setting this flag also. The TI clocks have had this way of using mux
>> clocks from the legacy times... might just be a design flaw.
>
> The non-CCF clock framework never automatically switched parents on rate
> changes, AFAIK.

That might be true yea, so this must have been introduced with CCF.

> The only case that approached this was with PLLs.  PLLs would
> automatically be placed into bypass if the PLL rate was set to the bypass
> rate.

Someone could argue that this is rather strange approach also and would 
be better to use some other API for the purpose.

.Tero


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:04 ` [RFC PATCH] clk: ti: set " Tomi Valkeinen
  2014-06-17  8:11   ` Tero Kristo
  2014-06-17  8:15   ` Paul Walmsley
@ 2014-06-17 13:23   ` Felipe Balbi
  2014-06-19 11:33   ` Tero Kristo
  3 siblings, 0 replies; 16+ messages in thread
From: Felipe Balbi @ 2014-06-17 13:23 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Tero Kristo, Paul Walmsley, Mike Turquette, linux-omap,
	Nishanth Menon, Felipe Balbi

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

On Tue, Jun 17, 2014 at 11:04:32AM +0300, Tomi Valkeinen wrote:
> When setting the rate of a clock, by default the clock framework will
> change the parent of the clock to the most suitable one in
> __clk_mux_determine_rate() (most suitable by looking at the clock rate).
> 
> This is a rather dangerous default, and causes problems on AM43x when
> using display and ethernet. There are multiple ways to select the clock
> muxes on AM43x, and some of those clock paths have the same source
> clocks for display and ethernet. When changing the clock rate for the
> display subsystem, the clock framework decides to change the display mux
> from the dedicated display PLL to a shared PLL which is used by the
> ethernet, and then changes the rate of the shared PLL, breaking the
> ethernet.
> 
> As I don't think there ever is a case where we want the clock framework
> to automatically change the parent clock of a clock mux, this patch sets
> the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

am437x-sk presents no problem with this patch:

Tested-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:23       ` Tero Kristo
@ 2014-06-17 21:08         ` Mike Turquette
  2014-06-18  6:33         ` Paul Walmsley
  1 sibling, 0 replies; 16+ messages in thread
From: Mike Turquette @ 2014-06-17 21:08 UTC (permalink / raw)
  To: Tero Kristo, Paul Walmsley
  Cc: Tomi Valkeinen, linux-omap, Nishanth Menon, Felipe Balbi

Quoting Tero Kristo (2014-06-17 01:23:31)
> On 06/17/2014 11:19 AM, Paul Walmsley wrote:
> > On Tue, 17 Jun 2014, Tero Kristo wrote:
> >
> >> I am fine with this approach, as it seems pretty much all the other mux-clock
> >> users are setting this flag also. The TI clocks have had this way of using mux
> >> clocks from the legacy times... might just be a design flaw.
> >
> > The non-CCF clock framework never automatically switched parents on rate
> > changes, AFAIK.
> 
> That might be true yea, so this must have been introduced with CCF.

Correct, which is why the flag exists.

> 
> > The only case that approached this was with PLLs.  PLLs would
> > automatically be placed into bypass if the PLL rate was set to the bypass
> > rate.
> 
> Someone could argue that this is rather strange approach also and would 
> be better to use some other API for the purpose.

I have always liked this feature. I had a hack patch on the list a long
time ago for testing bypass mode on the OMAP4 MPU when set to 400MHZ or
something like that (and chained from dpll_core I think...).

The point is to get the rate you ask for when you call clk_set_rate. The
method by which the PLL achieves that rate isn't really important, so
long as you have glitchless clocks (which OMAP's PRCM does).

Regards,
Mike

> 
> .Tero
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:15   ` Paul Walmsley
@ 2014-06-17 21:34     ` Mike Turquette
  2014-06-18  6:57       ` Paul Walmsley
  0 siblings, 1 reply; 16+ messages in thread
From: Mike Turquette @ 2014-06-17 21:34 UTC (permalink / raw)
  To: Paul Walmsley, Tomi Valkeinen
  Cc: Tero Kristo, linux-omap, Nishanth Menon, Felipe Balbi

Quoting Paul Walmsley (2014-06-17 01:15:09)
> On Tue, 17 Jun 2014, Tomi Valkeinen wrote:
> 
> > When setting the rate of a clock, by default the clock framework will
> > change the parent of the clock to the most suitable one in
> > __clk_mux_determine_rate() (most suitable by looking at the clock rate).
> 
> That is just insane.

The patch description is insane. The framework has nothing to do with
this dynamic re-parenting behavior and certainly the framework does not
force this behavior on clock providers. This behavior is specific to
users of __clk_mux_determine_rate. Those are:

1) drivers/clk/clk-mux.c
2) drivers/clk/qcom/mmcc-msm8960.c
3) drivers/clk/samsung/clk-s3c2410-dclk.c
4) drivers/clk/ti/mux.c

If dynamic re-parenting by default doesn't work for your platform then
you have two choices:

1) use the CLK_SET_RATE_NO_REPARENT flag (as this patch does)
2) don't use the basic divider type and write your own

If you choose #2 then all you have to do when implementing
.determine_rate is ignore the best_parent_rate argument.

Finally when the .determine_rate callback was introduced (allowing
dynamic re-parenting from a call to clk_set_rate) the
CLK_SET_RATE_NO_REPARENT flag was applied to all affected users to
maintain prior behavior and prevent regressions.

I have some local patches to improve documentation around these areas
for 3.17.

Regards,
Mike

> 
> > This is a rather dangerous default, and causes problems on AM43x when
> > using display and ethernet. There are multiple ways to select the clock
> > muxes on AM43x, and some of those clock paths have the same source
> > clocks for display and ethernet. When changing the clock rate for the
> > display subsystem, the clock framework decides to change the display mux
> > from the dedicated display PLL to a shared PLL which is used by the
> > ethernet, and then changes the rate of the shared PLL, breaking the
> > ethernet.
> > 
> > As I don't think there ever is a case where we want the clock framework
> > to automatically change the parent clock of a clock mux, this patch sets
> > the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Reviewed-by: Paul Walmsley <paul@pwsan.com>
> 
> 
> - Paul

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:23       ` Tero Kristo
  2014-06-17 21:08         ` Mike Turquette
@ 2014-06-18  6:33         ` Paul Walmsley
  1 sibling, 0 replies; 16+ messages in thread
From: Paul Walmsley @ 2014-06-18  6:33 UTC (permalink / raw)
  To: Tero Kristo
  Cc: Tomi Valkeinen, Mike Turquette, linux-omap, Nishanth Menon, Felipe Balbi

On Tue, 17 Jun 2014, Tero Kristo wrote:

> On 06/17/2014 11:19 AM, Paul Walmsley wrote:
> 
> > The only case that approached this was with PLLs.  PLLs would
> > automatically be placed into bypass if the PLL rate was set to the bypass
> > rate.
> 
> Someone could argue that this is rather strange approach also and would be
> better to use some other API for the purpose.

Probably so.  clk_set_parent() doesn't fit very well, since the parent is 
exactly the same.  Ideally there'd be some other API, like clk_lock_loop() 
and clk_unlock_loop(), given the sheer number of clock sources with 
optional, active loop compensation.

Returning to the specific OMAP PLL case.  When it was written there was 
some serious thought put into trying to determine how reasonable or safe 
of a thing it was to do.  At the time, it was concluded that the bypass 
rates were unlikely to be configured by accident, since they were so low.  
Furthermore no one identified any negative effect of switching to the the 
bypass clock for the PLLs where it was practical (DPLL1, DPLL2).  In fact 
it was considered a feature...


- Paul

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17 21:34     ` Mike Turquette
@ 2014-06-18  6:57       ` Paul Walmsley
  2014-06-18  7:06         ` Paul Walmsley
  0 siblings, 1 reply; 16+ messages in thread
From: Paul Walmsley @ 2014-06-18  6:57 UTC (permalink / raw)
  To: Mike Turquette
  Cc: Tomi Valkeinen, Tero Kristo, linux-omap, Nishanth Menon, Felipe Balbi

On Tue, 17 Jun 2014, Mike Turquette wrote:

> Quoting Paul Walmsley (2014-06-17 01:15:09)
> > On Tue, 17 Jun 2014, Tomi Valkeinen wrote:
> > 
> > > When setting the rate of a clock, by default the clock framework will
> > > change the parent of the clock to the most suitable one in
> > > __clk_mux_determine_rate() (most suitable by looking at the clock rate).
> > 
> > That is just insane.
> 
> The patch description is insane. The framework has nothing to do with
> this dynamic re-parenting behavior and certainly the framework does not
> force this behavior on clock providers. This behavior is specific to
> users of __clk_mux_determine_rate.

drivers/clk/clk-mux.c and drivers/clk/clk.c aren't considered part of the 
clock framework?

> Those are:
> 
> 1) drivers/clk/clk-mux.c
> 2) drivers/clk/qcom/mmcc-msm8960.c
> 3) drivers/clk/samsung/clk-s3c2410-dclk.c
> 4) drivers/clk/ti/mux.c
> 
> If dynamic re-parenting by default doesn't work for your platform then
> you have two choices:
> 
> 1) use the CLK_SET_RATE_NO_REPARENT flag (as this patch does)
> 2) don't use the basic divider type and write your own
> 
> If you choose #2 then all you have to do when implementing
> .determine_rate is ignore the best_parent_rate argument.
> 
> Finally when the .determine_rate callback was introduced (allowing
> dynamic re-parenting from a call to clk_set_rate) the
> CLK_SET_RATE_NO_REPARENT flag was applied to all affected users to
> maintain prior behavior and prevent regressions.

I don't disagree that some platforms might want this behavior.  But it's 
not a safe default, the flag should be the other way around.  Rare is the 
software engineer that knows whether the clock muxes on their platform are 
glitchless.


- Paul

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-18  6:57       ` Paul Walmsley
@ 2014-06-18  7:06         ` Paul Walmsley
  0 siblings, 0 replies; 16+ messages in thread
From: Paul Walmsley @ 2014-06-18  7:06 UTC (permalink / raw)
  To: Mike Turquette
  Cc: Tomi Valkeinen, Tero Kristo, linux-omap, Nishanth Menon, Felipe Balbi

On Wed, 18 Jun 2014, Paul Walmsley wrote:

> I don't disagree that some platforms might want this behavior.  But it's 
> not a safe default, the flag should be the other way around.  Rare is the 
> software engineer that knows whether the clock muxes on their platform are 
> glitchless.

(beyond all the other issues with unexpected parent changes, races, etc.)


- Paul

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-17  8:04 ` [RFC PATCH] clk: ti: set " Tomi Valkeinen
                     ` (2 preceding siblings ...)
  2014-06-17 13:23   ` Felipe Balbi
@ 2014-06-19 11:33   ` Tero Kristo
  2014-07-01 19:48     ` Felipe Balbi
  3 siblings, 1 reply; 16+ messages in thread
From: Tero Kristo @ 2014-06-19 11:33 UTC (permalink / raw)
  To: Tomi Valkeinen, Paul Walmsley, Mike Turquette, linux-omap
  Cc: Nishanth Menon, Felipe Balbi

On 06/17/2014 11:04 AM, Tomi Valkeinen wrote:
> When setting the rate of a clock, by default the clock framework will
> change the parent of the clock to the most suitable one in
> __clk_mux_determine_rate() (most suitable by looking at the clock rate).
>
> This is a rather dangerous default, and causes problems on AM43x when
> using display and ethernet. There are multiple ways to select the clock
> muxes on AM43x, and some of those clock paths have the same source
> clocks for display and ethernet. When changing the clock rate for the
> display subsystem, the clock framework decides to change the display mux
> from the dedicated display PLL to a shared PLL which is used by the
> ethernet, and then changes the rate of the shared PLL, breaking the
> ethernet.
>
> As I don't think there ever is a case where we want the clock framework
> to automatically change the parent clock of a clock mux, this patch sets
> the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/clk/ti/mux.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
> index 0197a478720c..e9d650e51287 100644
> --- a/drivers/clk/ti/mux.c
> +++ b/drivers/clk/ti/mux.c
> @@ -160,7 +160,7 @@ static void of_mux_clk_setup(struct device_node *node)
>   	u8 clk_mux_flags = 0;
>   	u32 mask = 0;
>   	u32 shift = 0;
> -	u32 flags = 0;
> +	u32 flags = CLK_SET_RATE_NO_REPARENT;
>
>   	num_parents = of_clk_get_parent_count(node);
>   	if (num_parents < 2) {
>

Thanks, queued for 3.16-rc fixes.

-Tero

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-06-19 11:33   ` Tero Kristo
@ 2014-07-01 19:48     ` Felipe Balbi
  2014-07-03  7:41       ` Tero Kristo
  0 siblings, 1 reply; 16+ messages in thread
From: Felipe Balbi @ 2014-07-01 19:48 UTC (permalink / raw)
  To: Tero Kristo
  Cc: Tomi Valkeinen, Paul Walmsley, Mike Turquette, linux-omap,
	Nishanth Menon, Felipe Balbi

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

Hi,

On Thu, Jun 19, 2014 at 02:33:14PM +0300, Tero Kristo wrote:
> On 06/17/2014 11:04 AM, Tomi Valkeinen wrote:
> >When setting the rate of a clock, by default the clock framework will
> >change the parent of the clock to the most suitable one in
> >__clk_mux_determine_rate() (most suitable by looking at the clock rate).
> >
> >This is a rather dangerous default, and causes problems on AM43x when
> >using display and ethernet. There are multiple ways to select the clock
> >muxes on AM43x, and some of those clock paths have the same source
> >clocks for display and ethernet. When changing the clock rate for the
> >display subsystem, the clock framework decides to change the display mux
> >from the dedicated display PLL to a shared PLL which is used by the
> >ethernet, and then changes the rate of the shared PLL, breaking the
> >ethernet.
> >
> >As I don't think there ever is a case where we want the clock framework
> >to automatically change the parent clock of a clock mux, this patch sets
> >the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.
> >
> >Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >---
> >  drivers/clk/ti/mux.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
> >index 0197a478720c..e9d650e51287 100644
> >--- a/drivers/clk/ti/mux.c
> >+++ b/drivers/clk/ti/mux.c
> >@@ -160,7 +160,7 @@ static void of_mux_clk_setup(struct device_node *node)
> >  	u8 clk_mux_flags = 0;
> >  	u32 mask = 0;
> >  	u32 shift = 0;
> >-	u32 flags = 0;
> >+	u32 flags = CLK_SET_RATE_NO_REPARENT;
> >
> >  	num_parents = of_clk_get_parent_count(node);
> >  	if (num_parents < 2) {
> >
> 
> Thanks, queued for 3.16-rc fixes.

did you skip a few -rcs by any chance? Looks like this could've been
merged on v3.16-rc3... Just checking.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-07-01 19:48     ` Felipe Balbi
@ 2014-07-03  7:41       ` Tero Kristo
  2014-07-03 22:06         ` Mike Turquette
  0 siblings, 1 reply; 16+ messages in thread
From: Tero Kristo @ 2014-07-03  7:41 UTC (permalink / raw)
  To: balbi
  Cc: Tomi Valkeinen, Paul Walmsley, Mike Turquette, linux-omap,
	Nishanth Menon

On 07/01/2014 10:48 PM, Felipe Balbi wrote:
> Hi,
>
> On Thu, Jun 19, 2014 at 02:33:14PM +0300, Tero Kristo wrote:
>> On 06/17/2014 11:04 AM, Tomi Valkeinen wrote:
>>> When setting the rate of a clock, by default the clock framework will
>>> change the parent of the clock to the most suitable one in
>>> __clk_mux_determine_rate() (most suitable by looking at the clock rate).
>>>
>>> This is a rather dangerous default, and causes problems on AM43x when
>>> using display and ethernet. There are multiple ways to select the clock
>>> muxes on AM43x, and some of those clock paths have the same source
>>> clocks for display and ethernet. When changing the clock rate for the
>>> display subsystem, the clock framework decides to change the display mux
>> >from the dedicated display PLL to a shared PLL which is used by the
>>> ethernet, and then changes the rate of the shared PLL, breaking the
>>> ethernet.
>>>
>>> As I don't think there ever is a case where we want the clock framework
>>> to automatically change the parent clock of a clock mux, this patch sets
>>> the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>   drivers/clk/ti/mux.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
>>> index 0197a478720c..e9d650e51287 100644
>>> --- a/drivers/clk/ti/mux.c
>>> +++ b/drivers/clk/ti/mux.c
>>> @@ -160,7 +160,7 @@ static void of_mux_clk_setup(struct device_node *node)
>>>   	u8 clk_mux_flags = 0;
>>>   	u32 mask = 0;
>>>   	u32 shift = 0;
>>> -	u32 flags = 0;
>>> +	u32 flags = CLK_SET_RATE_NO_REPARENT;
>>>
>>>   	num_parents = of_clk_get_parent_count(node);
>>>   	if (num_parents < 2) {
>>>
>>
>> Thanks, queued for 3.16-rc fixes.
>
> did you skip a few -rcs by any chance? Looks like this could've been
> merged on v3.16-rc3... Just checking.

This goes through Mike's clk tree, so there is extra latency there. Not 
sure when he plans to send next pull-req for clk-fixes to linux-master.

-Tero

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH] clk: ti: set CLK_SET_RATE_NO_REPARENT for ti,mux-clock
  2014-07-03  7:41       ` Tero Kristo
@ 2014-07-03 22:06         ` Mike Turquette
  0 siblings, 0 replies; 16+ messages in thread
From: Mike Turquette @ 2014-07-03 22:06 UTC (permalink / raw)
  To: Tero Kristo, balbi
  Cc: Tomi Valkeinen, Paul Walmsley, linux-omap, Nishanth Menon

Quoting Tero Kristo (2014-07-03 00:41:22)
> On 07/01/2014 10:48 PM, Felipe Balbi wrote:
> > Hi,
> >
> > On Thu, Jun 19, 2014 at 02:33:14PM +0300, Tero Kristo wrote:
> >> On 06/17/2014 11:04 AM, Tomi Valkeinen wrote:
> >>> When setting the rate of a clock, by default the clock framework will
> >>> change the parent of the clock to the most suitable one in
> >>> __clk_mux_determine_rate() (most suitable by looking at the clock rate).
> >>>
> >>> This is a rather dangerous default, and causes problems on AM43x when
> >>> using display and ethernet. There are multiple ways to select the clock
> >>> muxes on AM43x, and some of those clock paths have the same source
> >>> clocks for display and ethernet. When changing the clock rate for the
> >>> display subsystem, the clock framework decides to change the display mux
> >> >from the dedicated display PLL to a shared PLL which is used by the
> >>> ethernet, and then changes the rate of the shared PLL, breaking the
> >>> ethernet.
> >>>
> >>> As I don't think there ever is a case where we want the clock framework
> >>> to automatically change the parent clock of a clock mux, this patch sets
> >>> the CLK_SET_RATE_NO_REPARENT for all ti,mux-clocks.
> >>>
> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>> ---
> >>>   drivers/clk/ti/mux.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
> >>> index 0197a478720c..e9d650e51287 100644
> >>> --- a/drivers/clk/ti/mux.c
> >>> +++ b/drivers/clk/ti/mux.c
> >>> @@ -160,7 +160,7 @@ static void of_mux_clk_setup(struct device_node *node)
> >>>     u8 clk_mux_flags = 0;
> >>>     u32 mask = 0;
> >>>     u32 shift = 0;
> >>> -   u32 flags = 0;
> >>> +   u32 flags = CLK_SET_RATE_NO_REPARENT;
> >>>
> >>>     num_parents = of_clk_get_parent_count(node);
> >>>     if (num_parents < 2) {
> >>>
> >>
> >> Thanks, queued for 3.16-rc fixes.
> >
> > did you skip a few -rcs by any chance? Looks like this could've been
> > merged on v3.16-rc3... Just checking.
> 
> This goes through Mike's clk tree, so there is extra latency there. Not 
> sure when he plans to send next pull-req for clk-fixes to linux-master.

Expect it late next week as several new fixes pull requests have come
in. I merge those into clk-fixes, which then gets merged into clk-next
and all of that gets pulled into linux-next. After some cycles there and
testing on my end I send the fixes PR to Linus. So expect it to go
between -rc4 and -rc5.

Regards,
Mike

> 
> -Tero

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2014-07-03 22:07 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-17  8:04 [RFC PATCH] clk: ti: CLK_SET_RATE_NO_REPARENT for ti,mux-clock Tomi Valkeinen
2014-06-17  8:04 ` [RFC PATCH] clk: ti: set " Tomi Valkeinen
2014-06-17  8:11   ` Tero Kristo
2014-06-17  8:19     ` Paul Walmsley
2014-06-17  8:23       ` Tero Kristo
2014-06-17 21:08         ` Mike Turquette
2014-06-18  6:33         ` Paul Walmsley
2014-06-17  8:15   ` Paul Walmsley
2014-06-17 21:34     ` Mike Turquette
2014-06-18  6:57       ` Paul Walmsley
2014-06-18  7:06         ` Paul Walmsley
2014-06-17 13:23   ` Felipe Balbi
2014-06-19 11:33   ` Tero Kristo
2014-07-01 19:48     ` Felipe Balbi
2014-07-03  7:41       ` Tero Kristo
2014-07-03 22:06         ` Mike Turquette

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.