| Message ID | 1461660989-11846-1-git-send-email-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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.
Sent from my iPad > On 26.04.2016, at 21:31, 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 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
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@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.
> 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@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".
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, ÷r->div.hw); if (IS_ERR(clk)) return clk;