[PATCH V4 0/2] clk: bcm2835: use CLK_IS_CRITICAL/CLK_ENABLE_HAND_OFF

[PATCH V4 0/2] clk: bcm2835: use CLK_IS_CRITICAL/CLK_ENABLE_HAND_OFF

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

The bcm2835 firmware enables several clocks and plls before
booting the linux kernel, when these later are claimed by a driver
(temporarily) and then released, then the corresponding plls are
disabled, which result in a crashed system.

Ideally we want to use CLK_ENABLE_HAND_OFF to mark those already
enabled clocks, but as CLK_ENABLE_HAND_OFF is not merged
yet this patchset takes a 2 setp approach:
In the first patch it first makes use of the - already in
clk-next - CLK_IS_CRITICAL.
The second patch will change the use to CLK_ENABLE_HAND_OFF,
which can get applied when the HAND_OFF patch-set goes in.

Martin Sperl (2):
  clk: bcm2835: mark enabled clocks with CLK_IS_CRITICAL
  clk: bcm2835: use CLK_ENABLE_HAND_OFF instead of CLK_IS_CRITICAL

 drivers/clk/bcm/clk-bcm2835.c | 8 ++++++++
 1 file changed, 8 insertions(+)

--
2.1.4

[PATCH V4 1/2] clk: bcm2835: mark enabled clocks with CLK_IS_CRITICAL

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

The bcm2835 firmware enables several clocks and plls before
booting the linux kernel.

These plls should never get disabled as it may result in a
stopped system clock and more.

So during probing we check if the clock is enabled
and if it is then mark that clock with CLK_IS_CRITICAL.

As a consequence this will also enable the corresponding
parent plls and pll-divs.

This is intended as a stop-gap until CLK_ENABLE_HAND_OFF
becomes available, at which point it should be used instead.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/clk/bcm/clk-bcm2835.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 35f8de7..03b7f01 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1251,6 +1251,14 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 		init.flags |= CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE;
 	}
 
+	/* if the clock is running, then enable CRITICAL */
+	if (cprman_read(cprman, data->ctl_reg) & CM_ENABLE) {
+		dev_dbg(cprman->dev,
+			"found firmware enabled clock %s - enabling critical\n",
+			data->name);
+		init.flags |= CLK_IS_CRITICAL;
+	}
+
 	clock = devm_kzalloc(cprman->dev, sizeof(*clock), GFP_KERNEL);
 	if (!clock)
 		return NULL;
-- 
2.1.4

Re: [PATCH V4 1/2] clk: bcm2835: mark enabled clocks with CLK_IS_CRITICAL

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> The bcm2835 firmware enables several clocks and plls before
> booting the linux kernel.
>
> These plls should never get disabled as it may result in a
> stopped system clock and more.
>
> So during probing we check if the clock is enabled
> and if it is then mark that clock with CLK_IS_CRITICAL.
>
> As a consequence this will also enable the corresponding
> parent plls and pll-divs.
>
> This is intended as a stop-gap until CLK_ENABLE_HAND_OFF
> becomes available, at which point it should be used instead.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

I still think that we don't want this patch.  We should be able to
disable clocks that the firmware turned on, unless they really need to
stay on.  If you have troubles on the upstream DT, let's talk about
individual clocks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH V4 1/2] clk: bcm2835: mark enabled clocks with CLK_IS_CRITICAL

[Martin Sperl]

> On 02.05.2016, at 17:36, Eric Anholt <eric@anholt.net> wrote:
> 
> kernel@martin.sperl.org writes:
> 
>> From: Martin Sperl <kernel@martin.sperl.org>
>> 
>> The bcm2835 firmware enables several clocks and plls before
>> booting the linux kernel.
>> 
>> These plls should never get disabled as it may result in a
>> stopped system clock and more.
>> 
>> So during probing we check if the clock is enabled
>> and if it is then mark that clock with CLK_IS_CRITICAL.
>> 
>> As a consequence this will also enable the corresponding
>> parent plls and pll-divs.
>> 
>> This is intended as a stop-gap until CLK_ENABLE_HAND_OFF
>> becomes available, at which point it should be used instead.
>> 
>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> 
> I still think that we don't want this patch.  We should be able to
> disable clocks that the firmware turned on, unless they really need to
> stay on.  If you have troubles on the upstream DT, let's talk about
> individual clocks.

May I ask you what is your main concern about using
CLK_IS_CRITICAL as a stop-gap/in general?

This is just a stop-gap until we get CLK_ENABLE_HAND_OFF merged
and I really want to use HAND_OFF as soon as possible, that is why
I have split it into 2 patches.

We essentially just replace the CLK_IS_CRITICAL with
CLK_ENABLE_HAND_OFF (plus comments and debug messages).

Any custom temporary code for specific clocks (which you propose)
is much more complicated than this simple patch and does not handle
things better in any way.

Also the current situation of the machine crashing when releasing the
PCM clock when the parent is PLLC or PLLD is worse than having some
clocks/pll running unnecessarily.

Also this is an incentive to merge HAND_OFF - there is now an
immediate user that would make immediate use of it.

Re: [PATCH V4 1/2] clk: bcm2835: mark enabled clocks with CLK_IS_CRITICAL

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

Martin Sperl <kernel@martin.sperl.org> writes:

>> On 02.05.2016, at 17:36, Eric Anholt <eric@anholt.net> wrote:
>> 
>> kernel@martin.sperl.org writes:
>> 
>>> From: Martin Sperl <kernel@martin.sperl.org>
>>> 
>>> The bcm2835 firmware enables several clocks and plls before
>>> booting the linux kernel.
>>> 
>>> These plls should never get disabled as it may result in a
>>> stopped system clock and more.
>>> 
>>> So during probing we check if the clock is enabled
>>> and if it is then mark that clock with CLK_IS_CRITICAL.
>>> 
>>> As a consequence this will also enable the corresponding
>>> parent plls and pll-divs.
>>> 
>>> This is intended as a stop-gap until CLK_ENABLE_HAND_OFF
>>> becomes available, at which point it should be used instead.
>>> 
>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>> 
>> I still think that we don't want this patch.  We should be able to
>> disable clocks that the firmware turned on, unless they really need to
>> stay on.  If you have troubles on the upstream DT, let's talk about
>> individual clocks.
>
> May I ask you what is your main concern about using
> CLK_IS_CRITICAL as a stop-gap/in general?

Burning power when you shouldn't, which is a bug.

> Also the current situation of the machine crashing when releasing the
> PCM clock when the parent is PLLC or PLLD is worse than having some
> clocks/pll running unnecessarily.

Are you saying this happens on the upstream kernel?  This sounds like a
bug you'd see in the downstream kernel because they haven't hooked up
the clocks in their DT.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH V4 1/2] clk: bcm2835: mark enabled clocks with CLK_IS_CRITICAL

[Martin Sperl]

> On 03.05.2016, at 03:13, Eric Anholt <eric@anholt.net> wrote:
> 
> Martin Sperl <kernel@martin.sperl.org> writes:
> 
>>> On 02.05.2016, at 17:36, Eric Anholt <eric@anholt.net> wrote:
>>> 
>>> kernel@martin.sperl.org writes:
>>> 
>>>> From: Martin Sperl <kernel@martin.sperl.org>
>>>> 
>>>> The bcm2835 firmware enables several clocks and plls before
>>>> booting the linux kernel.
>>>> 
>>>> These plls should never get disabled as it may result in a
>>>> stopped system clock and more.
>>>> 
>>>> So during probing we check if the clock is enabled
>>>> and if it is then mark that clock with CLK_IS_CRITICAL.
>>>> 
>>>> As a consequence this will also enable the corresponding
>>>> parent plls and pll-divs.
>>>> 
>>>> This is intended as a stop-gap until CLK_ENABLE_HAND_OFF
>>>> becomes available, at which point it should be used instead.
>>>> 
>>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>>> 
>>> I still think that we don't want this patch.  We should be able to
>>> disable clocks that the firmware turned on, unless they really need to
>>> stay on.  If you have troubles on the upstream DT, let's talk about
>>> individual clocks.
>> 
>> May I ask you what is your main concern about using
>> CLK_IS_CRITICAL as a stop-gap/in general?
> 
> Burning power when you shouldn't, which is a bug.
It is a bug, but it is better than a crash that happens currently
because of a stopped clock.
Also there is a proposed way forward when hand-off
becomes available.

> 
>> Also the current situation of the machine crashing when releasing the
>> PCM clock when the parent is PLLC or PLLD is worse than having some
>> clocks/pll running unnecessarily.
> 
> Are you saying this happens on the upstream kernel?  This sounds like a
> bug you'd see in the downstream kernel because they haven't hooked up
> the clocks in their DT.
This happens for both upstream and downstream when using
cprman...

You remember the main-uart crashing discussion (when 
loaded as a module or not used as the kernel console?) it is
exactly what an unclaimed clock is triggering when the clock
Is prepared/enabled and then released.

I have also seen the same issue happen with PCM without
the patch - as soon as we request a frequency which will use
pllc/d-per as the parent, and we later release the clock the
system crashes -  but there only if we run the dt with minimal
clocks using cprman, as it is hidden as soon as you increase enable
count of the plls because of an enabled clock by a different
device that also consumes the same pll.

So this bug is mostly hidden behind the complete dts that
just consumes lots of parent clocks, but it still may occur 
when we select a frequency that will select the firmware 
enabled but not claimed plla_per - no idea what stopping
that pll would trigger in the system.

Hence we need this patch for the running clocks , but we
also need it for the pll_divs, so actually my patch is
incomplete!

Re: [PATCH V4 1/2] clk: bcm2835: mark enabled clocks with CLK_IS_CRITICAL

[Martin Sperl]

[-- Attachment #1: Type: text/plain, Size: 9328 bytes --]



On 03.05.2016 07:07, Martin Sperl wrote:
> 
>> On 03.05.2016, at 03:13, Eric Anholt <eric@anholt.net> <mailto:eric@anholt.net> wrote:
>> 
>> Martin Sperl <kernel@martin.sperl.org> <mailto:kernel@martin.sperl.org> writes:
>> 
>>>> On 02.05.2016, at 17:36, Eric Anholt <eric@anholt.net> <mailto:eric@anholt.net> wrote:
>>>> 
>>>> kernel@martin.sperl.org <mailto:kernel@martin.sperl.org> writes:
>>>> 
>>>>> From: Martin Sperl <kernel@martin.sperl.org> <mailto:kernel@martin.sperl.org>
>>>>> 
>>>>> The bcm2835 firmware enables several clocks and plls before
>>>>> booting the linux kernel.
>>>>> 
>>>>> These plls should never get disabled as it may result in a
>>>>> stopped system clock and more.
>>>>> 
>>>>> So during probing we check if the clock is enabled
>>>>> and if it is then mark that clock with CLK_IS_CRITICAL.
>>>>> 
>>>>> As a consequence this will also enable the corresponding
>>>>> parent plls and pll-divs.
>>>>> 
>>>>> This is intended as a stop-gap until CLK_ENABLE_HAND_OFF
>>>>> becomes available, at which point it should be used instead.
>>>>> 
>>>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> <mailto:kernel@martin.sperl.org>
>>>> I still think that we don't want this patch.  We should be able to
>>>> disable clocks that the firmware turned on, unless they really need to
>>>> stay on.  If you have troubles on the upstream DT, let's talk about
>>>> individual clocks.
>>> May I ask you what is your main concern about using
>>> CLK_IS_CRITICAL as a stop-gap/in general?
>> Burning power when you shouldn't, which is a bug.
> It is a bug, but it is better than a crash that happens currently
> because of a stopped clock.
> Also there is a proposed way forward when hand-off
> becomes available.
> 
>>> Also the current situation of the machine crashing when releasing the
>>> PCM clock when the parent is PLLC or PLLD is worse than having some
>>> clocks/pll running unnecessarily.
>> Are you saying this happens on the upstream kernel?  This sounds like a
>> bug you'd see in the downstream kernel because they haven't hooked up
>> the clocks in their DT.
> This happens for both upstream and downstream when using
> cprman...
> 
> You remember the main-uart crashing discussion (when 
> loaded as a module or not used as the kernel console?) it is
> exactly what an unclaimed clock is triggering when the clock
> Is prepared/enabled and then released.
> 
> I have also seen the same issue happen with PCM without
> the patch - as soon as we request a frequency which will use
> pllc/d-per as the parent, and we later release the clock the
> system crashes -  but there only if we run the dt with minimal
> clocks using cprman, as it is hidden as soon as you increase enable
> count of the plls because of an enabled clock by a different
> device that also consumes the same pll.
> 
> So this bug is mostly hidden behind the complete dts that
> just consumes lots of parent clocks, but it still may occur 
> when we select a frequency that will select the firmware 
> enabled but not claimed plla_per - no idea what stopping
> that pll would trigger in the system.
> 
> Hence we need this patch for the running clocks , but we
> also need it for the pll_divs, so actually my patch is
> incomplete!

To put in some perspective:

So sing 4.6.0-rc1 with pwm disabled in the dt and amba_pl011 as a module
(modprobe blacklisted) I get the following enabled clocks:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1

modprobe amba_pl011
gives these clocks:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:1

rmmod amba_pl011
results in a crash.

Similarly using my private dtb that includes i2s + drivers,
loading the i2s audio modules gives still the default enabled clocks:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1

Playing a sound with 48000Hz sample-rate useing speaker-test and 
looking at the enabled clocks gives:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:2
/sys/kernel/debug/clk/pcm/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1

No issue when stopping - clocks then look like this:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1

So now playing a sound at 44100Hz sample rate gives no sound
the clocks look like this:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:2
/sys/kernel/debug/clk/pcm/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/plld/clk_enable_count:1
/sys/kernel/debug/clk/plld_per/clk_enable_count:1

and when aborting the playback the machine crashes…

So it is fairly simple to see that this happens with an upstream
kernel using 2 distinct cases.

Using the “IS_CRITICAL” patch instead avoids crashing the hw in
the above circumstances - clocks after boot look like this:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/emmc/clk_enable_count:2
/sys/kernel/debug/clk/h264/clk_enable_count:1
/sys/kernel/debug/clk/hsm/clk_enable_count:1
/sys/kernel/debug/clk/isp/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:6
/sys/kernel/debug/clk/otp/clk_enable_count:1
/sys/kernel/debug/clk/plla/clk_enable_count:1
/sys/kernel/debug/clk/plla_core/clk_enable_count:3
/sys/kernel/debug/clk/pllc/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/plld/clk_enable_count:1
/sys/kernel/debug/clk/plld_per/clk_enable_count:2
/sys/kernel/debug/clk/timer/clk_enable_count:1
/sys/kernel/debug/clk/tsens/clk_enable_count:1
/sys/kernel/debug/clk/uart/clk_enable_count:1
/sys/kernel/debug/clk/v3d/clk_enable_count:1

There is one drawback with IS_CRITICAL is visible in the
above: the emmc clock count goes up to 2.

When playing with speaker-test the “crash” no longer happens,
but there is still that issue with rmmod amba_pl011 still persists
When loading/unloading while playing pcm still results in a crash
but this time one can hear noise on the audio lines….

So it is not perfect, but at least audio playback does not fail.

Now finally with ENABLE_HAND_OFF:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/h264/clk_enable_count:1
/sys/kernel/debug/clk/hsm/clk_enable_count:1
/sys/kernel/debug/clk/isp/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:6
/sys/kernel/debug/clk/otp/clk_enable_count:1
/sys/kernel/debug/clk/plla/clk_enable_count:1
/sys/kernel/debug/clk/plla_core/clk_enable_count:3
/sys/kernel/debug/clk/pllc/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/plld/clk_enable_count:1
/sys/kernel/debug/clk/plld_per/clk_enable_count:2
/sys/kernel/debug/clk/timer/clk_enable_count:1
/sys/kernel/debug/clk/tsens/clk_enable_count:1
/sys/kernel/debug/clk/uart/clk_enable_count:1
/sys/kernel/debug/clk/v3d/clk_enable_count:1

With the following observations:
* audio again works
* amba_pl011 remove still fails…

So hand-off for clocks still does not work for everything.

Especially we still have cases where enabling and then disabling
a clock still can crash the system, but at least the emmc count
stays at 1.

Finally: when marking the running pll_div with HAND_OFF or CITICAL
in addition to the running clocks, then "rmmod amba_pl011" does no
longer crash the system - instead it triggers the dmesg: 
Trying to free nonexistent resource <0000000020201000-0000000020201fff>

But I guess this is a different issue.

So as a way forward: should we enable pll_div also to CRITICAL
and then HAND_OFF?

Or should we just use HAND_OFF - when can we get HAND_OFF
merged so that we can really make use of it without having to have a
CRITICAL step in between?

Assuming that we get ENABLE_HAND_OFF  into clk-next, then I
could create a new patch that:
* only uses HAND_OFF 
   (without the intermediate step of using CRITCAl)
* marks running pll_divs and clocks as HAND_OFF.

Thanks,
		Martin




[-- Attachment #2: Type: text/html, Size: 14077 bytes --]

[PATCH V4 2/2] clk: bcm2835: use CLK_ENABLE_HAND_OFF instead of CLK_IS_CRITICAL

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

The use of CLK_IS_CRITICAL is just a stop-gap to avoid unpreparing
essential PLLs that may result stopping the system without intention.

This moves to use CLK_ENABLE_HAND_OFF instead of CLK_IS_CRITICAL,
so that clocks (and their parents) may get released
if there is a driver that claims (and then later releases) a
specific clock.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

--
Note that this requires the following patches applied:
* clk: introduce CLK_ENABLE_HAND_OFF flag
* clk: per-user clk prepare & enable ref counts
---
 drivers/clk/bcm/clk-bcm2835.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 03b7f01..6303c3a 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1251,12 +1251,12 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 		init.flags |= CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE;
 	}
 
-	/* if the clock is running, then enable CRITICAL */
+	/* if the clock is running, then enable CLK_ENABLE_HAND_OFF */
 	if (cprman_read(cprman, data->ctl_reg) & CM_ENABLE) {
 		dev_dbg(cprman->dev,
-			"found firmware enabled clock %s - enabling critical\n",
+			"found firmware enabled clock %s - enabling hand off\n",
 			data->name);
-		init.flags |= CLK_IS_CRITICAL;
+		init.flags |= CLK_ENABLE_HAND_OFF;
 	}
 
 	clock = devm_kzalloc(cprman->dev, sizeof(*clock), GFP_KERNEL);
-- 
2.1.4