Re: [PATCH v3 2/5] Documentation: common clk API
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:
[...]
+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?
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 do not think it's necessary to encode 'hw' in naming of clk_hw_fixed,
clk_hw_gate and any clocks wrapping 'struct clk' in there. For
example, naming like clk_dummy, clk_imx seems brevity and clear,
and we do not need clk_hw_dummy and clk_hw_imx necessarily.
[...]
+
+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.
+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
Is this really true? Based on a quick glance at the other patches, it
doesn't look it disallows set rate if the clock is enabled. Which is
correct, because clock rates can be change while they are enabled (I'm
referring to leaf clocks) as long as the hardware supports doing it
correctly. So, this needs rewording? I'm guessing you are trying to say
that you can't change the parent rate if it has multiple enabled child
clocks running off of it and one of them wants to cause a parent rate
change? I'm not sure if even that should be enforced in the common code --
doesn't look like you do either.
Same as my answer above. There is a flag which allows you to control
this behavior.
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?
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.
+2) using clk rate change notifiers to allow devices to handle dynamic
Must be 3)
Haha good catch.
+rate changes for clks which do support changing rates while enabled
Again, I guess this applies to the other clock. Not the one for which
clk_set_rate() is being called.
This applies to ANY clk which has the flag set and is called by
__clk_set_rate (which may be called many times in a recursive path).
+clk_set_rate(C, 26MHz);
+ __clk_set_rate(C, 26MHz);
+ clk-round_rate(C, 26MHz, *parent_rate);
+ [ round_rate returns 50MHz ]
+ [parent_rate is 52MHz ]
+ clk-set_rate(C, 50Mhz);
+ [ clk C is set to 50MHz, which sets divider to 2 ]
+ __clk_set_rate(clk-parent, parent_rate);
+ clk-round_rate(B, 52MHz, *parent_rate);
+ [ round_rate returns 100MHz ]
+ [parent_rate is 104MHz ]
+ clk-set_rate(B, 100MHz);
+ [ clk B stays at 100MHz, divider stays at 2 ]
+ __clk_set_rate(clk-parent, parent_rate);
+ [ round_rate returns 104MHz ]
+ [parent_rate is ignored ]
+
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 2/5] Documentation: common clk API
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:
+Below is the common struct clk definition from include/linux.clk.h. It
Typo
Will fix in V4.
+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?
I prefer clk_hw_ops, as it clearly delineates that these operations
know hardware-specific details.
+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?
Will fix in V4.
+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.
Firstly this is a summary of the clk API in clk.h, not clk_hw_ops.
There isn't a hardware op for this since we just return clk-parent.
Secondly it is up to the caller to hold a lock. Any code calling
clk_get_rate might likely want to hold that lock anyways. I'll update
the comments to be explicit about this.
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 think this needs locking too.
Same answer as above. The caller must hold the lock. I'll update the comments.
+
+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.
+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
Is this really true? Based on a quick glance at the other patches, it doesn't
look it disallows set rate if the clock is enabled. Which is correct, because
clock rates can be change while they are enabled (I'm referring to leaf
clocks) as long as the hardware supports doing it correctly. So, this needs
rewording? I'm guessing you are trying to say that you can't change the
parent rate if it has multiple enabled child clocks running off of it and one
of them wants to cause a parent rate change? I'm not sure if even that should
be enforced in the common code -- doesn't look like you do either.
Same as my answer above. There is a flag which allows you to control
this behavior.
+2) using clk rate change notifiers to allow devices to handle dynamic
Must be 3)
Haha good catch.
+rate changes for clks which do support changing rates while enabled
Again, I guess this applies to the other clock. Not the one for which
clk_set_rate() is being called.
This applies to ANY clk which has the flag set and is called by
__clk_set_rate (which may be called many times in a recursive path).
+clk_set_rate(C, 26MHz);
+ __clk_set_rate(C, 26MHz);
+ clk-round_rate(C, 26MHz, *parent_rate);
+
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
[PATCH v3 2/5] Documentation: common clk API
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 Turquette mturque...@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 Turquette mturque...@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
+is modified slightly for brevity:
+
+struct clk {
+ const char *name;
+ const struct clk_hw_ops *ops;
+ 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.
+
+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.
+
+clk_get_parent - Returns the cached parent for the clk. Does NOT query
+the hardware state. No lock is held.
+
+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. Holds the
+same prepare_mutex held by clk_prepare/clk_unprepare and clk_set_parent.
+
+clk_set_parent - Switches the input source for a clk. This only applies
+to mux clks with multiple parents. Holds the same prepare_mutex held by
+clk_prepare/clk_unprepare and clk_set_rate.
+
+ Part 2 - hardware clk implementations
+
+The strength of the common struct clk comes from its .ops pointer and
+the ability for platform and driver code to wrap the struct clk instance
+with hardware-specific data which the operations in the .ops pointer
+have knowledge of. To illustrate consider the simple gateable clk
+implementation in drivers/clk/clk-basic.c:
+
+struct clk_hw_gate {
+ struct clk clk;
+ struct clk *fixed_parent;
+
