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

From: Charles Keepax
Date: Thu Jul 07 2016 - 12:01:01 EST


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@xxxxxxxxx) which is the parent
of C1(AUDIO@xxxxxxxxx). 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