| Message ID | 20180208134338.24590-3-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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.
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.
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 <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.
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; }
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(+)