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([email protected]) which is the parent of C1([email protected]). 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
