| Message ID | 1456745963-2403-4-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> > > If a clock that has been enabled by the firmware gets disabled > by a driver this may right now result in a crash of the system > as then also the corresponding PLL_dividers as well as PLLs > get disabled (if not used) - some of which are used by the > VideoCore GPU (which also runs the firmware) > > This patch prepares/enables those clocks that have been > configured by the firmware. > > Whenever the clock framework implements either > CLK_IS_CRITICAL or HAND_OFF this can get changed to use this > new mechanism. > > For this to be completely successful (i.e not missing a clock > and subsequently a pll) it is recommended to add all the known > clocks of the soc so that this can get applied to all clocks. I think this makes sense to have, for now at least. Reviewed-by: Eric Anholt <eric@anholt.net>
Eric Anholt <eric@anholt.net> writes: > kernel@martin.sperl.org writes: > >> From: Martin Sperl <kernel@martin.sperl.org> >> >> If a clock that has been enabled by the firmware gets disabled >> by a driver this may right now result in a crash of the system >> as then also the corresponding PLL_dividers as well as PLLs >> get disabled (if not used) - some of which are used by the >> VideoCore GPU (which also runs the firmware) >> >> This patch prepares/enables those clocks that have been >> configured by the firmware. >> >> Whenever the clock framework implements either >> CLK_IS_CRITICAL or HAND_OFF this can get changed to use this >> new mechanism. >> >> For this to be completely successful (i.e not missing a clock >> and subsequently a pll) it is recommended to add all the known >> clocks of the soc so that this can get applied to all clocks. > > I think this makes sense to have, for now at least. > > Reviewed-by: Eric Anholt <eric@anholt.net> Scratch that, this breaks display. Since the clkgen clocks are flagged as needing to be gated in order to change dividers, it means you can't set clock rates for anything that was turned on at boot.
> On 17.03.2016, at 18:39, Eric Anholt <eric@anholt.net> wrote: > > Eric Anholt <eric@anholt.net> writes: > >> kernel@martin.sperl.org writes: >> >>> From: Martin Sperl <kernel@martin.sperl.org> >>> >>> If a clock that has been enabled by the firmware gets disabled >>> by a driver this may right now result in a crash of the system >>> as then also the corresponding PLL_dividers as well as PLLs >>> get disabled (if not used) - some of which are used by the >>> VideoCore GPU (which also runs the firmware) >>> >>> This patch prepares/enables those clocks that have been >>> configured by the firmware. >>> >>> Whenever the clock framework implements either >>> CLK_IS_CRITICAL or HAND_OFF this can get changed to use this >>> new mechanism. >>> >>> For this to be completely successful (i.e not missing a clock >>> and subsequently a pll) it is recommended to add all the known >>> clocks of the soc so that this can get applied to all clocks. >> >> I think this makes sense to have, for now at least. >> >> Reviewed-by: Eric Anholt <eric@anholt.net> > > Scratch that, this breaks display. Since the clkgen clocks are flagged > as needing to be gated in order to change dividers, it means you can't > set clock rates for anything that was turned on at boot. See that separate thread that triggered this: serial: clk: bcm2835: Strange effects when using aux-uart in console and this patch fixes this issue. To summarize the situation figured in the thread: * you load the module * you start using the tty (say by using "stty -F /dev/ttyAMA0") * this opens the device * this prepare the relevant clock (usage = 1) * this prepares the parent pll-divider (usage = 1) * this prepares the parent pll (usage = 1) * you stop using the tty (stty closes the device) * this release the clock * usage count drops to 0, so disable the clock * this releases the parent pll-divider * usage count drops to 0, so disable the pll-div * this releases the parent pll (and disables it as usage = 0) * usage count drops to 0, so disable the pll-div * system crashes (with a bit of delay) The prepare should just increase the usage so it never gets to a count of 0. Maybe we need to use those "CLK_IS_CRITICAL” “HANDS_OFF” flags instead? (when/if they become available) How do you want to solve that - I have not got a DSI display, but HDMI continues to work... Martin
Martin Sperl <kernel@martin.sperl.org> writes: >> On 17.03.2016, at 18:39, Eric Anholt <eric@anholt.net> wrote: >> >> Eric Anholt <eric@anholt.net> writes: >> >>> kernel@martin.sperl.org writes: >>> >>>> From: Martin Sperl <kernel@martin.sperl.org> >>>> >>>> If a clock that has been enabled by the firmware gets disabled >>>> by a driver this may right now result in a crash of the system >>>> as then also the corresponding PLL_dividers as well as PLLs >>>> get disabled (if not used) - some of which are used by the >>>> VideoCore GPU (which also runs the firmware) >>>> >>>> This patch prepares/enables those clocks that have been >>>> configured by the firmware. >>>> >>>> Whenever the clock framework implements either >>>> CLK_IS_CRITICAL or HAND_OFF this can get changed to use this >>>> new mechanism. >>>> >>>> For this to be completely successful (i.e not missing a clock >>>> and subsequently a pll) it is recommended to add all the known >>>> clocks of the soc so that this can get applied to all clocks. >>> >>> I think this makes sense to have, for now at least. >>> >>> Reviewed-by: Eric Anholt <eric@anholt.net> >> >> Scratch that, this breaks display. Since the clkgen clocks are flagged >> as needing to be gated in order to change dividers, it means you can't >> set clock rates for anything that was turned on at boot. > > See that separate thread that triggered this: > serial: clk: bcm2835: Strange effects when using aux-uart in console > and this patch fixes this issue. > > To summarize the situation figured in the thread: > * you load the module > > * you start using the tty (say by using "stty -F /dev/ttyAMA0") > * this opens the device > * this prepare the relevant clock (usage = 1) > * this prepares the parent pll-divider (usage = 1) > * this prepares the parent pll (usage = 1) > > * you stop using the tty (stty closes the device) > * this release the clock > * usage count drops to 0, so disable the clock > * this releases the parent pll-divider > * usage count drops to 0, so disable the pll-div > * this releases the parent pll (and disables it as usage = 0) > * usage count drops to 0, so disable the pll-div > > * system crashes (with a bit of delay) > > The prepare should just increase the usage so it never gets to a count of 0. > > Maybe we need to use those "CLK_IS_CRITICAL” “HANDS_OFF” flags instead? > (when/if they become available) > > How do you want to solve that - I have not got a DSI display, > but HDMI continues to work... We should just prepare the necessary divider, not the leaf clocks that we actually want to control at runtime.
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index 30d6486..1fbb55d 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> @@ -1478,6 +1479,7 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman, struct clk_init_data init; const char *parents[1 << CM_SRC_BITS]; size_t i; + struct clk *clk; /* * Replace our "xosc" references with the oscillator's @@ -1511,7 +1513,18 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman, clock->data = data; clock->hw.init = &init; - return devm_clk_register(cprman->dev, &clock->hw); + clk = devm_clk_register(cprman->dev, &clock->hw); + if (IS_ERR_OR_NULL(clk)) + return clk; + + /* enable/prepare if the clock is enabled by the firmware */ + if (cprman_read(cprman, data->ctl_reg) & CM_ENABLE) { + dev_info(cprman->dev, + "found firmware enabled clock %pC\n", clk); + clk_prepare_enable(clk); + } + + return clk; } static int bcm2835_clk_probe(struct platform_device *pdev)