[PATCH V2] clk: bcm2835: mark enabled pll_dividers as critical

[PATCH V2] clk: bcm2835: mark enabled pll_dividers as critical

[kernel at martin.sperl.org]

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 pll_divider is enabled
and if it is then mark that pll_divider as critical.
As a consequence this will also enable the corresponding parent pll.

Here the list of pll_div that are marked as critical:
[    2.022437] bcm2835-clk 20101000.cprman: found enabled pll_div plla_core - marking it as critical
[    2.031640] bcm2835-clk 20101000.cprman: found enabled pll_div pllb_arm - marking it as critical
[    2.040966] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_core0 - marking it as critical
[    2.050351] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_per - marking it as critical
[    2.059427] bcm2835-clk 20101000.cprman: found enabled pll_div plld_core - marking it as critical
[    2.068590] bcm2835-clk 20101000.cprman: found enabled pll_div plld_per - marking it as critical
[    2.077724] bcm2835-clk 20101000.cprman: found enabled pll_div pllh_pix - marking it as critical

Changelog:
  V1 -> V2: apply to pll_dividers instead of clocks
            use CLK_IS_CRITICAL flag instead of prepare/enable manually

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

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 6479283c..7f6838f 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1086,6 +1086,15 @@ bcm2835_register_pll_divider(struct bcm2835_cprman *cprman,
 	divider->cprman = cprman;
 	divider->data = data;
 
+	/* if the pll-divider is running, then mark is as critical */
+	if ((cprman_read(cprman, data->a2w_reg) &
+	     A2W_PLL_CHANNEL_DISABLE) == 0) {
+		dev_info(cprman->dev,
+			 "found enabled pll_div %s - marking it as critical\n",
+			 data->name);
+		init.flags |= CLK_IS_CRITICAL;
+	}
+
 	clk = devm_clk_register(cprman->dev, &divider->div.hw);
 	if (IS_ERR(clk))
 		return clk;
-- 
2.1.4

[PATCH V2] clk: bcm2835: mark enabled pll_dividers as critical

[Eric Anholt]

kernel at 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 pll_divider is enabled
> and if it is then mark that pll_divider as critical.
> As a consequence this will also enable the corresponding parent pll.
>
> Here the list of pll_div that are marked as critical:
> [    2.022437] bcm2835-clk 20101000.cprman: found enabled pll_div plla_core - marking it as critical
> [    2.031640] bcm2835-clk 20101000.cprman: found enabled pll_div pllb_arm - marking it as critical
> [    2.040966] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_core0 - marking it as critical
> [    2.050351] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_per - marking it as critical
> [    2.059427] bcm2835-clk 20101000.cprman: found enabled pll_div plld_core - marking it as critical
> [    2.068590] bcm2835-clk 20101000.cprman: found enabled pll_div plld_per - marking it as critical
> [    2.077724] bcm2835-clk 20101000.cprman: found enabled pll_div pllh_pix - marking it as critical

Yeah, pllh_pix isn't critical, though.  We want it to get turned off
when the driver asks to disable its clock, or we're going to just waste
a pile of power.

I'm sending out a patch that marks the VPU clock as critical (it's the
also AXI bus, so it certainly is critical), which should solve your
aux_uart clock disabling problem, I think.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160426/cd74e134/attachment-0001.sig>

[PATCH V2] clk: bcm2835: mark enabled pll_dividers as critical

[Martin Sperl]

Sent from my iPad
> On 26.04.2016, at 21:31, Eric Anholt <eric@anholt.net> wrote:
> 
> kernel at 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 pll_divider is enabled
>> and if it is then mark that pll_divider as critical.
>> As a consequence this will also enable the corresponding parent pll.
>> 
>> Here the list of pll_div that are marked as critical:
>> [    2.022437] bcm2835-clk 20101000.cprman: found enabled pll_div plla_core - marking it as critical
>> [    2.031640] bcm2835-clk 20101000.cprman: found enabled pll_div pllb_arm - marking it as critical
>> [    2.040966] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_core0 - marking it as critical
>> [    2.050351] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_per - marking it as critical
>> [    2.059427] bcm2835-clk 20101000.cprman: found enabled pll_div plld_core - marking it as critical
>> [    2.068590] bcm2835-clk 20101000.cprman: found enabled pll_div plld_per - marking it as critical
>> [    2.077724] bcm2835-clk 20101000.cprman: found enabled pll_div pllh_pix - marking it as critical
> 
> Yeah, pllh_pix isn't critical, though.  We want it to get turned off
> when the driver asks to disable its clock, or we're going to just waste
> a pile of power.
> 
> I'm sending out a patch that marks the VPU clock as critical (it's the
> also AXI bus, so it certainly is critical), which should solve your
> aux_uart clock disabling problem, I think.

The problem is that it also fails on the pcm clock alone when pllc or 
plld_per are used as parent, but it is fine when osc is used...

This started to show during the steps towards migration of 
downstream to use the new driver -PCM was the only clock that was 
referred in the dt to use the new clock driver while all others were 
using fixed clocks.

So as far as I can see this is a bigger problem and making all 
running pll_div Critical makes it work fine.

We potentially could mask pllh as non-critical, but even then
the parent selector could choose pllh-per and then release it
and then the hdmi would stop working... E.g if we would request a
PCM clock rate of  290039- then Pllh_aux would be ideal with 
a divider of 2 and on releasing it it would stop the pllh (assuming
no KMS)

There was this other approach proposed by Michael some time ago
that might be the better solution, but then I guess it had its own
drawbacks, which may not make it applicable in our situation.

Critical seems the right choice, but we would need a means
that would allow us to remove the critical flag when first requesting
the clock for some clocks - maybe via dt - so that when KMS is
enabled the critical flag for pllh_aux is removed.

As an alternative: maybe set the flag to use in case the pll-div
is enabled in the pll_div_data structure. That way we can
make some decisions on a per pll-div basis...

Martin

[PATCH V2] clk: bcm2835: mark enabled pll_dividers as critical

[Eric Anholt]

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

> Sent from my iPad
>> On 26.04.2016, at 21:31, Eric Anholt <eric@anholt.net> wrote:
>> 
>> kernel at 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 pll_divider is enabled
>>> and if it is then mark that pll_divider as critical.
>>> As a consequence this will also enable the corresponding parent pll.
>>> 
>>> Here the list of pll_div that are marked as critical:
>>> [    2.022437] bcm2835-clk 20101000.cprman: found enabled pll_div plla_core - marking it as critical
>>> [    2.031640] bcm2835-clk 20101000.cprman: found enabled pll_div pllb_arm - marking it as critical
>>> [    2.040966] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_core0 - marking it as critical
>>> [    2.050351] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_per - marking it as critical
>>> [    2.059427] bcm2835-clk 20101000.cprman: found enabled pll_div plld_core - marking it as critical
>>> [    2.068590] bcm2835-clk 20101000.cprman: found enabled pll_div plld_per - marking it as critical
>>> [    2.077724] bcm2835-clk 20101000.cprman: found enabled pll_div pllh_pix - marking it as critical
>> 
>> Yeah, pllh_pix isn't critical, though.  We want it to get turned off
>> when the driver asks to disable its clock, or we're going to just waste
>> a pile of power.
>> 
>> I'm sending out a patch that marks the VPU clock as critical (it's the
>> also AXI bus, so it certainly is critical), which should solve your
>> aux_uart clock disabling problem, I think.
>
> The problem is that it also fails on the pcm clock alone when pllc or 
> plld_per are used as parent, but it is fine when osc is used... 

For that you're going to want the HAND_OFF patches that mturquette is
working on: Don't let the clock and its parents get turned off until a
driver has shown up that has referenced the clock and done at least a
prepare on it once.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160426/f949ce9c/attachment.sig>

[PATCH V2] clk: bcm2835: mark enabled pll_dividers as critical

[Martin Sperl]

> On 27.04.2016, at 03:30, Eric Anholt <eric@anholt.net> wrote:
> 
> Martin Sperl <kernel@martin.sperl.org> writes:
> 
>> Sent from my iPad
>>> On 26.04.2016, at 21:31, Eric Anholt <eric@anholt.net> wrote:
>>> 
>>> kernel at 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 pll_divider is enabled
>>>> and if it is then mark that pll_divider as critical.
>>>> As a consequence this will also enable the corresponding parent pll.
>>>> 
>>>> Here the list of pll_div that are marked as critical:
>>>> [    2.022437] bcm2835-clk 20101000.cprman: found enabled pll_div plla_core - marking it as critical
>>>> [    2.031640] bcm2835-clk 20101000.cprman: found enabled pll_div pllb_arm - marking it as critical
>>>> [    2.040966] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_core0 - marking it as critical
>>>> [    2.050351] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_per - marking it as critical
>>>> [    2.059427] bcm2835-clk 20101000.cprman: found enabled pll_div plld_core - marking it as critical
>>>> [    2.068590] bcm2835-clk 20101000.cprman: found enabled pll_div plld_per - marking it as critical
>>>> [    2.077724] bcm2835-clk 20101000.cprman: found enabled pll_div pllh_pix - marking it as critical
>>> 
>>> Yeah, pllh_pix isn't critical, though.  We want it to get turned off
>>> when the driver asks to disable its clock, or we're going to just waste
>>> a pile of power.
>>> 
>>> I'm sending out a patch that marks the VPU clock as critical (it's the
>>> also AXI bus, so it certainly is critical), which should solve your
>>> aux_uart clock disabling problem, I think.
>> 
>> The problem is that it also fails on the pcm clock alone when pllc or 
>> plld_per are used as parent, but it is fine when osc is used...
> 
> For that you're going to want the HAND_OFF patches that mturquette is
> working on: Don't let the clock and its parents get turned off until a
> driver has shown up that has referenced the clock and done at least a
> prepare on it once.
That one was the one I thought of as well, but it does not solve the
issue at all, because:

Speaker-test opens the audio device for 32bit 41200 samples
Which in turn uses the i2s driver
Which enable_prepares the clock (uses pllc_per or plld_per)
Plays sound until the end
Then speaker test closes the audio device
Which disables/unprepared the PCM clock
Which disables/unprepares plld_per
Which disables/unprepares plld
Machine crashes

For those last few steps the handsoff still would release the
Pll-div and pll as it has been claimed correctly first.

Maybe handsoff would work if applied to the individual
Clocks (not the pll or pllc), but I am not sure if this is a correct
assumption, as I do not know if handsoff would propagate 
up the clock tree.

So the critical flag seems the "best" approach for now,
But as you have said: it is not ideal as it never disables
Any clocks when they are not really needed.

But so that we can continue we need something 
that works now (even if it is not perfect)

Another approach would be knowing the list of clocks 
that the firmware claims/owns and mark only those as "critical".

[PATCH V2] clk: bcm2835: mark enabled pll_dividers as critical

[Martin Sperl]

> On 27.04.2016, at 07:31, Martin Sperl <kernel@martin.sperl.org> wrote:
> 
> 
> 
>> On 27.04.2016, at 03:30, Eric Anholt <eric@anholt.net> wrote:
>> 
>> Martin Sperl <kernel@martin.sperl.org> writes:
>> 
>>> Sent from my iPad
>>>> On 26.04.2016, at 21:31, Eric Anholt <eric@anholt.net> wrote:
>>>> 
>>>> kernel at 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 pll_divider is enabled
>>>>> and if it is then mark that pll_divider as critical.
>>>>> As a consequence this will also enable the corresponding parent pll.
>>>>> 
>>>>> Here the list of pll_div that are marked as critical:
>>>>> [    2.022437] bcm2835-clk 20101000.cprman: found enabled pll_div plla_core - marking it as critical
>>>>> [    2.031640] bcm2835-clk 20101000.cprman: found enabled pll_div pllb_arm - marking it as critical
>>>>> [    2.040966] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_core0 - marking it as critical
>>>>> [    2.050351] bcm2835-clk 20101000.cprman: found enabled pll_div pllc_per - marking it as critical
>>>>> [    2.059427] bcm2835-clk 20101000.cprman: found enabled pll_div plld_core - marking it as critical
>>>>> [    2.068590] bcm2835-clk 20101000.cprman: found enabled pll_div plld_per - marking it as critical
>>>>> [    2.077724] bcm2835-clk 20101000.cprman: found enabled pll_div pllh_pix - marking it as critical
>>>> 
>>>> Yeah, pllh_pix isn't critical, though.  We want it to get turned off
>>>> when the driver asks to disable its clock, or we're going to just waste
>>>> a pile of power.
>>>> 
>>>> I'm sending out a patch that marks the VPU clock as critical (it's the
>>>> also AXI bus, so it certainly is critical), which should solve your
>>>> aux_uart clock disabling problem, I think.
>>> 
>>> The problem is that it also fails on the pcm clock alone when pllc or 
>>> plld_per are used as parent, but it is fine when osc is used...
>> 
>> For that you're going to want the HAND_OFF patches that mturquette is
>> working on: Don't let the clock and its parents get turned off until a
>> driver has shown up that has referenced the clock and done at least a
>> prepare on it once.
> That one was the one I thought of as well, but it does not solve the
> issue at all, because:
> 
> Speaker-test opens the audio device for 32bit 41200 samples
> Which in turn uses the i2s driver
> Which enable_prepares the clock (uses pllc_per or plld_per)
> Plays sound until the end
> Then speaker test closes the audio device
> Which disables/unprepared the PCM clock
> Which disables/unprepares plld_per
> Which disables/unprepares plld
> Machine crashes
> 
> For those last few steps the handsoff still would release the
> Pll-div and pll as it has been claimed correctly first.
> 
> Maybe handsoff would work if applied to the individual
> Clocks (not the pll or pllc), but I am not sure if this is a correct
> assumption, as I do not know if handsoff would propagate 
> up the clock tree.
> 
> So the critical flag seems the "best" approach for now,
> But as you have said: it is not ideal as it never disables
> Any clocks when they are not really needed.
> 
> But so that we can continue we need something 
> that works now (even if it is not perfect)
> 
> Another approach would be knowing the list of clocks 
> that the firmware claims/owns and mark only those as "critical?.
I did a test with HAND_OFF and that works very well indeed!

Here the corresponding patch:
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 35f8de7..31417fd 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 mark is as critical */
+       if (cprman_read(cprman, data->ctl_reg) & CM_ENABLE) {
+               dev_info(cprman->dev,
+                        "found enabled clock %s - enabling hand off\n",
+                        data->name);
+               init.flags |= CLK_ENABLE_HAND_OFF;
+       }
+
        clock = devm_kzalloc(cprman->dev, sizeof(*clock), GFP_KERNEL);
        if (!clock)
                return NULL;

Note that I am not sure if that is also needed for the pll
and pll-div - there may be some explicit consumers of the pll_div

At least that latest issue we have seen with the pcm clock has gone away.

Registered clocks with hand-off:
bcm2835-clk 20101000.cprman: found enabled clock timer - enabling hand off
bcm2835-clk 20101000.cprman: found enabled clock otp - enabling hand off
bcm2835-clk 20101000.cprman: found enabled clock uart - enabling hand off
bcm2835-clk 20101000.cprman: found enabled clock v3d - enabling hand off
bcm2835-clk 20101000.cprman: found enabled clock isp - enabling hand off
bcm2835-clk 20101000.cprman: found enabled clock h264 - enabling hand off
bcm2835-clk 20101000.cprman: found enabled clock vec - enabling hand off
bcm2835-clk 20101000.cprman: found enabled clock tsens - enabling hand off
bcm2835-clk 20101000.cprman: found enabled clock emmc - enabling hand off

Enabled clock count:
root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/apb_pclk/clk_enable_count:1
/sys/kernel/debug/clk/clock/clk_enable_count:1
/sys/kernel/debug/clk/core/clk_enable_count:3
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/h264/clk_enable_count:1
/sys/kernel/debug/clk/isp/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:7
/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:1
/sys/kernel/debug/clk/pllh_aux/clk_enable_count:1
/sys/kernel/debug/clk/pllh_aux_prediv/clk_enable_count:1
/sys/kernel/debug/clk/pllh/clk_enable_count:1
/sys/kernel/debug/clk/timer/clk_enable_count:1
/sys/kernel/debug/clk/tsens/clk_enable_count:1
/sys/kernel/debug/clk/uart0_pclk/clk_enable_count:1
/sys/kernel/debug/clk/uart/clk_enable_count:1
/sys/kernel/debug/clk/v3d/clk_enable_count:1
/sys/kernel/debug/clk/vec/clk_enable_count:1

That is a setup where the clock bcm2835 is only referenced in the device 
tree for pcm.

Using pcm-audio would trigger a machine halt before this patch.
Now this does not happen.

Hope that this is the best solution and an incentive to get those
CLK_ENABLE_HAND_OFF patches in as there is now the first user.

I will post this as a patch.

Martin