[4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate

Commit Message

Boris Brezillon Feb. 8, 2018, 1:43 p.m. UTC

All bcm2835 PLLs should be gated before their rate can be changed.
Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
not enough to make the code work in all situations. Indeed, the
CLK_SET_RATE_GATE flag prevents a user from changing the rate while
the clock is enabled, but this check only guarantees there's no Linux
users. In our case, the clock might have been enabled by the
bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
disables the PLL. So we have to make sure the PLL is actually disabled
before changing the rate.

Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
Cc: <[e-mail]>
Signed-off-by: Boris Brezillon <[e-mail]>
---
 drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Eric Anholt Feb. 8, 2018, 3:20 p.m. UTC | #1

Boris Brezillon <[e-mail]> writes:

> All bcm2835 PLLs should be gated before their rate can be changed.
> Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
> not enough to make the code work in all situations. Indeed, the
> CLK_SET_RATE_GATE flag prevents a user from changing the rate while
> the clock is enabled, but this check only guarantees there's no Linux
> users. In our case, the clock might have been enabled by the
> bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
> disables the PLL. So we have to make sure the PLL is actually disabled
> before changing the rate.
>
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
> Cc: <[e-mail]>
> Signed-off-by: Boris Brezillon <[e-mail]>
> ---
>  drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 6c5d4a8e426c..051ce769c109 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -678,6 +678,18 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw,
>  	u32 ana[4];
>  	int i;
>  
> +	/*
> +	 * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
> +	 * the rate while the clock is enabled, but this check only makes sure
> +	 * there's no Linux users.
> +	 * In our case, the clock might have been enabled by the bootloader/FW,
> +	 * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
> +	 * So we have to make sure the clk is actually disabled before changing
> +	 * the rate.
> +	 */
> +	if (bcm2835_pll_is_on(hw))
> +		bcm2835_pll_off(hw);
> +

I'm not sure this improves the situation.  If the PLL was on, then
presumably there's a divider using it and a CM clock using that, so
we'll probably end up driving some glitches on them.

Does the common clk framework have a way to disable unused clocks from
the leaf clocks up to this root, before the general
disable-unused-clocks path happens late in the boot process?

Boris Brezillon Feb. 8, 2018, 5:56 p.m. UTC | #2

On Thu, 08 Feb 2018 15:20:16 +0000
Eric Anholt <[e-mail]> wrote:

> Boris Brezillon <[e-mail]> writes:
> 
> > All bcm2835 PLLs should be gated before their rate can be changed.
> > Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
> > not enough to make the code work in all situations. Indeed, the
> > CLK_SET_RATE_GATE flag prevents a user from changing the rate while
> > the clock is enabled, but this check only guarantees there's no Linux
> > users. In our case, the clock might have been enabled by the
> > bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
> > disables the PLL. So we have to make sure the PLL is actually disabled
> > before changing the rate.
> >
> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
> > Cc: <[e-mail]>
> > Signed-off-by: Boris Brezillon <[e-mail]>
> > ---
> >  drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> > index 6c5d4a8e426c..051ce769c109 100644
> > --- a/drivers/clk/bcm/clk-bcm2835.c
> > +++ b/drivers/clk/bcm/clk-bcm2835.c
> > @@ -678,6 +678,18 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw,
> >  	u32 ana[4];
> >  	int i;
> >  
> > +	/*
> > +	 * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
> > +	 * the rate while the clock is enabled, but this check only makes sure
> > +	 * there's no Linux users.
> > +	 * In our case, the clock might have been enabled by the bootloader/FW,
> > +	 * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
> > +	 * So we have to make sure the clk is actually disabled before changing
> > +	 * the rate.
> > +	 */
> > +	if (bcm2835_pll_is_on(hw))
> > +		bcm2835_pll_off(hw);
> > +  
> 
> I'm not sure this improves the situation.  If the PLL was on, then
> presumably there's a divider using it and a CM clock using that, so
> we'll probably end up driving some glitches on them.

Hm, yes, but if someone is trying to change the rate of the PLL, and the
core doesn't know other clks depend on this PLL (which is the case if
we reach this point), we're already in big trouble.

> 
> Does the common clk framework have a way to disable unused clocks from
> the leaf clocks up to this root, before the general
> disable-unused-clocks path happens late in the boot process?

Not that I know of. What do you have in mind?

Eric Anholt Feb. 9, 2018, 9:32 a.m. UTC | #3

Boris Brezillon <[e-mail]> writes:

> On Thu, 08 Feb 2018 15:20:16 +0000
> Eric Anholt <[e-mail]> wrote:
>
>> Boris Brezillon <[e-mail]> writes:
>> 
>> > All bcm2835 PLLs should be gated before their rate can be changed.
>> > Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
>> > not enough to make the code work in all situations. Indeed, the
>> > CLK_SET_RATE_GATE flag prevents a user from changing the rate while
>> > the clock is enabled, but this check only guarantees there's no Linux
>> > users. In our case, the clock might have been enabled by the
>> > bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
>> > disables the PLL. So we have to make sure the PLL is actually disabled
>> > before changing the rate.
>> >
>> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
>> > Cc: <[e-mail]>
>> > Signed-off-by: Boris Brezillon <[e-mail]>
>> > ---
>> >  drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
>> >  1 file changed, 13 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> > index 6c5d4a8e426c..051ce769c109 100644
>> > --- a/drivers/clk/bcm/clk-bcm2835.c
>> > +++ b/drivers/clk/bcm/clk-bcm2835.c
>> > @@ -678,6 +678,18 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw,
>> >  	u32 ana[4];
>> >  	int i;
>> >  
>> > +	/*
>> > +	 * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
>> > +	 * the rate while the clock is enabled, but this check only makes sure
>> > +	 * there's no Linux users.
>> > +	 * In our case, the clock might have been enabled by the bootloader/FW,
>> > +	 * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
>> > +	 * So we have to make sure the clk is actually disabled before changing
>> > +	 * the rate.
>> > +	 */
>> > +	if (bcm2835_pll_is_on(hw))
>> > +		bcm2835_pll_off(hw);
>> > +  
>> 
>> I'm not sure this improves the situation.  If the PLL was on, then
>> presumably there's a divider using it and a CM clock using that, so
>> we'll probably end up driving some glitches on them.
>
> Hm, yes, but if someone is trying to change the rate of the PLL, and the
> core doesn't know other clks depend on this PLL (which is the case if
> we reach this point), we're already in big trouble.
>
>> 
>> Does the common clk framework have a way to disable unused clocks from
>> the leaf clocks up to this root, before the general
>> disable-unused-clocks path happens late in the boot process?
>
> Not that I know of. What do you have in mind? 

I was hoping that Stephen Boyd or Mike might have an answer for this
problem.

Boris Brezillon Feb. 12, 2018, 9:27 a.m. UTC | #4

-Stephen Warren
+Stefan Wahren

On Fri, 09 Feb 2018 09:32:40 +0000
Eric Anholt <[e-mail]> wrote:

> Boris Brezillon <[e-mail]> writes:
> 
> > On Thu, 08 Feb 2018 15:20:16 +0000
> > Eric Anholt <[e-mail]> wrote:
> >  
> >> Boris Brezillon <[e-mail]> writes:
> >>   
> >> > All bcm2835 PLLs should be gated before their rate can be changed.
> >> > Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
> >> > not enough to make the code work in all situations. Indeed, the
> >> > CLK_SET_RATE_GATE flag prevents a user from changing the rate while
> >> > the clock is enabled, but this check only guarantees there's no Linux
> >> > users. In our case, the clock might have been enabled by the
> >> > bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
> >> > disables the PLL. So we have to make sure the PLL is actually disabled
> >> > before changing the rate.
> >> >
> >> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
> >> > Cc: <[e-mail]>
> >> > Signed-off-by: Boris Brezillon <[e-mail]>
> >> > ---
> >> >  drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
> >> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> >> > index 6c5d4a8e426c..051ce769c109 100644
> >> > --- a/drivers/clk/bcm/clk-bcm2835.c
> >> > +++ b/drivers/clk/bcm/clk-bcm2835.c
> >> > @@ -678,6 +678,18 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw,
> >> >  	u32 ana[4];
> >> >  	int i;
> >> >  
> >> > +	/*
> >> > +	 * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
> >> > +	 * the rate while the clock is enabled, but this check only makes sure
> >> > +	 * there's no Linux users.
> >> > +	 * In our case, the clock might have been enabled by the bootloader/FW,
> >> > +	 * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
> >> > +	 * So we have to make sure the clk is actually disabled before changing
> >> > +	 * the rate.
> >> > +	 */
> >> > +	if (bcm2835_pll_is_on(hw))
> >> > +		bcm2835_pll_off(hw);
> >> > +    
> >> 
> >> I'm not sure this improves the situation.  If the PLL was on, then
> >> presumably there's a divider using it and a CM clock using that, so
> >> we'll probably end up driving some glitches on them.  
> >
> > Hm, yes, but if someone is trying to change the rate of the PLL, and the
> > core doesn't know other clks depend on this PLL (which is the case if
> > we reach this point), we're already in big trouble.
> >  
> >> 
> >> Does the common clk framework have a way to disable unused clocks from
> >> the leaf clocks up to this root, before the general
> >> disable-unused-clocks path happens late in the boot process?  
> >
> > Not that I know of. What do you have in mind?   
> 
> I was hoping that Stephen Boyd or Mike might have an answer for this
> problem.

Having a generic solution for this sort of issue is definitely the
way to go, but I think this temporary hack is needed to make HDMI/SDTV
work properly. If we don't have it and the FW configures and enables
PLLH with a rate that is different from the one the HDMI or SDTV
encoder tries to set, we're screwed, because I doubt the CPRMAN block
allows you to change the rate of the PLL when it's not gated. Which
means the new rate is not applied and the clk user has no way of
knowing that, which in turn means the display output is likely to not
work properly the first time it's enabled.

Of course, this all goes away the second time the HDMI/SDTV encoder is
enabled, because then clk_disable_unprepare() is called which has the
effect of disabling the PLL.

Stephen Boyd March 12, 2018, 9:21 p.m. UTC | #5

Quoting Boris Brezillon (2018-02-12 01:27:52)
> -Stephen Warren
> +Stefan Wahren
> 
> On Fri, 09 Feb 2018 09:32:40 +0000
> Eric Anholt <[e-mail]> wrote:
> 
> > Boris Brezillon <[e-mail]> writes:
> > 
> > > On Thu, 08 Feb 2018 15:20:16 +0000
> > > Eric Anholt <[e-mail]> wrote:
> > >  
> > >> Boris Brezillon <[e-mail]> writes:
> > >>   
> > >> > All bcm2835 PLLs should be gated before their rate can be changed.
> > >> > Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
> > >> > not enough to make the code work in all situations. Indeed, the
> > >> > CLK_SET_RATE_GATE flag prevents a user from changing the rate while
> > >> > the clock is enabled, but this check only guarantees there's no Linux
> > >> > users. In our case, the clock might have been enabled by the
> > >> > bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
> > >> > disables the PLL. So we have to make sure the PLL is actually disabled
> > >> > before changing the rate.
> > >> >
> > >> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
> > >> > Cc: <[e-mail]>
> > >> > Signed-off-by: Boris Brezillon <[e-mail]>
> > >> > ---
> > >> >  drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
> > >> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> > >> > index 6c5d4a8e426c..051ce769c109 100644
> > >> > --- a/drivers/clk/bcm/clk-bcm2835.c
> > >> > +++ b/drivers/clk/bcm/clk-bcm2835.c
> > >> > @@ -678,6 +678,18 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw,
> > >> >          u32 ana[4];
> > >> >          int i;
> > >> >  
> > >> > +        /*
> > >> > +         * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
> > >> > +         * the rate while the clock is enabled, but this check only makes sure
> > >> > +         * there's no Linux users.
> > >> > +         * In our case, the clock might have been enabled by the bootloader/FW,
> > >> > +         * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
> > >> > +         * So we have to make sure the clk is actually disabled before changing
> > >> > +         * the rate.
> > >> > +         */
> > >> > +        if (bcm2835_pll_is_on(hw))
> > >> > +                bcm2835_pll_off(hw);
> > >> > +    
> > >> 
> > >> I'm not sure this improves the situation.  If the PLL was on, then
> > >> presumably there's a divider using it and a CM clock using that, so
> > >> we'll probably end up driving some glitches on them.  
> > >
> > > Hm, yes, but if someone is trying to change the rate of the PLL, and the
> > > core doesn't know other clks depend on this PLL (which is the case if
> > > we reach this point), we're already in big trouble.
> > >  
> > >> 
> > >> Does the common clk framework have a way to disable unused clocks from
> > >> the leaf clocks up to this root, before the general
> > >> disable-unused-clocks path happens late in the boot process?  
> > >
> > > Not that I know of. What do you have in mind?   
> > 
> > I was hoping that Stephen Boyd or Mike might have an answer for this
> > problem.
> 
> Having a generic solution for this sort of issue is definitely the
> way to go, but I think this temporary hack is needed to make HDMI/SDTV
> work properly. If we don't have it and the FW configures and enables
> PLLH with a rate that is different from the one the HDMI or SDTV
> encoder tries to set, we're screwed, because I doubt the CPRMAN block
> allows you to change the rate of the PLL when it's not gated. Which
> means the new rate is not applied and the clk user has no way of
> knowing that, which in turn means the display output is likely to not
> work properly the first time it's enabled.
> 
> Of course, this all goes away the second time the HDMI/SDTV encoder is
> enabled, because then clk_disable_unprepare() is called which has the
> effect of disabling the PLL.
> 

There isn't any sort of API to disable unused clks from a leaf up to a
particular point in the tree. Actually, the disabling of unused clks
during late init makes the framework harder to maintain so expanding on
it is not high on the list of things to do.

What exactly is going on here? It sounds like the framework isn't aware
of the 'on/off' boot state of certain clks (a known problem) and that's
causing some sort of problem when changing rates? This usually happens
with PLLs that are enabled at boot time and can't support their rate
changing when they're enabled. We really should start reading on/off
state and "hand off" that enabled state to something in the framework so
we at least know if a clk is enabled or not out of boot. There was some
work on clk handoff done a while ago by Mike that never landed which may
be useful to finish this off. Maybe we can pass that enabled state off
to the clk we always create for a clk_hw structure at registration time
and then have clk_disable_unused operate on that clk pointer at late
init.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eric Anholt March 13, 2018, 4:56 p.m. UTC | #6

Stephen Boyd <[e-mail]> writes:

> Quoting Boris Brezillon (2018-02-12 01:27:52)
>> -Stephen Warren
>> +Stefan Wahren
>> 
>> On Fri, 09 Feb 2018 09:32:40 +0000
>> Eric Anholt <[e-mail]> wrote:
>> 
>> > Boris Brezillon <[e-mail]> writes:
>> > 
>> > > On Thu, 08 Feb 2018 15:20:16 +0000
>> > > Eric Anholt <[e-mail]> wrote:
>> > >  
>> > >> Boris Brezillon <[e-mail]> writes:
>> > >>   
>> > >> > All bcm2835 PLLs should be gated before their rate can be changed.
>> > >> > Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
>> > >> > not enough to make the code work in all situations. Indeed, the
>> > >> > CLK_SET_RATE_GATE flag prevents a user from changing the rate while
>> > >> > the clock is enabled, but this check only guarantees there's no Linux
>> > >> > users. In our case, the clock might have been enabled by the
>> > >> > bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
>> > >> > disables the PLL. So we have to make sure the PLL is actually disabled
>> > >> > before changing the rate.
>> > >> >
>> > >> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
>> > >> > Cc: <[e-mail]>
>> > >> > Signed-off-by: Boris Brezillon <[e-mail]>
>> > >> > ---
>> > >> >  drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
>> > >> >  1 file changed, 13 insertions(+), 1 deletion(-)
>> > >> >
>> > >> > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> > >> > index 6c5d4a8e426c..051ce769c109 100644
>> > >> > --- a/drivers/clk/bcm/clk-bcm2835.c
>> > >> > +++ b/drivers/clk/bcm/clk-bcm2835.c
>> > >> > @@ -678,6 +678,18 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw,
>> > >> >          u32 ana[4];
>> > >> >          int i;
>> > >> >  
>> > >> > +        /*
>> > >> > +         * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
>> > >> > +         * the rate while the clock is enabled, but this check only makes sure
>> > >> > +         * there's no Linux users.
>> > >> > +         * In our case, the clock might have been enabled by the bootloader/FW,
>> > >> > +         * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
>> > >> > +         * So we have to make sure the clk is actually disabled before changing
>> > >> > +         * the rate.
>> > >> > +         */
>> > >> > +        if (bcm2835_pll_is_on(hw))
>> > >> > +                bcm2835_pll_off(hw);
>> > >> > +    
>> > >> 
>> > >> I'm not sure this improves the situation.  If the PLL was on, then
>> > >> presumably there's a divider using it and a CM clock using that, so
>> > >> we'll probably end up driving some glitches on them.  
>> > >
>> > > Hm, yes, but if someone is trying to change the rate of the PLL, and the
>> > > core doesn't know other clks depend on this PLL (which is the case if
>> > > we reach this point), we're already in big trouble.
>> > >  
>> > >> 
>> > >> Does the common clk framework have a way to disable unused clocks from
>> > >> the leaf clocks up to this root, before the general
>> > >> disable-unused-clocks path happens late in the boot process?  
>> > >
>> > > Not that I know of. What do you have in mind?   
>> > 
>> > I was hoping that Stephen Boyd or Mike might have an answer for this
>> > problem.
>> 
>> Having a generic solution for this sort of issue is definitely the
>> way to go, but I think this temporary hack is needed to make HDMI/SDTV
>> work properly. If we don't have it and the FW configures and enables
>> PLLH with a rate that is different from the one the HDMI or SDTV
>> encoder tries to set, we're screwed, because I doubt the CPRMAN block
>> allows you to change the rate of the PLL when it's not gated. Which
>> means the new rate is not applied and the clk user has no way of
>> knowing that, which in turn means the display output is likely to not
>> work properly the first time it's enabled.
>> 
>> Of course, this all goes away the second time the HDMI/SDTV encoder is
>> enabled, because then clk_disable_unprepare() is called which has the
>> effect of disabling the PLL.
>> 
>
> There isn't any sort of API to disable unused clks from a leaf up to a
> particular point in the tree. Actually, the disabling of unused clks
> during late init makes the framework harder to maintain so expanding on
> it is not high on the list of things to do.
>
> What exactly is going on here? It sounds like the framework isn't aware
> of the 'on/off' boot state of certain clks (a known problem) and that's
> causing some sort of problem when changing rates? This usually happens
> with PLLs that are enabled at boot time and can't support their rate
> changing when they're enabled. We really should start reading on/off
> state and "hand off" that enabled state to something in the framework so
> we at least know if a clk is enabled or not out of boot. There was some
> work on clk handoff done a while ago by Mike that never landed which may
> be useful to finish this off. Maybe we can pass that enabled state off
> to the clk we always create for a clk_hw structure at registration time
> and then have clk_disable_unused operate on that clk pointer at late
> init.

Yes, the usual problem of clk not handling boot-time clock state well.

That said, it's patch 1 that's critical for fixing many of our users,
and we need that in as soon as possible.  #2 is also reviewed and ready.

Stephen Boyd March 19, 2018, 4:26 p.m. UTC | #7

Quoting Eric Anholt (2018-03-13 09:56:57)
> Stephen Boyd <[e-mail]> writes:
> >
> > What exactly is going on here? It sounds like the framework isn't aware
> > of the 'on/off' boot state of certain clks (a known problem) and that's
> > causing some sort of problem when changing rates? This usually happens
> > with PLLs that are enabled at boot time and can't support their rate
> > changing when they're enabled. We really should start reading on/off
> > state and "hand off" that enabled state to something in the framework so
> > we at least know if a clk is enabled or not out of boot. There was some
> > work on clk handoff done a while ago by Mike that never landed which may
> > be useful to finish this off. Maybe we can pass that enabled state off
> > to the clk we always create for a clk_hw structure at registration time
> > and then have clk_disable_unused operate on that clk pointer at late
> > init.
> 
> Yes, the usual problem of clk not handling boot-time clock state well.
> 
> That said, it's patch 1 that's critical for fixing many of our users,
> and we need that in as soon as possible.  #2 is also reviewed and ready.

Got it. I'll pick up the first two then.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Boris Brezillon March 22, 2018, 9:13 a.m. UTC | #8

Hi Stephen,

On Mon, 12 Mar 2018 14:21:22 -0700
Stephen Boyd <[e-mail]> wrote:

> Quoting Boris Brezillon (2018-02-12 01:27:52)
> > -Stephen Warren
> > +Stefan Wahren
> > 
> > On Fri, 09 Feb 2018 09:32:40 +0000
> > Eric Anholt <[e-mail]> wrote:
> >   
> > > Boris Brezillon <[e-mail]> writes:
> > >   
> > > > On Thu, 08 Feb 2018 15:20:16 +0000
> > > > Eric Anholt <[e-mail]> wrote:
> > > >    
> > > >> Boris Brezillon <[e-mail]> writes:
> > > >>     
> > > >> > All bcm2835 PLLs should be gated before their rate can be changed.
> > > >> > Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
> > > >> > not enough to make the code work in all situations. Indeed, the
> > > >> > CLK_SET_RATE_GATE flag prevents a user from changing the rate while
> > > >> > the clock is enabled, but this check only guarantees there's no Linux
> > > >> > users. In our case, the clock might have been enabled by the
> > > >> > bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
> > > >> > disables the PLL. So we have to make sure the PLL is actually disabled
> > > >> > before changing the rate.
> > > >> >
> > > >> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
> > > >> > Cc: <[e-mail]>
> > > >> > Signed-off-by: Boris Brezillon <[e-mail]>
> > > >> > ---
> > > >> >  drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
> > > >> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > >> >
> > > >> > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> > > >> > index 6c5d4a8e426c..051ce769c109 100644
> > > >> > --- a/drivers/clk/bcm/clk-bcm2835.c
> > > >> > +++ b/drivers/clk/bcm/clk-bcm2835.c
> > > >> > @@ -678,6 +678,18 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw,
> > > >> >          u32 ana[4];
> > > >> >          int i;
> > > >> >  
> > > >> > +        /*
> > > >> > +         * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
> > > >> > +         * the rate while the clock is enabled, but this check only makes sure
> > > >> > +         * there's no Linux users.
> > > >> > +         * In our case, the clock might have been enabled by the bootloader/FW,
> > > >> > +         * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
> > > >> > +         * So we have to make sure the clk is actually disabled before changing
> > > >> > +         * the rate.
> > > >> > +         */
> > > >> > +        if (bcm2835_pll_is_on(hw))
> > > >> > +                bcm2835_pll_off(hw);
> > > >> > +      
> > > >> 
> > > >> I'm not sure this improves the situation.  If the PLL was on, then
> > > >> presumably there's a divider using it and a CM clock using that, so
> > > >> we'll probably end up driving some glitches on them.    
> > > >
> > > > Hm, yes, but if someone is trying to change the rate of the PLL, and the
> > > > core doesn't know other clks depend on this PLL (which is the case if
> > > > we reach this point), we're already in big trouble.
> > > >    
> > > >> 
> > > >> Does the common clk framework have a way to disable unused clocks from
> > > >> the leaf clocks up to this root, before the general
> > > >> disable-unused-clocks path happens late in the boot process?    
> > > >
> > > > Not that I know of. What do you have in mind?     
> > > 
> > > I was hoping that Stephen Boyd or Mike might have an answer for this
> > > problem.  
> > 
> > Having a generic solution for this sort of issue is definitely the
> > way to go, but I think this temporary hack is needed to make HDMI/SDTV
> > work properly. If we don't have it and the FW configures and enables
> > PLLH with a rate that is different from the one the HDMI or SDTV
> > encoder tries to set, we're screwed, because I doubt the CPRMAN block
> > allows you to change the rate of the PLL when it's not gated. Which
> > means the new rate is not applied and the clk user has no way of
> > knowing that, which in turn means the display output is likely to not
> > work properly the first time it's enabled.
> > 
> > Of course, this all goes away the second time the HDMI/SDTV encoder is
> > enabled, because then clk_disable_unprepare() is called which has the
> > effect of disabling the PLL.
> >   
> 
> There isn't any sort of API to disable unused clks from a leaf up to a
> particular point in the tree. Actually, the disabling of unused clks
> during late init makes the framework harder to maintain so expanding on
> it is not high on the list of things to do.
> 
> What exactly is going on here? It sounds like the framework isn't aware
> of the 'on/off' boot state of certain clks (a known problem) and that's
> causing some sort of problem when changing rates? This usually happens
> with PLLs that are enabled at boot time and can't support their rate
> changing when they're enabled. We really should start reading on/off
> state and "hand off" that enabled state to something in the framework so
> we at least know if a clk is enabled or not out of boot.

Okay.

> There was some
> work on clk handoff done a while ago by Mike that never landed which may
> be useful to finish this off.

Can you point me to the initial work done by Mike? Are you referring to
[1]?

> Maybe we can pass that enabled state off
> to the clk we always create for a clk_hw structure at registration time

Actually, we already have a way to retrieve the HW state:
->is_prepared()/->is_enabled().

> and then have clk_disable_unused operate on that clk pointer at late
> init.

That won't solve the problem we have here, because the HDMI encoder
driver might try to change the clk rate before late init, and we must
disable the pll before changing its rate. And there's more than that:
the HDMI encoder manipulates a leaf clk and is not aware of the PLL
used as a source for this hdmi clk, so we can't even consider doing
something like that:

	if (clk_is_enabled(hdmi_clk))
		clk_disable_unprepare(hdmi_clk);
	
	clk_set_rate(hdmi_clk, xxx);
	clk_prepare_enable(hdmi_clk);

We need the framework to figure this out on his own and let him decide
to disable the clk before setting the pll rate.
This being said, I'm not sure we want to apply this behavior to
everyone. You may have setups where some clks initialized/enabled by
the FW/bootloader may rely on the PLL and its pre-configured rate and
you don't want another clk user from messing up with the PLL.

I'm not against discussing a generic solution to solve this problem
but I think it's not as easy as it seems and IMO it's worth
having the current hack for the bcm2835 driver (I can restrict it to
PLLH if you prefer).

[1]https://lwn.net/Articles/675658/

Eric Anholt March 22, 2018, 4:38 p.m. UTC | #9

Boris Brezillon <[e-mail]> writes:

> Hi Stephen,
>
> On Mon, 12 Mar 2018 14:21:22 -0700
> Stephen Boyd <[e-mail]> wrote:
>
>> Quoting Boris Brezillon (2018-02-12 01:27:52)
>> > -Stephen Warren
>> > +Stefan Wahren
>> > 
>> > On Fri, 09 Feb 2018 09:32:40 +0000
>> > Eric Anholt <[e-mail]> wrote:
>> >   
>> > > Boris Brezillon <[e-mail]> writes:
>> > >   
>> > > > On Thu, 08 Feb 2018 15:20:16 +0000
>> > > > Eric Anholt <[e-mail]> wrote:
>> > > >    
>> > > >> Boris Brezillon <[e-mail]> writes:
>> > > >>     
>> > > >> > All bcm2835 PLLs should be gated before their rate can be changed.
>> > > >> > Setting CLK_SET_RATE_GATE will let the core enforce that, but this is
>> > > >> > not enough to make the code work in all situations. Indeed, the
>> > > >> > CLK_SET_RATE_GATE flag prevents a user from changing the rate while
>> > > >> > the clock is enabled, but this check only guarantees there's no Linux
>> > > >> > users. In our case, the clock might have been enabled by the
>> > > >> > bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux never
>> > > >> > disables the PLL. So we have to make sure the PLL is actually disabled
>> > > >> > before changing the rate.
>> > > >> >
>> > > >> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
>> > > >> > Cc: <[e-mail]>
>> > > >> > Signed-off-by: Boris Brezillon <[e-mail]>
>> > > >> > ---
>> > > >> >  drivers/clk/bcm/clk-bcm2835.c | 14 +++++++++++++-
>> > > >> >  1 file changed, 13 insertions(+), 1 deletion(-)
>> > > >> >
>> > > >> > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> > > >> > index 6c5d4a8e426c..051ce769c109 100644
>> > > >> > --- a/drivers/clk/bcm/clk-bcm2835.c
>> > > >> > +++ b/drivers/clk/bcm/clk-bcm2835.c
>> > > >> > @@ -678,6 +678,18 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw,
>> > > >> >          u32 ana[4];
>> > > >> >          int i;
>> > > >> >  
>> > > >> > +        /*
>> > > >> > +         * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
>> > > >> > +         * the rate while the clock is enabled, but this check only makes sure
>> > > >> > +         * there's no Linux users.
>> > > >> > +         * In our case, the clock might have been enabled by the bootloader/FW,
>> > > >> > +         * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
>> > > >> > +         * So we have to make sure the clk is actually disabled before changing
>> > > >> > +         * the rate.
>> > > >> > +         */
>> > > >> > +        if (bcm2835_pll_is_on(hw))
>> > > >> > +                bcm2835_pll_off(hw);
>> > > >> > +      
>> > > >> 
>> > > >> I'm not sure this improves the situation.  If the PLL was on, then
>> > > >> presumably there's a divider using it and a CM clock using that, so
>> > > >> we'll probably end up driving some glitches on them.    
>> > > >
>> > > > Hm, yes, but if someone is trying to change the rate of the PLL, and the
>> > > > core doesn't know other clks depend on this PLL (which is the case if
>> > > > we reach this point), we're already in big trouble.
>> > > >    
>> > > >> 
>> > > >> Does the common clk framework have a way to disable unused clocks from
>> > > >> the leaf clocks up to this root, before the general
>> > > >> disable-unused-clocks path happens late in the boot process?    
>> > > >
>> > > > Not that I know of. What do you have in mind?     
>> > > 
>> > > I was hoping that Stephen Boyd or Mike might have an answer for this
>> > > problem.  
>> > 
>> > Having a generic solution for this sort of issue is definitely the
>> > way to go, but I think this temporary hack is needed to make HDMI/SDTV
>> > work properly. If we don't have it and the FW configures and enables
>> > PLLH with a rate that is different from the one the HDMI or SDTV
>> > encoder tries to set, we're screwed, because I doubt the CPRMAN block
>> > allows you to change the rate of the PLL when it's not gated. Which
>> > means the new rate is not applied and the clk user has no way of
>> > knowing that, which in turn means the display output is likely to not
>> > work properly the first time it's enabled.
>> > 
>> > Of course, this all goes away the second time the HDMI/SDTV encoder is
>> > enabled, because then clk_disable_unprepare() is called which has the
>> > effect of disabling the PLL.
>> >   
>> 
>> There isn't any sort of API to disable unused clks from a leaf up to a
>> particular point in the tree. Actually, the disabling of unused clks
>> during late init makes the framework harder to maintain so expanding on
>> it is not high on the list of things to do.
>> 
>> What exactly is going on here? It sounds like the framework isn't aware
>> of the 'on/off' boot state of certain clks (a known problem) and that's
>> causing some sort of problem when changing rates? This usually happens
>> with PLLs that are enabled at boot time and can't support their rate
>> changing when they're enabled. We really should start reading on/off
>> state and "hand off" that enabled state to something in the framework so
>> we at least know if a clk is enabled or not out of boot.

> everyone. You may have setups where some clks initialized/enabled by
> the FW/bootloader may rely on the PLL and its pre-configured rate and
> you don't want another clk user from messing up with the PLL.

FWIW, we are effectively doing this with how the PLLH dividers have
CLK_SET_RATE_PARENT set but, for example, PLLD_DSI* don't -- we don't
want the DSI clock to end up tweaking the clocks of all the devices
hanging off of PLLD.  The hardware is no different, we've just baked
this policy into the code.

Patch

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 6c5d4a8e426c..051ce769c109 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -678,6 +678,18 @@  static int bcm2835_pll_set_rate(struct clk_hw *hw,
 	u32 ana[4];
 	int i;
 
+	/*
+	 * Normally, the CLK_SET_RATE_GATE flag prevents a user from changing
+	 * the rate while the clock is enabled, but this check only makes sure
+	 * there's no Linux users.
+	 * In our case, the clock might have been enabled by the bootloader/FW,
+	 * and, since CLK_IGNORE_UNUSED flag is set, Linux never disables it.
+	 * So we have to make sure the clk is actually disabled before changing
+	 * the rate.
+	 */
+	if (bcm2835_pll_is_on(hw))
+		bcm2835_pll_off(hw);
+
 	if (rate > data->max_fb_rate) {
 		use_fb_prediv = true;
 		rate /= 2;
@@ -1318,7 +1330,7 @@  static struct clk_hw *bcm2835_register_pll(struct bcm2835_cprman *cprman,
 	init.num_parents = 1;
 	init.name = data->name;
 	init.ops = &bcm2835_pll_clk_ops;
-	init.flags = CLK_IGNORE_UNUSED;
+	init.flags = CLK_IGNORE_UNUSED | CLK_SET_RATE_GATE;
 
 	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
 	if (!pll)