Re: [PATCH v2 05/12] clk: qcom: ipq-cmn-pll: Add NSS clock support

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The NSS clock is unconditionally registered as a fixed-rate clock, meaning the new divider registration logic is never invoked.
- [Medium] A kernel WARN will trigger during boot if the 6-bit hardware register happens to be 0.
--

--- Patch [5]: [PATCH v2 05/12] clk: qcom: ipq-cmn-pll: Add NSS clock support ---
commit 4b97f2d301b3cfbdff0e9b058b5b63be9ee7d0dd
Author: Luo Jie <[email protected]>

clk: qcom: ipq-cmn-pll: Add NSS clock support

This patch adds NSS clock support by introducing a new clock registration
type and divider logic. It adds ipq_cmn_pll_regmap_div_register() to handle
the NSS clock divider and updates the clk_cmn_pll structures to specify the
clock type.

> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
> index 4fdf78f6829e8..c1b00699eb72d 100644
> --- a/drivers/clk/qcom/ipq-cmn-pll.c
> +++ b/drivers/clk/qcom/ipq-cmn-pll.c

[ ... ]

> @@ -105,6 +123,7 @@ struct clk_cmn_pll {
>  #define CLK_PLL_OUTPUT(_id, _name, _rate) {		\
>  	.id =		_id,				\
>  	.name =		_name,				\
> +	.type =		CMN_PLL_CLK_FIXED_RATE,		\
>  	.rate =		_rate,				\
>  }

[Severity: Medium]
Since CLK_PLL_OUTPUT unconditionally assigns CMN_PLL_CLK_FIXED_RATE, and no
other macro is introduced to set CMN_PLL_CLK_NSS, will the NSS clock
actually use the new registration type?

It seems the switch statement in ipq_cmn_pll_register_clks() will only ever
evaluate to CMN_PLL_CLK_FIXED_RATE, leaving the new divider registration logic
as dead code.

[ ... ]

> @@ -357,11 +376,46 @@ static struct clk_hw *ipq_cmn_pll_clk_hw_register(struct platform_device *pdev)
>  	return &cmn_pll->hw;
>  }
>  
> +static struct clk_hw *ipq_cmn_pll_regmap_div_register(struct platform_device *pdev,
> +						      struct regmap *regmap,
> +						      struct clk_hw *cmn_pll_hw,
> +						      const char *name,
> +						      u32 field_mask)
> +{
> +	struct clk_parent_data pdata = { .hw = cmn_pll_hw };
> +	struct device *dev = &pdev->dev;
> +	struct clk_regmap_div *div_clk;
> +	int ret;
> +
> +	div_clk = devm_kzalloc(dev, sizeof(*div_clk), GFP_KERNEL);
> +	if (!div_clk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	div_clk->reg = CMN_PLL_NSS_PPE_FREQ_CTRL;
> +	div_clk->shift = __ffs(field_mask);
> +	div_clk->width = hweight32(field_mask);
> +	div_clk->flags = CLK_DIVIDER_EVEN_INTEGERS_NO_OFFSET;

[Severity: Medium]
Will this trigger a generic clock framework WARN during boot if the
hardware register value happens to be 0?

Since the full 6-bit register range is now allowed, if the bootloader or
hardware leaves the register at 0, clk_regmap_div_ops.recalc_rate will
evaluate a divisor of 0. Without CLK_DIVIDER_ALLOW_ZERO included in the flags,
divider_recalc_rate() will trigger a WARN.

> +	div_clk->clkr.regmap = regmap;
> +	div_clk->clkr.hw.init = &(struct clk_init_data){
> +		.name = name,
> +		.parent_data = &pdata,
> +		.num_parents = 1,
> +		.ops = &clk_regmap_div_ops,
> +	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5