[PATCH] clk: prevent erronous parsing of children during rate change

[PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

In some cases, clocks can switch their parent with clk_set_rate, for
example clk_mux can do this in some cases. Current implementation of
clk_change_rate uses un-safe list iteration on the clock children, which
will cause wrong clocks to be parsed in case any of the clock children
change their parents during the change rate operation. Fixed by using
the safe list iterator instead.

The problem was detected due to some divide by zero errors generated
by clock init on dra7-evm board, see discussion under
http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
To: Mike Turquette <mturquette@linaro.org>
Reported-by: Nishanth Menon <nm@ti.com>
---
 drivers/clk/clk.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b76fa69..bacc06f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
 static void clk_change_rate(struct clk *clk)
 {
 	struct clk *child;
+	struct hlist_node *tmp;
 	unsigned long old_rate;
 	unsigned long best_parent_rate = 0;
 	bool skip_set_rate = false;
@@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
 	if (clk->notifier_count && old_rate != clk->rate)
 		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
 
-	hlist_for_each_entry(child, &clk->children, child_node) {
+	/*
+	 * Use safe iteration, as change_rate can actually swap parents
+	 * for certain clock types.
+	 */
+	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
 		/* Skip children who will be reparented to another clock */
 		if (child->new_parent && child->new_parent != clk)
 			continue;
-- 
1.7.9.5

[PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

In some cases, clocks can switch their parent with clk_set_rate, for
example clk_mux can do this in some cases. Current implementation of
clk_change_rate uses un-safe list iteration on the clock children, which
will cause wrong clocks to be parsed in case any of the clock children
change their parents during the change rate operation. Fixed by using
the safe list iterator instead.

The problem was detected due to some divide by zero errors generated
by clock init on dra7-evm board, see discussion under
http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
To: Mike Turquette <mturquette@linaro.org>
Reported-by: Nishanth Menon <nm@ti.com>
---
 drivers/clk/clk.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b76fa69..bacc06f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
 static void clk_change_rate(struct clk *clk)
 {
 	struct clk *child;
+	struct hlist_node *tmp;
 	unsigned long old_rate;
 	unsigned long best_parent_rate = 0;
 	bool skip_set_rate = false;
@@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
 	if (clk->notifier_count && old_rate != clk->rate)
 		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
 
-	hlist_for_each_entry(child, &clk->children, child_node) {
+	/*
+	 * Use safe iteration, as change_rate can actually swap parents
+	 * for certain clock types.
+	 */
+	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
 		/* Skip children who will be reparented to another clock */
 		if (child->new_parent && child->new_parent != clk)
 			continue;
-- 
1.7.9.5

[PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

In some cases, clocks can switch their parent with clk_set_rate, for
example clk_mux can do this in some cases. Current implementation of
clk_change_rate uses un-safe list iteration on the clock children, which
will cause wrong clocks to be parsed in case any of the clock children
change their parents during the change rate operation. Fixed by using
the safe list iterator instead.

The problem was detected due to some divide by zero errors generated
by clock init on dra7-evm board, see discussion under
http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
To: Mike Turquette <mturquette@linaro.org>
Reported-by: Nishanth Menon <nm@ti.com>
---
 drivers/clk/clk.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b76fa69..bacc06f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
 static void clk_change_rate(struct clk *clk)
 {
 	struct clk *child;
+	struct hlist_node *tmp;
 	unsigned long old_rate;
 	unsigned long best_parent_rate = 0;
 	bool skip_set_rate = false;
@@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
 	if (clk->notifier_count && old_rate != clk->rate)
 		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
 
-	hlist_for_each_entry(child, &clk->children, child_node) {
+	/*
+	 * Use safe iteration, as change_rate can actually swap parents
+	 * for certain clock types.
+	 */
+	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
 		/* Skip children who will be reparented to another clock */
 		if (child->new_parent && child->new_parent != clk)
 			continue;
-- 
1.7.9.5

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Nishanth Menon]

On 16:47-20140821, Tero Kristo wrote:
> In some cases, clocks can switch their parent with clk_set_rate, for
> example clk_mux can do this in some cases. Current implementation of
> clk_change_rate uses un-safe list iteration on the clock children, which
> will cause wrong clocks to be parsed in case any of the clock children
> change their parents during the change rate operation. Fixed by using
> the safe list iterator instead.
> 
> The problem was detected due to some divide by zero errors generated
> by clock init on dra7-evm board, see discussion under
> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
>

Fixes: 71472c0c06cf ("clk: add support for clock reparent on set_rate")
?

> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> To: Mike Turquette <mturquette@linaro.org>
> Reported-by: Nishanth Menon <nm@ti.com>

Tested-by: Nishanth Menon <nm@ti.com>
> ---
>  drivers/clk/clk.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b76fa69..bacc06f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>  static void clk_change_rate(struct clk *clk)
>  {
>  	struct clk *child;
> +	struct hlist_node *tmp;
>  	unsigned long old_rate;
>  	unsigned long best_parent_rate = 0;
>  	bool skip_set_rate = false;
> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>  	if (clk->notifier_count && old_rate != clk->rate)
>  		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>  
> -	hlist_for_each_entry(child, &clk->children, child_node) {
> +	/*
> +	 * Use safe iteration, as change_rate can actually swap parents
> +	 * for certain clock types.
> +	 */
> +	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>  		/* Skip children who will be reparented to another clock */
>  		if (child->new_parent && child->new_parent != clk)
>  			continue;
> -- 
> 1.7.9.5
> 

-- 
Regards,
Nishanth Menon

[PATCH] clk: prevent erronous parsing of children during rate change

[Nishanth Menon]

On 16:47-20140821, Tero Kristo wrote:
> In some cases, clocks can switch their parent with clk_set_rate, for
> example clk_mux can do this in some cases. Current implementation of
> clk_change_rate uses un-safe list iteration on the clock children, which
> will cause wrong clocks to be parsed in case any of the clock children
> change their parents during the change rate operation. Fixed by using
> the safe list iterator instead.
> 
> The problem was detected due to some divide by zero errors generated
> by clock init on dra7-evm board, see discussion under
> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
>

Fixes: 71472c0c06cf ("clk: add support for clock reparent on set_rate")
?

> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> To: Mike Turquette <mturquette@linaro.org>
> Reported-by: Nishanth Menon <nm@ti.com>

Tested-by: Nishanth Menon <nm@ti.com>
> ---
>  drivers/clk/clk.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b76fa69..bacc06f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>  static void clk_change_rate(struct clk *clk)
>  {
>  	struct clk *child;
> +	struct hlist_node *tmp;
>  	unsigned long old_rate;
>  	unsigned long best_parent_rate = 0;
>  	bool skip_set_rate = false;
> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>  	if (clk->notifier_count && old_rate != clk->rate)
>  		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>  
> -	hlist_for_each_entry(child, &clk->children, child_node) {
> +	/*
> +	 * Use safe iteration, as change_rate can actually swap parents
> +	 * for certain clock types.
> +	 */
> +	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>  		/* Skip children who will be reparented to another clock */
>  		if (child->new_parent && child->new_parent != clk)
>  			continue;
> -- 
> 1.7.9.5
> 

-- 
Regards,
Nishanth Menon

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Nishanth Menon]

On 16:47-20140821, Tero Kristo wrote:
> In some cases, clocks can switch their parent with clk_set_rate, for
> example clk_mux can do this in some cases. Current implementation of
> clk_change_rate uses un-safe list iteration on the clock children, which
> will cause wrong clocks to be parsed in case any of the clock children
> change their parents during the change rate operation. Fixed by using
> the safe list iterator instead.
> 
> The problem was detected due to some divide by zero errors generated
> by clock init on dra7-evm board, see discussion under
> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
>

Fixes: 71472c0c06cf ("clk: add support for clock reparent on set_rate")
?

> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> To: Mike Turquette <mturquette@linaro.org>
> Reported-by: Nishanth Menon <nm@ti.com>

Tested-by: Nishanth Menon <nm@ti.com>
> ---
>  drivers/clk/clk.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b76fa69..bacc06f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>  static void clk_change_rate(struct clk *clk)
>  {
>  	struct clk *child;
> +	struct hlist_node *tmp;
>  	unsigned long old_rate;
>  	unsigned long best_parent_rate = 0;
>  	bool skip_set_rate = false;
> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>  	if (clk->notifier_count && old_rate != clk->rate)
>  		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>  
> -	hlist_for_each_entry(child, &clk->children, child_node) {
> +	/*
> +	 * Use safe iteration, as change_rate can actually swap parents
> +	 * for certain clock types.
> +	 */
> +	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>  		/* Skip children who will be reparented to another clock */
>  		if (child->new_parent && child->new_parent != clk)
>  			continue;
> -- 
> 1.7.9.5
> 

-- 
Regards,
Nishanth Menon

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Mike Turquette]

Quoting Tero Kristo (2014-08-21 06:47:45)
> In some cases, clocks can switch their parent with clk_set_rate, for
> example clk_mux can do this in some cases. Current implementation of
> clk_change_rate uses un-safe list iteration on the clock children, which
> will cause wrong clocks to be parsed in case any of the clock children
> change their parents during the change rate operation. Fixed by using
> the safe list iterator instead.
> 
> The problem was detected due to some divide by zero errors generated
> by clock init on dra7-evm board, see discussion under
> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> To: Mike Turquette <mturquette@linaro.org>
> Reported-by: Nishanth Menon <nm@ti.com>

Applied to clk-fixes.

Thanks!
Mike

> ---
>  drivers/clk/clk.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b76fa69..bacc06f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>  static void clk_change_rate(struct clk *clk)
>  {
>         struct clk *child;
> +       struct hlist_node *tmp;
>         unsigned long old_rate;
>         unsigned long best_parent_rate = 0;
>         bool skip_set_rate = false;
> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>         if (clk->notifier_count && old_rate != clk->rate)
>                 __clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>  
> -       hlist_for_each_entry(child, &clk->children, child_node) {
> +       /*
> +        * Use safe iteration, as change_rate can actually swap parents
> +        * for certain clock types.
> +        */
> +       hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>                 /* Skip children who will be reparented to another clock */
>                 if (child->new_parent && child->new_parent != clk)
>                         continue;
> -- 
> 1.7.9.5
> 

[PATCH] clk: prevent erronous parsing of children during rate change

[Mike Turquette]

Quoting Tero Kristo (2014-08-21 06:47:45)
> In some cases, clocks can switch their parent with clk_set_rate, for
> example clk_mux can do this in some cases. Current implementation of
> clk_change_rate uses un-safe list iteration on the clock children, which
> will cause wrong clocks to be parsed in case any of the clock children
> change their parents during the change rate operation. Fixed by using
> the safe list iterator instead.
> 
> The problem was detected due to some divide by zero errors generated
> by clock init on dra7-evm board, see discussion under
> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> To: Mike Turquette <mturquette@linaro.org>
> Reported-by: Nishanth Menon <nm@ti.com>

Applied to clk-fixes.

Thanks!
Mike

> ---
>  drivers/clk/clk.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b76fa69..bacc06f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>  static void clk_change_rate(struct clk *clk)
>  {
>         struct clk *child;
> +       struct hlist_node *tmp;
>         unsigned long old_rate;
>         unsigned long best_parent_rate = 0;
>         bool skip_set_rate = false;
> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>         if (clk->notifier_count && old_rate != clk->rate)
>                 __clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>  
> -       hlist_for_each_entry(child, &clk->children, child_node) {
> +       /*
> +        * Use safe iteration, as change_rate can actually swap parents
> +        * for certain clock types.
> +        */
> +       hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>                 /* Skip children who will be reparented to another clock */
>                 if (child->new_parent && child->new_parent != clk)
>                         continue;
> -- 
> 1.7.9.5
> 

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

Hi Mike,

On Wed, Sep 03, 2014 at 12:22:03PM -0700, Mike Turquette wrote:
> Quoting Tero Kristo (2014-08-21 06:47:45)
> > In some cases, clocks can switch their parent with clk_set_rate, for
> > example clk_mux can do this in some cases. Current implementation of
> > clk_change_rate uses un-safe list iteration on the clock children, which
> > will cause wrong clocks to be parsed in case any of the clock children
> > change their parents during the change rate operation. Fixed by using
> > the safe list iterator instead.
> > 
> > The problem was detected due to some divide by zero errors generated
> > by clock init on dra7-evm board, see discussion under
> > http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
> > 
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > To: Mike Turquette <mturquette@linaro.org>
> > Reported-by: Nishanth Menon <nm@ti.com>
> 
> Applied to clk-fixes.

v3.17-rc5 and today's next still exhibit the same bug. Any chance we can
this fix into v3.17-final ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] clk: prevent erronous parsing of children during rate change

[Felipe Balbi]

Hi Mike,

On Wed, Sep 03, 2014 at 12:22:03PM -0700, Mike Turquette wrote:
> Quoting Tero Kristo (2014-08-21 06:47:45)
> > In some cases, clocks can switch their parent with clk_set_rate, for
> > example clk_mux can do this in some cases. Current implementation of
> > clk_change_rate uses un-safe list iteration on the clock children, which
> > will cause wrong clocks to be parsed in case any of the clock children
> > change their parents during the change rate operation. Fixed by using
> > the safe list iterator instead.
> > 
> > The problem was detected due to some divide by zero errors generated
> > by clock init on dra7-evm board, see discussion under
> > http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
> > 
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > To: Mike Turquette <mturquette@linaro.org>
> > Reported-by: Nishanth Menon <nm@ti.com>
> 
> Applied to clk-fixes.

v3.17-rc5 and today's next still exhibit the same bug. Any chance we can
this fix into v3.17-final ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140917/01cf6821/attachment.sig>

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

Hi Mike,

On Wed, Sep 03, 2014 at 12:22:03PM -0700, Mike Turquette wrote:
> Quoting Tero Kristo (2014-08-21 06:47:45)
> > In some cases, clocks can switch their parent with clk_set_rate, for
> > example clk_mux can do this in some cases. Current implementation of
> > clk_change_rate uses un-safe list iteration on the clock children, which
> > will cause wrong clocks to be parsed in case any of the clock children
> > change their parents during the change rate operation. Fixed by using
> > the safe list iterator instead.
> > 
> > The problem was detected due to some divide by zero errors generated
> > by clock init on dra7-evm board, see discussion under
> > http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
> > 
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > To: Mike Turquette <mturquette@linaro.org>
> > Reported-by: Nishanth Menon <nm@ti.com>
> 
> Applied to clk-fixes.

v3.17-rc5 and today's next still exhibit the same bug. Any chance we can
this fix into v3.17-final ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Stephen Boyd]

On 08/21, Tero Kristo wrote:
> In some cases, clocks can switch their parent with clk_set_rate, for
> example clk_mux can do this in some cases. Current implementation of
> clk_change_rate uses un-safe list iteration on the clock children, which
> will cause wrong clocks to be parsed in case any of the clock children
> change their parents during the change rate operation. Fixed by using
> the safe list iterator instead.
> 
> The problem was detected due to some divide by zero errors generated
> by clock init on dra7-evm board, see discussion under
> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> To: Mike Turquette <mturquette@linaro.org>
> Reported-by: Nishanth Menon <nm@ti.com>
> ---
>  drivers/clk/clk.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b76fa69..bacc06f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>  static void clk_change_rate(struct clk *clk)
>  {
>  	struct clk *child;
> +	struct hlist_node *tmp;
>  	unsigned long old_rate;
>  	unsigned long best_parent_rate = 0;
>  	bool skip_set_rate = false;
> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>  	if (clk->notifier_count && old_rate != clk->rate)
>  		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>  
> -	hlist_for_each_entry(child, &clk->children, child_node) {
> +	/*
> +	 * Use safe iteration, as change_rate can actually swap parents
> +	 * for certain clock types.
> +	 */
> +	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>  		/* Skip children who will be reparented to another clock */
>  		if (child->new_parent && child->new_parent != clk)
>  			continue;

Are we not hitting the new_parent check here? I don't understand
how we can be changing parents here unless the check is being
avoided, in which case I wonder why determine_rate isn't being
used.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] clk: prevent erronous parsing of children during rate change

[Stephen Boyd]

On 08/21, Tero Kristo wrote:
> In some cases, clocks can switch their parent with clk_set_rate, for
> example clk_mux can do this in some cases. Current implementation of
> clk_change_rate uses un-safe list iteration on the clock children, which
> will cause wrong clocks to be parsed in case any of the clock children
> change their parents during the change rate operation. Fixed by using
> the safe list iterator instead.
> 
> The problem was detected due to some divide by zero errors generated
> by clock init on dra7-evm board, see discussion under
> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> To: Mike Turquette <mturquette@linaro.org>
> Reported-by: Nishanth Menon <nm@ti.com>
> ---
>  drivers/clk/clk.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b76fa69..bacc06f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>  static void clk_change_rate(struct clk *clk)
>  {
>  	struct clk *child;
> +	struct hlist_node *tmp;
>  	unsigned long old_rate;
>  	unsigned long best_parent_rate = 0;
>  	bool skip_set_rate = false;
> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>  	if (clk->notifier_count && old_rate != clk->rate)
>  		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>  
> -	hlist_for_each_entry(child, &clk->children, child_node) {
> +	/*
> +	 * Use safe iteration, as change_rate can actually swap parents
> +	 * for certain clock types.
> +	 */
> +	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>  		/* Skip children who will be reparented to another clock */
>  		if (child->new_parent && child->new_parent != clk)
>  			continue;

Are we not hitting the new_parent check here? I don't understand
how we can be changing parents here unless the check is being
avoided, in which case I wonder why determine_rate isn't being
used.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> On 08/21, Tero Kristo wrote:
>> In some cases, clocks can switch their parent with clk_set_rate, for
>> example clk_mux can do this in some cases. Current implementation of
>> clk_change_rate uses un-safe list iteration on the clock children, which
>> will cause wrong clocks to be parsed in case any of the clock children
>> change their parents during the change rate operation. Fixed by using
>> the safe list iterator instead.
>>
>> The problem was detected due to some divide by zero errors generated
>> by clock init on dra7-evm board, see discussion under
>> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> To: Mike Turquette <mturquette@linaro.org>
>> Reported-by: Nishanth Menon <nm@ti.com>
>> ---
>>   drivers/clk/clk.c |    7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index b76fa69..bacc06f 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>>   static void clk_change_rate(struct clk *clk)
>>   {
>>   	struct clk *child;
>> +	struct hlist_node *tmp;
>>   	unsigned long old_rate;
>>   	unsigned long best_parent_rate = 0;
>>   	bool skip_set_rate = false;
>> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>>   	if (clk->notifier_count && old_rate != clk->rate)
>>   		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>>
>> -	hlist_for_each_entry(child, &clk->children, child_node) {
>> +	/*
>> +	 * Use safe iteration, as change_rate can actually swap parents
>> +	 * for certain clock types.
>> +	 */
>> +	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>>   		/* Skip children who will be reparented to another clock */
>>   		if (child->new_parent && child->new_parent != clk)
>>   			continue;
>
> Are we not hitting the new_parent check here? I don't understand
> how we can be changing parents here unless the check is being
> avoided, in which case I wonder why determine_rate isn't being
> used.
>

It depends how the clock underneath handles the situation. The error I 
am seeing actually happens with a SoC specific compound clock (DPLL) 
which integrates set_rate + mux functionality into a single clock node. 
A call to the clk_set_rate changes the parent of this clock (from bypass 
clock to reference clock), in addition to changing the rate (tune the 
mul+div.) I looked at using the determine rate call with this type but 
it breaks everything up... the parent gets changed but not the clock 
rate, in addition to some other issues.

-Tero

[PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> On 08/21, Tero Kristo wrote:
>> In some cases, clocks can switch their parent with clk_set_rate, for
>> example clk_mux can do this in some cases. Current implementation of
>> clk_change_rate uses un-safe list iteration on the clock children, which
>> will cause wrong clocks to be parsed in case any of the clock children
>> change their parents during the change rate operation. Fixed by using
>> the safe list iterator instead.
>>
>> The problem was detected due to some divide by zero errors generated
>> by clock init on dra7-evm board, see discussion under
>> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> To: Mike Turquette <mturquette@linaro.org>
>> Reported-by: Nishanth Menon <nm@ti.com>
>> ---
>>   drivers/clk/clk.c |    7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index b76fa69..bacc06f 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>>   static void clk_change_rate(struct clk *clk)
>>   {
>>   	struct clk *child;
>> +	struct hlist_node *tmp;
>>   	unsigned long old_rate;
>>   	unsigned long best_parent_rate = 0;
>>   	bool skip_set_rate = false;
>> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>>   	if (clk->notifier_count && old_rate != clk->rate)
>>   		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>>
>> -	hlist_for_each_entry(child, &clk->children, child_node) {
>> +	/*
>> +	 * Use safe iteration, as change_rate can actually swap parents
>> +	 * for certain clock types.
>> +	 */
>> +	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>>   		/* Skip children who will be reparented to another clock */
>>   		if (child->new_parent && child->new_parent != clk)
>>   			continue;
>
> Are we not hitting the new_parent check here? I don't understand
> how we can be changing parents here unless the check is being
> avoided, in which case I wonder why determine_rate isn't being
> used.
>

It depends how the clock underneath handles the situation. The error I 
am seeing actually happens with a SoC specific compound clock (DPLL) 
which integrates set_rate + mux functionality into a single clock node. 
A call to the clk_set_rate changes the parent of this clock (from bypass 
clock to reference clock), in addition to changing the rate (tune the 
mul+div.) I looked at using the determine rate call with this type but 
it breaks everything up... the parent gets changed but not the clock 
rate, in addition to some other issues.

-Tero

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> On 08/21, Tero Kristo wrote:
>> In some cases, clocks can switch their parent with clk_set_rate, for
>> example clk_mux can do this in some cases. Current implementation of
>> clk_change_rate uses un-safe list iteration on the clock children, which
>> will cause wrong clocks to be parsed in case any of the clock children
>> change their parents during the change rate operation. Fixed by using
>> the safe list iterator instead.
>>
>> The problem was detected due to some divide by zero errors generated
>> by clock init on dra7-evm board, see discussion under
>> http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> To: Mike Turquette <mturquette@linaro.org>
>> Reported-by: Nishanth Menon <nm@ti.com>
>> ---
>>   drivers/clk/clk.c |    7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index b76fa69..bacc06f 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
>>   static void clk_change_rate(struct clk *clk)
>>   {
>>   	struct clk *child;
>> +	struct hlist_node *tmp;
>>   	unsigned long old_rate;
>>   	unsigned long best_parent_rate = 0;
>>   	bool skip_set_rate = false;
>> @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
>>   	if (clk->notifier_count && old_rate != clk->rate)
>>   		__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>>
>> -	hlist_for_each_entry(child, &clk->children, child_node) {
>> +	/*
>> +	 * Use safe iteration, as change_rate can actually swap parents
>> +	 * for certain clock types.
>> +	 */
>> +	hlist_for_each_entry_safe(child, tmp, &clk->children, child_node) {
>>   		/* Skip children who will be reparented to another clock */
>>   		if (child->new_parent && child->new_parent != clk)
>>   			continue;
>
> Are we not hitting the new_parent check here? I don't understand
> how we can be changing parents here unless the check is being
> avoided, in which case I wonder why determine_rate isn't being
> used.
>

It depends how the clock underneath handles the situation. The error I 
am seeing actually happens with a SoC specific compound clock (DPLL) 
which integrates set_rate + mux functionality into a single clock node. 
A call to the clk_set_rate changes the parent of this clock (from bypass 
clock to reference clock), in addition to changing the rate (tune the 
mul+div.) I looked at using the determine rate call with this type but 
it breaks everything up... the parent gets changed but not the clock 
rate, in addition to some other issues.

-Tero

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Stephen Boyd]

On 09/23/14 06:38, Tero Kristo wrote:
> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>> On 08/21, Tero Kristo wrote:
>>>           /* Skip children who will be reparented to another clock */
>>>           if (child->new_parent && child->new_parent != clk)
>>>               continue;
>>
>> Are we not hitting the new_parent check here? I don't understand
>> how we can be changing parents here unless the check is being
>> avoided, in which case I wonder why determine_rate isn't being
>> used.
>>
>
> It depends how the clock underneath handles the situation. The error I
> am seeing actually happens with a SoC specific compound clock (DPLL)
> which integrates set_rate + mux functionality into a single clock
> node. A call to the clk_set_rate changes the parent of this clock
> (from bypass clock to reference clock), in addition to changing the
> rate (tune the mul+div.) I looked at using the determine rate call
> with this type but it breaks everything up... the parent gets changed
> but not the clock rate, in addition to some other issues.

Ok. Is this omap3_noncore_dpll_set_rate()? Can we use determine_rate +
clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
allow us to do the mult+div and the parent in the same op call, although
I don't understand why setting the parent and then setting the rate is
not going to work. I'm interested in the other issues that you mentioned
too.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] clk: prevent erronous parsing of children during rate change

[Stephen Boyd]

On 09/23/14 06:38, Tero Kristo wrote:
> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>> On 08/21, Tero Kristo wrote:
>>>           /* Skip children who will be reparented to another clock */
>>>           if (child->new_parent && child->new_parent != clk)
>>>               continue;
>>
>> Are we not hitting the new_parent check here? I don't understand
>> how we can be changing parents here unless the check is being
>> avoided, in which case I wonder why determine_rate isn't being
>> used.
>>
>
> It depends how the clock underneath handles the situation. The error I
> am seeing actually happens with a SoC specific compound clock (DPLL)
> which integrates set_rate + mux functionality into a single clock
> node. A call to the clk_set_rate changes the parent of this clock
> (from bypass clock to reference clock), in addition to changing the
> rate (tune the mul+div.) I looked at using the determine rate call
> with this type but it breaks everything up... the parent gets changed
> but not the clock rate, in addition to some other issues.

Ok. Is this omap3_noncore_dpll_set_rate()? Can we use determine_rate +
clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
allow us to do the mult+div and the parent in the same op call, although
I don't understand why setting the parent and then setting the rate is
not going to work. I'm interested in the other issues that you mentioned
too.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> On 09/23/14 06:38, Tero Kristo wrote:
>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>> On 08/21, Tero Kristo wrote:
>>>>            /* Skip children who will be reparented to another clock */
>>>>            if (child->new_parent && child->new_parent != clk)
>>>>                continue;
>>>
>>> Are we not hitting the new_parent check here? I don't understand
>>> how we can be changing parents here unless the check is being
>>> avoided, in which case I wonder why determine_rate isn't being
>>> used.
>>>
>>
>> It depends how the clock underneath handles the situation. The error I
>> am seeing actually happens with a SoC specific compound clock (DPLL)
>> which integrates set_rate + mux functionality into a single clock
>> node. A call to the clk_set_rate changes the parent of this clock
>> (from bypass clock to reference clock), in addition to changing the
>> rate (tune the mul+div.) I looked at using the determine rate call
>> with this type but it breaks everything up... the parent gets changed
>> but not the clock rate, in addition to some other issues.
>
> Ok. Is this omap3_noncore_dpll_set_rate()?

Yes.

 > Can we use determine_rate +
> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> allow us to do the mult+div and the parent in the same op call, although
> I don't understand why setting the parent and then setting the rate is
> not going to work.

Well, setting parent first, then rate later causes problems with the 
DPLL ending up running with illegal (non-specified) rate, the M+N values 
are most likely wrong if you just switch from bypass clock to reference 
clock first without programming the M+N first.

  I'm interested in the other issues that you mentioned
> too.

Mostly these were side-effects from the illegal DPLL setup I guess, like 
boot hang, failed drivers etc. I didn't really investigate this that 
much as it is much more simpler just to use safe list iteration here.

-Tero

[PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> On 09/23/14 06:38, Tero Kristo wrote:
>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>> On 08/21, Tero Kristo wrote:
>>>>            /* Skip children who will be reparented to another clock */
>>>>            if (child->new_parent && child->new_parent != clk)
>>>>                continue;
>>>
>>> Are we not hitting the new_parent check here? I don't understand
>>> how we can be changing parents here unless the check is being
>>> avoided, in which case I wonder why determine_rate isn't being
>>> used.
>>>
>>
>> It depends how the clock underneath handles the situation. The error I
>> am seeing actually happens with a SoC specific compound clock (DPLL)
>> which integrates set_rate + mux functionality into a single clock
>> node. A call to the clk_set_rate changes the parent of this clock
>> (from bypass clock to reference clock), in addition to changing the
>> rate (tune the mul+div.) I looked at using the determine rate call
>> with this type but it breaks everything up... the parent gets changed
>> but not the clock rate, in addition to some other issues.
>
> Ok. Is this omap3_noncore_dpll_set_rate()?

Yes.

 > Can we use determine_rate +
> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> allow us to do the mult+div and the parent in the same op call, although
> I don't understand why setting the parent and then setting the rate is
> not going to work.

Well, setting parent first, then rate later causes problems with the 
DPLL ending up running with illegal (non-specified) rate, the M+N values 
are most likely wrong if you just switch from bypass clock to reference 
clock first without programming the M+N first.

  I'm interested in the other issues that you mentioned
> too.

Mostly these were side-effects from the illegal DPLL setup I guess, like 
boot hang, failed drivers etc. I didn't really investigate this that 
much as it is much more simpler just to use safe list iteration here.

-Tero

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> On 09/23/14 06:38, Tero Kristo wrote:
>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>> On 08/21, Tero Kristo wrote:
>>>>            /* Skip children who will be reparented to another clock */
>>>>            if (child->new_parent && child->new_parent != clk)
>>>>                continue;
>>>
>>> Are we not hitting the new_parent check here? I don't understand
>>> how we can be changing parents here unless the check is being
>>> avoided, in which case I wonder why determine_rate isn't being
>>> used.
>>>
>>
>> It depends how the clock underneath handles the situation. The error I
>> am seeing actually happens with a SoC specific compound clock (DPLL)
>> which integrates set_rate + mux functionality into a single clock
>> node. A call to the clk_set_rate changes the parent of this clock
>> (from bypass clock to reference clock), in addition to changing the
>> rate (tune the mul+div.) I looked at using the determine rate call
>> with this type but it breaks everything up... the parent gets changed
>> but not the clock rate, in addition to some other issues.
>
> Ok. Is this omap3_noncore_dpll_set_rate()?

Yes.

 > Can we use determine_rate +
> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> allow us to do the mult+div and the parent in the same op call, although
> I don't understand why setting the parent and then setting the rate is
> not going to work.

Well, setting parent first, then rate later causes problems with the 
DPLL ending up running with illegal (non-specified) rate, the M+N values 
are most likely wrong if you just switch from bypass clock to reference 
clock first without programming the M+N first.

  I'm interested in the other issues that you mentioned
> too.

Mostly these were side-effects from the illegal DPLL setup I guess, like 
boot hang, failed drivers etc. I didn't really investigate this that 
much as it is much more simpler just to use safe list iteration here.

-Tero

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Mike Turquette]

Quoting Tero Kristo (2014-09-26 00:18:55)
> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> > On 09/23/14 06:38, Tero Kristo wrote:
> >> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> >>> On 08/21, Tero Kristo wrote:
> >>>>            /* Skip children who will be reparented to another clock */
> >>>>            if (child->new_parent && child->new_parent != clk)
> >>>>                continue;
> >>>
> >>> Are we not hitting the new_parent check here? I don't understand
> >>> how we can be changing parents here unless the check is being
> >>> avoided, in which case I wonder why determine_rate isn't being
> >>> used.
> >>>
> >>
> >> It depends how the clock underneath handles the situation. The error I
> >> am seeing actually happens with a SoC specific compound clock (DPLL)
> >> which integrates set_rate + mux functionality into a single clock
> >> node. A call to the clk_set_rate changes the parent of this clock
> >> (from bypass clock to reference clock), in addition to changing the
> >> rate (tune the mul+div.) I looked at using the determine rate call
> >> with this type but it breaks everything up... the parent gets changed
> >> but not the clock rate, in addition to some other issues.
> >
> > Ok. Is this omap3_noncore_dpll_set_rate()?
> 
> Yes.
> 
>  > Can we use determine_rate +
> > clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> > allow us to do the mult+div and the parent in the same op call, although
> > I don't understand why setting the parent and then setting the rate is
> > not going to work.
> 
> Well, setting parent first, then rate later causes problems with the 
> DPLL ending up running with illegal (non-specified) rate, the M+N values 
> are most likely wrong if you just switch from bypass clock to reference 
> clock first without programming the M+N first.

I took a quick look and it still seems to me that the OMAP DPLLs are
still not modeled properly as mux clocks. Is this correct?

This issue has been lingering for a long time and we can't use
determine_rate unless that clock has multiple parents. Simply hacking
knowledge of the parent bypass clock into the .set_rate callback is not
enough.

Regards,
Mike

> 
>   I'm interested in the other issues that you mentioned
> > too.
> 
> Mostly these were side-effects from the illegal DPLL setup I guess, like 
> boot hang, failed drivers etc. I didn't really investigate this that 
> much as it is much more simpler just to use safe list iteration here.
> 
> -Tero

[PATCH] clk: prevent erronous parsing of children during rate change

[Mike Turquette]

Quoting Tero Kristo (2014-09-26 00:18:55)
> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> > On 09/23/14 06:38, Tero Kristo wrote:
> >> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> >>> On 08/21, Tero Kristo wrote:
> >>>>            /* Skip children who will be reparented to another clock */
> >>>>            if (child->new_parent && child->new_parent != clk)
> >>>>                continue;
> >>>
> >>> Are we not hitting the new_parent check here? I don't understand
> >>> how we can be changing parents here unless the check is being
> >>> avoided, in which case I wonder why determine_rate isn't being
> >>> used.
> >>>
> >>
> >> It depends how the clock underneath handles the situation. The error I
> >> am seeing actually happens with a SoC specific compound clock (DPLL)
> >> which integrates set_rate + mux functionality into a single clock
> >> node. A call to the clk_set_rate changes the parent of this clock
> >> (from bypass clock to reference clock), in addition to changing the
> >> rate (tune the mul+div.) I looked at using the determine rate call
> >> with this type but it breaks everything up... the parent gets changed
> >> but not the clock rate, in addition to some other issues.
> >
> > Ok. Is this omap3_noncore_dpll_set_rate()?
> 
> Yes.
> 
>  > Can we use determine_rate +
> > clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> > allow us to do the mult+div and the parent in the same op call, although
> > I don't understand why setting the parent and then setting the rate is
> > not going to work.
> 
> Well, setting parent first, then rate later causes problems with the 
> DPLL ending up running with illegal (non-specified) rate, the M+N values 
> are most likely wrong if you just switch from bypass clock to reference 
> clock first without programming the M+N first.

I took a quick look and it still seems to me that the OMAP DPLLs are
still not modeled properly as mux clocks. Is this correct?

This issue has been lingering for a long time and we can't use
determine_rate unless that clock has multiple parents. Simply hacking
knowledge of the parent bypass clock into the .set_rate callback is not
enough.

Regards,
Mike

> 
>   I'm interested in the other issues that you mentioned
> > too.
> 
> Mostly these were side-effects from the illegal DPLL setup I guess, like 
> boot hang, failed drivers etc. I didn't really investigate this that 
> much as it is much more simpler just to use safe list iteration here.
> 
> -Tero

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/27/2014 02:24 AM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-26 00:18:55)
>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>> On 08/21, Tero Kristo wrote:
>>>>>>             /* Skip children who will be reparented to another clock */
>>>>>>             if (child->new_parent && child->new_parent != clk)
>>>>>>                 continue;
>>>>>
>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>> how we can be changing parents here unless the check is being
>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>> used.
>>>>>
>>>>
>>>> It depends how the clock underneath handles the situation. The error I
>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>> which integrates set_rate + mux functionality into a single clock
>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>> (from bypass clock to reference clock), in addition to changing the
>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>> with this type but it breaks everything up... the parent gets changed
>>>> but not the clock rate, in addition to some other issues.
>>>
>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>
>> Yes.
>>
>>   > Can we use determine_rate +
>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>> allow us to do the mult+div and the parent in the same op call, although
>>> I don't understand why setting the parent and then setting the rate is
>>> not going to work.
>>
>> Well, setting parent first, then rate later causes problems with the
>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>> are most likely wrong if you just switch from bypass clock to reference
>> clock first without programming the M+N first.
>
> I took a quick look and it still seems to me that the OMAP DPLLs are
> still not modeled properly as mux clocks. Is this correct?

Yeah, they are not mux clocks, but rather a compound of mux + DPLL 
multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL 
div/mult clock will still have overlapping usage of the DPLL_EN field, 
as the DPLL must be in bypass mode during M+N change. Or, should the 
DPLL rate change only be allowed if the mux is in bypass setting? 
Several drivers still depend on direct dpll clk_set_rate working 
'properly' (there are some other issues currently present also which 
have nothing to do with the mux behavior.)

> This issue has been lingering for a long time and we can't use
> determine_rate unless that clock has multiple parents. Simply hacking
> knowledge of the parent bypass clock into the .set_rate callback is not
> enough.

If you believe this _must_ be changed, I can take a look at this for 
next merge window, but this will cause a DT data compatibility break if 
nothing else (personally I don't care about this as I always rebuild DT 
blob with kernel, but lots of other people seem to do.)

-Tero

>
> Regards,
> Mike
>
>>
>>    I'm interested in the other issues that you mentioned
>>> too.
>>
>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>> boot hang, failed drivers etc. I didn't really investigate this that
>> much as it is much more simpler just to use safe list iteration here.
>>
>> -Tero

[PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/27/2014 02:24 AM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-26 00:18:55)
>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>> On 08/21, Tero Kristo wrote:
>>>>>>             /* Skip children who will be reparented to another clock */
>>>>>>             if (child->new_parent && child->new_parent != clk)
>>>>>>                 continue;
>>>>>
>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>> how we can be changing parents here unless the check is being
>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>> used.
>>>>>
>>>>
>>>> It depends how the clock underneath handles the situation. The error I
>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>> which integrates set_rate + mux functionality into a single clock
>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>> (from bypass clock to reference clock), in addition to changing the
>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>> with this type but it breaks everything up... the parent gets changed
>>>> but not the clock rate, in addition to some other issues.
>>>
>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>
>> Yes.
>>
>>   > Can we use determine_rate +
>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>> allow us to do the mult+div and the parent in the same op call, although
>>> I don't understand why setting the parent and then setting the rate is
>>> not going to work.
>>
>> Well, setting parent first, then rate later causes problems with the
>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>> are most likely wrong if you just switch from bypass clock to reference
>> clock first without programming the M+N first.
>
> I took a quick look and it still seems to me that the OMAP DPLLs are
> still not modeled properly as mux clocks. Is this correct?

Yeah, they are not mux clocks, but rather a compound of mux + DPLL 
multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL 
div/mult clock will still have overlapping usage of the DPLL_EN field, 
as the DPLL must be in bypass mode during M+N change. Or, should the 
DPLL rate change only be allowed if the mux is in bypass setting? 
Several drivers still depend on direct dpll clk_set_rate working 
'properly' (there are some other issues currently present also which 
have nothing to do with the mux behavior.)

> This issue has been lingering for a long time and we can't use
> determine_rate unless that clock has multiple parents. Simply hacking
> knowledge of the parent bypass clock into the .set_rate callback is not
> enough.

If you believe this _must_ be changed, I can take a look at this for 
next merge window, but this will cause a DT data compatibility break if 
nothing else (personally I don't care about this as I always rebuild DT 
blob with kernel, but lots of other people seem to do.)

-Tero

>
> Regards,
> Mike
>
>>
>>    I'm interested in the other issues that you mentioned
>>> too.
>>
>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>> boot hang, failed drivers etc. I didn't really investigate this that
>> much as it is much more simpler just to use safe list iteration here.
>>
>> -Tero

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/27/2014 02:24 AM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-26 00:18:55)
>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>> On 08/21, Tero Kristo wrote:
>>>>>>             /* Skip children who will be reparented to another clock */
>>>>>>             if (child->new_parent && child->new_parent != clk)
>>>>>>                 continue;
>>>>>
>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>> how we can be changing parents here unless the check is being
>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>> used.
>>>>>
>>>>
>>>> It depends how the clock underneath handles the situation. The error I
>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>> which integrates set_rate + mux functionality into a single clock
>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>> (from bypass clock to reference clock), in addition to changing the
>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>> with this type but it breaks everything up... the parent gets changed
>>>> but not the clock rate, in addition to some other issues.
>>>
>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>
>> Yes.
>>
>>   > Can we use determine_rate +
>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>> allow us to do the mult+div and the parent in the same op call, although
>>> I don't understand why setting the parent and then setting the rate is
>>> not going to work.
>>
>> Well, setting parent first, then rate later causes problems with the
>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>> are most likely wrong if you just switch from bypass clock to reference
>> clock first without programming the M+N first.
>
> I took a quick look and it still seems to me that the OMAP DPLLs are
> still not modeled properly as mux clocks. Is this correct?

Yeah, they are not mux clocks, but rather a compound of mux + DPLL 
multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL 
div/mult clock will still have overlapping usage of the DPLL_EN field, 
as the DPLL must be in bypass mode during M+N change. Or, should the 
DPLL rate change only be allowed if the mux is in bypass setting? 
Several drivers still depend on direct dpll clk_set_rate working 
'properly' (there are some other issues currently present also which 
have nothing to do with the mux behavior.)

> This issue has been lingering for a long time and we can't use
> determine_rate unless that clock has multiple parents. Simply hacking
> knowledge of the parent bypass clock into the .set_rate callback is not
> enough.

If you believe this _must_ be changed, I can take a look at this for 
next merge window, but this will cause a DT data compatibility break if 
nothing else (personally I don't care about this as I always rebuild DT 
blob with kernel, but lots of other people seem to do.)

-Tero

>
> Regards,
> Mike
>
>>
>>    I'm interested in the other issues that you mentioned
>>> too.
>>
>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>> boot hang, failed drivers etc. I didn't really investigate this that
>> much as it is much more simpler just to use safe list iteration here.
>>
>> -Tero

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Mike Turquette]

Quoting Tero Kristo (2014-09-29 01:09:24)
> On 09/27/2014 02:24 AM, Mike Turquette wrote:
> > Quoting Tero Kristo (2014-09-26 00:18:55)
> >> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> >>> On 09/23/14 06:38, Tero Kristo wrote:
> >>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> >>>>> On 08/21, Tero Kristo wrote:
> >>>>>>             /* Skip children who will be reparented to another clock */
> >>>>>>             if (child->new_parent && child->new_parent != clk)
> >>>>>>                 continue;
> >>>>>
> >>>>> Are we not hitting the new_parent check here? I don't understand
> >>>>> how we can be changing parents here unless the check is being
> >>>>> avoided, in which case I wonder why determine_rate isn't being
> >>>>> used.
> >>>>>
> >>>>
> >>>> It depends how the clock underneath handles the situation. The error I
> >>>> am seeing actually happens with a SoC specific compound clock (DPLL)
> >>>> which integrates set_rate + mux functionality into a single clock
> >>>> node. A call to the clk_set_rate changes the parent of this clock
> >>>> (from bypass clock to reference clock), in addition to changing the
> >>>> rate (tune the mul+div.) I looked at using the determine rate call
> >>>> with this type but it breaks everything up... the parent gets changed
> >>>> but not the clock rate, in addition to some other issues.
> >>>
> >>> Ok. Is this omap3_noncore_dpll_set_rate()?
> >>
> >> Yes.
> >>
> >>   > Can we use determine_rate +
> >>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> >>> allow us to do the mult+div and the parent in the same op call, although
> >>> I don't understand why setting the parent and then setting the rate is
> >>> not going to work.
> >>
> >> Well, setting parent first, then rate later causes problems with the
> >> DPLL ending up running with illegal (non-specified) rate, the M+N values
> >> are most likely wrong if you just switch from bypass clock to reference
> >> clock first without programming the M+N first.
> >
> > I took a quick look and it still seems to me that the OMAP DPLLs are
> > still not modeled properly as mux clocks. Is this correct?
> 
> Yeah, they are not mux clocks, but rather a compound of mux + DPLL 
> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL 
> div/mult clock will still have overlapping usage of the DPLL_EN field, 

I'm not talking about splitting up the clock into two separate clocks.
If memory serves the DPLL clock implementation "cheats" and hides the
bypass_clk info from the clock framework. To be explicit, from the
perspective of Linux clock framework DPLL clocks only have one parent.

In reality a typical DPLL should have at least 2 parents (and in some
cases starting with OMAP4, some of the DPLL output clocks should have a
second HSD parent). But the implementation does not reflect this.

> as the DPLL must be in bypass mode during M+N change. Or, should the 
> DPLL rate change only be allowed if the mux is in bypass setting? 
> Several drivers still depend on direct dpll clk_set_rate working 
> 'properly' (there are some other issues currently present also which 
> have nothing to do with the mux behavior.)
> 
> > This issue has been lingering for a long time and we can't use
> > determine_rate unless that clock has multiple parents. Simply hacking
> > knowledge of the parent bypass clock into the .set_rate callback is not
> > enough.
> 
> If you believe this _must_ be changed, I can take a look at this for 
> next merge window, but this will cause a DT data compatibility break if 
> nothing else (personally I don't care about this as I always rebuild DT 
> blob with kernel, but lots of other people seem to do.)

Well I guess the question is how long will we put up with the many small
headaches caused by incorrectly modeling the clock?

determine_rate and clk_set_parent_and_rate should be sufficient for the
OMAP DPLLs but only if they are correctly modeled in the framework.

Regards,
Mike

> 
> -Tero
> 
> >
> > Regards,
> > Mike
> >
> >>
> >>    I'm interested in the other issues that you mentioned
> >>> too.
> >>
> >> Mostly these were side-effects from the illegal DPLL setup I guess, like
> >> boot hang, failed drivers etc. I didn't really investigate this that
> >> much as it is much more simpler just to use safe list iteration here.
> >>
> >> -Tero
> 

[PATCH] clk: prevent erronous parsing of children during rate change

[Mike Turquette]

Quoting Tero Kristo (2014-09-29 01:09:24)
> On 09/27/2014 02:24 AM, Mike Turquette wrote:
> > Quoting Tero Kristo (2014-09-26 00:18:55)
> >> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> >>> On 09/23/14 06:38, Tero Kristo wrote:
> >>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> >>>>> On 08/21, Tero Kristo wrote:
> >>>>>>             /* Skip children who will be reparented to another clock */
> >>>>>>             if (child->new_parent && child->new_parent != clk)
> >>>>>>                 continue;
> >>>>>
> >>>>> Are we not hitting the new_parent check here? I don't understand
> >>>>> how we can be changing parents here unless the check is being
> >>>>> avoided, in which case I wonder why determine_rate isn't being
> >>>>> used.
> >>>>>
> >>>>
> >>>> It depends how the clock underneath handles the situation. The error I
> >>>> am seeing actually happens with a SoC specific compound clock (DPLL)
> >>>> which integrates set_rate + mux functionality into a single clock
> >>>> node. A call to the clk_set_rate changes the parent of this clock
> >>>> (from bypass clock to reference clock), in addition to changing the
> >>>> rate (tune the mul+div.) I looked at using the determine rate call
> >>>> with this type but it breaks everything up... the parent gets changed
> >>>> but not the clock rate, in addition to some other issues.
> >>>
> >>> Ok. Is this omap3_noncore_dpll_set_rate()?
> >>
> >> Yes.
> >>
> >>   > Can we use determine_rate +
> >>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> >>> allow us to do the mult+div and the parent in the same op call, although
> >>> I don't understand why setting the parent and then setting the rate is
> >>> not going to work.
> >>
> >> Well, setting parent first, then rate later causes problems with the
> >> DPLL ending up running with illegal (non-specified) rate, the M+N values
> >> are most likely wrong if you just switch from bypass clock to reference
> >> clock first without programming the M+N first.
> >
> > I took a quick look and it still seems to me that the OMAP DPLLs are
> > still not modeled properly as mux clocks. Is this correct?
> 
> Yeah, they are not mux clocks, but rather a compound of mux + DPLL 
> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL 
> div/mult clock will still have overlapping usage of the DPLL_EN field, 

I'm not talking about splitting up the clock into two separate clocks.
If memory serves the DPLL clock implementation "cheats" and hides the
bypass_clk info from the clock framework. To be explicit, from the
perspective of Linux clock framework DPLL clocks only have one parent.

In reality a typical DPLL should have at least 2 parents (and in some
cases starting with OMAP4, some of the DPLL output clocks should have a
second HSD parent). But the implementation does not reflect this.

> as the DPLL must be in bypass mode during M+N change. Or, should the 
> DPLL rate change only be allowed if the mux is in bypass setting? 
> Several drivers still depend on direct dpll clk_set_rate working 
> 'properly' (there are some other issues currently present also which 
> have nothing to do with the mux behavior.)
> 
> > This issue has been lingering for a long time and we can't use
> > determine_rate unless that clock has multiple parents. Simply hacking
> > knowledge of the parent bypass clock into the .set_rate callback is not
> > enough.
> 
> If you believe this _must_ be changed, I can take a look at this for 
> next merge window, but this will cause a DT data compatibility break if 
> nothing else (personally I don't care about this as I always rebuild DT 
> blob with kernel, but lots of other people seem to do.)

Well I guess the question is how long will we put up with the many small
headaches caused by incorrectly modeling the clock?

determine_rate and clk_set_parent_and_rate should be sufficient for the
OMAP DPLLs but only if they are correctly modeled in the framework.

Regards,
Mike

> 
> -Tero
> 
> >
> > Regards,
> > Mike
> >
> >>
> >>    I'm interested in the other issues that you mentioned
> >>> too.
> >>
> >> Mostly these were side-effects from the illegal DPLL setup I guess, like
> >> boot hang, failed drivers etc. I didn't really investigate this that
> >> much as it is much more simpler just to use safe list iteration here.
> >>
> >> -Tero
> 

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/30/2014 10:07 AM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-29 01:09:24)
>> On 09/27/2014 02:24 AM, Mike Turquette wrote:
>>> Quoting Tero Kristo (2014-09-26 00:18:55)
>>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>>>> On 08/21, Tero Kristo wrote:
>>>>>>>>              /* Skip children who will be reparented to another clock */
>>>>>>>>              if (child->new_parent && child->new_parent != clk)
>>>>>>>>                  continue;
>>>>>>>
>>>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>>>> how we can be changing parents here unless the check is being
>>>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>>>> used.
>>>>>>>
>>>>>>
>>>>>> It depends how the clock underneath handles the situation. The error I
>>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>>>> which integrates set_rate + mux functionality into a single clock
>>>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>>>> (from bypass clock to reference clock), in addition to changing the
>>>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>>>> with this type but it breaks everything up... the parent gets changed
>>>>>> but not the clock rate, in addition to some other issues.
>>>>>
>>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>>>
>>>> Yes.
>>>>
>>>>    > Can we use determine_rate +
>>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>>>> allow us to do the mult+div and the parent in the same op call, although
>>>>> I don't understand why setting the parent and then setting the rate is
>>>>> not going to work.
>>>>
>>>> Well, setting parent first, then rate later causes problems with the
>>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>>>> are most likely wrong if you just switch from bypass clock to reference
>>>> clock first without programming the M+N first.
>>>
>>> I took a quick look and it still seems to me that the OMAP DPLLs are
>>> still not modeled properly as mux clocks. Is this correct?
>>
>> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
>> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
>> div/mult clock will still have overlapping usage of the DPLL_EN field,
>
> I'm not talking about splitting up the clock into two separate clocks.
> If memory serves the DPLL clock implementation "cheats" and hides the
> bypass_clk info from the clock framework. To be explicit, from the
> perspective of Linux clock framework DPLL clocks only have one parent.
>
> In reality a typical DPLL should have at least 2 parents (and in some
> cases starting with OMAP4, some of the DPLL output clocks should have a
> second HSD parent). But the implementation does not reflect this.

No, this is not the DPLLs are modelled. Each DPLL has currently two 
parents, ref-clk and bypass-clk, which are both modelled as separate 
clock nodes, and the DPLL switches parents based on bypass/lock mode. 
The bypass clock is also usually a mux clock, which further selects 
separate bypass parent, resulting in 3 or more parents for a certain DPLL.

>
>> as the DPLL must be in bypass mode during M+N change. Or, should the
>> DPLL rate change only be allowed if the mux is in bypass setting?
>> Several drivers still depend on direct dpll clk_set_rate working
>> 'properly' (there are some other issues currently present also which
>> have nothing to do with the mux behavior.)
>>
>>> This issue has been lingering for a long time and we can't use
>>> determine_rate unless that clock has multiple parents. Simply hacking
>>> knowledge of the parent bypass clock into the .set_rate callback is not
>>> enough.
>>
>> If you believe this _must_ be changed, I can take a look at this for
>> next merge window, but this will cause a DT data compatibility break if
>> nothing else (personally I don't care about this as I always rebuild DT
>> blob with kernel, but lots of other people seem to do.)
>
> Well I guess the question is how long will we put up with the many small
> headaches caused by incorrectly modeling the clock?

Well, its not kind of incorrectly modelled, it is just modelled in such 
way that clk_set_rate doesn't cope too well with it.

> determine_rate and clk_set_parent_and_rate should be sufficient for the
> OMAP DPLLs but only if they are correctly modeled in the framework.

Do we have implementation for clk_set_parent_and_rate someplace? I 
looked at rc7 and didn't find this. I think this would fix the issues I 
am seeing combined with determine_rate, if clk_set_rate would internally 
handle changing both rate + parent.

-Tero

>
> Regards,
> Mike
>
>>
>> -Tero
>>
>>>
>>> Regards,
>>> Mike
>>>
>>>>
>>>>     I'm interested in the other issues that you mentioned
>>>>> too.
>>>>
>>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>>>> boot hang, failed drivers etc. I didn't really investigate this that
>>>> much as it is much more simpler just to use safe list iteration here.
>>>>
>>>> -Tero
>>

[PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/30/2014 10:07 AM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-29 01:09:24)
>> On 09/27/2014 02:24 AM, Mike Turquette wrote:
>>> Quoting Tero Kristo (2014-09-26 00:18:55)
>>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>>>> On 08/21, Tero Kristo wrote:
>>>>>>>>              /* Skip children who will be reparented to another clock */
>>>>>>>>              if (child->new_parent && child->new_parent != clk)
>>>>>>>>                  continue;
>>>>>>>
>>>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>>>> how we can be changing parents here unless the check is being
>>>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>>>> used.
>>>>>>>
>>>>>>
>>>>>> It depends how the clock underneath handles the situation. The error I
>>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>>>> which integrates set_rate + mux functionality into a single clock
>>>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>>>> (from bypass clock to reference clock), in addition to changing the
>>>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>>>> with this type but it breaks everything up... the parent gets changed
>>>>>> but not the clock rate, in addition to some other issues.
>>>>>
>>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>>>
>>>> Yes.
>>>>
>>>>    > Can we use determine_rate +
>>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>>>> allow us to do the mult+div and the parent in the same op call, although
>>>>> I don't understand why setting the parent and then setting the rate is
>>>>> not going to work.
>>>>
>>>> Well, setting parent first, then rate later causes problems with the
>>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>>>> are most likely wrong if you just switch from bypass clock to reference
>>>> clock first without programming the M+N first.
>>>
>>> I took a quick look and it still seems to me that the OMAP DPLLs are
>>> still not modeled properly as mux clocks. Is this correct?
>>
>> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
>> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
>> div/mult clock will still have overlapping usage of the DPLL_EN field,
>
> I'm not talking about splitting up the clock into two separate clocks.
> If memory serves the DPLL clock implementation "cheats" and hides the
> bypass_clk info from the clock framework. To be explicit, from the
> perspective of Linux clock framework DPLL clocks only have one parent.
>
> In reality a typical DPLL should have at least 2 parents (and in some
> cases starting with OMAP4, some of the DPLL output clocks should have a
> second HSD parent). But the implementation does not reflect this.

No, this is not the DPLLs are modelled. Each DPLL has currently two 
parents, ref-clk and bypass-clk, which are both modelled as separate 
clock nodes, and the DPLL switches parents based on bypass/lock mode. 
The bypass clock is also usually a mux clock, which further selects 
separate bypass parent, resulting in 3 or more parents for a certain DPLL.

>
>> as the DPLL must be in bypass mode during M+N change. Or, should the
>> DPLL rate change only be allowed if the mux is in bypass setting?
>> Several drivers still depend on direct dpll clk_set_rate working
>> 'properly' (there are some other issues currently present also which
>> have nothing to do with the mux behavior.)
>>
>>> This issue has been lingering for a long time and we can't use
>>> determine_rate unless that clock has multiple parents. Simply hacking
>>> knowledge of the parent bypass clock into the .set_rate callback is not
>>> enough.
>>
>> If you believe this _must_ be changed, I can take a look at this for
>> next merge window, but this will cause a DT data compatibility break if
>> nothing else (personally I don't care about this as I always rebuild DT
>> blob with kernel, but lots of other people seem to do.)
>
> Well I guess the question is how long will we put up with the many small
> headaches caused by incorrectly modeling the clock?

Well, its not kind of incorrectly modelled, it is just modelled in such 
way that clk_set_rate doesn't cope too well with it.

> determine_rate and clk_set_parent_and_rate should be sufficient for the
> OMAP DPLLs but only if they are correctly modeled in the framework.

Do we have implementation for clk_set_parent_and_rate someplace? I 
looked at rc7 and didn't find this. I think this would fix the issues I 
am seeing combined with determine_rate, if clk_set_rate would internally 
handle changing both rate + parent.

-Tero

>
> Regards,
> Mike
>
>>
>> -Tero
>>
>>>
>>> Regards,
>>> Mike
>>>
>>>>
>>>>     I'm interested in the other issues that you mentioned
>>>>> too.
>>>>
>>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>>>> boot hang, failed drivers etc. I didn't really investigate this that
>>>> much as it is much more simpler just to use safe list iteration here.
>>>>
>>>> -Tero
>>

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/30/2014 10:07 AM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-29 01:09:24)
>> On 09/27/2014 02:24 AM, Mike Turquette wrote:
>>> Quoting Tero Kristo (2014-09-26 00:18:55)
>>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>>>> On 08/21, Tero Kristo wrote:
>>>>>>>>              /* Skip children who will be reparented to another clock */
>>>>>>>>              if (child->new_parent && child->new_parent != clk)
>>>>>>>>                  continue;
>>>>>>>
>>>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>>>> how we can be changing parents here unless the check is being
>>>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>>>> used.
>>>>>>>
>>>>>>
>>>>>> It depends how the clock underneath handles the situation. The error I
>>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>>>> which integrates set_rate + mux functionality into a single clock
>>>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>>>> (from bypass clock to reference clock), in addition to changing the
>>>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>>>> with this type but it breaks everything up... the parent gets changed
>>>>>> but not the clock rate, in addition to some other issues.
>>>>>
>>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>>>
>>>> Yes.
>>>>
>>>>    > Can we use determine_rate +
>>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>>>> allow us to do the mult+div and the parent in the same op call, although
>>>>> I don't understand why setting the parent and then setting the rate is
>>>>> not going to work.
>>>>
>>>> Well, setting parent first, then rate later causes problems with the
>>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>>>> are most likely wrong if you just switch from bypass clock to reference
>>>> clock first without programming the M+N first.
>>>
>>> I took a quick look and it still seems to me that the OMAP DPLLs are
>>> still not modeled properly as mux clocks. Is this correct?
>>
>> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
>> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
>> div/mult clock will still have overlapping usage of the DPLL_EN field,
>
> I'm not talking about splitting up the clock into two separate clocks.
> If memory serves the DPLL clock implementation "cheats" and hides the
> bypass_clk info from the clock framework. To be explicit, from the
> perspective of Linux clock framework DPLL clocks only have one parent.
>
> In reality a typical DPLL should have at least 2 parents (and in some
> cases starting with OMAP4, some of the DPLL output clocks should have a
> second HSD parent). But the implementation does not reflect this.

No, this is not the DPLLs are modelled. Each DPLL has currently two 
parents, ref-clk and bypass-clk, which are both modelled as separate 
clock nodes, and the DPLL switches parents based on bypass/lock mode. 
The bypass clock is also usually a mux clock, which further selects 
separate bypass parent, resulting in 3 or more parents for a certain DPLL.

>
>> as the DPLL must be in bypass mode during M+N change. Or, should the
>> DPLL rate change only be allowed if the mux is in bypass setting?
>> Several drivers still depend on direct dpll clk_set_rate working
>> 'properly' (there are some other issues currently present also which
>> have nothing to do with the mux behavior.)
>>
>>> This issue has been lingering for a long time and we can't use
>>> determine_rate unless that clock has multiple parents. Simply hacking
>>> knowledge of the parent bypass clock into the .set_rate callback is not
>>> enough.
>>
>> If you believe this _must_ be changed, I can take a look at this for
>> next merge window, but this will cause a DT data compatibility break if
>> nothing else (personally I don't care about this as I always rebuild DT
>> blob with kernel, but lots of other people seem to do.)
>
> Well I guess the question is how long will we put up with the many small
> headaches caused by incorrectly modeling the clock?

Well, its not kind of incorrectly modelled, it is just modelled in such 
way that clk_set_rate doesn't cope too well with it.

> determine_rate and clk_set_parent_and_rate should be sufficient for the
> OMAP DPLLs but only if they are correctly modeled in the framework.

Do we have implementation for clk_set_parent_and_rate someplace? I 
looked at rc7 and didn't find this. I think this would fix the issues I 
am seeing combined with determine_rate, if clk_set_rate would internally 
handle changing both rate + parent.

-Tero

>
> Regards,
> Mike
>
>>
>> -Tero
>>
>>>
>>> Regards,
>>> Mike
>>>
>>>>
>>>>     I'm interested in the other issues that you mentioned
>>>>> too.
>>>>
>>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>>>> boot hang, failed drivers etc. I didn't really investigate this that
>>>> much as it is much more simpler just to use safe list iteration here.
>>>>
>>>> -Tero
>>

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Mike Turquette]

Quoting Tero Kristo (2014-09-30 01:48:49)
> On 09/30/2014 10:07 AM, Mike Turquette wrote:
> > Quoting Tero Kristo (2014-09-29 01:09:24)
> >> On 09/27/2014 02:24 AM, Mike Turquette wrote:
> >>> Quoting Tero Kristo (2014-09-26 00:18:55)
> >>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> >>>>> On 09/23/14 06:38, Tero Kristo wrote:
> >>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> >>>>>>> On 08/21, Tero Kristo wrote:
> >>>>>>>>              /* Skip children who will be reparented to another clock */
> >>>>>>>>              if (child->new_parent && child->new_parent != clk)
> >>>>>>>>                  continue;
> >>>>>>>
> >>>>>>> Are we not hitting the new_parent check here? I don't understand
> >>>>>>> how we can be changing parents here unless the check is being
> >>>>>>> avoided, in which case I wonder why determine_rate isn't being
> >>>>>>> used.
> >>>>>>>
> >>>>>>
> >>>>>> It depends how the clock underneath handles the situation. The error I
> >>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
> >>>>>> which integrates set_rate + mux functionality into a single clock
> >>>>>> node. A call to the clk_set_rate changes the parent of this clock
> >>>>>> (from bypass clock to reference clock), in addition to changing the
> >>>>>> rate (tune the mul+div.) I looked at using the determine rate call
> >>>>>> with this type but it breaks everything up... the parent gets changed
> >>>>>> but not the clock rate, in addition to some other issues.
> >>>>>
> >>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
> >>>>
> >>>> Yes.
> >>>>
> >>>>    > Can we use determine_rate +
> >>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> >>>>> allow us to do the mult+div and the parent in the same op call, although
> >>>>> I don't understand why setting the parent and then setting the rate is
> >>>>> not going to work.
> >>>>
> >>>> Well, setting parent first, then rate later causes problems with the
> >>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
> >>>> are most likely wrong if you just switch from bypass clock to reference
> >>>> clock first without programming the M+N first.
> >>>
> >>> I took a quick look and it still seems to me that the OMAP DPLLs are
> >>> still not modeled properly as mux clocks. Is this correct?
> >>
> >> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
> >> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
> >> div/mult clock will still have overlapping usage of the DPLL_EN field,
> >
> > I'm not talking about splitting up the clock into two separate clocks.
> > If memory serves the DPLL clock implementation "cheats" and hides the
> > bypass_clk info from the clock framework. To be explicit, from the
> > perspective of Linux clock framework DPLL clocks only have one parent.
> >
> > In reality a typical DPLL should have at least 2 parents (and in some
> > cases starting with OMAP4, some of the DPLL output clocks should have a
> > second HSD parent). But the implementation does not reflect this.
> 
> No, this is not the DPLLs are modelled. Each DPLL has currently two 
> parents, ref-clk and bypass-clk, which are both modelled as separate 
> clock nodes, and the DPLL switches parents based on bypass/lock mode. 
> The bypass clock is also usually a mux clock, which further selects 
> separate bypass parent, resulting in 3 or more parents for a certain DPLL.

I stand corrected. I thought it was still done the old way where the
machine-specific clock struct was holding the pointer to the ref_clk and
bypass_clk. I'm glad that is not the case any more.

> 
> >
> >> as the DPLL must be in bypass mode during M+N change. Or, should the
> >> DPLL rate change only be allowed if the mux is in bypass setting?
> >> Several drivers still depend on direct dpll clk_set_rate working
> >> 'properly' (there are some other issues currently present also which
> >> have nothing to do with the mux behavior.)
> >>
> >>> This issue has been lingering for a long time and we can't use
> >>> determine_rate unless that clock has multiple parents. Simply hacking
> >>> knowledge of the parent bypass clock into the .set_rate callback is not
> >>> enough.
> >>
> >> If you believe this _must_ be changed, I can take a look at this for
> >> next merge window, but this will cause a DT data compatibility break if
> >> nothing else (personally I don't care about this as I always rebuild DT
> >> blob with kernel, but lots of other people seem to do.)
> >
> > Well I guess the question is how long will we put up with the many small
> > headaches caused by incorrectly modeling the clock?
> 
> Well, its not kind of incorrectly modelled, it is just modelled in such 
> way that clk_set_rate doesn't cope too well with it.
> 
> > determine_rate and clk_set_parent_and_rate should be sufficient for the
> > OMAP DPLLs but only if they are correctly modeled in the framework.
> 
> Do we have implementation for clk_set_parent_and_rate someplace? I 
> looked at rc7 and didn't find this. I think this would fix the issues I 
> am seeing combined with determine_rate, if clk_set_rate would internally 
> handle changing both rate + parent.

I made it hard for you to find it because I typo'd. It's not a clk api
but a clk_op:

int (*set_rate_and_parent)(struct clk_hw *hw, unsigned long rate,
                           unsigned long parent_rate, u8 index);

Regards,
Mike

> 
> -Tero
> 
> >
> > Regards,
> > Mike
> >
> >>
> >> -Tero
> >>
> >>>
> >>> Regards,
> >>> Mike
> >>>
> >>>>
> >>>>     I'm interested in the other issues that you mentioned
> >>>>> too.
> >>>>
> >>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
> >>>> boot hang, failed drivers etc. I didn't really investigate this that
> >>>> much as it is much more simpler just to use safe list iteration here.
> >>>>
> >>>> -Tero
> >>
> 

[PATCH] clk: prevent erronous parsing of children during rate change

[Mike Turquette]

Quoting Tero Kristo (2014-09-30 01:48:49)
> On 09/30/2014 10:07 AM, Mike Turquette wrote:
> > Quoting Tero Kristo (2014-09-29 01:09:24)
> >> On 09/27/2014 02:24 AM, Mike Turquette wrote:
> >>> Quoting Tero Kristo (2014-09-26 00:18:55)
> >>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> >>>>> On 09/23/14 06:38, Tero Kristo wrote:
> >>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> >>>>>>> On 08/21, Tero Kristo wrote:
> >>>>>>>>              /* Skip children who will be reparented to another clock */
> >>>>>>>>              if (child->new_parent && child->new_parent != clk)
> >>>>>>>>                  continue;
> >>>>>>>
> >>>>>>> Are we not hitting the new_parent check here? I don't understand
> >>>>>>> how we can be changing parents here unless the check is being
> >>>>>>> avoided, in which case I wonder why determine_rate isn't being
> >>>>>>> used.
> >>>>>>>
> >>>>>>
> >>>>>> It depends how the clock underneath handles the situation. The error I
> >>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
> >>>>>> which integrates set_rate + mux functionality into a single clock
> >>>>>> node. A call to the clk_set_rate changes the parent of this clock
> >>>>>> (from bypass clock to reference clock), in addition to changing the
> >>>>>> rate (tune the mul+div.) I looked at using the determine rate call
> >>>>>> with this type but it breaks everything up... the parent gets changed
> >>>>>> but not the clock rate, in addition to some other issues.
> >>>>>
> >>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
> >>>>
> >>>> Yes.
> >>>>
> >>>>    > Can we use determine_rate +
> >>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> >>>>> allow us to do the mult+div and the parent in the same op call, although
> >>>>> I don't understand why setting the parent and then setting the rate is
> >>>>> not going to work.
> >>>>
> >>>> Well, setting parent first, then rate later causes problems with the
> >>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
> >>>> are most likely wrong if you just switch from bypass clock to reference
> >>>> clock first without programming the M+N first.
> >>>
> >>> I took a quick look and it still seems to me that the OMAP DPLLs are
> >>> still not modeled properly as mux clocks. Is this correct?
> >>
> >> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
> >> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
> >> div/mult clock will still have overlapping usage of the DPLL_EN field,
> >
> > I'm not talking about splitting up the clock into two separate clocks.
> > If memory serves the DPLL clock implementation "cheats" and hides the
> > bypass_clk info from the clock framework. To be explicit, from the
> > perspective of Linux clock framework DPLL clocks only have one parent.
> >
> > In reality a typical DPLL should have at least 2 parents (and in some
> > cases starting with OMAP4, some of the DPLL output clocks should have a
> > second HSD parent). But the implementation does not reflect this.
> 
> No, this is not the DPLLs are modelled. Each DPLL has currently two 
> parents, ref-clk and bypass-clk, which are both modelled as separate 
> clock nodes, and the DPLL switches parents based on bypass/lock mode. 
> The bypass clock is also usually a mux clock, which further selects 
> separate bypass parent, resulting in 3 or more parents for a certain DPLL.

I stand corrected. I thought it was still done the old way where the
machine-specific clock struct was holding the pointer to the ref_clk and
bypass_clk. I'm glad that is not the case any more.

> 
> >
> >> as the DPLL must be in bypass mode during M+N change. Or, should the
> >> DPLL rate change only be allowed if the mux is in bypass setting?
> >> Several drivers still depend on direct dpll clk_set_rate working
> >> 'properly' (there are some other issues currently present also which
> >> have nothing to do with the mux behavior.)
> >>
> >>> This issue has been lingering for a long time and we can't use
> >>> determine_rate unless that clock has multiple parents. Simply hacking
> >>> knowledge of the parent bypass clock into the .set_rate callback is not
> >>> enough.
> >>
> >> If you believe this _must_ be changed, I can take a look at this for
> >> next merge window, but this will cause a DT data compatibility break if
> >> nothing else (personally I don't care about this as I always rebuild DT
> >> blob with kernel, but lots of other people seem to do.)
> >
> > Well I guess the question is how long will we put up with the many small
> > headaches caused by incorrectly modeling the clock?
> 
> Well, its not kind of incorrectly modelled, it is just modelled in such 
> way that clk_set_rate doesn't cope too well with it.
> 
> > determine_rate and clk_set_parent_and_rate should be sufficient for the
> > OMAP DPLLs but only if they are correctly modeled in the framework.
> 
> Do we have implementation for clk_set_parent_and_rate someplace? I 
> looked at rc7 and didn't find this. I think this would fix the issues I 
> am seeing combined with determine_rate, if clk_set_rate would internally 
> handle changing both rate + parent.

I made it hard for you to find it because I typo'd. It's not a clk api
but a clk_op:

int (*set_rate_and_parent)(struct clk_hw *hw, unsigned long rate,
                           unsigned long parent_rate, u8 index);

Regards,
Mike

> 
> -Tero
> 
> >
> > Regards,
> > Mike
> >
> >>
> >> -Tero
> >>
> >>>
> >>> Regards,
> >>> Mike
> >>>
> >>>>
> >>>>     I'm interested in the other issues that you mentioned
> >>>>> too.
> >>>>
> >>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
> >>>> boot hang, failed drivers etc. I didn't really investigate this that
> >>>> much as it is much more simpler just to use safe list iteration here.
> >>>>
> >>>> -Tero
> >>
> 

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/30/2014 10:03 PM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-30 01:48:49)
>> On 09/30/2014 10:07 AM, Mike Turquette wrote:
>>> Quoting Tero Kristo (2014-09-29 01:09:24)
>>>> On 09/27/2014 02:24 AM, Mike Turquette wrote:
>>>>> Quoting Tero Kristo (2014-09-26 00:18:55)
>>>>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>>>>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>>>>>> On 08/21, Tero Kristo wrote:
>>>>>>>>>>               /* Skip children who will be reparented to another clock */
>>>>>>>>>>               if (child->new_parent && child->new_parent != clk)
>>>>>>>>>>                   continue;
>>>>>>>>>
>>>>>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>>>>>> how we can be changing parents here unless the check is being
>>>>>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>>>>>> used.
>>>>>>>>>
>>>>>>>>
>>>>>>>> It depends how the clock underneath handles the situation. The error I
>>>>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>>>>>> which integrates set_rate + mux functionality into a single clock
>>>>>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>>>>>> (from bypass clock to reference clock), in addition to changing the
>>>>>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>>>>>> with this type but it breaks everything up... the parent gets changed
>>>>>>>> but not the clock rate, in addition to some other issues.
>>>>>>>
>>>>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>     > Can we use determine_rate +
>>>>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>>>>>> allow us to do the mult+div and the parent in the same op call, although
>>>>>>> I don't understand why setting the parent and then setting the rate is
>>>>>>> not going to work.
>>>>>>
>>>>>> Well, setting parent first, then rate later causes problems with the
>>>>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>>>>>> are most likely wrong if you just switch from bypass clock to reference
>>>>>> clock first without programming the M+N first.
>>>>>
>>>>> I took a quick look and it still seems to me that the OMAP DPLLs are
>>>>> still not modeled properly as mux clocks. Is this correct?
>>>>
>>>> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
>>>> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
>>>> div/mult clock will still have overlapping usage of the DPLL_EN field,
>>>
>>> I'm not talking about splitting up the clock into two separate clocks.
>>> If memory serves the DPLL clock implementation "cheats" and hides the
>>> bypass_clk info from the clock framework. To be explicit, from the
>>> perspective of Linux clock framework DPLL clocks only have one parent.
>>>
>>> In reality a typical DPLL should have at least 2 parents (and in some
>>> cases starting with OMAP4, some of the DPLL output clocks should have a
>>> second HSD parent). But the implementation does not reflect this.
>>
>> No, this is not the DPLLs are modelled. Each DPLL has currently two
>> parents, ref-clk and bypass-clk, which are both modelled as separate
>> clock nodes, and the DPLL switches parents based on bypass/lock mode.
>> The bypass clock is also usually a mux clock, which further selects
>> separate bypass parent, resulting in 3 or more parents for a certain DPLL.
>
> I stand corrected. I thought it was still done the old way where the
> machine-specific clock struct was holding the pointer to the ref_clk and
> bypass_clk. I'm glad that is not the case any more.
>
>>
>>>
>>>> as the DPLL must be in bypass mode during M+N change. Or, should the
>>>> DPLL rate change only be allowed if the mux is in bypass setting?
>>>> Several drivers still depend on direct dpll clk_set_rate working
>>>> 'properly' (there are some other issues currently present also which
>>>> have nothing to do with the mux behavior.)
>>>>
>>>>> This issue has been lingering for a long time and we can't use
>>>>> determine_rate unless that clock has multiple parents. Simply hacking
>>>>> knowledge of the parent bypass clock into the .set_rate callback is not
>>>>> enough.
>>>>
>>>> If you believe this _must_ be changed, I can take a look at this for
>>>> next merge window, but this will cause a DT data compatibility break if
>>>> nothing else (personally I don't care about this as I always rebuild DT
>>>> blob with kernel, but lots of other people seem to do.)
>>>
>>> Well I guess the question is how long will we put up with the many small
>>> headaches caused by incorrectly modeling the clock?
>>
>> Well, its not kind of incorrectly modelled, it is just modelled in such
>> way that clk_set_rate doesn't cope too well with it.
>>
>>> determine_rate and clk_set_parent_and_rate should be sufficient for the
>>> OMAP DPLLs but only if they are correctly modeled in the framework.
>>
>> Do we have implementation for clk_set_parent_and_rate someplace? I
>> looked at rc7 and didn't find this. I think this would fix the issues I
>> am seeing combined with determine_rate, if clk_set_rate would internally
>> handle changing both rate + parent.
>
> I made it hard for you to find it because I typo'd. It's not a clk api
> but a clk_op:
>
> int (*set_rate_and_parent)(struct clk_hw *hw, unsigned long rate,
>                             unsigned long parent_rate, u8 index);

Oh I see. I have been experimenting with this last couple of days and it 
looks like I got a working solution with determine_rate / 
set_rate_and_parent for the DPLLs now. Need to do some cleanup for the 
patches and will hopefully post those tomorrow, but I guess this will 
probably miss the next merge window seeing we are pretty late in -rc.

-Tero

>
> Regards,
> Mike
>
>>
>> -Tero
>>
>>>
>>> Regards,
>>> Mike
>>>
>>>>
>>>> -Tero
>>>>
>>>>>
>>>>> Regards,
>>>>> Mike
>>>>>
>>>>>>
>>>>>>      I'm interested in the other issues that you mentioned
>>>>>>> too.
>>>>>>
>>>>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>>>>>> boot hang, failed drivers etc. I didn't really investigate this that
>>>>>> much as it is much more simpler just to use safe list iteration here.
>>>>>>
>>>>>> -Tero
>>>>
>>

[PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/30/2014 10:03 PM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-30 01:48:49)
>> On 09/30/2014 10:07 AM, Mike Turquette wrote:
>>> Quoting Tero Kristo (2014-09-29 01:09:24)
>>>> On 09/27/2014 02:24 AM, Mike Turquette wrote:
>>>>> Quoting Tero Kristo (2014-09-26 00:18:55)
>>>>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>>>>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>>>>>> On 08/21, Tero Kristo wrote:
>>>>>>>>>>               /* Skip children who will be reparented to another clock */
>>>>>>>>>>               if (child->new_parent && child->new_parent != clk)
>>>>>>>>>>                   continue;
>>>>>>>>>
>>>>>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>>>>>> how we can be changing parents here unless the check is being
>>>>>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>>>>>> used.
>>>>>>>>>
>>>>>>>>
>>>>>>>> It depends how the clock underneath handles the situation. The error I
>>>>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>>>>>> which integrates set_rate + mux functionality into a single clock
>>>>>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>>>>>> (from bypass clock to reference clock), in addition to changing the
>>>>>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>>>>>> with this type but it breaks everything up... the parent gets changed
>>>>>>>> but not the clock rate, in addition to some other issues.
>>>>>>>
>>>>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>     > Can we use determine_rate +
>>>>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>>>>>> allow us to do the mult+div and the parent in the same op call, although
>>>>>>> I don't understand why setting the parent and then setting the rate is
>>>>>>> not going to work.
>>>>>>
>>>>>> Well, setting parent first, then rate later causes problems with the
>>>>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>>>>>> are most likely wrong if you just switch from bypass clock to reference
>>>>>> clock first without programming the M+N first.
>>>>>
>>>>> I took a quick look and it still seems to me that the OMAP DPLLs are
>>>>> still not modeled properly as mux clocks. Is this correct?
>>>>
>>>> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
>>>> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
>>>> div/mult clock will still have overlapping usage of the DPLL_EN field,
>>>
>>> I'm not talking about splitting up the clock into two separate clocks.
>>> If memory serves the DPLL clock implementation "cheats" and hides the
>>> bypass_clk info from the clock framework. To be explicit, from the
>>> perspective of Linux clock framework DPLL clocks only have one parent.
>>>
>>> In reality a typical DPLL should have at least 2 parents (and in some
>>> cases starting with OMAP4, some of the DPLL output clocks should have a
>>> second HSD parent). But the implementation does not reflect this.
>>
>> No, this is not the DPLLs are modelled. Each DPLL has currently two
>> parents, ref-clk and bypass-clk, which are both modelled as separate
>> clock nodes, and the DPLL switches parents based on bypass/lock mode.
>> The bypass clock is also usually a mux clock, which further selects
>> separate bypass parent, resulting in 3 or more parents for a certain DPLL.
>
> I stand corrected. I thought it was still done the old way where the
> machine-specific clock struct was holding the pointer to the ref_clk and
> bypass_clk. I'm glad that is not the case any more.
>
>>
>>>
>>>> as the DPLL must be in bypass mode during M+N change. Or, should the
>>>> DPLL rate change only be allowed if the mux is in bypass setting?
>>>> Several drivers still depend on direct dpll clk_set_rate working
>>>> 'properly' (there are some other issues currently present also which
>>>> have nothing to do with the mux behavior.)
>>>>
>>>>> This issue has been lingering for a long time and we can't use
>>>>> determine_rate unless that clock has multiple parents. Simply hacking
>>>>> knowledge of the parent bypass clock into the .set_rate callback is not
>>>>> enough.
>>>>
>>>> If you believe this _must_ be changed, I can take a look at this for
>>>> next merge window, but this will cause a DT data compatibility break if
>>>> nothing else (personally I don't care about this as I always rebuild DT
>>>> blob with kernel, but lots of other people seem to do.)
>>>
>>> Well I guess the question is how long will we put up with the many small
>>> headaches caused by incorrectly modeling the clock?
>>
>> Well, its not kind of incorrectly modelled, it is just modelled in such
>> way that clk_set_rate doesn't cope too well with it.
>>
>>> determine_rate and clk_set_parent_and_rate should be sufficient for the
>>> OMAP DPLLs but only if they are correctly modeled in the framework.
>>
>> Do we have implementation for clk_set_parent_and_rate someplace? I
>> looked at rc7 and didn't find this. I think this would fix the issues I
>> am seeing combined with determine_rate, if clk_set_rate would internally
>> handle changing both rate + parent.
>
> I made it hard for you to find it because I typo'd. It's not a clk api
> but a clk_op:
>
> int (*set_rate_and_parent)(struct clk_hw *hw, unsigned long rate,
>                             unsigned long parent_rate, u8 index);

Oh I see. I have been experimenting with this last couple of days and it 
looks like I got a working solution with determine_rate / 
set_rate_and_parent for the DPLLs now. Need to do some cleanup for the 
patches and will hopefully post those tomorrow, but I guess this will 
probably miss the next merge window seeing we are pretty late in -rc.

-Tero

>
> Regards,
> Mike
>
>>
>> -Tero
>>
>>>
>>> Regards,
>>> Mike
>>>
>>>>
>>>> -Tero
>>>>
>>>>>
>>>>> Regards,
>>>>> Mike
>>>>>
>>>>>>
>>>>>>      I'm interested in the other issues that you mentioned
>>>>>>> too.
>>>>>>
>>>>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>>>>>> boot hang, failed drivers etc. I didn't really investigate this that
>>>>>> much as it is much more simpler just to use safe list iteration here.
>>>>>>
>>>>>> -Tero
>>>>
>>

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/30/2014 10:03 PM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-30 01:48:49)
>> On 09/30/2014 10:07 AM, Mike Turquette wrote:
>>> Quoting Tero Kristo (2014-09-29 01:09:24)
>>>> On 09/27/2014 02:24 AM, Mike Turquette wrote:
>>>>> Quoting Tero Kristo (2014-09-26 00:18:55)
>>>>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>>>>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>>>>>> On 08/21, Tero Kristo wrote:
>>>>>>>>>>               /* Skip children who will be reparented to another clock */
>>>>>>>>>>               if (child->new_parent && child->new_parent != clk)
>>>>>>>>>>                   continue;
>>>>>>>>>
>>>>>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>>>>>> how we can be changing parents here unless the check is being
>>>>>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>>>>>> used.
>>>>>>>>>
>>>>>>>>
>>>>>>>> It depends how the clock underneath handles the situation. The error I
>>>>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>>>>>> which integrates set_rate + mux functionality into a single clock
>>>>>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>>>>>> (from bypass clock to reference clock), in addition to changing the
>>>>>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>>>>>> with this type but it breaks everything up... the parent gets changed
>>>>>>>> but not the clock rate, in addition to some other issues.
>>>>>>>
>>>>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>     > Can we use determine_rate +
>>>>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>>>>>> allow us to do the mult+div and the parent in the same op call, although
>>>>>>> I don't understand why setting the parent and then setting the rate is
>>>>>>> not going to work.
>>>>>>
>>>>>> Well, setting parent first, then rate later causes problems with the
>>>>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>>>>>> are most likely wrong if you just switch from bypass clock to reference
>>>>>> clock first without programming the M+N first.
>>>>>
>>>>> I took a quick look and it still seems to me that the OMAP DPLLs are
>>>>> still not modeled properly as mux clocks. Is this correct?
>>>>
>>>> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
>>>> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
>>>> div/mult clock will still have overlapping usage of the DPLL_EN field,
>>>
>>> I'm not talking about splitting up the clock into two separate clocks.
>>> If memory serves the DPLL clock implementation "cheats" and hides the
>>> bypass_clk info from the clock framework. To be explicit, from the
>>> perspective of Linux clock framework DPLL clocks only have one parent.
>>>
>>> In reality a typical DPLL should have at least 2 parents (and in some
>>> cases starting with OMAP4, some of the DPLL output clocks should have a
>>> second HSD parent). But the implementation does not reflect this.
>>
>> No, this is not the DPLLs are modelled. Each DPLL has currently two
>> parents, ref-clk and bypass-clk, which are both modelled as separate
>> clock nodes, and the DPLL switches parents based on bypass/lock mode.
>> The bypass clock is also usually a mux clock, which further selects
>> separate bypass parent, resulting in 3 or more parents for a certain DPLL.
>
> I stand corrected. I thought it was still done the old way where the
> machine-specific clock struct was holding the pointer to the ref_clk and
> bypass_clk. I'm glad that is not the case any more.
>
>>
>>>
>>>> as the DPLL must be in bypass mode during M+N change. Or, should the
>>>> DPLL rate change only be allowed if the mux is in bypass setting?
>>>> Several drivers still depend on direct dpll clk_set_rate working
>>>> 'properly' (there are some other issues currently present also which
>>>> have nothing to do with the mux behavior.)
>>>>
>>>>> This issue has been lingering for a long time and we can't use
>>>>> determine_rate unless that clock has multiple parents. Simply hacking
>>>>> knowledge of the parent bypass clock into the .set_rate callback is not
>>>>> enough.
>>>>
>>>> If you believe this _must_ be changed, I can take a look at this for
>>>> next merge window, but this will cause a DT data compatibility break if
>>>> nothing else (personally I don't care about this as I always rebuild DT
>>>> blob with kernel, but lots of other people seem to do.)
>>>
>>> Well I guess the question is how long will we put up with the many small
>>> headaches caused by incorrectly modeling the clock?
>>
>> Well, its not kind of incorrectly modelled, it is just modelled in such
>> way that clk_set_rate doesn't cope too well with it.
>>
>>> determine_rate and clk_set_parent_and_rate should be sufficient for the
>>> OMAP DPLLs but only if they are correctly modeled in the framework.
>>
>> Do we have implementation for clk_set_parent_and_rate someplace? I
>> looked at rc7 and didn't find this. I think this would fix the issues I
>> am seeing combined with determine_rate, if clk_set_rate would internally
>> handle changing both rate + parent.
>
> I made it hard for you to find it because I typo'd. It's not a clk api
> but a clk_op:
>
> int (*set_rate_and_parent)(struct clk_hw *hw, unsigned long rate,
>                             unsigned long parent_rate, u8 index);

Oh I see. I have been experimenting with this last couple of days and it 
looks like I got a working solution with determine_rate / 
set_rate_and_parent for the DPLLs now. Need to do some cleanup for the 
patches and will hopefully post those tomorrow, but I guess this will 
probably miss the next merge window seeing we are pretty late in -rc.

-Tero

>
> Regards,
> Mike
>
>>
>> -Tero
>>
>>>
>>> Regards,
>>> Mike
>>>
>>>>
>>>> -Tero
>>>>
>>>>>
>>>>> Regards,
>>>>> Mike
>>>>>
>>>>>>
>>>>>>      I'm interested in the other issues that you mentioned
>>>>>>> too.
>>>>>>
>>>>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>>>>>> boot hang, failed drivers etc. I didn't really investigate this that
>>>>>> much as it is much more simpler just to use safe list iteration here.
>>>>>>
>>>>>> -Tero
>>>>
>>