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

From: Konrad Dybcio <konrad.dybcio@linaro.org>
To: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>,
	Bjorn Andersson <andersson@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Abhishek Sahu <absahu@codeaurora.org>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>,
	linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Ajit Pandey <quic_ajipan@quicinc.com>,
	Imran Shaik <quic_imrashai@quicinc.com>,
	Taniya Das <quic_tdas@quicinc.com>,
	Jagadeesh Kona <quic_jkona@quicinc.com>
Subject: Re: [PATCH 2/5] clk: qcom: clk-alpha-pll: Add support for Regera PLL ops
Date: Sat, 2 Mar 2024 00:56:11 +0100	[thread overview]
Message-ID: <630bb10a-2197-4573-a6d5-01fa6650c315@linaro.org> (raw)
In-Reply-To: <20240229-camcc-support-sm8150-v1-2-8c28c6c87990@quicinc.com>

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