serial: clk: bcm2835: Strange effects when using aux-uart in console

[Martin Sperl]

> On 13.02.2016, at 11:01, Stefan Wahren <stefan.wahren at i2se.com> wrote:
> 
> Hi Martin,
> 
>> Martin Sperl <kernel at martin.sperl.org> hat am 12. Februar 2016 um 20:44
>> geschrieben:
>> 
>> So the issue is triggered as soon as the plld_per
>> pll divider gets disabled/reenabled.
>> 
>> This happens because the clk_hw.core.prepare_count
>> drops down to 0 and then unprepare is called.
>> 
>> So we need to increase the ref-count for the pll
>> and pll_dividers to at least 1 so that these never
>> get disabled - at least for now until we can come
>> up with a better contract with the firmware.
>> 
>> Obviously this may impact other drivers as well
>> where a pll is used for the first time - if nothing
>> else uses it and the clock gets released, then
>> the clock would trigger a unprepare of the whole
>> branch of the clock tree.
>> 
>> The question is: how can we solve it in an acceptable
>> manner?
>> 
>> Do we need a driver that just holds a reference to
>> those clocks? Or should we just prepare the clock
>> after registering it in clk-bcm2835.c?
>> 
>> As for why does this not show up when compiled in?
>> It seems that in that case the amba_pl011 driver
>> never gets removed and then probed again.
>> 
>> This is possibly related to the optional use of DMA,
>> with the amba-pl011 driver that retries the install,
>> which is not supported on the bcm2835 - at least that
>> is what the datasheet says. And DMA is (probably) not
>> enabled during the early boot stages, so it does not
>> fail once when it tries to register DMA.
>> 
>> Thanks,
>> Martin
>> 
> 
> i think i must correct myself. The fixed apb should stay since there is no
> dynamic replacement and a proper disabling of plld shouldn't cause this freeze.
> 
> I have the suspicion the freeze is caused by a clock glitch or lock-up.
> 
> Could you please revert your changes and apply the attached patch? Maybe we can
> see more.

I will give it a try.

But to me it seems as if the disabling of PLLD produces a hickup in the 
firmware (which relies on PLLD-DIV and thus PLL).

This would also explain the 3-4 second bar/blanking pattern seen on HDMI,
which I would guess is related to a reset loop...

I got a simple fix for now, that just prepares a few clocks/pll/pll_divs
during probing, so that they will never get unprepared/fully stopped):

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 015e687..fe0c401 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -37,6 +37,7 @@
  * generator).
  */

+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
 #include <linux/clk/bcm2835.h>
@@ -1504,6 +1505,7 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
        struct clk **clks;
        struct bcm2835_cprman *cprman;
        struct resource *res;
+       int ret;

        cprman = devm_kzalloc(dev, sizeof(*cprman), GFP_KERNEL);
        if (!cprman)
@@ -1597,6 +1599,37 @@ static int bcm2835_clk_probe(struct platform_device *pdev
        clks[BCM2835_CLOCK_PWM] =
                bcm2835_register_clock(cprman, &bcm2835_clock_pwm_data);

+       /*
+        * Several PLLs, PLL-dividers and clocks need to get prepared
+        * so that they are never unprepared and released.
+        * We run this separately as there may be dependencies that would
+        * need to be fullfilled first...
+        * Note: Especially unpreparing PLLD_PER will kill the system
+        *       (even if it is prepared again almost immediately
+        *        the HDMI display will fail)
+        */
+#define CLK_PREPARE_INITIAL(clkid)                                     \
+       if (clks[clkid] && (ret = clk_prepare(clks[clkid])))            \
+               dev_err(dev, "could not prepare clock %pC - %d\n",      \
+                       clks[clkid], ret);
+       CLK_PREPARE_INITIAL(BCM2835_PLLA);
+       CLK_PREPARE_INITIAL(BCM2835_PLLB);
+       CLK_PREPARE_INITIAL(BCM2835_PLLC);
+       CLK_PREPARE_INITIAL(BCM2835_PLLD);
+       CLK_PREPARE_INITIAL(BCM2835_PLLH);
+       CLK_PREPARE_INITIAL(BCM2835_PLLA_CORE);
+       CLK_PREPARE_INITIAL(BCM2835_PLLA_PER);
+       CLK_PREPARE_INITIAL(BCM2835_PLLC_CORE0);
+       CLK_PREPARE_INITIAL(BCM2835_PLLC_CORE1);
+       CLK_PREPARE_INITIAL(BCM2835_PLLC_CORE2);
+       CLK_PREPARE_INITIAL(BCM2835_PLLC_PER);
+       CLK_PREPARE_INITIAL(BCM2835_PLLD_CORE);
+       CLK_PREPARE_INITIAL(BCM2835_PLLD_PER);
+       CLK_PREPARE_INITIAL(BCM2835_PLLH_RCAL);
+       CLK_PREPARE_INITIAL(BCM2835_PLLH_AUX);
+       CLK_PREPARE_INITIAL(BCM2835_PLLH_PIX);
+       CLK_PREPARE_INITIAL(BCM2835_CLOCK_VPU);
+
        return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
                                   &cprman->onecell);
 }

And that resolves the issue and also allows loading/unloading of the
amba-pl011 module as well without any hickups.

I guess it would be better if we did make this configurable in the DT.
(See patch 1 of my RFC patchset, which could get extended to handle this
as well - at least a portion thereof).

But in more general terms: we need to come up with some agreement
on ownership of the clocks between linux and the firmware and how
to change them without impacting the other. 

One example is the “overclocking” that currently happens for
downstream kernels in the firmware, which impacts some of the
plls and PLLdivs as well as the sram control registers.

The reason for this seems to be that for overclocking the SRAM
parameters also need to get changed and that can only get done
reliably when running the code from L1/L2 caches - and that
requirement of not touching SRAM while changing the SRAM parameters
is probably hard to achieve cleanly on ARM alone.

Ciao,
	Martin

P.s: unloading amba_pl011 gives the following error:
root at raspcm:~# rmmod amba_pl011
[  142.318263] Trying to free nonexistent resource <0000000020201000-0000000020201fff>
but that is an issue not related to clock...