Re: [PATCH 3/6] clk: qcom: ipq-cmn-pll: Add IPQ5018 SoC support

[George Moussalem]

On 5/2/25 14:38, Konrad Dybcio wrote:
> On 5/2/25 12:15 PM, George Moussalem via B4 Relay wrote:
> > From: George Moussalem <george.moussalem@xxxxxxxxxxx>
> >
> > The CMN PLL in IPQ5018 SoC supplies fixed clocks to XO, sleep, and the
> > ethernet block. The CMN PLL to the ethernet block must be enabled first
> > by setting a specific register in the TCSR area set in the device tree.
> >
> > Signed-off-by: George Moussalem <george.moussalem@xxxxxxxxxxx>
> > ---
>
> [...]
>
> > +static inline int ipq_cmn_pll_eth_enable(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	unsigned int cmn_pll_offset;
> > +	struct regmap *tcsr;
> > +	int ret;
> > +
> > +	tcsr = syscon_regmap_lookup_by_phandle_args(dev->of_node, "qcom,cmn-pll-eth-enable",
> > +						    1, &cmn_pll_offset);
>
> So we have syscon_regmap_lookup_by_phandle_args() and
> syscon_regmap_lookup_by_phandle_optional(), but we could also
> use a syscon_regmap_lookup_by_phandle_args_optional() - could
> you add that in drivers/mfd/syscon.c?

Yeah, sounds like a good plan. This was basically doing the same but it 
would be better to add it to the syscon driver.

> > +	if (IS_ERR(tcsr)) {
> > +		ret = PTR_ERR(tcsr);
> > +		/*
> > +		 * continue if -ENODEV is returned as not all IPQ SoCs
> > +		 * need to enable CMN PLL. If it's another error, return it.
> > +		 */
> > +		if (ret == -ENODEV)
> > +			tcsr = NULL;
> > +		else
> > +			return ret;
> > +	}
> > +
> > +	if (tcsr) {
> > +		ret = regmap_update_bits(tcsr, cmn_pll_offset + TCSR_CMN_PLL_ETH,
>
> I think it's better to just pass the exact register that we need,
> instead of some loosely defined subregion - especially given the
> structure likely will change across platforms

Will do.

> > +					 TCSR_CMN_PLL_ETH_ENABLE, TCSR_CMN_PLL_ETH_ENABLE);
>
> regmap_set_bits()

Will do.

> > +		if (ret)
> > +			return ret;
>
> You can initialize ret to 0 and return ret below, unconditionally
>
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   static int ipq_cmn_pll_clk_probe(struct platform_device *pdev)
> >   {
> >   	struct device *dev = &pdev->dev;
> >   	int ret;
> >   
> > +	ret = ipq_cmn_pll_eth_enable(pdev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Fail to enable CMN PLL to ethernet");
>
> Fail*ed*

Will fix the spelling.

> Konrad

Thanks,
George