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

Groups > linux.kernel > #1580866 > unrolled thread

[PATCH] clk: rockchip: Set "ignore unused" for PMU M0 clocks on rk3399

Back to article view | Back to linux.kernel

Contents

  [PATCH] clk: rockchip: Set "ignore unused" for PMU M0 clocks on rk3399 Douglas Anderson <dianders@chromium.org> - 2017-02-14 22:10 +0100
    Re: [PATCH] clk: rockchip: Set "ignore unused" for PMU M0 clocks on rk3399 Javier Martinez Canillas <javier@dowhile0.org> - 2017-02-15 16:30 +0100
      Re: [PATCH] clk: rockchip: Set "ignore unused" for PMU M0 clocks on rk3399 Doug Anderson <dianders@chromium.org> - 2017-02-15 17:50 +0100
      Re: [PATCH] clk: rockchip: Set "ignore unused" for PMU M0 clocks on rk3399 Javier Martinez Canillas <javier@dowhile0.org> - 2017-02-15 18:10 +0100
      Re: [PATCH] clk: rockchip: Set "ignore unused" for PMU M0 clocks on rk3399 Heiko Stübner <heiko@sntech.de> - 2017-02-15 18:10 +0100
    Re: [PATCH] clk: rockchip: Set "ignore unused" for PMU M0 clocks on rk3399 Heiko Stuebner <heiko@sntech.de> - 2017-02-21 18:50 +0100

#1580866 — [PATCH] clk: rockchip: Set "ignore unused" for PMU M0 clocks on rk3399

The PMU Cortex M0 on rk3399 is intended to be used for things like
DDRFreq transitions, suspend/resume, and other things that are the
purview of ARM Trusted Firmware and not the kernel.  As such, the
kernel shouldn't be messing with the clocks.  Add CLK_IGNORE_UNUSED to
these clocks.

Without this change, the following was observed on a Chromebook with a
rk3399 (using not-yet-upstream ARM Trusted Firmware code and
not-yet-upstream kernel code based on kernel-4.4):

1. We init the clock framework.

2. We start up "DDRFreq", which causes ATF to occasionally fire up the
   M0 for transitions.  Each time ATF fires up the M0 it will turn on
   these clocks and each time it is done it will turn them off.

3. We finally get to the the part of the kernel that calls
   clk_disable_unused() and we disables the clocks.

You can see the race above.  Basically everything is fine as long as
ARM Trusted Firmware isn't starting up the M0 at exactly the same time
that the kernel is disabling unused clocks.  ...but if the race
happens then we go boom.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/clk/rockchip/clk-rk3399.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 73121b144634..fa3cbef08776 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -1477,10 +1477,10 @@ static struct rockchip_clk_branch rk3399_clk_pmu_branches[] __initdata = {
 	GATE(PCLK_UART4_PMU, "pclk_uart4_pmu", "pclk_pmu_src", 0, RK3399_PMU_CLKGATE_CON(1), 14, GFLAGS),
 	GATE(PCLK_WDT_M0_PMU, "pclk_wdt_m0_pmu", "pclk_pmu_src", 0, RK3399_PMU_CLKGATE_CON(1), 15, GFLAGS),
 
-	GATE(FCLK_CM0S_PMU, "fclk_cm0s_pmu", "fclk_cm0s_src_pmu", 0, RK3399_PMU_CLKGATE_CON(2), 0, GFLAGS),
-	GATE(SCLK_CM0S_PMU, "sclk_cm0s_pmu", "fclk_cm0s_src_pmu", 0, RK3399_PMU_CLKGATE_CON(2), 1, GFLAGS),
-	GATE(HCLK_CM0S_PMU, "hclk_cm0s_pmu", "fclk_cm0s_src_pmu", 0, RK3399_PMU_CLKGATE_CON(2), 2, GFLAGS),
-	GATE(DCLK_CM0S_PMU, "dclk_cm0s_pmu", "fclk_cm0s_src_pmu", 0, RK3399_PMU_CLKGATE_CON(2), 3, GFLAGS),
+	GATE(FCLK_CM0S_PMU, "fclk_cm0s_pmu", "fclk_cm0s_src_pmu", CLK_IGNORE_UNUSED, RK3399_PMU_CLKGATE_CON(2), 0, GFLAGS),
+	GATE(SCLK_CM0S_PMU, "sclk_cm0s_pmu", "fclk_cm0s_src_pmu", CLK_IGNORE_UNUSED, RK3399_PMU_CLKGATE_CON(2), 1, GFLAGS),
+	GATE(HCLK_CM0S_PMU, "hclk_cm0s_pmu", "fclk_cm0s_src_pmu", CLK_IGNORE_UNUSED, RK3399_PMU_CLKGATE_CON(2), 2, GFLAGS),
+	GATE(DCLK_CM0S_PMU, "dclk_cm0s_pmu", "fclk_cm0s_src_pmu", CLK_IGNORE_UNUSED, RK3399_PMU_CLKGATE_CON(2), 3, GFLAGS),
 	GATE(HCLK_NOC_PMU, "hclk_noc_pmu", "fclk_cm0s_src_pmu", CLK_IGNORE_UNUSED, RK3399_PMU_CLKGATE_CON(2), 5, GFLAGS),
 };
 
-- 
2.11.0.483.g087da7b7c-goog

[toc] | [next] | [standalone]

#1581403

Hello Doug,

On Tue, Feb 14, 2017 at 6:01 PM, Douglas Anderson <dianders@chromium.org> wrote:
> The PMU Cortex M0 on rk3399 is intended to be used for things like
> DDRFreq transitions, suspend/resume, and other things that are the
> purview of ARM Trusted Firmware and not the kernel.  As such, the
> kernel shouldn't be messing with the clocks.  Add CLK_IGNORE_UNUSED to
> these clocks.
>

Isn't CLK_IS_CRITICAL a more suitable flag for this case?

Best regards,
Javier

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

#1581445

Hi,

On Wed, Feb 15, 2017 at 7:27 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Doug,
>
> On Tue, Feb 14, 2017 at 6:01 PM, Douglas Anderson <dianders@chromium.org> wrote:
>> The PMU Cortex M0 on rk3399 is intended to be used for things like
>> DDRFreq transitions, suspend/resume, and other things that are the
>> purview of ARM Trusted Firmware and not the kernel.  As such, the
>> kernel shouldn't be messing with the clocks.  Add CLK_IGNORE_UNUSED to
>> these clocks.
>>
>
> Isn't CLK_IS_CRITICAL a more suitable flag for this case?

As I understand it (AKA please correct me if I'm wrong)...

Usually CLK_IS_CRITICAL is more suitable than CLK_IGNORE_UNUSED since
lots of old code used CLK_IGNORE_UNUSED for critical clocks before
CLK_IS_CRITICAL existed.

...but in this case, I don't think it is more suitable.
CLK_IS_CRITICAL means that the kernel should be in charge of keeping
this clock on at all times.  The documentation I see says:

#define CLK_IS_CRITICAL         BIT(11) /* do not gate, ever */

In our case, as was so eloquently described in our private bug by our
firmware engineer:

  Just tell the kernel to keep its grubby hands off my clocks completely

AKA: this isn't a clock that the kernel should touch--it is entirely
managed by the firmware.  It's OK for the kernel to show it in the
clock tree, but otherwise "hands off".

-Doug

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

#1581476

Hello Heiko and Doug,

On Wed, Feb 15, 2017 at 2:01 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Mittwoch, 15. Februar 2017, 12:27:59 CET schrieb Javier Martinez Canillas:
>> Hello Doug,
>>
>> On Tue, Feb 14, 2017 at 6:01 PM, Douglas Anderson <dianders@chromium.org>
> wrote:
>> > The PMU Cortex M0 on rk3399 is intended to be used for things like
>> > DDRFreq transitions, suspend/resume, and other things that are the
>> > purview of ARM Trusted Firmware and not the kernel.  As such, the
>> > kernel shouldn't be messing with the clocks.  Add CLK_IGNORE_UNUSED to
>> > these clocks.
>>
>> Isn't CLK_IS_CRITICAL a more suitable flag for this case?
>
> From the patch description it looks like the clock is expected to be
> controlled from firmware in most cases as I guess the Cortex M0 will be used
> for that all the time now. And the clock is not expected to run all the time.
>
> So I'd think clk_ignore_unused is the correct one. The whole clock-subtree for
> these clocks also does not get affected by other clocks, as it is
> independendly coming from PLLs.
>
>

I see. Thanks a lot for your explanations. I just asked because
CLK_IGNORE_UNUSED only prevents the clock to be disabled by
clk_disable_unused() but still can be disabled if a driver explicitly
looks it up and calls clk_disable() or if the clock is affected by
other clocks.

But I understand now that both scenarios are not possible. So yes,
CLK_IGNORE_UNUSED is more suitable in this case.

> Heiko

Best regards,
Javier

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

#1581478

Am Mittwoch, 15. Februar 2017, 12:27:59 CET schrieb Javier Martinez Canillas:
> Hello Doug,
> 
> On Tue, Feb 14, 2017 at 6:01 PM, Douglas Anderson <dianders@chromium.org> 
wrote:
> > The PMU Cortex M0 on rk3399 is intended to be used for things like
> > DDRFreq transitions, suspend/resume, and other things that are the
> > purview of ARM Trusted Firmware and not the kernel.  As such, the
> > kernel shouldn't be messing with the clocks.  Add CLK_IGNORE_UNUSED to
> > these clocks.
> 
> Isn't CLK_IS_CRITICAL a more suitable flag for this case?

From the patch description it looks like the clock is expected to be 
controlled from firmware in most cases as I guess the Cortex M0 will be used 
for that all the time now. And the clock is not expected to run all the time.

So I'd think clk_ignore_unused is the correct one. The whole clock-subtree for 
these clocks also does not get affected by other clocks, as it is 
independendly coming from PLLs.


Heiko

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

#1585565

Am Dienstag, 14. Februar 2017, 13:01:14 CET schrieb Douglas Anderson:
> The PMU Cortex M0 on rk3399 is intended to be used for things like
> DDRFreq transitions, suspend/resume, and other things that are the
> purview of ARM Trusted Firmware and not the kernel.  As such, the
> kernel shouldn't be messing with the clocks.  Add CLK_IGNORE_UNUSED to
> these clocks.
> 
> Without this change, the following was observed on a Chromebook with a
> rk3399 (using not-yet-upstream ARM Trusted Firmware code and
> not-yet-upstream kernel code based on kernel-4.4):
> 
> 1. We init the clock framework.
> 
> 2. We start up "DDRFreq", which causes ATF to occasionally fire up the
>    M0 for transitions.  Each time ATF fires up the M0 it will turn on
>    these clocks and each time it is done it will turn them off.
> 
> 3. We finally get to the the part of the kernel that calls
>    clk_disable_unused() and we disables the clocks.
> 
> You can see the race above.  Basically everything is fine as long as
> ARM Trusted Firmware isn't starting up the M0 at exactly the same time
> that the kernel is disabling unused clocks.  ...but if the race
> happens then we go boom.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

applied for 4.12

Thanks
Heiko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web