Re: [PATCH v3 04/18] clk: qcom: clk-alpha-pll: Add support for common PLL configuration function

[Jagadeesh Kona]

On 3/27/2025 11:50 PM, Dmitry Baryshkov wrote:
> On Thu, Mar 27, 2025 at 03:51:33PM +0000, Bryan O'Donoghue wrote:
>> On 27/03/2025 09:52, Jagadeesh Kona wrote:
>>> From: Taniya Das <quic_tdas@xxxxxxxxxxx>
>>>
>>> To properly configure the PLLs on recent chipsets, it often requires more
>>> than one power domain to be kept ON. The support to enable multiple power
>>> domains is being added in qcom_cc_really_probe() and PLLs should be
>>> configured post all the required power domains are enabled.
>>>
>>> Hence integrate PLL configuration into clk_alpha_pll structure and add
>>> support for qcom_clk_alpha_pll_configure() function which can be called
>>> from qcom_cc_really_probe() to configure the clock controller PLLs after
>>> all required power domains are enabled.
>>>
>>> Signed-off-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
>>> Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
>>> ---
>>>   drivers/clk/qcom/clk-alpha-pll.c | 63 ++++++++++++++++++++++++++++++++++++++++
>>>   drivers/clk/qcom/clk-alpha-pll.h |  3 ++
>>>   2 files changed, 66 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>>> index cec0afea8e446010f0d4140d4ef63121706dde47..8ee842254e6690e24469053cdbd99a9953987e40 100644
>>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>>> @@ -63,6 +63,8 @@
>>>   #define PLL_OPMODE(p)		((p)->offset + (p)->regs[PLL_OFF_OPMODE])
>>>   #define PLL_FRAC(p)		((p)->offset + (p)->regs[PLL_OFF_FRAC])
>>> +#define GET_PLL_TYPE(pll)	(((pll)->regs - clk_alpha_pll_regs[0]) / PLL_OFF_MAX_REGS)
>>> +
>>>   const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
>>>   	[CLK_ALPHA_PLL_TYPE_DEFAULT] =  {
>>>   		[PLL_OFF_L_VAL] = 0x04,
>>> @@ -2960,3 +2962,64 @@ const struct clk_ops clk_alpha_pll_regera_ops = {
>>>   	.set_rate = clk_zonda_pll_set_rate,
>>>   };
>>>   EXPORT_SYMBOL_GPL(clk_alpha_pll_regera_ops);
>>> +
>>> +void qcom_clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap)
>>> +{
>>> +	const struct clk_init_data *init = pll->clkr.hw.init;
>>> +	const char *name = init->name;
>>> +
>>> +	if (!pll->config || !pll->regs) {
>>> +		pr_err("%s: missing pll config or regs\n", name);
>>> +		return;
>>> +	}
>>
>> Seems like a strange check - you are calling this function in a loop which
>> looks like
>>
>> for (i = 0; i < desc->num_alpha_plls; i++)
>> 	qcom_clk_alpha_pll_configure(desc->alpha_plls[i], regmap);
>>
>> Can num_alpha_plls be true but alpha_plls be NULL and why is regmap
>> considered valid ?
>>
>> I think you can drop this check.
>

The regmap check is already performed in qcom_cc_map() before we reach this
point.

It is not possible for num_alpha_plls to be true when alpha_plls is NULL, as
num_alpha_plls is derived using SIZE_OF(alpha_plls).

Including the above check is beneficial to catch any errors in case we missed
adding the necessary config/regs fields to the PLL structure in the CC code.
 
> I think pll->config should be moved to a calling code.
>
Yes, I can move above checks to calling code in next series. 

Thanks,
Jagadeesh

>>
>>> +
>>> +	switch (GET_PLL_TYPE(pll)) {
>>> +	case CLK_ALPHA_PLL_TYPE_LUCID_OLE:
>>> +		clk_lucid_ole_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_LUCID_EVO:
>>> +		clk_lucid_evo_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_TAYCAN_ELU:
>>> +		clk_taycan_elu_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_RIVIAN_EVO:
>>> +		clk_rivian_evo_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_TRION:
>>> +		clk_trion_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_HUAYRA_2290:
>>> +		clk_huayra_2290_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_FABIA:
>>> +		clk_fabia_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_AGERA:
>>> +		clk_agera_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_PONGO_ELU:
>>> +		clk_pongo_elu_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_ZONDA:
>>> +	case CLK_ALPHA_PLL_TYPE_ZONDA_OLE:
>>> +		clk_zonda_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_STROMER:
>>> +	case CLK_ALPHA_PLL_TYPE_STROMER_PLUS:
>>> +		clk_stromer_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	case CLK_ALPHA_PLL_TYPE_DEFAULT:
>>> +	case CLK_ALPHA_PLL_TYPE_DEFAULT_EVO:
>>> +	case CLK_ALPHA_PLL_TYPE_HUAYRA:
>>> +	case CLK_ALPHA_PLL_TYPE_HUAYRA_APSS:
>>> +	case CLK_ALPHA_PLL_TYPE_BRAMMO:
>>> +	case CLK_ALPHA_PLL_TYPE_BRAMMO_EVO:
>>> +		clk_alpha_pll_configure(pll, regmap, pll->config);
>>> +		break;
>>> +	default:
>>> +		WARN(1, "%s: invalid pll type\n", name);
>>> +		break;
>>> +	}
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_clk_alpha_pll_configure);
>>> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
>>> index 79aca8525262211ae5295245427d4540abf1e09a..7f35aaa7a35d88411beb11fd2be5d5dd5bfbe066 100644
>>> --- a/drivers/clk/qcom/clk-alpha-pll.h
>>> +++ b/drivers/clk/qcom/clk-alpha-pll.h
>>> @@ -81,6 +81,7 @@ struct pll_vco {
>>>    * struct clk_alpha_pll - phase locked loop (PLL)
>>>    * @offset: base address of registers
>>>    * @regs: alpha pll register map (see @clk_alpha_pll_regs)
>>> + * @config: array of pll settings
>>>    * @vco_table: array of VCO settings
>>>    * @num_vco: number of VCO settings in @vco_table
>>>    * @flags: bitmask to indicate features supported by the hardware
>>> @@ -90,6 +91,7 @@ struct clk_alpha_pll {
>>>   	u32 offset;
>>>   	const u8 *regs;
>>> +	const struct alpha_pll_config *config;
>>>   	const struct pll_vco *vco_table;
>>>   	size_t num_vco;
>>>   #define SUPPORTS_OFFLINE_REQ		BIT(0)
>>> @@ -237,5 +239,6 @@ void clk_stromer_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>>   			       const struct alpha_pll_config *config);
>>>   void clk_regera_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>>   			     const struct alpha_pll_config *config);
>>> +void qcom_clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap);
>>>   #endif
>>>
>