| Message ID | 1363091861-21534-1-git-send-email-bilhuang@nvidia.com |
|---|---|
| State | New |
| Headers | show |
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 <bilhuang@nvidia.com> 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 <bilhuang@nvidia.com> > > 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 <bilhuang@nvidia.com> >> >> 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 <bilhuang@nvidia.com> > >> > >> 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 <bilhuang@nvidia.com> >>>> >>>> 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 <bilhuang@nvidia.com> > >>>> > >>>> 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 <pdeschrijver@nvidia.com> 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 > linaro-dev@lists.linaro.org > 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 <pdeschrijver@nvidia.com> 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 > > linaro-dev@lists.linaro.org > > 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 <bilhuang@nvidia.com> wrote: > On Fri, 2013-03-15 at 18:08 +0800, Ulf Hansson wrote: >> On 15 March 2013 10:39, Peter De Schrijver <pdeschrijver@nvidia.com> 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 >> > linaro-dev@lists.linaro.org >> > 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 <bilhuang@nvidia.com> > > > > 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 <bilhuang@nvidia.com> > > > > > > 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 <bilhuang@nvidia.com> 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 <bilhuang@nvidia.com> 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 <swarren@wwwdotorg.org> wrote: > On 03/15/2013 06:33 AM, Ulf Hansson wrote: >> On 15 March 2013 13:06, Bill Huang <bilhuang@nvidia.com> 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 <swarren@wwwdotorg.org> wrote: > > On 03/15/2013 06:33 AM, Ulf Hansson wrote: > >> On 15 March 2013 13:06, Bill Huang <bilhuang@nvidia.com> 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 > linaro-dev@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-dev
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
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 <bilhuang@nvidia.com> --- drivers/clk/clk.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 40 ++++++++++++++++++++--------------- 2 files changed, 81 insertions(+), 17 deletions(-)