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

Groups > linux.kernel > #1435708 > unrolled thread

Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH v9 01/10] clk: fix initial state of critical clock's  parents Stephen Boyd <sboyd@codeaurora.org> - 2016-07-02 03:30 +0200
    Re: [PATCH v9 01/10] clk: fix initial state of critical clock's  parents James Liao <jamesjj.liao@mediatek.com> - 2016-07-04 06:00 +0200
      Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents Michael Turquette <mturquette@baylibre.com> - 2016-07-09 01:40 +0200
        Re: [PATCH v9 01/10] clk: fix initial state of critical clock's  parents Stephen Boyd <sboyd@codeaurora.org> - 2016-07-09 02:50 +0200

#1435708 — Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents

(Resending to everyone)

On 06/22, Erin Lo wrote:
> From: James Liao <jamesjj.liao@mediatek.com>
> 
> This patch fixed wrong state of parent clocks if they are registered
> after critical clocks.
> 
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>

It would be nice if you included the information about the
problem from James' previous mail. This says what it does, but
doesn't explain what the problem is and how it is fixing it.

> ---
>  drivers/clk/clk.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d584004..e9f5f89 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2388,8 +2388,15 @@ static int __clk_core_init(struct clk_core *core)
>  	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>  		struct clk_core *parent = __clk_init_parent(orphan);
>  
> -		if (parent)
> +		if (parent) {
>  			clk_core_reparent(orphan, parent);
> +
> +			if (orphan->prepare_count)
> +				clk_core_prepare(parent);
> +
> +			if (orphan->enable_count)
> +				clk_core_enable(parent);
> +		}
>  	}

I'm pretty sure I pointed this problem out to Mike when the
critical clk patches were being pushed. I can't recall what the
plan was though to fix the problem. I'm pretty sure he said that
clk_core_reparent() would take care of it, but obviously it is
not doing that. Or perhaps it was that clk handoff should figure
out that the parents of a critical clk are also on and thus keep
them on.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1436362

On Fri, 2016-07-01 at 18:21 -0700, Stephen Boyd wrote:
> (Resending to everyone)
> 
> On 06/22, Erin Lo wrote:
> > From: James Liao <jamesjj.liao@mediatek.com>
> > 
> > This patch fixed wrong state of parent clocks if they are registered
> > after critical clocks.
> > 
> > Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> > Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> 
> It would be nice if you included the information about the
> problem from James' previous mail. This says what it does, but
> doesn't explain what the problem is and how it is fixing it.
> 
> > ---
> >  drivers/clk/clk.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index d584004..e9f5f89 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2388,8 +2388,15 @@ static int __clk_core_init(struct clk_core *core)
> >  	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
> >  		struct clk_core *parent = __clk_init_parent(orphan);
> >  
> > -		if (parent)
> > +		if (parent) {
> >  			clk_core_reparent(orphan, parent);
> > +
> > +			if (orphan->prepare_count)
> > +				clk_core_prepare(parent);
> > +
> > +			if (orphan->enable_count)
> > +				clk_core_enable(parent);
> > +		}
> >  	}
> 
> I'm pretty sure I pointed this problem out to Mike when the
> critical clk patches were being pushed. I can't recall what the
> plan was though to fix the problem. I'm pretty sure he said that
> clk_core_reparent() would take care of it, but obviously it is
> not doing that. Or perhaps it was that clk handoff should figure
> out that the parents of a critical clk are also on and thus keep
> them on.

Hi Mike

Is there any other patch to fix this issue? Or did I misuse critical
clock flag?


Best regards,

James

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

#1439848 — Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents

Hi James,

Quoting James Liao (2016-07-03 20:51:48)
> On Fri, 2016-07-01 at 18:21 -0700, Stephen Boyd wrote:
> > (Resending to everyone)
> > 
> > On 06/22, Erin Lo wrote:
> > > From: James Liao <jamesjj.liao@mediatek.com>
> > > 
> > > This patch fixed wrong state of parent clocks if they are registered
> > > after critical clocks.
> > > 
> > > Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> > > Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> > 
> > It would be nice if you included the information about the
> > problem from James' previous mail. This says what it does, but
> > doesn't explain what the problem is and how it is fixing it.
> > 
> > > ---
> > >  drivers/clk/clk.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index d584004..e9f5f89 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -2388,8 +2388,15 @@ static int __clk_core_init(struct clk_core *core)
> > >     hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
> > >             struct clk_core *parent = __clk_init_parent(orphan);
> > >  
> > > -           if (parent)
> > > +           if (parent) {
> > >                     clk_core_reparent(orphan, parent);
> > > +
> > > +                   if (orphan->prepare_count)
> > > +                           clk_core_prepare(parent);
> > > +
> > > +                   if (orphan->enable_count)
> > > +                           clk_core_enable(parent);
> > > +           }
> > >     }
> > 
> > I'm pretty sure I pointed this problem out to Mike when the
> > critical clk patches were being pushed. I can't recall what the
> > plan was though to fix the problem. I'm pretty sure he said that
> > clk_core_reparent() would take care of it, but obviously it is
> > not doing that. Or perhaps it was that clk handoff should figure
> > out that the parents of a critical clk are also on and thus keep
> > them on.
> 
> Hi Mike
> 
> Is there any other patch to fix this issue? Or did I misuse critical
> clock flag?

There is no fix yes. Your fix is basically correct. I was mistaken back
when I told you and Stephen that the framework already took care of
this.

However, instead of "open coding" this solution, I would rather re-use
the __clk_set_parent_{before,after} helpers instead. Can you review/test
the following patch and let me know what you think?

Thanks,
Mike



From c0163b3f719b1e219b28ad425f94f9ef54a25a8f Mon Sep 17 00:00:00 2001
From: Michael Turquette <mturquette@baylibre.com>
Date: Fri, 8 Jul 2016 16:05:22 -0700
Subject: [PATCH] clk: migrate ref counts when orphans are reunited

It's always nice to see families reunited, and this is equally true when
talking about parent clocks and their children. However, if the orphan
clk had a positive prepare_count or enable_count, then we would not
migrate those counts up the parent chain correctly.

This has manifested with the recent critical clocks feature, which often
enables clocks very early, before their parents have been registered.

Fixed by replacing the call to clk_core_reparent with calls to
__clk_set_parent_{before,after}.

Cc: James Liao <jamesjj.liao@mediatek.com>
Cc: Erin Lo <erin.lo@mediatek.com>
Signed-off-by: Michael Turquette <mturquette@baylibre.com>
---
 drivers/clk/clk.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 820a939fb6bb..70efe4c4e0cc 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2449,8 +2449,14 @@ static int __clk_core_init(struct clk_core *core)
 	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
 		struct clk_core *parent = __clk_init_parent(orphan);
 
-		if (parent)
-			clk_core_reparent(orphan, parent);
+		/*
+		 * we could call __clk_set_parent, but that would result in a
+		 * reducant call to the .set_rate op, if it exists
+		 */
+		if (parent) {
+			__clk_set_parent_before(orphan, parent);
+			__clk_set_parent_after(orphan, parent, NULL);
+		}
 	}
 
 	/*
-- 
2.7.4 (Apple Git-66)

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

#1439861

On 07/08/2016 04:32 PM, Michael Turquette wrote:
> ---
>  drivers/clk/clk.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 820a939fb6bb..70efe4c4e0cc 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2449,8 +2449,14 @@ static int __clk_core_init(struct clk_core *core)
>  	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>  		struct clk_core *parent = __clk_init_parent(orphan);
>  
> -		if (parent)
> -			clk_core_reparent(orphan, parent);
> +		/*
> +		 * we could call __clk_set_parent, but that would result in a
> +		 * reducant call to the .set_rate op, if it exists

Did you mean .set_parent op?

> +		 */
> +		if (parent) {
> +			__clk_set_parent_before(orphan, parent);
> +			__clk_set_parent_after(orphan, parent, NULL);
> +		}
>  	}
>  
>  	/*


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web