Re: [PATCH 3/4] clk: bcm2835: De-assert/assert PLL reset signal when appropriate

[Eric Anholt <eric@anholt.net>]

[-- 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 --]