On Thu, Jul 07, 2016 at 02:42:17PM +0200, Krzysztof Kozlowski wrote:

On 07/07/2016 02:06 PM, Charles Keepax wrote:

quoted

On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote:

quoted

Hello Krzysztof,

On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote:

quoted

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.

quoted

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 at 26MHz) is the parent of C2(FLL at 24.576MHz) which is the parent
of C1(AUDIO at 24.576MHz). Which makes C2 both a parent and child of
C1. Its probably not that likely but I could see it happening.

quoted

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