Re: [PATCH v3 04/12] clk: qcom: ipq-cmn-pll: Register CMN PLL /2 clock

[Jie Luo <jie.luo@oss.qualcomm.com>]

On 9/3/2026 9:18 PM, Abel Vesa wrote:
> On 26-08-14 05:48:14, Luo Jie wrote:
>> Most CMN PLL output clocks are derived from the 12 GHz PLL output
>> through a shared, physical /2 stage, ahead of any further per-clock
>> processing: a gated fixed rate (PCS/ETH0/ETH1/ETH2/EPHY/ETH), a
>> configurable divider (NSS/PPE/PON), or a rate-select bit (EPHY-RAW).
>> This /2 stage is present in the CMN PLL block on every SoC supported
>> by this driver, not just IPQ5210.
>>
>> Register this /2 stage once as a fixed-factor clock, "cmn_pll_div2",
>> and store it on struct clk_cmn_pll so the output clock types added by
>> later patches can parent on it. The two plain fixed-rate outputs,
>> xo-24mhz and sleep-32khz, are the exception on IPQ5210 SoC: they stay
>> parented directly on the main PLL clock.
>>
>> On SoCs other than IPQ5210, every output clock is a plain fixed-rate
>> clock (CMN_PLL_CLK_FIXED_RATE) whose rate is a hardcoded absolute
>> value rather than one computed from the parent rate, so this /2 clock
>> is registered there but currently has no consumers; IPQ5210 is simply
>> the first SoC in this driver whose output clocks are modeled as
>> depending on parent_rate, which is what makes the /2 stage significant.
>>
>> This patch only registers the clock; later patches wire it in as the
>> effective parent for the output clock types that sit downstream of it.
> 
> Do not mention future patches here. Each patch has to stand by itself.
> 

I will remove the reference to later patches and ensure that this patch
is self-contained.

>>
>> Signed-off-by: Luo Jie <jie.luo@oss.qualcomm.com>
>> ---
>>  drivers/clk/qcom/ipq-cmn-pll.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
>> index 4fdf78f6829e..bcb51a5f559a 100644
>> --- a/drivers/clk/qcom/ipq-cmn-pll.c
>> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
>> @@ -96,10 +96,17 @@ struct cmn_pll_fixed_output_clk {
>>   * struct clk_cmn_pll - CMN PLL hardware specific data
>>   * @regmap: hardware regmap.
>>   * @hw: handle between common and hardware-specific interfaces
>> + * @div2_hw: fixed /2 clock derived from the CMN PLL output; present on
>> + *           every supported SoC, but only IPQ5210 currently parents
>> + *           any output clock on it (every output clock except the
>> + *           plain fixed-rate xo/sleep clocks, which stay on the main
>> + *           PLL); other SoCs' output clocks use hardcoded rates that
>> + *           never depend on a parent
>>   */
>>  struct clk_cmn_pll {
>>  	struct regmap *regmap;
>>  	struct clk_hw hw;
>> +	struct clk_hw *div2_hw;
>>  };
>>  
>>  #define CLK_PLL_OUTPUT(_id, _name, _rate) {		\
>> @@ -362,6 +369,7 @@ static int ipq_cmn_pll_register_clks(struct platform_device *pdev)
>>  	const struct cmn_pll_fixed_output_clk *p, *fixed_clk;
>>  	struct clk_hw_onecell_data *hw_data;
>>  	struct device *dev = &pdev->dev;
>> +	struct clk_cmn_pll *cmn_pll;
>>  	struct clk_hw *cmn_pll_hw;
>>  	unsigned int num_clks;
>>  	struct clk_hw *hw;
>> @@ -388,6 +396,20 @@ static int ipq_cmn_pll_register_clks(struct platform_device *pdev)
>>  	if (IS_ERR(cmn_pll_hw))
>>  		return PTR_ERR(cmn_pll_hw);
>>  
>> +	cmn_pll = to_clk_cmn_pll(cmn_pll_hw);
>> +
>> +	/*
>> +	 * The CMN PLL output feeds a shared, physical /2 stage ahead of
>> +	 * any further per-clock processing (a gated fixed rate, a
>> +	 * configurable divider, or a rate-select bit). Register it once
>> +	 * as a fixed-factor clock so the output clock types added by
>> +	 * later patches can parent on it.
> 
> The comment here should only say why this is needed. Do not mention
> future patches.
> 

Understood, thanks.

>> +	 */
>> +	cmn_pll->div2_hw = devm_clk_hw_register_fixed_factor_parent_hw(dev, "cmn_pll_div2",
>> +								       cmn_pll_hw, 0, 1, 2);
>> +	if (IS_ERR(cmn_pll->div2_hw))
>> +		return PTR_ERR(cmn_pll->div2_hw);
> 
> In fact, I think this clock registering is self explanatory as is, so you
> should drop the comment above it.
> 

Agreed. I will drop the comment above the clock registration.

>> +
>>  	/* Register the fixed rate output clocks. */
>>  	for (i = 0; i < num_clks; i++) {
>>  		struct clk_parent_data pdata = { .hw = cmn_pll_hw };
>>
>> -- 
>> 2.43.0
>>