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
      [PATCH] Revert "clk: avoid circular clock topology" Stephen Boyd <sboyd@codeaurora.org> - 2016-02-23 00:20 +0100
      Re: [PATCH v2 12/16] clk: avoid circular clock topology Joachim Eastwood <manabian@gmail.com> - 2016-02-23 00: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] | [next] | [standalone]

#1340077 — [PATCH] Revert "clk: avoid circular clock topology"

This reverts commit 858d5881564026cbc4e6f5e25ae878a27df5d4c9.

Joachim reports that this commit breaks lpc18xx boot. This is
because the hardware has circular clk topology where PLLs can
feed into dividers and the same dividers can feed into the PLLs.
The hardware is designed this way so that you can choose to put
the divider before the PLL or after the PLL depending on what you
configure to be the parent of the divider and what you configure
to be the parent of the PLL.

So let's drop this patch for now because we have hardware that
actually has loops. A future patch could check for circular
parents when we change parents and fail the switch, but that's
probably best left to some debugging Kconfig option so that we
don't suffer the sanity checking cost all the time.

Reported-by: Joachim Eastwood <manabian@gmail.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk.c | 40 ----------------------------------------
 1 file changed, 40 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 58ef3dab894a..51d673370d42 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2242,38 +2242,6 @@ static inline void clk_debug_unregister(struct clk_core *core)
 #endif
 
 /**
- * __clk_is_ancestor - check if a clk_core is a possible ancestor of another
- * @core: clock core
- * @ancestor: ancestor clock core
- *
- * Returns true if there is a possibility that @ancestor can be an ancestor
- * of @core, false otherwise.
- *
- * This function can be used against @core or @ancestor that has not been
- * registered yet.
- */
-static bool __clk_is_ancestor(struct clk_core *core, struct clk_core *ancestor)
-{
-	struct clk_core *parent;
-	int i;
-
-	for (i = 0; i < core->num_parents; i++) {
-		parent = clk_core_get_parent_by_index(core, i);
-		/*
-		 * If ancestor has not been added to clk_{root,orphan}_list
-		 * yet, clk_core_lookup() cannot find it.  If parent is NULL,
-		 * compare the name strings, too.
-		 */
-		if ((parent && (parent == ancestor ||
-				__clk_is_ancestor(parent, ancestor))) ||
-		    (!parent && !strcmp(core->parent_names[i], ancestor->name)))
-			return true;
-	}
-
-	return false;
-}
-
-/**
  * __clk_core_init - initialize the data structures in a struct clk_core
  * @core:	clk_core being initialized
  *
@@ -2338,14 +2306,6 @@ static int __clk_core_init(struct clk_core *core)
 				"%s: invalid NULL in %s's .parent_names\n",
 				__func__, core->name);
 
-	/* If core is an ancestor of itself, it would make a loop. */
-	if (__clk_is_ancestor(core, core)) {
-		pr_err("%s: %s would create circular parent\n", __func__,
-		       core->name);
-		ret = -EINVAL;
-		goto out;
-	}
-
 	core->parent = __clk_init_parent(core);
 
 	/*
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1340083

On 22 February 2016 at 03:29, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web