Re: [PATCH 6.12 0139/1276] arm64: dts: qcom: sc7180: Add power-domain and iface clk for ice node

Hi Harshit,

Apologies for the late response, I am currently on vacation.

On 24-07-2026 05:50 pm, Harshit Mogalapalli wrote:
> Hi Greg/Sasha,
> 
> On 21/07/26 8:39 pm, Greg Kroah-Hartman wrote:
>> 6.12-stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Harshal Dev <[email protected]>
>>
>> [ Upstream commit 7cd7271ac525e4eadd22734f418219f247638f43 ]
>>
>> Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
>> for its own resources. Before accessing ICE hardware during probe, to
>> avoid potential unclocked register access issues (when clk_ignore_unused
>> is not passed on the kernel command line), in addition to the 'core' clock
>> the 'iface' clock should also be turned on by the driver. This can only be
>> done if the UFS_PHY_GDSC power domain is enabled. Specify both the
>> UFS_PHY_GDSC power domain and the 'iface' clock in the ICE node for sc7180.
>>
>> Fixes: 858536d9dc946 ("arm64: dts: qcom: sc7180: Add UFS nodes")
>> Reviewed-by: Konrad Dybcio <[email protected]>
>> Reviewed-by: Kuldeep Singh <[email protected]>
>> Signed-off-by: Harshal Dev <[email protected]>
>> Link: https://lore.kernel.org/r/20260416-qcom_ice_power_and_clk_vote-v5-6-5ccf5d7e2846@oss.qualcomm.com
>> Signed-off-by: Bjorn Andersson <[email protected]>
>> Signed-off-by: Sasha Levin <[email protected]>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7180.dtsi | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 6ae5ca00c7187a..e52d938df1bece 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -1601,7 +1601,11 @@ ice: crypto@1d90000 {
>>               compatible = "qcom,sc7180-inline-crypto-engine",
>>                        "qcom,inline-crypto-engine";
>>               reg = <0 0x01d90000 0 0x8000>;
>> -            clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>> +            clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
>> +                 <&gcc GCC_UFS_PHY_AHB_CLK>;
>> +            clock-names = "core",
>> +                      "iface";
>> +            power-domains = <&gcc UFS_PHY_GDSC>;
>>           };
> I ran an AI-assisted backport review and what it found is likely worth noting:
> 
> I reviewed these four 6.12.y commits and checked the current 6.12 ICE driver and binding: Patches 139-142:
> 
> - e49e61994f4d ("arm64: dts: qcom: sc7180: Add power-domain and iface clk for ice node")
> - 4960fae719ad ("arm64: dts: qcom: kodiak: Add power-domain and iface clk for ice node")
> - bac1108e0040 ("arm64: dts: qcom: sm8450: Add power-domain and iface clk for ice node")
> - c4a4e6c6255d ("arm64: dts: qcom: sm8650: Add power-domain and iface clk for ice node")
> 
> I may be missing something, so I wanted to sanity-check my reading.
> 
> These DTS patches add a second clock named "iface" and also add
> power-domains to the ICE node.
> 
> Upstream commit 0d5dc5818191 updates the driver to request both clocks:
> 
> if (!engine->core_clk)
>         engine->core_clk = devm_clk_get_optional_enabled(dev, "core");
> 
> engine->iface_clk = devm_clk_get_optional_enabled(dev, "iface");
> 
> But 6.12.y still only requests one clock:
> 
> engine->core_clk = devm_clk_get_optional_enabled(dev, "ice_core_clk");
> if (!engine->core_clk)
>         engine->core_clk = devm_clk_get_optional_enabled(dev, "ice");
> if (!engine->core_clk)
>         engine->core_clk = devm_clk_get_enabled(dev, NULL);
> 
> if (!qcom_ice_check_supported(engine))
>         return ERR_PTR(-EOPNOTSUPP);
> 
> My understanding is that these DTS patches add a second clock to the ICE node, but the 6.12.y ICE driver still enables only one clock from that node.
> 
> If so, the new "iface" clock on the ICE node would never be requested on
> 6.12.y, and ICE could still touch registers during probe and resume with only the "core" clock enabled. Is that the right reading?
>

Yes this analysis is correct. I can see Greg already picked this patch:
https://lore.kernel.org/all/[email protected]/
 
> Separately, the 6.12 binding still seems to have clocks: maxItems: 1 and
> does not describe clock-names or power-domains.
>

It doesn't right now, yes.

> If that reading is right, should 6.12.y take adapted versions of these two commits before the four DTS patches?
> 
> - 0d5dc5818191 ("soc: qcom: ice: Allow explicit votes on 'iface' clock for ICE")
> - e27264daac7d ("dt-bindings: crypto: qcom,ice: Fix missing power-domain and iface clk")
>

The 1st can be taken on-top of the four DTS, since describing the clocks first but enabling them
later via the commit is totally fine.

The 2nd one should be taken to make the DTS patches comply with an updated binding. The binding is
written in a backward compatible way so it will not break any out of tree DTS still relying on
the old binding. However, the patch must be adapted to 6.12 to not make the power-domain and
clock-names mandatory for Eliza and Milos since those targets arrived in 7.1 kernel. Only the first
portion of the patch is applicable for 6.12.

Regards,
Harshal 
> Aslo the patches are from a series which includes the above two, so should we either queue the above two or drop the 4 patches for now ?
> 
> Thanks,
> Harshit
> 
> 
>>             ipa: ipa@1e40000 {
>