* [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions @ 2018-02-08 13:43 Boris Brezillon 2018-02-08 13:43 ` [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers Boris Brezillon ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Boris Brezillon @ 2018-02-08 13:43 UTC (permalink / raw) To: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, Eric Anholt, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk Cc: Boris Brezillon, stable ana->maskX values are already '~'-ed in bcm2835_pll_set_rate(). Remove the '~' in the definition to fix ANA setup. Note that this commit fixes a long standing bug preventing one from using an HDMI display if it's plugged after the FW has booted Linux. This is because PLLH is used by the HDMI encoder to generate the pixel clock. Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") Cc: <stable@vger.kernel.org> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> Reviewed-by: Eric Anholt <eric@anholt.net> --- drivers/clk/bcm/clk-bcm2835.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index 44301a3d9963..2108a274185a 100644 --- a/drivers/clk/bcm/clk-bcm2835.c +++ b/drivers/clk/bcm/clk-bcm2835.c @@ -449,17 +449,17 @@ struct bcm2835_pll_ana_bits { static const struct bcm2835_pll_ana_bits bcm2835_ana_default = { .mask0 = 0, .set0 = 0, - .mask1 = (u32)~(A2W_PLL_KI_MASK | A2W_PLL_KP_MASK), + .mask1 = A2W_PLL_KI_MASK | A2W_PLL_KP_MASK, .set1 = (2 << A2W_PLL_KI_SHIFT) | (8 << A2W_PLL_KP_SHIFT), - .mask3 = (u32)~A2W_PLL_KA_MASK, + .mask3 = A2W_PLL_KA_MASK, .set3 = (2 << A2W_PLL_KA_SHIFT), .fb_prediv_mask = BIT(14), }; static const struct bcm2835_pll_ana_bits bcm2835_ana_pllh = { - .mask0 = (u32)~(A2W_PLLH_KA_MASK | A2W_PLLH_KI_LOW_MASK), + .mask0 = A2W_PLLH_KA_MASK | A2W_PLLH_KI_LOW_MASK, .set0 = (2 << A2W_PLLH_KA_SHIFT) | (2 << A2W_PLLH_KI_LOW_SHIFT), - .mask1 = (u32)~(A2W_PLLH_KI_HIGH_MASK | A2W_PLLH_KP_MASK), + .mask1 = A2W_PLLH_KI_HIGH_MASK | A2W_PLLH_KP_MASK, .set1 = (6 << A2W_PLLH_KP_SHIFT), .mask3 = 0, .set3 = 0, -- 2.14.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers 2018-02-08 13:43 [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions Boris Brezillon @ 2018-02-08 13:43 ` Boris Brezillon 2018-02-08 15:14 ` Eric Anholt ` (2 more replies) 2018-02-08 13:43 ` [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate Boris Brezillon ` (2 subsequent siblings) 3 siblings, 3 replies; 25+ messages in thread From: Boris Brezillon @ 2018-02-08 13:43 UTC (permalink / raw) To: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, Eric Anholt, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk Cc: Boris Brezillon, stable CM_PLLx and A2W_XOSC_CTRL registers are accessed by different clock handlers and must be accessed with ->regs_lock held. Update the sections where this protection is missing. Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") Cc: <stable@vger.kernel.org> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- drivers/clk/bcm/clk-bcm2835.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index 2108a274185a..a07f6451694a 100644 --- a/drivers/clk/bcm/clk-bcm2835.c +++ b/drivers/clk/bcm/clk-bcm2835.c @@ -623,8 +623,10 @@ static int bcm2835_pll_on(struct clk_hw *hw) ~A2W_PLL_CTRL_PWRDN); /* Take the PLL out of reset. */ + spin_lock(&cprman->regs_lock); cprman_write(cprman, data->cm_ctrl_reg, cprman_read(cprman, data->cm_ctrl_reg) & ~CM_PLL_ANARST); + spin_unlock(&cprman->regs_lock); /* Wait for the PLL to lock. */ timeout = ktime_add_ns(ktime_get(), LOCK_TIMEOUT_NS); @@ -701,9 +703,11 @@ static int bcm2835_pll_set_rate(struct clk_hw *hw, } /* Unmask the reference clock from the oscillator. */ + spin_lock(&cprman->regs_lock); cprman_write(cprman, A2W_XOSC_CTRL, cprman_read(cprman, A2W_XOSC_CTRL) | data->reference_enable_mask); + spin_unlock(&cprman->regs_lock); if (do_ana_setup_first) bcm2835_pll_write_ana(cprman, data->ana_reg_base, ana); -- 2.14.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers 2018-02-08 13:43 ` [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers Boris Brezillon @ 2018-02-08 15:14 ` Eric Anholt 2018-02-09 9:34 ` Eric Anholt 2018-03-19 16:29 ` Stephen Boyd 2 siblings, 0 replies; 25+ messages in thread From: Eric Anholt @ 2018-02-08 15:14 UTC (permalink / raw) To: Boris Brezillon, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk Cc: Boris Brezillon, stable [-- Attachment #1: Type: text/plain, Size: 468 bytes --] Boris Brezillon <boris.brezillon@bootlin.com> writes: > CM_PLLx and A2W_XOSC_CTRL registers are accessed by different clock > handlers and must be accessed with ->regs_lock held. > Update the sections where this protection is missing. > > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") > Cc: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> Reviewed-by: Eric Anholt <eric@anholt.net> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers 2018-02-08 13:43 ` [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers Boris Brezillon 2018-02-08 15:14 ` Eric Anholt @ 2018-02-09 9:34 ` Eric Anholt 2018-03-19 16:29 ` Stephen Boyd 2 siblings, 0 replies; 25+ messages in thread From: Eric Anholt @ 2018-02-09 9:34 UTC (permalink / raw) To: Boris Brezillon, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk Cc: Boris Brezillon, stable [-- Attachment #1: Type: text/plain, Size: 341 bytes --] Boris Brezillon <boris.brezillon@bootlin.com> writes: > CM_PLLx and A2W_XOSC_CTRL registers are accessed by different clock > handlers and must be accessed with ->regs_lock held. > Update the sections where this protection is missing. Just to be clear, while I think we have an interesting question on patch 4/4, I'd love to see 1-2 land. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers 2018-02-08 13:43 ` [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers Boris Brezillon @ 2018-03-19 16:29 ` Stephen Boyd 2018-02-09 9:34 ` Eric Anholt 2018-03-19 16:29 ` Stephen Boyd 2 siblings, 0 replies; 25+ messages in thread From: Stephen Boyd @ 2018-03-19 16:29 UTC (permalink / raw) To: Boris Brezillon, Eric Anholt, Florian Fainelli, Lee Jones, Mike Turquette, Ray Jui, Scott Branden, Stephen Boyd, Stephen Warren, bcm-kernel-feedback-list, linux-clk, linux-rpi-kernel Cc: Boris Brezillon, stable Quoting Boris Brezillon (2018-02-08 05:43:36) > CM_PLLx and A2W_XOSC_CTRL registers are accessed by different clock > handlers and must be accessed with ->regs_lock held. > Update the sections where this protection is missing. > > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") > Cc: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- Applied to clk-fixes ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers @ 2018-03-19 16:29 ` Stephen Boyd 0 siblings, 0 replies; 25+ messages in thread From: Stephen Boyd @ 2018-03-19 16:29 UTC (permalink / raw) To: Boris Brezillon, Eric Anholt, Florian Fainelli, Lee Jones, Mike Turquette, Ray Jui, Scott Branden, Stephen Boyd, Stephen Warren, bcm-kernel-feedback-list, linux-clk, linux-rpi-kernel Cc: Boris Brezillon, stable Quoting Boris Brezillon (2018-02-08 05:43:36) > CM_PLLx and A2W_XOSC_CTRL registers are accessed by different clock > handlers and must be accessed with ->regs_lock held. > Update the sections where this protection is missing. > = > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio= domain clocks") > Cc: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- Applied to clk-fixes ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate 2018-02-08 13:43 [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions Boris Brezillon 2018-02-08 13:43 ` [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers Boris Brezillon @ 2018-02-08 13:43 ` Boris Brezillon 2018-02-08 15:15 ` Eric Anholt 2018-02-08 13:43 ` [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate Boris Brezillon 2018-03-19 16:29 ` Stephen Boyd 3 siblings, 1 reply; 25+ messages in thread From: Boris Brezillon @ 2018-02-08 13:43 UTC (permalink / raw) To: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, Eric Anholt, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk Cc: Boris Brezillon, stable In order to enable a PLL, not only the PLL has to be powered up and locked, but you also have to de-assert the reset signal. The last part was missing. Add it so PLLs that were not enabled by the FW/bootloader can be enabled from Linux. Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") Cc: <stable@vger.kernel.org> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- drivers/clk/bcm/clk-bcm2835.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index a07f6451694a..6c5d4a8e426c 100644 --- a/drivers/clk/bcm/clk-bcm2835.c +++ b/drivers/clk/bcm/clk-bcm2835.c @@ -602,6 +602,9 @@ static void bcm2835_pll_off(struct clk_hw *hw) const struct bcm2835_pll_data *data = pll->data; spin_lock(&cprman->regs_lock); + cprman_write(cprman, data->a2w_ctrl_reg, + cprman_read(cprman, data->a2w_ctrl_reg) & + ~A2W_PLL_CTRL_PRST_DISABLE); cprman_write(cprman, data->cm_ctrl_reg, cprman_read(cprman, data->cm_ctrl_reg) | CM_PLL_ANARST); @@ -640,6 +643,10 @@ static int bcm2835_pll_on(struct clk_hw *hw) cpu_relax(); } + cprman_write(cprman, data->a2w_ctrl_reg, + cprman_read(cprman, data->a2w_ctrl_reg) | + A2W_PLL_CTRL_PRST_DISABLE); + return 0; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate 2018-02-08 13:43 ` [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate Boris Brezillon @ 2018-02-08 15:15 ` Eric Anholt 2018-02-08 17:49 ` Boris Brezillon 2018-02-14 10:37 ` Boris Brezillon 0 siblings, 2 replies; 25+ messages in thread From: Eric Anholt @ 2018-02-08 15:15 UTC (permalink / raw) To: Boris Brezillon, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk Cc: Boris Brezillon, stable [-- Attachment #1: Type: text/plain, Size: 1770 bytes --] Boris Brezillon <boris.brezillon@bootlin.com> writes: > In order to enable a PLL, not only the PLL has to be powered up and > locked, but you also have to de-assert the reset signal. The last part > was missing. Add it so PLLs that were not enabled by the FW/bootloader > can be enabled from Linux. > > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") > Cc: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/clk/bcm/clk-bcm2835.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c > index a07f6451694a..6c5d4a8e426c 100644 > --- a/drivers/clk/bcm/clk-bcm2835.c > +++ b/drivers/clk/bcm/clk-bcm2835.c > @@ -602,6 +602,9 @@ static void bcm2835_pll_off(struct clk_hw *hw) > const struct bcm2835_pll_data *data = pll->data; > > spin_lock(&cprman->regs_lock); > + cprman_write(cprman, data->a2w_ctrl_reg, > + cprman_read(cprman, data->a2w_ctrl_reg) & > + ~A2W_PLL_CTRL_PRST_DISABLE); > cprman_write(cprman, data->cm_ctrl_reg, > cprman_read(cprman, data->cm_ctrl_reg) | > CM_PLL_ANARST); For turning off, the FW just does the equivalent of: cprman_write(cprman, data->cm_ctrl_reg, CM_PLL_ANARST); cprman_write(cprman, data->a2w_ctrl_reg, A2W_PLL_CTRL_PWRDN); How about we do that, instead? > @@ -640,6 +643,10 @@ static int bcm2835_pll_on(struct clk_hw *hw) > cpu_relax(); > } > > + cprman_write(cprman, data->a2w_ctrl_reg, > + cprman_read(cprman, data->a2w_ctrl_reg) | > + A2W_PLL_CTRL_PRST_DISABLE); > + > return 0; > } I agree with this hunk -- they drop PRST at the very end, after lock. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate 2018-02-08 15:15 ` Eric Anholt @ 2018-02-08 17:49 ` Boris Brezillon 2018-02-14 10:37 ` Boris Brezillon 1 sibling, 0 replies; 25+ messages in thread From: Boris Brezillon @ 2018-02-08 17:49 UTC (permalink / raw) To: Eric Anholt Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable On Thu, 08 Feb 2018 15:15:42 +0000 Eric Anholt <eric@anholt.net> wrote: > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > In order to enable a PLL, not only the PLL has to be powered up and > > locked, but you also have to de-assert the reset signal. The last part > > was missing. Add it so PLLs that were not enabled by the FW/bootloader > > can be enabled from Linux. > > > > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > drivers/clk/bcm/clk-bcm2835.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c > > index a07f6451694a..6c5d4a8e426c 100644 > > --- a/drivers/clk/bcm/clk-bcm2835.c > > +++ b/drivers/clk/bcm/clk-bcm2835.c > > @@ -602,6 +602,9 @@ static void bcm2835_pll_off(struct clk_hw *hw) > > const struct bcm2835_pll_data *data = pll->data; > > > > spin_lock(&cprman->regs_lock); > > + cprman_write(cprman, data->a2w_ctrl_reg, > > + cprman_read(cprman, data->a2w_ctrl_reg) & > > + ~A2W_PLL_CTRL_PRST_DISABLE); > > cprman_write(cprman, data->cm_ctrl_reg, > > cprman_read(cprman, data->cm_ctrl_reg) | > > CM_PLL_ANARST); > > For turning off, the FW just does the equivalent of: > > cprman_write(cprman, data->cm_ctrl_reg, CM_PLL_ANARST); > cprman_write(cprman, data->a2w_ctrl_reg, A2W_PLL_CTRL_PWRDN); > > How about we do that, instead? Agreed. > > > @@ -640,6 +643,10 @@ static int bcm2835_pll_on(struct clk_hw *hw) > > cpu_relax(); > > } > > > > + cprman_write(cprman, data->a2w_ctrl_reg, > > + cprman_read(cprman, data->a2w_ctrl_reg) | > > + A2W_PLL_CTRL_PRST_DISABLE); > > + > > return 0; > > } > > I agree with this hunk -- they drop PRST at the very end, after lock. -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate 2018-02-08 15:15 ` Eric Anholt 2018-02-08 17:49 ` Boris Brezillon @ 2018-02-14 10:37 ` Boris Brezillon 2018-02-22 20:18 ` Eric Anholt 1 sibling, 1 reply; 25+ messages in thread From: Boris Brezillon @ 2018-02-14 10:37 UTC (permalink / raw) To: Eric Anholt Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable On Thu, 08 Feb 2018 15:15:42 +0000 Eric Anholt <eric@anholt.net> wrote: > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > In order to enable a PLL, not only the PLL has to be powered up and > > locked, but you also have to de-assert the reset signal. The last part > > was missing. Add it so PLLs that were not enabled by the FW/bootloader > > can be enabled from Linux. > > > > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > drivers/clk/bcm/clk-bcm2835.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c > > index a07f6451694a..6c5d4a8e426c 100644 > > --- a/drivers/clk/bcm/clk-bcm2835.c > > +++ b/drivers/clk/bcm/clk-bcm2835.c > > @@ -602,6 +602,9 @@ static void bcm2835_pll_off(struct clk_hw *hw) > > const struct bcm2835_pll_data *data = pll->data; > > > > spin_lock(&cprman->regs_lock); > > + cprman_write(cprman, data->a2w_ctrl_reg, > > + cprman_read(cprman, data->a2w_ctrl_reg) & > > + ~A2W_PLL_CTRL_PRST_DISABLE); > > cprman_write(cprman, data->cm_ctrl_reg, > > cprman_read(cprman, data->cm_ctrl_reg) | > > CM_PLL_ANARST); > > For turning off, the FW just does the equivalent of: > > cprman_write(cprman, data->cm_ctrl_reg, CM_PLL_ANARST); > cprman_write(cprman, data->a2w_ctrl_reg, A2W_PLL_CTRL_PWRDN); Hm, the write to ->a2w_ctrl_reg overwrites the NDIV/PDIV values done in bcm2835_pll_set_rate(). So, either we do: cprman_write(cprman, data->cm_ctrl_reg, CM_PLL_ANARST); cprman_write(cprman, data->a2w_ctrl_reg, cprman_read(cprman, data->a2w_ctrl_reg) | A2W_PLL_CTRL_PWRDN); or we cache the pdiv/ndiv values in struct bcm2835_pll and only apply them in bcm2835_pll_on(). I'd recommend going for the former to keep the changes easily backportable to older kernels. > > How about we do that, instead? > > > @@ -640,6 +643,10 @@ static int bcm2835_pll_on(struct clk_hw *hw) > > cpu_relax(); > > } > > > > + cprman_write(cprman, data->a2w_ctrl_reg, > > + cprman_read(cprman, data->a2w_ctrl_reg) | > > + A2W_PLL_CTRL_PRST_DISABLE); > > + > > return 0; > > } > > I agree with this hunk -- they drop PRST at the very end, after lock. -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate 2018-02-14 10:37 ` Boris Brezillon @ 2018-02-22 20:18 ` Eric Anholt 0 siblings, 0 replies; 25+ messages in thread From: Eric Anholt @ 2018-02-22 20:18 UTC (permalink / raw) To: Boris Brezillon Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable [-- Attachment #1: Type: text/plain, Size: 2216 bytes --] Boris Brezillon <boris.brezillon@bootlin.com> writes: > On Thu, 08 Feb 2018 15:15:42 +0000 > Eric Anholt <eric@anholt.net> wrote: > >> Boris Brezillon <boris.brezillon@bootlin.com> writes: >> >> > In order to enable a PLL, not only the PLL has to be powered up and >> > locked, but you also have to de-assert the reset signal. The last part >> > was missing. Add it so PLLs that were not enabled by the FW/bootloader >> > can be enabled from Linux. >> > >> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") >> > Cc: <stable@vger.kernel.org> >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> > --- >> > drivers/clk/bcm/clk-bcm2835.c | 7 +++++++ >> > 1 file changed, 7 insertions(+) >> > >> > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c >> > index a07f6451694a..6c5d4a8e426c 100644 >> > --- a/drivers/clk/bcm/clk-bcm2835.c >> > +++ b/drivers/clk/bcm/clk-bcm2835.c >> > @@ -602,6 +602,9 @@ static void bcm2835_pll_off(struct clk_hw *hw) >> > const struct bcm2835_pll_data *data = pll->data; >> > >> > spin_lock(&cprman->regs_lock); >> > + cprman_write(cprman, data->a2w_ctrl_reg, >> > + cprman_read(cprman, data->a2w_ctrl_reg) & >> > + ~A2W_PLL_CTRL_PRST_DISABLE); >> > cprman_write(cprman, data->cm_ctrl_reg, >> > cprman_read(cprman, data->cm_ctrl_reg) | >> > CM_PLL_ANARST); >> >> For turning off, the FW just does the equivalent of: >> >> cprman_write(cprman, data->cm_ctrl_reg, CM_PLL_ANARST); >> cprman_write(cprman, data->a2w_ctrl_reg, A2W_PLL_CTRL_PWRDN); > > Hm, the write to ->a2w_ctrl_reg overwrites the > NDIV/PDIV values done in bcm2835_pll_set_rate(). So, either we do: > > cprman_write(cprman, data->cm_ctrl_reg, CM_PLL_ANARST); > cprman_write(cprman, data->a2w_ctrl_reg, > cprman_read(cprman, data->a2w_ctrl_reg) | > A2W_PLL_CTRL_PWRDN); > > or we cache the pdiv/ndiv values in struct bcm2835_pll and only apply > them in bcm2835_pll_on(). > > I'd recommend going for the former to keep the changes easily > backportable to older kernels. Oh, right. I like your cprman_write() solution above. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-02-08 13:43 [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions Boris Brezillon 2018-02-08 13:43 ` [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers Boris Brezillon 2018-02-08 13:43 ` [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate Boris Brezillon @ 2018-02-08 13:43 ` Boris Brezillon 2018-02-08 15:20 ` Eric Anholt 2018-03-19 16:29 ` Stephen Boyd 3 siblings, 1 reply; 25+ messages in thread From: Boris Brezillon @ 2018-02-08 13:43 UTC (permalink / raw) To: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, Eric Anholt, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk Cc: Boris Brezillon, stable 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: <stable@vger.kernel.org> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- 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); + 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) -- 2.14.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-02-08 13:43 ` [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate Boris Brezillon @ 2018-02-08 15:20 ` Eric Anholt 2018-02-08 17:56 ` Boris Brezillon 0 siblings, 1 reply; 25+ messages in thread From: Eric Anholt @ 2018-02-08 15:20 UTC (permalink / raw) To: Boris Brezillon, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk Cc: Boris Brezillon, stable [-- Attachment #1: Type: text/plain, Size: 2161 bytes --] Boris Brezillon <boris.brezillon@bootlin.com> 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: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > 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? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-02-08 15:20 ` Eric Anholt @ 2018-02-08 17:56 ` Boris Brezillon 2018-02-09 9:32 ` Eric Anholt 0 siblings, 1 reply; 25+ messages in thread From: Boris Brezillon @ 2018-02-08 17:56 UTC (permalink / raw) To: Eric Anholt Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable On Thu, 08 Feb 2018 15:20:16 +0000 Eric Anholt <eric@anholt.net> wrote: > Boris Brezillon <boris.brezillon@bootlin.com> 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: <stable@vger.kernel.org> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > 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? -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-02-08 17:56 ` Boris Brezillon @ 2018-02-09 9:32 ` Eric Anholt 2018-02-12 9:27 ` Boris Brezillon 0 siblings, 1 reply; 25+ messages in thread From: Eric Anholt @ 2018-02-09 9:32 UTC (permalink / raw) To: Boris Brezillon Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stephen Warren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable [-- Attachment #1: Type: text/plain, Size: 2787 bytes --] Boris Brezillon <boris.brezillon@bootlin.com> writes: > On Thu, 08 Feb 2018 15:20:16 +0000 > Eric Anholt <eric@anholt.net> wrote: > >> Boris Brezillon <boris.brezillon@bootlin.com> 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: <stable@vger.kernel.org> >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> > --- >> > 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-02-09 9:32 ` Eric Anholt @ 2018-02-12 9:27 ` Boris Brezillon 2018-03-12 21:21 ` Stephen Boyd 0 siblings, 1 reply; 25+ messages in thread From: Boris Brezillon @ 2018-02-12 9:27 UTC (permalink / raw) To: Eric Anholt Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stefan Wahren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable -Stephen Warren +Stefan Wahren On Fri, 09 Feb 2018 09:32:40 +0000 Eric Anholt <eric@anholt.net> wrote: > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > On Thu, 08 Feb 2018 15:20:16 +0000 > > Eric Anholt <eric@anholt.net> wrote: > > > >> Boris Brezillon <boris.brezillon@bootlin.com> 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: <stable@vger.kernel.org> > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > >> > --- > >> > 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. -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-02-12 9:27 ` Boris Brezillon @ 2018-03-12 21:21 ` Stephen Boyd 0 siblings, 0 replies; 25+ messages in thread From: Stephen Boyd @ 2018-03-12 21:21 UTC (permalink / raw) To: Boris Brezillon, Eric Anholt Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stefan Wahren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable Quoting Boris Brezillon (2018-02-12 01:27:52) > -Stephen Warren > +Stefan Wahren > > On Fri, 09 Feb 2018 09:32:40 +0000 > Eric Anholt <eric@anholt.net> wrote: > > > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > > > On Thu, 08 Feb 2018 15:20:16 +0000 > > > Eric Anholt <eric@anholt.net> wrote: > > > > > >> Boris Brezillon <boris.brezillon@bootlin.com> 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: <stable@vger.kernel.org> > > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > >> > --- > > >> > 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate @ 2018-03-12 21:21 ` Stephen Boyd 0 siblings, 0 replies; 25+ messages in thread From: Stephen Boyd @ 2018-03-12 21:21 UTC (permalink / raw) To: Boris Brezillon, Eric Anholt Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stefan Wahren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable Quoting Boris Brezillon (2018-02-12 01:27:52) > -Stephen Warren > +Stefan Wahren > = > On Fri, 09 Feb 2018 09:32:40 +0000 > Eric Anholt <eric@anholt.net> wrote: > = > > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > = > > > On Thu, 08 Feb 2018 15:20:16 +0000 > > > Eric Anholt <eric@anholt.net> wrote: > > > = > > >> Boris Brezillon <boris.brezillon@bootlin.com> 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 Li= nux > > >> > users. In our case, the clock might have been enabled by the > > >> > bootloader/FW, and, because we have CLK_IGNORE_UNUSED set, Linux n= ever > > >> > disables the PLL. So we have to make sure the PLL is actually disa= bled > > >> > before changing the rate. > > >> > > > >> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming th= e audio domain clocks") > > >> > Cc: <stable@vger.kernel.org> > > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > >> > --- > > >> > 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-b= cm2835.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 f= rom changing > > >> > + * the rate while the clock is enabled, but this check on= ly 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 b= efore 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 fr= om > > >> 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-03-12 21:21 ` Stephen Boyd (?) @ 2018-03-13 16:56 ` Eric Anholt 2018-03-19 16:26 ` Stephen Boyd -1 siblings, 1 reply; 25+ messages in thread From: Eric Anholt @ 2018-03-13 16:56 UTC (permalink / raw) To: Stephen Boyd, Boris Brezillon Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stefan Wahren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable [-- Attachment #1: Type: text/plain, Size: 5571 bytes --] Stephen Boyd <sboyd@kernel.org> 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 <eric@anholt.net> wrote: >> >> > Boris Brezillon <boris.brezillon@bootlin.com> writes: >> > >> > > On Thu, 08 Feb 2018 15:20:16 +0000 >> > > Eric Anholt <eric@anholt.net> wrote: >> > > >> > >> Boris Brezillon <boris.brezillon@bootlin.com> 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: <stable@vger.kernel.org> >> > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> > >> > --- >> > >> > 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-03-13 16:56 ` Eric Anholt @ 2018-03-19 16:26 ` Stephen Boyd 0 siblings, 0 replies; 25+ messages in thread From: Stephen Boyd @ 2018-03-19 16:26 UTC (permalink / raw) To: Boris Brezillon, Eric Anholt Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stefan Wahren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable Quoting Eric Anholt (2018-03-13 09:56:57) > Stephen Boyd <sboyd@kernel.org> 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate @ 2018-03-19 16:26 ` Stephen Boyd 0 siblings, 0 replies; 25+ messages in thread From: Stephen Boyd @ 2018-03-19 16:26 UTC (permalink / raw) To: Boris Brezillon, Eric Anholt Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stefan Wahren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable Quoting Eric Anholt (2018-03-13 09:56:57) > Stephen Boyd <sboyd@kernel.org> 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-03-12 21:21 ` Stephen Boyd (?) (?) @ 2018-03-22 9:13 ` Boris Brezillon 2018-03-22 16:38 ` Eric Anholt -1 siblings, 1 reply; 25+ messages in thread From: Boris Brezillon @ 2018-03-22 9:13 UTC (permalink / raw) To: Stephen Boyd Cc: Eric Anholt, Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stefan Wahren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable Hi Stephen, On Mon, 12 Mar 2018 14:21:22 -0700 Stephen Boyd <sboyd@kernel.org> 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 <eric@anholt.net> wrote: > > > > > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > > > > > On Thu, 08 Feb 2018 15:20:16 +0000 > > > > Eric Anholt <eric@anholt.net> wrote: > > > > > > > >> Boris Brezillon <boris.brezillon@bootlin.com> 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: <stable@vger.kernel.org> > > > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > >> > --- > > > >> > 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/ -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate 2018-03-22 9:13 ` Boris Brezillon @ 2018-03-22 16:38 ` Eric Anholt 0 siblings, 0 replies; 25+ messages in thread From: Eric Anholt @ 2018-03-22 16:38 UTC (permalink / raw) To: Boris Brezillon, Stephen Boyd Cc: Florian Fainelli, Ray Jui, Scott Branden, bcm-kernel-feedback-list, Stefan Wahren, Lee Jones, linux-rpi-kernel, Mike Turquette, Stephen Boyd, linux-clk, stable [-- Attachment #1: Type: text/plain, Size: 5869 bytes --] Boris Brezillon <boris.brezillon@bootlin.com> writes: > Hi Stephen, > > On Mon, 12 Mar 2018 14:21:22 -0700 > Stephen Boyd <sboyd@kernel.org> 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 <eric@anholt.net> wrote: >> > >> > > Boris Brezillon <boris.brezillon@bootlin.com> writes: >> > > >> > > > On Thu, 08 Feb 2018 15:20:16 +0000 >> > > > Eric Anholt <eric@anholt.net> wrote: >> > > > >> > > >> Boris Brezillon <boris.brezillon@bootlin.com> 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: <stable@vger.kernel.org> >> > > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> > > >> > --- >> > > >> > 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions 2018-02-08 13:43 [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions Boris Brezillon @ 2018-03-19 16:29 ` Stephen Boyd 2018-02-08 13:43 ` [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate Boris Brezillon ` (2 subsequent siblings) 3 siblings, 0 replies; 25+ messages in thread From: Stephen Boyd @ 2018-03-19 16:29 UTC (permalink / raw) To: Boris Brezillon, Eric Anholt, Florian Fainelli, Lee Jones, Mike Turquette, Ray Jui, Scott Branden, Stephen Boyd, Stephen Warren, bcm-kernel-feedback-list, linux-clk, linux-rpi-kernel Cc: Boris Brezillon, stable Quoting Boris Brezillon (2018-02-08 05:43:35) > ana->maskX values are already '~'-ed in bcm2835_pll_set_rate(). Remove > the '~' in the definition to fix ANA setup. > > Note that this commit fixes a long standing bug preventing one from > using an HDMI display if it's plugged after the FW has booted Linux. > This is because PLLH is used by the HDMI encoder to generate the pixel > clock. > > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") > Cc: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > Reviewed-by: Eric Anholt <eric@anholt.net> > --- Applied to clk-fixes ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions @ 2018-03-19 16:29 ` Stephen Boyd 0 siblings, 0 replies; 25+ messages in thread From: Stephen Boyd @ 2018-03-19 16:29 UTC (permalink / raw) To: Boris Brezillon, Eric Anholt, Florian Fainelli, Lee Jones, Mike Turquette, Ray Jui, Scott Branden, Stephen Boyd, Stephen Warren, bcm-kernel-feedback-list, linux-clk, linux-rpi-kernel Cc: Boris Brezillon, stable Quoting Boris Brezillon (2018-02-08 05:43:35) > ana->maskX values are already '~'-ed in bcm2835_pll_set_rate(). Remove > the '~' in the definition to fix ANA setup. > = > Note that this commit fixes a long standing bug preventing one from > using an HDMI display if it's plugged after the FW has booted Linux. > This is because PLLH is used by the HDMI encoder to generate the pixel > clock. > = > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio= domain clocks") > Cc: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > Reviewed-by: Eric Anholt <eric@anholt.net> > --- Applied to clk-fixes ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-03-22 16:38 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-08 13:43 [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions Boris Brezillon 2018-02-08 13:43 ` [PATCH 2/4] clk: bcm2835: Protect sections updating shared registers Boris Brezillon 2018-02-08 15:14 ` Eric Anholt 2018-02-09 9:34 ` Eric Anholt 2018-03-19 16:29 ` Stephen Boyd 2018-03-19 16:29 ` Stephen Boyd 2018-02-08 13:43 ` [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate Boris Brezillon 2018-02-08 15:15 ` Eric Anholt 2018-02-08 17:49 ` Boris Brezillon 2018-02-14 10:37 ` Boris Brezillon 2018-02-22 20:18 ` Eric Anholt 2018-02-08 13:43 ` [PATCH 4/4] clk: bcm2835: Make sure the PLL is gated before changing its rate Boris Brezillon 2018-02-08 15:20 ` Eric Anholt 2018-02-08 17:56 ` Boris Brezillon 2018-02-09 9:32 ` Eric Anholt 2018-02-12 9:27 ` Boris Brezillon 2018-03-12 21:21 ` Stephen Boyd 2018-03-12 21:21 ` Stephen Boyd 2018-03-13 16:56 ` Eric Anholt 2018-03-19 16:26 ` Stephen Boyd 2018-03-19 16:26 ` Stephen Boyd 2018-03-22 9:13 ` Boris Brezillon 2018-03-22 16:38 ` Eric Anholt 2018-03-19 16:29 ` [PATCH 1/4] clk: bcm2835: Fix ana->maskX definitions Stephen Boyd 2018-03-19 16:29 ` Stephen Boyd
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.