Re: [PATCH 3/5] clk: remove COMMON_CLK_DISABLE_UNUSED
On 05/06/2012 10:08 PM, Mike Turquette wrote:
Exposing this option generates confusion and incorrect behavior for
single-image builds across platforms. Enable this behavior permanently.
Signed-off-by: Mike Turquettemturque...@linaro.org
---
drivers/clk/Kconfig | 11 ---
drivers/clk/clk.c |2 --
2 files changed, 0 insertions(+), 13 deletions(-)
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index f05a60d..4864407 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -23,17 +23,6 @@ config COMMON_CLK
menu Common Clock Framework
depends on COMMON_CLK
-config COMMON_CLK_DISABLE_UNUSED
- bool Disabled unused clocks at boot
- depends on COMMON_CLK
- ---help---
- Traverses the entire clock tree and disables any clocks that are
- enabled in hardware but have not been enabled by any device drivers.
- This saves power and keeps the software model of the clock in line
- with reality.
-
- If in doubt, say N.
-
config COMMON_CLK_DEBUG
bool DebugFS representation of clock tree
depends on COMMON_CLK
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7ceca0e..e5d5dc1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -196,7 +196,6 @@ late_initcall(clk_debug_init);
static inline int clk_debug_register(struct clk *clk) { return 0; }
#endif
-#ifdef CONFIG_COMMON_CLK_DISABLE_UNUSED
/* caller must hold prepare_lock */
static void clk_disable_unused_subtree(struct clk *clk)
{
@@ -246,7 +245,6 @@ static int clk_disable_unused(void)
return 0;
}
late_initcall(clk_disable_unused);
-#endif
/***helper functions ***/
Good decision! Acked.
-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 5/5] clk: add a fixed factor clock
On 05/06/2012 10:08 PM, Mike Turquette wrote:
From: Sascha Hauers.ha...@pengutronix.de
Having fixed factors/dividers in hardware is a common pattern, so
add a basic clock type doing this. It basically describes a fixed
factor clock using a nominator and a denominator.
Signed-off-by: Sascha Hauers.ha...@pengutronix.de
Reviewed-by: Viresh Kumarviresh.ku...@st.com
[mturque...@linaro.org: constify parent_names in static init macro]
[mturque...@linaro.org: copy/paste bug from mux in static init macro]
[mturque...@linaro.org: fix error handling in clk_register_fixed_factor]
Signed-off-by: Mike Turquettemturque...@linaro.org
---
drivers/clk/Makefile |2 +-
drivers/clk/clk-fixed-factor.c | 95
include/linux/clk-private.h| 20
include/linux/clk-provider.h | 23 ++
4 files changed, 139 insertions(+), 1 deletions(-)
create mode 100644 drivers/clk/clk-fixed-factor.c
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 1f736bc..24aa714 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,4 +1,4 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
obj-$(CONFIG_COMMON_CLK) += clk.o clk-fixed-rate.o clk-gate.o \
- clk-mux.o clk-divider.o
+ clk-mux.o clk-divider.o clk-fixed-factor.o
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
new file mode 100644
index 000..1f2da01
--- /dev/null
+++ b/drivers/clk/clk-fixed-factor.c
@@ -0,0 +1,95 @@
+/*
+ * Copyright (C) 2011 Sascha Hauer, Pengutronixs.ha...@pengutronix.de
+ *
+ * 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.
+ */
+#includelinux/module.h
+#includelinux/clk-provider.h
+#includelinux/slab.h
+#includelinux/err.h
+
+/*
+ * DOC: basic fixed multiplier and 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 fixed. clk-rate = parent-rate / div * mult
+ * parent - fixed parent. No clk_set_parent support
+ */
+
+#define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw)
+
+static unsigned long clk_factor_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
+
+ return (parent_rate / fix-div) * fix-mult;
Wouldn't multiplying and then dividing result in better accuracy? Is it
worth doing that (since we will have to account for overflow)?
+}
+
+static long clk_factor_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *prate)
+{
+ struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
+
+ if (__clk_get_flags(hw-clk) CLK_SET_RATE_PARENT) {
+ unsigned long best_parent;
+
+ best_parent = (rate / fix-mult) * fix-div;
+ *prate = __clk_round_rate(__clk_get_parent(hw-clk),
+ best_parent);
+ }
+
+ return (*prate / fix-div) * fix-mult;
+}
+
+static int clk_factor_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ return 0;
+}
+
+struct clk_ops clk_fixed_factor_ops = {
+ .round_rate = clk_factor_round_rate,
+ .set_rate = clk_factor_set_rate,
+ .recalc_rate = clk_factor_recalc_rate,
+};
+EXPORT_SYMBOL_GPL(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)
+{
+ 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).
+
+ /* struct clk_fixed_factor assignments */
+ fix-mult = mult;
+ fix-div = div;
+ fix-hw.init =init;
+
+ init.name = name;
+ init.ops =clk_fixed_factor_ops;
+ init.flags = flags;
+ init.parent_names =parent_name;
+ init.num_parents = 1;
+
+ clk = clk_register(dev,fix-hw);
+
+ if (IS_ERR(clk))
+ kfree(fix);
+
+ return clk;
+}
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index b258532..eb3f84b 100644
--- a/include/linux/clk-private.h
+++ b/include/linux/clk-private.h
@@ -143,6 +143,26 @@ struct clk {
Re: [PATCH 5/5] clk: add a fixed factor clock
On 05/07/2012 01:14 PM, Turquette, Mike wrote:
On Mon, May 7, 2012 at 12:54 PM, Saravana Kannanskan...@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.
I would think those clocks would have some type of register control. At
least for enable/disable. This clock just seems to implement a simple
integer divider/fractional multiplier. I think we should add this check.
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?
May be the other macros should be refactored to not be nested too?
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.
Never mind. If you are using this clock type, then it's okay to support
static init.
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...
I still prefer them to be split out since one doesn't need to include
(and be recompiled when it changes) stuff they don't care about. But
it's not yet a significant point to argue about. So, let's wait and see
how it goes.
-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 13/13] clk: basic: improve parent_names return errors
On Tue, April 17, 2012 12:17 am, Shawn Guo wrote: On 17 April 2012 11:50, Turquette, Mike mturque...@ti.com wrote: 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. Saravana, (*nudge*) (*nudge*) goes to you now :) Done! I especially like how it turned out. -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 13/13] clk: basic: improve parent_names return errors
On 04/17/2012 12:17 AM, Shawn Guo wrote: On 17 April 2012 11:50, Turquette, Mikemturque...@ti.com wrote: 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. Saravana, (*nudge*) (*nudge*) goes to you now :) Regards, Shawn Sorry :) Will do soon. -Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/28/2012 10:08 AM, Turquette, Mike wrote: On Tue, Mar 27, 2012 at 8:06 PM, Saravana Kannanskan...@codeaurora.org wrote: snip I think there is still a problem with not being able to differentiate between pre-change recalc and post-change recalc. This applies for any set parent and set rate on a clock that has children. Consider this simple example: * Divider clock B is fed from clock A. * Clock B can never output 120 MHz due to downstream HW/clock limitations. * Clock A is running at 200 MHz * Clock B divider is set to 2. Now, say the rate of clock A is changing from 200 MHz to 300 MHz (due to set rate or set parent). In this case, the clock B divider should be set to 3 pre-rate change to guarantee that the output of clock B is never 120 MHz. So the rate of clock B will go from 100 MHz (200/2) to 66 MHz (200/3) to 100 MHz (300/3) and everything is good. Assume we somehow managed to do the above. So, now clock A is at 300 MHz, clock B divider is at 3 and the clock B output is 100 MHz. Now, say the rate of clock A changes from 300 MHz to 100 MHz. In this case the clock B divider should only be changed post rate change. If we do it pre rate change, then the output will go from 100 MHz (300/3) to 150 MHz (300/1) to 100 MHz (100/1). We went past the 120 MHz limit of clock B's output rate. If we do this post rate change, we can do this without exceeding the max output limit of clock B. It will go from 100 MHz (300/3) to 33 MHz (100/3) to 100 MHz (100/1). We never went past the 120 MHz limit. So, at least for this reason above, I think we need to pass a pre/post parameter to the recalc ops. Sorry if I wasn't clear. But the case above is a separate issue from what I mention below. What are your thoughts on handling this? Pass msg to recalc_rates? While we are at it, we should probably just add a failure option for recalc to make it easy to reject unacceptable rate changes. To keep the clock framework code simpler, you could decide to allow errors only for the pre-change recalc calls. That way, the error case roll back code won't be crazy. recalc is too late to catch this. I think you mean round_rate. We want to determine which rate changes are out-of-spec during clk_calc_new_rates (for clk_set_rate) which involves round_rate being a bit smarter about what it can and cannot do. The case I'm referring to is set_parent(). set_rate() and set_parent() have a lot of common implications and it doesn't look like the clock framework is handling the common cases the same way for both set_parent() and set_rate(). It almost feels like set_parent() and set_rate() should just be wrappers around some common function that handles most of the work. After all, set_parent() is just a slimmed down version of set_rate(). Set rate is just a combination of set parent and set divider. Anyways I'm looking at ways to do this in my clk-dependencies branch. Are you also looking into the pre/post rate change divider handling case I mentioned above? Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/23/2012 04:28 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 4:04 PM, Saravana Kannanskan...@codeaurora.org wrote: On 03/23/2012 03:32 PM, Turquette, Mike wrote: .recalc_rate serves two purposes: first it recalculates the rate after the rate has changed and you pass in a new parent_rate argument. The second purpose is to speculate a rate change. You can pass in any rate for the parent_rate parameter when you call .recalc_rates. This is what __speculate_rates does before the rate changes. For clk_set_parent we call, __clk_speculate_rates(clk, parent-rate); Where parent above is the *new* parent. So this will let us propagate pre-rate change notifiers with the new rate. Your .recalc_rate callback doesn't need to differentiate between the two calls to .recalc_rate. It should just take in the parent_rate value and do the calculation required of it. Yeah, this is generally true. But, in this specific case, the clock is a mux that can literally measure the real rate. I would like the rate of this mux clock to be the real measured rate and not just the parent rate. That's pretty cool hardware. Do you find that software-only tracking doesn't match up with sampling the frequency in hardware? If software tracking of the rate changes is correct then you could just skip using this hardware feature. We use this HW for automated testing of the SW. It's very handy. So, I definitely need to support it. I could actually measure the current rate and return that for recalc_rate(), but then the reported rate change during the pre-change notification would be wrong. Having the msg will let us at least fake the rate correctly for pre change notifiers. In previous series I had separate .recalc_rate and .speculate_rate callbacks. I merged them since they were almost identical. I'd prefer not to reintroduce them since it creates a burden on others to support seperate callbacks, and passing in the message is one way to solve that... I'll think on this a bit more. For this specific clock, I don't think anyone is really going to care if the pre rate change notification is correct. So, I will just go with reporting the wrong rate during pre-change for now. But it does increase the total testing time quite a bit as I have to measure the real rate during pre and post change and measurement is relatively slow. It actually does add up into minutes when the whole test suite is run. So, would be nice to have this fixed eventually but it's not urgent for this measure clock. How does a child (or grand child) clock (not a driver using the clock) reject a rate change if it know it can't handle that freq from the parent? I won't claim to know this part of the code thoroughly, but I can't find an easy way to do this. Technically you could subscribe a notifier to your grand-child clock, even if there is no driver for it. The same code that implements your platform's clock operations could register the notifier handler. You can see how this works in clk_propagate_rate_change. That usage is certainly targeted more at drivers but could be made to work for your case. Let me know what you think; this is an interesting issue. I think notifications should be left to drivers. Notifications are too heavy handed for enforcing requirements of the clock tree. Also, clk_hw to clk might no longer be a 1-1 mapping in the future. It's quite possible that each clk_get() would get a different struct clk based on the caller to keep track of their constraints separately. So, clock drivers shouldn't ever use them to identify a clock. Reason for rejection could be things like the counter (for division, etc) has an upper limit on how fast it can run. Are you hitting this in practice today? Such HW limitations are real. But we don't use clk_set_parent() in our drivers often. The clock framework shouldn't be a perfect piece of code that can validate every possible combination imaginable. Some burden falls on the code calling the clk api (especially if that code is platform code) to make sure that it doesn't do stupid things. We can't expect a normal driver to know that the clk_set_parent() shouldn't be called when the parent is at a particular rate since there are clock HW limitations. The clock framework doesn't need to validate everything, but it should give the clock driver the option of rejecting invalid requests. Btw, I think this earlier comment from me is wrong. Nevermind, my brain isn't working today. I can just do that different ordering in ops-set_parent(). I think there is still a problem with not being able to differentiate between pre-change recalc and post-change recalc. This applies for any set parent and set rate on a clock that has children. Consider this simple example: * Divider clock B is fed from clock A. * Clock B can never output 120 MHz due to downstream HW/clock limitations. * Clock A is running at 200 MHz * Clock B divider is set to 2. Now, say the
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/20/2012 08:10 PM, Saravana Kannan wrote: On 03/20/2012 04:53 PM, Turquette, Mike wrote: It does make me wonder if it would be a good idea to pass in the parent rate for .set_parent, which is analogous to .set_rate in many ways. I need to think a bit more about this. I was thinking about this. I think the common clock fwk should let the set_parent ops return the rate of the clock in addition to passing the rate of the parent in. Say this is a divider clock and some one changes the parent. The cached rate of the clock in the clock fwk is no longer correct. So, the clock fwk should also add a *new_rate param to set parent ops. Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/23/2012 02:39 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 2:33 PM, Saravana Kannanskan...@codeaurora.org wrote: On 03/20/2012 08:10 PM, Saravana Kannan wrote: On 03/20/2012 04:53 PM, Turquette, Mike wrote: It does make me wonder if it would be a good idea to pass in the parent rate for .set_parent, which is analogous to .set_rate in many ways. I need to think a bit more about this. I was thinking about this. I think the common clock fwk should let the set_parent ops return the rate of the clock in addition to passing the rate of the parent in. Say this is a divider clock and some one changes the parent. The cached rate of the clock in the clock fwk is no longer correct. So, the clock fwk should also add a *new_rate param to set parent ops. __clk_recalc_rates is called by __clk_reparent which is called by clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. Does this not handle the old cached clk-rate for you? Yeah, I realized this just after I sent the email. I'm looking at the code to see if that's sufficient or not. Will get back soon. -Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/23/2012 02:39 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 2:33 PM, Saravana Kannanskan...@codeaurora.org wrote: On 03/20/2012 08:10 PM, Saravana Kannan wrote: On 03/20/2012 04:53 PM, Turquette, Mike wrote: It does make me wonder if it would be a good idea to pass in the parent rate for .set_parent, which is analogous to .set_rate in many ways. I need to think a bit more about this. I was thinking about this. I think the common clock fwk should let the set_parent ops return the rate of the clock in addition to passing the rate of the parent in. Say this is a divider clock and some one changes the parent. The cached rate of the clock in the clock fwk is no longer correct. So, the clock fwk should also add a *new_rate param to set parent ops. __clk_recalc_rates is called by __clk_reparent which is called by clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. Does this not handle the old cached clk-rate for you? For the set_parent case, ops-recalc_rate() is called twice. Once for PRE_CHANGE and once for POST_CHANGE. For this clock, I can only really recalc the rate during the POST_CHANGE call. So, how should I differentiate the two cases? On a separate note: Sorry if I missed any earlier discussion on this, but what's the reason for calling recalc_rate() pre-change and post-change but without giving it the ability to differentiate between the two? I think it's quite useful for recalc_rate to be called pre/post change (some steps have to be done pre/post change depending on whether the parent rate is increasing or decreasing). But I don't see the msg being passed along. Also, I noticed that clk_set_parent() is treating a NULL as an invalid clock. Should that be fixed? set_parent(NULL) could be treated as a grounding the clock. Should we let the ops-set_parent determine if NULL is valid option? Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/23/2012 03:32 PM, Turquette, Mike wrote: On Fri, Mar 23, 2012 at 3:12 PM, Saravana Kannanskan...@codeaurora.org wrote: On 03/23/2012 02:39 PM, Turquette, Mike wrote: __clk_recalc_rates is called by __clk_reparent which is called by clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. Does this not handle the old cached clk-rate for you? For the set_parent case, ops-recalc_rate() is called twice. Once for PRE_CHANGE and once for POST_CHANGE. For this clock, I can only really recalc the rate during the POST_CHANGE call. So, how should I differentiate the two cases? .recalc_rate serves two purposes: first it recalculates the rate after the rate has changed and you pass in a new parent_rate argument. The second purpose is to speculate a rate change. You can pass in any rate for the parent_rate parameter when you call .recalc_rates. This is what __speculate_rates does before the rate changes. For clk_set_parent we call, __clk_speculate_rates(clk, parent-rate); Where parent above is the *new* parent. So this will let us propagate pre-rate change notifiers with the new rate. Your .recalc_rate callback doesn't need to differentiate between the two calls to .recalc_rate. It should just take in the parent_rate value and do the calculation required of it. Yeah, this is generally true. But, in this specific case, the clock is a mux that can literally measure the real rate. I would like the rate of this mux clock to be the real measured rate and not just the parent rate. I could actually measure the current rate and return that for recalc_rate(), but then the reported rate change during the pre-change notification would be wrong. Having the msg will let us at least fake the rate correctly for pre change notifiers. I think it's quite useful for recalc_rate to be called pre/post change (some steps have to be done pre/post change depending on whether the parent rate is increasing or decreasing). But I don't see the msg being passed along. What kind of steps? Does your .recalc_rate perform these steps? I need more details to understand your requirements. Nevermind, my brain isn't working today. I can just do that different ordering in ops-set_parent(). But the measure clocks case is still true though. But this did bring up another point in my head. Hopefully this one isn't my brain playing tricks again today. How does a child (or grand child) clock (not a driver using the clock) reject a rate change if it know it can't handle that freq from the parent? I won't claim to know this part of the code thoroughly, but I can't find an easy way to do this. Reason for rejection could be things like the counter (for division, etc) has an upper limit on how fast it can run. Also, I noticed that clk_set_parent() is treating a NULL as an invalid clock. Should that be fixed? set_parent(NULL) could be treated as a grounding the clock. Should we let the ops-set_parent determine if NULL is valid option? We must be looking at different code. clk_set_parent doesn't return any error if parent == NULL. Bringing this to my attention does show that we do deref the parent pointer without a check though... Do you have a real use case for this? Due to the way that we match the parent pointer with the cached clk-parents member it would be painful to support NULL parents as valid. I don't have a real use case for MSM. We just have a ground_clock. It is also worth considering whether clk_set_parent is really the correct operation for grounding a clock. clk_unprepare might be a better candidate. Yeah, not a real use case for MSM. Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Tue, March 20, 2012 7:02 am, Shawn Guo wrote:
On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote:
...
+struct clk_ops {
+int (*prepare)(struct clk_hw *hw);
+void(*unprepare)(struct clk_hw *hw);
+int (*enable)(struct clk_hw *hw);
+void(*disable)(struct clk_hw *hw);
+int (*is_enabled)(struct clk_hw *hw);
+unsigned long (*recalc_rate)(struct clk_hw *hw,
+unsigned long parent_rate);
I believe I have heard people love the interface with parent_rate
passed in. I love that too. But I would like to ask the same thing
on .round_rate and .set_rate as well for the same reason why we have
it for .recalc_rate.
In my case, for most clocks, set rate involves reparenting. So, what does
passing parent_rate for these even mean? Passing parent_rate seems more
apt for recalc_rate since it's called when the parent rate changes -- so,
the actual parent itself is not expected to change.
I could ignore the parameter, but just wondering how many of the others
see value in this. And if we do add this parameter, it shouldn't be made
mandatory for the platform driver to use it (due to other assumptions the
clock framework might make).
-Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/20/2012 04:53 PM, Turquette, Mike wrote:
On Tue, Mar 20, 2012 at 10:46 AM, Saravana Kannan
skan...@codeaurora.org wrote:
On Tue, March 20, 2012 7:02 am, Shawn Guo wrote:
On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote:
...
+struct clk_ops {
+int (*prepare)(struct clk_hw *hw);
+void(*unprepare)(struct clk_hw *hw);
+int (*enable)(struct clk_hw *hw);
+void(*disable)(struct clk_hw *hw);
+int (*is_enabled)(struct clk_hw *hw);
+unsigned long (*recalc_rate)(struct clk_hw *hw,
+unsigned long parent_rate);
I believe I have heard people love the interface with parent_rate
passed in. I love that too. But I would like to ask the same thing
on .round_rate and .set_rate as well for the same reason why we have
it for .recalc_rate.
In my case, for most clocks, set rate involves reparenting. So, what does
passing parent_rate for these even mean? Passing parent_rate seems more
apt for recalc_rate since it's called when the parent rate changes -- so,
the actual parent itself is not expected to change.
From my conversations with folks across many platforms, I think that
the way your clock tree expects to change rates is the exception, not
the rule. As such you should just ignore the parent_rate parameter as
it useless to you.
I could ignore the parameter, but just wondering how many of the others
see value in this. And if we do add this parameter, it shouldn't be made
mandatory for the platform driver to use it (due to other assumptions the
clock framework might make).
From my rough census of folks that actually need .set_rate support, I
think that everyone except MSM could benefit from this. Your concept
of clk_set_rate is everyone else's clk_set_parent.
To clarify, MSM's set parent is a subset of steps needed to be done to
finish set parent. So, it's not that we just randomly decided to swap
the meanings of these two functions.
Also, I think don't think the difference in view of set_rate is due to
the difference in the clock trees between platforms with complicated
trees. I think it's more because of who actually decides the rates for
the clock tree. It looks like some platforms pick a top-down approach to
deciding the rates of the clock tre while others pick a bottom-up approach.
Ignoring the new parameter should cause you no harm.
As long as this is guaranteed, I have no problems with Shawn's suggestion.
It does make me
wonder if it would be a good idea to pass in the parent rate for
.set_parent, which is analogous to .set_rate in many ways.
I need to think a bit more about this.
-Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On 03/20/2012 08:15 PM, Nicolas Pitre wrote: On Tue, 20 Mar 2012, Paul Walmsley wrote: We need to indicate in some way that the existing code and API is very likely to change in ways that could involve quite a bit of work for adopters. [...] Anyway. It is okay if we want to have some starter common clock framework in mainline; this is why deferring the merge hasn't been proposed. But the point is that anyone who bases their code or platform on the common clock framework needs to be aware that, to satisfy one of its major use-cases, the behavior and/or API of the common clock code may need to change significantly. Paul, While I understand your concern, please don't forget that the perfect is the enemy of the good. This common clk API has been under development for over *two* years already, with several attempts to merge it. And each previous merge attempt aborted because someone came along at the last minute to do exactly what you are doing i.e. underline all the flaws and call for a redesign. This is becoming a bad joke. We finally have something that the majority of reviewers are happy with and which should cover the majority of existing cases out there. Let's give it a chance, shall we? Otherwise one might ask where were you during those development years if you claim that the behavior and/or API of the common clock code still need to change significantly? Explicitly stating this is not only simple courtesy to its users, many of whom won't have been privy to its development. It also is intended to make the code easier to change once it reaches mainline. The code will be easier to change once it is in mainline, simply due to the fact that you can also change all its users at once. And it is well possible that most users won't have to deal with the same magnitude of complexity as yours, again reducing the scope for resistance to changes. Let's see how things go with the current code and improve it incrementally. Otherwise no one will get involved with this API which is almost just as bad as not having the code merged at all. So, until the API is well-defined and does all that it needs to do for its major users, [...] No, the API simply can't ever be well defined if people don't actually start using it to eventually refine it further. Major users were converted to it, and in most cases The API does all that it needs to do already. Otherwise you'll be stuck in a catch22 situation forever. And APIs in the Linux kernel do change all the time. There is no stable API in the kernel. Extensions will come about eventually, and existing users (simple ones by definition if the current API already meets their needs) should be converted over easily. +1 to the general idea in Nicolas's email. To add a few more thoughts, while I agree with Paul that there is room for improvement in the APIs, I think the difference in opinion comes when we ask the question: When we eventually refine the APIs in the future to be more expressive, do we think that the current APIs can or cannot be made as wrappers around the new more expressive APIs? Absolutes are almost never right, so I can't give an absolute answer. But I'm strongly leaning towards we can as the answer to the question. That combined with the reasons Nicholas mentioned is why I think the APIs should not be marked as experimental in anyway. -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 1/3] Documentation: common clk API
On 03/21/2012 12:56 PM, Mark Brown wrote: On Wed, Mar 21, 2012 at 12:41:41PM -0700, Saravana Kannan wrote: On 03/21/2012 12:33 PM, Tony Lindgren wrote: * Mark Brownbroo...@opensource.wolfsonmicro.com [120321 12:11]: These aren't new APIs, the clock API has been around since forever. I disagree. When I say clock API, I mean the actual functions and their semantics. Not the existence of a header file. The meaning of clk_enable/disable has been changed and they won't work without calling clk_prepare/unprepare. So, these are definitely new APIs. If it weren't new APIs, then none of the general drivers would need to change. clk_prepare() and clk_unprepare() are already there for any clk.h user and are stubbed in no matter what, they're really not a meaningful barrier here. Isn't this whole experimental vs non-experimental discussion about the actual external facing clock APIs and not the platform driver facing APIs? Sure, prepare/unprepare are already there in the .h file. But they are stubs and have no impact till we move to the common clock framework or platforms move to them with their own implementation (certainly not happening in upstream, so let's leave that part out of this discussion). So. IMO, for all practical purposes, common clk fwk forces the move to these new APIs and hence IMO forces the new APIs. -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 1/3] Documentation: common clk API
On 03/21/2012 12:33 PM, Tony Lindgren wrote: * Mark Brownbroo...@opensource.wolfsonmicro.com [120321 12:11]: On Wed, Mar 21, 2012 at 11:38:58AM -0700, Saravana Kannan wrote: So it would be interesting to know more about why you (or anyone else) perceive that the Kconfig changes would be harmful. But the enthusiasm of the clock driver developers doesn't necessarily translate to users of the clock APIs (other driver devs). My worry with marking it as experimental in Kconfig and to a certain extent in the documentation is that it will discourage the driver devs from switching to the new APIs. If no one is using the new APIs, then platforms can't switch to the common clock framework These aren't new APIs, the clock API has been around since forever. I disagree. When I say clock API, I mean the actual functions and their semantics. Not the existence of a header file. The meaning of clk_enable/disable has been changed and they won't work without calling clk_prepare/unprepare. So, these are definitely new APIs. If it weren't new APIs, then none of the general drivers would need to change. For driver authors working on anything that isn't platform specific the issue has been that it's not implemented at all on the overwhelming majority of architectures and those that do all have their own random implementations with their own random quirks and with no ability for anything except the platform to even try to do incredibly basic stuff like register a new clock. Simply having something, anything, in place even if it's going to churn is a massive step forward here for people working with clocks. Right, and now at least the people reading this thread are pretty aware of the yet unsolved issues with clock fwk api :) :-) Shhh... not so loud! -Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/19/2012 12:33 PM, Turquette, Mike wrote: On Mon, Mar 19, 2012 at 12:13 PM, Saravana Kannan skan...@codeaurora.org wrote: I saw some responses from you over the weekend but not to mine. So, I assumed you were busy with other stuff and I started working on a patch on top of v7. I only answer trivial emails on the weekend ;-) I will send that out if I get around to finishing it before you do. Hope that's alright with you. I'm happy to for you to take a crack at it. I don't know what your implementation looks like, but here are a couple concerns I have: 1) if you're copying the data from the initializer over to the struct clk then make sure you handle the __init data aspects of it properly Not copying them into clk again. Just leaving them in clk_hw and using them as is. The only fields moved into clk_hw are: + const char *name; + const struct clk_ops*ops; + char**parent_names; + u8 num_parents; + unsigned long flags; 2) are the members of struct clk_hw visible to the rest of the world? Are they modifiable (i.e., not const)? This is undesirable. I'm leaving the const as is. Some of them you can't mark as const if you want to dynamically create them. Yes, they are visible to all the platform drivers (not the rest of the world). But these values are provided by the platform drivers anyway, so I don't see a problem with it. I also don't see any useful hacks a platform driver can do by messing with this fields without crashing the kernel since they don't have access to the locks. flags might be the one that provides some possibilities since I think you look at them often in the core code. We could just copy it into clk if people really feel strongly about it. At the worst case, we can have a full copy of all these fields inside clk and copy all of them over, but I think that would be overkill for things like names, ops and parent info. Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/19/2012 11:56 AM, Turquette, Mike wrote: On Fri, Mar 16, 2012 at 8:28 PM, Saravana Kannanskan...@codeaurora.org wrote: On 03/15/2012 11:11 PM, Mike Turquette wrote: The common clock framework defines a common struct clk useful across most platforms as well as an implementation of the clk api that drivers can use safely for managing clocks. The net result is consolidation of many different struct clk definitions and platform-specific clock framework implementations. This patch introduces the common struct clk, struct clk_ops and an implementation of the well-known clock api in include/clk/clk.h. Platforms may define their own hardware-specific clock structure and their own clock operation callbacks, so long as it wraps an instance of struct clk_hw. See Documentation/clk.txt for more details. This patch is based on the work of Jeremy Kerr, which in turn was based on the work of Ben Herrenschmidt. Signed-off-by: Mike Turquettemturque...@linaro.org Signed-off-by: Mike Turquettemturque...@ti.com Reviewed-by: Thomas Gleixnert...@linutronix.de Tested-by: Andrew Lunnand...@lunn.ch Reviewed-by: Rob Herringrob.herringatcalxeda.com Cc: Russell Kingli...@arm.linux.org.uk Cc: Jeremy Kerrjeremy.k...@canonical.com Cc: Arnd Bergmanarnd.bergm...@linaro.org Cc: Paul Walmsleyp...@pwsan.com Cc: Shawn Guoshawn@freescale.com Cc: Sascha Hauers.ha...@pengutronix.de Cc: Richard Zhaorichard.z...@linaro.org Cc: Saravana Kannanskan...@codeaurora.org Cc: Magnus Dammmagnus.d...@gmail.com Cc: Mark Brownbroo...@opensource.wolfsonmicro.com Cc: Linus Walleijlinus.wall...@stericsson.com Cc: Stephen Boydsb...@codeaurora.org Cc: Amit Kucheriaamit.kuche...@linaro.org Cc: Deepak Saxenadsax...@linaro.org Cc: Grant Likelygrant.lik...@secretlab.ca --- Mike, Thanks for the patches! Glad to see that it's finally getting in! I sent a request for a minor change as a reply to the v5 series (since it had more context). Can you please take a look at that and let me know if you can send out a v8 or a patch on top of this to do that? Hi Saravana, Hi Mike, I'm not sending a v8 series since Arnd has taken in v7 for the 3.4 merge window. Yeah, I later realized it might be better to send patches on top of v7. I'm formulating a reply to your v5 queries, but I'm not done looking at the implications of the initializer stuff. Lets keep the technical discussion in that thread for now. I saw some responses from you over the weekend but not to mine. So, I assumed you were busy with other stuff and I started working on a patch on top of v7. I will send that out if I get around to finishing it before you do. Hope that's alright with you. Based on what I have done so far, to me it just looked like a search and replace of clk-name with clk-hw.name and similar changes for the rest of the fields. It looks like we might be able to remove clk_register_mux, clk_register_divider, etc if we go with this. No stong opinion about removing those, but they seemed redundant after the suggester refactor. I think it would be okay to just move those init fields into clk_hw and not bother with renaming it to clk_initializer (I would prefer, clk_info or clk_common) if it makes the match much less invasive. Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 03/15/2012 11:11 PM, Mike Turquette wrote: The common clock framework defines a common struct clk useful across most platforms as well as an implementation of the clk api that drivers can use safely for managing clocks. The net result is consolidation of many different struct clk definitions and platform-specific clock framework implementations. This patch introduces the common struct clk, struct clk_ops and an implementation of the well-known clock api in include/clk/clk.h. Platforms may define their own hardware-specific clock structure and their own clock operation callbacks, so long as it wraps an instance of struct clk_hw. See Documentation/clk.txt for more details. This patch is based on the work of Jeremy Kerr, which in turn was based on the work of Ben Herrenschmidt. Signed-off-by: Mike Turquettemturque...@linaro.org Signed-off-by: Mike Turquettemturque...@ti.com Reviewed-by: Thomas Gleixnert...@linutronix.de Tested-by: Andrew Lunnand...@lunn.ch Reviewed-by: Rob Herringrob.herringat calxeda.com Cc: Russell Kingli...@arm.linux.org.uk Cc: Jeremy Kerrjeremy.k...@canonical.com Cc: Arnd Bergmanarnd.bergm...@linaro.org Cc: Paul Walmsleyp...@pwsan.com Cc: Shawn Guoshawn@freescale.com Cc: Sascha Hauers.ha...@pengutronix.de Cc: Richard Zhaorichard.z...@linaro.org Cc: Saravana Kannanskan...@codeaurora.org Cc: Magnus Dammmagnus.d...@gmail.com Cc: Mark Brownbroo...@opensource.wolfsonmicro.com Cc: Linus Walleijlinus.wall...@stericsson.com Cc: Stephen Boydsb...@codeaurora.org Cc: Amit Kucheriaamit.kuche...@linaro.org Cc: Deepak Saxenadsax...@linaro.org Cc: Grant Likelygrant.lik...@secretlab.ca --- Mike, Thanks for the patches! Glad to see that it's finally getting in! I sent a request for a minor change as a reply to the v5 series (since it had more context). Can you please take a look at that and let me know if you can send out a v8 or a patch on top of this to do that? Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On 03/16/2012 05:54 PM, Rob Herring wrote: On 03/16/2012 06:47 PM, Sascha Hauer wrote: Hi Paul, On Fri, Mar 16, 2012 at 04:21:17PM -0600, Paul Walmsley wrote: Hi On Fri, 16 Mar 2012, Arnd Bergmann wrote: If the common clock code is to go upstream now, it should be marked as experimental. No, please don't do this. This effectively marks the architectures using the generic clock framework experimental. We can mark drivers as 'you have been warned', but marking an architecture as experimental is the wrong sign for both its users and people willing to adopt the framework. Also we get this: warning: (ARCH_MX1 MACH_MX21 ARCH_MX25 MACH_MX27) selects COMMON_CLK which has unmet direct dependencies (EXPERIMENTAL) (and no, I don't want to support to clock frameworks in parallel) +1 For simple users at least, the api is perfectly adequate and it is not experimental (unless new means experimental). +1 - not experimental please. While I agree with Mike and Paul that there is room for a lot of improvements to the clock APIs, I think the existing APIs in this patch series can become macro wrappers around any new APIs improvements we add in the future. We should have really done that for the clk_prepare_enable/unprepare_disable(), but it should certainly be doable for future API additions now that we have the atomic/non-atomic differentiation out of the way. 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 v5 3/4] clk: introduce the common clock framework
On 03/07/2012 01:20 PM, Turquette, Mike wrote:
On Tue, Mar 6, 2012 at 11:00 AM, Sascha Hauers.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 Hauers.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 intenable_count;
unsigned intprepare_count;
struct hlist_head children;
struct hlist_node child_node;
unsigned intnotifier_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 for connecting discrete devices.
Yes, I've seen this and I really like it. Also the change that
multiplexers return an index to an
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On 12/17/2011 03:04 AM, Russell King - ARM Linux wrote:
On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote:
On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixnert...@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() ?
No it should not.
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);
So this is basically buggy. Look, it's quite simple. Convert _all_
your drivers to clk_prepare/clk_unprepare _before_ you start switching
your platform to use these new functions. You can do that _today_
without exception.
We must refuse to merge _any_ user which does this the old way - and
we should have been doing this since my commit was merged into mainline
to allow drivers to be converted.
And stop trying to think of ways around this inside clk_prepare/
clk_unprepare/clk_enable/clk_disable. You can't do it. Just fix _all_
the drivers. Now. Before you start implementing clk_prepare/clk_unprepare.
I agree with Russell's suggestion. This is what I'm trying to do with
the MSM platform. Not sure if I'm too optimistic, but as of today, I'm
still optimistic I can push the MSM driver devs to get this done before
we enable real prepare/unprepare support.
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 v4 3/6] clk: introduce the common clock framework
On 01/12/2012 04:48 PM, Rob Herring wrote:
On 01/12/2012 06:04 PM, Saravana Kannan wrote:
On 01/04/2012 08:07 PM, Turquette, Mike wrote:
On Wed, Jan 4, 2012 at 6:11 PM, Rob Herringrobherri...@gmail.com
wrote:
On 01/04/2012 07:01 PM, Turquette, Mike wrote:
On Wed, Jan 4, 2012 at 6:32 AM, Rob Herringrobherri...@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
Gleixnert...@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_hwhw;
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
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On 01/04/2012 08:07 PM, Turquette, Mike wrote:
On Wed, Jan 4, 2012 at 6:11 PM, Rob Herringrobherri...@gmail.com wrote:
On 01/04/2012 07:01 PM, Turquette, Mike wrote:
On Wed, Jan 4, 2012 at 6:32 AM, Rob Herringrobherri...@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 Gleixnert...@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_hwhw;
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).
While the original clk_hw suggestion
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On 11/21/2011 05:40 PM, Mike Turquette wrote:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
new file mode 100644
index 000..12c9994
--- /dev/null
+++ b/drivers/clk/clk.c
@@ -0,0 +1,538 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltdjeremy.k...@canonical.com
+ * Copyright (C) 2011 Linaro Ltdmturque...@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
+ */
+
+#includelinux/clk.h
+#includelinux/module.h
+#includelinux/mutex.h
+#includelinux/slab.h
+#includelinux/spinlock.h
+#includelinux/err.h
+
+static DEFINE_SPINLOCK(enable_lock);
+static DEFINE_MUTEX(prepare_lock);
+
+void __clk_unprepare(struct clk *clk)
+{
+ if (!clk)
+ return;
+
+ if (WARN_ON(clk-prepare_count == 0))
+ return;
+
+ if (--clk-prepare_count 0)
Space before ?
+ return;
+
+ WARN_ON(clk-enable_count 0);
The spacing comment applies to all such instances the follow too.
+
+ if (clk-ops-unprepare)
+ clk-ops-unprepare(clk);
+
+ __clk_unprepare(clk-parent);
+}
+
+/**
+ * clk_unprepare - perform the slow part of a clk gate
+ * @clk: the clk being gated
+ *
+ * clk_unprepare may sleep, which differentiates it from clk_disable. In a
+ * simple case, clk_unprepare can be used instead of clk_disable to gate a clk
+ * if the operation may sleep. One example is a clk which is accessed over
+ * I2c. In the complex case a clk gate operation may require a fast and a slow
+ * part. It is this reason that clk_unprepare and clk_disable are not mutually
+ * exclusive. In fact clk_disable must be called before clk_unprepare.
+ */
+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;
+}
+
+/**
+ * clk_prepare - perform the slow part of a clk ungate
+ * @clk: the clk being ungated
+ *
+ * clk_prepare may sleep, which differentiates it from clk_enable. In a simple
+ * case, clk_prepare can be used instead of clk_enable to ungate a clk if the
+ * operation may sleep. One example is a clk which is accessed over I2c. In
+ * the complex case a clk ungate operation may require a fast and a slow part.
+ * It is this reason that clk_prepare and clk_enable are not mutually
+ * exclusive. In fact clk_prepare must be called before clk_enable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+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);
+}
+
+/**
+ * clk_disable - perform the fast part of a clk gate
+ * @clk: the clk being gated
+ *
+ * clk_disable must not sleep, which differentiates it from clk_unprepare. In
+ * a simple case, clk_disable can be used instead of clk_unprepare to gate a
+ * clk if the operation is fast and will never sleep. One example is a
+ * SoC-internal clk which is controlled via simple register writes. In the
+ * complex case a clk gate operation may require a fast and a slow part. It is
+ * this reason that clk_unprepare and clk_disable are not mutually exclusive.
+ * In fact clk_disable must be called before clk_unprepare.
+ */
+void clk_disable(struct clk *clk)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(enable_lock, flags);
+ __clk_disable(clk);
+ spin_unlock_irqrestore(enable_lock, flags);
+}
+EXPORT_SYMBOL_GPL(clk_disable);
+
+int __clk_enable(struct clk *clk)
+{
+ int ret = 0;
+
+ if (!clk)
+ return 0;
+
+ if (WARN_ON(clk-prepare_count == 0))
+ return -ESHUTDOWN;
EPERM? EBADFD?
+
+ if (clk-enable_count == 0) {
+ if (clk-parent)
+ ret =
Re: [PATCH v3 2/5] Documentation: common clk API
On 11/21/2011 05:40 PM, Mike Turquette wrote:
Provide documentation for the common clk structures and APIs. This code
can be found in drivers/clk/ and include/linux/clk.h.
Signed-off-by: Mike Turquettemturque...@linaro.org
---
Documentation/clk.txt | 312 +
1 files changed, 312 insertions(+), 0 deletions(-)
create mode 100644 Documentation/clk.txt
diff --git a/Documentation/clk.txt b/Documentation/clk.txt
new file mode 100644
index 000..ef4333d
--- /dev/null
+++ b/Documentation/clk.txt
@@ -0,0 +1,312 @@
+ The Common Clk Framework
+ Mike Turquettemturque...@ti.com
+
+ Part 1 - common data structures and API
+
+The common clk framework is a combination of a common definition of
+struct clk which can be used across most platforms as well as a set of
+driver-facing APIs which operate on those clks. Platforms can enable it
+by selecting CONFIG_GENERIC_CLK.
+
+Below is the common struct clk definition from include/linux.clk.h. It
Typo
+is modified slightly for brevity:
+
+struct clk {
+ const char *name;
+ const struct clk_hw_ops *ops;
No strong opinion, but can we call this clk_ops for brevity?
+ struct clk *parent;
+ unsigned long rate;
+ unsigned long flags;
+ unsigned intenable_count;
+ unsigned intprepare_count;
+ struct hlist_head children;
+ struct hlist_node child_node;
+};
+
+The .name, .parent and .children members make up the core of the clk
+tree topology which can be visualized by enabling
+CONFIG_COMMON_CLK_SYSFS. The ops member points to an instance of struct
+clk_hw_ops:
+
+ 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,
+ 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);
+ };
+
+These callbacks correspond to the clk API that has existed in
+include/linux/clk.h for a while. Below is a quick summary of the
+operations in that header, as implemented in drivers/clk/clk.c. These
+comprise the driver-facing API:
+
+clk_prepare - does everything needed to get a clock ready to generate a
+proper signal which may include ungating the clk and actually generating
+that signal. clk_prepare MUST be called before clk_enable. This call
+holds the global prepare_mutex, which also prevents clk rates and
+topology from changing while held. This API is meant to be the slow
+part of a clk enable sequence, if applicable. This function must not be
+called in an interrupt context.
in an atomic context?
+
+clk_unprepare - the opposite of clk_prepare: does everything needed to
+make a clk no longer ready to generate a proper signal, which may
+include gating an active clk. clk_disable must be called before
+clk_unprepare. All of the same rules for clk_prepare apply.
+
+clk_enable - ungate a clock and immediately start generating a valid clk
+signal. This is the fast part of a clk enable sequence, and maybe the
+only functional part of that sequence. Regardless clk_prepare must be
+called BEFORE clk_enable. The enable_spinlock is held across this call,
+which means that clk_enable must not sleep.
+
+clk_disable - the opposite of clk_enable: gates a clock immediately.
+clk_disable must be called before calling clk_unprepare. All of the
+same rules for clk_enable apply.
+
+clk_get_rate - Returns the cached rate for the clk. Does NOT query the
+hardware state. No lock is held.
I wrote the stuff below and then realized that this ops is not really
present in the code. Looks like stale doc. Can you please remove this?
But I think the comments below do hold true for the actual
clk_set_rate()/get_rate() implementation. I will try to repeat this as
part of the actual code review.
I will be looking into the other patches in order, so, forgive me if I'm
asking a question that has an obvious answer in the remaining patches.
I think a lock needs to be taken for this call too. What prevents a
clock set rate from getting called and modifying the cached rate
variable while it's bring read. I don't think we should have a common
code assume that read/write of longs are atomic.
+
+clk_get_parent - Returns the cached parent for the clk. Does NOT query
+the hardware state. No lock is held.
Same question as above. Can we assume a read of a pointer variable is
atomic? I'm not sure. I
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On 09/22/2011 03:26 PM, Mike Turquette wrote:
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..d6ae10b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
+#ifdef CONFIG_GENERIC_CLK
+
+struct clk_hw {
+ struct clk *clk;
+};
+
+/**
+ * struct clk_hw_ops - Callback operations for hardware clocks; these are to
+ * be provided by the clock implementation, and will be called by drivers
+ * through the clk_* API.
+ *
+ * @prepare: Prepare the clock for enabling. This must not return until
+ * the clock is fully prepared, and it's safe to call clk_enable.
+ * This callback is intended to allow clock implementations to
+ * do any initialisation that may sleep. Called with
+ * prepare_lock held.
+ *
+ * @unprepare: Release the clock from its prepared state. This will typically
+ * undo any work done in the @prepare callback. Called with
+ * prepare_lock held.
+ *
+ * @enable:Enable the clock atomically. This must not return until the
+ * clock is generating a valid clock signal, usable by consumer
+ * devices. Called with enable_lock held. This function must not
+ * sleep.
+ *
+ * @disable: Disable the clock atomically. Called with enable_lock held.
+ * This function must not sleep.
+ *
+ * @recalc_rateRecalculate the rate of this clock, by quering hardware
+ * and/or the clock's parent. Called with the global clock mutex
+ * held. Optional, but recommended - if this op is not set,
+ * clk_get_rate will return 0.
+ *
+ * @get_parent Query the parent of this clock; for clocks with multiple
+ * possible parents, query the hardware for the current
+ * parent. Currently only called when the clock is first
+ * registered.
+ *
+ * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
+ * implementations to split any work between atomic (enable) and sleepable
+ * (prepare) contexts. If a clock requires sleeping code to be turned on, this
+ * should be done in clk_prepare. Switching that will not sleep should be done
+ * in clk_enable.
+ *
+ * Typically, drivers will call clk_prepare when a clock may be needed later
+ * (eg. when a device is opened), and clk_enable when the clock is actually
+ * required (eg. from an interrupt). Note that clk_prepare *must* have been
+ * called before clk_enable.
*/
+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 *);
+ 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.
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 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.
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 4/7] clk: Add simple gated clock
On 09/26/2011 04:30 PM, Rob Herring wrote:
On 09/26/2011 05:37 PM, Turquette, Mike wrote:
On Mon, Sep 26, 2011 at 12:37 PM, Jamie Ilesja...@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
By definition of simple gated clock, the other bits have to be for
another clock. The restriction is that all the other bits can only be
clock gate bits.
Rob, do you feel these assumptions are OK and locking can remain the
same in this patch?
Perhaps it is rare enough that it is not worth it use generic code in
this case. If so, the documentation should be clear about this
constraint. It is not something anyone will have hit before because
everyone used a single global lock. Now with the api being split between
2 locks, this adds a new complexity.
I kinda agree with Rob on this. There are very few, if any, such simple
clocks on MSM chips. It's very easy to a SoC clock developer to
accidentally use these simple clocks without realizing the point that
Rob brings up.
I think the simple gated clock code should be usable for any clock
controlled by a single bit in a 32-bit register independent of other
things in that register.
To take care of the scenario Rob bring up, the prepare/unprepare and
enable/disable code will have to grab a per-tree register-lock before
accessing any registers. The prepare/unprepare code should obviously be
written to hold this register-lock for as small of a duration as
possible. For example, if the prepare code is doing voltage increase,
the register-lock should be grabber _after_ the voltage is increased. At
least, this is approximately how the MSM clock code can be mapped onto
this generic framework.
I think we should just go ahead and implement the per-tree register lock
so that the generic clock implementations are more useful. The lock will
really be held only for a very short time and hence shouldn't matter
that there is a single lock for all the clocks in a tree.
Thomas,
Did you get a chance to send out your patches with support for per-tree
locking? I would really like to see that as part of the first patch
series that gets pulled in.
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
