| Message ID | 1346415691-13371-2-git-send-email-ulf.hansson@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Ulf Hansson (2012-08-31 05:21:28) > From: Ulf Hansson <ulf.hansson@linaro.org> > > By using CLK_GET_RATE_NOCACHE flag, we tell the clk_get_rate API to > issue the hw for an updated clock rate. This can be used for a clock > which rate may be updated without a client necessary modifying it. > I'm glad to see this. We discussed whether the default behavior should be cached or from the hardware at length some time back, so having a flag to support the non-default is great. > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/clk/clk.c | 43 +++++++++++++++++++++++------------------- > include/linux/clk-provider.h | 1 + > 2 files changed, 25 insertions(+), 19 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index efdfd00..d9cbae0 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -558,25 +558,6 @@ int clk_enable(struct clk *clk) > EXPORT_SYMBOL_GPL(clk_enable); > > /** > - * clk_get_rate - return the rate of clk > - * @clk: the clk whose rate is being returned > - * > - * Simply returns the cached rate of the clk. Does not query the hardware. If > - * clk is NULL then returns 0. > - */ > -unsigned long clk_get_rate(struct clk *clk) > -{ > - unsigned long rate; > - > - mutex_lock(&prepare_lock); > - rate = __clk_get_rate(clk); > - mutex_unlock(&prepare_lock); > - > - return rate; > -} > -EXPORT_SYMBOL_GPL(clk_get_rate); > - > -/** > * __clk_round_rate - round the given rate for a clk > * @clk: round the rate of this clock > * > @@ -702,6 +683,30 @@ static void __clk_recalc_rates(struct clk *clk, unsigned long msg) > } > > /** > + * clk_get_rate - return the rate of clk > + * @clk: the clk whose rate is being returned > + * > + * Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE flag > + * is set, which means a recalc_rate will be issued. > + * If clk is NULL then returns 0. > + */ > +unsigned long clk_get_rate(struct clk *clk) > +{ > + unsigned long rate; > + > + mutex_lock(&prepare_lock); > + > + if (clk && (clk->flags & CLK_GET_RATE_NOCACHE)) > + __clk_recalc_rates(clk, 0); This is a bit subtle. Calling __clk_recalc_rates will walk the subtree of children recalculating rates as well as firing off notifiers. Is this what you want? If your clock changes rates behind your back AND has chilren then this is probably the right thing to do. However you might be better off with: if (clk && (clk->flags & CLK_GET_RATE_NOCACHE)) rate = clk->ops->recalc_rate(clk->hw, clk->parent->rate); This doesn't update children or fire off notifiers. What is best for your platform? Regards, Mike > + > + rate = __clk_get_rate(clk); > + mutex_unlock(&prepare_lock); > + > + return rate; > +} > +EXPORT_SYMBOL_GPL(clk_get_rate); > + > +/** > * __clk_speculate_rates > * @clk: first clk in the subtree > * @parent_rate: the "future" rate of clk's parent > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 77335fa..1b15307 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -26,6 +26,7 @@ > #define CLK_IGNORE_UNUSED BIT(3) /* do not gate even if unused */ > #define CLK_IS_ROOT BIT(4) /* root clk, has no parent */ > #define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */ > +#define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */ > > struct clk_hw; > > -- > 1.7.10
Hi Mike, Thanks for your input, and sorry for my late reply! On 31 August 2012 21:29, Mike Turquette <mturquette@ti.com> wrote: > Quoting Ulf Hansson (2012-08-31 05:21:28) >> From: Ulf Hansson <ulf.hansson@linaro.org> >> >> By using CLK_GET_RATE_NOCACHE flag, we tell the clk_get_rate API to >> issue the hw for an updated clock rate. This can be used for a clock >> which rate may be updated without a client necessary modifying it. >> > > I'm glad to see this. We discussed whether the default behavior should > be cached or from the hardware at length some time back, so having a > flag to support the non-default is great. > >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/clk/clk.c | 43 +++++++++++++++++++++++------------------- >> include/linux/clk-provider.h | 1 + >> 2 files changed, 25 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index efdfd00..d9cbae0 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -558,25 +558,6 @@ int clk_enable(struct clk *clk) >> EXPORT_SYMBOL_GPL(clk_enable); >> >> /** >> - * clk_get_rate - return the rate of clk >> - * @clk: the clk whose rate is being returned >> - * >> - * Simply returns the cached rate of the clk. Does not query the hardware. If >> - * clk is NULL then returns 0. >> - */ >> -unsigned long clk_get_rate(struct clk *clk) >> -{ >> - unsigned long rate; >> - >> - mutex_lock(&prepare_lock); >> - rate = __clk_get_rate(clk); >> - mutex_unlock(&prepare_lock); >> - >> - return rate; >> -} >> -EXPORT_SYMBOL_GPL(clk_get_rate); >> - >> -/** >> * __clk_round_rate - round the given rate for a clk >> * @clk: round the rate of this clock >> * >> @@ -702,6 +683,30 @@ static void __clk_recalc_rates(struct clk *clk, unsigned long msg) >> } >> >> /** >> + * clk_get_rate - return the rate of clk >> + * @clk: the clk whose rate is being returned >> + * >> + * Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE flag >> + * is set, which means a recalc_rate will be issued. >> + * If clk is NULL then returns 0. >> + */ >> +unsigned long clk_get_rate(struct clk *clk) >> +{ >> + unsigned long rate; >> + >> + mutex_lock(&prepare_lock); >> + >> + if (clk && (clk->flags & CLK_GET_RATE_NOCACHE)) >> + __clk_recalc_rates(clk, 0); > > This is a bit subtle. Calling __clk_recalc_rates will walk the subtree > of children recalculating rates as well as firing off notifiers. Is > this what you want? If your clock changes rates behind your back AND > has chilren then this is probably the right thing to do. However you > might be better off with: > > if (clk && (clk->flags & CLK_GET_RATE_NOCACHE)) > rate = clk->ops->recalc_rate(clk->hw, clk->parent->rate); > > This doesn't update children or fire off notifiers. What is best for > your platform? For my platform, ux500 and for the clock connected to this patchseries, your suggesting above is enough. (Well some additional error handling is needed in your code proposal though :-) ) The reason for why I used "__clk_recalc_rates" was because I think it could make sense to have a more generic approach, not sure if it is needed as you mention. Additionally, using __clk_recalc_rates with "0" as the notification argument, should prevent notifications from happen, right? So basically, I wanted the clock rates for the children to be updated as well as the parent clock rate, but no notifications. I can happily update the patch according to your proposal if you still think it is the best way to do it, just tell me again then. :-) Kind regards Ulf Hansson
Quoting Ulf Hansson (2012-09-06 02:09:33) > On 31 August 2012 21:29, Mike Turquette <mturquette@ti.com> wrote: > > This is a bit subtle. Calling __clk_recalc_rates will walk the subtree > > of children recalculating rates as well as firing off notifiers. Is > > this what you want? If your clock changes rates behind your back AND > > has chilren then this is probably the right thing to do. However you > > might be better off with: > > > > if (clk && (clk->flags & CLK_GET_RATE_NOCACHE)) > > rate = clk->ops->recalc_rate(clk->hw, clk->parent->rate); > > > > This doesn't update children or fire off notifiers. What is best for > > your platform? > > For my platform, ux500 and for the clock connected to this > patchseries, your suggesting above is enough. (Well some additional > error handling is needed in your code proposal though :-) ) > > The reason for why I used "__clk_recalc_rates" was because I think it > could make sense to have a more generic approach, not sure if it is > needed as you mention. Additionally, using __clk_recalc_rates with > "0" as the notification argument, should prevent notifications from > happen, right? > You are right. I didn't catch that when running through this patch the first time. > So basically, I wanted the clock rates for the children to be updated > as well as the parent clock rate, but no notifications. > This is the answer I was looking for. You DO want to walk the subtree of children and recalc the rates. Since you are the first user of such a feature I am happy to shape it for your needs ;-) > I can happily update the patch according to your proposal if you still > think it is the best way to do it, just tell me again then. :-) > No your patch does the right thing for your platform and looks sane and generic for others. I feel much better about not firing off random notifiers (which I missed when I reviewed your patch last time). I'll take this series into clk-next. Regards, Mike > Kind regards > Ulf Hansson
On Thu, Sep 6, 2012 at 5:19 PM, Mike Turquette <mturquette@ti.com> wrote: > I'll take this series into clk-next. > Oops, I forgot to ask about patch #3. Which tree do you want that to go through? Thanks, Mike > Regards, > Mike > >> Kind regards >> Ulf Hansson
On Fri, Sep 7, 2012 at 3:00 AM, Turquette, Mike <mturquette@ti.com> wrote: > On Thu, Sep 6, 2012 at 5:19 PM, Mike Turquette <mturquette@ti.com> wrote: >> I'll take this series into clk-next. > > Oops, I forgot to ask about patch #3. Which tree do you want that to > go through? Just take it all through your tree if there are no major conflicts. Acked-by etc. Yours, Linus Walleij
On 7 September 2012 10:21, Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Sep 7, 2012 at 3:00 AM, Turquette, Mike <mturquette@ti.com> wrote: >> On Thu, Sep 6, 2012 at 5:19 PM, Mike Turquette <mturquette@ti.com> wrote: >>> I'll take this series into clk-next. >> >> Oops, I forgot to ask about patch #3. Which tree do you want that to >> go through? > > Just take it all through your tree if there are no major conflicts. > Acked-by etc. Agree, it makes it simpler to go through Mike's clock tree. At least let's try it out. I will likely have similar patch series with cross dependencies later on, so this can be a good first test and hopefully it works. Although, don't we need an ack by Samuel Ortiz for the patch on mfd? "mfd: dbx500: Provide a more accurate smp_twd clock" Or, is it enough with Linus ack since he is the maintainer of that specific mfd file I have patched? Kind regards Ulf Hansson
On Fri, Sep 7, 2012 at 2:29 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 7 September 2012 10:21, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Fri, Sep 7, 2012 at 3:00 AM, Turquette, Mike <mturquette@ti.com> wrote: >>> On Thu, Sep 6, 2012 at 5:19 PM, Mike Turquette <mturquette@ti.com> wrote: >>>> I'll take this series into clk-next. >>> >>> Oops, I forgot to ask about patch #3. Which tree do you want that to >>> go through? >> >> Just take it all through your tree if there are no major conflicts. >> Acked-by etc. > > Agree, it makes it simpler to go through Mike's clock tree. At least > let's try it out. > I will likely have similar patch series with cross dependencies later > on, so this can be a good first test and hopefully it works. That will probably hit the next merge window anyway - these patches will go into v3.7 as it looks, then you can finalize the next series for v3.8. > Although, don't we need an ack by Samuel Ortiz for the patch on mfd? > "mfd: dbx500: Provide a more accurate smp_twd clock" Confused, isn't that patch part of the latter series, and not part of what Mike merged? > Or, is it enough with Linus ack since he is the maintainer of that > specific mfd file I have patched? That's up to Mike, but I hardly think Sam is going to get very angry about this if we merge it. Yours, Linus Walleij
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index efdfd00..d9cbae0 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -558,25 +558,6 @@ int clk_enable(struct clk *clk) EXPORT_SYMBOL_GPL(clk_enable); /** - * clk_get_rate - return the rate of clk - * @clk: the clk whose rate is being returned - * - * Simply returns the cached rate of the clk. Does not query the hardware. If - * clk is NULL then returns 0. - */ -unsigned long clk_get_rate(struct clk *clk) -{ - unsigned long rate; - - mutex_lock(&prepare_lock); - rate = __clk_get_rate(clk); - mutex_unlock(&prepare_lock); - - return rate; -} -EXPORT_SYMBOL_GPL(clk_get_rate); - -/** * __clk_round_rate - round the given rate for a clk * @clk: round the rate of this clock * @@ -702,6 +683,30 @@ static void __clk_recalc_rates(struct clk *clk, unsigned long msg) } /** + * clk_get_rate - return the rate of clk + * @clk: the clk whose rate is being returned + * + * Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE flag + * is set, which means a recalc_rate will be issued. + * If clk is NULL then returns 0. + */ +unsigned long clk_get_rate(struct clk *clk) +{ + unsigned long rate; + + mutex_lock(&prepare_lock); + + if (clk && (clk->flags & CLK_GET_RATE_NOCACHE)) + __clk_recalc_rates(clk, 0); + + rate = __clk_get_rate(clk); + mutex_unlock(&prepare_lock); + + return rate; +} +EXPORT_SYMBOL_GPL(clk_get_rate); + +/** * __clk_speculate_rates * @clk: first clk in the subtree * @parent_rate: the "future" rate of clk's parent diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 77335fa..1b15307 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -26,6 +26,7 @@ #define CLK_IGNORE_UNUSED BIT(3) /* do not gate even if unused */ #define CLK_IS_ROOT BIT(4) /* root clk, has no parent */ #define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */ +#define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */ struct clk_hw;