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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <DS7PR19MB88831E73F6787E04E0C43AB49D8D2@DS7PR19MB8883.namprd19.prod.outlook.com>
Date: Fri, 2 May 2025 17:04:59 +0400
From: George Moussalem <george.moussalem@...look.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Luo Jie <quic_luoj@...cinc.com>,
 Lee Jones <lee@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>
Cc: linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] clk: qcom: ipq-cmn-pll: Add IPQ5018 SoC support



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@...look.com>
>>
>> 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@...look.com>
>> ---
> 
> [...]
> 
>> +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