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