Re: [PATCH 0/5] more clk-next fixes
On Tue, May 8, 2012 at 9:12 AM, Shawn Guo shawn@linaro.org wrote: On Sun, May 06, 2012 at 10:08:25PM -0700, Mike Turquette wrote: If no one complains about these then I'll commit them to clk-next and (finally) send my pull request to Arnd. On mach-mxs: Tested-by: Shawn Guo shawn@linaro.org Mike, I haven't seen too many outstanding comments on these patches, except the following one that Saravana put on clk-fixed-factor.c, which should be easy to fix. + return (parent_rate / fix-div) * fix-mult; Wouldn't multiplying and then dividing result in better accuracy? Can you please quickly fix it and publish the patches on your clk-next, so that I can ask Arnd to pull my platform porting, on which mach-mxs device tree series depends. I'll fix it up on my end and send the pull request to Arnd. Regards, Mike -- Regards, Shawn ___ 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 0/5] more clk-next fixes
On 20120506-22:08, Mike Turquette wrote: Hi all, These five patches are hopefully the final set of core framework changes for 3.5. There is the obligatory MAINTAINERS file change, three new fixes and the fixed-factor clock patch. That last patch is being reposted since three bugs were found in it (one on the list, two in my testing). If no one complains about these then I'll commit them to clk-next and (finally) send my pull request to Arnd. Arnd, Please pull the following changes since commit 66f75a5d028beaf67c931435fdc3e7823125730c: Linux 3.4-rc4 (2012-04-21 14:47:52 -0700) are available in the git repository at: git://git.linaro.org/people/mturquette/linux.git clk-next Mark Brown (2): clk: Remove comment for end of CONFIG_COMMON_CLK section clk: Constify parent name arrays Mike Turquette (10): clk: core: correct clk_set_rate kerneldoc clk: core: remove dead code paths clk: core: clk_calc_new_rates handles NULL parents clk: core: enforce clk_ops consistency clk: core: copy parent_names return error codes clk: basic: improve parent_names return errors MAINTAINERS: add entry for common clk framework clk: prevent spurious parent rate propagation clk: remove COMMON_CLK_DISABLE_UNUSED clk: mux: assign init data Rajendra Nayak (2): clk: Make clk_get_rate() return 0 on error clk: constify parent name arrays in macros Rob Herring (2): clk: select CLKDEV_LOOKUP for COMMON_CLK clk: remove trailing whitespace from clk.h Saravana Kannan (1): clk: Use a separate struct for holding init data. Sascha Hauer (1): clk: add a fixed factor clock Shawn Guo (7): clk: use kzalloc in clk_register_mux clk: remove unnecessary EXPORT_SYMBOL_GPL clk: add const for clk_ops of basic clks clk: declare clk_ops of basic clks in clk-provider.h clk: always pass parent_rate into .round_rate clk: pass parent_rate into .set_rate clk: propagate round_rate for CLK_SET_RATE_PARENT case Viresh Kumar (5): clk: Fix typo in comment clk: clk-gate: Create clk_gate_endisable() clk: clk-private: Add DEFINE_CLK macro clk: Don't set clk-new_rate twice clk: clk_set_rate() must fail if CLK_SET_RATE_GATE is set and clk is enabled MAINTAINERS| 10 ++ drivers/clk/Kconfig| 12 +-- drivers/clk/Makefile |2 +- drivers/clk/clk-divider.c | 68 +- drivers/clk/clk-fixed-factor.c | 95 ++ drivers/clk/clk-fixed-rate.c | 49 drivers/clk/clk-gate.c | 104 drivers/clk/clk-mux.c | 27 +++-- drivers/clk/clk.c | 270 +-- include/linux/clk-private.h| 99 +++ include/linux/clk-provider.h | 118 -- include/linux/clk.h|6 +- 12 files changed, 537 insertions(+), 323 deletions(-) create mode 100644 drivers/clk/clk-fixed-factor.c Note that the Orion patches aren't here, but I figure that Andrew L. probably wants to check those against the final clk-next before I pull them. Regards, Mike Mike Turquette (4): MAINTAINERS: add entry for common clk framework clk: prevent spurious parent rate propagation clk: remove COMMON_CLK_DISABLE_UNUSED clk: mux: assign init data Sascha Hauer (1): clk: add a fixed factor clock MAINTAINERS| 10 drivers/clk/Kconfig| 11 - drivers/clk/Makefile |2 +- drivers/clk/clk-fixed-factor.c | 95 drivers/clk/clk-mux.c |1 + drivers/clk/clk.c |9 +++- include/linux/clk-private.h| 20 include/linux/clk-provider.h | 23 ++ 8 files changed, 156 insertions(+), 15 deletions(-) create mode 100644 drivers/clk/clk-fixed-factor.c -- 1.7.5.4 ___ 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 2/5] clk: prevent spurious parent rate propagation
On Mon, May 7, 2012 at 12:58 AM, Sascha Hauer s.ha...@pengutronix.de wrote: On Sun, May 06, 2012 at 10:08:27PM -0700, Mike Turquette wrote: Patch 'clk: always pass parent_rate into .round_rate' made a subtle change to the semantics of .round_rate. It is now expected for the parent's rate to always be passed in, simplifying the implemenation of various .round_rate callback definitions. However the patch also introduced a bug in clk_calc_new_rates whereby a clock without the CLK_SET_RATE_PARENT flag set could still propagate a rate change up to a parent clock if the the .round_rate callback modified the best_parent_rate value in any way. This patch fixes the issue at the framework level (in clk_calc_new_rates) by specifically handling the case where the CLK_SET_RATE_PARENT flag is not set. Signed-off-by: Mike Turquette mturque...@linaro.org Reported-by: Sascha Hauers.ha...@pengutronix.de I think a warning might be useful when a clk driver tries to change the parent rate even if it is not allowed to. This can be added in a later patch, so A fine idea. Acked-by: Sascha Hauer s.ha...@pengutronix.de Thanks, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 5/5] clk: add a fixed factor clock
On Mon, May 7, 2012 at 12:54 PM, Saravana Kannan skan...@codeaurora.org wrote:
On 05/06/2012 10:08 PM, Mike Turquette wrote:
From: Sascha Hauers.ha...@pengutronix.de
+struct clk *clk_register_fixed_factor(struct device *dev, const char
*name,
+ const char *parent_name, unsigned long flags,
+ unsigned int mult, unsigned int div)
+{
+ struct clk_fixed_factor *fix;
+ struct clk_init_data init;
+ struct clk *clk;
+
+ fix = kmalloc(sizeof(*fix), GFP_KERNEL);
+ if (!fix) {
+ pr_err(%s: could not allocate fixed factor clk\n,
__func__);
+ return ERR_PTR(-ENOMEM);
+ }
Can we add a check for mult = div? It doesn't look like this clock is meant
to capture clock multipliers (if there is even anything like that other than
PLLs).
I don't think we should enforce a rule like that. Some folks with
static PLLs that never change their rates (and maybe are set up by the
bootloader) might find this clock type to be perfect for them as a
multiplier.
snip
+#define DEFINE_CLK_FIXED_FACTOR(_name, _parent_name, \
+ _parent_ptr, _flags, \
+ _mult, _div) \
+ static struct clk _name; \
+ static const char *_name##_parent_names[] = { \
+ _parent_name, \
+ }; \
+ static struct clk *_name##_parents[] = { \
+ _parent_ptr, \
+ }; \
+ static struct clk_fixed_factor _name##_hw = { \
+ .hw = { \
+ .clk =_name, \
+ }, \
+ .mult = _mult, \
+ .div = _div, \
+ }; \
+ DEFINE_CLK(_name, clk_fixed_factor_ops, _flags, \
+ _name##_parent_names, _name##_parents);
+
I would prefer not defining a macro for this. But if we are going to do it,
can we please at least stop doing nested macros here? It would be much
easier for a new comer to read if we don't nest these clock macros.
This macro follows the others in every way. Why should we make it
look less uniform?
Also, should we stop adding support for fully static allocation for new
clock types? Since it's supposed to be going away soon. Since Mike didn't
add this clock type, I'm guessing he doesn't need the clock type now and
hence doesn't need static allocation support for it.
I'm afraid I don't follow. I do need this clock in fact (see
https://github.com/mturquette/linux/commits/clk-next-may06-omap), and
my platform's data is still static.
/**
* __clk_init - initialize the data structures in a struct clk
* @dev: device initializing this clk, placeholder for now
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5db3412..c1c23b9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -277,6 +277,29 @@ struct clk *clk_register_mux(struct device *dev,
const char *name,
u8 clk_mux_flags, spinlock_t *lock);
/**
+ * struct clk_fixed_factor - fixed multiplier and divider clock
+ *
+ * @hw: handle between common and hardware-specific
interfaces
+ * @mult: multiplier
+ * @div: divider
+ *
+ * Clock with a fixed multiplier and divider. The output frequency is the
+ * parent clock rate divided by div and multiplied by mult.
+ * Implements .recalc_rate, .set_rate and .round_rate
+ */
+
+struct clk_fixed_factor {
+ struct clk_hw hw;
+ unsigned int mult;
+ unsigned int div;
+};
+
+extern struct clk_ops clk_fixed_factor_ops;
+struct clk *clk_register_fixed_factor(struct device *dev, const char
*name,
+ const char *parent_name, unsigned long flags,
+ unsigned int mult, unsigned int div);
Should we have one header file for each common clock type? We seem to be
adding a lot of those (which is good), but it almost feels like
clock-provider get out of hand soon.
I think clk-provider.h is fine for now. It's a good one-stop-shop for
people that are just now figuring out what basic clock types exist and
applying them to their platform. The file itself is only 336 lines
which is hardly out of control...
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.
___
Re: [PATCH 12/13] clk: core: copy parent_names return error codes
On Mon, Apr 16, 2012 at 1:30 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Wed, Apr 11, 2012 at 06:02:50PM -0700, Mike Turquette wrote:
struct clk *clk_register(struct device *dev, const char *name,
const struct clk_ops *ops, struct clk_hw *hw,
const char **parent_names, u8 num_parents, unsigned long flags)
{
+ int i, ret = -ENOMEM;
I suggest to move the initialization of ret from here...
struct clk *clk;
clk = kzalloc(sizeof(*clk), GFP_KERNEL);
- if (!clk)
- return NULL;
+ if (!clk) {
+ pr_err(%s: could not allocate clk\n, __func__);
+ goto fail_out;
+ }
clk-name = name;
clk-ops = ops;
clk-hw = hw;
clk-flags = flags;
- clk-parent_names = parent_names;
clk-num_parents = num_parents;
hw-clk = clk;
- __clk_init(dev, clk);
+ /* allocate local copy in case parent_names is __initdata */
+ clk-parent_names = kzalloc((sizeof(char*) * num_parents),
+ GFP_KERNEL);
+
+ if (!clk-parent_names) {
+ pr_err(%s: could not allocate clk-parent_names\n, __func__);
+ goto fail_parent_names;
+ }
+
+ /* copy each string name in case parent_names is __initdata */
... to here.
The rationale is that when this code is changed later someone might use
ret above and doesn't remember that the code below expects ret to be
initialized with -ENOMEM. Also it's easier to see that the code is
correct.
That is sensible.
Thanks,
Mike
Sascha
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 13/13] clk: basic: improve parent_names return errors
On Wed, Apr 11, 2012 at 11:49 PM, Shawn Guo shawn@linaro.org wrote:
On Wed, Apr 11, 2012 at 06:02:51PM -0700, Mike Turquette wrote:
...
@@ -175,23 +188,32 @@ struct clk *clk_register_divider(struct device *dev,
const char *name,
div-flags = clk_divider_flags;
div-lock = lock;
+ /* allocate the temporary parent_names */
if (parent_name) {
- div-parent[0] = kstrdup(parent_name, GFP_KERNEL);
- if (!div-parent[0])
- goto out;
+ parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
+ if (!parent_names[0]) {
+ pr_err(%s: could not allocate parent_names\n,
+ __func__);
+ goto fail_parent_names;
+ }
}
Why do we need to copy the parent_names here at all? clk_register()
has done that for each basic clk.
Yes, this was a braindead change on my part. I'll remove the kstrdup
in my next series (the rest of this patch will stay in).
Thanks,
Mike
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 13/13] clk: basic: improve parent_names return errors
On Mon, Apr 16, 2012 at 1:52 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Wed, Apr 11, 2012 at 06:02:51PM -0700, Mike Turquette wrote:
This patch is the basic clk version of 'clk: core: copy parent_names
return error codes'.
The registration functions are changed to allow the core code to copy
the array of strings and allow platforms to declare those arrays as
__initdata.
This patch also converts all of the basic clk registration functions to
return error codes which better aligns them with the existing clk.h api.
+ */
struct clk *clk_register_divider(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 shift, u8 width,
u8 clk_divider_flags, spinlock_t *lock)
{
struct clk_divider *div;
- struct clk *clk;
+ struct clk *clk = ERR_PTR(-ENOMEM);
+ const char *parent_names[1];
+ /* allocate the divider */
div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
-
if (!div) {
pr_err(%s: could not allocate divider clk\n, __func__);
return NULL;
You missed a conversion to ERR_PTR here.
Thanks for the catch.
Mike
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- |
___
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 13/13] clk: basic: improve parent_names return errors
On Mon, Apr 16, 2012 at 6:46 PM, Shawn Guo shawn@linaro.org wrote: On 17 April 2012 07:10, Turquette, Mike mturque...@ti.com wrote: ... Yes, this was a braindead change on my part. I'll remove the kstrdup in my next series (the rest of this patch will stay in). Do you have an ETA on that? A few platform porting are waiting for a stable branch with all necessary fixup/cleanup integrated to publish the patches. That is a good question. I think it is worth waiting on Saravana's patch which exposes non-private members of struct clk via struct clk_hw. This will have an effect on both platform clock data and code. I do not think that there is a point in pushing another series out until I get that in since it will shake things up for platforms trying to convert over. And due to the invasive nature I'm still not sure if this stuff will go into an -rc or some -next branch for 3.5. I don't see the harm in keeping it in a -next branch for 3.5 where all platforms can base on that branch since they will be queuing up for 3.5 anyway. Regards, Mike Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 00/13] common clk framework misc fixes
On Fri, Apr 13, 2012 at 2:21 AM, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Wed, Apr 11, 2012 at 06:02:38PM -0700, Mike Turquette wrote: This series collects many of the fixes posted for the recently merged common clock framework as well as some general clean-up. Most of the code classifies as a clean-up moreso than a bug fix; hopefully this is not a problem since the common clk framework is new code pulled for 3.4. Can you please CC people on your cover letters as well as on the individual patches? Yes. Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 01/13] clk: core: correct clk_set_rate kerneldoc
On Wed, Apr 11, 2012 at 9:28 PM, Viresh Kumar viresh.ku...@st.com wrote: On 4/12/2012 6:32 AM, Mike Turquette wrote: - * 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. Why is CLK_SET_RATE_GATE removed? I already sent a patch to fix clk_set_rate() for this. And i think it should be required. I removed it from the documentation since it was not present in the code. I was originally targeting this for an -rc and I was trying to limit the changes purely to fixes and cleanups, not new features. I haven't finished digging through all of the responses yet, so my goal for where these patches are headed might change. Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 02/13] clk: core: remove dead code paths
On Wed, Apr 11, 2012 at 11:14 PM, Viresh Kumar viresh.ku...@st.com wrote:
On 4/12/2012 6:32 AM, Mike Turquette wrote:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3ed36d3..4daacf5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -194,7 +194,7 @@ static int __init clk_debug_init(void)
late_initcall(clk_debug_init);
#else
static inline int clk_debug_register(struct clk *clk) { return 0; }
-#endif /* CONFIG_COMMON_CLK_DEBUG */
+#endif
Why is this updated? Isn't this considered good practice anymore?
See comments in this thread:
http://article.gmane.org/gmane.linux.kernel/1276102
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 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 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 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 1/3] Documentation: common clk API
On Sat, Mar 17, 2012 at 2:05 AM, Arnd Bergmann a...@arndb.de wrote: On Friday 16 March 2012, Turquette, Mike wrote: On Fri, Mar 16, 2012 at 3:21 PM, Paul Walmsley p...@pwsan.com wrote: From: Paul Walmsley p...@pwsan.com Date: Fri, 16 Mar 2012 16:06:30 -0600 Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now Mark the common clk code as depending on CONFIG_EXPERIMENTAL. The API is not well-defined and both it and the underlying mechanics are likely to need significant changes to support non-trivial uses of the rate changing code, such as DVFS with external I/O devices. So any platforms that switch their implementation over to this may need to revise much of their driver code and revalidate their implementations until the behavior of the code is better-defined. A good time for removing this EXPERIMENTAL designation would be after at least two platforms that do DVFS on groups of external I/O devices have ported their clock implementations over to the common clk code. Signed-off-by: Paul Walmsley p...@pwsan.com Cc: Mike Turquette mturque...@ti.com ACK. This will set some reasonable expectations while things are in flux. Arnd are you willing to take this in? I think it's rather pointless, because the option is not going to be user selectable but will get selected by the platform unless I'm mistaken. The platform maintainers that care already know the state of the framework. Also, there are no user space interfaces that we have to warn users about not being stable, because the framework is internal to the kernel. The consensus seems to be to not set experimental. However, I wonder whether we need the patch below to prevent users from accidentally enabling COMMON_CLK on platforms that already provide their own implementation. Arnd diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 2eaf17e..a0a83de 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,7 +12,7 @@ config HAVE_MACH_CLKDEV menuconfig COMMON_CLK - bool Common Clock Framework + bool select HAVE_CLK_PREPARE ---help--- The common clock framework is a single definition of struct clk, useful across many platforms, as well as an Much like experimental I'm not sure how needed this change is. The help section does say to leave it disabled by default, if unsure. If you merge it I won't object but this might be fixing an imaginary problem. Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Thu, Mar 15, 2012 at 2:43 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Wed, Mar 14, 2012 at 05:51:48PM -0700, Turquette, Mike wrote:
@@ -84,9 +78,9 @@ static int clk_divider_bestdiv(struct clk_hw *hw,
unsigned long rate,
for (i = 1; i = maxdiv; i++) {
parent_rate = __clk_round_rate(__clk_get_parent(hw-clk),
- MULT_ROUND_UP(rate, i));
+ (rate * i));
I think MULT_ROUND_UP is the right thing to use here (not sure if this
is a good name though)
Consider we want to have an output rate of 33Hz. Now acceptable input
rates for a divider value of 3 would be 99, 100 and 101Hz, so we have
to call round_rate for the parent with 101Hz which includes 100 and
99Hz.
We're back to the point Rob brought up about .round_rate rounding up
or down. We really need a least-upper-bounds or greatest-lower-bounds
flag, similar to how CPUfreq selects target frequencies today. I'm
freezing features for this patchset now, so I'll keep your
MULT_ROUND_UP approach and some day I'll fix .round_rate for good.
If you have problems with your PLL than most likely because it does
something different on clk_round_rate than it does in clk_set_rate,
for example clk_round_rate(1) returns 1, but clk_set_rate then
sets the rate due to some rounding error. Being consistent between
round_rate and set_rate is very important for this mechanism to work
properly. It did cost me some nerves to get it right for the divider
(and even more nerves to figure out why it is correct the way it works)
What I'd like to do is request the *exact* frequency I want when
passing in a rate to my PLLs .round_rate. Due to the way that my MN
dividers are calculated, and due to some jitter avoidance code, it is
bad for me to request 60001 Hz when I really want 600MHz exactly.
Anyways I fixed this up on my end enough to work with MULT_ROUND_UP,
so that can stay for the immediate future.
now = parent_rate / i;
- if (now = rate now = best) {
+ if (now = rate now best) {
This change is an optimization, but should be unrelated to your PLL
problem, right?
MULT_ROUND_UP yields multiples of 'rate' plus an incrementing value (m
- 1). Without that incrementing value added to the rate passed into
__clk_round_rate the for loop above will always max out the divider.
To illustrate:
The rate we want is 300MHz. Without MULT_ROUND_UP the for loop will
start yielding these combinations:
__clk_round_rate(parent, 300MHz), divide-by-1
__clk_round_rate(parent, 600MHz), divide-by-2
__clk_round_rate(parent, 900MHz), divide-by-3
__clk_round_rate(parent, 1200MHz), divide-by-4
...etc...
These all yield the desired 300MHz for our divider so it just keeps
going until you max out the divider and requests some crazy rate for
the parent. On most hardware that I am aware of it is desirable to
keep the divider as low as possible so the change to the conditional
prevents us from overwriting best every single time while keeping the
divider low. So yes, it is an optimization, but it is also quite
necessary without MULT_ROUND_UP.
Anyways I've kept MULT_ROUND_UP exactly as-is with the exception that
I've added that small optimization to keep dividers low. I hope there
are no objections to that.
Regards,
Mike
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 3/3] clk: basic clock hardware types
On Fri, Mar 16, 2012 at 5:25 AM, Richard Zhao richard.z...@linaro.org wrote:
[...]
+static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
+ unsigned long *best_parent_rate)
+{
+ struct clk_divider *divider = to_clk_divider(hw);
+ int i, bestdiv = 0;
+ unsigned long parent_rate, best = 0, now, maxdiv;
+
+ if (!rate)
+ rate = 1;
+
+ maxdiv = (1 divider-width);
+
+ if (divider-flags CLK_DIVIDER_ONE_BASED)
+ maxdiv--;
+
+ if (!best_parent_rate) {
+ parent_rate = __clk_get_rate(__clk_get_parent(hw-clk));
+ bestdiv = DIV_ROUND_UP(parent_rate, rate);
+ bestdiv = bestdiv == 0 ? 1 : bestdiv;
+ bestdiv = bestdiv maxdiv ? maxdiv : bestdiv;
+ return bestdiv;
+ }
+
+ /*
+ * The maximum divider we can use without overflowing
+ * unsigned long in rate * i below
+ */
+ maxdiv = min(ULONG_MAX / rate, maxdiv);
+
+ for (i = 1; i = maxdiv; i++) {
+ parent_rate = __clk_round_rate(__clk_get_parent(hw-clk),
+ MULT_ROUND_UP(rate, i));
+ now = parent_rate / i;
+ if (now = rate now best) {
+ bestdiv = i;
+ best = now;
+ *best_parent_rate = parent_rate;
Better add if (now == rate)
break;
There may be more than one hit for (now == rate). We'd better select
smallest div, thus smallest parent_rate.
Hmm I forgot to take this in. Notice that the conditional above has changed:
if (now = rate now best) {
So the smallest divider _will_ be selected, but adding in your check
for now == rate is nice since it will prevent unnecessary looping if
we've managed to hit the exact rate that we want. I'll ninja this one
into the merge request.
Thanks,
Mike
It's the same comment for v6, but not show stopper.
Thanks
Richard
+ }
+ }
+
+ if (!bestdiv) {
+ bestdiv = (1 divider-width);
+ if (divider-flags CLK_DIVIDER_ONE_BASED)
+ bestdiv--;
+ *best_parent_rate =
__clk_round_rate(__clk_get_parent(hw-clk), 1);
+ }
+
+ return bestdiv;
+}
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On Fri, Mar 16, 2012 at 1:57 PM, Arnd Bergmann a...@arndb.de wrote: On Friday 16 March 2012, Arnd Bergmann wrote: Can we shoe-horn this thing into 3.4 (it is a bit late, i know) so that platform ports can gather speed? Several people are waiting for a somewhat stable version before starting their ports. And what is the path into mainline - will Russell carry it or Arnd (with Russell's blessing)? I would suggest putting it into a late/* branch of arm-soc so it finally gets some linux-next exposure, and then sending it in the second week of the merge window. Russell, please let me know if you want to carry it instead or if you have found any last-minute show stoppers in the code. FWIW, it's in arm-soc now, and it's the last thing I put in there for v3.4. From now on until v3.4-rc1, feature patches can only get removed from arm-soc, not added. Thanks Arnd. Regards, Mike Arnd ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On Fri, Mar 16, 2012 at 3:21 PM, Paul Walmsley p...@pwsan.com wrote: From: Paul Walmsley p...@pwsan.com Date: Fri, 16 Mar 2012 16:06:30 -0600 Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now Mark the common clk code as depending on CONFIG_EXPERIMENTAL. The API is not well-defined and both it and the underlying mechanics are likely to need significant changes to support non-trivial uses of the rate changing code, such as DVFS with external I/O devices. So any platforms that switch their implementation over to this may need to revise much of their driver code and revalidate their implementations until the behavior of the code is better-defined. A good time for removing this EXPERIMENTAL designation would be after at least two platforms that do DVFS on groups of external I/O devices have ported their clock implementations over to the common clk code. Signed-off-by: Paul Walmsley p...@pwsan.com Cc: Mike Turquette mturque...@ti.com ACK. This will set some reasonable expectations while things are in flux. Arnd are you willing to take this in? Thanks, Mike --- drivers/clk/Kconfig | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 2eaf17e..a0a83de 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,6 +12,7 @@ config HAVE_MACH_CLKDEV menuconfig COMMON_CLK bool Common Clock Framework select HAVE_CLK_PREPARE + depends on EXPERIMENTAL ---help--- The common clock framework is a single definition of struct clk, useful across many platforms, as well as an -- 1.7.9.1 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Wed, Mar 14, 2012 at 1:48 AM, Sascha Hauer s.ha...@pengutronix.de wrote: On Tue, Mar 13, 2012 at 04:43:57PM -0700, Turquette, Mike wrote: On Tue, Mar 13, 2012 at 4:24 AM, Sascha Hauer s.ha...@pengutronix.de wrote: On Sat, Mar 03, 2012 at 12:29:00AM -0800, 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 Turquette mturque...@linaro.org Signed-off-by: Mike Turquette mturque...@ti.com Cc: Jeremy Kerr jeremy.k...@canonical.com Cc: Thomas Gleixner t...@linutronix.de Cc: Arnd Bergman arnd.bergm...@linaro.org Cc: Paul Walmsley p...@pwsan.com Cc: Shawn Guo shawn@freescale.com Cc: Richard Zhao richard.z...@linaro.org Cc: Saravana Kannan skan...@codeaurora.org Cc: Magnus Damm magnus.d...@gmail.com Cc: Rob Herring rob.herr...@calxeda.com Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Stephen Boyd sb...@codeaurora.org Cc: Amit Kucheria amit.kuche...@linaro.org Cc: Deepak Saxena dsax...@linaro.org Cc: Grant Likely grant.lik...@secretlab.ca Cc: Andrew Lunn and...@lunn.ch --- drivers/clk/Kconfig | 28 + drivers/clk/Makefile | 1 + drivers/clk/clk.c | 1323 ++ include/linux/clk-private.h | 68 +++ include/linux/clk-provider.h | 169 ++ include/linux/clk.h | 68 ++- 6 files changed, 1652 insertions(+), 5 deletions(-) create mode 100644 drivers/clk/clk.c create mode 100644 include/linux/clk-private.h create mode 100644 include/linux/clk-provider.h diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 3912576..18eb8c2 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -11,3 +11,31 @@ config HAVE_MACH_CLKDEV config HAVE_CLK_PREPARE bool + +menuconfig COMMON_CLK + bool Common Clock Framework + select HAVE_CLK_PREPARE + ---help--- + The common clock framework is a single definition of struct + clk, useful across many platforms, as well as an + implementation of the clock API in include/linux/clk.h. + Architectures utilizing the common struct clk should select + this automatically, but it may be necessary to manually select + this option for loadable modules requiring the common clock + framework. + + If in doubt, say N. + +if COMMON_CLK + +config COMMON_CLK_DEBUG + bool DebugFS representation of clock tree + depends on COMMON_CLK + ---help--- + Creates a directory hierchy in debugfs for visualizing the clk + tree structure. Each directory contains read-only members + that export information specific to that clk node: clk_rate, + clk_flags, clk_prepare_count, clk_enable_count + clk_notifier_count. + +endif diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 07613fa..ff362c4 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o +obj-$(CONFIG_COMMON_CLK) += clk.o diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c new file mode 100644 index 000..b979d74 --- /dev/null +++ b/drivers/clk/clk.c @@ -0,0 +1,1323 @@ +/* + * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com + * Copyright (C) 2011-2012 Linaro Ltd mturque...@linaro.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Standard functionality for the common clock API. See Documentation/clk.txt + */ + +#include linux/clk-private.h +#include linux/module.h +#include linux/mutex.h +#include linux/spinlock.h +#include linux/err.h +#include linux/list.h +#include linux/slab.h + +static DEFINE_SPINLOCK(enable_lock); +static DEFINE_MUTEX(prepare_lock); + +static HLIST_HEAD(clk_root_list); +static HLIST_HEAD(clk_orphan_list); +static LIST_HEAD(clk_notifier_list); + +/*** debugfs support ***/ + +#ifdef CONFIG_COMMON_CLK_DEBUG +#include linux/debugfs.h
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Wed, Mar 14, 2012 at 2:28 PM, Thomas Gleixner t...@linutronix.de wrote: On Wed, 14 Mar 2012, Turquette, Mike wrote: Could you folks please trim your replies? It's annoying to page down a gazillion of lines to find the gist. Sure. My mailer does this for me so I forget to do it sometimes... On Wed, Mar 14, 2012 at 1:48 AM, Sascha Hauer s.ha...@pengutronix.de wrote: Also, do you forsee needing hole in parent_names for any reason other than described above? I need it only for the case where a some values in the mux are marked as reserved in the datasheet or we simply do not have the corresponding clock in our tree (yet). We could also say that NULL pointers are not allowed in parent arrays, but instead orphan or dummy should be used. Then __clk_init should check for NULL pointers to make this clear. I've added a WARN in __clk_init if any entries in .parent_names are NULL. I think it best to populate it with dummy, or maybe a platform-specific name if it helps you during development. There is no guarantee that the selection of a parent can be mapped linear to register values. Agreed. I have always viewed .parent_names as only a list of the names of all possible parents, nothing more. And of course its array indexing should line up with struct clk **parents. So the right way to deal with it is to have an array of valid names with no holes and NULL pointers allowed and have a mapping from the array index to the register value. This is essentially what the .set_rate callback does. It takes as input u8 index and peforms the hardware specific magic to select the correct parent clock. This might be a register write using that exact same index, or it might be a single-bit register write using that index as the shift value, or it might translate that index into the data sent to an i2c device (where the address would be stored in struct clk_foo), etc etc. We both agree that .parent_names must contain valid names and should not have holes. What I don't understand is if you are saying that we should allow NULL ptrs as names; that seems contradictory but I want to make sure I'm reading you correctly. Thanks, Mike That makes the core code robust and allows to handle all corner cases including reserved bits, not implemented clocks and weird register layouts. Thanks, tglx ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote:
On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
I tried another
approach on the weekend which basically does not try to do all in a
single recursion but instead sets the rate in multiple steps:
1) call a function which calculates all new rates of affected clocks
in a rate change and safes the value in a clk-new_rate field. This
function returns the topmost clock which has to be changed.
2) starting from the topmost clock notify all clients. This walks the
whole subtree even if a notfifier refuses the change. If necessary
we can walk the whole subtree again to abort the change.
3) actually change rates starting from the topmost clocks and notify
all clients on the way. I changed the set_rate callback to void.
Instead of failing (what is failing in case of set_rate? The clock
will still have some rate) I check for the result with
clk_ops-recalc_rate.
The way described above works for me now, see this branch:
git://git.pengutronix.de/git/imx/linux-2.6.git v3.3-rc6-clkv6-fixup
You may not necessarily like it as it changes quite a lot in the rate
changing code.
I tried that code and I really like it! It is much more readable and
feels less fragile than the previous recursive __clk_set_rate. I
did quite a bit of testing with this code today. One of the tests
looks like this:
pll (adjustable to anything)
|
clk_divider (5 bits wide)
|
dummy(no clk_ops)
The new code did a fine job arbitrating rates for the PLL and the
intermediate divider even if I put weird constraints on the PLL. For
instance if I artificially limited it to a minimum of 600MHz and then
ran clk_set_rate(dummy, 300MHz) it would lock at 600MHz and set
clk_divider to divide-by-2. Setting to 600MHz or more set the divider
back to 1 and relocked the PLL appropriately. Pretty cool.
I also tested the notifiers with this code and they seem to function
properly. I'll take this code in for v7. Thanks a lot for this
helpful contribution.
I did find that MULT_ROUND_UP caused trouble for my PLL's round_rate
implementation. Maybe my PLL code is fragile but a quick fix was to
make sure that we send the exact value we want to the round_rate code.
I also feel this is more correct. Let me know what you think:
8---
commit 189fecedb175d0366759246c4192f45b0bc39a50
Author: Mike Turquette mturque...@linaro.org
Date: Wed Mar 14 17:29:51 2012 -0700
clk-divider.c: round the actual rate we care about
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 86ca9cd..06ef4a0 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -47,12 +47,6 @@ static unsigned long clk_divider_recalc_rate(struct
clk_hw *hw,
}
EXPORT_SYMBOL_GPL(clk_divider_recalc_rate);
-/*
- * The reverse of DIV_ROUND_UP: The maximum number which
- * divided by m is r
- */
-#define MULT_ROUND_UP(r, m) ((r) * (m) + (m) - 1)
-
static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
unsigned long *best_parent_rate)
{
@@ -84,9 +78,9 @@ static int clk_divider_bestdiv(struct clk_hw *hw,
unsigned long rate,
for (i = 1; i = maxdiv; i++) {
parent_rate = __clk_round_rate(__clk_get_parent(hw-clk),
- MULT_ROUND_UP(rate, i));
+ (rate * i));
now = parent_rate / i;
- if (now = rate now = best) {
+ if (now = rate now best) {
bestdiv = i;
best = now;
*best_parent_rate = parent_rate;
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Tue, Mar 13, 2012 at 2:48 PM, Rob Herring robherri...@gmail.com wrote: Mike, On 03/10/2012 01:54 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. 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 Turquette mturque...@linaro.org Signed-off-by: Mike Turquette mturque...@ti.com Cc: Russell King li...@arm.linux.org.uk Cc: Jeremy Kerr jeremy.k...@canonical.com Cc: Thomas Gleixner t...@linutronix.de Cc: Arnd Bergman arnd.bergm...@linaro.org Cc: Paul Walmsley p...@pwsan.com Cc: Shawn Guo shawn@freescale.com Cc: Sascha Hauer s.ha...@pengutronix.de Cc: Richard Zhao richard.z...@linaro.org Cc: Saravana Kannan skan...@codeaurora.org Cc: Magnus Damm magnus.d...@gmail.com Cc: Rob Herring rob.herr...@calxeda.com Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Stephen Boyd sb...@codeaurora.org Cc: Amit Kucheria amit.kuche...@linaro.org Cc: Deepak Saxena dsax...@linaro.org Cc: Grant Likely grant.lik...@secretlab.ca Cc: Andrew Lunn and...@lunn.ch snip + + /* + * walk the list of orphan clocks and reparent any that are children of + * this clock + */ + hlist_for_each_entry(orphan, tmp, clk_orphan_list, child_node) In __clk_init, this needs to be hlist_for_each_entry_safe as entries can be removed. Thanks for the catch Rob. I'll take this in. Regards, Mike Rob ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Tue, Mar 13, 2012 at 4:24 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sat, Mar 03, 2012 at 12:29:00AM -0800, 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 Turquette mturque...@linaro.org
Signed-off-by: Mike Turquette mturque...@ti.com
Cc: Jeremy Kerr jeremy.k...@canonical.com
Cc: Thomas Gleixner t...@linutronix.de
Cc: Arnd Bergman arnd.bergm...@linaro.org
Cc: Paul Walmsley p...@pwsan.com
Cc: Shawn Guo shawn@freescale.com
Cc: Richard Zhao richard.z...@linaro.org
Cc: Saravana Kannan skan...@codeaurora.org
Cc: Magnus Damm magnus.d...@gmail.com
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Linus Walleij linus.wall...@stericsson.com
Cc: Stephen Boyd sb...@codeaurora.org
Cc: Amit Kucheria amit.kuche...@linaro.org
Cc: Deepak Saxena dsax...@linaro.org
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Andrew Lunn and...@lunn.ch
---
drivers/clk/Kconfig | 28 +
drivers/clk/Makefile | 1 +
drivers/clk/clk.c | 1323
++
include/linux/clk-private.h | 68 +++
include/linux/clk-provider.h | 169 ++
include/linux/clk.h | 68 ++-
6 files changed, 1652 insertions(+), 5 deletions(-)
create mode 100644 drivers/clk/clk.c
create mode 100644 include/linux/clk-private.h
create mode 100644 include/linux/clk-provider.h
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 3912576..18eb8c2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -11,3 +11,31 @@ config HAVE_MACH_CLKDEV
config HAVE_CLK_PREPARE
bool
+
+menuconfig COMMON_CLK
+ bool Common Clock Framework
+ select HAVE_CLK_PREPARE
+ ---help---
+ The common clock framework is a single definition of struct
+ clk, useful across many platforms, as well as an
+ implementation of the clock API in include/linux/clk.h.
+ Architectures utilizing the common struct clk should select
+ this automatically, but it may be necessary to manually select
+ this option for loadable modules requiring the common clock
+ framework.
+
+ If in doubt, say N.
+
+if COMMON_CLK
+
+config COMMON_CLK_DEBUG
+ bool DebugFS representation of clock tree
+ depends on COMMON_CLK
+ ---help---
+ Creates a directory hierchy in debugfs for visualizing the clk
+ tree structure. Each directory contains read-only members
+ that export information specific to that clk node: clk_rate,
+ clk_flags, clk_prepare_count, clk_enable_count
+ clk_notifier_count.
+
+endif
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 07613fa..ff362c4 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
+obj-$(CONFIG_COMMON_CLK) += clk.o
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
new file mode 100644
index 000..b979d74
--- /dev/null
+++ b/drivers/clk/clk.c
@@ -0,0 +1,1323 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com
+ * Copyright (C) 2011-2012 Linaro Ltd mturque...@linaro.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API. See
Documentation/clk.txt
+ */
+
+#include linux/clk-private.h
+#include linux/module.h
+#include linux/mutex.h
+#include linux/spinlock.h
+#include linux/err.h
+#include linux/list.h
+#include linux/slab.h
+
+static DEFINE_SPINLOCK(enable_lock);
+static DEFINE_MUTEX(prepare_lock);
+
+static HLIST_HEAD(clk_root_list);
+static HLIST_HEAD(clk_orphan_list);
+static LIST_HEAD(clk_notifier_list);
+
+/*** debugfs support ***/
+
+#ifdef CONFIG_COMMON_CLK_DEBUG
+#include linux/debugfs.h
+
+static struct dentry *rootdir;
+static struct dentry *orphandir;
+static int inited = 0;
+
+/* caller must hold prepare_lock */
+static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
+{
+ struct dentry *d;
+ int ret = -ENOMEM;
+
+ if
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Mon, Mar 5, 2012 at 1:22 AM, Richard Zhao richard.z...@freescale.com wrote:
Hi Mike,
Hi Richard,
Sorry for missing this earlier. I've taken in most of your
suggestions and commented on some of them below. Any of your feedback
that I cut from this mail was taken in as a fix in v7 :-)
On Sat, Mar 03, 2012 at 12:29:00AM -0800, Mike Turquette wrote:
+/**
+ * __clk_round_rate - round the given rate for a clk
+ * @clk: round the rate of this clock
+ *
+ * Caller must hold prepare_lock. Useful for clk_ops such as .set_rate
+ */
+unsigned long __clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ if (!clk !clk-ops-round_rate)
be || ?
This is fixed in v7 by a refactor of how clk_round_rate and
__clk_round_rate work.
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ unsigned long ret = rate;
+
+ mutex_lock(prepare_lock);
+ if (clk clk-ops-round_rate)
+ ret = __clk_round_rate(clk, rate);
+ mutex_unlock(prepare_lock);
+
+ return ret;
If clk is NULL, clk_round_rate and __clk_round_rate return different
values. Do you mean it?
This also is fixed in v7 by a refactor of how clk_round_rate and
__clk_round_rate work.
+static int __clk_speculate_rates(struct clk *clk, unsigned long parent_rate)
+{
+ struct hlist_node *tmp;
+ struct clk *child;
+ unsigned long new_rate;
+ int ret = NOTIFY_DONE;
+
+ if (!clk-ops-recalc_rate)
+ goto out;
When recalc_rate is NULL, it's possible for it and its children to have
notifier too.
Fixed by assuming parent rate without .recalc_rate callback. This
mirrors __clk_recalc_rates.
+
+ new_rate = clk-ops-recalc_rate(clk-hw, parent_rate);
+
+ /* abort the rate change if a driver returns NOTIFY_BAD */
+ if (clk-notifier_count)
+ ret = __clk_notify(clk, PRE_RATE_CHANGE, clk-rate, new_rate);
+
+ if (ret == NOTIFY_BAD)
+ goto out;
+
+ hlist_for_each_entry(child, tmp, clk-children, child_node) {
+ ret = __clk_speculate_rates(child, new_rate);
+ if (ret == NOTIFY_BAD)
+ break;
+ }
Tell the notifier that already receive PRE_RATE_CHANGE abort?
All of this stuff might change due to existing thread with Sascha on
how clk_set_rate should propagate parent rate changes. But for
completeness sake, a single ABORT goes out at the end of clk_set_rate
in this implementation, so there isn't a problem here.
+static struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ struct clk *fail_clk = NULL;
+ int ret = NOTIFY_DONE;
+ unsigned long old_rate = clk-rate;
+ unsigned long new_rate;
+ unsigned long parent_old_rate;
+ unsigned long parent_new_rate = 0;
+ struct clk *child;
+ struct hlist_node *tmp;
+
+ /* bail early if we can't change rate while clk is enabled */
+ if ((clk-flags CLK_SET_RATE_GATE) clk-enable_count)
+ return clk;
+
+ /* find the new rate and see if parent rate should change too */
+ WARN_ON(!clk-ops-round_rate);
+
+ new_rate = clk-ops-round_rate(clk-hw, rate, parent_new_rate);
+
+ /* NOTE: pre-rate change notifications will stack */
+ if (clk-notifier_count)
+ ret = __clk_notify(clk, PRE_RATE_CHANGE, clk-rate, new_rate);
if ((clk-flags CLK_SET_RATE_PARENT) parent_new_rate)
dowstream notify from parent clk?
The pre-rate change notifiers stack; also no problem here.
+
+ if (ret == NOTIFY_BAD)
+ return clk;
+
+ /* speculate rate changes down the tree */
+ hlist_for_each_entry(child, tmp, clk-children, child_node) {
+ ret = __clk_speculate_rates(child, new_rate);
+ if (ret == NOTIFY_BAD)
roll back?
Similar to the above. The post-rate change notifiers are sent out
once from the top of the subtree in __clk_set_rate; no problem here.
+ return clk;
+ }
+
+ /* change the rate of this clk */
+ if (clk-ops-set_rate)
If set_rate is NULL, it can fail at the beginning of this function.
We don't want to fail. In the conditional block below we allow for
propagating the rate request up to the parent. This would allow for a
gate clock at the bottom of the tree to have clk_set_rate called
against it, even though its hardware doesn't support it, and it will
propagate the request up the tree until it finds a clock that can
adjust the rate. This feature is very desirable for driver folks who
do not want to know about the tree topology in their driver beyond the
clocks needed by their device.
+ ret = clk-ops-set_rate(clk-hw, new_rate);
+
+ if (ret == NOTIFY_BAD)
you mean to check set_rate fail? Notify ABORT_RATE_CHANGE?
As stated, this happens once in clk_set_rate.
+ return clk;
+
+ /*
+ * change the rate of the parent clk if necessary
+ *
+ * hitting the nested 'if'
Re: [PATCH v6 3/3] clk: basic clock hardware types
On Mon, Mar 12, 2012 at 1:18 PM, Rob Herring robherri...@gmail.com wrote:
On 03/10/2012 01:54 AM, Mike Turquette wrote:
Many platforms support simple gateable clocks, fixed-rate clocks,
adjustable divider clocks and multi-parent multiplexer clocks.
This patch introduces basic clock types for the above-mentioned hardware
which share some common characteristics.
Based on original work by Jeremy Kerr and contribution by Jamie Iles.
Dividers and multiplexor clocks originally contributed by Richard Zhao
Sascha Hauer.
Signed-off-by: Mike Turquette mturque...@linaro.org
Signed-off-by: Mike Turquette mturque...@ti.com
Cc: Russell King li...@arm.linux.org.uk
Cc: Jeremy Kerr jeremy.k...@canonical.com
Cc: Thomas Gleixner t...@linutronix.de
Cc: Arnd Bergman arnd.bergm...@linaro.org
Cc: Paul Walmsley p...@pwsan.com
Cc: Shawn Guo shawn@freescale.com
Cc: Sascha Hauer s.ha...@pengutronix.de
Cc: Jamie Iles ja...@jamieiles.com
Cc: Richard Zhao richard.z...@linaro.org
Cc: Saravana Kannan skan...@codeaurora.org
Cc: Magnus Damm magnus.d...@gmail.com
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Linus Walleij linus.wall...@stericsson.com
Cc: Stephen Boyd sb...@codeaurora.org
Cc: Amit Kucheria amit.kuche...@linaro.org
Cc: Deepak Saxena dsax...@linaro.org
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Andrew Lunn and...@lunn.ch
One issue with spinlocks below, but otherwise:
Reviewed-by: Rob Herring rob.herr...@calxeda.com
---
Changes since v5:
* standardized the hw-specific locking in the basic clock types
* export the individual ops for each basic clock type
* improve registration for single-parent basic clocks (thanks Sascha)
* fixed bugs in gate clock's static initializers (thanks Andrew)
drivers/clk/Makefile | 3 +-
drivers/clk/clk-divider.c | 204
++
drivers/clk/clk-fixed-rate.c | 82 +
drivers/clk/clk-gate.c | 157
drivers/clk/clk-mux.c | 123 +
include/linux/clk-private.h | 124 +
include/linux/clk-provider.h | 127 ++
7 files changed, 819 insertions(+), 1 deletions(-)
create mode 100644 drivers/clk/clk-divider.c
create mode 100644 drivers/clk/clk-fixed-rate.c
create mode 100644 drivers/clk/clk-gate.c
create mode 100644 drivers/clk/clk-mux.c
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index ff362c4..1f736bc 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
-obj-$(CONFIG_COMMON_CLK) += clk.o
+obj-$(CONFIG_COMMON_CLK) += clk.o clk-fixed-rate.o clk-gate.o \
+ clk-mux.o clk-divider.o
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
new file mode 100644
index 000..c0c4e0b
--- /dev/null
+++ b/drivers/clk/clk-divider.c
@@ -0,0 +1,204 @@
+/*
+ * Copyright (C) 2011 Sascha Hauer, Pengutronix s.ha...@pengutronix.de
+ * Copyright (C) 2011 Richard Zhao, Linaro richard.z...@linaro.org
+ * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd
mturque...@linaro.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Adjustable divider clock implementation
+ */
+
+#include linux/clk-provider.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/io.h
+#include linux/err.h
+#include linux/string.h
+
+/*
+ * DOC: basic adjustable divider clock that cannot gate
+ *
+ * Traits of this clock:
+ * prepare - clk_prepare only ensures that parents are prepared
+ * enable - clk_enable only ensures that parents are enabled
+ * rate - rate is adjustable. clk-rate = parent-rate / divisor
+ * parent - fixed parent. No clk_set_parent support
+ */
+
+#define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
+
+static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct clk_divider *divider = to_clk_divider(hw);
+ unsigned int div;
+ unsigned long flags = 0;
+
+ if (divider-lock)
+ spin_lock_irqsave(divider-lock, flags);
+
+ div = readl(divider-reg) divider-shift;
+ div = (1 divider-width) - 1;
+
+ if (divider-lock)
+ spin_unlock_irqrestore(divider-lock, flags);
What are you locking against? You are only reading the register.
Hi Rob,
These register-level locks originally came in from the divider
multiplexer patches from Richard and Sascha and I'm sure they can give
you more details than I.
Basically on their platform they have some 32-bits regs that have a
lot of overlapping clock functions in them, such as enable/disable and
adjusting a divider all in one
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote:
On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
Hi Mike,
I was about to give my tested-by when I decided to test the set_rate
function. Unfortunately this is broken for several reasons. I'll try
to come up with a fixup series later the day.
I haven't tested clk_set_rate since V4, but I also haven't changed the
code appreciably. I'll retest on my end also.
On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
+ /* find the new rate and see if parent rate should change too */
+ WARN_ON(!clk-ops-round_rate);
+
+ new_rate = clk-ops-round_rate(clk-hw, rate, parent_new_rate);
You don't need a WARN_ON when you derefence clk-ops-round_rate anyway.
Agreed that the WARN_ON should not be there.
The v6 Documentation/clk.txt states that .round_rate is mandatory for
clocks that can adjust their rate, but I need to clarify this a bit
more. Ideally we want to be able to call clk_set_rate on any clock
and get a changed rate (if possible) by either adjusting that clocks
rate direction (e.g. a PLL or an adjustable divider) or by propagating
__clk_set_rate up the parents (assuming of course that
CLK_SET_RATE_PARENT flag is set appropriately).
Also, even when the current clock does not have a set_rate function it
can still change its rate when the CLK_SET_RATE_PARENT is set.
Correct. I'll clean this up and make the documentation a bit more
verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.
+
+ /* NOTE: pre-rate change notifications will stack */
+ if (clk-notifier_count)
+ ret = __clk_notify(clk, PRE_RATE_CHANGE, clk-rate,
new_rate);
+
+ if (ret == NOTIFY_BAD)
+ return clk;
+
+ /* speculate rate changes down the tree */
+ hlist_for_each_entry(child, tmp, clk-children, child_node) {
+ ret = __clk_speculate_rates(child, new_rate);
+ if (ret == NOTIFY_BAD)
+ return clk;
+ }
+
+ /* change the rate of this clk */
+ if (clk-ops-set_rate)
+ ret = clk-ops-set_rate(clk-hw, new_rate);
I don't know the reason why you change the child clock before the parent
clock, but it cannot work since this clock will change its rate based on
the old parent rate and not the new one.
This depends on the .round_rate implementation, which I admit to
having lost some sleep over. A clever .round_rate will request the
intermediate rate for a clock when propagating a request to change
the parent rate later on. Take for instance the following:
pll @ 200MHz (locked)
|
parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
|
child @ 25MHz (can divide by 2 or 4; currently divider is 4)
If we want child to run at 100MHz then the desirable configuration
would be to have parent divide-by-1 and child divide-by-2. When we
call,
clk_set_rate(child, 100MHz);
Its .round_rate should return 50MHz, and parent_new_rate should be
200MHz. So 50MHz is an intermediate rate, but it gets us the
divider we want. And in fact 50MHz reflects reality because that will
be the rate of child until the parent propagation completes and we can
adjust parent's dividers. (this is one reason why I prefer for
pre-rate change notifiers to stack on top of each other).
So now that parent_new_rate is 0, __clk_set_rate will propagate the
request up and parent's .round_rate will simply return 200MHz and
leave it's own parent_new_rate at 0. This will change from
divide-by-2 to divide-by-1 and from this highest point in the tree we
will propagate post-rate change notifiers downstream, as part of the
recalc_rate tree walk.
I have tested this with OMAP4's CPUfreq driver and I think, while
complicated, it is a sound way to approach the problem. Maybe the API
can be cleaned up, if you have any suggestions.
I cannot see all implications this way will have. All this rate
propagation is more complex than I thought it would be.
Hi Sascha,
Yes it is very complicated. The solution I have now (recursive
__clk_set_rate, clever .round_rate which requests parent rate) was not
something I arrived at immediately.
I decided to validate the v6 patches more thoroughly today, based on
your claim that clk_set_rate is broken and here is what I found:
1) clk_set_rate works. I pulled in the latest OMAP4 CPUfreq code into
my common clk branch and it Just Worked. This is a dumb
implementation involving no upwards parent propagation, and the clock
changing is of type struct clk_hw_omap (relocking a PLL)
2) while I was at it I verified the rate change notifiers +
clk_set_parent, which also work (I had not touched these since v4 and
wanted to make sure nothing was broken)
Here is where things get interesting. I tried the same parent rate
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
Hi Mike,
I was about to give my tested-by when I decided to test the set_rate
function. Unfortunately this is broken for several reasons. I'll try
to come up with a fixup series later the day.
I haven't tested clk_set_rate since V4, but I also haven't changed the
code appreciably. I'll retest on my end also.
On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
+ /* find the new rate and see if parent rate should change too */
+ WARN_ON(!clk-ops-round_rate);
+
+ new_rate = clk-ops-round_rate(clk-hw, rate, parent_new_rate);
You don't need a WARN_ON when you derefence clk-ops-round_rate anyway.
Agreed that the WARN_ON should not be there.
The v6 Documentation/clk.txt states that .round_rate is mandatory for
clocks that can adjust their rate, but I need to clarify this a bit
more. Ideally we want to be able to call clk_set_rate on any clock
and get a changed rate (if possible) by either adjusting that clocks
rate direction (e.g. a PLL or an adjustable divider) or by propagating
__clk_set_rate up the parents (assuming of course that
CLK_SET_RATE_PARENT flag is set appropriately).
Also, even when the current clock does not have a set_rate function it
can still change its rate when the CLK_SET_RATE_PARENT is set.
Correct. I'll clean this up and make the documentation a bit more
verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.
+
+ /* NOTE: pre-rate change notifications will stack */
+ if (clk-notifier_count)
+ ret = __clk_notify(clk, PRE_RATE_CHANGE, clk-rate, new_rate);
+
+ if (ret == NOTIFY_BAD)
+ return clk;
+
+ /* speculate rate changes down the tree */
+ hlist_for_each_entry(child, tmp, clk-children, child_node) {
+ ret = __clk_speculate_rates(child, new_rate);
+ if (ret == NOTIFY_BAD)
+ return clk;
+ }
+
+ /* change the rate of this clk */
+ if (clk-ops-set_rate)
+ ret = clk-ops-set_rate(clk-hw, new_rate);
I don't know the reason why you change the child clock before the parent
clock, but it cannot work since this clock will change its rate based on
the old parent rate and not the new one.
This depends on the .round_rate implementation, which I admit to
having lost some sleep over. A clever .round_rate will request the
intermediate rate for a clock when propagating a request to change
the parent rate later on. Take for instance the following:
pll @ 200MHz (locked)
|
parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
|
child @ 25MHz (can divide by 2 or 4; currently divider is 4)
If we want child to run at 100MHz then the desirable configuration
would be to have parent divide-by-1 and child divide-by-2. When we
call,
clk_set_rate(child, 100MHz);
Its .round_rate should return 50MHz, and parent_new_rate should be
200MHz. So 50MHz is an intermediate rate, but it gets us the
divider we want. And in fact 50MHz reflects reality because that will
be the rate of child until the parent propagation completes and we can
adjust parent's dividers. (this is one reason why I prefer for
pre-rate change notifiers to stack on top of each other).
So now that parent_new_rate is 0, __clk_set_rate will propagate the
request up and parent's .round_rate will simply return 200MHz and
leave it's own parent_new_rate at 0. This will change from
divide-by-2 to divide-by-1 and from this highest point in the tree we
will propagate post-rate change notifiers downstream, as part of the
recalc_rate tree walk.
I have tested this with OMAP4's CPUfreq driver and I think, while
complicated, it is a sound way to approach the problem. Maybe the API
can be cleaned up, if you have any suggestions.
Regards,
Mike
There are more things, as said I'll try to come up with a fixup series.
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 v5 3/4] clk: introduce the common clock framework
On Wed, Mar 7, 2012 at 10:27 PM, Andrew Lunn and...@lunn.ch wrote: Assuming that some day OMAP code can be refactored to allow for lazy (or at least initcall-based) registration of clocks then perhaps your suggestion can take root. Which leads me to this question: are there any other platforms out there that require the level of expose to struct clk present in this patchset? OMAP does, for now, but if that changes then I need to know if others require this as well. Hi Mike For kirkwood, i use static clk's for all but my root clock. I cannot statically know the rate of the root clock, so i have to determine it at boot time using heuristics, PCI ID, etc. I used statics thinking it would be less code. No idea if it actually is, and there is nothing stopping me moving to creating the clocks after creating the root clock. One comment i have about the current static clks. I completely missing you need to call __clk_init() on them and so ended up with lots of division by zero errors, since they did not have a parent, and so the code seemed not be to able to determine the rate. So 1) Please add __clk_init() to the documentation in the section about static clocks. Done. 2) Should maybe the name change? It seems strange having to call a __X() function. If this is a function which is supposed to be used, drop the __. Maybe clk_static_register()? That function is used internally by clk_register and is only exposed by clk-private.h, so I think the naming scheme is sane. I basically want to create a sense of worry in anyone using clk-private.h :-) 3) Maybe, if the parent is missing, clk_get_rate() should return an error? Non-root, non-fixed-rate, orphan clocks can return an error in this case. Will update for V6. Any idea on best -EERROR? I'm thinking ENODEV, but ECHILD and EPIPE are kinda funny in this context. Regards, Mike Andrew ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Thu, Mar 8, 2012 at 11:57 PM, Andrew Lunn and...@lunn.ch wrote:
I'd say use the nonstatic ones. I think using the static initializers
will cause us much pain in the future. I've been through several rebases
on the i.MX clock rework and everytime I wish my sed foo would be
better. Now imagine what happens when it turns out that the internal
struct clk layout or the structs for the muxes/dividers/gates have to
be changed.
/*
* CLK tree
/
static DEFINE_SPINLOCK(gating_lock);
#define DEFINE_KIRKWOOD_CLK_GATE(_name, _bit) \
DEFINE_CLK_GATE(_name, tclk, NULL, 0, \
(void __iomem *)CLOCK_GATING_CTRL, \
_bit, 0, gating_lock)
DEFINE_KIRKWOOD_CLK_GATE(clk_ge0, CGC_BIT_GE0);
DEFINE_KIRKWOOD_CLK_GATE(clk_pex0, CGC_BIT_PEX0);
DEFINE_KIRKWOOD_CLK_GATE(clk_usb0, CGC_BIT_USB0);
DEFINE_KIRKWOOD_CLK_GATE(clk_sdio, CGC_BIT_SDIO);
DEFINE_KIRKWOOD_CLK_GATE(clk_tsu, CGC_BIT_TSU);
DEFINE_KIRKWOOD_CLK_GATE(clk_dunit, CGC_BIT_DUNIT);
DEFINE_KIRKWOOD_CLK_GATE(clk_runit, CGC_BIT_RUNIT);
I've so far not had any problems, and not needed an sed foo. I do
only have a dozen or so clocks, which helps. But even so, all the real
pain is hidden inside DEFINE_CLK_GATE() which Mike maintains.
It's true that if the argument list for the macros doesn't change then
the pain of static initialization is hidden from the platform data.
However if you have the ability to use the clk_foo_register functions
please do use them in place of static initialization. The static init
stuff is only for folks backed into a corner and forced to use it...
for now. I'm looking at ways to allow for kmalloc'ing in early boot,
as well as reducing the number of clocks that my platform registers
during early boot drastically.
I guess the problem comes when you are not using the basic clk
providers, but your own provider. What might help is if
linux/clk-provider.h could provide some macros to do most of the
generic definitions. Something like:
I'd rather people just used the registration functions instead.
Thanks,
Mike
#define DEFINE_CLK_GENERIC(_name, _flags, _ops) \
static struct clk _name; \
static char *_name##_parent_names[] = {}; \
static struct clk _name = { \
.name = #_name, \
.ops = _ops, \
.hw = _name##_hw.hw, \
.parent_names = _name##_parent_names, \
.num_parents = \
ARRAY_SIZE(_name##_parent_names), \
.flags = _flags, \
};
and then you have something like
#define DEFINE_CLK_IMX(_name, _flags, _foo, _bar) \
static struct clk_imx _name##_hw = { \
.hw = { \
.clk = _name, \
}, \
.foo = _foo, \
.bar = _bar, \
}; \
DEFINE_CLK_GENERIC(_name, _flags, clk_imx_ops)
Andrew
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 0/4] common clk framework
On Thu, Mar 8, 2012 at 6:34 PM, Richard Zhao richard.z...@freescale.com wrote: Hello Mike, The main interface for clk implementer is to register clocks dynamically. I think it highly depends on clk DT bindings. From the patch Grant sent out, it looks like he doesn't like one node per clk. So how do we register clocks dynamically? You have any sample code? I can only speak for my own platform, but after talks with Paul W. and Benoit C. I believe that OMAP will not push ALL of it's on-SoC clocks into DT. DT is meant to solve board-level integration problems and the majority of OMAP clocks are SoC-level, not board level. Some clocks will migrate out to DT such as the primary oscillator (which varies per board) as well as some leaf clocks and modules clocks that depend on board-specific peripherals. Otherwise the answer to your question is 'no'. I don't have any sample code for DT bindings. It should not be hard to map some sample clock bindings onto the existing registration functions... but it will still be one clock per node. I'm not really a fan of making clock black-boxes where the details of the tree are hidden from the framework anyways... that's something that clkdev already achieves to some degree (by not exposing the entire tree to drivers, but only the clocks that we want exposed). Regards, Mike Thanks Richard ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 4/4] clk: basic clock hardware types
On Wed, Mar 7, 2012 at 1:20 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sat, Mar 03, 2012 at 12:29:01AM -0800, Mike Turquette wrote:
+struct clk *clk_register_divider(struct device *dev, const char *name,
+ const char *parent_name, unsigned long flags,
+ void __iomem *reg, u8 shift, u8 width,
+ u8 clk_divider_flags, spinlock_t *lock)
+{
+ struct clk_divider *div;
+ char **parent_names = NULL;
+ u8 len;
+
+ div = kmalloc(sizeof(struct clk_divider), GFP_KERNEL);
+
+ if (!div) {
+ pr_err(%s: could not allocate divider clk\n, __func__);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ /* struct clk_divider assignments */
+ div-reg = reg;
+ div-shift = shift;
+ div-width = width;
+ div-flags = clk_divider_flags;
+ div-lock = lock;
+
+ if (parent_name) {
+ parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
+
+ if (! parent_names)
+ goto out;
+
+ len = sizeof(char) * strlen(parent_name);
+
+ parent_names[0] = kmalloc(len, GFP_KERNEL);
+
+ if (!parent_names[0])
+ goto out;
+
+ strncpy(parent_names[0], parent_name, len);
+ }
+
+out:
+ return clk_register(dev, name,
+ clk_divider_ops, div-hw,
+ parent_names,
+ (parent_name ? 1 : 0),
+ flags);
+}
clk_register_divider and also clk_register_gate have some problems.
First you allocate memory with exactly the string length without
the terminating 0. Then the functions leak memory when clk_register
fails. Could you fold in the following patch to fix this?
Thanks for the patch Sascha. This has been pulled into v6 and tested.
Regards,
Mike
Sascha
8--
fix divider/gate registration
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
drivers/clk/clk-divider.c | 34 +++---
drivers/clk/clk-gate.c | 33 ++---
include/linux/clk-provider.h | 2 ++
3 files changed, 31 insertions(+), 38 deletions(-)
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 8f02930..99b6b55 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -156,14 +156,13 @@ struct clk *clk_register_divider(struct device *dev,
const char *name,
u8 clk_divider_flags, spinlock_t *lock)
{
struct clk_divider *div;
- char **parent_names = NULL;
- u8 len;
+ struct clk *clk;
- div = kmalloc(sizeof(struct clk_divider), GFP_KERNEL);
+ div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
if (!div) {
pr_err(%s: could not allocate divider clk\n, __func__);
- return ERR_PTR(-ENOMEM);
+ return NULL;
}
/* struct clk_divider assignments */
@@ -174,25 +173,22 @@ struct clk *clk_register_divider(struct device *dev,
const char *name,
div-lock = lock;
if (parent_name) {
- parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
-
- if (! parent_names)
- goto out;
-
- len = sizeof(char) * strlen(parent_name);
-
- parent_names[0] = kmalloc(len, GFP_KERNEL);
-
- if (!parent_names[0])
+ div-parent[0] = kstrdup(parent_name, GFP_KERNEL);
+ if (!div-parent[0])
goto out;
-
- strncpy(parent_names[0], parent_name, len);
}
-out:
- return clk_register(dev, name,
+ clk = clk_register(dev, name,
clk_divider_ops, div-hw,
- parent_names,
+ div-parent,
(parent_name ? 1 : 0),
flags);
+ if (clk)
+ return clk;
+
+out:
+ kfree(div-parent[0]);
+ kfree(div);
+
+ return NULL;
}
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index e831f7b..92c0489 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -80,14 +80,13 @@ struct clk *clk_register_gate(struct device *dev, const
char *name,
u8 clk_gate_flags, spinlock_t *lock)
{
struct clk_gate *gate;
- char **parent_names = NULL;
- u8 len;
+ struct clk *clk;
- gate = kmalloc(sizeof(struct clk_gate), GFP_KERNEL);
+ gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
if (!gate) {
pr_err(%s: could not allocate gated clk\n, __func__);
- return ERR_PTR(-ENOMEM);
+ return NULL;
}
/* struct clk_gate assignments */
@@ -97,25 +96,21 @@ struct clk *clk_register_gate(struct device *dev, const
char *name,
Re: [PATCH v5 4/4] clk: basic clock hardware types
On Mon, Mar 5, 2012 at 2:17 AM, Andrew Lunn and...@lunn.ch wrote: I think i can wrap your simple gate clock, to make my complex gate clock. What would help is if you would EXPORT_SYMBOL_GPL clk_gate_enable() and clk_gate_disable(), since they do exactly what i want. I can then build my own clk_ops structure, with my own unprepare() function. I would probably use DEFINE_CLK_GATE as is, and then at run time, before calling __clk_init() overwrite the .ops with my own version. Maybe I don't get your point, but clk_unprepare should be used when you have to sleep to disable your clock. When clk_gate_disable() is exactly why do you want to use clk_unprepare instead of clk_disable? I'm trying to avoid having to implement a new clock provider. The whole point of the generic clk code is to consolidate code. It seems silly to create a new clk provider which is 95% identical to Mike's gated provider, if i can avoid it. I will export the operations in my next patchset, but I'm concerned over how useful this might be... Using your example of struct clk_gate, both clk_gate_enable and clk_gate_disable call to_clk_gate. So you would either have to re-use struct clk_gate for your own needs (which involves hacking up a specific struct clk_gate_foo_ops for your needs) or you could not use struct clk_gate and pack your data identically (struct clk_hw must be the first member) which is too horrible to imagine. Hmm, or you could re-use struct clk_gate but provide your own struct clk_ops AND your own registration functions (since you won't be able to pass in the ops to your clk_register_gate). So that sounds sane, if a bit convoluted. It does re-use code though... Regards, Mike If i stuff it into clk_disable(), it means i cannot use the basic gate clock Mike provides in the generic clock framework. Which is a shame, since it does exactly what i want in terms of gating the clock. If i can use unprepare(), which basic gate does not use, i can use Mikes code, and just extend it. It is there, it is unused, so why not use it? Andrew ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Tue, Mar 6, 2012 at 11:00 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Mon, Mar 05, 2012 at 12:03:15PM -0800, Turquette, Mike wrote:
On Sun, Mar 4, 2012 at 11:38 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sun, Mar 04, 2012 at 04:12:21PM -0800, Turquette, Mike wrote:
I believe this patch already does what you suggest, but I might be
missing your point.
In include/linux/clk-private.h you expose struct clk outside the core.
This has to be done to make static initializers possible. There is a big
warning in this file that it must not be included from files
implementing
struct clk_ops. You can simply avoid this warning by declaring struct
clk
with only a single member:
include/linux/clk.h:
struct clk {
struct clk_internal *internal;
};
This way everybody knows struct clk (thus can embed it in their static
initializers), but doesn't know anything about the internal members. Now
in drivers/clk/clk.c you declare struct clk_internal exactly like struct
clk was declared before:
struct clk_internal {
const char *name;
const struct clk_ops *ops;
struct clk_hw *hw;
struct clk *parent;
char **parent_names;
struct clk **parents;
u8 num_parents;
unsigned long rate;
unsigned long flags;
unsigned int enable_count;
unsigned int prepare_count;
struct hlist_head children;
struct hlist_node child_node;
unsigned int notifier_count;
#ifdef CONFIG_COMMON_CLK_DEBUG
struct dentry *dentry;
#endif
};
An instance of struct clk_internal will be allocated in
__clk_init/clk_register. Now the private data stays completely inside
the core and noone can abuse it.
Hi Sascha,
I see the disconnect here. For OMAP (and possibly other platforms) at
least some clock data is necessary during early boot, before the
regular allocation methods are available (timers for instance).
We had this problem on i.MX aswell. It turned out that the timer clock
is the only clock that is needed so early. We solved this by moving the
clock init to the system timer init function.
When you say mov[ed] the clock init to the system timer init
function do you mean that you statically allocated struct clk and
used the clk framework api, or instead you just did some direct
register writes to initialize things properly?
I meant that on i.MX we do the clock tree initialization when kmalloc is
available, see the attached patch for omap4 based on your branch which
does the same for Omap. The first clock you need is the one for the
timer, so you can initialize the clocktree at the beginning of
time_init() and don't need statically initialized clocks anymore.
Well, the file is work in progress, you probably fix this before sending
it out, but I bet people will include clk-private.h and nobody else
notices it.
clock44xx_data.c does not violate that rule. None of the logic that
implements ops for those clocks is present clock44xx_data.c.
Indeed, I missed that. It only has the ops but not the individual
functions.
All of
the code in that file is simply initialization and registration of
OMAP4 clocks. Many of the clocks are basic clock types (divider,
multiplexer and fixed-rate are used in that file) with protected code
drivers/clk/clk-*.c and the remaining clocks are of the struct
clk_hw_omap variety, which has code spread across several files:
arch/arm/mach-omap2/clock.c
arch/arm/mach-omap2/clock.h
arch/arm/mach-omap2/clkt_dpll.c
arch/arm/mach-omap2/clkt_clksel.c
arch/arm/mach-omap2/dpll3xxx.c
arch/arm/mach-omap2/dpll4xxx.c
All of the above files include linux/clk-provider.h, not
linux/clk-private.h. That code makes heavy use of the
__clk_get_whatever helpers and shows how a platform might honor the
layer of separation between struct clk and stuct clk_ops/struct
clk_foo. You are correct that the code is a work-in-progress, but
there are no layering violations that I can see.
I also think we are talking past each other to some degree. One point
I would like to make (and maybe you already know this from code
review) is that it is unnecessary to have pointers to your parent
struct clk*'s when either initializing or registering your clocks. In
fact the existing clk_register_foo functions don't even allow you to
pass in parent pointers and rely wholly on string name matching. I
just wanted to point that out in case it went unnoticed, as it is a
new way of doing things from the previous series and was born out of
Thomas' review of V4 and multi-parent handling. This also keeps
device-tree in mind where we might not know the struct clk *pointer at
compile time
Re: [PATCH v5 2/4] clk: Kconfig: add entry for HAVE_CLK_PREPARE
On Sun, Mar 4, 2012 at 6:04 PM, Richard Zhao richard.z...@freescale.com wrote: On Sat, Mar 03, 2012 at 12:28:59AM -0800, Mike Turquette wrote: The common clk framework provides clk_prepare and clk_unprepare implementations. Create an entry for HAVE_CLK_PREPARE so that COMMON_CLK can select it. Signed-off-by: Mike Turquette mturque...@linaro.org Signed-off-by: Mike Turquette mturque...@ti.com Acked-by: Shawn Guo shawn@linaro.org Cc: Jeremy Kerr jeremy.k...@canonical.com Cc: Thomas Gleixner t...@linutronix.de Cc: Arnd Bergman arnd.bergm...@linaro.org Cc: Paul Walmsley p...@pwsan.com Cc: Richard Zhao richard.z...@linaro.org Cc: Saravana Kannan skan...@codeaurora.org Cc: Magnus Damm magnus.d...@gmail.com Cc: Rob Herring rob.herr...@calxeda.com Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Stephen Boyd sb...@codeaurora.org Cc: Amit Kucheria amit.kuche...@linaro.org Cc: Deepak Saxena dsax...@linaro.org Cc: Grant Likely grant.lik...@secretlab.ca Cc: Andrew Lunn and...@lunn.ch --- drivers/clk/Kconfig | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 9b3cd08..3912576 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -8,3 +8,6 @@ config HAVE_CLK_PREPARE config HAVE_MACH_CLKDEV bool + +config HAVE_CLK_PREPARE + bool We've already had it. redundant? I didn't realize that Shawn merged this in since the last posting of V4... Will drop it. Regards, Mike Thanks Richard -- 1.7.5.4 ___ 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 v5 3/4] clk: introduce the common clock framework
On Sun, Mar 4, 2012 at 11:38 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sun, Mar 04, 2012 at 04:12:21PM -0800, Turquette, Mike wrote:
I believe this patch already does what you suggest, but I might be
missing your point.
In include/linux/clk-private.h you expose struct clk outside the core.
This has to be done to make static initializers possible. There is a big
warning in this file that it must not be included from files implementing
struct clk_ops. You can simply avoid this warning by declaring struct clk
with only a single member:
include/linux/clk.h:
struct clk {
struct clk_internal *internal;
};
This way everybody knows struct clk (thus can embed it in their static
initializers), but doesn't know anything about the internal members. Now
in drivers/clk/clk.c you declare struct clk_internal exactly like struct
clk was declared before:
struct clk_internal {
const char *name;
const struct clk_ops *ops;
struct clk_hw *hw;
struct clk *parent;
char **parent_names;
struct clk **parents;
u8 num_parents;
unsigned long rate;
unsigned long flags;
unsigned int enable_count;
unsigned int prepare_count;
struct hlist_head children;
struct hlist_node child_node;
unsigned int notifier_count;
#ifdef CONFIG_COMMON_CLK_DEBUG
struct dentry *dentry;
#endif
};
An instance of struct clk_internal will be allocated in
__clk_init/clk_register. Now the private data stays completely inside
the core and noone can abuse it.
Hi Sascha,
I see the disconnect here. For OMAP (and possibly other platforms) at
least some clock data is necessary during early boot, before the
regular allocation methods are available (timers for instance).
We had this problem on i.MX aswell. It turned out that the timer clock
is the only clock that is needed so early. We solved this by moving the
clock init to the system timer init function.
When you say mov[ed] the clock init to the system timer init
function do you mean that you statically allocated struct clk and
used the clk framework api, or instead you just did some direct
register writes to initialize things properly?
Due
to this my idea of static initialization was to take care of
everything that would normally require an allocator, which includes
the internals of struct clk; thus exposing struct clk is useful here
as you can still use the clock framework during very early boot. Your
method above doesn't satisfy this requirement. I'm not sure what the
purpose would be of statically allocating your version of struct clk,
which will ultimately need to allocate memory for the clock internals
anyways. Can you elaborate the benefit of this approach over just
using the clk_foo_register functions?
As said the benefit is that you do not have to expose the internal
layout of struct clk outside the clock framework. Note that in the
That is a benefit for sure, but if it does not solve the problem of
allowing for static allocation then we still have an issue.
file you referenced here:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/clock44xx_data.c;h=7f833a7b2dca84a52c2bd1e7c8d9cfe560771258;hb=v3.3-rc5-clkv5-omap#l205
You violate what you have in a comment above clk-private.h:
/* __clk_init is only exposed via clk-private.h and is intended for use with
* very large numbers of clocks that need to be statically initialized. It is
* a layering violation to include clk-private.h from any code which
implements
* a clock's .ops; as such any statically initialized clock data MUST be in
* separate C file from the logic that implements it's operations.
*/
Well, the file is work in progress, you probably fix this before sending
it out, but I bet people will include clk-private.h and nobody else
notices it.
clock44xx_data.c does not violate that rule. None of the logic that
implements ops for those clocks is present clock44xx_data.c. All of
the code in that file is simply initialization and registration of
OMAP4 clocks. Many of the clocks are basic clock types (divider,
multiplexer and fixed-rate are used in that file) with protected code
drivers/clk/clk-*.c and the remaining clocks are of the struct
clk_hw_omap variety, which has code spread across several files:
arch/arm/mach-omap2/clock.c
arch/arm/mach-omap2/clock.h
arch/arm/mach-omap2/clkt_dpll.c
arch/arm/mach-omap2/clkt_clksel.c
arch/arm/mach-omap2/dpll3xxx.c
arch/arm/mach-omap2/dpll4xxx.c
All of the above files include linux/clk-provider.h, not
linux/clk-private.h. That code makes heavy use of the
__clk_get_whatever helpers and shows how a platform might honor the
layer of separation between struct
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Sun, Mar 4, 2012 at 3:52 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sat, Mar 03, 2012 at 09:14:43AM -0800, Turquette, Mike wrote:
On Sat, Mar 3, 2012 at 5:31 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sat, Mar 03, 2012 at 12:29:00AM -0800, 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.
+
+/**
+ * struct clk_hw - handle for traversing from a struct clk to its
corresponding
+ * hardware-specific structure. struct clk_hw should be declared within
struct
+ * clk_foo and then referenced by the struct clk instance that uses
struct
+ * clk_foo's clk_ops
+ *
+ * clk: pointer to the struct clk instance that points back to this
struct
+ * clk_hw instance
+ */
+struct clk_hw {
+ struct clk *clk;
+};
The reason for doing this is that struct clk should be an opaque cookie
for both drivers and implementers of clocks. I recently had the idea
whether
the roles of these two structs could be swapped. So instead of the above we
could do:
struct clk {
struct clk_hw *hw;
}
Firstly, struct clk is an opaque cookie for both drivers and
implementers of clocks with this patchset.
Secondly, struct clk does indeed have a pointer to struct clk_hw.
Refer to include/linux/clk-private.h in this patch.
The reference is cyclical. A reference to struct clk can navigate to
struct clk_foo via container_of (usually something like #define
to_clk_foo(_hw) container_of(_hw, struct clk_foo, hw) where struct
clk's pointer to it's .hw member is passed into one of the struct
clk_ops callbacks.
Likewise if struct clk_foo needs the struct clk ptr for any reason
then it can get it from foo-hw-clk.
I believe this patch already does what you suggest, but I might be
missing your point.
In include/linux/clk-private.h you expose struct clk outside the core.
This has to be done to make static initializers possible. There is a big
warning in this file that it must not be included from files implementing
struct clk_ops. You can simply avoid this warning by declaring struct clk
with only a single member:
include/linux/clk.h:
struct clk {
struct clk_internal *internal;
};
This way everybody knows struct clk (thus can embed it in their static
initializers), but doesn't know anything about the internal members. Now
in drivers/clk/clk.c you declare struct clk_internal exactly like struct
clk was declared before:
struct clk_internal {
const char *name;
const struct clk_ops *ops;
struct clk_hw *hw;
struct clk *parent;
char **parent_names;
struct clk **parents;
u8 num_parents;
unsigned long rate;
unsigned long flags;
unsigned int enable_count;
unsigned int prepare_count;
struct hlist_head children;
struct hlist_node child_node;
unsigned int notifier_count;
#ifdef CONFIG_COMMON_CLK_DEBUG
struct dentry *dentry;
#endif
};
An instance of struct clk_internal will be allocated in
__clk_init/clk_register. Now the private data stays completely inside
the core and noone can abuse it.
Hi Sascha,
I see the disconnect here. For OMAP (and possibly other platforms) at
least some clock data is necessary during early boot, before the
regular allocation methods are available (timers for instance). Due
to this my idea of static initialization was to take care of
everything that would normally require an allocator, which includes
the internals of struct clk; thus exposing struct clk is useful here
as you can still use the clock framework during very early boot. Your
method above doesn't satisfy this requirement. I'm not sure what the
purpose would be of statically allocating your version of struct clk,
which will ultimately need to allocate memory for the clock internals
anyways. Can you elaborate the benefit of this approach over just
using the clk_foo_register functions?
Note that struct clk is still declared in clk.h (and clk-provider.h),
so platforms that have struct clk *clk declarations won't have a
problem
Re: [PATCH v5 4/4] clk: basic clock hardware types
On Sun, Mar 4, 2012 at 6:35 AM, Andrew Lunn and...@lunn.ch wrote:
+#define DEFINE_CLK_GATE(_name, _parent_name, _parent_ptr, \
+ _flags, _reg, _bit_idx, \
+ _gate_flags, _lock) \
+ static struct clk _name; \
+ static char *_name##_parent_names[] = { \
+ _parent_name, \
+ }; \
+ static struct clk *_name##_parents[] = { \
+ _parent_ptr, \
+ }; \
+ static struct clk_gate _name##_hw = { \
+ .hw = { \
+ .clk = _name, \
+ }, \
+ .reg = _reg, \
+ .bit_idx = _bit_idx, \
+ .flags = _gate_flags \
+ .lock = _lock, \
+ }; \
+ static struct clk _name = { \
+ .name = #_name, \
+ .ops = clk_gate_ops, \
+ .hw = _name##_hw.hw, \
+ .parent_names = _name##_parent_names, \
+ .num_parents = \
+ ARRAY_SIZE(_name##parent_names), \
Hi Mike
This should be _name##_parent_names, i.e. you are missing a _.
With this and the previous change, i get something which at least
compiles...
The gate clock is the only type of the basic clocks that I do not
(currently) use in my OMAP port and it's test coverage has suffered as
a result. My bad.
Thanks for the review and the patch in the separate thread. I'll take
the changes in.
Regards,
Mike
Andrew
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 4/4] clk: basic clock hardware types
On Sun, Mar 4, 2012 at 9:42 AM, Andrew Lunn and...@lunn.ch wrote: On Sat, Mar 03, 2012 at 12:29:01AM -0800, Mike Turquette wrote: Many platforms support simple gateable clocks, fixed-rate clocks, adjustable divider clocks and multi-parent multiplexer clocks. This patch introduces basic clock types for the above-mentioned hardware which share some common characteristics. Hi Mike These basic clocks don't allow the use of prepare/unprepare, from the side of the clock provider. I think i need this. This is an interesting point and might help us nail down exactly what we expect from clk_prepare/clk_unprepare. The Orion Kirkwood SoC has clock gating for most of its on chip peripherals, which the other older Orion devices don't have. The SATA and PCIe also allows the PHY to be shut down, again which older Orion devices don't have. The current code links the clock and the PHY together, shutting both down are the same time. So i would like to perform the PHY shutdown in the unprepare() function of the clk driver. Do you feel it is The Right Thing to enable/disable the phy from clk_prepare/clk_unprepare? I had always imagined that clk_prepare's purpose was always specific to clock-related activities; for example maybe turn on a parent PLL that takes a long time to lock, as well as for the sleepable cases such as clocks over i2c, etc. However, I know that some others have already started performing voltage scaling from clk_prepare, so clearly the api is not strict in it's purpose. Have you investigated powering up/down the phy from a different layer such pm_runtime? That might create a better layer of separation between other resources needed by an IP block and the clock framework. For instance on OMAP we use pm_runtime_get(_sync) to call clk_prepare clk_enable and handle other power management-related resources. Perhaps using the clock framework to power up/down your IP block is not the right tool for the job. Is allowing to pass prepare/unprepare functions to basic clocks something you want to support? If i prepare a patch would you consider it? My original instinct was no. The simple gate clock was always supposed to be simple and if you need more than it provides then it might be best for your platform to implement a specific clock type. Especially since the purpose of clk_prepare is still up in the air. But I'll keep an open mind and hopefully others can chime in. Please let me know what you think about my pm_runtime suggestion. I've Cc'd Rafael just in case he has something to add here from a design perspective. Regards, Mike Thanks Andrew ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] clk: Fix compile errors in DEFINE_CLK_GATE
On Sun, Mar 4, 2012 at 12:33 PM, Andrew Lunn and...@lunn.ch wrote:
From 71e9a676b2b2f0dc2bb0cc395e8325cf38f4808b Mon Sep 17 00:00:00 2001
From: Andrew Lunn and...@lunn.ch
Date: Sun, 4 Mar 2012 16:31:14 +0100
Subject: [PATCH] [clk] Fix compile errors in DEFINE_CLK_GATE()
Signed-off-by: Andrew Lunn and...@lunn.ch
Thanks Andrew. Will roll this fix in.
Regards,
Mike
---
include/linux/clk-private.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index d06e6fb..9d5c4b1 100644
--- a/include/linux/clk-private.h
+++ b/include/linux/clk-private.h
@@ -95,7 +95,7 @@ extern struct clk_ops clk_gate_ops;
}, \
.reg = _reg, \
.bit_idx = _bit_idx, \
- .flags = _gate_flags \
+ .flags = _gate_flags, \
.lock = _lock, \
}; \
static struct clk _name = { \
@@ -104,7 +104,7 @@ extern struct clk_ops clk_gate_ops;
.hw = _name##_hw.hw, \
.parent_names = _name##_parent_names, \
.num_parents = \
- ARRAY_SIZE(_name##parent_names), \
+ ARRAY_SIZE(_name##_parent_names), \
.parents = _name##_parents, \
.flags = _flags, \
};
--
1.7.2.5
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Sat, Mar 3, 2012 at 5:31 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sat, Mar 03, 2012 at 12:29:00AM -0800, 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.
+
+/**
+ * struct clk_hw - handle for traversing from a struct clk to its
corresponding
+ * hardware-specific structure. struct clk_hw should be declared within
struct
+ * clk_foo and then referenced by the struct clk instance that uses struct
+ * clk_foo's clk_ops
+ *
+ * clk: pointer to the struct clk instance that points back to this struct
+ * clk_hw instance
+ */
+struct clk_hw {
+ struct clk *clk;
+};
The reason for doing this is that struct clk should be an opaque cookie
for both drivers and implementers of clocks. I recently had the idea whether
the roles of these two structs could be swapped. So instead of the above we
could do:
struct clk {
struct clk_hw *hw;
}
Firstly, struct clk is an opaque cookie for both drivers and
implementers of clocks with this patchset.
Secondly, struct clk does indeed have a pointer to struct clk_hw.
Refer to include/linux/clk-private.h in this patch.
The reference is cyclical. A reference to struct clk can navigate to
struct clk_foo via container_of (usually something like #define
to_clk_foo(_hw) container_of(_hw, struct clk_foo, hw) where struct
clk's pointer to it's .hw member is passed into one of the struct
clk_ops callbacks.
Likewise if struct clk_foo needs the struct clk ptr for any reason
then it can get it from foo-hw-clk.
I believe this patch already does what you suggest, but I might be
missing your point.
The effect would be the same, but this version would make the struct clk
pointer known at compile time thus making it possible for static
initializers to specify their parent clock.
This series achieves this goal also. include/linux/clk-private.h
defines struct clk as well as macros for statically initializing the
basic clock types. Pointers to the struct clk of a parent clock can
and are pre-populated in these macros, along with the mandatory array
of parent string names. (note that parent pointer initialization is
optional and you can always pass in NULL into that specific field of
the macro)
I just saw that clk_register now takes a parent name array, that may
make this change unnecessary anyway.
clk_register exists for dynamic allocation and solves a different
problem from what you describe above. For statically initialized
clocks the following is sufficient:
#include linux/clk-private.h
static char *abe_dpll_refclk_mux_ck_parent_names[] = {
sys_clkin_ck,
sys_32k_ck,
};
static struct clk *abe_dpll_refclk_mux_ck_parents[] = {
sys_clkin_ck,
sys_32k_ck,
};
DEFINE_CLK_MUX(abe_dpll_refclk_mux_ck,
abe_dpll_refclk_mux_ck_parent_names,
abe_dpll_refclk_mux_ck_parents,
0x0,
OMAP4430_CM_ABE_PLL_REF_CLKSEL,
OMAP4430_CLKSEL_0_0_SHIFT,
OMAP4430_CLKSEL_0_0_WIDTH,
0x0,
NULL);
__clk_init(NULL, abe_dpll_refclk_mux_ck);
You might find it helpful to see how I've handled statically
initialized clocks for OMAP4. These patches are a work in progress
but I'll send an RFC to the list very soon since others might find it
useful. For now the above example can be seen in more detail at:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/clock44xx_data.c;h=7f833a7b2dca84a52c2bd1e7c8d9cfe560771258;hb=v3.3-rc5-clkv5-omap#l205
Let me know if I've missed your point somehow.
Thanks for the review,
Mike
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 v5 1/9] cpuidle: Add commonly used functionality for consolidation
On Tue, Feb 28, 2012 at 3:33 PM, Rob Lee rob@linaro.org wrote: I brought this topic up internally and Jon suggested that the 'usage' statistics that are reported in sysfs should also reflect failed versus successful C-state transitions, which is a great idea. This could simply be achieved by renaming the current 'usage' count to something like 'transitions_attempted' and then conditionally increment a new counter within the 'if (entered_state = 0)' block, perhaps named, 'transition_succeeded'. This way the old 'usage' count paradigm is as accurate as the new time-keeping code. Being able to easily observe which C-state tend to fail the most would be invaluable in tuning idle policy for maximum effectiveness. Thoughts? Sounds reasonable. In some cases it may be helpful to track state demotion as well. Since I'm still a noob and wearing my submission training wheels, I'm trying to minimize things that fall outside of this basic consolidation effort for this patch series. But I added Jon's suggestion to this cpuidle page which contains future cpuidle items to consider adding: https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/CPUIdle#Track_both_attempted_and_successful_enter_attempts Yeah, I don't want to feature-bloat your submission more than necessary. I'm happy for the usage counter stuff to get tackled at a later date, but you're still on board for setting last_residency to zero in this series, right? Regards, Mike Regards, Mike Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob@linaro.org wrote:
+/**
+ * cpuidle_enter_wrap - performing timekeeping and irq around enter function
+ * @dev: pointer to a valid cpuidle_device object
+ * @drv: pointer to a valid cpuidle_driver object
+ * @index: index of the target cpuidle state.
+ */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index,
+ int (*enter)(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int
index))
+{
+ ktime_t time_start, time_end;
+ s64 diff;
+
+ time_start = ktime_get();
+
+ index = enter(dev, drv, index);
+
+ time_end = ktime_get();
+
+ local_irq_enable();
+
+ diff = ktime_to_us(ktime_sub(time_end, time_start));
+ if (diff INT_MAX)
+ diff = INT_MAX;
+
+ dev-last_residency = (int) diff;
+
+ return index;
+}
Hi Rob,
In a previous series I brought up the idea of not accounting for time
if a C-state transition fails. My post on that thread can be found
here:
http://article.gmane.org/gmane.linux.ports.arm.kernel/149293/
How do you feel about adding something like the following?
if (IS_ERR(index))
dev-last_residency = 0;
return index;
Obviously it will up to the platforms to figure out how to propagate
that error up from their respective low power code.
Regards,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH v4 1/4] cpuidle: Add time keeping and irq enabling
On Sat, Feb 4, 2012 at 11:02 AM, Colin Cross ccr...@google.com wrote:
On Tue, Jan 31, 2012 at 7:00 PM, Robert Lee rob@linaro.org wrote:
Make necessary changes to add implement time keepign and irq enabling
keeping
in the core cpuidle code. This will allow the remove of these
functionalities from the platform cpuidle implementations.
Signed-off-by: Robert Lee rob@linaro.org
---
drivers/cpuidle/cpuidle.c | 75
+++-
include/linux/cpuidle.h | 26 ++-
2 files changed, 76 insertions(+), 25 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 59f4261..8ea0fc3 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -57,14 +57,18 @@ static int __cpuidle_register_device(struct
cpuidle_device *dev);
* cpuidle_idle_call - the main idle loop
*
* NOTE: no locks or semaphores should be used here
+ * NOTE: Should only be called from a local irq disabled context
* return non-zero on failure
+ *
*/
int cpuidle_idle_call(void)
{
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_driver();
struct cpuidle_state *target_state;
- int next_state, entered_state;
+ int idx, ret = 0;
+ ktime_t t1, t2;
+ s64 diff;
if (off)
return -ENODEV;
@@ -86,37 +90,76 @@ int cpuidle_idle_call(void)
#endif
/* ask the governor for the next state */
- next_state = cpuidle_curr_governor-select(drv, dev);
+ idx = cpuidle_curr_governor-select(drv, dev);
+
+ target_state = drv-states[idx];
+
+ /*
+ * Check with the device to see if it can enter this state or if
another
+ * state should be used.
+ */
+ if (target_state-pre_enter) {
+ idx = target_state-
+ pre_enter(dev, drv, idx);
Unnecessary line wrap and braces.
What's the point of the pre_enter call? This seems very similar to
the prepare call that was removed in 3.2. Drivers can already demote
Hi Colin,
I asked Rob to re-introduce the .prepare callback (not .pre_enter).
The short version of why I requested this is so that I can experiment
with modifying wake-up latency and theshold values dynamically based
on PM QoS constraints. For an OMAP-specific example, I'd like to see
our C-states no longer model both MPU and CORE power domains, and
instead only model the MPU. Then when entering idle the PM QoS
constraints against the CORE power domain's wake-up latency can be
considered in the .prepare callback which will affect the C-state
parameters as well as program the CORE low power target state.
.pre_enter isn't really right for the above case so I'm happy for it
to be dropped, but I'll probably re-introduce something like .prepare
in the future...
Regards,
Mike
the target state in their enter call. The only thing you do between
pre_enter and enter is trace and account for the time. Is there some
long call you don't want included in the idle time? Some
documentation would help, and you need to very clearly define the
semantics of when post_enter gets called when pre_enter or enter
return errors.
___
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: [RFC PATCH v4 1/4] cpuidle: Add time keeping and irq enabling
On Sat, Feb 4, 2012 at 5:36 PM, Colin Cross ccr...@google.com wrote: On Sat, Feb 4, 2012 at 2:06 PM, Turquette, Mike mturque...@ti.com wrote: On Sat, Feb 4, 2012 at 11:02 AM, Colin Cross ccr...@google.com wrote: What's the point of the pre_enter call? This seems very similar to the prepare call that was removed in 3.2. Drivers can already demote Hi Colin, I asked Rob to re-introduce the .prepare callback (not .pre_enter). The short version of why I requested this is so that I can experiment with modifying wake-up latency and theshold values dynamically based on PM QoS constraints. For an OMAP-specific example, I'd like to see our C-states no longer model both MPU and CORE power domains, and instead only model the MPU. Then when entering idle the PM QoS constraints against the CORE power domain's wake-up latency can be considered in the .prepare callback which will affect the C-state parameters as well as program the CORE low power target state. prepare makes sense to adjust latencies, as long as it is not used for state demotion as well. Latency adjustment is the plan. State demotion is not. Rob, If you cook up another version of this series feel free to drop .pre_enter. It would be nice if you re-introduced .prepare as per our original discussion but if that's a roadblock then you can forget that point too and I'll take a crack at it later. Regards, Mike .pre_enter isn't really right for the above case so I'm happy for it to be dropped, but I'll probably re-introduce something like .prepare in the future... Regards, Mike the target state in their enter call. The only thing you do between pre_enter and enter is trace and account for the time. Is there some long call you don't want included in the idle time? Some documentation would help, and you need to very clearly define the semantics of when post_enter gets called when pre_enter or enter return errors. ___ 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 v4 2/6] Documentation: common clk API
On Thu, Jan 5, 2012 at 6:31 AM, Amit Kucheria amit.kuche...@linaro.org wrote: Tiny, tiny typo... On 11 Dec 13, Mike Turquette wrote: +clk_set_rate deserves a special mention because it is more complex than +the other operations. There are three key concepts to the common +clk_set_rate implementation: + +1) recursively traversing up the clk tree and changing clk rates, one +parent at a time, if each clk allows it +2) failing to change rate if the clk is enabled and must only change +rates while disabled +3) using clk rate change notifiers to allow devices to handle dynamic +rate changes for clks which do support changing rates while enabled + +For the simple, non-recursive case the call graph looks like: + +clk_set_rate(clk, rate); + __clk_set_rate(clk, rate); + clk-round_rate(clk, rate *parent_rate); need a comma here? The next sentence kept me busy for 5 mins. Thanks, will fix in next submission (along with general rework of messy documentation). Mike + clk-set_rate(clk, rate); + +You might be wondering what that third paramater in .round_rate is. If +a clk supports the CLK_PARENT_SET_RATE flag then that enables it's +hardware-specific .round_rate function to provide a new rate that the +parent should transition to. For example, imagine a rate-adjustable clk +A that is the parent of clk B, which has a fixed divider of 2. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring robherri...@gmail.com wrote:
On 01/03/2012 08:15 PM, Richard Zhao wrote:
On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner t...@linutronix.de wrote:
On Tue, 13 Dec 2011, Mike Turquette wrote:
snip
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware for the
+ * parent and rate and sets them both. Adds the clk to the sysfs tree
+ * topology.
+ *
+ * Caller must populate clk-name and clk-flags before calling
I'm not too happy about this construct. That leaves struct clk and its
members exposed to the world instead of making it a real opaque
cookie. I know from my own painful experience, that this will lead to
random fiddling in that data structure in drivers and arch code just
because the core code has a shortcoming.
Why can't we make struct clk a real cookie and confine the data
structure to the core code ?
That would change the init call to something like:
struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
struct clk *parent)
And have:
struct clk_hw {
struct clk_hw_ops *ops;
const char *name;
unsigned long flags;
};
Implementers can do:
struct my_clk_hw {
struct clk_hw hw;
mydata;
};
And then change the clk ops callbacks to take struct clk_hw * as an
argument.
We have to define static clocks before we adopt DT binding.
If clk is opaque and allocate memory in clk core, it'll make hard
to define static clocks. And register/init will pass a long parameter
list.
DT is not a prerequisite for having dynamically created clocks. You can
make clock init dynamic without DT.
Agreed.
What data goes in struct clk vs. struct clk_hw could change over time.
So perhaps we can start with some data in clk_hw and plan to move it to
struct clk later. Even if almost everything ends up in clk_hw initially,
at least the structure is in place to have common, core-only data
separate from platform data.
What is the point of this?
The original clk_hw was defined simply as:
struct clk_hw {
struct clk *clk;
};
It's only purpose in life was as a handle for navigation between the
opaque struct clk and the hardware-specific struct my_clk_hw. struct
clk_hw is defined in clk.h and everyone can see it. If we're suddenly
OK putting clk data in this structure then why bother with an opaque
struct clk at all?
What is the actual data you need to be static and accessible to the
platform code? A ptr to parent clk is the main thing I've seen for
static initialization. So make the parent ptr be struct clk_hw* and
allow the platforms to access.
To answer your question on what data we're trying to expose: platform
code commonly needs the parent pointer and the clk rate (and by
extension, the rate of the parent). For debug/error prints it is also
nice to have the clk name. Generic clk flags are also conceivably
something that platform code might want.
I'd like to spin the question around: if we're OK exposing some stuff
(in your example above, the parent pointer), then what clk data are
you trying to hide?
Regards,
Mike
Rob
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring robherri...@gmail.com wrote:
On 01/04/2012 07:01 PM, Turquette, Mike wrote:
On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring robherri...@gmail.com wrote:
On 01/03/2012 08:15 PM, Richard Zhao wrote:
On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner t...@linutronix.de
wrote:
On Tue, 13 Dec 2011, Mike Turquette wrote:
snip
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware for the
+ * parent and rate and sets them both. Adds the clk to the sysfs tree
+ * topology.
+ *
+ * Caller must populate clk-name and clk-flags before calling
I'm not too happy about this construct. That leaves struct clk and its
members exposed to the world instead of making it a real opaque
cookie. I know from my own painful experience, that this will lead to
random fiddling in that data structure in drivers and arch code just
because the core code has a shortcoming.
Why can't we make struct clk a real cookie and confine the data
structure to the core code ?
That would change the init call to something like:
struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
struct clk *parent)
And have:
struct clk_hw {
struct clk_hw_ops *ops;
const char *name;
unsigned long flags;
};
Implementers can do:
struct my_clk_hw {
struct clk_hw hw;
mydata;
};
And then change the clk ops callbacks to take struct clk_hw * as an
argument.
We have to define static clocks before we adopt DT binding.
If clk is opaque and allocate memory in clk core, it'll make hard
to define static clocks. And register/init will pass a long parameter
list.
DT is not a prerequisite for having dynamically created clocks. You can
make clock init dynamic without DT.
Agreed.
What data goes in struct clk vs. struct clk_hw could change over time.
So perhaps we can start with some data in clk_hw and plan to move it to
struct clk later. Even if almost everything ends up in clk_hw initially,
at least the structure is in place to have common, core-only data
separate from platform data.
What is the point of this?
To have a way forward. It would be nice to have a clk infrastructure
before I retire...
Haha, agreed.
The original clk_hw was defined simply as:
struct clk_hw {
struct clk *clk;
};
It's only purpose in life was as a handle for navigation between the
opaque struct clk and the hardware-specific struct my_clk_hw. struct
clk_hw is defined in clk.h and everyone can see it. If we're suddenly
OK putting clk data in this structure then why bother with an opaque
struct clk at all?
What is the actual data you need to be static and accessible to the
platform code? A ptr to parent clk is the main thing I've seen for
static initialization. So make the parent ptr be struct clk_hw* and
allow the platforms to access.
To answer your question on what data we're trying to expose: platform
code commonly needs the parent pointer and the clk rate (and by
extension, the rate of the parent). For debug/error prints it is also
nice to have the clk name. Generic clk flags are also conceivably
something that platform code might want.
I agree with the need to have the parent and flags from a static init
perspective. There's not really a good reason the others can't be
accessed thru accessors though.
I'd like to spin the question around: if we're OK exposing some stuff
(in your example above, the parent pointer), then what clk data are
you trying to hide?
Well, everything from drivers which the current clk implementations do
hide. Catching abuse in with drivers coming in from all different trees
and lists will be impossible.
For platform code it is more fuzzy. I don't think platform code should
be allowed to muck with prepare/enable counts for example.
So there is a clear dichotomy: drivers shouldn't be exposed to any of
it and platform code should be exposed to some of it.
How about a drivers/clk/clk-private.h which will define struct clk and
must only be included by clk drivers in drivers/clk/*?
This establishes a bright line between those things which are allowed
to know the details of struct clk and those that are not: namely that
clk drivers in drivers/clk/ may use '#include clk-private.h'.
Obviously struct clk is opaque to the rest of the kernel (in the same
way it has been prior to the common clk patches) and there is no need
for struct clk_hw anymore. Also helper functions are no longer needed
for clock driver code, which I think *is* a manageable set of code to
review. Also clk drivers must live in drivers/clk/ for this to work
(without a big ugly path in someone's #include directive somewhere).
Thoughts?
Regards,
Mike
Rob
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner t...@linutronix.de wrote:
On Tue, 13 Dec 2011, Mike Turquette wrote:
+void __clk_unprepare(struct clk *clk)
+{
+ if (!clk)
+ return;
+
+ if (WARN_ON(clk-prepare_count == 0))
+ return;
+
+ if (--clk-prepare_count 0)
+ return;
+
+ WARN_ON(clk-enable_count 0);
So this leaves the clock enable count set. I'm a bit wary about
that. Shouldn't it either return (including bumping the prepare_count
again) or call clk_disable() ?
I've hit this in my port of OMAP. It comes from this simple situation:
driver 1 (adapted for clk_prepare/clk_unprepare):
clk_prepare(clk);
clk_enable(clk);
...
driver2 (not adapted for clk_prepare/clk_unprepare):
clk_enable(clk);
...
driver1:
clk_disable(clk);
clk_unprepare(clk);
In such a case we don't want to bump the prepare_count, since the
offending driver2 won't decrement that count. Also we don't want to
shut down that clk since driver2 is using it.
Returning after the WARN is maybe OK, but the point of this check is
really to find code which hasn't yet been made prepare-aware, and the
current implementation is noisy enough about it.
+/**
+ * clk_get_parent - return the parent of a clk
+ * @clk: the clk whose parent gets returned
+ *
+ * Simply returns clk-parent. It is up to the caller to hold the
+ * prepare_lock, if desired. Returns NULL if clk is NULL.
Holding the prepare lock in the caller will deadlock. So it's not a
real good idea to document it as an option :)
Oops. That comment is left over from before clk_get_parent held the
lock. Will remove.
+ */
+struct clk *clk_get_parent(struct clk *clk)
+{
+ struct clk *parent;
+
+ if (!clk)
+ return NULL;
+
+ mutex_lock(prepare_lock);
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware for the
+ * parent and rate and sets them both. Adds the clk to the sysfs tree
+ * topology.
+ *
+ * Caller must populate clk-name and clk-flags before calling
I'm not too happy about this construct. That leaves struct clk and its
members exposed to the world instead of making it a real opaque
cookie. I know from my own painful experience, that this will lead to
random fiddling in that data structure in drivers and arch code just
because the core code has a shortcoming.
Why can't we make struct clk a real cookie and confine the data
structure to the core code ?
That would change the init call to something like:
struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
struct clk *parent)
And have:
struct clk_hw {
struct clk_hw_ops *ops;
const char *name;
unsigned long flags;
};
Implementers can do:
struct my_clk_hw {
struct clk_hw hw;
mydata;
};
And then change the clk ops callbacks to take struct clk_hw * as an
argument.
So the core code can allocate the clk data structure and you return a
real opaque cookie. You do the same thing for the basic gated clock in
one of the next patches, so doing it for struct clk itself is just
consequent.
The opaque cookie would work if platform code never needs any
information outside of the data a single clk_hw_whatever provides.
Unfortunately hardware doesn't make things that easy on us and the
operations we do for a single clk are affected by the state of other
clks.
Here is an example I've been using a lot lately: on OMAP we have two
clock inputs to our PLLs: a bypass clk and reference clk (actually we
can have more than this). When changing the PLL rate both clks must
be enabled, regardless of which clk ends up driving the PLL. To
illustrate, the PLL may be driven by clk_ref at 400MHz, and then we
call clk_set_rate(pll_clk, 19600); which will also leave it
clocked by clk_ref but at 196MHz. For this to happen however, the
clk_bypass had to be enabled during the rate change operation. Here
is the relevant .set_rate implementation:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/dpll3xxx.c;h=b2412574b63829944593c1a7a6eda5fa4350686f;hb=738bde65918ae1ac743b1498801da0b860e2ee32#l461
In order for the OMAP platform code to do this, we have to have two
things: firstly we need the struct clk so that we can call
clk_enable/disable and __clk_prepare/unprepare on the ref and bypass
clks from within .set_rate. The second thing is that we need
__clk_prepare and __clk_unprepare to be visible by this code so that
we don't nest the prepare_lock mutex.
Is there a good way to solve such problems if we hide struct clk from
the platform code/clk driver implementation?
Also note that if top-level clk APIs are going to be holding
prepare_lock (clk_get_rate, clk_get_parent, etc) then we can't call
these APIs to get the
Re: [PATCH v4 5/6] clk: basic gateable and fixed-rate clks
On Tue, Dec 13, 2011 at 9:15 PM, Ryan Mallon rmal...@gmail.com wrote:
On 14/12/11 14:53, Mike Turquette wrote:
Many platforms support simple gateable clks and fixed-rate clks that
should not be re-implemented by every platform.
This patch introduces a gateable clk with a common programming model of
gate control via a write of 1 bit to a register. Both set-to-enable and
clear-to-enable are supported.
Also introduced is a fixed-rate clk which has no reprogrammable aspects.
The purpose of both types of clks is documented in drivers/clk/basic.c.
TODO: add support for a simple divider, simple mux and a dummy clk for
stubbing out platform support.
Based on original patch by Jeremy Kerr and contribution by Jamie Iles.
Signed-off-by: Mike Turquette mturque...@linaro.org
Cc: Jeremy Kerr jeremy.k...@canonical.com
Cc: Jamie Iles ja...@jamieiles.com
snip
+int clk_register_gate(struct device *dev, const char *name, unsigned long
flags,
+ struct clk *fixed_parent, void __iomem *reg, u8 bit_idx,
+ int set_to_enable)
+{
+ struct clk_hw_gate *gclk;
+ struct clk *clk;
+
+ gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL);
+
+ if (!gclk) {
+ pr_err(%s: could not allocate gated clk\n, __func__);
+ return -ENOMEM;
+ }
+
+ clk = gclk-clk;
+
+ /* struct clk_hw_gate assignments */
+ gclk-fixed_parent = fixed_parent;
+ gclk-reg = reg;
+ gclk-bit_idx = bit_idx;
+
+ /* struct clk assignments */
+ clk-name = name;
+ clk-flags = flags;
+
+ if (set_to_enable)
+ clk-ops = clk_hw_gate_set_enable_ops;
+ else
+ clk-ops = clk_hw_gate_set_disable_ops;
You could avoid having two sets of operations if you stored the
set_to_enable value in struct clk_hw_gate. It might be useful to store
additional information in struct clk_hw_gate if you also want to extend
to supporting non-32bit registers (readb, etc), clocks with write only
registers, or support clocks which require more than one bit to be set
or cleared to enable them, etc. See the basic mmio gpio driver for a
similar case.
I haven't given the basic clks enough attention, mostly because I
haven't started using them yet in my own platform's conversion to
common struct clk :-/
I think the original reason for the extra operations is to avoid the
conditional in the enable/disable path, but if we're going to end up
adding other conditionals anyways (such as the register locking in the
iMX platform implementation) then we should probably forget about that
approach and just deal with the complexity in one set of common
enable/disable functions.
Thanks for the review,
Mike
~Ryan
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On Tue, Dec 13, 2011 at 8:52 PM, Ryan Mallon rmal...@gmail.com wrote:
On 14/12/11 14:53, Mike Turquette wrote:
+void __clk_unprepare(struct clk *clk)
+{
+ if (!clk)
+ return;
+
+ if (WARN_ON(clk-prepare_count == 0))
+ return;
+
+ if (--clk-prepare_count 0)
+ return;
+
+ WARN_ON(clk-enable_count 0);
+
+ if (clk-ops-unprepare)
+ clk-ops-unprepare(clk);
+
+ __clk_unprepare(clk-parent);
+}
I think you can rewrite this to get rid of the recursion as below:
while (clk) {
if (WARN_ON(clk-prepare_count == 0))
return;
if (--clk-prepare_count 0)
return;
WARN_ON(clk-enable_count 0)
if (clk-ops-unprepare)
clk-ops-unprepare(clk);
clk = clk-parent;
}
Looks good. I'll roll into next set.
Also, should this function be static?
No, since platform clk code will occasionally be forced to call
clk_(un)prepare while the prepare_lock mutex is held. For a valid
example see:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/dpll3xxx.c;h=b2412574b63829944593c1a7a6eda5fa4350686f;hb=738bde65918ae1ac743b1498801da0b860e2ee32#l461
+void clk_unprepare(struct clk *clk)
+{
+ mutex_lock(prepare_lock);
+ __clk_unprepare(clk);
+ mutex_unlock(prepare_lock);
+}
+EXPORT_SYMBOL_GPL(clk_unprepare);
+
+int __clk_prepare(struct clk *clk)
+{
+ int ret = 0;
+
+ if (!clk)
+ return 0;
+
+ if (clk-prepare_count == 0) {
+ ret = __clk_prepare(clk-parent);
+ if (ret)
+ return ret;
+
+ if (clk-ops-prepare) {
+ ret = clk-ops-prepare(clk);
+ if (ret) {
+ __clk_unprepare(clk-parent);
+ return ret;
+ }
+ }
+ }
+
+ clk-prepare_count++;
+
+ return 0;
+}
This is unfortunately a bit tricky to remove the recursion from without
keeping a local stack of the clocks leading up to first unprepared
client :-/.
Again, should this be static? What outside this file needs to
prepare/unprepare clocks without the lock held?
Same as above.
+int clk_prepare(struct clk *clk)
+{
+ int ret;
+
+ mutex_lock(prepare_lock);
+ ret = __clk_prepare(clk);
+ mutex_unlock(prepare_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_prepare);
+
+void __clk_disable(struct clk *clk)
+{
+ if (!clk)
+ return;
+
+ if (WARN_ON(clk-enable_count == 0))
+ return;
+
+ if (--clk-enable_count 0)
+ return;
+
+ if (clk-ops-disable)
+ clk-ops-disable(clk);
+
+ if (clk-parent)
+ __clk_disable(clk-parent);
Easy to get rid of the recursion here. Also should be static?
Yes the enable/disable should be static. I originally made them
non-static when I converted the prepare/unprepare set, but of course
it's possible to call these with the prepare_lock mutex held so I'll
fix this up in the next set.
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ if (clk clk-ops-round_rate)
+ return clk-ops-round_rate(clk, rate, NULL);
+
+ return rate;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
If the clock doesn't provide a round rate function then shouldn't we
return an error to the caller? Telling the caller that the rate is
perfect might lead to odd driver bugs?
Yes this code should so something better. I've been focused mostly on
the write aspects of the clk API (set_rate, set_parent,
enable/disable and prepare/unprepare) and less on the read aspects
of the clk API (get_rate, get_parent, round_rate, etc). I'll give
this a closer look for the next set.
+/**
+ * DOC: Using the CLK_PARENT_SET_RATE flag
+ *
+ * __clk_set_rate changes the child's rate before the parent's to more
+ * easily handle failure conditions.
+ *
+ * This means clk might run out of spec for a short time if its rate is
+ * increased before the parent's rate is updated.
+ *
+ * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
+ * clk where you also set the CLK_PARENT_SET_RATE flag
+ */
Is this standard kerneldoc format?
It is:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=Documentation/kernel-doc-nano-HOWTO.txt;h=3d8a97747f7731c801ca7d3a1483858feeb76b6c;hb=refs/heads/v3.2-rc5-clkv4-omap#l298
+struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
static?
I'll make it static. I don't think any platform code needs to call
this (at least I hope not).
+{
+ struct clk *fail_clk = NULL;
+ int ret = 0;
+ unsigned long old_rate = clk-rate;
+ unsigned long new_rate;
+ unsigned long parent_old_rate;
+ unsigned
Re: [PATCH v4 0/6] common clk framework
On Tue, Dec 13, 2011 at 7:53 PM, Mike Turquette mturque...@linaro.org wrote: From: Mike Turquette mturque...@ti.com The common clk framework is an attempt to define a generic struct clk which most platforms can use to build a clk tree and perform a set of well-defined operations. Forgot to mention: these patches are based on v3.2-rc5 and can be pulled from: http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=shortlog;h=refs/heads/v3.2-rc5-clkv4 git://git.linaro.org/people/mturquette/linux.git v3.2-rc5-clk-v4 Regards, Mike The previous patchset, v3, can be found at, http://article.gmane.org/gmane.linux.kernel/1218622 New stuff in v4: * clk rate change notifiers * clk debug info via debugfs (instead of sysfs) * lots of bug fixes Stuff that is known to be missing in v4: * basic mux and divider clk types * fix for migrating clk_prepare_count/clk_enable_count in clk_set_parent * minor rework comments from v3 * Documentation/clk.txt needs love All of the mising items above will be rolled into v5 ASAP. I wanted to go ahead and push out the new notifier changes for review and gather comments on those since those were a big gap in the v3 patchset. Paul W. also had some good comments about the greater clk API, and the opportunity to fix some of that stuff while this patchset is still under discussion. I didn't address those here because they require more thought, and more comments from reviewers. Finally, OMAP4 support for the common struct clk will be posted immediately after this patch series to LAKML and LOML, along with some hack patches that show how to use the recursive clk_set_rate for propagating rate changes up the tree for CPUfreq and how to use the new clk rate change notifiers in a driver. Mike Turquette (6): clk: Kconfig: add entry for HAVE_CLK_PREPARE Documentation: common clk API clk: introduce the common clock framework clk: introduce rate change notifiers clk: basic gateable and fixed-rate clks clk: export the clk tree topology to debugfs Documentation/clk.txt | 312 +++ drivers/clk/Kconfig | 23 ++ drivers/clk/Makefile | 4 +- drivers/clk/clk-basic.c | 208 ++ drivers/clk/clk.c | 992 +++ include/linux/clk.h | 230 +++- 6 files changed, 1765 insertions(+), 4 deletions(-) create mode 100644 Documentation/clk.txt create mode 100644 drivers/clk/clk-basic.c create mode 100644 drivers/clk/clk.c -- 1.7.5.4 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
On Mon, Dec 12, 2011 at 11:47 AM, Andrew Lunn and...@lunn.ch wrote: Hi Mike +int clk_register_gate(struct device *dev, const char *name, unsigned long flags, + struct clk *fixed_parent, void __iomem *reg, u8 bit_idx, + int set_to_enable) + How do you suggest handling gated clocks which are already running when calling the register function? On my kirkwood bases system, the bootloader has already turned on a number of clocks. It does not seem right to start messing with clk-enable_count and clk-prepare_count. Could clk_register_gate() have one more parameter, a bool indicating running? I don't like this approach. If the bool for a particular clk is statically defined then it could be wrong (bootloader/kernel mismatch). I've been thinking of adding a clk-ops-init callback in clk_init, which is defined for a platform to use however the author sees fit. There have been a few cases where it would be nice to init a clk only once, when it is registered. That code could also handle detecting if a clk is enabled or not. On the other hand we already have a .get_parent callback which is only good for figuring out which parent a mux clk is using... maybe a .is_enabled or .get_enabled would be a good idea which also served the purpose of dynamically determining whether a clk is disabled or running. In general though I think we should try to keep the solution in the core code, not by having platform code pass in a bool. The kirkwood mach code keeps a bitmap of which platform_data init functions are called from the board file. In a late_initcall function it then enables and disables clocks as needed. What i was thinking is i can ask the hardware what clocks are already running before i register them and register them as running/not running. Then let the driver probe functions use the API to enable clocks which are really needed. In a late_initcall function, i would then call clk_disable(), clk_unprepare() on clocks which the boot loader started, thus turning them off if no driver has claimed them. The problem here is that you're solving the disabled unused clks issue in platform code. I've started to lay out a little groundwork for that with a flag in struct clk: http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=include/linux/clk.h;h=3b0eb3f1caf1d6346b62c785b74a648587bfcc7f;hb=586c6e8922a889a2893ba4467bb3d13b471656a9#l35 The idea behind CLK_IGNORE_UNUSED flag on line 35 is that the common clk framework can walk the tree (probably as part of a late_initcall, as you suggested) and disable any clks that aren't already enabled and don't have this flag set. This involves zero platform-specific code, but I haven't gotten around to introducing the feature yet as I'm really trying to minimize footprint for the core code (and I'm not doing a good job of that since it keeps growing). Regards, Mike Is this how you envisage it working? Thanks Andrew ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Mon, Nov 21, 2011 at 5:40 PM, Mike Turquette mturque...@ti.com wrote:
+/**
+ * clk_get_parent - return the parent of a clk
+ * @clk: the clk whose parent gets returned
+ *
+ * Simply returns clk-parent. It is up to the caller to hold the
+ * prepare_lock, if desired. Returns NULL if clk is NULL.
+ */
+struct clk *clk_get_parent(struct clk *clk)
+{
+ if (!clk)
+ return NULL;
+
+ return clk-parent;
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);
While auditing the uses/expectations of the current clk API users, I
see that clk_get_parent is rarely used; it is in fact only called in
11 files throughout the kernel.
I decided to have a little audit and here is what I found:
arch/arm/mach-shmobile/board-ap4evb.c: fsi_ak4642_set_rate
Used within clk_set_rate. Can dereference clk-parent directly and
ideally should propagate up the clk tree using the new recursive
clk_set_rate.
arch/arm/mach-tegra/usb_phy.c: tegra_usb_phy_open
Used within a clk_get_rate. pll_u should ideally have it's own
clk-rate populated, reducing the need for tegra_usb_phy_open to know
details of the clk tree. The logic to pull pll_u's rate from it's
parent belongs to pll_u's .recalc_rate callback.
arch/arm/plat-s5p/clock.c: s5p_spdif_set_rate
Another clk_get_parent call from within a .set_rate callback. Again:
use clk-parent directly and better yet, propagate the rate change up
via the recursive clk_set_rate.
arch/arm/plat-s5p/clock.c: s5p_spdif_get_rate
Another clk_get_parent call from within a .recalc_rate callback.
Again, clk-rate should be populated with parent's rate correctly,
otherwise dereference clk-parent directly without use of
clk_get_parent.
arch/arm/plat-s5p/s5p-time.c: s5p_clockevent_init
This problem would be solved by propagating rate_change requests up
two-levels of parents via the new recursive clk_set_rate. There is
also a clk_set_parent call in here, which has nothing to do with the
clk_get_parent call, but it makes me wonder if we should revisit the
issue of switching parents as part of clk_set_rate:
clk_set_rate(tin_event, pclk-rate / 2);
arch/arm/plat-samsung/pwm.c: pwm_is_tdiv
Used in only two places: as part of pwm_calc_tin which could be
replaced wholesale by a better .round_rate implementation and for a
debug print (who cares).
arch/arm/plat-samsung/pwm.c: pwm_calc_tin
Just a .round_rate implementation. Can dereference clk-parent directly.
arch/arm/plat-samsung/time.c: s3c2410_timer_setup
Same as s5p_clockevent_init above.
drivers/sh/clk/core.c: clk_round_parent
An elaborate .round_rate that should have resulted from recursive
propagation up to clk-parent.
drivers/video/omap2/dss/dss.c:
Every call to clk_get_parent in here is wrapped in clk_get_rate. The
functions that use this are effectively .set_rate, .get_rate and
.recalc_rate doppelgangers. Also a debug print calls clk_get_parent
but who cares.
drivers/video/sh_mobile_hdmi.c:
Used in a .set_rate and .round_rate implementation. No reason not to
deref clk-parent directly.
The above explanations take a little liberty listing certain functions
as .round_rate, .set_rate and .recalc_rate callbacks, but that is
because a lot of the code mentioned could be neatly wrapped up that
way.
Do we really need clk_get_parent? The primary problem with it is
ambiguity in the API: should the caller hold a lock? Is the rate
guaranteed not the change after being called? A top-level clk API
function that can be called within other top-level clk API functions
is inconsitent: most of the time this would incur nested locking.
Also having a top-level API expose the tree structure encourages
platform and driver developers to put clk tree details into their code
as opposed to having clever .round_rate and .set_rate callbacks hide
these details from them with the new recursive clk_set_rate.
Thoughts?
Thanks,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Thu, Dec 1, 2011 at 6:42 AM, Mark Brown
broo...@opensource.wolfsonmicro.com wrote:
On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote:
Clock rate/parent-change notifiers are requirements for DVFS use-cases,
and they must be paired with something like the
clk_{allow,block}_rate_change() functions to work efficiently. I intend
to comment on this later; it's not a simple problem. It might be worth
noting that Tero and I implemented a simplified version of this for the
N900.
I'm thinking that if we're going to have clk_{allow,block}_rate_change()
we may as well make that the main interface to enable rate changes - if
a device wants to change the clock rate it allows rate changes using
that interface rather than by disabling the clocks. I've got devices
which can do glitch free updates of active clocks so having to disable
would be a real restriction, and cpufreq would have issues with actually
disabling the clock too I expect.
I agree that imposing a disable clk before changing rate policy in
the framework core is a bad idea. During discussion on the
CLK_GATE_SET_RATE flag in the patch #2 Shawn commented that he has
some clks that must be enabled to change their rates (I assume he
means PLLs but that detail doesn't really matter).
Regards,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 0/5] common clk framework
On Fri, Nov 25, 2011 at 11:06 PM, Shawn Guo shawn@freescale.com wrote: On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote: .speaking of which, clk_set_rate has been overhauled and is now recursive. *collective groan*. clk_set_rate is still simple for the common case of simply setting a single clk's rate. But if your clk has the CLK_PARENT_SET_RATE flag and the .round_rate callback recommends changing the parent rate, then clk_set_rate will recurse upwards to the parent and try it all over again. In the event of a failure everything unwinds and all the clks go out for drinks. I smell that this will be the part which makes the whole series risky of being accepted in a desired time frame, with saying rate change notifications are still missing for now. I agree. I'll send out V4 this coming week with the clk rate change notifiers rolled in. That means that there won't be any missing pieces for my vision of how a complex clk tree should interact with drivers. If it is not what people want then we can probably de-scope pieces of it, but I think the architecture is sound. Thanks for reviewing Shawn. Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 2/5] Documentation: common clk API
On Sat, Nov 26, 2011 at 12:47 AM, Shawn Guo shawn@freescale.com wrote: On Wed, Nov 23, 2011 at 12:33:47PM -0800, Turquette, Mike wrote: On Tue, Nov 22, 2011 at 6:03 PM, Saravana Kannan skan...@codeaurora.org wrote: On 11/21/2011 05:40 PM, Mike Turquette wrote: No strong opinion, but can we call this clk_ops for brevity? I prefer clk_hw_ops, as it clearly delineates that these operations know hardware-specific details. I tend to agree with Saravana for brevity. Looking at clk-basic.c, I will drop hw for V4. + +clk_set_rate - Attempts to change the clk rate to the one specified. +Depending on a variety of common flags it may fail to maintain system +stability or result in a variety of other clk rates changing. I'm not sure if this is intentional or if it's a mistake in phrasing it. IMHO, the rates of other clocks that are actually made available to device drivers should not be changed. This call might trigger rate changes inside the tree to accommodate the request from various children, but should not affect the rate of the leaf nodes. Can you please clarify the intention and/or the wording? Setting the flag CLK_GATE_SET_RATE tells clk_set_rate not to change the rate if the clk is enabled. This policy is not enforced abritrarily: you don't have to set the flag on your clk. I'll update the doc to make explicit mention of this flag. I guess the concern is not about the flag but the result of clk_set_rate that might change a variety of other clocks, while Saravana said it should not. And I agree with Saravana. This behavior is entirely within the control of whoever ports their platform over to the common clk fwk. The set of clks whose rates will be directly changed by a call to clk_set_rate is: the clk specified in the call to clk_set_rate AND any parent clks that __clk_set_rate propagates upwards to. The set of clks whose rates will be indirectly changed are the children of clks in the direct set that are not themselves in the direct set. I'll make it clear here that CLK_PARENT_SET_RATE only steps up one parent and then whole thing is re-evaluated: meaning that if clk sets CLK_PARENT_SET_RATE then we might go up to clk-parent (based on the outcome of clk's .round_rate) and then if clk-parent does NOT set CLK_PARENT_SET_RATE then propagation ends there. Based on your comment below where iMX6 leaf clks only need their parent rate changed, then this will work beautifully for you as leaf_clk-parent won't set CLK_PARENT_SET_RATE in your case. On the contrary, I have clocks on mxs platform which can be set rate only when they are enabled, while there are users call clk_set_rate when the clock is not enabled yet. Do we need another flag CLK_ON_SET_RATE for this type of clocks? This brings up the point of where flags belong. The point of CLK_GATE_SET_RATE is to either avoid changing rates across a clk that is not glitchless, or upsetting a functional module at the end of a chain of clks which cannot gracefully withstand sudden rate change. This is common enough that it merits being in the common clk code. Likewise if CLK_ON_SET_RATE is very common, it too should belong in the common clk code. If iMX6 is the only platform like this, maybe your .set_rate should implement this logic and return -ESHUTDOWN or -EPERM or something so that __clk_set_rate can bail out gracefully. Do any other platforms need that flag? I'm unsure that clk API users will all agree with the use of the flags. From the code, the clock framework will make the call fail if users attempt to clk_set_rate an enabled clock with flag CLK_GATE_SET_RATE. And clk API users might argue that they do not (need to) know this clock details, and it's clock itself (clock framework or/and clock driver) who should handle this type of details. One way around this is to have clk_set_rate call clk_disable/__clk_unprepare if CLK_GATE_SET_RATE is set. Then if the usecount is still 0 then clk_set_rate will fail. Personally I don't like having the common clk_set_rate making cross-calls to the enable/disable stuff, but for the sake of exploring the topic... In your case it will be the opposite: clk_set_rate will call __clk_prepare/clk_enable and if the usecount is 0 then it will fail. This starts to get really complicated though and at some point all the permutation eventually mean that drivers will have to know some details. If these flags don't exist I also think that the result will be drivers that know exactly the details. In my case it will be: clk_disable clk_set_rate clk_enable In your case it will be: clk_enable clk_set_rate clk_disable Maybe I'm over-thinking it? clk A | | | clk B ---\ | | | | | | clk C clk D You have stated Another caveat is that child clks
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Sat, Nov 26, 2011 at 5:22 AM, Shawn Guo shawn@freescale.com wrote:
On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote:
+ * To prevent this consider setting the CLK_GATE_SET_RATE flag on any
+ * clk where you also set the CLK_PARENT_SET_RATE flag
Eh, this is how flag CLK_GATE_SET_RATE is born? Does that mean to make
the call succeed all the clocks on the propagating path need to be gated
before clk_set_rate gets called?
Setting that flag on any clk implies nothing about the parents of that
clk. Obviously if a clk's child is enabled then clk is enabled and
won't have it's rate change if the flag is set, which is the whole
point of the flag. Again, you don't have to set the flag.
+ /* change the rate of this clk */
+ ret = clk-ops-set_rate(clk, new_rate);
Yes, right here, the clock gets set to some unexpected rate, since the
parent clock has not been set to the target rate yet at this point.
Sequence is irrelevant for the case where both parent and child change
dividers, since the output of the child clk will be weird at some
point.
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 7213b52..3b0eb3f 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,8 @@
*
* Copyright (C) 2004 ARM Limited.
* Written by Deep Blue Solutions Limited.
+ * Copyright (c) 2010-2011 Jeremy Kerr jeremy.k...@canonical.com
+ * Copyright (C) 2011 Linaro Ltd mturque...@linaro.org
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -13,17 +15,134 @@
#include linux/kernel.h
+#include linux/kernel.h
Eh, why adding a duplication?
Will fix in v4.
+#include linux/errno.h
+
struct device;
+struct clk;
Do you really need this?
Strange. This line existed pre-common clk patches. I'll look into
why it is getting added back in. This declaration is necessary for
the old-style clk frameworks which each defined their own struct clk.
I assume it is still needed but I'll look at the git history to figure
it out.
+struct clk_hw_ops {
+ int (*prepare)(struct clk *clk);
+ void (*unprepare)(struct clk *clk);
+ int (*enable)(struct clk *clk);
+ void (*disable)(struct clk *clk);
+ unsigned long (*recalc_rate)(struct clk *clk);
+ long (*round_rate)(struct clk *clk, unsigned long,
The return type seems interesting. If we intend to return a rate, it
should be 'unsigned long' rather than 'long'. I'm just curious about
the possible reason behind that.
Yeah these are mostly from the original patch set. I'll go over these
with a fine-tooth comb for v4. To some extent I think they model the
return types of the clk API in clk.h.
+ unsigned long *);
+ int (*set_parent)(struct clk *clk, struct clk *);
+ struct clk * (*get_parent)(struct clk *clk);
+ int (*set_rate)(struct clk *clk, unsigned long);
Nit: would it be reasonable to put set_rate after round_rate to group
*_rate functions together?
Will fix in v4.
+int __clk_enable(struct clk *clk);
+void __clk_disable(struct clk *clk);
+int __clk_prepare(struct clk *clk);
+void __clk_unprepare(struct clk *clk);
+void __clk_reparent(struct clk *clk, struct clk *new_parent);
+
Do we really need to export all these common clk internal functions?
I think we do. Saravana also felt this was necessary.
I know that for OMAP we need __clk_prepare, __clk_unprepare and
__clk_reparent just to handle our PLLs. I don't think we need
__clk_enable or __clk_disable since we can call the proper APIs for
those with the prepare_mutex held.
If no one needs __clk_enable and __clk_disable then I am happy to
remove those declarations.
Thanks for reviewing,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
On Sat, Nov 26, 2011 at 5:48 AM, Shawn Guo shawn@freescale.com wrote:
On Mon, Nov 21, 2011 at 05:40:46PM -0800, Mike Turquette wrote:
Many platforms support simple gateable clks and fixed-rate clks that
should not be re-implemented by every platform.
This patch introduces a gateable clk with a common programming model of
gate control via a write of 1 bit to a register. Both set-to-enable and
clear-to-enable are supported.
Also introduced is a fixed-rate clk which has no reprogrammable aspects.
The purpose of both types of clks is documented in drivers/clk/basic.c.
What I have seen is drivers/clk/clk-basic.c.
Will fix in v4.
+int clk_register_gate(struct device *dev, const char *name, unsigned long
flags,
+ struct clk *fixed_parent, void __iomem *reg, u8 bit_idx,
+ int set_to_enable)
+{
+ struct clk_hw_gate *gclk;
+ struct clk *clk;
+
+ gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL);
+
+ if (!gclk) {
+ pr_err(%s: could not allocate gated clk\n, __func__);
+ return -ENOMEM;
+ }
+
+ clk = gclk-clk;
+
+ /* struct clk_hw_gate assignments */
+ gclk-fixed_parent = fixed_parent;
+ gclk-reg = reg;
+ gclk-bit_idx = bit_idx;
+
+ /* struct clk assignments */
+ clk-name = name;
+ clk-flags = flags;
+
+ if (set_to_enable)
+ clk-ops = clk_hw_gate_set_enable_ops;
+ else
+ clk-ops = clk_hw_gate_set_disable_ops;
+
+ clk_init(NULL, clk);
+
+ return 0;
The device tree support needs to get this 'struct clk *', so we may
want to have all these registering functions return the 'clk'.
Thanks for the input. Truthfully I'm very DT-ignorant so I'm happy to
reshape any APIs for that topic. Hope to fix that soon.
Thanks for reviewing,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
On Tue, Nov 22, 2011 at 7:48 PM, Saravana Kannan skan...@codeaurora.org wrote: On 11/22/2011 11:13 AM, Greg KH wrote: On Tue, Nov 22, 2011 at 09:57:41AM -0800, Mike Turquette wrote: Ah, comments like this warm my heart. Come on, no abusing the kobject code please, if have problems with how the kernel core works, and it doesn't do things you want it to, then why not change it to work properly for you, or at the least, ASK ME!!! Ok, I'm asking you now. There are two ways to solve this problem: 1) have kobject core create the lists linking the objects but defer allocations and any interactions with sysfs until later in the boot sequence, OR 2) my code can create a list of clks (the same way that clkdev does) and defer kobject/sysfs stuff until later, which walks the list made during early-boot #1 is most closely aligned with the code I have here, #2 presents challenges that I haven't really though through. I know that OMAP uses the clk framework VERY early in it's boot sequence, but as long as the per-clk data is properly initialized then it should be OK. What do you think? #3 - use debugfs and don't try to create a sysfs interface for the clock structures :) I would prefer debugfs too, but for my own selfish reasons. In our current I'll cook up debugfs code for the fwk and drop sysfs. Maybe not part of V4 (per Arnd's suggestion to focus on upstreamable bits), or maybe I will if I have time. Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
On Tue, Nov 22, 2011 at 12:02 PM, Arnd Bergmann a...@arndb.de wrote: On Tuesday 22 November 2011 12:19:51 Grant Likely wrote: On Tue, Nov 22, 2011 at 11:01 AM, Mike Turquette mturque...@linaro.org wrote: Others have requested to have knobs made available for actually performing clk_enable/clk_disable and even clk_set_rate from userspace. I hate this idea and won't implement it. I encourage anyone that needs this to do it in debugfs. Does that work-split make sense to you, or do you still not like the idea of having topology and read-only info in sysfs? Unless there is a solid real-world use case for exporting this data to userspace, I do not think sysfs is a good idea. As long as the usage (beyond debug) is theoretical I think the whole thing should be in debugfs. It can always be moved at a later date If a real use case does become important. I would recomment not to spend any time on implementing a debugfs interface for this right now. As far as I can tell, nothing relies on exporting the structure to user space, so Mike's time is better spent on getting the other four patches merged. Seems to be some consensus on this. Will drop userspace-visible clk tree from V4 patchset. Regards, Mike Note that the static topology information about clocks will already be visible in /proc/devicetree when that is enabled and the clocks are described in the .dts file. Arnd ___ 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 v2 1/7] clk: Add a generic clock infrastructure
On Sun, Oct 23, 2011 at 5:55 AM, Shawn Guo shawn@freescale.com wrote: Hi Mike, Some random comments/nits ... Thanks for reviewing Shawn. Will roll changes into V3. Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 2/7] clk: Implement clk_set_rate
On Sun, Oct 23, 2011 at 7:24 AM, Shawn Guo shawn@freescale.com wrote: On Thu, Sep 22, 2011 at 03:26:57PM -0700, Mike Turquette wrote: From: Jeremy Kerr jeremy.k...@canonical.com [...] + * @set_rate Change the rate of this clock. If this callback returns + * CLK_SET_RATE_PROPAGATE, the rate change will be propagated to s/CLK_SET_RATE_PROPAGATE/CLK_PARENT_RATE_CHANGE, as suggested by your change log above? Thanks for reviewing. Will roll into V3 patchset. Regards, Mike + * the parent clock (which may propagate again). The requested + * rate of the parent is passed back from the callback in the + * second 'unsigned long *' argument. + * -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Fri, Oct 14, 2011 at 7:24 PM, Richard Zhao richard.z...@linaro.org wrote:
On Fri, Oct 14, 2011 at 11:14:19AM -0700, Turquette, Mike wrote:
On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette mturque...@ti.com wrote:
unsigned long omap_recalc_rate(struct clk_hw *hw)
{
struct clk *parent;
struct clk_hw_omap *oclk;
parent = hw-clk-parent;
clk drivers can not see struct clk details. I use clk_get_parent.
clk_get_parent should query the hardware to see what the parent is.
This can have undesireable overhead. It is quite acceptable to
reference a clock's parent through clk-parent, just as it is
acceptable to get a clock rate through clk-rate.
An analogous situation is a clk_get_rate call which uses a clk's
.recalc. There is undesirable overhead involved in .recalc for clocks
whose rates won't change behind our backs, so best to just treat the
data in struct clk as cache and reference it directly.
oclk = to_clk_omap(hw);
...
}
...
unsigned long omap_recalc_rate(struct clk *clk)
{
struct clk *parent;
struct clk_hw_omap *oclk;
parent = clk-parent;
oclk = to_clk_omap(clk-hw);
...
}
In my understanding, struct clk stores things specific to clk core,
struct clk_hw stores common things needed by clk drivers. For static clk
driver
there' some problems:
- For clocks without mux, I need duplicate a .parent and set .get_parent.
Even when we adopt DT and dynamicly create clk, it's still a problem.
Moving .parent to clk_hw can fix it.
For clocks with a fixed parent we should just pass it in at
register-time. We should definitely not move .parent out of struct
clk, since struct clk should be the platform agnostic bit that lets us
do tree walks, build topology, etc etc.
If you really want a .parent outside of struct clk then duplicate it
in your struct clk_hw_imx and teach your .ops about it (analogous to
struct clk_hw_fixed-rate).
- When I define a clk array, I don't need to find another place to store
.ops.
It's not problem for dynamic creating clock.
Something like the following?
static struct clk aess_fclk;
static const clk_hw_ops aess_fclk_ops = {
.recalc = omap2_clksel_recalc,
.round_rate = omap2_clksel_round_rate,
.set_rate = omap2_clksel_set_rate,
};
static struct clk_hw_omap aess_fclk_hw = {
.hw = {
.clk = aess_fclk,
},
.clksel = aess_fclk_div,
.clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL,
.clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK,
};
static struct clk aess_fclk = {
.name = aess_fclk,
.ops = aess_fclk_ops,
.hw = aess_fclk_hw.hw,
.parent = abe_clk,
};
- As I mentioned in another mail, clk group need no lock version
prepare/unprepare
and enable/disable functions
Clock groups are out of scope for this first series. We should
discuss more what the needs are for your clock groups. If it boils
down to just enabling all of the clocks for a given device then you
might want to abstract that away with pm_runtime_* calls, and maybe a
supplementary layer like OMAP's hwmod. But I might be way off base, I
really don't understand your use case for clock groups.
Another way is, add a {struct clk_hw *clks; int count} in clk_hw, let clk
core handle it.
I prefer the second way, but I'm not sure whether it's common enough. It's
still a problem for dynamic creating clock.
struct clk_hw is just a pointer for navigating from struct clk -
struct your_custom_clk and vice versa. Again can you elaborate on
your needs for managing multiple clocks with a single struct clk_hw?
Thanks,
Mike
Thanks
Richard
It is a small nitpick, but it affects the API for everybody so best to
get it right now before folks start migrating over to it.
Thanks,
Mike
int (*set_rate)(struct clk_hw *,
unsigned long, unsigned long *);
long (*round_rate)(struct clk_hw *, unsigned long);
int (*set_parent)(struct clk_hw *, struct clk *);
struct clk * (*get_parent)(struct clk_hw *);
};
___
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 v2 1/7] clk: Add a generic clock infrastructure
On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette mturque...@ti.com wrote:
From: Jeremy Kerr jeremy.k...@canonical.com
struct clk_hw_ops {
int (*prepare)(struct clk_hw *);
void (*unprepare)(struct clk_hw *);
int (*enable)(struct clk_hw *);
void (*disable)(struct clk_hw *);
unsigned long (*recalc_rate)(struct clk_hw *);
In implementing recalc for divider clocks, I started to wonder, why
not just pass struct clk *clk into the clk_hw_ops func ptrs?.
recalc is an obvious example whereby we need access to parent-rate.
The code usually ends up looking something like:
unsigned long omap_recalc_rate(struct clk_hw *hw)
{
struct clk *parent;
struct clk_hw_omap *oclk;
parent = hw-clk-parent;
oclk = to_clk_omap(hw);
...
}
That's a bit of a song and dance to have to do in almost every op, and
often these ops will need access to stuff like clk-rate also. Is
there any opposition to just passing in struct clk? e.g:
unsigned long omap_recalc_rate(struct clk *clk)
{
struct clk *parent;
struct clk_hw_omap *oclk;
parent = clk-parent;
oclk = to_clk_omap(clk-hw);
...
}
It is a small nitpick, but it affects the API for everybody so best to
get it right now before folks start migrating over to it.
Thanks,
Mike
int (*set_rate)(struct clk_hw *,
unsigned long, unsigned long *);
long (*round_rate)(struct clk_hw *, unsigned long);
int (*set_parent)(struct clk_hw *, struct clk *);
struct clk * (*get_parent)(struct clk_hw *);
};
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Thu, Oct 13, 2011 at 7:44 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
struct clk_hw_ops {
int (*prepare)(struct clk_hw *);
void (*unprepare)(struct clk_hw *);
int (*enable)(struct clk_hw *);
void (*disable)(struct clk_hw *);
unsigned long (*recalc_rate)(struct clk_hw *);
int (*set_rate)(struct clk_hw *,
unsigned long, unsigned long *);
long (*round_rate)(struct clk_hw *, unsigned long);
int (*set_parent)(struct clk_hw *, struct clk *);
struct clk * (*get_parent)(struct clk_hw *);
};
Platform clock code can register a clock through clk_register, passing a
set of operations, and a pointer to hardware-specific data:
struct clk_hw_foo {
struct clk_hw clk;
void __iomem *enable_reg;
};
Eww, no, this really isn't going to scale for platforms like OMAP - to
have all the operations indirected through a set of function pointers
for every clock just because the enable register (or enable bit) is
in a different position is completely absurd.
Russel,
I'm porting the OMAP clock framework to common clock right now and it
is going well.
For many clocks near the root of the tree I've been able to re-use
struct clk_hw_fixed clk_fixed_ops (see patch 3 or 4 in this series),
which actually represents a decrease in memory consumption over the
old OMAP struct clk (which populated many of the operations func
pointers directly, causing duplication).
So far I don't see scalability as an issue. In fact the design solves
some problems neatly for us. For now I am creating one uber clock,
struct clk_hw_omap, which dumps all of the clksel, pll, gate control
mux crap directly from OMAP's old struct clk. This is not optimal for
the long-term, but is a reasonable stepping stone which handles 90% of
OMAP clocks. The new common struct clk makes it much easier for us to
treat PLLs differently from typical dividers, which can be treated
differently from dividers in modules, which can be treated differently
from pure mux clocks, etc.
I'm not soo concerned about the increase in memory usage (for 100 to 200
clock definitions its only 4 to 8k) but what worries me the most is
initializing these damned things. It's an awful lot of initializers,
which means the potential for an awful lot of conflicts should something
change in this structure.
As I stated above, so far memory usage has actually *decreased* due to
removing so many duplicated function pointers for OMAP's old struct
clk. I wouldn't be surprised if other SoCs experienced the same lift.
Also, in my own tree I've broken out clk_register into clk_init also.
clk_register is still the thing to call if you have dynamically
created clocks, as it handles the malloc, and it then calls clk_init.
clk_init can be used by for static clock data and just handles
initializing the mutex/list/whatever. Does this allay some of your
concerns? Are you just trying to avoid allocating all of that memory
dynamically?
Regards,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 4/7] clk: Add simple gated clock
On Thu, Oct 13, 2011 at 7:45 AM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote: diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/* + * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Simple clk gate implementation + */ + +#include linux/clk.h +#include linux/module.h +#include asm/io.h linux/io.h please. Will roll into v3. Thanks for reviewing, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 4/7] clk: Add simple gated clock
On Tue, Oct 11, 2011 at 11:46 PM, Richard Zhao
richard.z...@freescale.com wrote:
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.k...@canonical.com
Signed-off-by: Jeremy Kerr jeremy.k...@canonical.com
Signed-off-by: Mark Brown broo...@opensource.wolfsonmicro.com
Signed-off-by: Jamie Iles ja...@jamieiles.com
Signed-off-by: Mike Turquette mturque...@ti.com
---
Changes since v1:
Add copyright header
Fold in Jamie's patch for set-to-disable clks
Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++
drivers/clk/Makefile | 1 +
drivers/clk/clk-gate.c | 78
include/linux/clk.h | 13
4 files changed, 96 insertions(+), 0 deletions(-)
create mode 100644 drivers/clk/clk-gate.c
I feel hard to tell the tree the clk parent, at register/init time. For the
simple gate clk, the only way is to set .get_parent. But normally, for clk
without any divider we set .get_parent to NULL. Maybe we can put .parent to
struct clk_hw?
For non-mux clocks, whose parent is *always* going to be the same, you
should create a duplicate .parent in the clk_hw_* structure and then
have .get_parent return clk_hw_*-parent.
This is analogous to the way clk_hw_fixed returns clk_hw_fixed-rate
when .recalc is called on it.
Regards,
Mike
Thanks
Richard
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index d8313d7..a78967c 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -12,3 +12,7 @@ config GENERIC_CLK
config GENERIC_CLK_FIXED
bool
depends on GENERIC_CLK
+
+config GENERIC_CLK_GATE
+ bool
+ depends on GENERIC_CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 9a3325a..d186446 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
obj-$(CONFIG_GENERIC_CLK) += clk.o
obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o
+obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
new file mode 100644
index 000..a1d8e79
--- /dev/null
+++ b/drivers/clk/clk-gate.c
@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Simple clk gate implementation
+ */
+
+#include linux/clk.h
+#include linux/module.h
+#include asm/io.h
+
+#define to_clk_gate(clk) container_of(clk, struct clk_gate, hw)
+
+static unsigned long clk_gate_get_rate(struct clk_hw *clk)
+{
+ return clk_get_rate(clk_get_parent(clk-clk));
+}
+
+static void clk_gate_set_bit(struct clk_hw *clk)
+{
+ struct clk_gate *gate = to_clk_gate(clk);
+ u32 reg;
+
+ reg = __raw_readl(gate-reg);
+ reg |= BIT(gate-bit_idx);
+ __raw_writel(reg, gate-reg);
+}
+
+static void clk_gate_clear_bit(struct clk_hw *clk)
+{
+ struct clk_gate *gate = to_clk_gate(clk);
+ u32 reg;
+
+ reg = __raw_readl(gate-reg);
+ reg = ~BIT(gate-bit_idx);
+ __raw_writel(reg, gate-reg);
+}
+
+static int clk_gate_enable_set(struct clk_hw *clk)
+{
+ clk_gate_set_bit(clk);
+
+ return 0;
+}
+
+static void clk_gate_disable_clear(struct clk_hw *clk)
+{
+ clk_gate_clear_bit(clk);
+}
+
+struct clk_hw_ops clk_gate_set_enable_ops = {
+ .recalc_rate = clk_gate_get_rate,
+ .enable = clk_gate_enable_set,
+ .disable = clk_gate_disable_clear,
+};
+EXPORT_SYMBOL_GPL(clk_gate_set_enable_ops);
+
+static int clk_gate_enable_clear(struct clk_hw *clk)
+{
+ clk_gate_clear_bit(clk);
+
+ return 0;
+}
+
+static void clk_gate_disable_set(struct clk_hw *clk)
+{
+ clk_gate_set_bit(clk);
+}
+
+struct clk_hw_ops clk_gate_set_disable_ops = {
+ .recalc_rate = clk_gate_get_rate,
+ .enable = clk_gate_enable_clear,
+ .disable = clk_gate_disable_set,
+};
+EXPORT_SYMBOL_GPL(clk_gate_set_disable_ops);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1903dd8..626fd0d 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -124,6 +124,19 @@ extern struct clk_hw_ops clk_fixed_ops;
#endif /* CONFIG_GENERIC_CLK_FIXED */
+#ifdef CONFIG_GENERIC_CLK_GATE
+
+struct clk_gate {
+ struct clk_hw hw;
+ void __iomem *reg;
+ u8 bit_idx;
+};
+
+extern struct clk_hw_ops clk_gate_set_enable_ops;
+extern struct clk_hw_ops clk_gate_set_disable_ops;
+
+#endif /* CONFIG_GENERIC_CLK_GATE */
+
/**
* clk_register - register and initialize a new clock
*
--
1.7.4.1
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Wed, Oct 5, 2011 at 6:17 PM, Saravana Kannan skan...@codeaurora.org wrote: On 09/22/2011 03:26 PM, Mike Turquette wrote: + unsigned long (*recalc_rate)(struct clk_hw *); + long (*round_rate)(struct clk_hw *, unsigned long); + struct clk * (*get_parent)(struct clk_hw *); +}; I would like to understand the need for recalc rate if that's something that we want to go into the common framework (even if it's optional). I have mostly heard only second hand explanations of the need for recalc_rate(), so I might not have the full picture. But for all the cases that I can think of, recalc_rate seems like a paradox. Recalc rate has four main uses that I can think of off the top of my head: 1) clk_set_rate is called on clock0, which is a non-leaf clock. All clocks below clock0 have had their rates changed, yet no one called clk_set_rate on those child clocks. We use recalc to walk the sub-tree of children and recalculate their rates based on the new rate of their parent, clock0. 2) exact same as #1, but using clk_set_parent instead of clk_set_rate. Again, changing the rate of a clock high up in the tree will affect the rates of many child clocks below it. 3) at boot-time/init-time when we don't trust the bootloader and need to determine the clock rates by parsing registers 4) modeling rates for clocks that we don't really control. This is not as common as the above three, but there are times when we're interested in knowing a clock rate (perhaps for debug purposes), but it's scaling logic is in firmware or somehow independent of the Linux clock framework. If recalc_rate() is used to make sure the current rate of a clock A is always known even if it's parent clock B's rate is changed, then it also means that the rate of clock A can change without clk_set_rate(clock A, new rate). That in turn means that the clk_get_rate() just gives the instantaneous snapshot of the rate. So, any use of clk_get_rate(clock A) for anything other than printing/logging the return value is broken code. In For most clocks, the first three examples I give above will cover all of the times that a clock's rate will change. As long as a recalc/tree-walk is present then clk-rate is not out of sync and thus printing/reading that value is not broken. which case, do we really care for recalc_rate()? We could just return the rate that it was set to when clk_set_rate() was called and call it a day or return 0 for such clocks to indicate that the clock rate is unknown. What's the point of tracking a rate if it can't be trusted? Also, it is important to recalc rates whenever changes are made high up in the clock tree once we start to work on rate-change-arbitration issues, etc. Regards, Mike The whole concept of trying to recalculate the rate for a clock makes me feel uneasy since it promotes misunderstanding the behavior of the clock and writing bad code based on that misunderstanding. I would like to hear to real usecases before I propose some alternatives that I have in mind. 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 v2 1/7] clk: Add a generic clock infrastructure
On Mon, Oct 3, 2011 at 3:02 PM, Rob Herring robherri...@gmail.com wrote: Mike, On 09/22/2011 05:26 PM, Mike Turquette wrote: diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 6db161f..e2a9719 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -23,6 +23,13 @@ static LIST_HEAD(clocks); static DEFINE_MUTEX(clocks_mutex); +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported + * through other headers; we don't want them used anywhere but here. */ +#ifdef CONFIG_USE_COMMON_STRUCT_CLK +extern int __clk_get(struct clk *clk); +extern void __clk_put(struct clk *clk); +#endif + This is dead code left from prior versions. Indeed it is. Will pull it out for V3. Thanks, Mike Rob ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 4/7] clk: Add simple gated clock
On Mon, Sep 26, 2011 at 12:37 PM, Jamie Iles ja...@jamieiles.com wrote:
On Mon, Sep 26, 2011 at 02:10:32PM -0500, Rob Herring wrote:
On 09/26/2011 01:40 PM, Jamie Iles wrote:
On Mon, Sep 26, 2011 at 01:33:08PM -0500, Rob Herring wrote:
+static void clk_gate_set_bit(struct clk_hw *clk)
+{
+ struct clk_gate *gate = to_clk_gate(clk);
+ u32 reg;
+
+ reg = __raw_readl(gate-reg);
+ reg |= BIT(gate-bit_idx);
+ __raw_writel(reg, gate-reg);
Don't these read-mod-writes need a spinlock around it?
It's possible to have an enable bits and dividers in the same register.
If you did a set_rate and while doing an enable/disable, there would be
a problem. Also, it may be 2 different clocks in the same register, so
the spinlock needs to be shared and not per clock.
Well the prepare lock will be held here and I believe that would be
sufficient.
No, the enable spinlock is protecting enable/disable. But set_rate is
protected by the prepare mutex. So you clearly don't need locking if you
have a register of only 1 bit enables. If you have a register accessed
by both enable/disable and prepare/unprepare/set_rate, then you need
some protection.
OK fair point, but I would guess that if you had a clock like this then
you probably wouldn't use this simple gated clock would you? (speaking
from my world where we have quite simple clocks ;-))
I think it is a safe assumption that if a register controls both
enable/disable and some programmable divider then,
1) those controls are probably for the same clock
2) that clock won't be using the cookie-cutter gated-clock
implementation anyways
Rob, do you feel these assumptions are OK and locking can remain the
same in this patch?
Regards,
Mike
Jamie
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 4/7] clk: Add simple gated clock
On Sat, Sep 24, 2011 at 9:02 PM, Grant Likely grant.lik...@secretlab.ca wrote: On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote: From: Jeremy Kerr jeremy.k...@canonical.com Signed-off-by: Jeremy Kerr jeremy.k...@canonical.com Signed-off-by: Mark Brown broo...@opensource.wolfsonmicro.com Signed-off-by: Jamie Iles ja...@jamieiles.com Signed-off-by: Mike Turquette mturque...@ti.com --- Changes since v1: Add copyright header Fold in Jamie's patch for set-to-disable clks Use BIT macro instead of shift drivers/clk/Kconfig | 4 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-gate.c | 78 include/linux/clk.h | 13 4 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-gate.c diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index d8313d7..a78967c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,7 @@ config GENERIC_CLK config GENERIC_CLK_FIXED bool depends on GENERIC_CLK + +config GENERIC_CLK_GATE + bool + depends on GENERIC_CLK I see zero documentation on what a gated clock is supposed to be or how it works, and there are zero comments in the code. It's kind of hard to review that way, and even harder to use. Will add Documentation and re-post. Thanks, Mike g. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Sat, Sep 24, 2011 at 8:55 PM, Grant Likely grant.lik...@secretlab.ca wrote:
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.k...@canonical.com
We currently have ~21 definitions of struct clk in the ARM architecture,
each defined on a per-platform basis. This makes it difficult to define
platform- (or architecture-) independent clock sources without making
assumptions about struct clk, and impossible to compile two
platforms with different struct clks into a single image.
This change is an effort to unify struct clk where possible, by defining
a common struct clk, and a set of clock operations. Different clock
implementations can set their own operations, and have a standard
interface for generic code. The callback interface is exposed to the
kernel proper, while the clock implementations only need to be seen by
the platform internals.
The interface is split into two halves:
* struct clk, which is the generic-device-driver interface. This
provides a set of functions which drivers may use to request
enable/disable, query or manipulate in a hardware-independent manner.
* struct clk_hw and struct clk_hw_ops, which is the hardware-specific
interface. Clock drivers implement the ops, which allow the core
clock code to implement the generic 'struct clk' API.
This allows us to share clock code among platforms, and makes it
possible to dynamically create clock devices in platform-independent
code.
Platforms can enable the generic struct clock through
CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a
common, opaque struct clk, and a set of clock operations (defined per
type of clock):
struct clk_hw_ops {
int (*prepare)(struct clk_hw *);
void (*unprepare)(struct clk_hw *);
int (*enable)(struct clk_hw *);
void (*disable)(struct clk_hw *);
unsigned long (*recalc_rate)(struct clk_hw *);
int (*set_rate)(struct clk_hw *,
unsigned long, unsigned long *);
long (*round_rate)(struct clk_hw *, unsigned long);
int (*set_parent)(struct clk_hw *, struct clk *);
struct clk * (*get_parent)(struct clk_hw *);
};
Platform clock code can register a clock through clk_register, passing a
set of operations, and a pointer to hardware-specific data:
struct clk_hw_foo {
struct clk_hw clk;
void __iomem *enable_reg;
};
#define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
static int clk_foo_enable(struct clk_hw *clk)
{
struct clk_foo *foo = to_clk_foo(clk);
raw_writeb(foo-enable_reg, 1);
return 0;
}
struct clk_hw_ops clk_foo_ops = {
.enable = clk_foo_enable,
};
And in the platform initialisation code:
struct clk_foo my_clk_foo;
void init_clocks(void)
{
my_clk_foo.enable_reg = ioremap(...);
clk_register(clk_foo_ops, my_clk_foo, NULL);
Shouldn't this be:
clk_register(clk_foo_ops, my_clk_foo-clk, NULL);
?
Also, this documentation would be good to have in the Documentation
directory instead of lost in a commit header.
Thanks for your review Grant. Will fix the changelog and add proper
Documentation/ in the next round.
Regards,
Mike
Otherwise looks okay to me.
Reviewed-by: Grant Likely grant.lik...@secretlab.ca
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 6/7] clk: Add initial WM831x clock driver
On Sat, Sep 24, 2011 at 9:08 PM, Grant Likely grant.lik...@secretlab.ca wrote:
On Thu, Sep 22, 2011 at 03:27:01PM -0700, Mike Turquette wrote:
From: Mark Brown broo...@opensource.wolfsonmicro.com
The WM831x and WM832x series of PMICs contain a flexible clocking
subsystem intended to provide always on and system core clocks. It
features:
- A 32.768kHz crystal oscillator which can optionally be used to pass
through an externally generated clock.
- A FLL which can be clocked from either the 32.768kHz oscillator or
the CLKIN pin.
- A CLKOUT pin which can bring out either the oscillator or the FLL
output.
- The 32.768kHz clock can also optionally be brought out on the GPIO
pins of the device.
This driver fully supports the 32.768kHz oscillator and CLKOUT. The FLL
is supported only in AUTO mode, the full flexibility of the FLL cannot
currently be used. The use of clock references other than the internal
oscillator is not currently supported, and since clk_set_parent() is not
implemented in the generic clock API the clock tree configuration cannot
be changed at runtime.
Due to a lack of access to systems where the core SoC has been converted
to use the generic clock API this driver has been compile tested only.
Signed-off-by: Mark Brown broo...@opensource.wolfsonmicro.com
Signed-off-by: Mike Turquette mturque...@ti.com
A few minor comments below. Otherwise looks fine to me.
+static __devinit int wm831x_clk_probe(struct platform_device *pdev)
+{
+ struct wm831x *wm831x = dev_get_drvdata(pdev-dev.parent);
+ struct wm831x_clk *clkdata;
+ int ret;
+
+ clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
If devm_kzalloc() is used, then all the kfree unwinding can be
dropped.
+static int __init wm831x_clk_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(wm831x_clk_driver);
+ if (ret != 0)
+ pr_err(Failed to register WM831x clock driver: %d\n, ret);
+
+ return ret;
No need for this song-and-dance. The driver core is pretty well
debugged. Just use return platform_driver_register(...);
Grant,
Thanks for the review.
Mark,
I know you're not carrying this whole set of patches but do you want
to rework this and resend or do you just want me to fix it up?
Changes are trivial if you don't want to touch it.
Regards,
Mike
g.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 0/7] Add a generic struct clk
On Thu, Sep 22, 2011 at 3:26 PM, Mike Turquette mturque...@ti.com wrote: Hi all, The goal of this series is to provide a cross-platform clock framework that platforms can use to model their clock trees and perform common operations on them. Currently everyone re-invents their own clock tree inside platform code which makes it impossible for drivers to use the clock APIs safely across many platforms and for distro's to compile multi-platform kernels which all redefine struct clk and its operations. This is the second version of the common clock patches which were originally posted by Jeremy Kerr and then re-posted with some additional patches by Mark Brown. Mark's re-post didn't have any changes done to the original four patches from Jeremy which is why this series is v2. The changes in this series are minimal: I've folded in some of Mark's fixes and most of the comments posted to his series as well as rebasing on top of v3.1-rc7. The design and functionality hasn't changed much since Jeremy posted v1 of this series. Propagating the rate change up to the parent has been removed from clk_set_rate since that needs some more thought. I also dropped Mark's change to append a device's name to a clk name since device tree might solve this neatly. Again more discussion around that would be good. For convenience these patches can be found at: git://git.linaro.org/people/mturquette/linux.git v3.1-rc7-clkv2 Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/2] cpumask: introduce cpumask for hotpluggable CPUs
On Wed, Aug 10, 2011 at 11:06 PM, Amit Kucheria amit.kuche...@linaro.org wrote: See comments inline. On 11 Aug 10, Mike Turquette wrote: On some platforms it is possible to have some CPUs which support CPU hotplug and some which do not. Currently the prescence of an 'online' sysfs entry in userspace is adequate for applications to know that a CPU supports hotplug, but there is no convenient way to make the same determination in the kernel. To better model this relationship this patch introduces a new cpumask to track CPUs that support CPU hotplug operations. This new cpumask is populated at boot-time and remains static for the life of the machine. Bits set in the mask indicate a CPU which supports hotplug, but make no guarantees about whether that CPU is currently online or not. Likewise a cleared bit in the mask indicates either a CPU which cannot hotplug or a lack of a populated CPU. The purpose of this new cpumask is to aid kernel code which uses CPU to take CPUs online and offline. Possible uses are as a thermal event mitigation technique or as a power capping mechanism. Signed-off-by: Mike Turquette mturque...@ti.com --- Change log: v2: fixed missing parentheses in cpumask_test_cpu and improved grammar in comments include/linux/cpumask.h | 27 ++- kernel/cpu.c | 18 ++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index b24ac56..52e64a7 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -39,10 +39,11 @@ extern int nr_cpu_ids; * The following particular system cpumasks and operations manage * possible, present, active and online cpus. * - * cpu_possible_mask- has bit 'cpu' set iff cpu is populatable - * cpu_present_mask - has bit 'cpu' set iff cpu is populated - * cpu_online_mask - has bit 'cpu' set iff cpu available to scheduler - * cpu_active_mask - has bit 'cpu' set iff cpu available to migration + * cpu_possible_mask - has bit 'cpu' set iff cpu is populatable + * cpu_hotpluggable_mask - has bit 'cpu' set iff cpu is hotpluggable + * cpu_present_mask - has bit 'cpu' set iff cpu is populated + * cpu_online_mask - has bit 'cpu' set iff cpu available to scheduler + * cpu_active_mask - has bit 'cpu' set iff cpu available to migration * * If !CONFIG_HOTPLUG_CPU, present == possible, and active == online. * @@ -51,7 +52,11 @@ extern int nr_cpu_ids; * life of that system boot. The cpu_present_mask is dynamic(*), * representing which CPUs are currently plugged in. And * cpu_online_mask is the dynamic subset of cpu_present_mask, - * indicating those CPUs available for scheduling. + * indicating those CPUs available for scheduling. The + * cpu_hotpluggable_mask is also fixed at boot time as the set of CPU + * id's which are possible AND can hotplug. Cleared bits in this mask + * mean that either the CPU is not possible, or it is possible but does + * not support CPU hotplug operations. * * If HOTPLUG is enabled, then cpu_possible_mask is forced to have * all NR_CPUS bits set, otherwise it is just the set of CPUs that @@ -61,6 +66,9 @@ extern int nr_cpu_ids; * depending on what ACPI reports as currently plugged in, otherwise * cpu_present_mask is just a copy of cpu_possible_mask. * + * If HOTPLUG is not enabled then cpu_hotpluggable_mask is the empty + * set. + * * (*) Well, cpu_present_mask is dynamic in the hotplug case. If not * hotplug, it's a copy of cpu_possible_mask, hence fixed at boot. * @@ -76,6 +84,7 @@ extern int nr_cpu_ids; */ extern const struct cpumask *const cpu_possible_mask; +extern const struct cpumask *const cpu_hotpluggable_mask; extern const struct cpumask *const cpu_online_mask; extern const struct cpumask *const cpu_present_mask; extern const struct cpumask *const cpu_active_mask; @@ -85,19 +94,23 @@ extern const struct cpumask *const cpu_active_mask; #define num_possible_cpus() cpumask_weight(cpu_possible_mask) #define num_present_cpus() cpumask_weight(cpu_present_mask) #define num_active_cpus() cpumask_weight(cpu_active_mask) +#define num_hotpluggable_cpus() cpumask_weight(cpu_hotpluggable_mask) #define cpu_online(cpu) cpumask_test_cpu((cpu), cpu_online_mask) #define cpu_possible(cpu) cpumask_test_cpu((cpu), cpu_possible_mask) #define cpu_present(cpu) cpumask_test_cpu((cpu), cpu_present_mask) #define cpu_active(cpu) cpumask_test_cpu((cpu), cpu_active_mask) +#define cpu_hotpluggable(cpu) cpumask_test_cpu((cpu, cpu_hotpluggable_mask)) The bracket should be around cpu, like this (cpu) so that there are no side-effects when passing anything to the macro. See the other #defines above. Wow, not sure how that happened... will send in V3... #else
Re: [PATCH v2 0/2] new cpumask for hotpluggable CPUs
On Thu, Aug 11, 2011 at 11:30 AM, Peter Zijlstra pet...@infradead.org wrote: On Wed, 2011-08-10 at 13:03 -0700, Mike Turquette wrote: This patch series introduces a new cpumask which tracks CPUs that support hotplugging. The purpose of this patch series is to provide a simple method for kernel code to know which CPUs can be hotplugged and which ones cannot. Potential users of this code might be a thermal mitigation technique which uses hotplug to lower temperature, or a power capping mechanism which uses hotplug to lower power consumption. All the of usual cpumask helper functions are created for this new mask. The second patch in this series simply sets the bit for elligible CPUs while they are being registered. The cpumask itself is static after boot and should not change (like the possbile mask). I still most strongly object to people using hotplug for these goals. Why do you need to go through the entire dance of hotplug just to idle a cpu? Hotplug not only idles the cpu but tears down (and rebuilds) an insane amount of resources associated with the cpu. I think you're nacking the wrong series. This patchset simply allows kernel space to know which CPUs can go offline and which one can't, which seems pretty innocuous. Are you fundamentally opposed to the kernel having better accessor functions to this data? I'll soon be posting some code which does implement hotplug as a power-capping feature. I think *that* is the patch that you'll want to nack. Thanks for reviewing, Mike Nacked-by: Peter Zijlstra a.p.zijls...@chello.nl ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: do not mark CPU 0 as hotpluggable
On Fri, Jul 22, 2011 at 5:53 AM, Santosh Shilimkar santosh.shilim...@ti.com wrote: On 7/22/2011 6:15 PM, Woodruff, Richard wrote: From: linux-arm-kernel-boun...@lists.infradead.org [mailto:linux-arm- kernel-boun...@lists.infradead.org] On Behalf Of Shilimkar, Santosh With fixed IRQ migration and forcing CPU0 into an infinite WFI loop, I can offline CPU0 and still have a running system. The secure software runs only on CPU0 and that's the biggest limitation. Other issues like hand-shake as you pointed out, power sequencing etc can be worked around. As you know well some of the secure APIs work on CPU1 and others do not. I notice in other thread Russell made assumption about CPU1 being able to use all because it could run some. This is not the case. I clarified that on the other thread. I've asked a few other ARM folks (out of band) to weigh in on this thread to determine if their platform has similar limitations as OMAP. Unfortunately no one else has responded. Still the limitation for OMAP remains. Earlier in this thread I provided an alternative to blacklisting CPU0 for all ARM platforms by instead using a config option, but it received no comments. What is the best way to move forward? Thanks, Mike Regards Santosh ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/2] cpumask: introduce cpumask for hotpluggable CPUs
On Wed, Aug 10, 2011 at 1:42 AM, Amit Kucheria amit.kuche...@linaro.org wrote: On 11 Aug 09, Mike Turquette wrote: On some platforms it is possible to have some CPUs which support CPU hotplug and some which do not. Currently the prescence of an 'online' sysfs entry in userspace is adequate for applications to know that a CPU supports hotplug, but there is no convenient way to make the same determination in the kernel. To better model this relationship this patch introduces a new cpumask to track CPUs that support CPU hotplug operations. This new cpumask is populated at boot-time and remains static for the life of the machine. Bits set in the mask indicate a CPU which supports hotplug, but make no guarantees about whether that CPU is currently online or not. Likewise a cleared bit in the mask indicates either a CPU which cannot hotplug or a lack of a populated CPU. The purpose of this new cpumask is to aid kernel code which uses CPU to take CPUs online and offline. Possible uses are as a thermal event mitigation technique or as a power capping mechanism. Signed-off-by: Mike Turquette mturque...@ti.com --- include/linux/cpumask.h | 27 ++- kernel/cpu.c | 18 ++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index b24ac56..9eed444 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -39,10 +39,11 @@ extern int nr_cpu_ids; * The following particular system cpumasks and operations manage * possible, present, active and online cpus. * - * cpu_possible_mask- has bit 'cpu' set iff cpu is populatable - * cpu_present_mask - has bit 'cpu' set iff cpu is populated - * cpu_online_mask - has bit 'cpu' set iff cpu available to scheduler - * cpu_active_mask - has bit 'cpu' set iff cpu available to migration + * cpu_possible_mask - has bit 'cpu' set iff cpu is populatable + * cpu_hotpluggable_mask - has bit 'cpu' set iff cpu is hotpluggable + * cpu_present_mask - has bit 'cpu' set iff cpu is populated + * cpu_online_mask - has bit 'cpu' set iff cpu available to scheduler + * cpu_active_mask - has bit 'cpu' set iff cpu available to migration * * If !CONFIG_HOTPLUG_CPU, present == possible, and active == online. * @@ -51,7 +52,11 @@ extern int nr_cpu_ids; * life of that system boot. The cpu_present_mask is dynamic(*), * representing which CPUs are currently plugged in. And * cpu_online_mask is the dynamic subset of cpu_present_mask, - * indicating those CPUs available for scheduling. + * indicating those CPUs available for scheduling. The + * cpu_hotpluggable_mask is also fixed at boot time, as the set of CPU + * id's which are possible AND can hotplug. Cleared bits in this mask + * mean that either the CPU is not possible, or it is possible but does + * not support CPU hotplug operations. * * If HOTPLUG is enabled, then cpu_possible_mask is forced to have * all NR_CPUS bits set, otherwise it is just the set of CPUs that @@ -61,6 +66,9 @@ extern int nr_cpu_ids; * depending on what ACPI reports as currently plugged in, otherwise * cpu_present_mask is just a copy of cpu_possible_mask. * + * If CONFIG_HOTPLUG_CPU is enabled, then cpu_hotpluggable_mask matches + * the description above, otherwise it is the empty set. + * * (*) Well, cpu_present_mask is dynamic in the hotplug case. If not * hotplug, it's a copy of cpu_possible_mask, hence fixed at boot. * @@ -76,6 +84,7 @@ extern int nr_cpu_ids; */ extern const struct cpumask *const cpu_possible_mask; +extern const struct cpumask *const cpu_hotpluggable_mask; extern const struct cpumask *const cpu_online_mask; extern const struct cpumask *const cpu_present_mask; extern const struct cpumask *const cpu_active_mask; @@ -85,19 +94,23 @@ extern const struct cpumask *const cpu_active_mask; #define num_possible_cpus() cpumask_weight(cpu_possible_mask) #define num_present_cpus() cpumask_weight(cpu_present_mask) #define num_active_cpus() cpumask_weight(cpu_active_mask) +#define num_hotpluggable_cpus() cpumask_weight(cpu_hotpluggable_mask) #define cpu_online(cpu) cpumask_test_cpu((cpu), cpu_online_mask) #define cpu_possible(cpu) cpumask_test_cpu((cpu), cpu_possible_mask) #define cpu_present(cpu) cpumask_test_cpu((cpu), cpu_present_mask) #define cpu_active(cpu) cpumask_test_cpu((cpu), cpu_active_mask) +#define cpu_hotpluggable(cpu) cpumask_test_cpu((cpu, cpu_hotpluggable_mask) missing closing bracket? Oops. Will fix in V2. Thanks, Mike #else #define num_online_cpus() 1U #define num_possible_cpus() 1U #define num_present_cpus() 1U #define num_active_cpus() 1U +#define num_hotpluggable_cpus() 0 #define
Re: [PATCH 1/2] cpumask: introduce cpumask for hotpluggable CPUs
On Tue, Aug 9, 2011 at 7:01 PM, Christian Robottom Reis k...@linaro.org wrote: On Tue, Aug 09, 2011 at 06:33:26PM -0700, Mike Turquette wrote: - * cpu_possible_mask- has bit 'cpu' set iff cpu is populatable - * cpu_present_mask - has bit 'cpu' set iff cpu is populated - * cpu_online_mask - has bit 'cpu' set iff cpu available to scheduler - * cpu_active_mask - has bit 'cpu' set iff cpu available to migration + * cpu_possible_mask - has bit 'cpu' set iff cpu is populatable + * cpu_hotpluggable_mask - has bit 'cpu' set iff cpu is hotpluggable + * cpu_present_mask - has bit 'cpu' set iff cpu is populated + * cpu_online_mask - has bit 'cpu' set iff cpu available to scheduler + * cpu_active_mask - has bit 'cpu' set iff cpu available to migration Why not fix the 'iff' typo while you're here? iff = if and only if http://en.wikipedia.org/wiki/If_and_only_if Thanks! Mike Christian Robottom Reis, Engineering VP Brazil (GMT-3) | [+55] 16 9112 6430 | [+1] 612 216 4935 Linaro.org: Open Source Software for ARM SoCs ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: do not mark CPU 0 as hotpluggable
On Thu, Jul 21, 2011 at 12:45 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Wed, Jul 20, 2011 at 04:32:25PM -0700, Mike Turquette wrote:
A quick poll of the ARM platforms that implement CPU Hotplug support
shows that every platform treats CPU 0 as a special case that cannot be
hotplugged. In fact every platform has identical code for
platform_cpu_die which returns -EPERM in the case of CPU 0.
Are you sure that's just not because everyone copied what Realview has
been doing (highly likely)?
Copy/paste is always likely. Would be nice for other platform folks
to weigh in on this.
I suspect that there's no reason that CPU0 can't be taken down, especially
on those platforms which don't take the CPU fully offline but just put it
into a WFI loop.
Those which restart the CPUs through the boot loader probably detect CPU0
as the boot CPU, so they probably can't take CPU0 down.
OMAP does seem to have this limitation and Santosh/Richard can provide
much better details than me in the parallel thread. Again, would be
nice to hear if other platforms have similar limitations from their
stakeholders.
The idea here is to not mark a CPU hotpluggable if it cannot be
hotplugged. If the limitations turn out to be legitimate for some
platforms but not others, what's the best way to handle it? Either
move the topology initialization to platform code or allow platforms
to set some config option. Patches for the latter are below, but I
think the current discussion on whether or not other platforms can
actually hotplug CPU0 should run its course before considering below
patches too seriously.
From b734cedf23a9f8366faa1a961d87cb4bc221291e Mon Sep 17 00:00:00 2001
From: Mike Turquette mturque...@ti.com
Date: Mon, 18 Jul 2011 17:33:22 -0700
Subject: [PATCH 1/2] ARM: conditionally allow master CPU hotplugging
Currently all CPUs are marked as hotpluggable in topology_init(). This
is not true for all ARM SMP platforms including OMAP4. In the OMAP4
case, CPU0 is considered as privileged due to TrustZone software and
other considerations.
This patch introduces a new config option, HOTPLUG_CPU_MASTER, which
prevents the first CPU in possible_cpu_mask from being marked
hotpluggable if set.
Signed-off-by: Mike Turquette mturque...@ti.com
---
arch/arm/Kconfig|8
arch/arm/kernel/setup.c |5 +
2 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9adc278..a6e34f5 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1393,6 +1393,14 @@ config HOTPLUG_CPU
Say Y here to experiment with turning CPUs off and on. CPUs
can be controlled through /sys/devices/system/cpu.
+config HOTPLUG_CPU_MASTER
+ bool Prevent first CPU (master) from hotplugging
+ depends on HOTPLUG_CPU
+ help
+ Say Y here if your platform treats the first CPU as a special
+ master CPU and cannot allow it to hotplug. This prevents the
+ online entry in sysfs for that CPU.
+
config LOCAL_TIMERS
bool Use local timer interrupts
depends on SMP
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index ed11fb0..3785420 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -940,7 +940,12 @@ static int __init topology_init(void)
for_each_possible_cpu(cpu) {
struct cpuinfo_arm *cpuinfo = per_cpu(cpu_data, cpu);
+#ifdef CONFIG_HOTPLUG_CPU_MASTER
+ if (cpu)
+ cpuinfo-cpu.hotpluggable = 1;
+#else
cpuinfo-cpu.hotpluggable = 1;
+#endif
register_cpu(cpuinfo-cpu, cpu);
}
--
1.7.4.1
and,
From 5e22ea1fa6a10ea0440057832496087297079a5d Mon Sep 17 00:00:00 2001
From: Mike Turquette mturque...@ti.com
Date: Tue, 19 Jul 2011 16:47:11 -0700
Subject: [PATCH 2/2] OMAP4: Kconfig: remove hotplug control for CPU0
On OMAP4, CPU0 cannot be dynamically offlined through the usual CPU
hotplug mechanism due to a variety of constraints. It is treated as a
special master CPU for these reasons.
Select HOTPLUG_CPU_MASTER to prevent creation of the hotplug control
entry in sysfs.
Signed-off-by: Mike Turquette mturque...@ti.com
---
arch/arm/mach-omap2/Kconfig |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 4ae6257..b6199e3 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -51,6 +51,7 @@ config ARCH_OMAP4
select ARCH_HAS_OPP
select PM_OPP if PM
select USB_ARCH_HAS_EHCI
+ select HOTPLUG_CPU_MASTER
comment OMAP Core Type
depends on ARCH_OMAP2
--
1.7.4.1
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] OMAP PM: Optimize cpufreq transition latency
On Thu, Nov 25, 2010 at 7:38 AM, Vishwanath BS vishwanath...@ti.com wrote: Currently cpufreq transition latency value does not really reflect the actual OMAP OPP transition delay. This patch has the actual latency value. I did profile the DVFS latency on OMAP3430, OMAP3630 and OMAP4 for worstcase(MPU and Core together) and found that in none of these platforms, transiton value goes beyong 10ms. Added a buffer of 20ms to avoid too frequent ondemand timer expiry. With this change, performance of ondemand governor is improved when tested using cpufreqbench tool. Without this patch, cpufreq-bench reported ondemand performance as 40% of performance governor, and with this patch it's around 70% (using below procedure). cpufreq-bench: http://lwn.net/Articles/339862/ http://ftp.riken.go.jp/archives/Linux/suse/people/ckornacker/cpufreq-bench/ Command used for performance testing: cpufreq-bench -l 5 -s 10 -x 5 -y 10 -g ondemand -r 5 -n 5 -v Signed-off-by: Vishwanath BS vishwanath...@ti.com --- arch/arm/plat-omap/cpu-omap.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) mode change 100644 = 100755 arch/arm/plat-omap/cpu-omap.c diff --git a/arch/arm/plat-omap/cpu-omap.c b/arch/arm/plat-omap/cpu-omap.c old mode 100644 new mode 100755 index c47faf8..d3fc423 --- a/arch/arm/plat-omap/cpu-omap.c +++ b/arch/arm/plat-omap/cpu-omap.c @@ -158,8 +158,8 @@ static int __init omap_cpu_init(struct cpufreq_policy *policy) policy-max = policy-cpuinfo.max_freq; policy-cur = omap_getspeed(0); - /* FIXME: what's the actual transition time? */ - policy-cpuinfo.transition_latency = 300 * 1000; + /* Program the actual transition time for worstcase */ + policy-cpuinfo.transition_latency = 30 * 1000; Vishwa, This is a frequent periodic timer. Does a smaller value have any affect on CPUidle wakeups? Thanks, Mike return 0; } -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
