Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
Hi Stephen, On Fri, 2016-08-12 at 17:39 -0700, Stephen Boyd wrote: > On 08/12, James Liao wrote: > > On Wed, 2016-08-10 at 14:09 -0700, Stephen Boyd wrote: > > > (Including lists) > > > > > > On 08/09, James Liao wrote: > > > > On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: > > > >> > > > >> Hi Mike, > > > >> > > > >> Do you have new patches to fix new clock parents? If not, I think we > > > >> can > > > >> use my patch first. Is it okay? > > > >> > > > > > > > > Hi Stephen, > > > > > > > > Do you have comments for the bug fixing? I prefer to use my patch (clk: > > > > fix initial state of critical clock's parents). How do you think? > > > > > > > > > > How about we recalc accuracies and rates in addition to the patch > > > from Mike? That will fix everything? > > > > Hi Stephen, > > > > It works! > > > > I'll send a new series of MT2701 clock support in few days. Should I > > include this patch in my series? Or you'll merge it into clk-next > > directly? > > > > Thanks. I can take that as a tested-by? I can merge it into Yes, please feel free to add: Tested-by: James Liao> clk-next directly, but do we need to put the mt2701 patches on a > separate branch to merge into arm-soc? If so we'll need to put > this patch first to avoid bisection failures. I prefer to merge clk driver into mainline first. Patches that depend on mt2701 clks such as dtsi can base on next kernel release. Best regards, James
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
Hi Stephen, On Fri, 2016-08-12 at 17:39 -0700, Stephen Boyd wrote: > On 08/12, James Liao wrote: > > On Wed, 2016-08-10 at 14:09 -0700, Stephen Boyd wrote: > > > (Including lists) > > > > > > On 08/09, James Liao wrote: > > > > On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: > > > >> > > > >> Hi Mike, > > > >> > > > >> Do you have new patches to fix new clock parents? If not, I think we > > > >> can > > > >> use my patch first. Is it okay? > > > >> > > > > > > > > Hi Stephen, > > > > > > > > Do you have comments for the bug fixing? I prefer to use my patch (clk: > > > > fix initial state of critical clock's parents). How do you think? > > > > > > > > > > How about we recalc accuracies and rates in addition to the patch > > > from Mike? That will fix everything? > > > > Hi Stephen, > > > > It works! > > > > I'll send a new series of MT2701 clock support in few days. Should I > > include this patch in my series? Or you'll merge it into clk-next > > directly? > > > > Thanks. I can take that as a tested-by? I can merge it into Yes, please feel free to add: Tested-by: James Liao > clk-next directly, but do we need to put the mt2701 patches on a > separate branch to merge into arm-soc? If so we'll need to put > this patch first to avoid bisection failures. I prefer to merge clk driver into mainline first. Patches that depend on mt2701 clks such as dtsi can base on next kernel release. Best regards, James
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On 08/12, James Liao wrote: > On Wed, 2016-08-10 at 14:09 -0700, Stephen Boyd wrote: > > (Including lists) > > > > On 08/09, James Liao wrote: > > > On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: > > >> > > >> Hi Mike, > > >> > > >> Do you have new patches to fix new clock parents? If not, I think we can > > >> use my patch first. Is it okay? > > >> > > > > > > Hi Stephen, > > > > > > Do you have comments for the bug fixing? I prefer to use my patch (clk: > > > fix initial state of critical clock's parents). How do you think? > > > > > > > How about we recalc accuracies and rates in addition to the patch > > from Mike? That will fix everything? > > Hi Stephen, > > It works! > > I'll send a new series of MT2701 clock support in few days. Should I > include this patch in my series? Or you'll merge it into clk-next > directly? > Thanks. I can take that as a tested-by? I can merge it into clk-next directly, but do we need to put the mt2701 patches on a separate branch to merge into arm-soc? If so we'll need to put this patch first to avoid bisection failures. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On 08/12, James Liao wrote: > On Wed, 2016-08-10 at 14:09 -0700, Stephen Boyd wrote: > > (Including lists) > > > > On 08/09, James Liao wrote: > > > On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: > > >> > > >> Hi Mike, > > >> > > >> Do you have new patches to fix new clock parents? If not, I think we can > > >> use my patch first. Is it okay? > > >> > > > > > > Hi Stephen, > > > > > > Do you have comments for the bug fixing? I prefer to use my patch (clk: > > > fix initial state of critical clock's parents). How do you think? > > > > > > > How about we recalc accuracies and rates in addition to the patch > > from Mike? That will fix everything? > > Hi Stephen, > > It works! > > I'll send a new series of MT2701 clock support in few days. Should I > include this patch in my series? Or you'll merge it into clk-next > directly? > Thanks. I can take that as a tested-by? I can merge it into clk-next directly, but do we need to put the mt2701 patches on a separate branch to merge into arm-soc? If so we'll need to put this patch first to avoid bisection failures. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On Wed, 2016-08-10 at 14:09 -0700, Stephen Boyd wrote: > (Including lists) > > On 08/09, James Liao wrote: > > On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: > >> > >> Hi Mike, > >> > >> Do you have new patches to fix new clock parents? If not, I think we can > >> use my patch first. Is it okay? > >> > > > > Hi Stephen, > > > > Do you have comments for the bug fixing? I prefer to use my patch (clk: > > fix initial state of critical clock's parents). How do you think? > > > > How about we recalc accuracies and rates in addition to the patch > from Mike? That will fix everything? Hi Stephen, It works! I'll send a new series of MT2701 clock support in few days. Should I include this patch in my series? Or you'll merge it into clk-next directly? Best regards, James > ---8< > From: Michael Turquette> 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 > Cc: Erin Lo > Signed-off-by: Michael Turquette > [sb...@codeaurora.org: Recalc accuracies and rates too] > Signed-off-by: Stephen Boyd > --- > drivers/clk/clk.c | 12 ++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 820a939fb6bb..dc3fff2bf839 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2449,8 +2449,16 @@ static int __clk_core_init(struct clk_core *core) > hlist_for_each_entry_safe(orphan, tmp2, _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); > + __clk_recalc_accuracies(orphan); > + __clk_recalc_rates(orphan, 0); > + } > } > > /*
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On Wed, 2016-08-10 at 14:09 -0700, Stephen Boyd wrote: > (Including lists) > > On 08/09, James Liao wrote: > > On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: > >> > >> Hi Mike, > >> > >> Do you have new patches to fix new clock parents? If not, I think we can > >> use my patch first. Is it okay? > >> > > > > Hi Stephen, > > > > Do you have comments for the bug fixing? I prefer to use my patch (clk: > > fix initial state of critical clock's parents). How do you think? > > > > How about we recalc accuracies and rates in addition to the patch > from Mike? That will fix everything? Hi Stephen, It works! I'll send a new series of MT2701 clock support in few days. Should I include this patch in my series? Or you'll merge it into clk-next directly? Best regards, James > ---8< > From: Michael Turquette > 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 > Cc: Erin Lo > Signed-off-by: Michael Turquette > [sb...@codeaurora.org: Recalc accuracies and rates too] > Signed-off-by: Stephen Boyd > --- > drivers/clk/clk.c | 12 ++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 820a939fb6bb..dc3fff2bf839 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2449,8 +2449,16 @@ static int __clk_core_init(struct clk_core *core) > hlist_for_each_entry_safe(orphan, tmp2, _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); > + __clk_recalc_accuracies(orphan); > + __clk_recalc_rates(orphan, 0); > + } > } > > /*
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
(Including lists) On 08/09, James Liao wrote: > On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: >> >> Hi Mike, >> >> Do you have new patches to fix new clock parents? If not, I think we can >> use my patch first. Is it okay? >> > > Hi Stephen, > > Do you have comments for the bug fixing? I prefer to use my patch (clk: > fix initial state of critical clock's parents). How do you think? > How about we recalc accuracies and rates in addition to the patch from Mike? That will fix everything? ---8< From: Michael TurquetteSubject: [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 Cc: Erin Lo Signed-off-by: Michael Turquette [sb...@codeaurora.org: Recalc accuracies and rates too] Signed-off-by: Stephen Boyd --- drivers/clk/clk.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 820a939fb6bb..dc3fff2bf839 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2449,8 +2449,16 @@ static int __clk_core_init(struct clk_core *core) hlist_for_each_entry_safe(orphan, tmp2, _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); + __clk_recalc_accuracies(orphan); + __clk_recalc_rates(orphan, 0); + } } /* -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
(Including lists) On 08/09, James Liao wrote: > On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: >> >> Hi Mike, >> >> Do you have new patches to fix new clock parents? If not, I think we can >> use my patch first. Is it okay? >> > > Hi Stephen, > > Do you have comments for the bug fixing? I prefer to use my patch (clk: > fix initial state of critical clock's parents). How do you think? > How about we recalc accuracies and rates in addition to the patch from Mike? That will fix everything? ---8< From: Michael Turquette 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 Cc: Erin Lo Signed-off-by: Michael Turquette [sb...@codeaurora.org: Recalc accuracies and rates too] Signed-off-by: Stephen Boyd --- drivers/clk/clk.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 820a939fb6bb..dc3fff2bf839 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2449,8 +2449,16 @@ static int __clk_core_init(struct clk_core *core) hlist_for_each_entry_safe(orphan, tmp2, _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); + __clk_recalc_accuracies(orphan); + __clk_recalc_rates(orphan, 0); + } } /* -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: > On Mon, 2016-07-11 at 16:24 +0800, James Liao wrote: > > Hi Mike, > > > > On Fri, 2016-07-08 at 16:32 -0700, Michael Turquette wrote: > > > 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> > > > > > > > > > > > This patch fixed wrong state of parent clocks if they are registered > > > > > > after critical clocks. > > > > > > > > > > > > Signed-off-by: James Liao > > > > > > Signed-off-by: Erin Lo > > > > > > > > > > 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, _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 > > > 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 > > > Cc: Erin Lo > > > Signed-off-by: Michael Turquette > > > --- > > > 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, _orphan_list, child_node) { > > > struct clk_core *parent = __clk_init_parent(orphan); > > > > > > - if (parent) > > > - clk_core_reparent(orphan, parent); > > > > Is it correct to remove clk_core_reparent()? It lacks > > __clk_recalc_accuracies() and __clk_recalc_rates(), so the new parent's > > rate will not propagate correctly. > > > > For example, I set vdec_sel as a critical clock. Without your patch, the > > result was: > > > > vdecpll 00 33800 > >vdecpll_ck
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On Wed, 2016-08-03 at 13:46 +0800, James Liao wrote: > On Mon, 2016-07-11 at 16:24 +0800, James Liao wrote: > > Hi Mike, > > > > On Fri, 2016-07-08 at 16:32 -0700, Michael Turquette wrote: > > > 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 > > > > > > > > > > > > This patch fixed wrong state of parent clocks if they are registered > > > > > > after critical clocks. > > > > > > > > > > > > Signed-off-by: James Liao > > > > > > Signed-off-by: Erin Lo > > > > > > > > > > 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, _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 > > > 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 > > > Cc: Erin Lo > > > Signed-off-by: Michael Turquette > > > --- > > > 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, _orphan_list, child_node) { > > > struct clk_core *parent = __clk_init_parent(orphan); > > > > > > - if (parent) > > > - clk_core_reparent(orphan, parent); > > > > Is it correct to remove clk_core_reparent()? It lacks > > __clk_recalc_accuracies() and __clk_recalc_rates(), so the new parent's > > rate will not propagate correctly. > > > > For example, I set vdec_sel as a critical clock. Without your patch, the > > result was: > > > > vdecpll 00 33800 > >vdecpll_ck 11 33800 > > vdec_sel 11 33800 > > > > With your patch, it became: > > > > vdecpll 11 33800 >
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On Mon, 2016-07-11 at 16:24 +0800, James Liao wrote: > Hi Mike, > > On Fri, 2016-07-08 at 16:32 -0700, Michael Turquette wrote: > > 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> > > > > > > > > > This patch fixed wrong state of parent clocks if they are registered > > > > > after critical clocks. > > > > > > > > > > Signed-off-by: James Liao > > > > > Signed-off-by: Erin Lo > > > > > > > > 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, _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 > > 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 > > Cc: Erin Lo > > Signed-off-by: Michael Turquette > > --- > > 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, _orphan_list, child_node) { > > struct clk_core *parent = __clk_init_parent(orphan); > > > > - if (parent) > > - clk_core_reparent(orphan, parent); > > Is it correct to remove clk_core_reparent()? It lacks > __clk_recalc_accuracies() and __clk_recalc_rates(), so the new parent's > rate will not propagate correctly. > > For example, I set vdec_sel as a critical clock. Without your patch, the > result was: > > vdecpll 00 33800 >vdecpll_ck 11 33800 > vdec_sel 11 33800 > > With your patch, it became: > > vdecpll 11 33800 >vdecpll_ck 11 0 > vdec_sel 11 0 > > The
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On Mon, 2016-07-11 at 16:24 +0800, James Liao wrote: > Hi Mike, > > On Fri, 2016-07-08 at 16:32 -0700, Michael Turquette wrote: > > 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 > > > > > > > > > > This patch fixed wrong state of parent clocks if they are registered > > > > > after critical clocks. > > > > > > > > > > Signed-off-by: James Liao > > > > > Signed-off-by: Erin Lo > > > > > > > > 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, _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 > > 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 > > Cc: Erin Lo > > Signed-off-by: Michael Turquette > > --- > > 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, _orphan_list, child_node) { > > struct clk_core *parent = __clk_init_parent(orphan); > > > > - if (parent) > > - clk_core_reparent(orphan, parent); > > Is it correct to remove clk_core_reparent()? It lacks > __clk_recalc_accuracies() and __clk_recalc_rates(), so the new parent's > rate will not propagate correctly. > > For example, I set vdec_sel as a critical clock. Without your patch, the > result was: > > vdecpll 00 33800 >vdecpll_ck 11 33800 > vdec_sel 11 33800 > > With your patch, it became: > > vdecpll 11 33800 >vdecpll_ck 11 0 > vdec_sel 11 0 > > The prepare_count and enable_count are correct with your patch, but the > rates of vdecpll_ck and vdec_sel become incorrect. > > > Best regards, > > James > > > + /* > > +
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
Hi Mike, On Fri, 2016-07-08 at 16:32 -0700, Michael Turquette wrote: > 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> > > > > > > > This patch fixed wrong state of parent clocks if they are registered > > > > after critical clocks. > > > > > > > > Signed-off-by: James Liao > > > > Signed-off-by: Erin Lo > > > > > > 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, _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 > 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 > Cc: Erin Lo > Signed-off-by: Michael Turquette > --- > 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, _orphan_list, child_node) { > struct clk_core *parent = __clk_init_parent(orphan); > > - if (parent) > - clk_core_reparent(orphan, parent); Is it correct to remove clk_core_reparent()? It lacks __clk_recalc_accuracies() and __clk_recalc_rates(), so the new parent's rate will not propagate correctly. For example, I set vdec_sel as a critical clock. Without your patch, the result was: vdecpll 00 33800 vdecpll_ck 11 33800 vdec_sel 11 33800 With your patch, it became: vdecpll 11 33800 vdecpll_ck 11 0 vdec_sel 11 0 The prepare_count and enable_count are correct with your patch, but the rates of vdecpll_ck and vdec_sel become incorrect. Best regards, James > + /* > + * we could call __clk_set_parent, but that would result in a > + * reducant call to the .set_rate op, if it exists > +
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
Hi Mike, On Fri, 2016-07-08 at 16:32 -0700, Michael Turquette wrote: > 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 > > > > > > > > This patch fixed wrong state of parent clocks if they are registered > > > > after critical clocks. > > > > > > > > Signed-off-by: James Liao > > > > Signed-off-by: Erin Lo > > > > > > 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, _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 > 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 > Cc: Erin Lo > Signed-off-by: Michael Turquette > --- > 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, _orphan_list, child_node) { > struct clk_core *parent = __clk_init_parent(orphan); > > - if (parent) > - clk_core_reparent(orphan, parent); Is it correct to remove clk_core_reparent()? It lacks __clk_recalc_accuracies() and __clk_recalc_rates(), so the new parent's rate will not propagate correctly. For example, I set vdec_sel as a critical clock. Without your patch, the result was: vdecpll 00 33800 vdecpll_ck 11 33800 vdec_sel 11 33800 With your patch, it became: vdecpll 11 33800 vdecpll_ck 11 0 vdec_sel 11 0 The prepare_count and enable_count are correct with your patch, but the rates of vdecpll_ck and vdec_sel become incorrect. Best regards, James > + /* > + * 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);
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
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, _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
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
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, _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
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> > > > > > This patch fixed wrong state of parent clocks if they are registered > > > after critical clocks. > > > > > > Signed-off-by: James Liao > > > Signed-off-by: Erin Lo > > > > 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, _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 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 Cc: Erin Lo Signed-off-by: Michael Turquette --- 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, _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)
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 > > > > > > This patch fixed wrong state of parent clocks if they are registered > > > after critical clocks. > > > > > > Signed-off-by: James Liao > > > Signed-off-by: Erin Lo > > > > 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, _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 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 Cc: Erin Lo Signed-off-by: Michael Turquette --- 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, _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)
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On Fri, 2016-07-01 at 18:21 -0700, Stephen Boyd wrote: > (Resending to everyone) > > On 06/22, Erin Lo wrote: > > From: James Liao> > > > This patch fixed wrong state of parent clocks if they are registered > > after critical clocks. > > > > Signed-off-by: James Liao > > Signed-off-by: Erin Lo > > 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, _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
Re: [PATCH v9 01/10] clk: fix initial state of critical clock's parents
On Fri, 2016-07-01 at 18:21 -0700, Stephen Boyd wrote: > (Resending to everyone) > > On 06/22, Erin Lo wrote: > > From: James Liao > > > > This patch fixed wrong state of parent clocks if they are registered > > after critical clocks. > > > > Signed-off-by: James Liao > > Signed-off-by: Erin Lo > > 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, _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
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> > This patch fixed wrong state of parent clocks if they are registered > after critical clocks. > > Signed-off-by: James Liao > Signed-off-by: Erin Lo 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, _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
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 > > This patch fixed wrong state of parent clocks if they are registered > after critical clocks. > > Signed-off-by: James Liao > Signed-off-by: Erin Lo 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, _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