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

[Abel Vesa]

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.

> 
> Signed-off-by: Luo Jie <jie.luo@xxxxxxxxxxxxxxxx>
> ---
>  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.

> +	 */
> +	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.

> +
>  	/* 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
>