Re: [PATCH v2 12/16] clk: avoid circular clock topology

[Joachim Eastwood]

On 22 February 2016 at 03:29, Masahiro Yamada
<yamada.masahiro@xxxxxxxxxxxxx> wrote:
> Hi Joachim,
>
>
> 2016-02-22 6:39 GMT+09:00 Joachim Eastwood <manabian@xxxxxxxxx>:
>> Hi everyone,
>>
>> On 28 December 2015 at 11:10, Masahiro Yamada
>> <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>>> Currently, clk_register() never checks a circular parent looping,
>>> but clock providers could register such an insane clock topology.
>>> For example, "clk_a" could have "clk_b" as a parent, and vice versa.
>>> In this case, clk_core_reparent() creates a circular parent list
>>> and __clk_recalc_accuracies() calls itself recursively forever.
>>>
>>> The core infrastructure should be kind enough to bail out, showing
>>> an appropriate error message in such a case.  This helps to easily
>>> find a bug in clock providers.  (uh, I made such a silly mistake
>>> when I was implementing my clock providers first.  I was upset
>>> because the kernel did not respond, without any error message.)
>>>
>>> This commit adds a new helper function, __clk_is_ancestor().  It
>>> returns true if the second argument is a possible ancestor of the
>>> first one.  If a clock core is a possible ancestor of itself, it
>>> would make a loop when it were registered.  That should be detected
>>> as an error.
>>
>> This commit breaks lpc18xx boot in next right now. See
>> http://marc.info/?l=linux-arm-kernel&m=145608597106087&w=2
>>
>> The Clock Generation Unit (CGU) on lpc18xx allow for circular parents
>> in hardware. While it is obliviously not a good idea to configure the
>> clocks in that manner there is nothing that stops you either.
>>
>> Please take a look at the second figure on:
>> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
>> All PLLs can feed clock into the dividers and the dividers can feed
>> clock into the PLLs.
>>
>> The reason why this is made possible in the CGU is because you can
>> then choose where to put your divider; either before the PLL or after.
>>
>
>
> Sorry for breaking your board.

No worries, that is why we have next so we can catch it before it hits
mainline :-)


> I am OK with reverting b58f75aa83fb.
>
>
>
> I guess your hardware could make clock looping for the best flexibility
> but you do not make clock looping in actual use cases.

That's right.


> Maybe, does it make sense to check the parent looping
> in clk_set_parent() or somewhere, not in clk_register()?

I think that would be a nice addition. While the CGU can certainly be
configured with loops it is indeed something that we should prevent
from happening.


regards,
Joachim Eastwood