| Message ID | F7B22AC0-58A7-431D-B203-0C7A9802A040@martin.sperl.org |
|---|---|
| State | New |
| Headers | show |
Hi Martin & Stefan, Quoting Martin Sperl (2016-02-13 03:24:43) > > > On 13.02.2016, at 11:01, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > > Hi Martin, > > > >> Martin Sperl <kernel@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); You should not only prepare the clock, but enable it as well, even if your .enable callback is no-op. s/clk_prepare/clk_prepare_enable/ should do it. But instead of all of this stuff below... > + 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); ... how about using the shiny new HAND_OFF clocks feature? See: http://lkml.kernel.org/r/<1455225554-13267-1-git-send-email-mturquette@baylibre.com> It seems like you need clocks to remain enabled from the time they are registered until a clk consumer driver claims them and Does The Right Thing. (e.g. only gates them when it is safe to do so, such as display being turned off by user). Let me know if those patches will help you. The first three patches in the series introduce CRITICAL clocks, which never gate. Patches 4-6 are a solution that I prefer, allowing the clocks to remain enabled by the bootloader, and only gated later on when a consumer driver claims them. No changes to clock consumer drivers are required, the framework handles the transition of reference counts and the clock provider driver only has to set the CLK_ENABLE_HAND_OFF flag for the affected clocks. Regards, Mike > + > 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@raspcm:~# rmmod amba_pl011 > [ 142.318263] Trying to free nonexistent resource <0000000020201000-0000000020201fff> > but that is an issue not related to clock... > > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16.02.2016 19:57, Michael Turquette wrote: > Hi Martin & Stefan, ... > ... how about using the shiny new HAND_OFF clocks feature? See: > > http://lkml.kernel.org/r/<1455225554-13267-1-git-send-email-mturquette@baylibre.com> > > It seems like you need clocks to remain enabled from the time they are > registered until a clk consumer driver claims them and Does The Right > Thing. (e.g. only gates them when it is safe to do so, such as display > being turned off by user). > > Let me know if those patches will help you. The first three patches in > the series introduce CRITICAL clocks, which never gate. Patches 4-6 are > a solution that I prefer, allowing the clocks to remain enabled by the > bootloader, and only gated later on when a consumer driver claims them. > No changes to clock consumer drivers are required, the framework handles > the transition of reference counts and the clock provider driver only > has to set the CLK_ENABLE_HAND_OFF flag for the affected clocks. I wonder if those patches would really help in this use-case. HAND_OFF seems to show an issue around the fact that as soon as a clock is claimed once it is handled "normally". So when the amba-pl011 driver requests and prepares the uart clock the "lower" PLLD_per (divider) clock as well as the PLLD (pll) clock become "prepared" as they are parents. So fine so good - and that works right now fine. The problem is that the amba-pl011 driver when loaded as a module - for some reason - runs clk_unprepare/disable when the tty is closed. Using stty instead of getty for simplicity (as getty closes and reopens the tty - or so it seems): root@raspcm:~# stty -F /dev/ttyAMA0 [ 94.328906] pl011_startup - start [ 94.332401] pl011_hwinit - prepare-enable > HERE PLLD gets prepared/enabled [ 94.336499] bcm2835_pll_on - plld - start [ 94.340642] cprman_write - 0x010c = 0x0000020a [ 94.345170] bcm2835_pll_on - plld - before_wait_pll_lock [ 94.350637] bcm2835_pll_on - plld - end > HERE PLLD_PER gets prepared/enabled [ 94.354558] cprman_write - 0x1540 = 0x00000004 [ 94.359097] cprman_write - 0x010c = 0x0000020a > HERE UART gets prepares/enabled [ 94.363660] cprman_write - 0x00f0 = 0x000002d6 [ 94.368203] pl011_hwinit - prepare-enable - ret = 0 [ 94.375212] pl011_startup - exit OUTPUT BY STTY: speed 9600 baud; line = 0; OUTPUT BY STTY: -brkint -imaxbel [ 94.381528] pl011_shutdown - start [ 94.385722] pl011_shutdown - disable_unprepare > HERE UART gets disabled/unprepared [ 94.391024] cprman_write - 0x00f0 = 0x00000286 > here PLLD_PER gets disabled/unprepared [ 94.396037] cprman_write - 0x010c = 0x0000028a [ 94.400616] cprman_write - 0x1540 = 0x00000104 > HERE PLL gets disabled/unprepared [ 94.405143] bcm2835_pll_off - plld - start [ 94.409316] cprman_write - 0x010c = 0x0000038a [ 94.413906] cprman_write - 0x1140 = 0x00031034 [ 94.418435] bcm2835_pll_off - plld - end [ 94.422500] pl011_shutdown - exit root@raspcm:~# So with the HAND_OFF the PLLD (as it has been prepared) would follow thru the "release" code as expected, which results in stopping the PLLD_per clock and subsequently the PLLD clock. And this disabling of PLLD is what - as far as I can tell - brings the system to it knees, so this needs to be avoided And that is what the HAND_OFF flag would not stop, as the clock - at that time has been claimed - at least when using it directly on PLLD. Using CLK_IS_CRITICAL instead would solve that issue, as it just does the prepare/enable inside the framework once and never unprepares it. I guess it would also help to understand the clock-tree, but the information on the clock-tree is scarce (or under NDA), so it is hard to understand why the PLLD impacts the system in such a way. Possibly some of the CPU interfaces are sourced from PLLD (say PLLD_core) and that is why the CPU stops working. Things that come to mind are: * SRAM * AXI bus * ARM - core (even though that is supposed to be driven of PLLB) * ARM - L2 cache * GPU - run by firmware... And thus disabling the PLLD would impact those interfaces... (but I could be wrong) But - as said - documentation on the clock tree details is unfortunately lacking... A bit of digging in the source-code shows that the sdram clock has the clocks in bcm2835_clock_vpu_parents assigned as parents. The regdump for the sdram clock shows: root@raspcm:/build/linux# cat /sys/kernel/debug/clk/sdram/regdump ctl = 0x00004006 div = 0x00003000 So we use the 6th clock from the list of vpu clocks. (ctl & 0x0f) and that is the plld_core clock! This means that obviously disabling PLLD is NOT a good idea, as it will effectively disable sdram. Maybe we actually should be forcing the sdram clock to be always prepared/enabled manually or via CLK_IS_CRITICAL or CLK_HAND_OFF. Martin P.s: As far as I understand the reason why the amba_pl011 driver compiled in is working is because then the driver is used as the console, so it is always opened and thus the clocks are always prepared. Starting getty at a later stage during the boot will thus never risk getting the prepare_count of the PLLD to 0 - the kernel console will still hold the prepare reference and thus we are at 1.
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); }