common clock framework

common clock framework

[Chao Xie]

Hi,
The common clock driver in drivers/clk/ has been integrated into mainline. In fact, I still have some questions to the driver.
1. Why need big lock: enable_lock and prepare_lock?
   Using these locks means that the clock enable and prepare operation cannot re-entered.
2. How to handle the depended clocks?
   For example clock1 and clock2 both have multiple inputs, and they both depend on clock3. It means that before enable clock1 or clock2, clock3 should be enabled, and if clock1 and clock2 are both off, clock2 need to be off too to save power. Clock3 is not the parent of clock1 and clock2, because the clock3 is not one of inputs of clock1 and clock2.
   If we do not handle it in clock framework, we have to let device driver to know it. It means that the device driver need to handle clock dependency. It will make the device driver to be complicated especially it is shared by different SOCs.
3. how to handle voltage changes in clock framework?
   When some clock's rate is changed, the voltage for the component that using the clock may need changed too. So where to add the voltage change sequence?
   

common clock framework

[Sascha Hauer]

On Thu, May 03, 2012 at 07:02:07PM -0700, Chao Xie wrote:
> Hi,
> The common clock driver in drivers/clk/ has been integrated into mainline. In fact, I still have some questions to the driver.
> 1. Why need big lock: enable_lock and prepare_lock?
>    Using these locks means that the clock enable and prepare operation cannot re-entered.

This has been discussed at length on the list. Short answer is that it's
not trivial to properly lock the clock tree (or parts thereof) if we have
individual clock locks. The possibility of having a lock per tree has
been discussed if the single lock approach turns out to be unsufficient.
This may be added later.

> 2. How to handle the depended clocks?
>    For example clock1 and clock2 both have multiple inputs, and they
>    both depend on clock3. It means that before enable clock1 or
>    clock2, clock3 should be enabled, and if clock1 and clock2 are both
>    off, clock2 need to be off too to save power. Clock3 is not the
>    parent of clock1 and clock2, because the clock3 is not one of
>    inputs of clock1 and clock2.  If we do not handle it in clock
>    framework, we have to let device driver to know it. It means that
>    the device driver need to handle clock dependency. It will make the
>    device driver to be complicated especially it is shared by
>    different SOCs.

If clock3 is not parent of clock2 and clock1 then the dependency does
not come from your clock tree and thus can't be handled in the clock
framework.
If you have to enable clock3 to make your device work, then yes, your
device must turn it on. It could also be that clock3 is not used by
your device directly but by a bus which your device depends on. In this
case you would have to describe the dependency on bus level.
The clock framework can only describe a clock tree and that's all it
*should* do.

> 3. how to handle voltage changes in clock framework?  When some
> clock's rate is changed, the voltage for the component that using the
> clock may need changed too. So where to add the voltage change
> sequence?

Clocks have notifiers attached to them. You can hook yourself into a pre
rate change (increasing the voltage here) and into a post rate change
(decreasing the voltage here).
That said, we are not there yet. Once the clock framework has settled
and is actually used in SoCs I'm sure somebody will come up with some
driver which can be passed a freq <-> voltage table for a given
regulator clock pair.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

common clock framework

[Chao Xie]

Hi
For the clock dependency, what about bus clock. What do you mean about bus level.
As I understand the device driver only need care about enabling a clock to make it work. For example a USB driver. To make a USB work, it may need turn on bus clock, function clock, and phy clock. For some SOCs, phy and function may share same clock. There is a problem, we leave this dependency to device driver or keep it in our clock framework.
To keep it in clock framework, the device driver only need know that enable one clock. To leave the dependency to device driver, the device driver will need know about all the clocks and dependency. If the device driver is shared by many SOCs, it will add some #ifdef into it.
So why not add a dependency list into clock structure, and it will do enable the dependency clock before enable its own clock.


-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de] 
Sent: Friday, May 04, 2012 4:21 PM
To: Chao Xie
Cc: Turquette, Mike; linux-arm-kernel at lists.infradead.org
Subject: Re: common clock framework

On Thu, May 03, 2012 at 07:02:07PM -0700, Chao Xie wrote:
> Hi,
> The common clock driver in drivers/clk/ has been integrated into mainline. In fact, I still have some questions to the driver.
> 1. Why need big lock: enable_lock and prepare_lock?
>    Using these locks means that the clock enable and prepare operation cannot re-entered.

This has been discussed at length on the list. Short answer is that it's
not trivial to properly lock the clock tree (or parts thereof) if we have
individual clock locks. The possibility of having a lock per tree has
been discussed if the single lock approach turns out to be unsufficient.
This may be added later.

> 2. How to handle the depended clocks?
>    For example clock1 and clock2 both have multiple inputs, and they
>    both depend on clock3. It means that before enable clock1 or
>    clock2, clock3 should be enabled, and if clock1 and clock2 are both
>    off, clock2 need to be off too to save power. Clock3 is not the
>    parent of clock1 and clock2, because the clock3 is not one of
>    inputs of clock1 and clock2.  If we do not handle it in clock
>    framework, we have to let device driver to know it. It means that
>    the device driver need to handle clock dependency. It will make the
>    device driver to be complicated especially it is shared by
>    different SOCs.

If clock3 is not parent of clock2 and clock1 then the dependency does
not come from your clock tree and thus can't be handled in the clock
framework.
If you have to enable clock3 to make your device work, then yes, your
device must turn it on. It could also be that clock3 is not used by
your device directly but by a bus which your device depends on. In this
case you would have to describe the dependency on bus level.
The clock framework can only describe a clock tree and that's all it
*should* do.

> 3. how to handle voltage changes in clock framework?  When some
> clock's rate is changed, the voltage for the component that using the
> clock may need changed too. So where to add the voltage change
> sequence?

Clocks have notifiers attached to them. You can hook yourself into a pre
rate change (increasing the voltage here) and into a post rate change
(decreasing the voltage here).
That said, we are not there yet. Once the clock framework has settled
and is actually used in SoCs I'm sure somebody will come up with some
driver which can be passed a freq <-> voltage table for a given
regulator clock pair.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

common clock framework

[Russell King - ARM Linux]

On Fri, May 04, 2012 at 01:45:15AM -0700, Chao Xie wrote:
> For the clock dependency, what about bus clock. What do you mean about
> bus level.
>
> As I understand the device driver only need care about enabling a clock
> to make it work. For example a USB driver. To make a USB work, it may
> need turn on bus clock, function clock, and phy clock. For some SOCs,
> phy and function may share same clock. There is a problem, we leave this
> dependency to device driver or keep it in our clock framework.

If the hardware block takes three clocks, then the driver should be asking
for three clocks and turning them on as if they were three separate clocks.
Note I'm talking here about the USB module itself _before_ it is integrated
onto the SoC.

Where SoCs don't provide explicit control of three clocks, but instead
combine it as one clock, then you need to provide all three clock lookups
from the _very_ _same_ struct clk for the device driver.  That means when
any one of the clocks is requested to be enabled, they all will be.

This eliminates this kind of horrible dependency at the clock layer as well
as the driver layer.  This is also how the clk API is intended to be used
by drivers.

We don't ifdef clocks in drivers.  We provide what the driver (and
ultimately hardware) requires.

common clock framework

[Sascha Hauer]

Please do not top post.

On Fri, May 04, 2012 at 01:45:15AM -0700, Chao Xie wrote:
> Hi
> For the clock dependency, what about bus clock. What do you mean about
> bus level.  As I understand the device driver only need care about
> enabling a clock to make it work. For example a USB driver. To make a
> USB work, it may need turn on bus clock, function clock, and phy
> clock. For some SOCs, phy and function may share same clock. There is
> a problem, we leave this dependency to device driver or keep it in our
> clock framework.  To keep it in clock framework, the device driver
> only need know that enable one clock. To leave the dependency to
> device driver, the device driver will need know about all the clocks
> and dependency. If the device driver is shared by many SOCs, it will
> add some #ifdef into it.  So why not add a dependency list into clock
> structure, and it will do enable the dependency clock before enable
> its own clock.

Additional to what Russell already said:
If a clock is not needed by the device directly but for example by the
bus which connects the device, then you need an instance which controls
the bus. Then it's the bus that knows which clocks to activate when a
device on this bus is activated. That said, there currently is hardly
any infrastructure for this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

common clock framework

[Turquette, Mike]

On Fri, May 4, 2012 at 1:21 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Thu, May 03, 2012 at 07:02:07PM -0700, Chao Xie wrote:
>> Hi,
>> The common clock driver in drivers/clk/ has been integrated into mainline. In fact, I still have some questions to the driver.
>> 1. Why need big lock: enable_lock and prepare_lock?
>> ? ?Using these locks means that the clock enable and prepare operation cannot re-entered.
>
> This has been discussed at length on the list. Short answer is that it's
> not trivial to properly lock the clock tree (or parts thereof) if we have
> individual clock locks. The possibility of having a lock per tree has
> been discussed if the single lock approach turns out to be unsufficient.
> This may be added later.

I think it is worth discussing improvements to the locking scheme.
More on that below.

snip

>> 3. how to handle voltage changes in clock framework? ?When some
>> clock's rate is changed, the voltage for the component that using the
>> clock may need changed too. So where to add the voltage change
>> sequence?
>
> Clocks have notifiers attached to them. You can hook yourself into a pre
> rate change (increasing the voltage here) and into a post rate change
> (decreasing the voltage here).
> That said, we are not there yet. Once the clock framework has settled
> and is actually used in SoCs I'm sure somebody will come up with some
> driver which can be passed a freq <-> voltage table for a given
> regulator clock pair.

We are very close.  I have been hacking on this in a private branch
and have converted OMAP's CPUfreq driver over to using *only* the the
clock framework for DVFS.

Good news: it works!  The CPUfreq driver's .target callback does the
normal rate validation and table lookup to get a good rate for the
CPU, then it simply calls clk_set_rate.  The pre- and post-rate-change
notifiers are then fired off.  I have added notifier handlers to the
OMAP regulator drivers which use a look-up-table (soon to be converted
to the OPP library) to determine voltage and they simply call
regulator_set_voltage either before or after the clock rate changes,
depending on scaling direction.

Bad news: lockdep gets cranky about possible deadlocks due to holding
prepare_lock and then trying to hold it again in a rate-change
notifier handler (from OMAP's regulator code).  Specifically
clk_set_rate holds prepare_lock, and then the i2c message sent to the
PMIC to raise voltage calls clk_prepare as part of the standard
messaging sequence.  This is clearly a candidate for a deadlock.

Having clk_set_rate and clk_prepare both hold the same prepare_lock
mutex seems suboptimal, but it is easy.  Having reentrant accesses to
the clock tree is going to be hard...  I've spent some time thinking
of ways to solve this, but I would appreciate suggestions.  I suspect
the exact same case I'm describing above will affect many SoCs.

Regards,
Mike

common clock framework

[Sascha Hauer]

On Fri, May 04, 2012 at 04:08:20PM -0700, Turquette, Mike wrote:
> 
> We are very close.  I have been hacking on this in a private branch
> and have converted OMAP's CPUfreq driver over to using *only* the the
> clock framework for DVFS.
> 
> Good news: it works!  The CPUfreq driver's .target callback does the
> normal rate validation and table lookup to get a good rate for the
> CPU, then it simply calls clk_set_rate.  The pre- and post-rate-change
> notifiers are then fired off.  I have added notifier handlers to the
> OMAP regulator drivers which use a look-up-table (soon to be converted
> to the OPP library) to determine voltage and they simply call
> regulator_set_voltage either before or after the clock rate changes,
> depending on scaling direction.
> 
> Bad news: lockdep gets cranky about possible deadlocks due to holding
> prepare_lock and then trying to hold it again in a rate-change
> notifier handler (from OMAP's regulator code).  Specifically
> clk_set_rate holds prepare_lock, and then the i2c message sent to the
> PMIC to raise voltage calls clk_prepare as part of the standard
> messaging sequence.  This is clearly a candidate for a deadlock.
> 
> Having clk_set_rate and clk_prepare both hold the same prepare_lock
> mutex seems suboptimal, but it is easy.  Having reentrant accesses to
> the clock tree is going to be hard...  I've spent some time thinking
> of ways to solve this, but I would appreciate suggestions.  I suspect
> the exact same case I'm describing above will affect many SoCs.

That's interesting. Here's another one: What will happen when Mark
attaches one of his i2c wolfson chips to a omap core and wants to test
his new clock driver? a clk_prepare on some clock on the wolfson chip
will trigger another clk_prepare inside the i2c driver.
So the reentrancy problem is not limited to prepare vs. set_rate.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

common clock framework

[Turquette, Mike]

+ Mark & Graeme (for audio/pmic perspective)

On Sat, May 5, 2012 at 1:28 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Fri, May 04, 2012 at 04:08:20PM -0700, Turquette, Mike wrote:
>>
>> We are very close. ?I have been hacking on this in a private branch
>> and have converted OMAP's CPUfreq driver over to using *only* the the
>> clock framework for DVFS.
>>
>> Good news: it works! ?The CPUfreq driver's .target callback does the
>> normal rate validation and table lookup to get a good rate for the
>> CPU, then it simply calls clk_set_rate. ?The pre- and post-rate-change
>> notifiers are then fired off. ?I have added notifier handlers to the
>> OMAP regulator drivers which use a look-up-table (soon to be converted
>> to the OPP library) to determine voltage and they simply call
>> regulator_set_voltage either before or after the clock rate changes,
>> depending on scaling direction.
>>
>> Bad news: lockdep gets cranky about possible deadlocks due to holding
>> prepare_lock and then trying to hold it again in a rate-change
>> notifier handler (from OMAP's regulator code). ?Specifically
>> clk_set_rate holds prepare_lock, and then the i2c message sent to the
>> PMIC to raise voltage calls clk_prepare as part of the standard
>> messaging sequence. ?This is clearly a candidate for a deadlock.
>>
>> Having clk_set_rate and clk_prepare both hold the same prepare_lock
>> mutex seems suboptimal, but it is easy. ?Having reentrant accesses to
>> the clock tree is going to be hard... ?I've spent some time thinking
>> of ways to solve this, but I would appreciate suggestions. ?I suspect
>> the exact same case I'm describing above will affect many SoCs.
>
> That's interesting. Here's another one: What will happen when Mark
> attaches one of his i2c wolfson chips to a omap core and wants to test
> his new clock driver? a clk_prepare on some clock on the wolfson chip
> will trigger another clk_prepare inside the i2c driver.
> So the reentrancy problem is not limited to prepare vs. set_rate.

common clock framework

[Mike Turquette]

On 20120505-13:33, Raul Xiong wrote:
> 2012/5/5 Turquette, Mike <mturquette@ti.com>
> > Bad news: lockdep gets cranky about possible deadlocks due to holding
> > prepare_lock and then trying to hold it again in a rate-change
> > notifier handler (from OMAP's regulator code).  Specifically
> >
> 
> Glad to see common clock framework will support DVFS. Can we use different
> spinlock for different clocks with different lockdep lock classes to avoid
> the dead lock and lockdep warnings?

I understand that different lockdep classes could allow for nested
locking, but I don't have a good idea of how that would actually be
implemented.  Could you elaborate a bit more?

In the mean time I'm looking at some different locking semantics that
don't involve lockdep class mangling, just a more fine-grained approach
to locking exactly what we want.

Regards,
Mike

common clock framework

[Raul Xiong]

2012/5/7 Mike Turquette <mturquette@ti.com>:
> On 20120505-13:33, Raul Xiong wrote:
>> 2012/5/5 Turquette, Mike <mturquette@ti.com>
>> > Bad news: lockdep gets cranky about possible deadlocks due to holding
>> > prepare_lock and then trying to hold it again in a rate-change
>> > notifier handler (from OMAP's regulator code). ?Specifically
>> >
>>
>> Glad to see common clock framework will support DVFS. Can we use different
>> spinlock for different clocks with different lockdep lock classes to avoid
>> the dead lock and lockdep warnings?
>
> I understand that different lockdep classes could allow for nested
> locking, but I don't have a good idea of how that would actually be
> implemented. ?Could you elaborate a bit more?
>
> In the mean time I'm looking at some different locking semantics that
> don't involve lockdep class mangling, just a more fine-grained approach
> to locking exactly what we want.
>
> Regards,
> Mike

I think the key point is if we decide to use separate spinlock for different
clocks. If yes we can call spin_lock_init and lockdep_set_class(lock,
&clk->lockdep_key)
explicitly in __clk_init with the lockdep_key we have to add in struct clk:

struct clk {
        ...
#ifdef CONFIG_LOCKDEP
         struct lock_class_key lockdep_key;
#endif
        ...
}

common clock framework

[Mark Brown]

On Sat, May 05, 2012 at 10:44:17AM -0700, Turquette, Mike wrote:
> + Mark & Graeme (for audio/pmic perspective)

> >> Having clk_set_rate and clk_prepare both hold the same prepare_lock
> >> mutex seems suboptimal, but it is easy. ?Having reentrant accesses to
> >> the clock tree is going to be hard... ?I've spent some time thinking
> >> of ways to solve this, but I would appreciate suggestions. ?I suspect
> >> the exact same case I'm describing above will affect many SoCs.

> > That's interesting. Here's another one: What will happen when Mark
> > attaches one of his i2c wolfson chips to a omap core and wants to test
> > his new clock driver? a clk_prepare on some clock on the wolfson chip
> > will trigger another clk_prepare inside the i2c driver.
> > So the reentrancy problem is not limited to prepare vs. set_rate.

This sounds pretty much expected - you'll also see similar things on
some SoCs where IPs for clocks might get clock gated when not in use and
reentrancy could crop up while exiting low power modes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120508/d985f537/attachment.sig>

common clock framework

[Turquette, Mike]

On Tue, May 8, 2012 at 2:01 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, May 05, 2012 at 10:44:17AM -0700, Turquette, Mike wrote:
>> + Mark & Graeme (for audio/pmic perspective)
>
>> >> Having clk_set_rate and clk_prepare both hold the same prepare_lock
>> >> mutex seems suboptimal, but it is easy. ?Having reentrant accesses to
>> >> the clock tree is going to be hard... ?I've spent some time thinking
>> >> of ways to solve this, but I would appreciate suggestions. ?I suspect
>> >> the exact same case I'm describing above will affect many SoCs.
>
>> > That's interesting. Here's another one: What will happen when Mark
>> > attaches one of his i2c wolfson chips to a omap core and wants to test
>> > his new clock driver? a clk_prepare on some clock on the wolfson chip
>> > will trigger another clk_prepare inside the i2c driver.
>> > So the reentrancy problem is not limited to prepare vs. set_rate.
>
> This sounds pretty much expected - you'll also see similar things on
> some SoCs where IPs for clocks might get clock gated when not in use and
> reentrancy could crop up while exiting low power modes.

Going down this locking path has been ugly.  I got sidetracked by the
whole spinlock/mutex raciness thing yesterday (which I abhor).  I
don't think those two can be unified unless we have sleeping spinlocks
or something similar in the future...

But I digress.  Today I'm playing with a scheme that only affects
users of the prepare_lock mutex.  Basically each clock has it's own
mutex; we hold the global prepare_lock mutex when a top-level
operation is called, then lock only the affected clocks' per-clock
mutex (while under the global lock), then release the global
prepare_lock mutex and continue the operation.

The scheme described above means that the clock tree is more
re-entrant than it used to be... however if clock A is involved in two
separate operations then one will block while the other holds clock
A's mutex.

So it's basically a hybrid scheme.  Thoughts?

Mike

common clock framework

[Chao Xie]

Hi
Since the issues are not described very clearly, so I want to clarify it.
Issue 1. clock with mux, divisor and trigger
The clock A will have a single register that some bits control the mux, some bits control the divisor, and a bit is a trigger. It means that setting mux and divisor bits will only take effect when trigger bit is set.
There are 2 models that we can abstract this kind of clock.
a. make three clocks.
clock A <= clock div <= clock mux
clock mux control the mux bits while clock div control the divisor bits, while clock a control trigger bit.
For example mux has two inputs 312MHZ and 1066MHZ, while divisor can be 1 or 2. At beginning clock mux is from 312M and clock div is 1. Clock A can only have clock as 312MHZ and 533MHZ.
so when invoke clk_set_rate(A, 512MHZ), it will try to upstream and find the "top" clock that can satisfy the 512MHZ. the "top" clock is clock mux. After find the "top" clock, the current code will have the following flows.

clk_notify(PRE_RATE_CHANGE)
    ==> clk_change_rate(mux)
        ==> mux->setrate: it will set mux bits
        ==> polling all children, and for each one, invoke clk_change_rate
           ==> clk_change_rate(div)
              ==> div->setrate: it will set div bits
              ==> polling all children, and for each one, invoke clk_change_rate
                  ==>A->setrate: it will set trigger bit
>From above that the function looks well, but there will be some problem.
Clock mux change the mux bits will not finally switch the input from 312MHZ to 533MHZ. The switch is controlled by "trigger" bit. So clock mux should have .setrate implementation as you suggest as
      1. enable new parent
      2. set mux bits
      3. __clk_reparent
      4. disable old parent
The clock mux implementation looks like omap dpll implementation. 
But there are two bugs.
   a) "Disable old parent" is only SW setting, we depends on trigger to do it. So if for clock mux and clock div we all have trigger set when set mux bits and div bits. It will import a temporary clock 1066MHZ(input = 1066, div = 1), it will harm the device. In fact, there is why there is a trigger bit here. It will forbid there is no temporary clock.
   b) if clk_change_rate(A) will invoke cpu_notify(POST_RATE_CHANGE), while __clk_reparent will invoke __clk_recalc_rates, and in __clk_recalc_rates, it will polling every children, and for each one will invoke __clk_recalc_rates too. For __clk_recalc_rates, it will invoke cpu_notify(POST_RATE_CHANGE), and it means that the notification of POST_RATE_CHANGE will be invoke twice. I think the omap dpll implementation will have this bug too.

b. make a special clock a that includes all bits setting.
Then we come back that we need patch " [PATCH V2] clk: give clock chance to change parent at setrate stage" in maillist

Issue 2: clock with dependency
As we know that each vendor may have some version of SOCs, and these SOC may share some same components.
For example, device a need function clock A while device B need function clock B, there share some bus, and there a clock for the bus clock gating. It means that if we want device B working, we need enable clock B and clock bus.
In fact, we can not consider that clock bus is the parent of clock A/B, because clock A/B has inputs from PLLs.
So how do we control these clocks? If leaving the dependency in clock tree, we need additional patch for it. If leaving it to device driver, it will make the device driver to know more details about clock framework, and for different SOCs share same device driver, the clock framework may not same. So it will make the device driver to be SOCs depended, and will need more detailed platform data.




-----Original Message-----
From: Mike Turquette [mailto:mturquette at ti.com] 
Sent: Monday, May 07, 2012 7:50 AM
To: Raul Xiong
Cc: Sascha Hauer; linux-arm-kernel at lists.infradead.org; Chao Xie
Subject: Re: common clock framework

On 20120505-13:33, Raul Xiong wrote:
> 2012/5/5 Turquette, Mike <mturquette@ti.com>
> > Bad news: lockdep gets cranky about possible deadlocks due to holding
> > prepare_lock and then trying to hold it again in a rate-change
> > notifier handler (from OMAP's regulator code).  Specifically
> >
> 
> Glad to see common clock framework will support DVFS. Can we use different
> spinlock for different clocks with different lockdep lock classes to avoid
> the dead lock and lockdep warnings?

I understand that different lockdep classes could allow for nested
locking, but I don't have a good idea of how that would actually be
implemented.  Could you elaborate a bit more?

In the mean time I'm looking at some different locking semantics that
don't involve lockdep class mangling, just a more fine-grained approach
to locking exactly what we want.

Regards,
Mike

common clock framework

[Mike Turquette]

On 20120518-01:41, Chao Xie wrote:
> Issue 2: clock with dependency As we know that each vendor may have
> some version of SOCs, and these SOC may share some same components.
> For example, device a need function clock A while device B need
> function clock B, there share some bus, and there a clock for the bus
> clock gating. It means that if we want device B working, we need
> enable clock B and clock bus.  In fact, we can not consider that clock
> bus is the parent of clock A/B, because clock A/B has inputs from
> PLLs.  So how do we control these clocks? If leaving the dependency in
> clock tree, we need additional patch for it. If leaving it to device
> driver, it will make the device driver to know more details about
> clock framework, and for different SOCs share same device driver, the
> clock framework may not same. So it will make the device driver to be
> SOCs depended, and will need more detailed platform data.

This has been discussed in this thread already.  Functional dependencies
are outside the scope of the clock framework.

The brute force solution is to put a clk_prepare_enable() call for both
the functional clock and the bus clock in your driver.  A better
approach (and one employed in the OMAP platform code) is to abstract
away these details from the drivers.  OMAP uses the pm_runtime
framework which makes life easier for drivers by simply calling
pm_runtime_get/pm_runtime_put, which takes care of functional clocks as
well as bus clocks (and other stuff).

The details of the callbacks used by the pm_runtime framework can be
different for each SoC, which addresses your concerns above.

I agree that the details of bus clocks should be abstracted away
somewhere, but the clock framework is not the place for that.

Regards,
Mike

common clock framework

[Mike Turquette]

On 20120518-01:41, Chao Xie wrote:
> Hi
> Since the issues are not described very clearly, so I want to clarify it.

Is is possible for you to wrap your lines to a reasonable length?  I've
done it for you in the email below.

> Issue 1. clock with mux, divisor and trigger
> The clock A will have a single register that some bits control the
> mux, some bits control the divisor, and a bit is a trigger. It means
> that setting mux and divisor bits will only take effect when trigger
> bit is set.  There are 2 models that we can abstract this kind of
> clock.
> a. make three clocks.
> clock A <= clock div <= clock mux
> clock mux control the mux bits while clock div control the divisor
> bits, while clock a control trigger bit.  For example mux has two
> inputs 312MHZ and 1066MHZ, while divisor can be 1 or 2. At beginning
> clock mux is from 312M and clock div is 1. Clock A can only have clock
> as 312MHZ and 533MHZ.  so when invoke clk_set_rate(A, 512MHZ), it will
> try to upstream and find the "top" clock that can satisfy the 512MHZ.
> the "top" clock is clock mux. After find the "top" clock, the current
> code will have the following flows.
> 
> clk_notify(PRE_RATE_CHANGE)
>     ==> clk_change_rate(mux)
>         ==> mux->setrate: it will set mux bits
>         ==> polling all children, and for each one, invoke clk_change_rate
>            ==> clk_change_rate(div)
>               ==> div->setrate: it will set div bits
>               ==> polling all children, and for each one, invoke clk_change_rate
>                   ==>A->setrate: it will set trigger bit
> From above that the function looks well, but there will be some
> problem.  Clock mux change the mux bits will not finally switch the
> input from 312MHZ to 533MHZ. The switch is controlled by "trigger"
> bit. So clock mux should have .setrate implementation as you suggest
> as
>       1. enable new parent
>       2. set mux bits
>       3. __clk_reparent
>       4. disable old parent
> The clock mux implementation looks like omap dpll implementation. 
> But there are two bugs.
>    a) "Disable old parent" is only SW setting, we depends on trigger
>    to do it. So if for clock mux and clock div we all have trigger set
>    when set mux bits and div bits. It will import a temporary clock
>    1066MHZ(input = 1066, div = 1), it will harm the device. In fact,
>    there is why there is a trigger bit here. It will forbid there is
>    no temporary clock.

Let me make sure I understand you here.  You are concerned that the
.set_rate implementations for each clock (div and mux) will set the
trigger bit, causing intermediate clock rates.  Do I have that correct?

If that is the case then one solution is to not have your div and mux
.set_rate implementations set the trigger bit.  Instead always propagate
up to "clk A", the trigger clock so that the whole operation is atomic.

The only problem with this scenario is if you ever change only the div
(and not the mux) and there is no parent propagation up to "clk A".  Is
this a possibility on your platform?

>    b) if clk_change_rate(A) will invoke cpu_notify(POST_RATE_CHANGE),
>    while __clk_reparent will invoke __clk_recalc_rates, and in
>    __clk_recalc_rates, it will polling every children, and for each
>    one will invoke __clk_recalc_rates too. For __clk_recalc_rates, it
>    will invoke cpu_notify(POST_RATE_CHANGE), and it means that the
>    notification of POST_RATE_CHANGE will be invoke twice. I think the
>    omap dpll implementation will have this bug too.
> 

This has come up on the list recently.  I think one solution is to
remove the call to __clk_recalc_rates from __clk_reparent.  To be honest
__clk_reparent has grown beyond it's original intent.

> b. make a special clock a that includes all bits setting.
> Then we come back that we need patch " [PATCH V2] clk: give clock
> chance to change parent at setrate stage" in maillist
> 

This is a last resort option.  I'd prefer to make the multiple clocks
thing work.

Regards,
Mike

common clock framework

[Lei Wen]

Hi Mike,

On Wed, May 23, 2012 at 3:11 AM, Mike Turquette <mturquette@ti.com> wrote:
> On 20120518-01:41, Chao Xie wrote:
>> Hi
>> Since the issues are not described very clearly, so I want to clarify it.
>
> Is is possible for you to wrap your lines to a reasonable length? ?I've
> done it for you in the email below.
>
>> Issue 1. clock with mux, divisor and trigger
>> The clock A will have a single register that some bits control the
>> mux, some bits control the divisor, and a bit is a trigger. It means
>> that setting mux and divisor bits will only take effect when trigger
>> bit is set. ?There are 2 models that we can abstract this kind of
>> clock.
>> a. make three clocks.
>> clock A <= clock div <= clock mux
>> clock mux control the mux bits while clock div control the divisor
>> bits, while clock a control trigger bit. ?For example mux has two
>> inputs 312MHZ and 1066MHZ, while divisor can be 1 or 2. At beginning
>> clock mux is from 312M and clock div is 1. Clock A can only have clock
>> as 312MHZ and 533MHZ. ?so when invoke clk_set_rate(A, 512MHZ), it will
>> try to upstream and find the "top" clock that can satisfy the 512MHZ.
>> the "top" clock is clock mux. After find the "top" clock, the current
>> code will have the following flows.
>>
>> clk_notify(PRE_RATE_CHANGE)
>> ? ? ==> clk_change_rate(mux)
>> ? ? ? ? ==> mux->setrate: it will set mux bits
>> ? ? ? ? ==> polling all children, and for each one, invoke clk_change_rate
>> ? ? ? ? ? ?==> clk_change_rate(div)
>> ? ? ? ? ? ? ? ==> div->setrate: it will set div bits
>> ? ? ? ? ? ? ? ==> polling all children, and for each one, invoke clk_change_rate
>> ? ? ? ? ? ? ? ? ? ==>A->setrate: it will set trigger bit
>> From above that the function looks well, but there will be some
>> problem. ?Clock mux change the mux bits will not finally switch the
>> input from 312MHZ to 533MHZ. The switch is controlled by "trigger"
>> bit. So clock mux should have .setrate implementation as you suggest
>> as
>> ? ? ? 1. enable new parent
>> ? ? ? 2. set mux bits
>> ? ? ? 3. __clk_reparent
>> ? ? ? 4. disable old parent
>> The clock mux implementation looks like omap dpll implementation.
>> But there are two bugs.
>> ? ?a) "Disable old parent" is only SW setting, we depends on trigger
>> ? ?to do it. So if for clock mux and clock div we all have trigger set
>> ? ?when set mux bits and div bits. It will import a temporary clock
>> ? ?1066MHZ(input = 1066, div = 1), it will harm the device. In fact,
>> ? ?there is why there is a trigger bit here. It will forbid there is
>> ? ?no temporary clock.
>
> Let me make sure I understand you here. ?You are concerned that the
> .set_rate implementations for each clock (div and mux) will set the
> trigger bit, causing intermediate clock rates. ?Do I have that correct?
>

Yes, that is correct, we don't want the trigger bit to set multiple times.


> If that is the case then one solution is to not have your div and mux
> .set_rate implementations set the trigger bit. ?Instead always propagate
> up to "clk A", the trigger clock so that the whole operation is atomic.
>
> The only problem with this scenario is if you ever change only the div
> (and not the mux) and there is no parent propagation up to "clk A". ?Is
> this a possibility on your platform?

I think it may has some problem here. We need to adjust "clk A"'s parent's
speed when we need change "clk A" speed.

However, we could handle it by another way by caching the register
modification and no really touch the hw register in the div/mux clk node.
So we also could propagation to A's parent, while have no trouble like
set trigger bit multi-times.


>
>> ? ?b) if clk_change_rate(A) will invoke cpu_notify(POST_RATE_CHANGE),
>> ? ?while __clk_reparent will invoke __clk_recalc_rates, and in
>> ? ?__clk_recalc_rates, it will polling every children, and for each
>> ? ?one will invoke __clk_recalc_rates too. For __clk_recalc_rates, it
>> ? ?will invoke cpu_notify(POST_RATE_CHANGE), and it means that the
>> ? ?notification of POST_RATE_CHANGE will be invoke twice. I think the
>> ? ?omap dpll implementation will have this bug too.
>>
>
> This has come up on the list recently. ?I think one solution is to
> remove the call to __clk_recalc_rates from __clk_reparent. ?To be honest
> __clk_reparent has grown beyond it's original intent.

Could we have something like flag judgment here?
If some flag is set, then do __clk_recalc_rates inside __clk_reparent.
By this way, we could have more flexible to handle complex scenario.

Thanks,
Lei

common clock framework

[Turquette, Mike]

On Tue, Apr 24, 2012 at 6:46 PM, Chao Xie <cxie4@marvell.com> wrote:
> Hi, Mike
> I have a question about the code layout.
> Why put the clock implementation for every SOC chip into drivers/clk/XXX/? Why not just put it into arch/arm/XXX?

Hi Chao,

Looping in the rest of LAKML since this these discussions are better
on the list.

The platforms converting over to common clk may put their clock code
in drivers/clk/ or leave it in arch/arm/.  For OMAP we're leaving the
code in arch/arm/mach-omap2/ for the time being due to the disruptive
nature of moving it into drivers/, though we might try to move it into
drivers some day.

There is no hard and fast rule forcing you to migrate your code into
drivers/clk/.  However doing so does expose your code to more eyeballs
and increases the likelihood that we can find common solutions to
problems shared by multiple SoCs/devices.

Regards,
Mike