Re: [PATCH 2/5] clk: qcom: clk-alpha-pll: Add support for Regera PLL ops

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 29.02.2024 06:38, Satya Priya Kakitapalli wrote:
> From: Taniya Das <quic_tdas@quicinc.com>
> 
> Regera PLL ops are required to control the Regera PLL from clock
> controller drivers, thus add support for the same.
> 
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---

[...]


> +static int clk_regera_pll_enable(struct clk_hw *hw)

This function is 1:1 clk_zonda_pll_enable() logic-wise, except for
the `if (val & ZONDA_STAY_IN_CFA)` part. Would it be an issue on
Regera?

> +static void clk_regera_pll_disable(struct clk_hw *hw)

This again is clk_zonda_pll_disable(), except the very last value written
to PLL_OPMODE is different. Could you commonize them?


> +
> +static void zonda_pll_adjust_l_val(unsigned long rate, unsigned long prate,
> +									u32 *l)

(Ugly wrapping, please touch it up)

..should it apply to zonda as the name suggests? Also, any explanations?

> +	u64 remainder, quotient;
> +
> +	quotient = rate;
> +	remainder = do_div(quotient, prate);
> +	*l = quotient;
> +
> +	if ((remainder * 2) / prate)
> +		*l = *l + 1;
> +}
> +
> +static int clk_regera_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long prate)
> +{
> +	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> +	unsigned long rrate;
> +	u32 l, alpha_width = pll_alpha_width(pll);
> +	u64 a;
> +	int ret;
> +
> +	rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width);
> +
> +	ret = alpha_pll_check_rate_margin(hw, rrate, rate);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (a && (a & BIT(15)))

What is BIT(15)?

Also, the left part of the condition is totally bogus, if a is 0 then
a & BIT(15) will surely be zero as well.

Konrad