From: Saravana Kannan <skannan@codeaurora.org> To: "Turquette, Mike" <mturquette@ti.com> Cc: Shawn Guo <shawn.guo@linaro.org>, Paul Walmsley <paul@pwsan.com>, Russell King <linux@arm.linux.org.uk>, Linus Walleij <linus.walleij@stericsson.com>, patches@linaro.org, Magnus Damm <magnus.damm@gmail.com>, Sascha Hauer <s.hauer@pengutronix.de>, Mark Brown <broonie@opensource.wolfsonmicro.com>, Stephen Boyd <sboyd@codeaurora.org>, linux-kernel@vger.kernel.org, linaro-dev@lists.linaro.org, Jeremy Kerr <jeremy.kerr@canonical.com>, linux-arm-kernel@lists.infradead.org, Arnd Bergman <arnd.bergmann@linaro.org> Subject: Re: [PATCH v7 2/3] clk: introduce the common clock framework Date: Fri, 23 Mar 2012 16:04:07 -0700 [thread overview] Message-ID: <4F6D0167.3040704@codeaurora.org> (raw) In-Reply-To: <CAJOA=zOnR1NwZ1Cmkfb+b7f7=N_vg+txAUqKXF2Nf2nVYFMjjg@mail.gmail.com> On 03/23/2012 03:32 PM, Turquette, Mike wrote: > On Fri, Mar 23, 2012 at 3:12 PM, Saravana Kannan<skannan@codeaurora.org> wrote: >> On 03/23/2012 02:39 PM, Turquette, Mike wrote: >>> __clk_recalc_rates is called by __clk_reparent which is called by >>> clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. >>> >>> Does this not handle the old cached clk->rate for you? >> >> For the set_parent case, ops->recalc_rate() is called twice. Once for >> PRE_CHANGE and once for POST_CHANGE. For this clock, I can only really >> recalc the rate during the POST_CHANGE call. So, how should I differentiate >> the two cases? > > .recalc_rate serves two purposes: first it recalculates the rate after > the rate has changed and you pass in a new parent_rate argument. The > second purpose is to "speculate" a rate change. You can pass in any > rate for the parent_rate parameter when you call .recalc_rates. This > is what __speculate_rates does before the rate changes. For > clk_set_parent we call, > > __clk_speculate_rates(clk, parent->rate); > > Where parent above is the *new* parent. So this will let us propagate > pre-rate change notifiers with the new rate. > > Your .recalc_rate callback doesn't need to differentiate between the > two calls to .recalc_rate. It should just take in the parent_rate > value and do the calculation required of it. Yeah, this is generally true. But, in this specific case, the clock is a mux that can literally measure the real rate. I would like the rate of this mux clock to be the real measured rate and not just the parent rate. I could actually measure the current rate and return that for recalc_rate(), but then the reported rate change during the pre-change notification would be wrong. Having the "msg" will let us at least fake the rate correctly for pre change notifiers. >> I think it's quite useful for recalc_rate to be called pre/post change (some >> steps have to be done pre/post change depending on whether the parent rate >> is increasing or decreasing). But I don't see the "msg" being passed along. > > What kind of steps? Does your .recalc_rate perform these steps? I > need more details to understand your requirements. Nevermind, my brain isn't working today. I can just do that different ordering in ops->set_parent(). But the measure clocks case is still true though. But this did bring up another point in my head. Hopefully this one isn't my brain playing tricks again today. How does a child (or grand child) clock (not a driver using the clock) reject a rate change if it know it can't handle that freq from the parent? I won't claim to know this part of the code thoroughly, but I can't find an easy way to do this. Reason for rejection could be things like the counter (for division, etc) has an upper limit on how fast it can run. >> Also, I noticed that clk_set_parent() is treating a NULL as an invalid >> clock. Should that be fixed? set_parent(NULL) could be treated as a >> grounding the clock. Should we let the ops->set_parent determine if NULL is >> valid option? > > We must be looking at different code. clk_set_parent doesn't return > any error if parent == NULL. Bringing this to my attention does show > that we do deref the parent pointer without a check though... > > Do you have a real use case for this? Due to the way that we match > the parent pointer with the cached clk->parents member it would be > painful to support NULL parents as valid. I don't have a real use case for MSM. We just have a ground_clock. > It is also worth considering whether clk_set_parent is really the > correct operation for grounding a clock. clk_unprepare might be a > better candidate. Yeah, not a real use case for MSM. Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
WARNING: multiple messages have this Message-ID (diff)
From: skannan@codeaurora.org (Saravana Kannan) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v7 2/3] clk: introduce the common clock framework Date: Fri, 23 Mar 2012 16:04:07 -0700 [thread overview] Message-ID: <4F6D0167.3040704@codeaurora.org> (raw) In-Reply-To: <CAJOA=zOnR1NwZ1Cmkfb+b7f7=N_vg+txAUqKXF2Nf2nVYFMjjg@mail.gmail.com> On 03/23/2012 03:32 PM, Turquette, Mike wrote: > On Fri, Mar 23, 2012 at 3:12 PM, Saravana Kannan<skannan@codeaurora.org> wrote: >> On 03/23/2012 02:39 PM, Turquette, Mike wrote: >>> __clk_recalc_rates is called by __clk_reparent which is called by >>> clk_set_parent. __clk_recalc_rates is also called by clk_set_rate. >>> >>> Does this not handle the old cached clk->rate for you? >> >> For the set_parent case, ops->recalc_rate() is called twice. Once for >> PRE_CHANGE and once for POST_CHANGE. For this clock, I can only really >> recalc the rate during the POST_CHANGE call. So, how should I differentiate >> the two cases? > > .recalc_rate serves two purposes: first it recalculates the rate after > the rate has changed and you pass in a new parent_rate argument. The > second purpose is to "speculate" a rate change. You can pass in any > rate for the parent_rate parameter when you call .recalc_rates. This > is what __speculate_rates does before the rate changes. For > clk_set_parent we call, > > __clk_speculate_rates(clk, parent->rate); > > Where parent above is the *new* parent. So this will let us propagate > pre-rate change notifiers with the new rate. > > Your .recalc_rate callback doesn't need to differentiate between the > two calls to .recalc_rate. It should just take in the parent_rate > value and do the calculation required of it. Yeah, this is generally true. But, in this specific case, the clock is a mux that can literally measure the real rate. I would like the rate of this mux clock to be the real measured rate and not just the parent rate. I could actually measure the current rate and return that for recalc_rate(), but then the reported rate change during the pre-change notification would be wrong. Having the "msg" will let us at least fake the rate correctly for pre change notifiers. >> I think it's quite useful for recalc_rate to be called pre/post change (some >> steps have to be done pre/post change depending on whether the parent rate >> is increasing or decreasing). But I don't see the "msg" being passed along. > > What kind of steps? Does your .recalc_rate perform these steps? I > need more details to understand your requirements. Nevermind, my brain isn't working today. I can just do that different ordering in ops->set_parent(). But the measure clocks case is still true though. But this did bring up another point in my head. Hopefully this one isn't my brain playing tricks again today. How does a child (or grand child) clock (not a driver using the clock) reject a rate change if it know it can't handle that freq from the parent? I won't claim to know this part of the code thoroughly, but I can't find an easy way to do this. Reason for rejection could be things like the counter (for division, etc) has an upper limit on how fast it can run. >> Also, I noticed that clk_set_parent() is treating a NULL as an invalid >> clock. Should that be fixed? set_parent(NULL) could be treated as a >> grounding the clock. Should we let the ops->set_parent determine if NULL is >> valid option? > > We must be looking at different code. clk_set_parent doesn't return > any error if parent == NULL. Bringing this to my attention does show > that we do deref the parent pointer without a check though... > > Do you have a real use case for this? Due to the way that we match > the parent pointer with the cached clk->parents member it would be > painful to support NULL parents as valid. I don't have a real use case for MSM. We just have a ground_clock. > It is also worth considering whether clk_set_parent is really the > correct operation for grounding a clock. clk_unprepare might be a > better candidate. Yeah, not a real use case for MSM. Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
next prev parent reply other threads:[~2012-03-23 23:04 UTC|newest] Thread overview: 242+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-03-16 6:11 [PATCH v7 0/3] common clk framework Mike Turquette 2012-03-16 6:11 ` Mike Turquette 2012-03-16 6:11 ` [PATCH v7 1/3] Documentation: common clk API Mike Turquette 2012-03-16 6:11 ` Mike Turquette 2012-03-16 8:25 ` Linus Walleij 2012-03-16 8:25 ` Linus Walleij 2012-03-16 10:29 ` Thomas Gleixner 2012-03-16 10:29 ` Thomas Gleixner 2012-03-16 11:14 ` Amit Kucheria 2012-03-16 11:14 ` Amit Kucheria 2012-03-16 12:18 ` Arnd Bergmann 2012-03-16 12:18 ` Arnd Bergmann 2012-03-16 20:57 ` Arnd Bergmann 2012-03-16 20:57 ` Arnd Bergmann 2012-03-16 21:40 ` Turquette, Mike 2012-03-16 21:40 ` Turquette, Mike 2012-03-16 21:50 ` Nicolas Pitre 2012-03-16 21:50 ` Nicolas Pitre 2012-03-16 22:21 ` Paul Walmsley 2012-03-16 22:21 ` Paul Walmsley 2012-03-16 22:21 ` Paul Walmsley 2012-03-16 22:33 ` Turquette, Mike 2012-03-16 22:33 ` Turquette, Mike 2012-03-17 9:05 ` Arnd Bergmann 2012-03-17 9:05 ` Arnd Bergmann 2012-03-17 9:05 ` Arnd Bergmann 2012-03-17 18:02 ` Turquette, Mike 2012-03-17 18:02 ` Turquette, Mike 2012-03-17 18:02 ` Turquette, Mike 2012-03-17 18:33 ` Arnd Bergmann 2012-03-17 18:33 ` Arnd Bergmann 2012-03-17 18:33 ` Arnd Bergmann 2012-03-17 20:29 ` Sascha Hauer 2012-03-17 20:29 ` Sascha Hauer 2012-03-17 20:29 ` Sascha Hauer 2012-03-17 21:13 ` Arnd Bergmann 2012-03-17 21:13 ` Arnd Bergmann 2012-03-17 21:13 ` Arnd Bergmann 2012-03-20 23:40 ` Paul Walmsley 2012-03-20 23:40 ` Paul Walmsley 2012-03-21 8:59 ` Arnd Bergmann 2012-03-21 8:59 ` Arnd Bergmann 2012-03-16 23:47 ` Sascha Hauer 2012-03-16 23:47 ` Sascha Hauer 2012-03-16 23:47 ` Sascha Hauer 2012-03-17 0:54 ` Rob Herring 2012-03-17 0:54 ` Rob Herring 2012-03-17 0:54 ` Rob Herring 2012-03-17 3:38 ` Saravana Kannan 2012-03-17 3:38 ` Saravana Kannan 2012-03-17 3:38 ` Saravana Kannan 2012-03-20 23:31 ` Paul Walmsley 2012-03-20 23:31 ` Paul Walmsley 2012-03-20 23:31 ` Paul Walmsley 2012-03-21 3:15 ` Nicolas Pitre 2012-03-21 3:15 ` Nicolas Pitre 2012-03-21 3:15 ` Nicolas Pitre 2012-03-21 3:26 ` Saravana Kannan 2012-03-21 3:26 ` Saravana Kannan 2012-03-21 3:26 ` Saravana Kannan 2012-03-21 7:44 ` Paul Walmsley 2012-03-21 7:44 ` Paul Walmsley 2012-03-21 7:44 ` Paul Walmsley 2012-03-21 9:10 ` Sascha Hauer 2012-03-21 9:10 ` Sascha Hauer 2012-03-21 9:10 ` Sascha Hauer 2012-03-21 18:38 ` Saravana Kannan 2012-03-21 18:38 ` Saravana Kannan 2012-03-21 18:38 ` Saravana Kannan 2012-03-21 19:07 ` Mark Brown 2012-03-21 19:07 ` Mark Brown 2012-03-21 19:07 ` Mark Brown 2012-03-21 19:33 ` Tony Lindgren 2012-03-21 19:33 ` Tony Lindgren 2012-03-21 19:33 ` Tony Lindgren 2012-03-21 19:41 ` Saravana Kannan 2012-03-21 19:41 ` Saravana Kannan 2012-03-21 19:41 ` Saravana Kannan 2012-03-21 19:56 ` Mark Brown 2012-03-21 19:56 ` Mark Brown 2012-03-21 19:56 ` Mark Brown 2012-03-21 20:04 ` Saravana Kannan 2012-03-21 20:04 ` Saravana Kannan 2012-03-21 20:04 ` Saravana Kannan 2012-03-21 20:10 ` Mark Brown 2012-03-21 20:10 ` Mark Brown 2012-03-21 20:10 ` Mark Brown 2012-03-22 0:42 ` Russell King - ARM Linux 2012-03-22 0:42 ` Russell King - ARM Linux 2012-03-22 0:42 ` Russell King - ARM Linux 2012-03-21 7:30 ` Paul Walmsley 2012-03-21 7:30 ` Paul Walmsley 2012-03-21 7:30 ` Paul Walmsley 2012-03-21 13:23 ` Nicolas Pitre 2012-03-21 13:23 ` Nicolas Pitre 2012-03-21 13:23 ` Nicolas Pitre 2012-03-16 6:11 ` [PATCH v7 2/3] clk: introduce the common clock framework Mike Turquette 2012-03-16 6:11 ` Mike Turquette 2012-03-17 3:28 ` Saravana Kannan 2012-03-17 3:28 ` Saravana Kannan 2012-03-19 18:56 ` Turquette, Mike 2012-03-19 18:56 ` Turquette, Mike 2012-03-19 19:13 ` Saravana Kannan 2012-03-19 19:13 ` Saravana Kannan 2012-03-19 19:33 ` Turquette, Mike 2012-03-19 19:33 ` Turquette, Mike 2012-03-19 19:49 ` Saravana Kannan 2012-03-19 19:49 ` Saravana Kannan 2012-03-20 3:38 ` [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn Saravana Kannan 2012-03-20 3:38 ` Saravana Kannan 2012-03-20 3:38 ` [PATCH 2/2] clk: Move init fields from clk to clk_hw Saravana Kannan 2012-03-20 3:38 ` Saravana Kannan 2012-03-20 7:20 ` Shawn Guo 2012-03-20 7:20 ` Shawn Guo 2012-03-20 7:54 ` Saravana Kannan 2012-03-20 7:54 ` Saravana Kannan 2012-03-20 7:54 ` Saravana Kannan 2012-03-20 8:13 ` Shawn Guo 2012-03-20 8:13 ` Shawn Guo 2012-03-20 9:40 ` Sascha Hauer 2012-03-20 9:40 ` Sascha Hauer 2012-03-20 10:17 ` Saravana Kannan 2012-03-20 10:17 ` Saravana Kannan 2012-03-20 10:17 ` Saravana Kannan 2012-03-20 18:14 ` Sascha Hauer 2012-03-20 18:14 ` Sascha Hauer 2012-03-20 20:14 ` Saravana Kannan 2012-03-20 20:14 ` Saravana Kannan 2012-03-20 22:40 ` Sascha Hauer 2012-03-20 22:40 ` Sascha Hauer 2012-03-22 3:23 ` Shawn Guo 2012-03-22 3:23 ` Shawn Guo 2012-03-20 14:18 ` Shawn Guo 2012-03-20 14:18 ` Shawn Guo 2012-03-20 18:10 ` Sascha Hauer 2012-03-20 18:10 ` Sascha Hauer 2012-03-20 20:06 ` Saravana Kannan 2012-03-20 20:06 ` Saravana Kannan 2012-03-20 23:12 ` Sascha Hauer 2012-03-20 23:12 ` Sascha Hauer 2012-03-21 1:47 ` Turquette, Mike 2012-03-21 1:47 ` Turquette, Mike 2012-03-21 3:01 ` Saravana Kannan 2012-03-21 3:01 ` Saravana Kannan 2012-03-27 4:35 ` Saravana Kannan 2012-03-27 4:35 ` Saravana Kannan 2012-03-27 18:49 ` Turquette, Mike 2012-03-27 18:49 ` Turquette, Mike 2012-03-27 22:27 ` Saravana Kannan 2012-03-27 22:27 ` Saravana Kannan 2012-04-06 1:30 ` Saravana Kannan 2012-04-06 1:30 ` Saravana Kannan 2012-04-11 17:59 ` Turquette, Mike 2012-04-11 17:59 ` Turquette, Mike 2012-04-11 19:57 ` Saravana Kannan 2012-04-11 19:57 ` Saravana Kannan 2012-04-11 19:57 ` Saravana Kannan 2012-04-11 20:17 ` Turquette, Mike 2012-04-11 20:17 ` Turquette, Mike 2012-04-11 20:21 ` Saravana Kannan 2012-04-11 20:21 ` Saravana Kannan 2012-04-11 20:21 ` Saravana Kannan 2012-03-20 23:47 ` Paul Walmsley 2012-03-20 23:47 ` Paul Walmsley 2012-03-21 9:16 ` Sascha Hauer 2012-03-21 9:16 ` Sascha Hauer 2012-03-20 7:19 ` [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn Sascha Hauer 2012-03-20 7:19 ` Sascha Hauer 2012-03-20 7:46 ` Saravana Kannan 2012-03-20 7:46 ` Saravana Kannan 2012-03-20 7:46 ` Saravana Kannan 2012-03-21 0:13 ` Turquette, Mike 2012-03-21 0:13 ` Turquette, Mike 2012-03-21 2:32 ` Saravana Kannan 2012-03-21 2:32 ` Saravana Kannan 2012-03-21 5:45 ` Turquette, Mike 2012-03-21 5:45 ` Turquette, Mike 2012-03-21 6:33 ` Saravana Kannan 2012-03-21 6:33 ` Saravana Kannan 2012-03-21 6:33 ` Saravana Kannan 2012-03-21 9:07 ` Russell King - ARM Linux 2012-03-21 9:07 ` Russell King - ARM Linux 2012-03-21 19:56 ` Turquette, Mike 2012-03-21 19:56 ` Turquette, Mike 2012-03-18 13:46 ` [PATCH v7 2/3] clk: introduce the common clock framework Shawn Guo 2012-03-18 13:46 ` Shawn Guo 2012-03-19 18:58 ` Turquette, Mike 2012-03-19 18:58 ` Turquette, Mike 2012-03-18 14:07 ` Shawn Guo 2012-03-18 14:07 ` Shawn Guo 2012-03-19 19:00 ` Turquette, Mike 2012-03-19 19:00 ` Turquette, Mike 2012-03-19 11:22 ` Rajendra Nayak 2012-03-19 11:22 ` Rajendra Nayak 2012-03-19 11:28 ` Sascha Hauer 2012-03-19 11:28 ` Sascha Hauer 2012-03-19 19:09 ` Turquette, Mike 2012-03-19 19:09 ` Turquette, Mike 2012-03-19 19:53 ` Turquette, Mike 2012-03-19 19:53 ` Turquette, Mike 2012-03-20 14:02 ` Shawn Guo 2012-03-20 14:02 ` Shawn Guo 2012-03-20 17:46 ` Saravana Kannan 2012-03-20 17:46 ` Saravana Kannan 2012-03-20 23:53 ` Turquette, Mike 2012-03-20 23:53 ` Turquette, Mike 2012-03-21 3:10 ` Saravana Kannan 2012-03-21 3:10 ` Saravana Kannan 2012-03-23 21:33 ` Saravana Kannan 2012-03-23 21:33 ` Saravana Kannan 2012-03-23 21:39 ` Turquette, Mike 2012-03-23 21:39 ` Turquette, Mike 2012-03-23 21:51 ` Saravana Kannan 2012-03-23 21:51 ` Saravana Kannan 2012-03-23 22:12 ` Saravana Kannan 2012-03-23 22:12 ` Saravana Kannan 2012-03-23 22:32 ` Turquette, Mike 2012-03-23 22:32 ` Turquette, Mike 2012-03-23 23:04 ` Saravana Kannan [this message] 2012-03-23 23:04 ` Saravana Kannan 2012-03-23 23:28 ` Turquette, Mike 2012-03-23 23:28 ` Turquette, Mike 2012-03-28 3:06 ` Saravana Kannan 2012-03-28 3:06 ` Saravana Kannan 2012-03-28 17:08 ` Turquette, Mike 2012-03-28 17:08 ` Turquette, Mike 2012-03-28 22:25 ` Saravana Kannan 2012-03-28 22:25 ` Saravana Kannan 2012-03-28 23:49 ` Turquette, Mike 2012-03-28 23:49 ` Turquette, Mike 2012-03-20 23:46 ` Turquette, Mike 2012-03-20 23:46 ` Turquette, Mike 2012-03-21 5:46 ` Shawn Guo 2012-03-21 5:46 ` Shawn Guo 2012-03-16 6:11 ` [PATCH v7 3/3] clk: basic clock hardware types Mike Turquette 2012-03-16 6:11 ` Mike Turquette 2012-03-16 12:25 ` Richard Zhao 2012-03-16 12:25 ` Richard Zhao 2012-03-16 16:51 ` Turquette, Mike 2012-03-16 16:51 ` Turquette, Mike 2012-03-16 10:57 ` [PATCH v7 0/3] common clk framework Sascha Hauer 2012-03-16 10:57 ` Sascha Hauer
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=4F6D0167.3040704@codeaurora.org \ --to=skannan@codeaurora.org \ --cc=arnd.bergmann@linaro.org \ --cc=broonie@opensource.wolfsonmicro.com \ --cc=jeremy.kerr@canonical.com \ --cc=linaro-dev@lists.linaro.org \ --cc=linus.walleij@stericsson.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=magnus.damm@gmail.com \ --cc=mturquette@ti.com \ --cc=patches@linaro.org \ --cc=paul@pwsan.com \ --cc=s.hauer@pengutronix.de \ --cc=sboyd@codeaurora.org \ --cc=shawn.guo@linaro.org \ /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.