Re: [PATCH 2/3] clk: pistachio: Lock the PLL when enabled upon rate change

[Stephen Boyd <sboyd@codeaurora.org>]

On 05/26, Ezequiel Garcia wrote:
> Currently, when the rate is changed, the driver makes sure the
> PLL is enabled before doing so. This is done because the PLL
> cannot be locked while disabled. Once locked, the drivers
> returns the PLL to its previous enable/disable state.
> 
> This is a bit cumbersome, and can be simplified.
> 
> This commit reworks the .set_rate() functions for the integer
> and fractional PLLs. Upon rate change, the PLL is now locked
> only if it's already enabled.
> 
> Also, the driver locks the PLL on .enable(). This makes sure
> the PLL is locked when enabled, and not locked when disabled.
> 
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> ---
>  drivers/clk/pistachio/clk-pll.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/clk/pistachio/clk-pll.c b/drivers/clk/pistachio/clk-pll.c
> index 9ce1be7..f12d520 100644
> --- a/drivers/clk/pistachio/clk-pll.c
> +++ b/drivers/clk/pistachio/clk-pll.c
> @@ -130,6 +130,8 @@ static int pll_gf40lp_frac_enable(struct clk_hw *hw)
>  	val &= ~PLL_FRAC_CTRL4_BYPASS;
>  	pll_writel(pll, val, PLL_CTRL4);
>  
> +	pll_lock(pll);
> +
>  	return 0;
>  }
>  
> @@ -155,17 +157,13 @@ static int pll_gf40lp_frac_set_rate(struct clk_hw *hw, unsigned long rate,
>  {
>  	struct pistachio_clk_pll *pll = to_pistachio_pll(hw);
>  	struct pistachio_pll_rate_table *params;
> -	bool was_enabled;
> +	int enabled = pll_gf40lp_frac_is_enabled(hw);

Is there any sort of spinlock here so that we protect the
sleeping set_rate() path against the non-sleeping enable/disable
path?  There should be a spinlock of some kind to prevent that,
or the enable/disable for the PLL should move to
prepare/unprepare so that we can't disable the PLL in the middle
of a rate switch.

This is an existing problem though, so I applied this to clk-next
anyway.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project