| Message ID | 20120320140220.GE32469@S2101-09.ap.freescale.net |
|---|---|
| State | New |
| Headers | show |
On Tue, March 20, 2012 7:02 am, Shawn Guo wrote: > On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote: > ... >> +struct clk_ops { >> + int (*prepare)(struct clk_hw *hw); >> + void (*unprepare)(struct clk_hw *hw); >> + int (*enable)(struct clk_hw *hw); >> + void (*disable)(struct clk_hw *hw); >> + int (*is_enabled)(struct clk_hw *hw); >> + unsigned long (*recalc_rate)(struct clk_hw *hw, >> + unsigned long parent_rate); > > I believe I have heard people love the interface with parent_rate > passed in. I love that too. But I would like to ask the same thing > on .round_rate and .set_rate as well for the same reason why we have > it for .recalc_rate. In my case, for most clocks, set rate involves reparenting. So, what does passing parent_rate for these even mean? Passing parent_rate seems more apt for recalc_rate since it's called when the parent rate changes -- so, the actual parent itself is not expected to change. I could ignore the parameter, but just wondering how many of the others see value in this. And if we do add this parameter, it shouldn't be made mandatory for the platform driver to use it (due to other assumptions the clock framework might make). -Saravana
On Tue, Mar 20, 2012 at 7:02 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote: > ... >> +struct clk_ops { >> + int (*prepare)(struct clk_hw *hw); >> + void (*unprepare)(struct clk_hw *hw); >> + int (*enable)(struct clk_hw *hw); >> + void (*disable)(struct clk_hw *hw); >> + int (*is_enabled)(struct clk_hw *hw); >> + unsigned long (*recalc_rate)(struct clk_hw *hw, >> + unsigned long parent_rate); > > I believe I have heard people love the interface with parent_rate > passed in. I love that too. But I would like to ask the same thing > on .round_rate and .set_rate as well for the same reason why we have > it for .recalc_rate. This is something that was discussed on the list but not taken in due to rapid flux in code. I always liked the idea however. > >> + long (*round_rate)(struct clk_hw *hw, unsigned long, >> + unsigned long *); > > Yes, we already have parent_rate passed in .round_rate with an pointer. > But I think it'd be better to have it passed in no matter flag > CLK_SET_RATE_PARENT is set or not. Something like: This places the burden of checking the flags onto the .round_rate implementation with __clk_get_flags, but that's not a problem really. > > 8<--- > @@ -584,7 +584,7 @@ EXPORT_SYMBOL_GPL(clk_get_rate); > */ > unsigned long __clk_round_rate(struct clk *clk, unsigned long rate) > { > - unsigned long unused; > + unsigned long parent_rate = 0; > > if (!clk) > return -EINVAL; > @@ -592,10 +592,10 @@ unsigned long __clk_round_rate(struct clk *clk, unsigned long rate) > if (!clk->ops->round_rate) > return clk->rate; > > - if (clk->flags & CLK_SET_RATE_PARENT) > - return clk->ops->round_rate(clk->hw, rate, &unused); > - else > - return clk->ops->round_rate(clk->hw, rate, NULL); > + if (clk->parent) > + parent_rate = clk->parent->rate; > + > + return clk->ops->round_rate(clk->hw, rate, &parent_rate); > } > > /** > @@ -765,9 +765,12 @@ static void clk_calc_subtree(struct clk *clk, unsigned long new_rate) > static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate) > { > struct clk *top = clk; > - unsigned long best_parent_rate = clk->parent->rate; > + unsigned long best_parent_rate = 0; > unsigned long new_rate; > > + if (clk->parent) > + best_parent_rate = clk->parent->rate; > + > if (!clk->ops->round_rate && !(clk->flags & CLK_SET_RATE_PARENT)) { > clk->new_rate = clk->rate; > return NULL; > @@ -780,10 +783,7 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate) > goto out; > } > > - if (clk->flags & CLK_SET_RATE_PARENT) > - new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate); > - else > - new_rate = clk->ops->round_rate(clk->hw, rate, NULL); > + new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate); > > if (best_parent_rate != clk->parent->rate) { > top = clk_calc_new_rates(clk->parent, best_parent_rate); > > --->8 ACK > > The reason behind the change is that any clk implements .round_rate will > mostly need to get the parent rate, no matter flag CLK_SET_RATE_PARENT > is set or not. Instead of expecting every .round_rate implementation > to get the parent rate in the way beloe > > parent_rate = __clk_get_rate(__clk_get_parent(hw->clk)); > > we can just pass the parent rate into .round_rate. > >> + int (*set_parent)(struct clk_hw *hw, u8 index); >> + u8 (*get_parent)(struct clk_hw *hw); >> + int (*set_rate)(struct clk_hw *hw, unsigned long); > > For the same reason, I would change .set_rate interface like below. > > 8<--- > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index d5ac6a7..6bd8037 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -119,7 +119,8 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate, > } > EXPORT_SYMBOL_GPL(clk_divider_round_rate); > > -static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate) > +static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > { > struct clk_divider *divider = to_clk_divider(hw); > unsigned int div; > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index dbc310a..d74ac4b 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -833,17 +833,18 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even > static void clk_change_rate(struct clk *clk) > { > struct clk *child; > - unsigned long old_rate; > + unsigned long old_rate, parent_rate = 0; > struct hlist_node *tmp; > > old_rate = clk->rate; > + if (clk->parent) > + parent_rate = clk->parent->rate; > > if (clk->ops->set_rate) > - clk->ops->set_rate(clk->hw, clk->new_rate); > + clk->ops->set_rate(clk->hw, clk->new_rate, parent_rate); > > if (clk->ops->recalc_rate) > - clk->rate = clk->ops->recalc_rate(clk->hw, > - clk->parent->rate); > + clk->rate = clk->ops->recalc_rate(clk->hw, parent_rate); > else > clk->rate = clk->parent->rate; > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 5508897..1031f1f 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -125,7 +125,8 @@ struct clk_ops { > unsigned long *); > int (*set_parent)(struct clk_hw *hw, u8 index); > u8 (*get_parent)(struct clk_hw *hw); > - int (*set_rate)(struct clk_hw *hw, unsigned long); > + int (*set_rate)(struct clk_hw *hw, unsigned long, > + unsigned long); > void (*init)(struct clk_hw *hw); > }; > > --->8 ACK > > Then with parent rate passed into .round_rate and .set_rate like what > we do with .recalc_rate right now, we can save particular clk > implementation from calling __clk_get_parent() and __clk_get_rate to > get parent rate. > > For example, the following will the be diff we gain on clk-divider. > > 8<--- > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index 6bd8037..8a28ffb 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -68,8 +68,8 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate, > if (divider->flags & CLK_DIVIDER_ONE_BASED) > maxdiv--; > > - if (!best_parent_rate) { > - parent_rate = __clk_get_rate(__clk_get_parent(hw->clk)); > + if (!(divider->flags & CLK_SET_RATE_PARENT)) { This is wrong. CLK_SET_RATE_PARENT is a common clock flag applied to struct clk's .flags member, not the divider. This function must still use __clk_get_flags(hw->clk) here, but that's OK. > + parent_rate = *best_parent_rate; > bestdiv = DIV_ROUND_UP(parent_rate, rate); > bestdiv = bestdiv == 0 ? 1 : bestdiv; > bestdiv = bestdiv > maxdiv ? maxdiv : bestdiv; > @@ -109,13 +109,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate, > int div; > div = clk_divider_bestdiv(hw, rate, prate); > > - if (prate) > - return *prate / div; > - else { > - unsigned long r; > - r = __clk_get_rate(__clk_get_parent(hw->clk)); > - return r / div; > - } > + return *prate / div; > } > EXPORT_SYMBOL_GPL(clk_divider_round_rate); > > @@ -127,7 +121,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, > unsigned long flags = 0; > u32 val; > > - div = __clk_get_rate(__clk_get_parent(hw->clk)) / rate; > + div = parent_rate / rate; > > if (!(divider->flags & CLK_DIVIDER_ONE_BASED)) > div--; > > --->8 ACK, besides my comment above. > > I always get a sense of worry in using functions named in __xxx which > sounds like something somehow internal. With the requested interface > change above, I can get all __xxx functions out of my clk_ops > implementation. As mentioned above, you'll still need to check for CLK_SET_RATE_PARENT in your .round_rate implementation with __clk_get_flags(hw->clk). Did you want to send a formal patch or just have me absorb this into the fixes I'm brewing already? I've already fixed the potential null ptr dereferences in clk_calc_new_rates, but that's no big deal. Regards, Mike
On Tue, Mar 20, 2012 at 10:46 AM, Saravana Kannan <skannan@codeaurora.org> wrote: > On Tue, March 20, 2012 7:02 am, Shawn Guo wrote: >> On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote: >> ... >>> +struct clk_ops { >>> + int (*prepare)(struct clk_hw *hw); >>> + void (*unprepare)(struct clk_hw *hw); >>> + int (*enable)(struct clk_hw *hw); >>> + void (*disable)(struct clk_hw *hw); >>> + int (*is_enabled)(struct clk_hw *hw); >>> + unsigned long (*recalc_rate)(struct clk_hw *hw, >>> + unsigned long parent_rate); >> >> I believe I have heard people love the interface with parent_rate >> passed in. I love that too. But I would like to ask the same thing >> on .round_rate and .set_rate as well for the same reason why we have >> it for .recalc_rate. > > In my case, for most clocks, set rate involves reparenting. So, what does > passing parent_rate for these even mean? Passing parent_rate seems more > apt for recalc_rate since it's called when the parent rate changes -- so, > the actual parent itself is not expected to change. From my conversations with folks across many platforms, I think that the way your clock tree expects to change rates is the exception, not the rule. As such you should just ignore the parent_rate parameter as it useless to you. > I could ignore the parameter, but just wondering how many of the others > see value in this. And if we do add this parameter, it shouldn't be made > mandatory for the platform driver to use it (due to other assumptions the > clock framework might make). From my rough census of folks that actually need .set_rate support, I think that everyone except MSM could benefit from this. Your concept of clk_set_rate is everyone else's clk_set_parent. Ignoring the new parameter should cause you no harm. It does make me wonder if it would be a good idea to pass in the parent rate for .set_parent, which is analogous to .set_rate in many ways. Regards, Mike > -Saravana
On 03/20/2012 04:53 PM, Turquette, Mike wrote: > On Tue, Mar 20, 2012 at 10:46 AM, Saravana Kannan > <skannan@codeaurora.org> wrote: >> On Tue, March 20, 2012 7:02 am, Shawn Guo wrote: >>> On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote: >>> ... >>>> +struct clk_ops { >>>> + int (*prepare)(struct clk_hw *hw); >>>> + void (*unprepare)(struct clk_hw *hw); >>>> + int (*enable)(struct clk_hw *hw); >>>> + void (*disable)(struct clk_hw *hw); >>>> + int (*is_enabled)(struct clk_hw *hw); >>>> + unsigned long (*recalc_rate)(struct clk_hw *hw, >>>> + unsigned long parent_rate); >>> >>> I believe I have heard people love the interface with parent_rate >>> passed in. I love that too. But I would like to ask the same thing >>> on .round_rate and .set_rate as well for the same reason why we have >>> it for .recalc_rate. >> >> In my case, for most clocks, set rate involves reparenting. So, what does >> passing parent_rate for these even mean? Passing parent_rate seems more >> apt for recalc_rate since it's called when the parent rate changes -- so, >> the actual parent itself is not expected to change. > > From my conversations with folks across many platforms, I think that > the way your clock tree expects to change rates is the exception, not > the rule. As such you should just ignore the parent_rate parameter as > it useless to you. > >> I could ignore the parameter, but just wondering how many of the others >> see value in this. And if we do add this parameter, it shouldn't be made >> mandatory for the platform driver to use it (due to other assumptions the >> clock framework might make). > > From my rough census of folks that actually need .set_rate support, I > think that everyone except MSM could benefit from this. Your concept > of clk_set_rate is everyone else's clk_set_parent. To clarify, MSM's set parent is a subset of steps needed to be done to finish set parent. So, it's not that we just randomly decided to swap the meanings of these two functions. Also, I think don't think the difference in view of set_rate is due to the difference in the clock trees between platforms with complicated trees. I think it's more because of who actually decides the rates for the clock tree. It looks like some platforms pick a top-down approach to deciding the rates of the clock tre while others pick a bottom-up approach. > Ignoring the new parameter should cause you no harm. As long as this is guaranteed, I have no problems with Shawn's suggestion. > It does make me > wonder if it would be a good idea to pass in the parent rate for > .set_parent, which is analogous to .set_rate in many ways. I need to think a bit more about this. -Saravana
On 21 March 2012 07:46, Turquette, Mike <mturquette@ti.com> wrote: ... > As mentioned above, you'll still need to check for CLK_SET_RATE_PARENT > in your .round_rate implementation with __clk_get_flags(hw->clk). > For my particular case, the clk is PLL with fixed rate clk (oscillator) as parent. It's known that flag CLK_SET_RATE_PARENT will never be set for this type of clks. > Did you want to send a formal patch or just have me absorb this into > the fixes I'm brewing already? I've already fixed the potential null > ptr dereferences in clk_calc_new_rates, but that's no big deal. > The code was attached for better discussion, and I would leave the formal patch to you. Regards, Shawn
On 03/20/2012 08:10 PM, Saravana Kannan wrote: > On 03/20/2012 04:53 PM, Turquette, Mike wrote: >> It does make me >> wonder if it would be a good idea to pass in the parent rate for >> .set_parent, which is analogous to .set_rate in many ways. > > I need to think a bit more about this. I was thinking about this. I think the common clock fwk should let the set_parent ops "return" the rate of the clock in addition to passing the rate of the parent in. Say this is a divider clock and some one changes the parent. The cached "rate" of the clock in the clock fwk is no longer correct. So, the clock fwk should also add a "*new_rate" param to set parent ops. Thanks, Saravana
On Fri, Mar 23, 2012 at 2:33 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 03/20/2012 08:10 PM, Saravana Kannan wrote: >> >> On 03/20/2012 04:53 PM, Turquette, Mike wrote: >>> >>> It does make me >>> wonder if it would be a good idea to pass in the parent rate for >>> .set_parent, which is analogous to .set_rate in many ways. >> >> >> I need to think a bit more about this. > > > I was thinking about this. I think the common clock fwk should let the > set_parent ops "return" the rate of the clock in addition to passing the > rate of the parent in. > > Say this is a divider clock and some one changes the parent. The cached > "rate" of the clock in the clock fwk is no longer correct. So, the clock fwk > should also add a "*new_rate" param to set parent ops. __clk_recalc_rates is called by __clk_reparent which is called by clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. Does this not handle the old cached clk->rate for you? Thanks, Mike > > Thanks, > > Saravana > > -- > Sent by an employee of the Qualcomm Innovation Center, Inc. > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On 03/23/2012 02:39 PM, Turquette, Mike wrote: > On Fri, Mar 23, 2012 at 2:33 PM, Saravana Kannan<skannan@codeaurora.org> wrote: >> On 03/20/2012 08:10 PM, Saravana Kannan wrote: >>> >>> On 03/20/2012 04:53 PM, Turquette, Mike wrote: >>>> >>>> It does make me >>>> wonder if it would be a good idea to pass in the parent rate for >>>> .set_parent, which is analogous to .set_rate in many ways. >>> >>> >>> I need to think a bit more about this. >> >> >> I was thinking about this. I think the common clock fwk should let the >> set_parent ops "return" the rate of the clock in addition to passing the >> rate of the parent in. >> >> Say this is a divider clock and some one changes the parent. The cached >> "rate" of the clock in the clock fwk is no longer correct. So, the clock fwk >> should also add a "*new_rate" param to set parent ops. > > __clk_recalc_rates is called by __clk_reparent which is called by > clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. > > Does this not handle the old cached clk->rate for you? > Yeah, I realized this just after I sent the email. I'm looking at the code to see if that's sufficient or not. Will get back soon. -Saravana
On 03/23/2012 02:39 PM, Turquette, Mike wrote: > On Fri, Mar 23, 2012 at 2:33 PM, Saravana Kannan<skannan@codeaurora.org> wrote: >> On 03/20/2012 08:10 PM, Saravana Kannan wrote: >>> >>> On 03/20/2012 04:53 PM, Turquette, Mike wrote: >>>> >>>> It does make me >>>> wonder if it would be a good idea to pass in the parent rate for >>>> .set_parent, which is analogous to .set_rate in many ways. >>> >>> >>> I need to think a bit more about this. >> >> >> I was thinking about this. I think the common clock fwk should let the >> set_parent ops "return" the rate of the clock in addition to passing the >> rate of the parent in. >> >> Say this is a divider clock and some one changes the parent. The cached >> "rate" of the clock in the clock fwk is no longer correct. So, the clock fwk >> should also add a "*new_rate" param to set parent ops. > > __clk_recalc_rates is called by __clk_reparent which is called by > clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. > > Does this not handle the old cached clk->rate for you? For the set_parent case, ops->recalc_rate() is called twice. Once for PRE_CHANGE and once for POST_CHANGE. For this clock, I can only really recalc the rate during the POST_CHANGE call. So, how should I differentiate the two cases? On a separate note: Sorry if I missed any earlier discussion on this, but what's the reason for calling recalc_rate() pre-change and post-change but without giving it the ability to differentiate between the two? I think it's quite useful for recalc_rate to be called pre/post change (some steps have to be done pre/post change depending on whether the parent rate is increasing or decreasing). But I don't see the "msg" being passed along. Also, I noticed that clk_set_parent() is treating a NULL as an invalid clock. Should that be fixed? set_parent(NULL) could be treated as a grounding the clock. Should we let the ops->set_parent determine if NULL is valid option? Thanks, Saravana
On Fri, Mar 23, 2012 at 3:12 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 03/23/2012 02:39 PM, Turquette, Mike wrote: >> __clk_recalc_rates is called by __clk_reparent which is called by >> clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. >> >> Does this not handle the old cached clk->rate for you? > > For the set_parent case, ops->recalc_rate() is called twice. Once for > PRE_CHANGE and once for POST_CHANGE. For this clock, I can only really > recalc the rate during the POST_CHANGE call. So, how should I differentiate > the two cases? .recalc_rate serves two purposes: first it recalculates the rate after the rate has changed and you pass in a new parent_rate argument. The second purpose is to "speculate" a rate change. You can pass in any rate for the parent_rate parameter when you call .recalc_rates. This is what __speculate_rates does before the rate changes. For clk_set_parent we call, __clk_speculate_rates(clk, parent->rate); Where parent above is the *new* parent. So this will let us propagate pre-rate change notifiers with the new rate. Your .recalc_rate callback doesn't need to differentiate between the two calls to .recalc_rate. It should just take in the parent_rate value and do the calculation required of it. Take a look at __clk_speculate_rates and __clk_recalc_rates and you'll see how to use it. > On a separate note: > Sorry if I missed any earlier discussion on this, but what's the reason for > calling recalc_rate() pre-change and post-change but without giving it the > ability to differentiate between the two? I think my answer above covers this. The .recalc_rate callback only exists to perform a hardware-specific calculation. The context of pre- or post-change is irrelevant since the parent_rate passed in could be the actual rate of the parent or a future rate of the parent. > I think it's quite useful for recalc_rate to be called pre/post change (some > steps have to be done pre/post change depending on whether the parent rate > is increasing or decreasing). But I don't see the "msg" being passed along. What kind of steps? Does your .recalc_rate perform these steps? I need more details to understand your requirements. > Also, I noticed that clk_set_parent() is treating a NULL as an invalid > clock. Should that be fixed? set_parent(NULL) could be treated as a > grounding the clock. Should we let the ops->set_parent determine if NULL is > valid option? We must be looking at different code. clk_set_parent doesn't return any error if parent == NULL. Bringing this to my attention does show that we do deref the parent pointer without a check though... Do you have a real use case for this? Due to the way that we match the parent pointer with the cached clk->parents member it would be painful to support NULL parents as valid. It is also worth considering whether clk_set_parent is really the correct operation for grounding a clock. clk_unprepare might be a better candidate. Regards, Mike
On 03/23/2012 03:32 PM, Turquette, Mike wrote: > On Fri, Mar 23, 2012 at 3:12 PM, Saravana Kannan<skannan@codeaurora.org> wrote: >> On 03/23/2012 02:39 PM, Turquette, Mike wrote: >>> __clk_recalc_rates is called by __clk_reparent which is called by >>> clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. >>> >>> Does this not handle the old cached clk->rate for you? >> >> For the set_parent case, ops->recalc_rate() is called twice. Once for >> PRE_CHANGE and once for POST_CHANGE. For this clock, I can only really >> recalc the rate during the POST_CHANGE call. So, how should I differentiate >> the two cases? > > .recalc_rate serves two purposes: first it recalculates the rate after > the rate has changed and you pass in a new parent_rate argument. The > second purpose is to "speculate" a rate change. You can pass in any > rate for the parent_rate parameter when you call .recalc_rates. This > is what __speculate_rates does before the rate changes. For > clk_set_parent we call, > > __clk_speculate_rates(clk, parent->rate); > > Where parent above is the *new* parent. So this will let us propagate > pre-rate change notifiers with the new rate. > > Your .recalc_rate callback doesn't need to differentiate between the > two calls to .recalc_rate. It should just take in the parent_rate > value and do the calculation required of it. Yeah, this is generally true. But, in this specific case, the clock is a mux that can literally measure the real rate. I would like the rate of this mux clock to be the real measured rate and not just the parent rate. I could actually measure the current rate and return that for recalc_rate(), but then the reported rate change during the pre-change notification would be wrong. Having the "msg" will let us at least fake the rate correctly for pre change notifiers. >> I think it's quite useful for recalc_rate to be called pre/post change (some >> steps have to be done pre/post change depending on whether the parent rate >> is increasing or decreasing). But I don't see the "msg" being passed along. > > What kind of steps? Does your .recalc_rate perform these steps? I > need more details to understand your requirements. Nevermind, my brain isn't working today. I can just do that different ordering in ops->set_parent(). But the measure clocks case is still true though. But this did bring up another point in my head. Hopefully this one isn't my brain playing tricks again today. How does a child (or grand child) clock (not a driver using the clock) reject a rate change if it know it can't handle that freq from the parent? I won't claim to know this part of the code thoroughly, but I can't find an easy way to do this. Reason for rejection could be things like the counter (for division, etc) has an upper limit on how fast it can run. >> Also, I noticed that clk_set_parent() is treating a NULL as an invalid >> clock. Should that be fixed? set_parent(NULL) could be treated as a >> grounding the clock. Should we let the ops->set_parent determine if NULL is >> valid option? > > We must be looking at different code. clk_set_parent doesn't return > any error if parent == NULL. Bringing this to my attention does show > that we do deref the parent pointer without a check though... > > Do you have a real use case for this? Due to the way that we match > the parent pointer with the cached clk->parents member it would be > painful to support NULL parents as valid. I don't have a real use case for MSM. We just have a ground_clock. > It is also worth considering whether clk_set_parent is really the > correct operation for grounding a clock. clk_unprepare might be a > better candidate. Yeah, not a real use case for MSM. Thanks, Saravana
On Fri, Mar 23, 2012 at 4:04 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 03/23/2012 03:32 PM, Turquette, Mike wrote: >> .recalc_rate serves two purposes: first it recalculates the rate after >> the rate has changed and you pass in a new parent_rate argument. The >> second purpose is to "speculate" a rate change. You can pass in any >> rate for the parent_rate parameter when you call .recalc_rates. This >> is what __speculate_rates does before the rate changes. For >> clk_set_parent we call, >> >> __clk_speculate_rates(clk, parent->rate); >> >> Where parent above is the *new* parent. So this will let us propagate >> pre-rate change notifiers with the new rate. >> >> Your .recalc_rate callback doesn't need to differentiate between the >> two calls to .recalc_rate. It should just take in the parent_rate >> value and do the calculation required of it. > > Yeah, this is generally true. But, in this specific case, the clock is a mux > that can literally measure the real rate. I would like the rate of this mux > clock to be the real measured rate and not just the parent rate. That's pretty cool hardware. Do you find that software-only tracking doesn't match up with sampling the frequency in hardware? If software tracking of the rate changes is correct then you could just skip using this hardware feature. > I could actually measure the current rate and return that for recalc_rate(), > but then the reported rate change during the pre-change notification would > be wrong. Having the "msg" will let us at least fake the rate correctly for > pre change notifiers. In previous series I had separate .recalc_rate and .speculate_rate callbacks. I merged them since they were almost identical. I'd prefer not to reintroduce them since it creates a burden on others to support seperate callbacks, and passing in the message is one way to solve that... I'll think on this a bit more. > How does a child (or grand child) clock (not a driver using the clock) > reject a rate change if it know it can't handle that freq from the parent? I > won't claim to know this part of the code thoroughly, but I can't find an > easy way to do this. Technically you could subscribe a notifier to your grand-child clock, even if there is no driver for it. The same code that implements your platform's clock operations could register the notifier handler. You can see how this works in clk_propagate_rate_change. That usage is certainly targeted more at drivers but could be made to work for your case. Let me know what you think; this is an interesting issue. > Reason for rejection could be things like the counter (for division, etc) > has an upper limit on how fast it can run. Are you hitting this in practice today? The clock framework shouldn't be a perfect piece of code that can validate every possible combination imaginable. Some burden falls on the code calling the clk api (especially if that code is platform code) to make sure that it doesn't do stupid things. >>> Also, I noticed that clk_set_parent() is treating a NULL as an invalid >>> clock. Should that be fixed? set_parent(NULL) could be treated as a >>> grounding the clock. Should we let the ops->set_parent determine if NULL >>> is >>> valid option? >> >> >> We must be looking at different code. clk_set_parent doesn't return >> any error if parent == NULL. Bringing this to my attention does show >> that we do deref the parent pointer without a check though... >> >> Do you have a real use case for this? Due to the way that we match >> the parent pointer with the cached clk->parents member it would be >> painful to support NULL parents as valid. > > I don't have a real use case for MSM. We just have a ground_clock. I'm electing to ignore the issue until we have a real use-case for it. Regards, Mike
On 03/23/2012 04:28 PM, Turquette, Mike wrote: > On Fri, Mar 23, 2012 at 4:04 PM, Saravana Kannan<skannan@codeaurora.org> wrote: >> On 03/23/2012 03:32 PM, Turquette, Mike wrote: >>> .recalc_rate serves two purposes: first it recalculates the rate after >>> the rate has changed and you pass in a new parent_rate argument. The >>> second purpose is to "speculate" a rate change. You can pass in any >>> rate for the parent_rate parameter when you call .recalc_rates. This >>> is what __speculate_rates does before the rate changes. For >>> clk_set_parent we call, >>> >>> __clk_speculate_rates(clk, parent->rate); >>> >>> Where parent above is the *new* parent. So this will let us propagate >>> pre-rate change notifiers with the new rate. >>> >>> Your .recalc_rate callback doesn't need to differentiate between the >>> two calls to .recalc_rate. It should just take in the parent_rate >>> value and do the calculation required of it. >> >> Yeah, this is generally true. But, in this specific case, the clock is a mux >> that can literally measure the real rate. I would like the rate of this mux >> clock to be the real measured rate and not just the parent rate. > > That's pretty cool hardware. Do you find that software-only tracking > doesn't match up with sampling the frequency in hardware? If software > tracking of the rate changes is correct then you could just skip using > this hardware feature. We use this HW for automated testing of the SW. It's very handy. So, I definitely need to support it. >> I could actually measure the current rate and return that for recalc_rate(), >> but then the reported rate change during the pre-change notification would >> be wrong. Having the "msg" will let us at least fake the rate correctly for >> pre change notifiers. > > In previous series I had separate .recalc_rate and .speculate_rate > callbacks. I merged them since they were almost identical. I'd > prefer not to reintroduce them since it creates a burden on others to > support seperate callbacks, and passing in the message is one way to > solve that... I'll think on this a bit more. For this specific clock, I don't think anyone is really going to care if the pre rate change notification is correct. So, I will just go with reporting the wrong rate during pre-change for now. But it does increase the total testing time quite a bit as I have to measure the real rate during pre and post change and measurement is relatively slow. It actually does add up into minutes when the whole test suite is run. So, would be nice to have this fixed eventually but it's not urgent for this measure clock. >> How does a child (or grand child) clock (not a driver using the clock) >> reject a rate change if it know it can't handle that freq from the parent? I >> won't claim to know this part of the code thoroughly, but I can't find an >> easy way to do this. > > Technically you could subscribe a notifier to your grand-child clock, > even if there is no driver for it. The same code that implements your > platform's clock operations could register the notifier handler. > > You can see how this works in clk_propagate_rate_change. That usage > is certainly targeted more at drivers but could be made to work for > your case. Let me know what you think; this is an interesting issue. I think notifications should be left to drivers. Notifications are too heavy handed for enforcing requirements of the clock tree. Also, clk_hw to clk might no longer be a 1-1 mapping in the future. It's quite possible that each clk_get() would get a different struct clk based on the caller to keep track of their constraints separately. So, clock drivers shouldn't ever use them to identify a clock. >> Reason for rejection could be things like the counter (for division, etc) >> has an upper limit on how fast it can run. > > Are you hitting this in practice today? Such HW limitations are real. But we don't use clk_set_parent() in our drivers often. > The clock framework shouldn't > be a perfect piece of code that can validate every possible > combination imaginable. Some burden falls on the code calling the clk > api (especially if that code is platform code) to make sure that it > doesn't do stupid things. We can't expect a normal driver to know that the clk_set_parent() shouldn't be called when the parent is at a particular rate since there are clock HW limitations. The clock framework doesn't need to validate everything, but it should give the clock driver the option of rejecting invalid requests. Btw, I think this earlier comment from me is wrong. > Nevermind, my brain isn't working today. I can just do that different > ordering in ops->set_parent(). I think there is still a problem with not being able to differentiate between pre-change recalc and post-change recalc. This applies for any set parent and set rate on a clock that has children. Consider this simple example: * Divider clock B is fed from clock A. * Clock B can never output > 120 MHz due to downstream HW/clock limitations. * Clock A is running at 200 MHz * Clock B divider is set to 2. Now, say the rate of clock A is changing from 200 MHz to 300 MHz (due to set rate or set parent). In this case, the clock B divider should be set to 3 pre-rate change to guarantee that the output of clock B is never > 120 MHz. So the rate of clock B will go from 100 MHz (200/2) to 66 MHz (200/3) to 100 MHz (300/3) and everything is good. Assume we somehow managed to do the above. So, now clock A is at 300 MHz, clock B divider is at 3 and the clock B output is 100 MHz. Now, say the rate of clock A changes from 300 MHz to 100 MHz. In this case the clock B divider should only be changed post rate change. If we do it pre rate change, then the output will go from 100 MHz (300/3) to 150 MHz (300/1) to 100 MHz (100/1). We went past the 120 MHz limit of clock B's output rate. If we do this post rate change, we can do this without exceeding the max output limit of clock B. It will go from 100 MHz (300/3) to 33 MHz (100/3) to 100 MHz (100/1). We never went past the 120 MHz limit. So, at least for this reason above, I think we need to pass a pre/post parameter to the recalc ops. While we are at it, we should probably just add a failure option for recalc to make it easy to reject unacceptable rate changes. To keep the clock framework code simpler, you could decide to allow errors only for the pre-change recalc calls. That way, the error case roll back code won't be crazy. >>> Do you have a real use case for this? Due to the way that we match >>> the parent pointer with the cached clk->parents member it would be >>> painful to support NULL parents as valid. >> >> I don't have a real use case for MSM. We just have a ground_clock. > > I'm electing to ignore the issue until we have a real use-case for it. Sounds reasonable. Thanks, Saravana
On Tue, Mar 27, 2012 at 8:06 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 03/23/2012 04:28 PM, Turquette, Mike wrote: >> On Fri, Mar 23, 2012 at 4:04 PM, Saravana Kannan<skannan@codeaurora.org> >> wrote: >>> On 03/23/2012 03:32 PM, Turquette, Mike wrote: >>> How does a child (or grand child) clock (not a driver using the clock) >>> reject a rate change if it know it can't handle that freq from the >>> parent? I >>> won't claim to know this part of the code thoroughly, but I can't find an >>> easy way to do this. >> >> >> Technically you could subscribe a notifier to your grand-child clock, >> even if there is no driver for it. The same code that implements your >> platform's clock operations could register the notifier handler. > >> >> >> You can see how this works in clk_propagate_rate_change. That usage >> is certainly targeted more at drivers but could be made to work for >> your case. Let me know what you think; this is an interesting issue. > > > I think notifications should be left to drivers. Notifications are too heavy > handed for enforcing requirements of the clock tree. Agreed. I'm working on a "clock dependency" design at the moment, which should hopefully answer your question. > Also, clk_hw to clk > might no longer be a 1-1 mapping in the future. It's quite possible that > each clk_get() would get a different struct clk based on the caller to keep > track of their constraints separately. So, clock drivers shouldn't ever use > them to identify a clock. I'm also working on this same thing! Lots to keep me busy these days. snip > I think there is still a problem with not being able to differentiate > between pre-change recalc and post-change recalc. This applies for any set > parent and set rate on a clock that has children. > > Consider this simple example: > * Divider clock B is fed from clock A. > * Clock B can never output > 120 MHz due to downstream > HW/clock limitations. > * Clock A is running at 200 MHz > * Clock B divider is set to 2. > > Now, say the rate of clock A is changing from 200 MHz to 300 MHz (due to set > rate or set parent). In this case, the clock B divider should be set to 3 > pre-rate change to guarantee that the output of clock B is never > 120 MHz. > So the rate of clock B will go from 100 MHz (200/2) to 66 MHz (200/3) to 100 > MHz (300/3) and everything is good. > > Assume we somehow managed to do the above. So, now clock A is at 300 MHz, > clock B divider is at 3 and the clock B output is 100 MHz. > > Now, say the rate of clock A changes from 300 MHz to 100 MHz. In this case > the clock B divider should only be changed post rate change. If we do it pre > rate change, then the output will go from 100 MHz (300/3) to 150 MHz (300/1) > to 100 MHz (100/1). We went past the 120 MHz limit of clock B's output rate. > > If we do this post rate change, we can do this without exceeding the max > output limit of clock B. It will go from 100 MHz (300/3) to 33 MHz (100/3) > to 100 MHz (100/1). We never went past the 120 MHz limit. > > So, at least for this reason above, I think we need to pass a pre/post > parameter to the recalc ops. > > While we are at it, we should probably just add a failure option for recalc > to make it easy to reject unacceptable rate changes. To keep the clock > framework code simpler, you could decide to allow errors only for the > pre-change recalc calls. That way, the error case roll back code won't be > crazy. recalc is too late to catch this. I think you mean round_rate. We want to determine which rate changes are out-of-spec during clk_calc_new_rates (for clk_set_rate) which involves round_rate being a bit smarter about what it can and cannot do. Anyways I'm looking at ways to do this in my clk-dependencies branch. Regards, Mike
On 03/28/2012 10:08 AM, Turquette, Mike wrote: > On Tue, Mar 27, 2012 at 8:06 PM, Saravana Kannan<skannan@codeaurora.org> wrote: snip >> I think there is still a problem with not being able to differentiate >> between pre-change recalc and post-change recalc. This applies for any set >> parent and set rate on a clock that has children. >> >> Consider this simple example: >> * Divider clock B is fed from clock A. >> * Clock B can never output> 120 MHz due to downstream >> HW/clock limitations. >> * Clock A is running at 200 MHz >> * Clock B divider is set to 2. >> >> Now, say the rate of clock A is changing from 200 MHz to 300 MHz (due to set >> rate or set parent). In this case, the clock B divider should be set to 3 >> pre-rate change to guarantee that the output of clock B is never> 120 MHz. >> So the rate of clock B will go from 100 MHz (200/2) to 66 MHz (200/3) to 100 >> MHz (300/3) and everything is good. >> >> Assume we somehow managed to do the above. So, now clock A is at 300 MHz, >> clock B divider is at 3 and the clock B output is 100 MHz. >> >> Now, say the rate of clock A changes from 300 MHz to 100 MHz. In this case >> the clock B divider should only be changed post rate change. If we do it pre >> rate change, then the output will go from 100 MHz (300/3) to 150 MHz (300/1) >> to 100 MHz (100/1). We went past the 120 MHz limit of clock B's output rate. >> >> If we do this post rate change, we can do this without exceeding the max >> output limit of clock B. It will go from 100 MHz (300/3) to 33 MHz (100/3) >> to 100 MHz (100/1). We never went past the 120 MHz limit. >> >> So, at least for this reason above, I think we need to pass a pre/post >> parameter to the recalc ops. Sorry if I wasn't clear. But the case above is a separate issue from what I mention below. What are your thoughts on handling this? Pass "msg" to recalc_rates? >> While we are at it, we should probably just add a failure option for recalc >> to make it easy to reject unacceptable rate changes. To keep the clock >> framework code simpler, you could decide to allow errors only for the >> pre-change recalc calls. That way, the error case roll back code won't be >> crazy. > > recalc is too late to catch this. I think you mean round_rate. We > want to determine which rate changes are out-of-spec during > clk_calc_new_rates (for clk_set_rate) which involves round_rate being > a bit smarter about what it can and cannot do. The case I'm referring to is set_parent(). set_rate() and set_parent() have a lot of common implications and it doesn't look like the clock framework is handling the common cases the same way for both set_parent() and set_rate(). It almost feels like set_parent() and set_rate() should just be wrappers around some common function that handles most of the work. After all, set_parent() is just a slimmed down version of set_rate(). Set rate is just a combination of set parent and set divider. > > Anyways I'm looking at ways to do this in my clk-dependencies branch. > Are you also looking into the pre/post rate change divider handling case I mentioned above? Thanks, Saravana
On Wed, Mar 28, 2012 at 3:25 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 03/28/2012 10:08 AM, Turquette, Mike wrote: >> On Tue, Mar 27, 2012 at 8:06 PM, Saravana Kannan<skannan@codeaurora.org> >> wrote: >>> I think there is still a problem with not being able to differentiate >>> between pre-change recalc and post-change recalc. This applies for any >>> set >>> parent and set rate on a clock that has children. >>> >>> Consider this simple example: >>> * Divider clock B is fed from clock A. >>> * Clock B can never output> 120 MHz due to downstream >>> HW/clock limitations. >>> * Clock A is running at 200 MHz >>> * Clock B divider is set to 2. >>> >>> Now, say the rate of clock A is changing from 200 MHz to 300 MHz (due to >>> set >>> rate or set parent). In this case, the clock B divider should be set to 3 >>> pre-rate change to guarantee that the output of clock B is never> 120 >>> MHz. >>> So the rate of clock B will go from 100 MHz (200/2) to 66 MHz (200/3) to >>> 100 >>> MHz (300/3) and everything is good. >>> >>> Assume we somehow managed to do the above. So, now clock A is at 300 MHz, >>> clock B divider is at 3 and the clock B output is 100 MHz. >>> >>> Now, say the rate of clock A changes from 300 MHz to 100 MHz. In this >>> case >>> the clock B divider should only be changed post rate change. If we do it >>> pre >>> rate change, then the output will go from 100 MHz (300/3) to 150 MHz >>> (300/1) >>> to 100 MHz (100/1). We went past the 120 MHz limit of clock B's output >>> rate. >>> >>> If we do this post rate change, we can do this without exceeding the max >>> output limit of clock B. It will go from 100 MHz (300/3) to 33 MHz >>> (100/3) >>> to 100 MHz (100/1). We never went past the 120 MHz limit. >>> >>> So, at least for this reason above, I think we need to pass a pre/post >>> parameter to the recalc ops. > > Sorry if I wasn't clear. But the case above is a separate issue from what I > mention below. What are your thoughts on handling this? Pass "msg" to > recalc_rates? I'm OK passing the msg to in .recalc_rates for your hardware which samples frequency. >>> While we are at it, we should probably just add a failure option for >>> recalc >>> to make it easy to reject unacceptable rate changes. To keep the clock >>> framework code simpler, you could decide to allow errors only for the >>> pre-change recalc calls. That way, the error case roll back code won't be >>> crazy. We do need a rate validation mechanism, which neither .round_rate or .recalc_rates provide. But I don't want to bolt one on right now because the proper solution to that problem is not a quick fix. Coupled to this issue are lingering topics that we've discussed tentatively in the past: 1) rate ranges (min/max) 2) clock dependencies; these should enforce functional dependencies between clocks 3) intended rates; this strays into the scary arena of policy, which has traditionally fallen outside the purview of the clock framework. But if we want a better clock framework we probably need a way for downstream clocks to make intelligent decisions about their rates when parent rates change. We could overload .round_rate to do this for clk_set_rate, but that doesn't help out clk_set_parent. And I'm against overloading the functionality of these callbacks any more than I already have. I'm not sure that just overloading .recalc_rates is the best approach. I like that .recalc_rates only recalculates the rate! For clocks that need to make an intelligent decision about their rates when parent rates change we might need to introduce a new callback and/or refactor the clk_set_rate and clk_set_parent paths to use some common code. >> recalc is too late to catch this. I think you mean round_rate. We >> want to determine which rate changes are out-of-spec during >> clk_calc_new_rates (for clk_set_rate) which involves round_rate being >> a bit smarter about what it can and cannot do. > > > The case I'm referring to is set_parent(). set_rate() and set_parent() have > a lot of common implications and it doesn't look like the clock framework is > handling the common cases the same way for both set_parent() and set_rate(). Agreed that there needs to be some unification here. Certainly a common rate validation mechanism is missing and the downstream notification diverged in the last set of patches. > It almost feels like set_parent() and set_rate() should just be wrappers > around some common function that handles most of the work. After all, > set_parent() is just a slimmed down version of set_rate(). Set rate is just > a combination of set parent and set divider. I mostly agree. A big caveat is that clk_set_parent will never propagate a request up the tree, but that doesn't mean the implementation details of the two functions are mutually exclusive. >> Anyways I'm looking at ways to do this in my clk-dependencies branch. >> > > Are you also looking into the pre/post rate change divider handling case I > mentioned above? I'll admit that my initial focus on clk-deps was more along the lines of: "Clock A wants to go faster, and has a functional dependency to scale Clock B to a faster rate first" That sort of situation is often necessary to keep certain timing requirements valid. But your example reinforces that clocks need to have some clue about rate ranges, and it resets my opinion on writing a clk-dep mechanism which is just focused on raising rates. The real solution should be generic and able to handle lowering and NACKing rates also. In short: for now I'll just pass the msg into .recalc_rates. The other stuff is a design activity which is out-of-scope for the upcoming -rc bugfix windows. Regards, Mike
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index d5ac6a7..6bd8037 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -119,7 +119,8 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate, } EXPORT_SYMBOL_GPL(clk_divider_round_rate); -static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate) +static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) { struct clk_divider *divider = to_clk_divider(hw); unsigned int div; diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index dbc310a..d74ac4b 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -833,17 +833,18 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even static void clk_change_rate(struct clk *clk) { struct clk *child; - unsigned long old_rate; + unsigned long old_rate, parent_rate = 0; struct hlist_node *tmp; old_rate = clk->rate; + if (clk->parent) + parent_rate = clk->parent->rate; if (clk->ops->set_rate) - clk->ops->set_rate(clk->hw, clk->new_rate); + clk->ops->set_rate(clk->hw, clk->new_rate, parent_rate); if (clk->ops->recalc_rate) - clk->rate = clk->ops->recalc_rate(clk->hw, - clk->parent->rate); + clk->rate = clk->ops->recalc_rate(clk->hw, parent_rate); else clk->rate = clk->parent->rate; diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 5508897..1031f1f 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -125,7 +125,8 @@ struct clk_ops { unsigned long *); int (*set_parent)(struct clk_hw *hw, u8 index); u8 (*get_parent)(struct clk_hw *hw); - int (*set_rate)(struct clk_hw *hw, unsigned long); + int (*set_rate)(struct clk_hw *hw, unsigned long, + unsigned long); void (*init)(struct clk_hw *hw); }; --->8 Then with parent rate passed into .round_rate and .set_rate like what we do with .recalc_rate right now, we can save particular clk implementation from calling __clk_get_parent() and __clk_get_rate to get parent rate. For example, the following will the be diff we gain on clk-divider. 8<--- diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 6bd8037..8a28ffb 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -68,8 +68,8 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate, if (divider->flags & CLK_DIVIDER_ONE_BASED) maxdiv--; - if (!best_parent_rate) { - parent_rate = __clk_get_rate(__clk_get_parent(hw->clk)); + if (!(divider->flags & CLK_SET_RATE_PARENT)) { + parent_rate = *best_parent_rate; bestdiv = DIV_ROUND_UP(parent_rate, rate); bestdiv = bestdiv == 0 ? 1 : bestdiv; bestdiv = bestdiv > maxdiv ? maxdiv : bestdiv; @@ -109,13 +109,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate, int div; div = clk_divider_bestdiv(hw, rate, prate); - if (prate) - return *prate / div; - else { - unsigned long r; - r = __clk_get_rate(__clk_get_parent(hw->clk)); - return r / div; - } + return *prate / div; } EXPORT_SYMBOL_GPL(clk_divider_round_rate); @@ -127,7 +121,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long flags = 0; u32 val; - div = __clk_get_rate(__clk_get_parent(hw->clk)) / rate; + div = parent_rate / rate; if (!(divider->flags & CLK_DIVIDER_ONE_BASED)) div--;