Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1338989 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 12/16] clk: avoid circular clock topology Joachim Eastwood <manabian@gmail.com> - 2016-02-21 22:50 +0100
    Re: [PATCH v2 12/16] clk: avoid circular clock topology Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-02-22 03:40 +0100

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

Hi everyone,

On 28 December 2015 at 11:10, Masahiro Yamada
<yamada.masahiro@socionext.com> 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.


regards,
Joachim Eastwood

[toc] | [next] | [standalone]

#1339072

Hi Joachim,


2016-02-22 6:39 GMT+09:00 Joachim Eastwood <manabian@gmail.com>:
> Hi everyone,
>
> On 28 December 2015 at 11:10, Masahiro Yamada
> <yamada.masahiro@socionext.com> 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.

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.

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


-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web