Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/28/2012 10:08 AM, Turquette, Mike wrote: On Tue, Mar 27, 2012 at 8:06 PM, Saravana Kannanskan...@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 -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/23/2012 04:28 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 4:04 PM, Saravana Kannanskan...@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
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Tue, Mar 27, 2012 at 8:06 PM, Saravana Kannan skan...@codeaurora.org wrote: On 03/23/2012 04:28 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 4:04 PM, Saravana Kannanskan...@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 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Wed, Mar 28, 2012 at 3:25 PM, Saravana Kannan skan...@codeaurora.org wrote: On 03/28/2012 10:08 AM, Turquette, Mike wrote: On Tue, Mar 27, 2012 at 8:06 PM, Saravana Kannanskan...@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
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Fri, Mar 23, 2012 at 4:04 PM, Saravana Kannan skan...@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 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
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 -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/23/2012 02:39 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 2:33 PM, Saravana Kannanskan...@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 -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/23/2012 02:39 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 2:33 PM, Saravana Kannanskan...@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 -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/23/2012 03:32 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 3:12 PM, Saravana Kannanskan...@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 -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
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
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/20/2012 04:53 PM, Turquette, Mike wrote:
On Tue, Mar 20, 2012 at 10:46 AM, Saravana Kannan
skan...@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
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
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.
+ 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:
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
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);
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Tue, Mar 20, 2012 at 7:02 AM, Shawn Guo shawn@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)
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Tue, Mar 20, 2012 at 10:46 AM, Saravana Kannan
skan...@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
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 21 March 2012 07:46, Turquette, Mike mturque...@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 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
Hi Mike,
On Friday 16 March 2012 11:41 AM, Mike Turquette wrote:
The common clock framework defines a common struct clk useful across
most platforms as well as an implementation of the clk api that drivers
can use safely for managing clocks.
The net result is consolidation of many different struct clk definitions
and platform-specific clock framework implementations.
This patch introduces the common struct clk, struct clk_ops and an
implementation of the well-known clock api in include/clk/clk.h.
Platforms may define their own hardware-specific clock structure and
their own clock operation callbacks, so long as it wraps an instance of
struct clk_hw.
See Documentation/clk.txt for more details.
+/*
+ * calculate the new rates returning the topmost clock that has to be
+ * changed.
+ */
+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;
Shouldn't you check for a valid parent before dereferencing it? A
clk_set_rate() on a root clock might throw up some issues otherwise.
+ unsigned long new_rate;
+
+ if (!clk-ops-round_rate !(clk-flags CLK_SET_RATE_PARENT)) {
+ clk-new_rate = clk-rate;
+ return NULL;
So does this mean a clk_set_rate() fails for a clk which does not have
a valid .round_rate and does not have a CLK_SET_RATE_PARENT flag set?
I was thinking this could do a..
clk-new_rate = rate;
top = clk;
goto out;
..instead.
regards,
Rajendra
+ }
+
+ if (!clk-ops-round_rate (clk-flags CLK_SET_RATE_PARENT)) {
+ top = clk_calc_new_rates(clk-parent, rate);
+ new_rate = clk-new_rate = clk-parent-new_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);
+
+ if (best_parent_rate != clk-parent-rate) {
+ top = clk_calc_new_rates(clk-parent, best_parent_rate);
+
+ goto out;
+ }
+
+out:
+ clk_calc_subtree(clk, new_rate);
+
+ return top;
+}
+
+/*
+ * Notify about rate changes in a subtree. Always walk down the whole tree
+ * so that in case of an error we can walk down the whole tree again and
+ * abort the change.
+ */
+static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long
event)
+{
+ struct hlist_node *tmp;
+ struct clk *child, *fail_clk = NULL;
+ int ret = NOTIFY_DONE;
+
+ if (clk-rate == clk-new_rate)
+ return 0;
+
+ if (clk-notifier_count) {
+ ret = __clk_notify(clk, event, clk-rate, clk-new_rate);
+ if (ret == NOTIFY_BAD)
+ fail_clk = clk;
+ }
+
+ hlist_for_each_entry(child, tmp,clk-children, child_node) {
+ clk = clk_propagate_rate_change(child, event);
+ if (clk)
+ fail_clk = clk;
+ }
+
+ return fail_clk;
+}
+
+/*
+ * walk down a subtree and set the new rates notifying the rate
+ * change on the way
+ */
+static void clk_change_rate(struct clk *clk)
+{
+ struct clk *child;
+ unsigned long old_rate;
+ struct hlist_node *tmp;
+
+ old_rate = clk-rate;
+
+ if (clk-ops-set_rate)
+ clk-ops-set_rate(clk-hw, clk-new_rate);
+
+ if (clk-ops-recalc_rate)
+ clk-rate = clk-ops-recalc_rate(clk-hw,
+ clk-parent-rate);
+ else
+ clk-rate = clk-parent-rate;
+
+ if (clk-notifier_count old_rate != clk-rate)
+ __clk_notify(clk, POST_RATE_CHANGE, old_rate, clk-rate);
+
+ hlist_for_each_entry(child, tmp,clk-children, child_node)
+ clk_change_rate(child);
+}
+
+/**
+ * clk_set_rate - specify a new rate for clk
+ * @clk: the clk whose rate is being changed
+ * @rate: the new rate for clk
+ *
+ * In the simplest case clk_set_rate will only change the rate of clk.
+ *
+ * If clk has the CLK_SET_RATE_GATE flag set and it is enabled this call
+ * will fail; only when the clk is disabled will it be able to change
+ * its rate.
+ *
+ * Setting the CLK_SET_RATE_PARENT flag allows clk_set_rate to
+ * recursively propagate up to clk's parent; whether or not this happens
+ * depends on the outcome of clk's .round_rate implementation. If
+ * *parent_rate is 0 after calling .round_rate then upstream parent
+ * propagation is ignored. If *parent_rate comes back with a new rate
+ * for clk's parent then we propagate up to clk's parent and set it's
+ * rate. Upward propagation will continue until either a clk does not
+ * support the CLK_SET_RATE_PARENT flag or .round_rate stops requesting
+ * changes to clk's parent_rate. If there is a failure during upstream
+ * propagation then clk_set_rate will unwind
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Mon, Mar 19, 2012 at 04:52:05PM +0530, Rajendra Nayak wrote:
Hi Mike,
+/*
+ * calculate the new rates returning the topmost clock that has to be
+ * changed.
+ */
+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;
Shouldn't you check for a valid parent before dereferencing it? A
clk_set_rate() on a root clock might throw up some issues otherwise.
Yes, should be checked.
+unsigned long new_rate;
+
+if (!clk-ops-round_rate !(clk-flags CLK_SET_RATE_PARENT)) {
+clk-new_rate = clk-rate;
+return NULL;
So does this mean a clk_set_rate() fails for a clk which does not have
a valid .round_rate and does not have a CLK_SET_RATE_PARENT flag set?
I was thinking this could do a..
clk-new_rate = rate;
top = clk;
goto out;
..instead.
The core should make sure that either both set_rate and round_rate are
present or none of them.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Fri, Mar 16, 2012 at 8:28 PM, Saravana Kannan skan...@codeaurora.org wrote: On 03/15/2012 11:11 PM, Mike Turquette wrote: The common clock framework defines a common struct clk useful across most platforms as well as an implementation of the clk api that drivers can use safely for managing clocks. The net result is consolidation of many different struct clk definitions and platform-specific clock framework implementations. This patch introduces the common struct clk, struct clk_ops and an implementation of the well-known clock api in include/clk/clk.h. Platforms may define their own hardware-specific clock structure and their own clock operation callbacks, so long as it wraps an instance of struct clk_hw. See Documentation/clk.txt for more details. This patch is based on the work of Jeremy Kerr, which in turn was based on the work of Ben Herrenschmidt. Signed-off-by: Mike Turquettemturque...@linaro.org Signed-off-by: Mike Turquettemturque...@ti.com Reviewed-by: Thomas Gleixnert...@linutronix.de Tested-by: Andrew Lunnand...@lunn.ch Reviewed-by: Rob Herringrob.herringat calxeda.com Cc: Russell Kingli...@arm.linux.org.uk Cc: Jeremy Kerrjeremy.k...@canonical.com Cc: Arnd Bergmanarnd.bergm...@linaro.org Cc: Paul Walmsleyp...@pwsan.com Cc: Shawn Guoshawn@freescale.com Cc: Sascha Hauers.ha...@pengutronix.de Cc: Richard Zhaorichard.z...@linaro.org Cc: Saravana Kannanskan...@codeaurora.org Cc: Magnus Dammmagnus.d...@gmail.com Cc: Mark Brownbroo...@opensource.wolfsonmicro.com Cc: Linus Walleijlinus.wall...@stericsson.com Cc: Stephen Boydsb...@codeaurora.org Cc: Amit Kucheriaamit.kuche...@linaro.org Cc: Deepak Saxenadsax...@linaro.org Cc: Grant Likelygrant.lik...@secretlab.ca --- Mike, Thanks for the patches! Glad to see that it's finally getting in! I sent a request for a minor change as a reply to the v5 series (since it had more context). Can you please take a look at that and let me know if you can send out a v8 or a patch on top of this to do that? Hi Saravana, I'm not sending a v8 series since Arnd has taken in v7 for the 3.4 merge window. I'm formulating a reply to your v5 queries, but I'm not done looking at the implications of the initializer stuff. Lets keep the technical discussion in that thread for now. Regards, 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. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Sun, Mar 18, 2012 at 6:46 AM, Shawn Guo shawn@linaro.org wrote: Reading the documentation of function clk_set_rate(), I'm not sure it exactly matches what the code does. If there is mismatch, it might be worth sending an incremental patch to update the documentation and avoid the confusion? The clk_set_rate code did change rapidly leading up to the merge request, and updating the documentation slipped through the cracks. I'll cook up a patch and it can probably go in one of the -rc's for 3.4. I'll take in all of your comments below. Thanks, Mike On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote: +/** + * clk_set_rate - specify a new rate for clk + * @clk: the clk whose rate is being changed + * @rate: the new rate for clk + * + * In the simplest case clk_set_rate will only change the rate of clk. + * + * If clk has the CLK_SET_RATE_GATE flag set and it is enabled this call + * will fail; only when the clk is disabled will it be able to change + * its rate. + * + * Setting the CLK_SET_RATE_PARENT flag allows clk_set_rate to + * recursively propagate up to clk's parent; whether or not this happens + * depends on the outcome of clk's .round_rate implementation. If + * *parent_rate is 0 after calling .round_rate then upstream parent Might *parent_rate is not changed be more accurate? + * propagation is ignored. If *parent_rate comes back with a new rate + * for clk's parent then we propagate up to clk's parent and set it's + * rate. Upward propagation will continue until either a clk does not + * support the CLK_SET_RATE_PARENT flag or .round_rate stops requesting + * changes to clk's parent_rate. + * If there is a failure during upstream + * propagation then clk_set_rate will unwind and restore each clk's rate + * that had been successfully changed. Afterwards a rate change abort + * notification will be propagated downstream, starting from the clk + * that failed. I'm not sure this part still matches the code. + * + * At the end of all of the rate setting, clk_set_rate internally calls + * __clk_recalc_rates and propagates the rate changes downstream, I do not see __clk_recalc_rates is called by clk_set_rate in any way. Regards, Shawn + * starting from the highest clk whose rate was changed. This has the + * added benefit of propagating post-rate change notifiers. + * + * Note that while post-rate change and rate change abort notifications + * are guaranteed to be sent to a clk only once per call to + * clk_set_rate, pre-change notifications will be sent for every clk + * whose rate is changed. Stacking pre-change notifications is noisy + * for the drivers subscribed to them, but this allows drivers to react + * to intermediate clk rate changes up until the point where the final + * rate is achieved at the end of upstream propagation. + * + * Returns 0 on success, -EERROR otherwise. + */ ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Sun, Mar 18, 2012 at 7:07 AM, Shawn Guo shawn@linaro.org wrote:
Another trivial comment. But if there is an incremental patch, maybe
consider to include it.
On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote:
...
+#ifdef CONFIG_COMMON_CLK_DISABLE_UNUSED
+static int clk_disable_unused(void)
+{
+ struct clk *clk;
+ struct hlist_node *tmp;
+
+ mutex_lock(prepare_lock);
+
+ hlist_for_each_entry(clk, tmp, clk_root_list, child_node)
+ clk_disable_unused_subtree(clk);
+
+ hlist_for_each_entry(clk, tmp, clk_orphan_list, child_node)
+ clk_disable_unused_subtree(clk);
+
+ mutex_unlock(prepare_lock);
+
+ return 0;
+}
+late_initcall(clk_disable_unused);
+#else
+static inline int clk_disable_unused(struct clk *clk) { return 0; }
This #else block seems completely unnecessary to me.
+#endif /* CONFIG_COMMON_CLK_DISABLE_UNUSED */
Oops. This is a leftover from when there was a separate
drivers/clk/clk-debug.c which implemented this stuff. I'll make a
cleanup series and roll all these little things into it.
Thanks,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Mon, Mar 19, 2012 at 4:28 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Mon, Mar 19, 2012 at 04:52:05PM +0530, Rajendra Nayak wrote:
Hi Mike,
+/*
+ * calculate the new rates returning the topmost clock that has to be
+ * changed.
+ */
+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;
Shouldn't you check for a valid parent before dereferencing it? A
clk_set_rate() on a root clock might throw up some issues otherwise.
Yes, should be checked.
The clk_calc_new_rates code assumes a valid parent pointer in several
locations. Thanks for the catch Rajendra. Will roll into my fixes
series.
+ unsigned long new_rate;
+
+ if (!clk-ops-round_rate !(clk-flags CLK_SET_RATE_PARENT)) {
+ clk-new_rate = clk-rate;
+ return NULL;
So does this mean a clk_set_rate() fails for a clk which does not have
a valid .round_rate and does not have a CLK_SET_RATE_PARENT flag set?
I was thinking this could do a..
clk-new_rate = rate;
top = clk;
goto out;
..instead.
The core should make sure that either both set_rate and round_rate are
present or none of them.
Agreed. The documentation covers which clk_ops are hard dependencies
(based on supported operations), but the code doesn't strictly check
this. I'll add a small state machine to __clk_init which validates
that .round_rate, .recalc_rate and .set_rate are *all* present if any
one of them are present, and present a WARN if otherwise.
Thanks,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Mon, Mar 19, 2012 at 12:13 PM, Saravana Kannan skan...@codeaurora.org wrote: I saw some responses from you over the weekend but not to mine. So, I assumed you were busy with other stuff and I started working on a patch on top of v7. I only answer trivial emails on the weekend ;-) I will send that out if I get around to finishing it before you do. Hope that's alright with you. I'm happy to for you to take a crack at it. I don't know what your implementation looks like, but here are a couple concerns I have: 1) if you're copying the data from the initializer over to the struct clk then make sure you handle the __init data aspects of it properly 2) are the members of struct clk_hw visible to the rest of the world? Are they modifiable (i.e., not const)? This is undesirable. Thanks, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Mon, Mar 19, 2012 at 4:22 AM, Rajendra Nayak rna...@ti.com wrote:
On Friday 16 March 2012 11:41 AM, Mike Turquette wrote:
+/*
+ * calculate the new rates returning the topmost clock that has to be
+ * changed.
+ */
+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;
Shouldn't you check for a valid parent before dereferencing it? A
clk_set_rate() on a root clock might throw up some issues otherwise.
Yes, the clk_calc_new_rates code assumes a valid parent pointer in
several locations. Thanks for the catch. Will roll into my fixes
series.
Regards,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/19/2012 12:33 PM, Turquette, Mike wrote: On Mon, Mar 19, 2012 at 12:13 PM, Saravana Kannan skan...@codeaurora.org wrote: I saw some responses from you over the weekend but not to mine. So, I assumed you were busy with other stuff and I started working on a patch on top of v7. I only answer trivial emails on the weekend ;-) I will send that out if I get around to finishing it before you do. Hope that's alright with you. I'm happy to for you to take a crack at it. I don't know what your implementation looks like, but here are a couple concerns I have: 1) if you're copying the data from the initializer over to the struct clk then make sure you handle the __init data aspects of it properly Not copying them into clk again. Just leaving them in clk_hw and using them as is. The only fields moved into clk_hw are: + const char *name; + const struct clk_ops*ops; + char**parent_names; + u8 num_parents; + unsigned long flags; 2) are the members of struct clk_hw visible to the rest of the world? Are they modifiable (i.e., not const)? This is undesirable. I'm leaving the const as is. Some of them you can't mark as const if you want to dynamically create them. Yes, they are visible to all the platform drivers (not the rest of the world). But these values are provided by the platform drivers anyway, so I don't see a problem with it. I also don't see any useful hacks a platform driver can do by messing with this fields without crashing the kernel since they don't have access to the locks. flags might be the one that provides some possibilities since I think you look at them often in the core code. We could just copy it into clk if people really feel strongly about it. At the worst case, we can have a full copy of all these fields inside clk and copy all of them over, but I think that would be overkill for things like names, ops and parent info. 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. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/19/2012 11:56 AM, Turquette, Mike wrote: On Fri, Mar 16, 2012 at 8:28 PM, Saravana Kannanskan...@codeaurora.org wrote: On 03/15/2012 11:11 PM, Mike Turquette wrote: The common clock framework defines a common struct clk useful across most platforms as well as an implementation of the clk api that drivers can use safely for managing clocks. The net result is consolidation of many different struct clk definitions and platform-specific clock framework implementations. This patch introduces the common struct clk, struct clk_ops and an implementation of the well-known clock api in include/clk/clk.h. Platforms may define their own hardware-specific clock structure and their own clock operation callbacks, so long as it wraps an instance of struct clk_hw. See Documentation/clk.txt for more details. This patch is based on the work of Jeremy Kerr, which in turn was based on the work of Ben Herrenschmidt. Signed-off-by: Mike Turquettemturque...@linaro.org Signed-off-by: Mike Turquettemturque...@ti.com Reviewed-by: Thomas Gleixnert...@linutronix.de Tested-by: Andrew Lunnand...@lunn.ch Reviewed-by: Rob Herringrob.herringatcalxeda.com Cc: Russell Kingli...@arm.linux.org.uk Cc: Jeremy Kerrjeremy.k...@canonical.com Cc: Arnd Bergmanarnd.bergm...@linaro.org Cc: Paul Walmsleyp...@pwsan.com Cc: Shawn Guoshawn@freescale.com Cc: Sascha Hauers.ha...@pengutronix.de Cc: Richard Zhaorichard.z...@linaro.org Cc: Saravana Kannanskan...@codeaurora.org Cc: Magnus Dammmagnus.d...@gmail.com Cc: Mark Brownbroo...@opensource.wolfsonmicro.com Cc: Linus Walleijlinus.wall...@stericsson.com Cc: Stephen Boydsb...@codeaurora.org Cc: Amit Kucheriaamit.kuche...@linaro.org Cc: Deepak Saxenadsax...@linaro.org Cc: Grant Likelygrant.lik...@secretlab.ca --- Mike, Thanks for the patches! Glad to see that it's finally getting in! I sent a request for a minor change as a reply to the v5 series (since it had more context). Can you please take a look at that and let me know if you can send out a v8 or a patch on top of this to do that? Hi Saravana, Hi Mike, I'm not sending a v8 series since Arnd has taken in v7 for the 3.4 merge window. Yeah, I later realized it might be better to send patches on top of v7. I'm formulating a reply to your v5 queries, but I'm not done looking at the implications of the initializer stuff. Lets keep the technical discussion in that thread for now. I saw some responses from you over the weekend but not to mine. So, I assumed you were busy with other stuff and I started working on a patch on top of v7. I will send that out if I get around to finishing it before you do. Hope that's alright with you. Based on what I have done so far, to me it just looked like a search and replace of clk-name with clk-hw.name and similar changes for the rest of the fields. It looks like we might be able to remove clk_register_mux, clk_register_divider, etc if we go with this. No stong opinion about removing those, but they seemed redundant after the suggester refactor. I think it would be okay to just move those init fields into clk_hw and not bother with renaming it to clk_initializer (I would prefer, clk_info or clk_common) if it makes the match much less invasive. 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. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
Reading the documentation of function clk_set_rate(), I'm not sure it exactly matches what the code does. If there is mismatch, it might be worth sending an incremental patch to update the documentation and avoid the confusion? On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote: +/** + * clk_set_rate - specify a new rate for clk + * @clk: the clk whose rate is being changed + * @rate: the new rate for clk + * + * In the simplest case clk_set_rate will only change the rate of clk. + * + * If clk has the CLK_SET_RATE_GATE flag set and it is enabled this call + * will fail; only when the clk is disabled will it be able to change + * its rate. + * + * Setting the CLK_SET_RATE_PARENT flag allows clk_set_rate to + * recursively propagate up to clk's parent; whether or not this happens + * depends on the outcome of clk's .round_rate implementation. If + * *parent_rate is 0 after calling .round_rate then upstream parent Might *parent_rate is not changed be more accurate? + * propagation is ignored. If *parent_rate comes back with a new rate + * for clk's parent then we propagate up to clk's parent and set it's + * rate. Upward propagation will continue until either a clk does not + * support the CLK_SET_RATE_PARENT flag or .round_rate stops requesting + * changes to clk's parent_rate. + * If there is a failure during upstream + * propagation then clk_set_rate will unwind and restore each clk's rate + * that had been successfully changed. Afterwards a rate change abort + * notification will be propagated downstream, starting from the clk + * that failed. I'm not sure this part still matches the code. + * + * At the end of all of the rate setting, clk_set_rate internally calls + * __clk_recalc_rates and propagates the rate changes downstream, I do not see __clk_recalc_rates is called by clk_set_rate in any way. Regards, Shawn + * starting from the highest clk whose rate was changed. This has the + * added benefit of propagating post-rate change notifiers. + * + * Note that while post-rate change and rate change abort notifications + * are guaranteed to be sent to a clk only once per call to + * clk_set_rate, pre-change notifications will be sent for every clk + * whose rate is changed. Stacking pre-change notifications is noisy + * for the drivers subscribed to them, but this allows drivers to react + * to intermediate clk rate changes up until the point where the final + * rate is achieved at the end of upstream propagation. + * + * Returns 0 on success, -EERROR otherwise. + */ ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
Another trivial comment. But if there is an incremental patch, maybe
consider to include it.
On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote:
...
+#ifdef CONFIG_COMMON_CLK_DISABLE_UNUSED
+static int clk_disable_unused(void)
+{
+ struct clk *clk;
+ struct hlist_node *tmp;
+
+ mutex_lock(prepare_lock);
+
+ hlist_for_each_entry(clk, tmp, clk_root_list, child_node)
+ clk_disable_unused_subtree(clk);
+
+ hlist_for_each_entry(clk, tmp, clk_orphan_list, child_node)
+ clk_disable_unused_subtree(clk);
+
+ mutex_unlock(prepare_lock);
+
+ return 0;
+}
+late_initcall(clk_disable_unused);
+#else
+static inline int clk_disable_unused(struct clk *clk) { return 0; }
This #else block seems completely unnecessary to me.
+#endif /* CONFIG_COMMON_CLK_DISABLE_UNUSED */
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/15/2012 11:11 PM, Mike Turquette wrote: The common clock framework defines a common struct clk useful across most platforms as well as an implementation of the clk api that drivers can use safely for managing clocks. The net result is consolidation of many different struct clk definitions and platform-specific clock framework implementations. This patch introduces the common struct clk, struct clk_ops and an implementation of the well-known clock api in include/clk/clk.h. Platforms may define their own hardware-specific clock structure and their own clock operation callbacks, so long as it wraps an instance of struct clk_hw. See Documentation/clk.txt for more details. This patch is based on the work of Jeremy Kerr, which in turn was based on the work of Ben Herrenschmidt. Signed-off-by: Mike Turquettemturque...@linaro.org Signed-off-by: Mike Turquettemturque...@ti.com Reviewed-by: Thomas Gleixnert...@linutronix.de Tested-by: Andrew Lunnand...@lunn.ch Reviewed-by: Rob Herringrob.herringat calxeda.com Cc: Russell Kingli...@arm.linux.org.uk Cc: Jeremy Kerrjeremy.k...@canonical.com Cc: Arnd Bergmanarnd.bergm...@linaro.org Cc: Paul Walmsleyp...@pwsan.com Cc: Shawn Guoshawn@freescale.com Cc: Sascha Hauers.ha...@pengutronix.de Cc: Richard Zhaorichard.z...@linaro.org Cc: Saravana Kannanskan...@codeaurora.org Cc: Magnus Dammmagnus.d...@gmail.com Cc: Mark Brownbroo...@opensource.wolfsonmicro.com Cc: Linus Walleijlinus.wall...@stericsson.com Cc: Stephen Boydsb...@codeaurora.org Cc: Amit Kucheriaamit.kuche...@linaro.org Cc: Deepak Saxenadsax...@linaro.org Cc: Grant Likelygrant.lik...@secretlab.ca --- Mike, Thanks for the patches! Glad to see that it's finally getting in! I sent a request for a minor change as a reply to the v5 series (since it had more context). Can you please take a look at that and let me know if you can send out a v8 or a patch on top of this to do that? 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. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
