clk: Per controller locks (prepare & enable)

clk: Per controller locks (prepare & enable)

[Krzysztof Kozlowski]

Hi,


Problems:
1. Deadlocks on prepare lock in following scenario:
 - prepare_enable(some external clock through i2c)
   - i2c transfer
     - prepare_enable(i2c lock in SoC)
       - deadlock
See [1]. We implemented a workaround for this in few places like
10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping I2C
clock prepared") and 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock by
keeping clock prepared")

The goal would be to remove also this workaround.

2. The global prepare and enable locks are over-used. I didn't test the
congestion but intuition says that they could be improved.


Solution:
Make this locks per controller. This will directly solve problem #1
because these are different controllers. Still in-controller deadlock
could occur but we couldn't find a real case with it.


Question:
What do you think about it? I know that talk is cheap and code looks
better but before starting the work I would like to hear some
comments/opinions/ideas.


Best regards,
Krzysztof


[1]
http://www.krzk.eu/builders/boot-odroid-xu3-multi_v7/builds/34/steps/Boot%20odroid/logs/serial

Re: clk: Per controller locks (prepare & enable)

[Javier Martinez Canillas]

Hello Krzysztof,

On 06/29/2016 03:23 AM, Krzysztof Kozlowski wrote:
> Hi,
> 
> 
> Problems:
> 1. Deadlocks on prepare lock in following scenario:
>  - prepare_enable(some external clock through i2c)
>    - i2c transfer
>      - prepare_enable(i2c lock in SoC)
>        - deadlock
> See [1]. We implemented a workaround for this in few places like
> 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping I2C
> clock prepared") and 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock by
> keeping clock prepared")
> 
> The goal would be to remove also this workaround.
>
> 2. The global prepare and enable locks are over-used. I didn't test the
> congestion but intuition says that they could be improved.
> 
> 
> Solution:
> Make this locks per controller. This will directly solve problem #1
> because these are different controllers. Still in-controller deadlock
> could occur but we couldn't find a real case with it.
>

Yes, I also tried first to remove the global locks but then noticed that
changing the global lock wouldn't be trivial, so I decided to got with
the workaround instead at the end.

> 
> Question:
> What do you think about it? I know that talk is cheap and code looks
> better but before starting the work I would like to hear some
> comments/opinions/ideas.
>

The problem is that the enable and prepare operations are propagated to
the parents, so what the locks want to protecting is really a sub-tree
of the clock tree. They currently protect the whole clock hierarchy to
make sure that the changes in the clock tree are atomically.

So a clock per controller won't suffice since you can have a parent for
a clock provided by a different controller and that won't be protected.

Another option is to have a prepare and enable locks per clock. Since
the operations are always propagated in the same direction, I think is
safe to do it.

But even in the case of a more fine-grained locking, I believe the
mentioned deadlocks can still happen. For example in 10ff4c5239a1 the
issue was that the s2mps11 PMIC has both regulators and clocks that are
controlled via I2C so the regulator and clocks drivers shares the same
I2C regmap.

What happened was something like this:

         CPU0                                   CPU1
         ----                                   ----
  regulator_set_voltage()                s3c_rtc_probe()
  regmap_write()                         clk_prepare()
  /* regmap->lock was aquired */         /* prepare_lock was aquired */
  regmap_i2c_write()                     s2mps11_clk_prepare()
  i2c_transfer()                         regmap_write()
  exynos5_i2c_xfer()                     /* deadlock due regmap->lock */
  clk_prepare_enable()
  clk_prepare_lock()
  /* prepare_lock was aquired */

So if for example a per clock lock is used, the deadlock can still happen
if both the I2C clock and S3C RTC source clock share the same parent in a
part of the hierarchy. But as you said this is less likely in practice so
probably is not an issue. 

The disadvantage with a per clock lock is that it may be too fine-grained,
so even when it will help with parallelism and scalability, it will hurt
performance since will be too costly to do a prepare and enable operation.

> 
> Best regards,
> Krzysztof
> 
> 
> [1]
> http://www.krzk.eu/builders/boot-odroid-xu3-multi_v7/builds/34/steps/Boot%20odroid/logs/serial
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: clk: Per controller locks (prepare & enable)

[Krzysztof Kozlowski]

On 06/30/2016 06:22 PM, Javier Martinez Canillas wrote:
>> Question:
>> What do you think about it? I know that talk is cheap and code looks
>> better but before starting the work I would like to hear some
>> comments/opinions/ideas.
>>
> 
> The problem is that the enable and prepare operations are propagated to
> the parents, so what the locks want to protecting is really a sub-tree
> of the clock tree. They currently protect the whole clock hierarchy to
> make sure that the changes in the clock tree are atomically.

Although there is a hierarchy between clocks from different controllers
but still these are all clocks controllers coming from one hardware
device (like SoC). At least on Exynos, I think there is no real
inter-device dependencies. The deadlock you mentioned (and which I want
to fix) is between:
1. clock in PMIC (the one needed by s3c_rtc_probe()),
2. clock for I2C in SoC (the one needed by regmap_write()),
3. and regmap lock:

What I want to say is that the relationship between clocks even when
crossing clock controller boundaries is still self-contained. It is
simple parent-child relationship so acquiring both
clock-controller-level locks is safe.

Current dead lock looks like, simplifying your code:
A:                            B:
lock(regmap)
                              lock(prepare)
lock(prepare) - wait
                              lock(regmap) - wait


When split locks per clock controller this would be:
A:                            B:
lock(regmap)
                              lock(s2mps11)
lock(i2c/exynos)
                              lock(regmap) - wait
do the transfer
unlock(i2c/exynos)
unlock(regmap)
                              lock(regmap) - acquired
                              lock(i2c/exynos)
                              do the transfer
                              unlock(i2c/exynos)
                              unlock(regmap)
                              unlock(s2mps11)

I still have to figure out how to properly protect the entire tree
hierarchy. Maybe the global prepare_lock should be used only for that -
for traversing the hierarchy.

> 
> So a clock per controller won't suffice since you can have a parent for
> a clock provided by a different controller and that won't be protected.
> 
> Another option is to have a prepare and enable locks per clock. Since
> the operations are always propagated in the same direction, I think is
> safe to do it.
> 
> But even in the case of a more fine-grained locking, I believe the
> mentioned deadlocks can still happen. For example in 10ff4c5239a1 the
> issue was that the s2mps11 PMIC has both regulators and clocks that are
> controlled via I2C so the regulator and clocks drivers shares the same
> I2C regmap.
> 
> What happened was something like this:
> 
>          CPU0                                   CPU1
>          ----                                   ----
>   regulator_set_voltage()                s3c_rtc_probe()
>   regmap_write()                         clk_prepare()
>   /* regmap->lock was aquired */         /* prepare_lock was aquired */
>   regmap_i2c_write()                     s2mps11_clk_prepare()
>   i2c_transfer()                         regmap_write()
>   exynos5_i2c_xfer()                     /* deadlock due regmap->lock */
>   clk_prepare_enable()
>   clk_prepare_lock()
>   /* prepare_lock was aquired */
> 
> So if for example a per clock lock is used, the deadlock can still happen
> if both the I2C clock and S3C RTC source clock share the same parent in a
> part of the hierarchy. But as you said this is less likely in practice so
> probably is not an issue.

I think these clocks do not share the parent.

Best regards,
Krzysztof

Re: clk: Per controller locks (prepare & enable)

[Javier Martinez Canillas]

Hello Krzysztof,

On 07/04/2016 04:24 AM, Krzysztof Kozlowski wrote:
> On 06/30/2016 06:22 PM, Javier Martinez Canillas wrote:
>>> Question:
>>> What do you think about it? I know that talk is cheap and code looks
>>> better but before starting the work I would like to hear some
>>> comments/opinions/ideas.
>>>
>>
>> The problem is that the enable and prepare operations are propagated to
>> the parents, so what the locks want to protecting is really a sub-tree
>> of the clock tree. They currently protect the whole clock hierarchy to
>> make sure that the changes in the clock tree are atomically.
> 
> Although there is a hierarchy between clocks from different controllers
> but still these are all clocks controllers coming from one hardware
> device (like SoC). At least on Exynos, I think there is no real
> inter-device dependencies. The deadlock you mentioned (and which I want
> to fix) is between:

Yes, my point was that this may not be the case in all systems. IOW the
framework should be generic enough to allow hierarchies where a parent
clock is a clock provided by a different controller from a different HW.

> 1. clock in PMIC (the one needed by s3c_rtc_probe()),
> 2. clock for I2C in SoC (the one needed by regmap_write()),
> 3. and regmap lock:
> 
> What I want to say is that the relationship between clocks even when
> crossing clock controller boundaries is still self-contained. It is
> simple parent-child relationship so acquiring both
> clock-controller-level locks is safe.
>

Is safe if the clock controllers are always aquired in the same order but
I'm not sure if that will always be the case. I.e: you have controllers A
and B that have clocks A{1,2} and B{1,2} respectively. So you could have
something like this:

A1 with parent B1
B2 with parent A2

That can cause a deadlock since in the first case, the controller A will be
aquired and then the controller B but in the other case, the opposite order
will be attempted.

> Current dead lock looks like, simplifying your code:
> A:                            B:
> lock(regmap)
>                               lock(prepare)
> lock(prepare) - wait
>                               lock(regmap) - wait
> 
> 
> When split locks per clock controller this would be:
> A:                            B:
> lock(regmap)
>                               lock(s2mps11)
> lock(i2c/exynos)
>                               lock(regmap) - wait
> do the transfer
> unlock(i2c/exynos)
> unlock(regmap)
>                               lock(regmap) - acquired
>                               lock(i2c/exynos)
>                               do the transfer
>                               unlock(i2c/exynos)
>                               unlock(regmap)
>                               unlock(s2mps11)
>

Yes, splitting the lock per controller will fix the possible deadlock in
this case but I think we need an approach that is safe for all possible
scenarios. Otherwise it will work more by coincidence than due a design.

> I still have to figure out how to properly protect the entire tree
> hierarchy. Maybe the global prepare_lock should be used only for that -
> for traversing the hierarchy.
>

Other thing to figure out is how to determine when a parent belongs to a
different controller to get its lock in clk_{prepare,enable}(), at least
part of the hierarchy has to be traversed on prepare/enable to propagate
the operations to the parents clocks.

>>
>> So a clock per controller won't suffice since you can have a parent for
>> a clock provided by a different controller and that won't be protected.
>>
>> Another option is to have a prepare and enable locks per clock. Since
>> the operations are always propagated in the same direction, I think is
>> safe to do it.
>>
>> But even in the case of a more fine-grained locking, I believe the
>> mentioned deadlocks can still happen. For example in 10ff4c5239a1 the
>> issue was that the s2mps11 PMIC has both regulators and clocks that are
>> controlled via I2C so the regulator and clocks drivers shares the same
>> I2C regmap.
>>
>> What happened was something like this:
>>
>>          CPU0                                   CPU1
>>          ----                                   ----
>>   regulator_set_voltage()                s3c_rtc_probe()
>>   regmap_write()                         clk_prepare()
>>   /* regmap->lock was aquired */         /* prepare_lock was aquired */
>>   regmap_i2c_write()                     s2mps11_clk_prepare()
>>   i2c_transfer()                         regmap_write()
>>   exynos5_i2c_xfer()                     /* deadlock due regmap->lock */
>>   clk_prepare_enable()
>>   clk_prepare_lock()
>>   /* prepare_lock was aquired */
>>
>> So if for example a per clock lock is used, the deadlock can still happen
>> if both the I2C clock and S3C RTC source clock share the same parent in a
>> part of the hierarchy. But as you said this is less likely in practice so
>> probably is not an issue.
> 
> I think these clocks do not share the parent.
>

Yes, I meant the issue of having a shared parent in general and not in this
particular case that no parent is shared. Sorry for the ambiguous sentence.

> Best regards,
> Krzysztof
>

So in summary, my point is that we should first define what these locks are
protecting. The good practice in the kernel is to protect data structures
instead of code, but the prepare and enable locks are not doing the former.

My understanding is that what the locks are protecting is a sub-tree of the
hierarchy but there isn't a data structure to represent a sub-tree so global
locks are used to protect the whole tree instead.

Moving the lock to a per controller/provider seems like an arbitrary thing
to me since what's important AFAIU is the clock hierarchy and not from where
these clocks come from.

But I'm very far from being a lock expert so maybe my assumptions are wrong,
I'll let Stephen and Mike to comment on this.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: clk: Per controller locks (prepare & enable)

[Javier Martinez Canillas]

On 07/04/2016 11:15 AM, Javier Martinez Canillas wrote:
> 
> But I'm very far from being a lock expert so maybe my assumptions are wrong,

I meant *clock* here, sorry for the typo.

> I'll let Stephen and Mike to comment on this.
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: clk: Per controller locks (prepare & enable)

[Krzysztof Kozlowski]

On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 07/04/2016 04:24 AM, Krzysztof Kozlowski wrote:
>> On 06/30/2016 06:22 PM, Javier Martinez Canillas wrote:
>>>> Question:
>>>> What do you think about it? I know that talk is cheap and code looks
>>>> better but before starting the work I would like to hear some
>>>> comments/opinions/ideas.
>>>>
>>>
>>> The problem is that the enable and prepare operations are propagated to
>>> the parents, so what the locks want to protecting is really a sub-tree
>>> of the clock tree. They currently protect the whole clock hierarchy to
>>> make sure that the changes in the clock tree are atomically.
>>
>> Although there is a hierarchy between clocks from different controllers
>> but still these are all clocks controllers coming from one hardware
>> device (like SoC). At least on Exynos, I think there is no real
>> inter-device dependencies. The deadlock you mentioned (and which I want
>> to fix) is between:
> 
> Yes, my point was that this may not be the case in all systems. IOW the
> framework should be generic enough to allow hierarchies where a parent
> clock is a clock provided by a different controller from a different HW.

Is there such configuration?

> 
>> 1. clock in PMIC (the one needed by s3c_rtc_probe()),
>> 2. clock for I2C in SoC (the one needed by regmap_write()),
>> 3. and regmap lock:
>>
>> What I want to say is that the relationship between clocks even when
>> crossing clock controller boundaries is still self-contained. It is
>> simple parent-child relationship so acquiring both
>> clock-controller-level locks is safe.
>>
> 
> Is safe if the clock controllers are always aquired in the same order but
> I'm not sure if that will always be the case. I.e: you have controllers A
> and B that have clocks A{1,2} and B{1,2} respectively. So you could have
> something like this:
> 
> A1 with parent B1
> B2 with parent A2

Again, is there such configuration? We thought here about it and at
least it is not known to us. Of course this is not a proof that such
configuration does not exist...

> 
> That can cause a deadlock since in the first case, the controller A will be
> aquired and then the controller B but in the other case, the opposite order
> will be attempted.

Yes.

> 
>> Current dead lock looks like, simplifying your code:
>> A:                            B:
>> lock(regmap)
>>                               lock(prepare)
>> lock(prepare) - wait
>>                               lock(regmap) - wait
>>
>>
>> When split locks per clock controller this would be:
>> A:                            B:
>> lock(regmap)
>>                               lock(s2mps11)
>> lock(i2c/exynos)
>>                               lock(regmap) - wait
>> do the transfer
>> unlock(i2c/exynos)
>> unlock(regmap)
>>                               lock(regmap) - acquired
>>                               lock(i2c/exynos)
>>                               do the transfer
>>                               unlock(i2c/exynos)
>>                               unlock(regmap)
>>                               unlock(s2mps11)
>>
> 
> Yes, splitting the lock per controller will fix the possible deadlock in
> this case but I think we need an approach that is safe for all possible
> scenarios. Otherwise it will work more by coincidence than due a design.

This is not a coincidence. This design is meant to fix this deadlock.
Not by coincidence. By design.

You are talking about theoretical different configurations... without
even real bug reports. I am providing an idea to fix a real deadlock and
your argument is that it might not fix other (non-reported) deadlocks.
These other deadlocks happen now as well probably...

Best regards,
Krzysztof

Re: clk: Per controller locks (prepare & enable)

[Javier Martinez Canillas]

Hello Krzysztof,

On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote:
> On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:

[snip]

>>
>> Yes, splitting the lock per controller will fix the possible deadlock in
>> this case but I think we need an approach that is safe for all possible
>> scenarios. Otherwise it will work more by coincidence than due a design.
> 
> This is not a coincidence. This design is meant to fix this deadlock.
> Not by coincidence. By design.
>

Ok, if the configurations I described doesn't exist in practice and are
just theoretical then yes, doing a per controller lock is a good design. 
 
> You are talking about theoretical different configurations... without
> even real bug reports. I am providing an idea to fix a real deadlock and
> your argument is that it might not fix other (non-reported) deadlocks.
> These other deadlocks happen now as well probably...
>

I'm not against you re-working the locks to do it per controller, is just
that I thought it would be good to have a solution that is going to work
for all possible scenarios.

You asked for comments/opinions/ideas and I gave mine, that's all :)

> Best regards,
> Krzysztof
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: clk: Per controller locks (prepare & enable)

[Charles Keepax]

On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote:
> > On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:
> 
> [snip]

I have also been have a brief look at this as we have been
encountering issues attempting to move some of the clocking on
our audio CODECs to the clock framework. The problems are even
worse when the device can be controlled over SPI as well, as the
SPI framework may occasionally defer the transfer to a worker
thread rather than doing it in the same thread which causes the
re-enterant behaviour of the clock locking to no longer function.

> 
> >>
> >> Yes, splitting the lock per controller will fix the possible deadlock in
> >> this case but I think we need an approach that is safe for all possible
> >> scenarios. Otherwise it will work more by coincidence than due a design.
> > 
> > This is not a coincidence. This design is meant to fix this deadlock.
> > Not by coincidence. By design.
> >
> 

I think there is still some milage in thinking about them just
to be sure, if we are going to the effort of changing the clock
framework locking it is worth getting it right as it will be
non-trivial.

I could perhaps imagine a situation where one device is passing
a clock to second device and that device is doing some FLL/PLL
and passing the resulting clock back. For example supplying a
non-audio rate clock to a CODEC which then supplies back a clock
at an audio rate, which is used for audio functions on the first
device.

Although that said I do think that by controller locking probably
fixes my primary issues right now.

> Ok, if the configurations I described doesn't exist in practice and are
> just theoretical then yes, doing a per controller lock is a good design. 
>  
> > You are talking about theoretical different configurations... without
> > even real bug reports. I am providing an idea to fix a real deadlock and
> > your argument is that it might not fix other (non-reported) deadlocks.
> > These other deadlocks happen now as well probably...
> >
> 
> I'm not against you re-working the locks to do it per controller, is just
> that I thought it would be good to have a solution that is going to work
> for all possible scenarios.
> 
> You asked for comments/opinions/ideas and I gave mine, that's all :)
> 

I had also been leaning more towards a lock per clock rather
than a lock per controller. But one other issue that needs to be
kept in mind (with both the controller or clock based locking)
through is that the enable and prepare operations propagate down
the clock tree, where as the set rate operations propagate up the
clock tree.  This makes things a rather furtile breeding ground
for mutex inversions as well.

Thanks,
Charles

Re: clk: Per controller locks (prepare & enable)

[Krzysztof Kozlowski]

On 07/07/2016 02:06 PM, Charles Keepax wrote:
> On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote:
>> Hello Krzysztof,
>>
>> On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote:
>>> On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:
>>
>> [snip]
> 
> I have also been have a brief look at this as we have been
> encountering issues attempting to move some of the clocking on
> our audio CODECs to the clock framework. The problems are even
> worse when the device can be controlled over SPI as well, as the
> SPI framework may occasionally defer the transfer to a worker
> thread rather than doing it in the same thread which causes the
> re-enterant behaviour of the clock locking to no longer function.

As you mentioned later, in such case per-controller-lock won't help.

> 
>>
>>>>
>>>> Yes, splitting the lock per controller will fix the possible deadlock in
>>>> this case but I think we need an approach that is safe for all possible
>>>> scenarios. Otherwise it will work more by coincidence than due a design.
>>>
>>> This is not a coincidence. This design is meant to fix this deadlock.
>>> Not by coincidence. By design.
>>>
>>
> 
> I think there is still some milage in thinking about them just
> to be sure, if we are going to the effort of changing the clock
> framework locking it is worth getting it right as it will be
> non-trivial.
> 
> I could perhaps imagine a situation where one device is passing
> a clock to second device and that device is doing some FLL/PLL
> and passing the resulting clock back. For example supplying a
> non-audio rate clock to a CODEC which then supplies back a clock
> at an audio rate, which is used for audio functions on the first
> device.

What do you think by "passing" here? Pass the pointer to struct?

> 
> Although that said I do think that by controller locking probably
> fixes my primary issues right now.
> 
>> Ok, if the configurations I described doesn't exist in practice and are
>> just theoretical then yes, doing a per controller lock is a good design. 
>>  
>>> You are talking about theoretical different configurations... without
>>> even real bug reports. I am providing an idea to fix a real deadlock and
>>> your argument is that it might not fix other (non-reported) deadlocks.
>>> These other deadlocks happen now as well probably...
>>>
>>
>> I'm not against you re-working the locks to do it per controller, is just
>> that I thought it would be good to have a solution that is going to work
>> for all possible scenarios.
>>
>> You asked for comments/opinions/ideas and I gave mine, that's all :)
>>
> 
> I had also been leaning more towards a lock per clock rather
> than a lock per controller. But one other issue that needs to be
> kept in mind (with both the controller or clock based locking)
> through is that the enable and prepare operations propagate down
> the clock tree, where as the set rate operations propagate up the
> clock tree.  This makes things a rather furtile breeding ground
> for mutex inversions as well.
> 

Yeah, that is the problem we were thinking about just a sec ago. :) The
set rate (and reparent which might cause set rate) is complicating the
design.

Idea I have is (simplifying only to prepare lock... leave away the enable):
1. Hava a global lock which will protect:
a. traversing clock controller hierarchy,
b. acquiring per clock controller locks,
2. Add struct for clock controller.
3. Add lock per clock controller.

The basic locking in case of prepare for a simplified case one clock per
clock controller:

A (top controller = top clock)
\-B
  \-C

clk_prepare(C) {
  global_lock();
  for (clk_ctrl = C) {
    lock(clk_ctrl);
    clk_ctrl = get_parent_controller(C);
  }
  global_unlock();

  prepare_cnt++;
  // do the same for hierarchy

  for (clk_ctrl = C) {
    unlock(clk_ctrl)
    clock = get_parent_controller(C);
  }
}

The global lock is actually needed for the case of inverted walk during
set_rate().

Best regards,
Krzysztof

Re: clk: Per controller locks (prepare & enable)

[Charles Keepax]

On Thu, Jul 07, 2016 at 02:42:17PM +0200, Krzysztof Kozlowski wrote:
> On 07/07/2016 02:06 PM, Charles Keepax wrote:
> > On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote:
> >> Hello Krzysztof,
> >>
> >> On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote:
> >>> On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:
> > I have also been have a brief look at this as we have been
> > encountering issues attempting to move some of the clocking on
> > our audio CODECs to the clock framework. The problems are even
> > worse when the device can be controlled over SPI as well, as the
> > SPI framework may occasionally defer the transfer to a worker
> > thread rather than doing it in the same thread which causes the
> > re-enterant behaviour of the clock locking to no longer function.
> 
> As you mentioned later, in such case per-controller-lock won't help.
> 

It should help as the SPI clocks and the (in this case) CODEC
clocks are unlikely to be on the same controller.

> > I could perhaps imagine a situation where one device is passing
> > a clock to second device and that device is doing some FLL/PLL
> > and passing the resulting clock back. For example supplying a
> > non-audio rate clock to a CODEC which then supplies back a clock
> > at an audio rate, which is used for audio functions on the first
> > device.
> 
> What do you think by "passing" here? Pass the pointer to struct?
> 

Apologies for being unclear there, I was really just referring to
where the source for each clock is coming from. Given controllers
C1 and C2, and putting the clock in brackets afterwards:

C1(MCLK@26MHz) is the parent of C2(FLL@24.576MHz) which is the parent
of C1(AUDIO@24.576MHz). Which makes C2 both a parent and child of
C1. Its probably not that likely but I could see it happening.

> > I had also been leaning more towards a lock per clock rather
> > than a lock per controller. But one other issue that needs to be
> > kept in mind (with both the controller or clock based locking)
> > through is that the enable and prepare operations propagate down
> > the clock tree, where as the set rate operations propagate up the
> > clock tree.  This makes things a rather furtile breeding ground
> > for mutex inversions as well.
> > 
> 
> Yeah, that is the problem we were thinking about just a sec ago. :) The
> set rate (and reparent which might cause set rate) is complicating the
> design.
> 
> Idea I have is (simplifying only to prepare lock... leave away the enable):

Certainly I think only worrying about prepare makes sense.

> 1. Hava a global lock which will protect:
> a. traversing clock controller hierarchy,
> b. acquiring per clock controller locks,
> 2. Add struct for clock controller.
> 3. Add lock per clock controller.
> 
> The basic locking in case of prepare for a simplified case one clock per
> clock controller:
> 
> A (top controller = top clock)
> \-B
>   \-C
> 
> clk_prepare(C) {
>   global_lock();
>   for (clk_ctrl = C) {
>     lock(clk_ctrl);
>     clk_ctrl = get_parent_controller(C);
>   }
>   global_unlock();
> 
>   prepare_cnt++;
>   // do the same for hierarchy
> 
>   for (clk_ctrl = C) {
>     unlock(clk_ctrl)
>     clock = get_parent_controller(C);
>   }
> }

I think this fixes the issues I have been having at my side. I
will try to find some time in the next few days to go through and
refresh my memory.

I guess lets wait and see if the clock guys have any thoughts.

Thanks,
Charles