From: Mike Turquette <mturquette@linaro.org> To: Tero Kristo <t-kristo@ti.com>, "Stephen Boyd" <sboyd@codeaurora.org> Cc: linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, tony@atomide.com, nm@ti.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] clk: prevent erronous parsing of children during rate change Date: Fri, 26 Sep 2014 16:24:53 -0700 [thread overview] Message-ID: <20140926232453.19023.25483@quantum> (raw) In-Reply-To: <5425135F.2070809@ti.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: mturquette@linaro.org (Mike Turquette) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] clk: prevent erronous parsing of children during rate change Date: Fri, 26 Sep 2014 16:24:53 -0700 [thread overview] Message-ID: <20140926232453.19023.25483@quantum> (raw) In-Reply-To: <5425135F.2070809@ti.com> 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
next prev parent reply other threads:[~2014-09-26 23:25 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-08-21 13:47 [PATCH] clk: prevent erronous parsing of children during rate change Tero Kristo 2014-08-21 13:47 ` Tero Kristo 2014-08-21 13:47 ` Tero Kristo 2014-08-21 13:59 ` Nishanth Menon 2014-08-21 13:59 ` Nishanth Menon 2014-08-21 13:59 ` Nishanth Menon 2014-09-03 19:22 ` Mike Turquette 2014-09-03 19:22 ` Mike Turquette 2014-09-17 18:27 ` Felipe Balbi 2014-09-17 18:27 ` Felipe Balbi 2014-09-17 18:27 ` Felipe Balbi 2014-09-22 19:18 ` Stephen Boyd 2014-09-22 19:18 ` Stephen Boyd 2014-09-23 13:38 ` Tero Kristo 2014-09-23 13:38 ` Tero Kristo 2014-09-23 13:38 ` Tero Kristo 2014-09-26 1:35 ` Stephen Boyd 2014-09-26 1:35 ` Stephen Boyd 2014-09-26 7:18 ` Tero Kristo 2014-09-26 7:18 ` Tero Kristo 2014-09-26 7:18 ` Tero Kristo 2014-09-26 23:24 ` Mike Turquette [this message] 2014-09-26 23:24 ` Mike Turquette 2014-09-29 8:09 ` Tero Kristo 2014-09-29 8:09 ` Tero Kristo 2014-09-29 8:09 ` Tero Kristo 2014-09-30 7:07 ` Mike Turquette 2014-09-30 7:07 ` Mike Turquette 2014-09-30 8:48 ` Tero Kristo 2014-09-30 8:48 ` Tero Kristo 2014-09-30 8:48 ` Tero Kristo 2014-09-30 19:03 ` Mike Turquette 2014-09-30 19:03 ` Mike Turquette 2014-10-02 13:31 ` Tero Kristo 2014-10-02 13:31 ` Tero Kristo 2014-10-02 13:31 ` Tero Kristo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140926232453.19023.25483@quantum \ --to=mturquette@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=nm@ti.com \ --cc=sboyd@codeaurora.org \ --cc=t-kristo@ti.com \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.