| Message ID | 20241121-ge-ian-debug-imx8-clk-tree-v1-4-0f1b722588fe@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: Fix simple video pipelines on i.MX8 | expand |
Quoting Miquel Raynal (2024-11-21 09:41:14) > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index adfc5bfb93b5a65b6f58c52ca2c432d651f7dd7d..94d93470479e77769e63e97462b176261103b552 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1927,7 +1927,6 @@ long clk_get_accuracy(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_get_accuracy); > > -__maybe_unused > static unsigned long clk_determine(struct clk_core *core, unsigned long rate) > { > struct clk_rate_request req = {}; Please add functions in the same patch that uses them. It is hard to review this patch when half the context is in another patch. > @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core) > { > struct clk_core *child; > > - core->new_rate = clk_recalc(core, core->parent->new_rate); > + if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) { > + core->new_rate = clk_determine(core, core->rate); > + if (!core->new_rate) > + core->new_rate = clk_recalc(core, core->parent->new_rate); > + } else { > + core->new_rate = clk_recalc(core, core->parent->new_rate); > + } > > hlist_for_each_entry(child, &core->children, child_node) > clk_calc_subtree(child); > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 200135e0f6d00d48b10e843259333b9733c97f38..baef0b442ac1d36ee935cbcaaaa4e2d95fe7654c 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -38,6 +38,8 @@ > #define CLK_OPS_PARENT_ENABLE BIT(12) > /* duty cycle call may be forwarded to the parent clock */ > #define CLK_DUTY_CYCLE_PARENT BIT(13) > +/* do not passively change this clock rate during subtree rate propagation */ > +#define CLK_NO_RATE_CHANGE_DURING_PROPAGATION BIT(14) Why doesn't rate locking work?
On Thu, Nov 21, 2024 at 06:41:14PM +0100, Miquel Raynal wrote: > There are mainly two ways to change a clock frequency. There's much more than that :) Off the top of my head, setting/clearing a min/max rate and changing the parent might also result in a rate change. And then, the firmware might get involved too. > The active way requires calling ->set_rate() in order to ask "on > purpose" for a frequency change. Otherwise, a clock can passively see > its frequency being updated depending on upstream clock frequency > changes. In most cases it is fine to just accept the new upstream > frequency - which by definition will have an impact on downstream > frequencies if we do not recalculate internal divisors. But there are > cases where, upon an upstream frequency change, we would like to > maintain a specific rate. Why is clk_set_rate_exclusive not enough? > As an example, on iMX8MP the video pipeline clocks are looking like this: > > video_pll1 > video_pll1_bypass > video_pll1_out > media_ldb > media_ldb_root_clk > media_disp2_pix > media_disp2_pix_root_clk > media_disp1_pix > media_disp1_pix_root_clk > > media_ldb, media_disp2_pix and media_disp1_pix are simple divisors from > which we might require 2 or 3 different rates, whereas video_pll1 is a > very configurable PLL which can achieve almost any frequency. There are > however relationships between them, typically the ldb clock must be 3.5 > or 7 times higher than the media_disp* clocks. > > Currently, if eg. media_disp2_pix is set to 71900000Hz, when media_ldb > is (later) set to 503300000Hz, media_disp2_pix is updated to 503300000Hz > as well, which clearly does not make sense. We want it to stay at > 71900000Hz during the subtree walk. > > Achieving this is the purpose of the new clock flag: > CLK_NO_RATE_CHANGE_DURING_PROPAGATION > > Please note, if the kernel was setting the ldb clock before a pixel > clock, the result would be different, and this is also what this patch > is trying to solve. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > In all cases, the LDB must be aware of the device configuration, and ask > for a clever frequency, we will never cope with slightly aware drivers > when addressing this kind of subtle situation. > --- > drivers/clk/clk.c | 9 +++++++-- > include/linux/clk-provider.h | 2 ++ > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index adfc5bfb93b5a65b6f58c52ca2c432d651f7dd7d..94d93470479e77769e63e97462b176261103b552 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1927,7 +1927,6 @@ long clk_get_accuracy(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_get_accuracy); > > -__maybe_unused > static unsigned long clk_determine(struct clk_core *core, unsigned long rate) > { > struct clk_rate_request req = {}; > @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core) > { > struct clk_core *child; > > - core->new_rate = clk_recalc(core, core->parent->new_rate); > + if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) { > + core->new_rate = clk_determine(core, core->rate); > + if (!core->new_rate) > + core->new_rate = clk_recalc(core, core->parent->new_rate); > + } else { > + core->new_rate = clk_recalc(core, core->parent->new_rate); > + } Sorry, it's not clear to me how it works. How will the parent clocks will get notified to adjust their dividers in that scenario? Also, what if they can't? Maxime
Hi Stephen, >> +/* do not passively change this clock rate during subtree rate propagation */ >> +#define CLK_NO_RATE_CHANGE_DURING_PROPAGATION BIT(14) > > Why doesn't rate locking work? Can you be more specific? What function from the API is supposed to do what I need? AFAIU, none of them is properly locking the rate during a subtree walk, but if I misread one of them, I'd be glad to drop all this. Thanks, Miquèl
Hi Maxime, On 17/12/2024 at 13:47:53 +01, Maxime Ripard <mripard@redhat.com> wrote: > On Thu, Nov 21, 2024 at 06:41:14PM +0100, Miquel Raynal wrote: >> There are mainly two ways to change a clock frequency. > > There's much more than that :) "mainly" Or maybe I should have added "on purpose". > > Off the top of my head, setting/clearing a min/max rate and changing the > parent might also result in a rate change. > > And then, the firmware might get involved too. > >> The active way requires calling ->set_rate() in order to ask "on >> purpose" for a frequency change. Otherwise, a clock can passively see >> its frequency being updated depending on upstream clock frequency >> changes. In most cases it is fine to just accept the new upstream >> frequency - which by definition will have an impact on downstream >> frequencies if we do not recalculate internal divisors. But there are >> cases where, upon an upstream frequency change, we would like to >> maintain a specific rate. > > Why is clk_set_rate_exclusive not enough? I am trying to protect these rate changes from subtree walks, I don't see where setting an exclusive rate would have an effect? But I might be overlooking something, definitely. ... >> @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core) >> { >> struct clk_core *child; >> >> - core->new_rate = clk_recalc(core, core->parent->new_rate); >> + if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) { >> + core->new_rate = clk_determine(core, core->rate); >> + if (!core->new_rate) >> + core->new_rate = clk_recalc(core, core->parent->new_rate); >> + } else { >> + core->new_rate = clk_recalc(core, core->parent->new_rate); >> + } > > Sorry, it's not clear to me how it works. How will the parent clocks > will get notified to adjust their dividers in that scenario? Also, what > if they can't? The idea is: if the flag is set, instead of accepting the new upstream rate and recalculate the downstream rate based on a previously set divider value, we change our divider value to match the same frequency as before. But if we cannot, then we just keep the old way. Cheers, Miquèl
Quoting Miquel Raynal (2024-12-23 10:38:20) > Hi Stephen, > > >> +/* do not passively change this clock rate during subtree rate propagation */ > >> +#define CLK_NO_RATE_CHANGE_DURING_PROPAGATION BIT(14) > > > > Why doesn't rate locking work? > > Can you be more specific? What function from the API is supposed to do > what I need? AFAIU, none of them is properly locking the rate during a > subtree walk, but if I misread one of them, I'd be glad to drop all this. > It's clk_set_rate_exclusive() as Maxime also asked about in the other thread.
Quoting Miquel Raynal (2024-12-23 10:43:13) > Hi Maxime, > > On 17/12/2024 at 13:47:53 +01, Maxime Ripard <mripard@redhat.com> wrote: > > > On Thu, Nov 21, 2024 at 06:41:14PM +0100, Miquel Raynal wrote: > >> There are mainly two ways to change a clock frequency. > > > > There's much more than that :) > > "mainly" > > Or maybe I should have added "on purpose". > > > > > Off the top of my head, setting/clearing a min/max rate and changing the > > parent might also result in a rate change. > > > > And then, the firmware might get involved too. > > > >> The active way requires calling ->set_rate() in order to ask "on > >> purpose" for a frequency change. Otherwise, a clock can passively see > >> its frequency being updated depending on upstream clock frequency > >> changes. In most cases it is fine to just accept the new upstream > >> frequency - which by definition will have an impact on downstream > >> frequencies if we do not recalculate internal divisors. But there are > >> cases where, upon an upstream frequency change, we would like to > >> maintain a specific rate. > > > > Why is clk_set_rate_exclusive not enough? > > I am trying to protect these rate changes from subtree walks, I don't > see where setting an exclusive rate would have an effect? But I might be > overlooking something, definitely. > > ... > > >> @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core) > >> { > >> struct clk_core *child; > >> > >> - core->new_rate = clk_recalc(core, core->parent->new_rate); > >> + if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) { > >> + core->new_rate = clk_determine(core, core->rate); > >> + if (!core->new_rate) > >> + core->new_rate = clk_recalc(core, core->parent->new_rate); > >> + } else { > >> + core->new_rate = clk_recalc(core, core->parent->new_rate); > >> + } > > > > Sorry, it's not clear to me how it works. How will the parent clocks > > will get notified to adjust their dividers in that scenario? Also, what > > if they can't? > > The idea is: if the flag is set, instead of accepting the new upstream > rate and recalculate the downstream rate based on a previously set > divider value, we change our divider value to match the same frequency > as before. But if we cannot, then we just keep the old way. > The exclusive rate code could support this if it doesn't already do so. If you call clk_set_rate_exclusive(child, <constant rate>) followed by clk_set_rate(parent, <new rate>) the core code should try to keep the child at the constant rate, or fail the clk_set_rate() call on the parent. It should be possible to confirm this with some KUnit tests for clk_set_rate_exclusive(). Similarly, if another child, child_B, of the parent changes the parent rate, we should speculate the new rate of the child_A that's protected and fail if we can't maintain the rate. We need to start generating a list of clks that we operate a rate change on to support this though, because right now we rely on the stack to track the clks that we change the rate of. Initially we thought that we could do this with clk notifiers. That may work here, but I suspect it will be clunky to get working because clk notifiers operate on struct clk.
On Mon, Dec 30, 2024 at 05:22:56PM -0800, Stephen Boyd wrote: > Quoting Miquel Raynal (2024-12-23 10:43:13) > > Hi Maxime, > > > > On 17/12/2024 at 13:47:53 +01, Maxime Ripard <mripard@redhat.com> wrote: > > > > > On Thu, Nov 21, 2024 at 06:41:14PM +0100, Miquel Raynal wrote: > > >> There are mainly two ways to change a clock frequency. > > > > > > There's much more than that :) > > > > "mainly" > > > > Or maybe I should have added "on purpose". > > > > > > > > Off the top of my head, setting/clearing a min/max rate and changing the > > > parent might also result in a rate change. > > > > > > And then, the firmware might get involved too. > > > > > >> The active way requires calling ->set_rate() in order to ask "on > > >> purpose" for a frequency change. Otherwise, a clock can passively see > > >> its frequency being updated depending on upstream clock frequency > > >> changes. In most cases it is fine to just accept the new upstream > > >> frequency - which by definition will have an impact on downstream > > >> frequencies if we do not recalculate internal divisors. But there are > > >> cases where, upon an upstream frequency change, we would like to > > >> maintain a specific rate. > > > > > > Why is clk_set_rate_exclusive not enough? > > > > I am trying to protect these rate changes from subtree walks, I don't > > see where setting an exclusive rate would have an effect? But I might be > > overlooking something, definitely. > > > > ... > > > > >> @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core) > > >> { > > >> struct clk_core *child; > > >> > > >> - core->new_rate = clk_recalc(core, core->parent->new_rate); > > >> + if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) { > > >> + core->new_rate = clk_determine(core, core->rate); > > >> + if (!core->new_rate) > > >> + core->new_rate = clk_recalc(core, core->parent->new_rate); > > >> + } else { > > >> + core->new_rate = clk_recalc(core, core->parent->new_rate); > > >> + } > > > > > > Sorry, it's not clear to me how it works. How will the parent clocks > > > will get notified to adjust their dividers in that scenario? Also, what > > > if they can't? > > > > The idea is: if the flag is set, instead of accepting the new upstream > > rate and recalculate the downstream rate based on a previously set > > divider value, we change our divider value to match the same frequency > > as before. But if we cannot, then we just keep the old way. > > > > The exclusive rate code could support this if it doesn't already do so. > If you call clk_set_rate_exclusive(child, <constant rate>) followed by > clk_set_rate(parent, <new rate>) the core code should try to keep the > child at the constant rate, or fail the clk_set_rate() call on the > parent. It should be possible to confirm this with some KUnit tests for > clk_set_rate_exclusive(). Similarly, if another child, child_B, of the > parent changes the parent rate, we should speculate the new rate of the > child_A that's protected and fail if we can't maintain the rate. We need > to start generating a list of clks that we operate a rate change on to > support this though, because right now we rely on the stack to track the > clks that we change the rate of. > > Initially we thought that we could do this with clk notifiers. That may > work here, but I suspect it will be clunky to get working because clk > notifiers operate on struct clk. I think notifiers are great for customers, but not really adequate for the clock drivers tree. Indeed, you can only react to a (sub)tree configuration using notifiers, but you can't affect it to try something new that would be a better fit. Like, if we have a PLL A, with two child clocks that are dividers. B is initially (exclusively) set to freq X, and then you want to set C to 2X. The best thing to do is to set A to 2X, and double B's divider. It's simple enough, but we have no way to try to negociate that at the moment. Maxime
Hi Stephen, >> The idea is: if the flag is set, instead of accepting the new upstream >> rate and recalculate the downstream rate based on a previously set >> divider value, we change our divider value to match the same frequency >> as before. But if we cannot, then we just keep the old way. >> > > The exclusive rate code could support this if it doesn't already do so. > If you call clk_set_rate_exclusive(child, <constant rate>) followed by > clk_set_rate(parent, <new rate>) the core code should try to keep the > child at the constant rate, or fail the clk_set_rate() call on the > parent. It should be possible to confirm this with some KUnit tests for > clk_set_rate_exclusive(). Similarly, if another child, child_B, of the > parent changes the parent rate, we should speculate the new rate of the > child_A that's protected and fail if we can't maintain the rate. We need > to start generating a list of clks that we operate a rate change on to > support this though, because right now we rely on the stack to track the > clks that we change the rate of. > > Initially we thought that we could do this with clk notifiers. That may > work here, but I suspect it will be clunky to get working because clk > notifiers operate on struct clk. I see, thanks a lot for the feedback, I'll have a look. Thanks, Miquèl
Hi Maxime, >> The exclusive rate code could support this if it doesn't already do so. >> If you call clk_set_rate_exclusive(child, <constant rate>) followed by >> clk_set_rate(parent, <new rate>) the core code should try to keep the >> child at the constant rate, or fail the clk_set_rate() call on the >> parent. It should be possible to confirm this with some KUnit tests for >> clk_set_rate_exclusive(). Similarly, if another child, child_B, of the >> parent changes the parent rate, we should speculate the new rate of the >> child_A that's protected and fail if we can't maintain the rate. We need >> to start generating a list of clks that we operate a rate change on to >> support this though, because right now we rely on the stack to track the >> clks that we change the rate of. >> >> Initially we thought that we could do this with clk notifiers. That may >> work here, but I suspect it will be clunky to get working because clk >> notifiers operate on struct clk. > > I think notifiers are great for customers, but not really adequate for > the clock drivers tree. Indeed, you can only react to a (sub)tree > configuration using notifiers, but you can't affect it to try something > new that would be a better fit. > > Like, if we have a PLL A, with two child clocks that are dividers. B is > initially (exclusively) set to freq X, and then you want to set C to 2X. > > The best thing to do is to set A to 2X, and double B's divider. It's > simple enough, but we have no way to try to negociate that at the > moment. Indeed. Do you have something in mind to address this situation (eg. a new clk provider or core API)? Thanks, Miquèl
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index adfc5bfb93b5a65b6f58c52ca2c432d651f7dd7d..94d93470479e77769e63e97462b176261103b552 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1927,7 +1927,6 @@ long clk_get_accuracy(struct clk *clk) } EXPORT_SYMBOL_GPL(clk_get_accuracy); -__maybe_unused static unsigned long clk_determine(struct clk_core *core, unsigned long rate) { struct clk_rate_request req = {}; @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core) { struct clk_core *child; - core->new_rate = clk_recalc(core, core->parent->new_rate); + if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) { + core->new_rate = clk_determine(core, core->rate); + if (!core->new_rate) + core->new_rate = clk_recalc(core, core->parent->new_rate); + } else { + core->new_rate = clk_recalc(core, core->parent->new_rate); + } hlist_for_each_entry(child, &core->children, child_node) clk_calc_subtree(child); diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 200135e0f6d00d48b10e843259333b9733c97f38..baef0b442ac1d36ee935cbcaaaa4e2d95fe7654c 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -38,6 +38,8 @@ #define CLK_OPS_PARENT_ENABLE BIT(12) /* duty cycle call may be forwarded to the parent clock */ #define CLK_DUTY_CYCLE_PARENT BIT(13) +/* do not passively change this clock rate during subtree rate propagation */ +#define CLK_NO_RATE_CHANGE_DURING_PROPAGATION BIT(14) struct clk; struct clk_hw;
There are mainly two ways to change a clock frequency. The active way requires calling ->set_rate() in order to ask "on purpose" for a frequency change. Otherwise, a clock can passively see its frequency being updated depending on upstream clock frequency changes. In most cases it is fine to just accept the new upstream frequency - which by definition will have an impact on downstream frequencies if we do not recalculate internal divisors. But there are cases where, upon an upstream frequency change, we would like to maintain a specific rate. As an example, on iMX8MP the video pipeline clocks are looking like this: video_pll1 video_pll1_bypass video_pll1_out media_ldb media_ldb_root_clk media_disp2_pix media_disp2_pix_root_clk media_disp1_pix media_disp1_pix_root_clk media_ldb, media_disp2_pix and media_disp1_pix are simple divisors from which we might require 2 or 3 different rates, whereas video_pll1 is a very configurable PLL which can achieve almost any frequency. There are however relationships between them, typically the ldb clock must be 3.5 or 7 times higher than the media_disp* clocks. Currently, if eg. media_disp2_pix is set to 71900000Hz, when media_ldb is (later) set to 503300000Hz, media_disp2_pix is updated to 503300000Hz as well, which clearly does not make sense. We want it to stay at 71900000Hz during the subtree walk. Achieving this is the purpose of the new clock flag: CLK_NO_RATE_CHANGE_DURING_PROPAGATION Please note, if the kernel was setting the ldb clock before a pixel clock, the result would be different, and this is also what this patch is trying to solve. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- In all cases, the LDB must be aware of the device configuration, and ask for a clever frequency, we will never cope with slightly aware drivers when addressing this kind of subtle situation. --- drivers/clk/clk.c | 9 +++++++-- include/linux/clk-provider.h | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-)