| Message ID | 1402992272-21413-2-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) { > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/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) {
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(-)