Add the below four notifier events so drivers which are interested in
knowing the clock status can act accordingly. This is extremely useful
in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
PRE_CLK_ENABLE
POST_CLK_ENABLE
PRE_CLK_DISABLE
POST_CLK_DISABLE
Signed-off-by: Bill Huang <[email protected]>
---
drivers/clk/clk.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/clk.h | 40 ++++++++++++++++++++---------------
2 files changed, 81 insertions(+), 17 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ed87b24..720a16a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1104,6 +1104,64 @@ struct clk *clk_get_parent(struct clk *clk)
}
EXPORT_SYMBOL_GPL(clk_get_parent);
+/**
+ * clk_prepare_enable - call clk_enable in non-atomic context.
+ * @clk: the clk to enable
+ *
+ * Send PRE_CLK_ENABLE/POST_CLK_ENABLE before/after calling
+ * clk_enalbe respectively
+ *
+ * Return success (0) or errno.
+ */
+int clk_prepare_enable(struct clk *clk)
+{
+ int ret;
+
+ mutex_lock(&prepare_lock);
+ ret = __clk_prepare(clk);
+ if (ret)
+ return ret;
+
+ if (clk->notifier_count)
+ __clk_notify(clk, PRE_CLK_ENABLE, clk->rate, clk->rate);
+
+ ret = clk_enable(clk);
+ if (ret)
+ __clk_unprepare(clk);
+
+ if (clk->notifier_count)
+ __clk_notify(clk, POST_CLK_ENABLE, clk->rate, clk->rate);
+
+ mutex_unlock(&prepare_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_prepare_enable);
+
+/**
+ * clk_disable_unprepare - call clk_disable in non-atomic context.
+ * @clk: the clk to enable
+ *
+ * Send PRE_CLK_DISABLE/POST_CLK_DISABLE before/after calling
+ * clk_disalbe respectively
+ */
+void clk_disable_unprepare(struct clk *clk)
+{
+ mutex_lock(&prepare_lock);
+
+ if (clk->notifier_count)
+ __clk_notify(clk, PRE_CLK_DISABLE, clk->rate, clk->rate);
+
+ clk_disable(clk);
+ __clk_unprepare(clk);
+
+ if (clk->notifier_count)
+ __clk_notify(clk, POST_CLK_DISABLE, clk->rate, clk->rate);
+
+ mutex_unlock(&prepare_lock);
+}
+EXPORT_SYMBOL_GPL(clk_disable_unprepare);
+
/*
* .get_parent is mandatory for clocks with multiple possible parents. It is
* optional for single-parent clocks. Always call .get_parent if it is
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b3ac22d..4bbe1903 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -43,6 +43,10 @@ struct clk;
#define PRE_RATE_CHANGE BIT(0)
#define POST_RATE_CHANGE BIT(1)
#define ABORT_RATE_CHANGE BIT(2)
+#define PRE_CLK_ENABLE BIT(3)
+#define POST_CLK_ENABLE BIT(4)
+#define PRE_CLK_DISABLE BIT(5)
+#define POST_CLK_DISABLE BIT(6)
/**
* struct clk_notifier - associate a clk with a notifier
@@ -276,6 +280,20 @@ struct clk *clk_get_parent(struct clk *clk);
*/
struct clk *clk_get_sys(const char *dev_id, const char *con_id);
+/**
+ * clk_prepare_enable - helps cases using clk_enable in non-atomic context.
+ * @clk: clock source
+ *
+ * Return success (0) or nagative errno.
+ */
+int clk_prepare_enable(struct clk *clk);
+
+/**
+ * clk_disable_unprepare - helps cases using clk_disable in non-atomic context.
+ * @clk: clock source
+ */
+void clk_disable_unprepare(struct clk *clk);
+
#else /* !CONFIG_HAVE_CLK */
static inline struct clk *clk_get(struct device *dev, const char *id)
@@ -324,30 +342,18 @@ static inline struct clk *clk_get_parent(struct clk *clk)
return NULL;
}
-#endif
-
-/* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
static inline int clk_prepare_enable(struct clk *clk)
{
- int ret;
-
- ret = clk_prepare(clk);
- if (ret)
- return ret;
- ret = clk_enable(clk);
- if (ret)
- clk_unprepare(clk);
-
- return ret;
+ return 0;
}
-/* clk_disable_unprepare helps cases using clk_disable in non-atomic context. */
-static inline void clk_disable_unprepare(struct clk *clk)
+static inline int clk_disable_unprepare(struct clk *clk)
{
- clk_disable(clk);
- clk_unprepare(clk);
+ return 0;
}
+#endif
+
/**
* clk_add_alias - add a new clock alias
* @alias: name for clock alias
--
1.7.9.5
On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
> Add the below four notifier events so drivers which are interested in
> knowing the clock status can act accordingly. This is extremely useful
> in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
>
> PRE_CLK_ENABLE
> POST_CLK_ENABLE
> PRE_CLK_DISABLE
> POST_CLK_DISABLE
>
> Signed-off-by: Bill Huang <[email protected]>
NAK. *Sigh* NO, this is the wrong level to be doing stuff like this.
The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should
*EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and
clk_unprepare(). Those two functions are *merely* helpers for drivers
who don't wish to make the individual calls.
Drivers are still completely free to call the individual functions, at
which point your proposal breaks horribly - and they _do_ call the
individual functions.
On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
> > Add the below four notifier events so drivers which are interested in
> > knowing the clock status can act accordingly. This is extremely useful
> > in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
> >
> > PRE_CLK_ENABLE
> > POST_CLK_ENABLE
> > PRE_CLK_DISABLE
> > POST_CLK_DISABLE
> >
> > Signed-off-by: Bill Huang <[email protected]>
>
> NAK. *Sigh* NO, this is the wrong level to be doing stuff like this.
>
> The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should
> *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and
> clk_unprepare(). Those two functions are *merely* helpers for drivers
> who don't wish to make the individual calls.
>
> Drivers are still completely free to call the individual functions, at
> which point your proposal breaks horribly - and they _do_ call the
> individual functions.
I'm proposing to give device driver a choice when it knows that some
driver might be interested in knowing its clock's enabled/disabled state
change at runtime, this is very important for centralized DVFS core
driver. It is not meant to be covering all cases especially for drivers
which is not part of the DVFS, so we don't care if it is calling
clk_enable/disable directly or not.
One major side effect being that it affects those existing drivers who
were calling clk_prepare_enable and clk_prepare_disable but it should
not hurt if they don't care the notifier, or maybe we should define a
new set of functions for the purpose?
Mike, how do you think? Thanks.
On 03/12/2013 07:47 PM, Bill Huang wrote:
> On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
>> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
>>> Add the below four notifier events so drivers which are interested in
>>> knowing the clock status can act accordingly. This is extremely useful
>>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
>>>
>>> PRE_CLK_ENABLE
>>> POST_CLK_ENABLE
>>> PRE_CLK_DISABLE
>>> POST_CLK_DISABLE
>>>
>>> Signed-off-by: Bill Huang <[email protected]>
>>
>> NAK. *Sigh* NO, this is the wrong level to be doing stuff like this.
>>
>> The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should
>> *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and
>> clk_unprepare(). Those two functions are *merely* helpers for drivers
>> who don't wish to make the individual calls.
>>
>> Drivers are still completely free to call the individual functions, at
>> which point your proposal breaks horribly - and they _do_ call the
>> individual functions.
>
> I'm proposing to give device driver a choice when it knows that some
> driver might be interested in knowing its clock's enabled/disabled state
> change at runtime, this is very important for centralized DVFS core
> driver. It is not meant to be covering all cases especially for drivers
> which is not part of the DVFS, so we don't care if it is calling
> clk_enable/disable directly or not.
I believe the point Russell is making is not that the idea behind this
patch is wrong, but simply that the function where you put the hooks is
wrong. The hooks should at least be in clk_enable/clk_disable and not
clk_prepare_enable/clk_disable_unprepare, since any driver is free to
call clk_prepare separately from clk_enable. The hooks should be
implemented in the lowest-level common function that all
driver-accessible paths call through.
On Wed, 2013-03-13 at 12:42 +0800, Stephen Warren wrote:
> On 03/12/2013 07:47 PM, Bill Huang wrote:
> > On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
> >> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
> >>> Add the below four notifier events so drivers which are interested in
> >>> knowing the clock status can act accordingly. This is extremely useful
> >>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
> >>>
> >>> PRE_CLK_ENABLE
> >>> POST_CLK_ENABLE
> >>> PRE_CLK_DISABLE
> >>> POST_CLK_DISABLE
> >>>
> >>> Signed-off-by: Bill Huang <[email protected]>
> >>
> >> NAK. *Sigh* NO, this is the wrong level to be doing stuff like this.
> >>
> >> The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should
> >> *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and
> >> clk_unprepare(). Those two functions are *merely* helpers for drivers
> >> who don't wish to make the individual calls.
> >>
> >> Drivers are still completely free to call the individual functions, at
> >> which point your proposal breaks horribly - and they _do_ call the
> >> individual functions.
> >
> > I'm proposing to give device driver a choice when it knows that some
> > driver might be interested in knowing its clock's enabled/disabled state
> > change at runtime, this is very important for centralized DVFS core
> > driver. It is not meant to be covering all cases especially for drivers
> > which is not part of the DVFS, so we don't care if it is calling
> > clk_enable/disable directly or not.
>
> I believe the point Russell is making is not that the idea behind this
> patch is wrong, but simply that the function where you put the hooks is
> wrong. The hooks should at least be in clk_enable/clk_disable and not
> clk_prepare_enable/clk_disable_unprepare, since any driver is free to
> call clk_prepare separately from clk_enable. The hooks should be
> implemented in the lowest-level common function that all
> driver-accessible paths call through.
Thanks, I know the point, but unfortunately there is no good choice for
hooking this since those low level functions clk_enable/clk_disable will
be called in interrupt context so it is not possible to send notify. We
might need to come out a better approach if we can think of any.
Currently I still think this is acceptable (Having all the drivers which
are using our interested clocks call these function to enable/disable
clock in their runtime_pm calls) though it's not perfect.
On 03/12/2013 11:08 PM, Bill Huang wrote:
> On Wed, 2013-03-13 at 12:42 +0800, Stephen Warren wrote:
>> On 03/12/2013 07:47 PM, Bill Huang wrote:
>>> On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
>>>> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
>>>>> Add the below four notifier events so drivers which are interested in
>>>>> knowing the clock status can act accordingly. This is extremely useful
>>>>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
>>>>>
>>>>> PRE_CLK_ENABLE
>>>>> POST_CLK_ENABLE
>>>>> PRE_CLK_DISABLE
>>>>> POST_CLK_DISABLE
>>>>>
>>>>> Signed-off-by: Bill Huang <[email protected]>
>>>>
>>>> NAK. *Sigh* NO, this is the wrong level to be doing stuff like this.
>>>>
>>>> The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should
>>>> *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and
>>>> clk_unprepare(). Those two functions are *merely* helpers for drivers
>>>> who don't wish to make the individual calls.
>>>>
>>>> Drivers are still completely free to call the individual functions, at
>>>> which point your proposal breaks horribly - and they _do_ call the
>>>> individual functions.
>>>
>>> I'm proposing to give device driver a choice when it knows that some
>>> driver might be interested in knowing its clock's enabled/disabled state
>>> change at runtime, this is very important for centralized DVFS core
>>> driver. It is not meant to be covering all cases especially for drivers
>>> which is not part of the DVFS, so we don't care if it is calling
>>> clk_enable/disable directly or not.
>>
>> I believe the point Russell is making is not that the idea behind this
>> patch is wrong, but simply that the function where you put the hooks is
>> wrong. The hooks should at least be in clk_enable/clk_disable and not
>> clk_prepare_enable/clk_disable_unprepare, since any driver is free to
>> call clk_prepare separately from clk_enable. The hooks should be
>> implemented in the lowest-level common function that all
>> driver-accessible paths call through.
>
> Thanks, I know the point, but unfortunately there is no good choice for
> hooking this since those low level functions clk_enable/clk_disable will
> be called in interrupt context so it is not possible to send notify. We
> might need to come out a better approach if we can think of any.
> Currently I still think this is acceptable (Having all the drivers which
> are using our interested clocks call these function to enable/disable
> clock in their runtime_pm calls) though it's not perfect.
No, that definitely won't work. Not all drivers use those APIs, nor
should they.
On Wed, 2013-03-13 at 13:24 +0800, Stephen Warren wrote:
> On 03/12/2013 11:08 PM, Bill Huang wrote:
> > On Wed, 2013-03-13 at 12:42 +0800, Stephen Warren wrote:
> >> On 03/12/2013 07:47 PM, Bill Huang wrote:
> >>> On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
> >>>> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
> >>>>> Add the below four notifier events so drivers which are interested in
> >>>>> knowing the clock status can act accordingly. This is extremely useful
> >>>>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
> >>>>>
> >>>>> PRE_CLK_ENABLE
> >>>>> POST_CLK_ENABLE
> >>>>> PRE_CLK_DISABLE
> >>>>> POST_CLK_DISABLE
> >>>>>
> >>>>> Signed-off-by: Bill Huang <[email protected]>
> >>>>
> >>>> NAK. *Sigh* NO, this is the wrong level to be doing stuff like this.
> >>>>
> >>>> The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should
> >>>> *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and
> >>>> clk_unprepare(). Those two functions are *merely* helpers for drivers
> >>>> who don't wish to make the individual calls.
> >>>>
> >>>> Drivers are still completely free to call the individual functions, at
> >>>> which point your proposal breaks horribly - and they _do_ call the
> >>>> individual functions.
> >>>
> >>> I'm proposing to give device driver a choice when it knows that some
> >>> driver might be interested in knowing its clock's enabled/disabled state
> >>> change at runtime, this is very important for centralized DVFS core
> >>> driver. It is not meant to be covering all cases especially for drivers
> >>> which is not part of the DVFS, so we don't care if it is calling
> >>> clk_enable/disable directly or not.
> >>
> >> I believe the point Russell is making is not that the idea behind this
> >> patch is wrong, but simply that the function where you put the hooks is
> >> wrong. The hooks should at least be in clk_enable/clk_disable and not
> >> clk_prepare_enable/clk_disable_unprepare, since any driver is free to
> >> call clk_prepare separately from clk_enable. The hooks should be
> >> implemented in the lowest-level common function that all
> >> driver-accessible paths call through.
> >
> > Thanks, I know the point, but unfortunately there is no good choice for
> > hooking this since those low level functions clk_enable/clk_disable will
> > be called in interrupt context so it is not possible to send notify. We
> > might need to come out a better approach if we can think of any.
> > Currently I still think this is acceptable (Having all the drivers which
> > are using our interested clocks call these function to enable/disable
> > clock in their runtime_pm calls) though it's not perfect.
>
> No, that definitely won't work. Not all drivers use those APIs, nor
> should they.
>
That will be too bad, it looks like we deadlock in the mechanism, we
cannot change existing drivers behavior (that means some call
clk_disable/enable directly, some are not), and we cannot hook notifier
in clk_disable/enable either, that means there seems no any chance to
get what we want, any idea?
On 03/12/2013 11:40 PM, Bill Huang wrote:
> On Wed, 2013-03-13 at 13:24 +0800, Stephen Warren wrote:
>> On 03/12/2013 11:08 PM, Bill Huang wrote:
>>> On Wed, 2013-03-13 at 12:42 +0800, Stephen Warren wrote:
>>>> On 03/12/2013 07:47 PM, Bill Huang wrote:
>>>>> On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
>>>>>> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
>>>>>>> Add the below four notifier events so drivers which are interested in
>>>>>>> knowing the clock status can act accordingly. This is extremely useful
>>>>>>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
>>>>>>>
>>>>>>> PRE_CLK_ENABLE
>>>>>>> POST_CLK_ENABLE
>>>>>>> PRE_CLK_DISABLE
>>>>>>> POST_CLK_DISABLE
...
>>> Thanks, I know the point, but unfortunately there is no good choice for
>>> hooking this since those low level functions clk_enable/clk_disable will
>>> be called in interrupt context so it is not possible to send notify. We
>>> might need to come out a better approach if we can think of any.
>>> Currently I still think this is acceptable (Having all the drivers which
>>> are using our interested clocks call these function to enable/disable
>>> clock in their runtime_pm calls) though it's not perfect.
>>
>> No, that definitely won't work. Not all drivers use those APIs, nor
>> should they.
>>
> That will be too bad, it looks like we deadlock in the mechanism, we
> cannot change existing drivers behavior (that means some call
> clk_disable/enable directly, some are not), and we cannot hook notifier
> in clk_disable/enable either, that means there seems no any chance to
> get what we want, any idea?
I don't know the correct answer.
But I have a question: Why can't we run notifications from the real
clk_enable? Does the notification mechanism itself inherently block, or
are we worried that implementations/receivers of these notifications
might block. Perhaps we can simply define that these notification types
may be triggered in atomic context and hence implementations have to
support executing in atomic context. Is possible to make that a
requirement? Is it possible to implement the code that receives these
notifications in atomic context?
Is it possible to defer triggering of notifications to some non-atomic
context, even if they happen in an atomic context?
I also wonder if this is the right conceptual level to be hooking into
the system. Perhaps DVFS is not something that should be triggered by
noticing that clocks have changed rates, but rather it should be some
form of higher layer that controls (calls into) both the regulator and
clock APIs?
On Thu, 2013-03-14 at 02:10 +0800, Stephen Warren wrote:
> On 03/12/2013 11:40 PM, Bill Huang wrote:
> > On Wed, 2013-03-13 at 13:24 +0800, Stephen Warren wrote:
> >> On 03/12/2013 11:08 PM, Bill Huang wrote:
> >>> On Wed, 2013-03-13 at 12:42 +0800, Stephen Warren wrote:
> >>>> On 03/12/2013 07:47 PM, Bill Huang wrote:
> >>>>> On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
> >>>>>> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
> >>>>>>> Add the below four notifier events so drivers which are interested in
> >>>>>>> knowing the clock status can act accordingly. This is extremely useful
> >>>>>>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
> >>>>>>>
> >>>>>>> PRE_CLK_ENABLE
> >>>>>>> POST_CLK_ENABLE
> >>>>>>> PRE_CLK_DISABLE
> >>>>>>> POST_CLK_DISABLE
> ...
> >>> Thanks, I know the point, but unfortunately there is no good choice for
> >>> hooking this since those low level functions clk_enable/clk_disable will
> >>> be called in interrupt context so it is not possible to send notify. We
> >>> might need to come out a better approach if we can think of any.
> >>> Currently I still think this is acceptable (Having all the drivers which
> >>> are using our interested clocks call these function to enable/disable
> >>> clock in their runtime_pm calls) though it's not perfect.
> >>
> >> No, that definitely won't work. Not all drivers use those APIs, nor
> >> should they.
> >>
> > That will be too bad, it looks like we deadlock in the mechanism, we
> > cannot change existing drivers behavior (that means some call
> > clk_disable/enable directly, some are not), and we cannot hook notifier
> > in clk_disable/enable either, that means there seems no any chance to
> > get what we want, any idea?
>
> I don't know the correct answer.
>
> But I have a question: Why can't we run notifications from the real
> clk_enable? Does the notification mechanism itself inherently block, or
> are we worried that implementations/receivers of these notifications
> might block. Perhaps we can simply define that these notification types
I think both.
> may be triggered in atomic context and hence implementations have to
> support executing in atomic context. Is possible to make that a
> requirement? Is it possible to implement the code that receives these
> notifications in atomic context?
Making it executing in atomic context won't work since for DVFS we
generally having to set voltage (while receiving the notify) through I2C
interface which can sleep.
>
> Is it possible to defer triggering of notifications to some non-atomic
> context, even if they happen in an atomic context?
I don't think deferring will work either, considering the usage of DVFS,
device voltage is tightly coupled with frequency, when clock rate is
about to increase, we have to boost voltage first and we can lower the
voltage after the clock rate has decreased. All the above sequence have
to be guaranteed or you might crash, so deferring not only make thing
complicated in controlling the order but also hurt performance.
>
> I also wonder if this is the right conceptual level to be hooking into
> the system. Perhaps DVFS is not something that should be triggered by
> noticing that clocks have changed rates, but rather it should be some
> form of higher layer that controls (calls into) both the regulator and
> clock APIs?
I wonder will there be a feasible API can do that since most of SoC is
characterizing DVFS on frequency vs. voltage, it makes sense that
reviewing voltage when the clock rate change happen, and the only
mechanism seem to be hooking notifier in not only clock rate change but
also clock enable/disable.
On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
> I don't think deferring will work either, considering the usage of DVFS,
> device voltage is tightly coupled with frequency, when clock rate is
> about to increase, we have to boost voltage first and we can lower the
> voltage after the clock rate has decreased. All the above sequence have
> to be guaranteed or you might crash, so deferring not only make thing
> complicated in controlling the order but also hurt performance.
But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
As clk_prepare/clk_unprepare have to be called before clk_enable or after
clk_disable, the voltage can be raised to a safe level, before the clock
becomes active.
Cheers,
Peter.
On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
>
> > I don't think deferring will work either, considering the usage of DVFS,
> > device voltage is tightly coupled with frequency, when clock rate is
> > about to increase, we have to boost voltage first and we can lower the
> > voltage after the clock rate has decreased. All the above sequence have
> > to be guaranteed or you might crash, so deferring not only make thing
> > complicated in controlling the order but also hurt performance.
>
> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
> As clk_prepare/clk_unprepare have to be called before clk_enable or after
> clk_disable, the voltage can be raised to a safe level, before the clock
> becomes active.
Thanks Peter, actually I'm just about to propose my v2 RFC which add
notifier in clk_prepare/clk_unprepare.
>
> Cheers,
>
> Peter.
On 03/14/2013 03:28 AM, Bill Huang wrote:
> On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
>> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
>>
>>> I don't think deferring will work either, considering the usage of DVFS,
>>> device voltage is tightly coupled with frequency, when clock rate is
>>> about to increase, we have to boost voltage first and we can lower the
>>> voltage after the clock rate has decreased. All the above sequence have
>>> to be guaranteed or you might crash, so deferring not only make thing
>>> complicated in controlling the order but also hurt performance.
>>
>> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
>> As clk_prepare/clk_unprepare have to be called before clk_enable or after
>> clk_disable, the voltage can be raised to a safe level, before the clock
>> becomes active.
>
> Thanks Peter, actually I'm just about to propose my v2 RFC which add
> notifier in clk_prepare/clk_unprepare.
Can't clk_set_rate() be called while the clock is prepared, or even
enabled? I don't see how your proposal would work.
On Fri, 2013-03-15 at 01:54 +0800, Stephen Warren wrote:
> On 03/14/2013 03:28 AM, Bill Huang wrote:
> > On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
> >> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
> >>
> >>> I don't think deferring will work either, considering the usage of DVFS,
> >>> device voltage is tightly coupled with frequency, when clock rate is
> >>> about to increase, we have to boost voltage first and we can lower the
> >>> voltage after the clock rate has decreased. All the above sequence have
> >>> to be guaranteed or you might crash, so deferring not only make thing
> >>> complicated in controlling the order but also hurt performance.
> >>
> >> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
> >> As clk_prepare/clk_unprepare have to be called before clk_enable or after
> >> clk_disable, the voltage can be raised to a safe level, before the clock
> >> becomes active.
> >
> > Thanks Peter, actually I'm just about to propose my v2 RFC which add
> > notifier in clk_prepare/clk_unprepare.
>
> Can't clk_set_rate() be called while the clock is prepared, or even
> enabled? I don't see how your proposal would work.
>
I think it works with just a little sacrifice on saving more power but
that's related minor. Taking clk_prepare as an indicator on that clock
will be enabled later, so we can raise the voltage to a safe level
(according to the current rate or maybe default rate when clk_prepare is
called, some time late when clk_set_rate() is called we can adjust again
according to the requested rate change) but there should be case that
clock will not be enabled after clk_prepare (I'm not sure is normal
though), that the case power might be wasted. Similarly, taking
clk_unprepare as an indicator on clock has been disabled but there might
be case clock is disabled but do not call clk_unprepare any time soon,
this is another case power is wasted but this should not be normal case.
So the point is, we can get notified on runtime clock change events (no
matter it is clock enable/disable or clock rate change) and act (get
best balance on power saving and system stability) accordingly.
On 03/14/2013 07:20 PM, Bill Huang wrote:
> On Fri, 2013-03-15 at 01:54 +0800, Stephen Warren wrote:
>> On 03/14/2013 03:28 AM, Bill Huang wrote:
>>> On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
>>>> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
>>>>
>>>>> I don't think deferring will work either, considering the usage of DVFS,
>>>>> device voltage is tightly coupled with frequency, when clock rate is
>>>>> about to increase, we have to boost voltage first and we can lower the
>>>>> voltage after the clock rate has decreased. All the above sequence have
>>>>> to be guaranteed or you might crash, so deferring not only make thing
>>>>> complicated in controlling the order but also hurt performance.
>>>>
>>>> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
>>>> As clk_prepare/clk_unprepare have to be called before clk_enable or after
>>>> clk_disable, the voltage can be raised to a safe level, before the clock
>>>> becomes active.
>>>
>>> Thanks Peter, actually I'm just about to propose my v2 RFC which add
>>> notifier in clk_prepare/clk_unprepare.
>>
>> Can't clk_set_rate() be called while the clock is prepared, or even
>> enabled? I don't see how your proposal would work.
>
> I think it works with just a little sacrifice on saving more power but
> that's related minor. Taking clk_prepare as an indicator on that clock
> will be enabled later, so we can raise the voltage to a safe level
> (according to the current rate or maybe default rate when clk_prepare is
> called, some time late when clk_set_rate() is called we can adjust again
> according to the requested rate change)
Is clk_set_rate() only legal to call in non-atomic contexts then? The
header file doesn't say, although I guess since many other functions
explicitly say they can't, then by omission it can...
On Fri, 2013-03-15 at 13:22 +0800, Stephen Warren wrote:
> On 03/14/2013 07:20 PM, Bill Huang wrote:
> > On Fri, 2013-03-15 at 01:54 +0800, Stephen Warren wrote:
> >> On 03/14/2013 03:28 AM, Bill Huang wrote:
> >>> On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
> >>>> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
> >>>>
> >>>>> I don't think deferring will work either, considering the usage of DVFS,
> >>>>> device voltage is tightly coupled with frequency, when clock rate is
> >>>>> about to increase, we have to boost voltage first and we can lower the
> >>>>> voltage after the clock rate has decreased. All the above sequence have
> >>>>> to be guaranteed or you might crash, so deferring not only make thing
> >>>>> complicated in controlling the order but also hurt performance.
> >>>>
> >>>> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
> >>>> As clk_prepare/clk_unprepare have to be called before clk_enable or after
> >>>> clk_disable, the voltage can be raised to a safe level, before the clock
> >>>> becomes active.
> >>>
> >>> Thanks Peter, actually I'm just about to propose my v2 RFC which add
> >>> notifier in clk_prepare/clk_unprepare.
> >>
> >> Can't clk_set_rate() be called while the clock is prepared, or even
> >> enabled? I don't see how your proposal would work.
> >
> > I think it works with just a little sacrifice on saving more power but
> > that's related minor. Taking clk_prepare as an indicator on that clock
> > will be enabled later, so we can raise the voltage to a safe level
> > (according to the current rate or maybe default rate when clk_prepare is
> > called, some time late when clk_set_rate() is called we can adjust again
> > according to the requested rate change)
>
> Is clk_set_rate() only legal to call in non-atomic contexts then? The
> header file doesn't say, although I guess since many other functions
> explicitly say they can't, then by omission it can...
I think clk_set_rate can only be called in non-atomic contexts since
there are existing non-atomic clock notifier hooked in it.
On Fri, Mar 15, 2013 at 06:22:47AM +0100, Stephen Warren wrote:
> On 03/14/2013 07:20 PM, Bill Huang wrote:
> > On Fri, 2013-03-15 at 01:54 +0800, Stephen Warren wrote:
> >> On 03/14/2013 03:28 AM, Bill Huang wrote:
> >>> On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
> >>>> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
> >>>>
> >>>>> I don't think deferring will work either, considering the usage of DVFS,
> >>>>> device voltage is tightly coupled with frequency, when clock rate is
> >>>>> about to increase, we have to boost voltage first and we can lower the
> >>>>> voltage after the clock rate has decreased. All the above sequence have
> >>>>> to be guaranteed or you might crash, so deferring not only make thing
> >>>>> complicated in controlling the order but also hurt performance.
> >>>>
> >>>> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
> >>>> As clk_prepare/clk_unprepare have to be called before clk_enable or after
> >>>> clk_disable, the voltage can be raised to a safe level, before the clock
> >>>> becomes active.
> >>>
> >>> Thanks Peter, actually I'm just about to propose my v2 RFC which add
> >>> notifier in clk_prepare/clk_unprepare.
> >>
> >> Can't clk_set_rate() be called while the clock is prepared, or even
> >> enabled? I don't see how your proposal would work.
> >
> > I think it works with just a little sacrifice on saving more power but
> > that's related minor. Taking clk_prepare as an indicator on that clock
> > will be enabled later, so we can raise the voltage to a safe level
> > (according to the current rate or maybe default rate when clk_prepare is
> > called, some time late when clk_set_rate() is called we can adjust again
> > according to the requested rate change)
>
> Is clk_set_rate() only legal to call in non-atomic contexts then? The
> header file doesn't say, although I guess since many other functions
> explicitly say they can't, then by omission it can...
Yes. Only clk_enable() and clk_disable() can be called in an atomic context.
Cheers,
Peter.
On 15 March 2013 10:39, Peter De Schrijver <[email protected]> wrote:
> On Fri, Mar 15, 2013 at 06:22:47AM +0100, Stephen Warren wrote:
>> On 03/14/2013 07:20 PM, Bill Huang wrote:
>> > On Fri, 2013-03-15 at 01:54 +0800, Stephen Warren wrote:
>> >> On 03/14/2013 03:28 AM, Bill Huang wrote:
>> >>> On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
>> >>>> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
>> >>>>
>> >>>>> I don't think deferring will work either, considering the usage of DVFS,
>> >>>>> device voltage is tightly coupled with frequency, when clock rate is
>> >>>>> about to increase, we have to boost voltage first and we can lower the
>> >>>>> voltage after the clock rate has decreased. All the above sequence have
>> >>>>> to be guaranteed or you might crash, so deferring not only make thing
>> >>>>> complicated in controlling the order but also hurt performance.
>> >>>>
>> >>>> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
>> >>>> As clk_prepare/clk_unprepare have to be called before clk_enable or after
>> >>>> clk_disable, the voltage can be raised to a safe level, before the clock
>> >>>> becomes active.
>> >>>
>> >>> Thanks Peter, actually I'm just about to propose my v2 RFC which add
>> >>> notifier in clk_prepare/clk_unprepare.
>> >>
>> >> Can't clk_set_rate() be called while the clock is prepared, or even
>> >> enabled? I don't see how your proposal would work.
>> >
>> > I think it works with just a little sacrifice on saving more power but
>> > that's related minor. Taking clk_prepare as an indicator on that clock
>> > will be enabled later, so we can raise the voltage to a safe level
>> > (according to the current rate or maybe default rate when clk_prepare is
>> > called, some time late when clk_set_rate() is called we can adjust again
>> > according to the requested rate change)
>>
>> Is clk_set_rate() only legal to call in non-atomic contexts then? The
>> header file doesn't say, although I guess since many other functions
>> explicitly say they can't, then by omission it can...
>
> Yes. Only clk_enable() and clk_disable() can be called in an atomic context.
>
> Cheers,
>
> Peter.
>
> _______________________________________________
> linaro-dev mailing list
> [email protected]
> http://lists.linaro.org/mailman/listinfo/linaro-dev
Just wanted to add my reflection on this topic;
Some prerequisites; I think am in favor of using the clk API to
trigger DVFS changes and then I agree on that clk_prepare|unprepare
needs to be possible to track from a DVFS perspective. clk_set_rate is
not enough.
So if we decide to do the above (using the clk API to trigger DVFS
changes), I believe we should discuss two possible solutions;
- clk notifiers or..
- dvfs clock type.
I am trying to make up my mind of what I think is the best solution.
Have you considered "dvfs clock type"?
I put some comments about this for "[PATCH 2/5] clk: notifier handler
for dynamic voltage scaling" recently as well.
What could the advantages/disadvantages be between the two options?
Kind regards
Ulf Hansson
On Fri, 2013-03-15 at 18:08 +0800, Ulf Hansson wrote:
> On 15 March 2013 10:39, Peter De Schrijver <[email protected]> wrote:
> > On Fri, Mar 15, 2013 at 06:22:47AM +0100, Stephen Warren wrote:
> >> On 03/14/2013 07:20 PM, Bill Huang wrote:
> >> > On Fri, 2013-03-15 at 01:54 +0800, Stephen Warren wrote:
> >> >> On 03/14/2013 03:28 AM, Bill Huang wrote:
> >> >>> On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
> >> >>>> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
> >> >>>>
> >> >>>>> I don't think deferring will work either, considering the usage of DVFS,
> >> >>>>> device voltage is tightly coupled with frequency, when clock rate is
> >> >>>>> about to increase, we have to boost voltage first and we can lower the
> >> >>>>> voltage after the clock rate has decreased. All the above sequence have
> >> >>>>> to be guaranteed or you might crash, so deferring not only make thing
> >> >>>>> complicated in controlling the order but also hurt performance.
> >> >>>>
> >> >>>> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
> >> >>>> As clk_prepare/clk_unprepare have to be called before clk_enable or after
> >> >>>> clk_disable, the voltage can be raised to a safe level, before the clock
> >> >>>> becomes active.
> >> >>>
> >> >>> Thanks Peter, actually I'm just about to propose my v2 RFC which add
> >> >>> notifier in clk_prepare/clk_unprepare.
> >> >>
> >> >> Can't clk_set_rate() be called while the clock is prepared, or even
> >> >> enabled? I don't see how your proposal would work.
> >> >
> >> > I think it works with just a little sacrifice on saving more power but
> >> > that's related minor. Taking clk_prepare as an indicator on that clock
> >> > will be enabled later, so we can raise the voltage to a safe level
> >> > (according to the current rate or maybe default rate when clk_prepare is
> >> > called, some time late when clk_set_rate() is called we can adjust again
> >> > according to the requested rate change)
> >>
> >> Is clk_set_rate() only legal to call in non-atomic contexts then? The
> >> header file doesn't say, although I guess since many other functions
> >> explicitly say they can't, then by omission it can...
> >
> > Yes. Only clk_enable() and clk_disable() can be called in an atomic context.
> >
> > Cheers,
> >
> > Peter.
> >
> > _______________________________________________
> > linaro-dev mailing list
> > [email protected]
> > http://lists.linaro.org/mailman/listinfo/linaro-dev
>
> Just wanted to add my reflection on this topic;
>
> Some prerequisites; I think am in favor of using the clk API to
> trigger DVFS changes and then I agree on that clk_prepare|unprepare
> needs to be possible to track from a DVFS perspective. clk_set_rate is
> not enough.
>
> So if we decide to do the above (using the clk API to trigger DVFS
> changes), I believe we should discuss two possible solutions;
> - clk notifiers or..
> - dvfs clock type.
>
> I am trying to make up my mind of what I think is the best solution.
> Have you considered "dvfs clock type"?
> I put some comments about this for "[PATCH 2/5] clk: notifier handler
> for dynamic voltage scaling" recently as well.
>
> What could the advantages/disadvantages be between the two options?
I personally prefer clk notifiers since that's easy and all the existing
device drivers don't need to be modified, a new clock or API might be
more thoroughly considered (and hence maybe more graceful) but that
means we need more time to cook and many drivers need to plug into that
API when it comes out, a lot of test/verification or maybe chaos
follows, I'm not sure will that be a little overkill.
I'm not against designing a more robust API, I'm just suggesting before
we have that defined we should at least let clk notifiers work.
>
> Kind regards
> Ulf Hansson
On 15 March 2013 13:06, Bill Huang <[email protected]> wrote:
> On Fri, 2013-03-15 at 18:08 +0800, Ulf Hansson wrote:
>> On 15 March 2013 10:39, Peter De Schrijver <[email protected]> wrote:
>> > On Fri, Mar 15, 2013 at 06:22:47AM +0100, Stephen Warren wrote:
>> >> On 03/14/2013 07:20 PM, Bill Huang wrote:
>> >> > On Fri, 2013-03-15 at 01:54 +0800, Stephen Warren wrote:
>> >> >> On 03/14/2013 03:28 AM, Bill Huang wrote:
>> >> >>> On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote:
>> >> >>>> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote:
>> >> >>>>
>> >> >>>>> I don't think deferring will work either, considering the usage of DVFS,
>> >> >>>>> device voltage is tightly coupled with frequency, when clock rate is
>> >> >>>>> about to increase, we have to boost voltage first and we can lower the
>> >> >>>>> voltage after the clock rate has decreased. All the above sequence have
>> >> >>>>> to be guaranteed or you might crash, so deferring not only make thing
>> >> >>>>> complicated in controlling the order but also hurt performance.
>> >> >>>>
>> >> >>>> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage no?
>> >> >>>> As clk_prepare/clk_unprepare have to be called before clk_enable or after
>> >> >>>> clk_disable, the voltage can be raised to a safe level, before the clock
>> >> >>>> becomes active.
>> >> >>>
>> >> >>> Thanks Peter, actually I'm just about to propose my v2 RFC which add
>> >> >>> notifier in clk_prepare/clk_unprepare.
>> >> >>
>> >> >> Can't clk_set_rate() be called while the clock is prepared, or even
>> >> >> enabled? I don't see how your proposal would work.
>> >> >
>> >> > I think it works with just a little sacrifice on saving more power but
>> >> > that's related minor. Taking clk_prepare as an indicator on that clock
>> >> > will be enabled later, so we can raise the voltage to a safe level
>> >> > (according to the current rate or maybe default rate when clk_prepare is
>> >> > called, some time late when clk_set_rate() is called we can adjust again
>> >> > according to the requested rate change)
>> >>
>> >> Is clk_set_rate() only legal to call in non-atomic contexts then? The
>> >> header file doesn't say, although I guess since many other functions
>> >> explicitly say they can't, then by omission it can...
>> >
>> > Yes. Only clk_enable() and clk_disable() can be called in an atomic context.
>> >
>> > Cheers,
>> >
>> > Peter.
>> >
>> > _______________________________________________
>> > linaro-dev mailing list
>> > [email protected]
>> > http://lists.linaro.org/mailman/listinfo/linaro-dev
>>
>> Just wanted to add my reflection on this topic;
>>
>> Some prerequisites; I think am in favor of using the clk API to
>> trigger DVFS changes and then I agree on that clk_prepare|unprepare
>> needs to be possible to track from a DVFS perspective. clk_set_rate is
>> not enough.
>>
>> So if we decide to do the above (using the clk API to trigger DVFS
>> changes), I believe we should discuss two possible solutions;
>> - clk notifiers or..
>> - dvfs clock type.
>>
>> I am trying to make up my mind of what I think is the best solution.
>> Have you considered "dvfs clock type"?
>> I put some comments about this for "[PATCH 2/5] clk: notifier handler
>> for dynamic voltage scaling" recently as well.
>>
>> What could the advantages/disadvantages be between the two options?
>
> I personally prefer clk notifiers since that's easy and all the existing
> device drivers don't need to be modified, a new clock or API might be
> more thoroughly considered (and hence maybe more graceful) but that
> means we need more time to cook and many drivers need to plug into that
> API when it comes out, a lot of test/verification or maybe chaos
> follows, I'm not sure will that be a little overkill.
I guess you did not fully got what I meant with "dvfs clock type". It
will not affect the clock API. But instead the dvfs is handled by
implementing a specific clk hw type. So the same thing is accomplished
as with clk notifiers, no changes should be needed to device drivers.
The difference is only that no notifiers will be needed, and all the
dvfs stuff will be handled in the clk hw instead. It will mean that we
will bundle dvfs stuff into the clock drivers, instead of separating
the code outside the clock drivers. But, on the other hand no
notifiers will be needed.
>
> I'm not against designing a more robust API, I'm just suggesting before
> we have that defined we should at least let clk notifiers work.
>>
>> Kind regards
>> Ulf Hansson
>
>
Kind regards
Ulf Hansson
On Tue, Mar 12, 2013 at 06:47:53PM -0700, Bill Huang wrote:
> On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
> > On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
> > > Add the below four notifier events so drivers which are interested in
> > > knowing the clock status can act accordingly. This is extremely useful
> > > in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
> > >
> > > PRE_CLK_ENABLE
> > > POST_CLK_ENABLE
> > > PRE_CLK_DISABLE
> > > POST_CLK_DISABLE
> > >
> > > Signed-off-by: Bill Huang <[email protected]>
> >
> > NAK. *Sigh* NO, this is the wrong level to be doing stuff like this.
> >
> > The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should
> > *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and
> > clk_unprepare(). Those two functions are *merely* helpers for drivers
> > who don't wish to make the individual calls.
> >
> > Drivers are still completely free to call the individual functions, at
> > which point your proposal breaks horribly - and they _do_ call the
> > individual functions.
>
> I'm proposing to give device driver a choice when it knows that some
> driver might be interested in knowing its clock's enabled/disabled state
> change at runtime, this is very important for centralized DVFS core
> driver. It is not meant to be covering all cases especially for drivers
> which is not part of the DVFS, so we don't care if it is calling
> clk_enable/disable directly or not.
But you're not giving drivers a choice. You're giving them an ultimatum.
Either they use clk_prepare_enable() which must only be called from non-
atomic contexts and have the notifiers, or if they need to use the
individual functions (which is what they _should_ be doing but people
are too lazy to properly convert stuff) they don't get the option of
the notifiers at all.
This sucks totally, design wise.
The whole point of clk_prepare_enable() is that it is a helper function
to _only_ do the clk_prepare() call followed by a clk_enable() call and
_nothing_ _else_ _what_ _so_ _ever_.
On Tue, Mar 12, 2013 at 10:42:30PM -0600, Stephen Warren wrote:
> I believe the point Russell is making is not that the idea behind this
> patch is wrong, but simply that the function where you put the hooks is
> wrong. The hooks should at least be in clk_enable/clk_disable and not
Indeed, remembering that clk_enable/clk_disable can be called from
atomic contexts.
If the hook needs to be non-atomic (iow, it can schedule) then it can't
go into clk_enable/clk_disable, and must go into clk_prepare/clk_unprepare,
which is the schedulable half of that API.
On Tue, Mar 12, 2013 at 10:40:04PM -0700, Bill Huang wrote:
> That will be too bad, it looks like we deadlock in the mechanism, we
> cannot change existing drivers behavior (that means some call
> clk_disable/enable directly, some are not), and we cannot hook notifier
> in clk_disable/enable either, that means there seems no any chance to
> get what we want, any idea?
Look, the whole point is:
- Drivers can call clk_enable/clk_disable from their atomic regions to
control the clock. Drivers which do this also call clk_prepare/
clk_unprepare from a schedulable context to perform any operations
necessary to allow the clock to be used.
- Drivers which only ever control the clock from a schedulable context
*can* use clk_prepare_enable()/clk_disable_unprepare() to control
their clock, which simplifies the coding in the driver.
The whole point here is to cater for what is found on different SoCs and
not need to keep rewriting the drivers between different SoCs.
So, the idea is that:
- clk_prepare() does whatever is needed to prepare a clock for use which
may require waiting for the clock to be in a state which it can be
enabled. In other words, if there is a PLL, the PLL is setup and
we wait for it to report that it has locked.
- clk_enable() is about turning the clock output on so that the device
receives the clock.
Now, in the case of a PLL directly feeding a device, it's entirely possible
that clk_prepare() ends up providing the clock signal to the device, and
clk_enable() does absolutely nothing.
Or, if the clock has a gate on it, it's entirely possible that clk_prepare()
does nothing, and clk_enable() unmasks the gate to allow the clock to be
provided to the device - which can happen from atomic contexts.
The whole point about the separation of these two functions is that device
driver writers _can_ code their drivers for both situations and not care
about how the SoC implements the clocking at all.
Why did we end up with this split in the first place? Because we ran into
the problem that some SoCs required a sleeping clk_enable() and others
didn't, and the whole thing was turning into an incompatible mess.
So, please. Realise that clk_prepare() and clk_enable() are the _official_
APIs, and that clk_prepare_enable() is merely a helper function for drivers
to allow them to automate the calling of those two functions in succession
with _no_ _further_ _processing_ at all.
So, if your hooks need to be callable from schedulable contexts, then you
need to put them inside clk_prepare(). If your hooks are callable from
atomic contexts, then they can go into clk_enable(). But what you can
not do is put them into clk_prepare_enable().
On Thu, Mar 14, 2013 at 11:22:47PM -0600, Stephen Warren wrote:
> Is clk_set_rate() only legal to call in non-atomic contexts then? The
> header file doesn't say, although I guess since many other functions
> explicitly say they can't, then by omission it can...
I think when all this was discussed alongside the clk_prepare/clk_unprepare
stuff, that yes, it was decided that clk_set_rate() was non-atomic only.
And yes, that is most definitely the case, because the first thing
clk_set_rate() does is take a mutex - which will trigger a might_sleep()
complaint if ever called from an atomic context.
On Fri, 15 Mar 2013, Russell King - ARM Linux wrote:
> On Tue, Mar 12, 2013 at 06:47:53PM -0700, Bill Huang wrote:
> > On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote:
> > > On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote:
> > > > Add the below four notifier events so drivers which are interested in
> > > > knowing the clock status can act accordingly. This is extremely useful
> > > > in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
> > > >
> > > > PRE_CLK_ENABLE
> > > > POST_CLK_ENABLE
> > > > PRE_CLK_DISABLE
> > > > POST_CLK_DISABLE
> > > >
> > > > Signed-off-by: Bill Huang <[email protected]>
> > >
> > > NAK. *Sigh* NO, this is the wrong level to be doing stuff like this.
> > >
> > > The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should
> > > *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and
> > > clk_unprepare(). Those two functions are *merely* helpers for drivers
> > > who don't wish to make the individual calls.
> > >
> > > Drivers are still completely free to call the individual functions, at
> > > which point your proposal breaks horribly - and they _do_ call the
> > > individual functions.
> >
> > I'm proposing to give device driver a choice when it knows that some
> > driver might be interested in knowing its clock's enabled/disabled state
> > change at runtime, this is very important for centralized DVFS core
> > driver. It is not meant to be covering all cases especially for drivers
> > which is not part of the DVFS, so we don't care if it is calling
> > clk_enable/disable directly or not.
>
> But you're not giving drivers a choice. You're giving them an ultimatum.
> Either they use clk_prepare_enable() which must only be called from non-
> atomic contexts and have the notifiers, or if they need to use the
> individual functions (which is what they _should_ be doing but people
> are too lazy to properly convert stuff) they don't get the option of
> the notifiers at all.
>
> This sucks totally, design wise.
>
> The whole point of clk_prepare_enable() is that it is a helper function
> to _only_ do the clk_prepare() call followed by a clk_enable() call and
> _nothing_ _else_ _what_ _so_ _ever_.
If people are too lazy and start abusing clk_prepare_enable() then this
helper function becomes counter-productive and should simply be removed.
Same issue as with IS_ERR_OR_NULL().
Nicolas
On 03/15/2013 06:33 AM, Ulf Hansson wrote:
> On 15 March 2013 13:06, Bill Huang <[email protected]> wrote:
>> On Fri, 2013-03-15 at 18:08 +0800, Ulf Hansson wrote:
...
>>> Some prerequisites; I think am in favor of using the clk API to
>>> trigger DVFS changes and then I agree on that clk_prepare|unprepare
>>> needs to be possible to track from a DVFS perspective. clk_set_rate is
>>> not enough.
>>>
>>> So if we decide to do the above (using the clk API to trigger DVFS
>>> changes), I believe we should discuss two possible solutions;
>>> - clk notifiers or..
>>> - dvfs clock type.
>>>
>>> I am trying to make up my mind of what I think is the best solution.
>>> Have you considered "dvfs clock type"?
>>> I put some comments about this for "[PATCH 2/5] clk: notifier handler
>>> for dynamic voltage scaling" recently as well.
>>>
>>> What could the advantages/disadvantages be between the two options?
>>
>> I personally prefer clk notifiers since that's easy and all the existing
>> device drivers don't need to be modified, a new clock or API might be
>> more thoroughly considered (and hence maybe more graceful) but that
>> means we need more time to cook and many drivers need to plug into that
>> API when it comes out, a lot of test/verification or maybe chaos
>> follows, I'm not sure will that be a little overkill.
>
> I guess you did not fully got what I meant with "dvfs clock type". It
> will not affect the clock API. But instead the dvfs is handled by
> implementing a specific clk hw type. So the same thing is accomplished
> as with clk notifiers, no changes should be needed to device drivers.
>
> The difference is only that no notifiers will be needed, and all the
> dvfs stuff will be handled in the clk hw instead. It will mean that we
> will bundle dvfs stuff into the clock drivers, instead of separating
> the code outside the clock drivers. But, on the other hand no
> notifiers will be needed.
The advantage here is that I assume that a notifier would continually
have to check whether the clock being modified was one that the DVFS
notifier cared about. By integrating the CVFS logic into the clk_hw
itself, it'll only ever get executed for clocks that really care about
DVFS. Presumably, the code that implements the clk_hw could also use
some common DVFS library as part of the implementation, and still share
code. Or perhaps, what about putting DVFS "ops" into a clk_hw alongside
any other existing ops, and having the clock core call them whenever
appropriate?
On Sat, 2013-03-16 at 03:38 +0800, Stephen Warren wrote:
> On 03/15/2013 06:33 AM, Ulf Hansson wrote:
> > On 15 March 2013 13:06, Bill Huang <[email protected]> wrote:
> >> On Fri, 2013-03-15 at 18:08 +0800, Ulf Hansson wrote:
> ...
> >>> Some prerequisites; I think am in favor of using the clk API to
> >>> trigger DVFS changes and then I agree on that clk_prepare|unprepare
> >>> needs to be possible to track from a DVFS perspective. clk_set_rate is
> >>> not enough.
> >>>
> >>> So if we decide to do the above (using the clk API to trigger DVFS
> >>> changes), I believe we should discuss two possible solutions;
> >>> - clk notifiers or..
> >>> - dvfs clock type.
> >>>
> >>> I am trying to make up my mind of what I think is the best solution.
> >>> Have you considered "dvfs clock type"?
> >>> I put some comments about this for "[PATCH 2/5] clk: notifier handler
> >>> for dynamic voltage scaling" recently as well.
> >>>
> >>> What could the advantages/disadvantages be between the two options?
> >>
> >> I personally prefer clk notifiers since that's easy and all the existing
> >> device drivers don't need to be modified, a new clock or API might be
> >> more thoroughly considered (and hence maybe more graceful) but that
> >> means we need more time to cook and many drivers need to plug into that
> >> API when it comes out, a lot of test/verification or maybe chaos
> >> follows, I'm not sure will that be a little overkill.
> >
> > I guess you did not fully got what I meant with "dvfs clock type". It
> > will not affect the clock API. But instead the dvfs is handled by
> > implementing a specific clk hw type. So the same thing is accomplished
> > as with clk notifiers, no changes should be needed to device drivers.
> >
> > The difference is only that no notifiers will be needed, and all the
> > dvfs stuff will be handled in the clk hw instead. It will mean that we
> > will bundle dvfs stuff into the clock drivers, instead of separating
> > the code outside the clock drivers. But, on the other hand no
> > notifiers will be needed.
>
> The advantage here is that I assume that a notifier would continually
> have to check whether the clock being modified was one that the DVFS
> notifier cared about. By integrating the CVFS logic into the clk_hw
Actually, we can register notifier only on clocks that DVFS care about.
On Fri, 2013-03-15 at 20:33 +0800, Ulf Hansson wrote:
> I guess you did not fully got what I meant with "dvfs clock type". It
> will not affect the clock API. But instead the dvfs is handled by
> implementing a specific clk hw type. So the same thing is accomplished
> as with clk notifiers, no changes should be needed to device drivers.
>
> The difference is only that no notifiers will be needed, and all the
> dvfs stuff will be handled in the clk hw instead. It will mean that we
> will bundle dvfs stuff into the clock drivers, instead of separating
> the code outside the clock drivers. But, on the other hand no
> notifiers will be needed.
>
Oh yes I misunderstand your origin point, but my thought is using
existing devfreq framework as frequency/voltage policy driver instead of
creating another one in clock driver and that's why I think we need the
notifier work.
By the way, some centralized DVFS implementation like Tegra's VDD_CORE
rail has association with tens of clocks which will need to be taken
care specially if we're doing those in clock driver I think.
> Kind regards
> Ulf Hansson
On Sat, 2013-03-16 at 01:09 +0800, Russell King - ARM Linux wrote:
> On Tue, Mar 12, 2013 at 10:40:04PM -0700, Bill Huang wrote:
> > That will be too bad, it looks like we deadlock in the mechanism, we
> > cannot change existing drivers behavior (that means some call
> > clk_disable/enable directly, some are not), and we cannot hook notifier
> > in clk_disable/enable either, that means there seems no any chance to
> > get what we want, any idea?
>
> Look, the whole point is:
>
> - Drivers can call clk_enable/clk_disable from their atomic regions to
> control the clock. Drivers which do this also call clk_prepare/
> clk_unprepare from a schedulable context to perform any operations
> necessary to allow the clock to be used.
>
> - Drivers which only ever control the clock from a schedulable context
> *can* use clk_prepare_enable()/clk_disable_unprepare() to control
> their clock, which simplifies the coding in the driver.
>
> The whole point here is to cater for what is found on different SoCs and
> not need to keep rewriting the drivers between different SoCs.
>
> So, the idea is that:
>
> - clk_prepare() does whatever is needed to prepare a clock for use which
> may require waiting for the clock to be in a state which it can be
> enabled. In other words, if there is a PLL, the PLL is setup and
> we wait for it to report that it has locked.
>
> - clk_enable() is about turning the clock output on so that the device
> receives the clock.
>
> Now, in the case of a PLL directly feeding a device, it's entirely possible
> that clk_prepare() ends up providing the clock signal to the device, and
> clk_enable() does absolutely nothing.
>
> Or, if the clock has a gate on it, it's entirely possible that clk_prepare()
> does nothing, and clk_enable() unmasks the gate to allow the clock to be
> provided to the device - which can happen from atomic contexts.
>
> The whole point about the separation of these two functions is that device
> driver writers _can_ code their drivers for both situations and not care
> about how the SoC implements the clocking at all.
>
> Why did we end up with this split in the first place? Because we ran into
> the problem that some SoCs required a sleeping clk_enable() and others
> didn't, and the whole thing was turning into an incompatible mess.
>
> So, please. Realise that clk_prepare() and clk_enable() are the _official_
> APIs, and that clk_prepare_enable() is merely a helper function for drivers
> to allow them to automate the calling of those two functions in succession
> with _no_ _further_ _processing_ at all.
>
> So, if your hooks need to be callable from schedulable contexts, then you
> need to put them inside clk_prepare(). If your hooks are callable from
> atomic contexts, then they can go into clk_enable(). But what you can
> not do is put them into clk_prepare_enable().
Thanks a lot for good point.
On 15 March 2013 20:38, Stephen Warren <[email protected]> wrote:
> On 03/15/2013 06:33 AM, Ulf Hansson wrote:
>> On 15 March 2013 13:06, Bill Huang <[email protected]> wrote:
>>> On Fri, 2013-03-15 at 18:08 +0800, Ulf Hansson wrote:
> ...
>>>> Some prerequisites; I think am in favor of using the clk API to
>>>> trigger DVFS changes and then I agree on that clk_prepare|unprepare
>>>> needs to be possible to track from a DVFS perspective. clk_set_rate is
>>>> not enough.
>>>>
>>>> So if we decide to do the above (using the clk API to trigger DVFS
>>>> changes), I believe we should discuss two possible solutions;
>>>> - clk notifiers or..
>>>> - dvfs clock type.
>>>>
>>>> I am trying to make up my mind of what I think is the best solution.
>>>> Have you considered "dvfs clock type"?
>>>> I put some comments about this for "[PATCH 2/5] clk: notifier handler
>>>> for dynamic voltage scaling" recently as well.
>>>>
>>>> What could the advantages/disadvantages be between the two options?
>>>
>>> I personally prefer clk notifiers since that's easy and all the existing
>>> device drivers don't need to be modified, a new clock or API might be
>>> more thoroughly considered (and hence maybe more graceful) but that
>>> means we need more time to cook and many drivers need to plug into that
>>> API when it comes out, a lot of test/verification or maybe chaos
>>> follows, I'm not sure will that be a little overkill.
>>
>> I guess you did not fully got what I meant with "dvfs clock type". It
>> will not affect the clock API. But instead the dvfs is handled by
>> implementing a specific clk hw type. So the same thing is accomplished
>> as with clk notifiers, no changes should be needed to device drivers.
>>
>> The difference is only that no notifiers will be needed, and all the
>> dvfs stuff will be handled in the clk hw instead. It will mean that we
>> will bundle dvfs stuff into the clock drivers, instead of separating
>> the code outside the clock drivers. But, on the other hand no
>> notifiers will be needed.
>
> The advantage here is that I assume that a notifier would continually
> have to check whether the clock being modified was one that the DVFS
> notifier cared about. By integrating the CVFS logic into the clk_hw
> itself, it'll only ever get executed for clocks that really care about
> DVFS. Presumably, the code that implements the clk_hw could also use
> some common DVFS library as part of the implementation, and still share
> code. Or perhaps, what about putting DVFS "ops" into a clk_hw alongside
> any other existing ops, and having the clock core call them whenever
> appropriate?
Thanks for your comment Stephen.
I agree to your reflections as well. It will probably be a more
optimized solution going this direction and we don't have to add more
"clk notifier code" to the clk API, which I guess is good.
It would be interesting to get some input from some of the maintainers
to this discussion as well, let's see.
Kind regards
Ulf Hansson
Quoting Ulf Hansson (2013-03-18 03:36:29)
> On 15 March 2013 20:38, Stephen Warren <[email protected]> wrote:
> > On 03/15/2013 06:33 AM, Ulf Hansson wrote:
> >> On 15 March 2013 13:06, Bill Huang <[email protected]> wrote:
> >>> On Fri, 2013-03-15 at 18:08 +0800, Ulf Hansson wrote:
> > ...
> >>>> Some prerequisites; I think am in favor of using the clk API to
> >>>> trigger DVFS changes and then I agree on that clk_prepare|unprepare
> >>>> needs to be possible to track from a DVFS perspective. clk_set_rate is
> >>>> not enough.
> >>>>
> >>>> So if we decide to do the above (using the clk API to trigger DVFS
> >>>> changes), I believe we should discuss two possible solutions;
> >>>> - clk notifiers or..
> >>>> - dvfs clock type.
> >>>>
> >>>> I am trying to make up my mind of what I think is the best solution.
> >>>> Have you considered "dvfs clock type"?
> >>>> I put some comments about this for "[PATCH 2/5] clk: notifier handler
> >>>> for dynamic voltage scaling" recently as well.
> >>>>
> >>>> What could the advantages/disadvantages be between the two options?
> >>>
> >>> I personally prefer clk notifiers since that's easy and all the existing
> >>> device drivers don't need to be modified, a new clock or API might be
> >>> more thoroughly considered (and hence maybe more graceful) but that
> >>> means we need more time to cook and many drivers need to plug into that
> >>> API when it comes out, a lot of test/verification or maybe chaos
> >>> follows, I'm not sure will that be a little overkill.
> >>
> >> I guess you did not fully got what I meant with "dvfs clock type". It
> >> will not affect the clock API. But instead the dvfs is handled by
> >> implementing a specific clk hw type. So the same thing is accomplished
> >> as with clk notifiers, no changes should be needed to device drivers.
> >>
> >> The difference is only that no notifiers will be needed, and all the
> >> dvfs stuff will be handled in the clk hw instead. It will mean that we
> >> will bundle dvfs stuff into the clock drivers, instead of separating
> >> the code outside the clock drivers. But, on the other hand no
> >> notifiers will be needed.
> >
> > The advantage here is that I assume that a notifier would continually
> > have to check whether the clock being modified was one that the DVFS
> > notifier cared about. By integrating the CVFS logic into the clk_hw
> > itself, it'll only ever get executed for clocks that really care about
> > DVFS. Presumably, the code that implements the clk_hw could also use
> > some common DVFS library as part of the implementation, and still share
> > code. Or perhaps, what about putting DVFS "ops" into a clk_hw alongside
> > any other existing ops, and having the clock core call them whenever
> > appropriate?
>
> Thanks for your comment Stephen.
>
> I agree to your reflections as well. It will probably be a more
> optimized solution going this direction and we don't have to add more
> "clk notifier code" to the clk API, which I guess is good.
> It would be interesting to get some input from some of the maintainers
> to this discussion as well, let's see.
>
I do not like the dvfs clock type at all. For the set of DVFS problems
that we are trying to solve, voltage scaling is more of a function of a
device's requirements than of a clock. Put another way, we don't scale
voltage because a clock runs at a rate, we scale voltage because a
device runs at a rate. That is why the clock rate change notifiers were
interesting for dvfs: the device scales the voltage in response to a
clock frequency change.
However it seems the rate-change notifiers are a bit messy for dvfs.
As such I'm currently hacking on a new rfc to introduce a separate dvfs
api. Based on recent discussions and some face-to-face feedback I think
that kicking off DVFS transitions from the clock framework (which looks
very enticing at first glance) is coming at the problem from the wrong
direction. A new api that builds on top of clocks, regulators and opps
is straight-forward, requires no weird cross-layer calls and is
generally cleaner. The downside is that driver authors will have to
choose between using clk_set_rate or magic_dvfs_set_rate apis. That
dilemma is entirely analogous to the runtime pm versus
clk_enable/clk_disable dilemma and hopefully won't be a big hurdle to
acceptance.
I'll post in the v2 thread as well, specifically addressing the
devfreq/clk_set_rate idea.
Regards,
Mike
> Kind regards
> Ulf Hansson
>
> _______________________________________________
> linaro-dev mailing list
> [email protected]
> http://lists.linaro.org/mailman/listinfo/linaro-dev