Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Harshal Dev <harshal.dev@oss.qualcomm.com>]

Hi Konrad,

On 1/30/2026 4:16 PM, Konrad Dybcio wrote:
> On 1/23/26 12:12 PM, Harshal Dev wrote:
>> Hi Krzysztof,
>>
>> On 1/23/2026 4:27 PM, Krzysztof Kozlowski wrote:
>>> On 23/01/2026 09:58, Krzysztof Kozlowski wrote:
>>>>>  
>>>>>  	return 0;
>>>>> @@ -584,6 +597,10 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>>>>>  	if (IS_ERR(engine->core_clk))
>>>>>  		return ERR_CAST(engine->core_clk);
>>>>>  
>>>>> +	engine->iface_clk = devm_clk_get_enabled(dev, "iface_clk");
>>>>> +	if (IS_ERR(engine->iface_clk))
>>>>> +		return ERR_CAST(engine->iface_clk);
>>>>
>>>> And here actual breakage of ALL in-tree and out-of-tree DTS.
>>>>
>>>> NAK.
>>>>
>>>> Please read internal guideline.
>>>
>>> Internal docs are pretty scattered and messy so I failed to find this
>>> there, which is surprising because this was frequent feedback. Therefore
>>> please update Kernel Upstreaming internal page with following:
>>>
>>> With few exceptions, it is not allowed to break the ABI, by making
>>> bindings or driver changes, where the existing or out of tree DTS would
>>> fail to boot. Updating in-tree DTS does not matter here, because DTS
>>> goes via different branch, thus driver branch would be always broken.
>>> This is explicitly documented in DT rules and explained also in
>>> maintainer-soc profile.
>>>
>>> You need to either provide strong justification for ABI break or make
>>> the changes backwards compatible.
> 
> If the ICE can not be powered on alone without this change (i.e. no UFS,
> just ICE), then please spell it out explicitly, Harshal. That makes for a
> valid reason to break the ABI.
> 
> Plus the fact that without an OPP table, the voltage requirements cannot
> be guaranteed to be met
> 

Ack, I have endorsed and stated this point on the DT-binding commit.
I'll wait for Krzysztof's view before updating the commit message to
strongly reflect this point.

>>
>> Ack and understood. Let me write this in a way that makes it backward
>> compatible by using devm_clk_get_optional_enabled(). Like I explained, for
>> Linux distros where CONFIG_SCSI_UFS_QCOM is override set to 'y'. This
>> clock vote isn't really needed during probe.
> 
> This is really a side-effect that we shouldn't be depending on, or
> even considering as a backup, since the UFS driver may change
> independently and stop behaving this way one day
> 
>> In qcom_ice_suspend/resume(). I'll only prepare/un-prepare this clock
>> if it was found during probe.
> 
> Clock APIs generally happily eat nullptrs

Ack, then I guess in either case we can continue to keep the calls to
prepare/un-prepare. Anyways, I am convinced we should specify and use the
'iface' clock.

Thanks,
Harshal

> 
> Konrad