* [PATCH v2 1/3] dt-bindings: crypto: qcom,ice: Add sa8255p support
2026-05-12 3:37 [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource Linlin Zhang
@ 2026-05-12 3:37 ` Linlin Zhang
2026-05-14 12:55 ` Krzysztof Kozlowski
2026-05-12 3:37 ` [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver Linlin Zhang
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Linlin Zhang @ 2026-05-12 3:37 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel
On sa8255p, resources such as PHY, clocks, regulators, and resets are
managed by remote firmware via the SCMI power protocol. As a result, the
ICE driver cannot directly access clocks and must instead use power-domains
to request resource configuration.
Add the qcom,sa8255p-inline-crypto-engine compatible string and make clocks
optional for platforms that use power-domains instead.
Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
---
.../crypto/qcom,inline-crypto-engine.yaml | 27 ++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
index 876bf90ed96e..4e7d9111d0eb 100644
--- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
+++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
@@ -17,6 +17,7 @@ properties:
- qcom,kaanapali-inline-crypto-engine
- qcom,milos-inline-crypto-engine
- qcom,qcs8300-inline-crypto-engine
+ - qcom,sa8255p-inline-crypto-engine
- qcom,sa8775p-inline-crypto-engine
- qcom,sc7180-inline-crypto-engine
- qcom,sc7280-inline-crypto-engine
@@ -32,6 +33,9 @@ properties:
clocks:
maxItems: 1
+ power-domains:
+ maxItems: 1
+
operating-points-v2: true
opp-table:
@@ -40,7 +44,20 @@ properties:
required:
- compatible
- reg
- - clocks
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sa8255p-inline-crypto-engine
+ then:
+ required:
+ - power-domains
+ else:
+ required:
+ - clocks
additionalProperties: false
@@ -75,4 +92,12 @@ examples:
};
};
};
+
+ - |
+ crypto@1d88000 {
+ compatible = "qcom,sa8255p-inline-crypto-engine",
+ "qcom,inline-crypto-engine";
+ reg = <0x01d88000 0x8000>;
+ power-domains = <&scmi26_pd 0>;
+ };
...
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 1/3] dt-bindings: crypto: qcom,ice: Add sa8255p support
2026-05-12 3:37 ` [PATCH v2 1/3] dt-bindings: crypto: qcom,ice: Add sa8255p support Linlin Zhang
@ 2026-05-14 12:55 ` Krzysztof Kozlowski
2026-05-15 13:23 ` Linlin Zhang
0 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-05-14 12:55 UTC (permalink / raw)
To: Linlin Zhang
Cc: Rob Herring, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel
On Mon, May 11, 2026 at 08:37:48PM -0700, Linlin Zhang wrote:
> On sa8255p, resources such as PHY, clocks, regulators, and resets are
> managed by remote firmware via the SCMI power protocol. As a result, the
> ICE driver cannot directly access clocks and must instead use power-domains
> to request resource configuration.
Then how can it be compatible with qcom,inline-crypto-engine?
>
> Add the qcom,sa8255p-inline-crypto-engine compatible string and make clocks
> optional for platforms that use power-domains instead.
>
> Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
> ---
> .../crypto/qcom,inline-crypto-engine.yaml | 27 ++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
So this is v2? But previous was v6? Look:
b4 diff '20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com'
Looking up https://lore.kernel.org/all/20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com/
Grabbing thread from lore.kernel.org/all/20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com/t.mbox.gz
Checking for older revisions
Grabbing search results from lore.kernel.org
---
Analyzing 8 messages in the thread
Could not find lower series to compare against.
>
> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> index 876bf90ed96e..4e7d9111d0eb 100644
> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
This should go to its own binding file just like in all other
SCMI-variant cases. And if you looked how these other files are done,
you would see my complains already that generic fallback is most likely
wrong.
Otherwise explain me what the generic fallback means here and how is it
supposed to work?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: crypto: qcom,ice: Add sa8255p support
2026-05-14 12:55 ` Krzysztof Kozlowski
@ 2026-05-15 13:23 ` Linlin Zhang
2026-05-15 13:26 ` Krzysztof Kozlowski
0 siblings, 1 reply; 15+ messages in thread
From: Linlin Zhang @ 2026-05-15 13:23 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel
On 5/14/2026 8:55 PM, Krzysztof Kozlowski wrote:
> On Mon, May 11, 2026 at 08:37:48PM -0700, Linlin Zhang wrote:
>> On sa8255p, resources such as PHY, clocks, regulators, and resets are
>> managed by remote firmware via the SCMI power protocol. As a result, the
>> ICE driver cannot directly access clocks and must instead use power-domains
>> to request resource configuration.
>
> Then how can it be compatible with qcom,inline-crypto-engine?
Thanks for the review.
You are right that the SCMI-based implementation differs from the
traditional inline crypto engine in terms of resource control. On
sa8255p, clocks and other resources are managed by remote firmware
via SCMI, so the driver does not directly control these resources
and instead relies on power domains.
Given this difference, the SCMI variant does not match the same
programming model as the existing qcom,inline-crypto-engine
implementation. Using it as a generic fallback is therefore not
appropriate, as the generic compatible implies that the device can
be handled equivalently by the same driver assumptions, which is
not the case here.
I will rework the bindings to:
- introduce a separate binding for the SCMI-based variant, and
- drop the generic fallback compatible string.
This should align better with other SCMI-based bindings and avoid
overloading the meaning of the generic compatible.
Thanks for pointing this out.
>
>>
>> Add the qcom,sa8255p-inline-crypto-engine compatible string and make clocks
>> optional for platforms that use power-domains instead.
>>
>> Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
>> ---
>> .../crypto/qcom,inline-crypto-engine.yaml | 27 ++++++++++++++++++-
>> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> So this is v2? But previous was v6? Look:
>
> b4 diff '20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com'
> Looking up https://lore.kernel.org/all/20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com/
> Grabbing thread from lore.kernel.org/all/20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com/t.mbox.gz
> Checking for older revisions
> Grabbing search results from lore.kernel.org
> ---
> Analyzing 8 messages in the thread
> Could not find lower series to compare against.
This patch was review internally, and the final version was v6 which is
approved for posting to upstream review. I'm sorry that I forgot update
the version to v1 when I posted this patch to https://lore.kernel.org/.
This is why you see the previous was v6.
I updated the patch to v2 (this is current patchset) with some changes.
Would you please help instruct me how to fix it? need I re-post the patch
serial from v1?
>
>
>>
>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>> index 876bf90ed96e..4e7d9111d0eb 100644
>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>
> This should go to its own binding file just like in all other
> SCMI-variant cases. And if you looked how these other files are done,
> you would see my complains already that generic fallback is most likely
> wrong.
>
> Otherwise explain me what the generic fallback means here and how is it
> supposed to work?
ACK. I'll separate the binding and remove the generic fallback.
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: crypto: qcom,ice: Add sa8255p support
2026-05-15 13:23 ` Linlin Zhang
@ 2026-05-15 13:26 ` Krzysztof Kozlowski
0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-05-15 13:26 UTC (permalink / raw)
To: Linlin Zhang
Cc: Rob Herring, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel
On 15/05/2026 15:23, Linlin Zhang wrote:
>
>
> On 5/14/2026 8:55 PM, Krzysztof Kozlowski wrote:
>> On Mon, May 11, 2026 at 08:37:48PM -0700, Linlin Zhang wrote:
>>> On sa8255p, resources such as PHY, clocks, regulators, and resets are
>>> managed by remote firmware via the SCMI power protocol. As a result, the
>>> ICE driver cannot directly access clocks and must instead use power-domains
>>> to request resource configuration.
>>
>> Then how can it be compatible with qcom,inline-crypto-engine?
>
> Thanks for the review.
>
> You are right that the SCMI-based implementation differs from the
> traditional inline crypto engine in terms of resource control. On
> sa8255p, clocks and other resources are managed by remote firmware
> via SCMI, so the driver does not directly control these resources
> and instead relies on power domains.
>
> Given this difference, the SCMI variant does not match the same
> programming model as the existing qcom,inline-crypto-engine
> implementation. Using it as a generic fallback is therefore not
> appropriate, as the generic compatible implies that the device can
> be handled equivalently by the same driver assumptions, which is
> not the case here.
Are you pasting here LLM responses? Two long paragraphs, written in over
formal English with no grammar errors and repeating standard AI slop
answer - "You are right".
Sending me LLM responses or LLM patches is unfortunately straight way to
get ignored.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver
2026-05-12 3:37 [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource Linlin Zhang
2026-05-12 3:37 ` [PATCH v2 1/3] dt-bindings: crypto: qcom,ice: Add sa8255p support Linlin Zhang
@ 2026-05-12 3:37 ` Linlin Zhang
2026-05-13 4:21 ` sashiko-bot
2026-05-14 14:06 ` Krzysztof Kozlowski
2026-05-12 3:37 ` [PATCH v2 3/3] soc: qcom: ice: Add SCMI support for sa8255p based targets Linlin Zhang
2026-05-14 12:52 ` [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource Krzysztof Kozlowski
3 siblings, 2 replies; 15+ messages in thread
From: Linlin Zhang @ 2026-05-12 3:37 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel, Neeraj Soni, Deepti Jaggi
The QCOM ICE driver manages the ICE core clock through direct calls to
clk_prepare_enable() and clk_disable_unprepare(), which limits integration
with platforms that rely on firmware-managed resources or platform-specific
power management mechanisms.
Replace direct clock management with runtime PM support by moving clock
enable and disable into runtime PM callbacks. Use
pm_runtime_resume_and_get() and pm_runtime_put_sync() in qcom_ice_resume()
and qcom_ice_suspend() to drive power state transitions, and enable runtime
PM in qcom_ice_probe().
Reviewed-by: Neeraj Soni <neeraj.soni@oss.qualcomm.com>
Reviewed-by: Deepti Jaggi <deepti.jaggi@oss.qualcomm.com>
Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
---
drivers/soc/qcom/ice.c | 58 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 53 insertions(+), 5 deletions(-)
diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
index b203bc685cad..6f9d679b530c 100644
--- a/drivers/soc/qcom/ice.c
+++ b/drivers/soc/qcom/ice.c
@@ -16,6 +16,7 @@
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/firmware/qcom/qcom_scm.h>
@@ -310,8 +311,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
struct device *dev = ice->dev;
int err;
- err = clk_prepare_enable(ice->core_clk);
- if (err) {
+ err = pm_runtime_resume_and_get(dev);
+ if (err < 0) {
dev_err(dev, "failed to enable core clock (%d)\n",
err);
return err;
@@ -323,7 +324,7 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
int qcom_ice_suspend(struct qcom_ice *ice)
{
- clk_disable_unprepare(ice->core_clk);
+ pm_runtime_put_sync(ice->dev);
ice->hwkm_init_complete = false;
return 0;
@@ -716,24 +717,69 @@ EXPORT_SYMBOL_GPL(devm_of_qcom_ice_get);
static int qcom_ice_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct qcom_ice *engine;
void __iomem *base;
+ int ret;
base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base)) {
- dev_warn(&pdev->dev, "ICE registers not found\n");
+ dev_warn(dev, "ICE registers not found\n");
return PTR_ERR(base);
}
- engine = qcom_ice_create(&pdev->dev, base);
+ engine = qcom_ice_create(dev, base);
if (IS_ERR(engine))
return PTR_ERR(engine);
platform_set_drvdata(pdev, engine);
+ ret = devm_pm_runtime_enable(dev);
+ if (ret) {
+ dev_warn(dev, "Enable runtime PM failed, ret: %d\n", ret);
+ return ret;
+ }
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0) {
+ dev_warn(dev, "Runtime PM fails to resume, ret: %d\n", ret);
+ return ret;
+ }
+
return 0;
}
+static void qcom_ice_remove(struct platform_device *pdev)
+{
+ pm_runtime_put_sync(&pdev->dev);
+}
+
+static int ice_runtime_resume(struct device *dev)
+{
+ struct qcom_ice *ice = dev_get_drvdata(dev);
+ int err = 0;
+
+ err = clk_prepare_enable(ice->core_clk);
+ if (err) {
+ dev_err(dev, "failed to enable core clock (%d)\n",
+ err);
+ }
+
+ return err;
+}
+
+static int ice_runtime_suspend(struct device *dev)
+{
+ struct qcom_ice *ice = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(ice->core_clk);
+ return 0;
+}
+
+static const struct dev_pm_ops ice_pm_ops = {
+ SET_RUNTIME_PM_OPS(ice_runtime_suspend, ice_runtime_resume, NULL)
+};
+
static const struct of_device_id qcom_ice_of_match_table[] = {
{ .compatible = "qcom,inline-crypto-engine" },
{ },
@@ -742,8 +788,10 @@ MODULE_DEVICE_TABLE(of, qcom_ice_of_match_table);
static struct platform_driver qcom_ice_driver = {
.probe = qcom_ice_probe,
+ .remove = qcom_ice_remove,
.driver = {
.name = "qcom-ice",
+ .pm = &ice_pm_ops,
.of_match_table = qcom_ice_of_match_table,
},
};
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver
2026-05-12 3:37 ` [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver Linlin Zhang
@ 2026-05-13 4:21 ` sashiko-bot
2026-05-14 14:06 ` Krzysztof Kozlowski
1 sibling, 0 replies; 15+ messages in thread
From: sashiko-bot @ 2026-05-13 4:21 UTC (permalink / raw)
To: Linlin Zhang; +Cc: devicetree, krzk+dt, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Runtime PM transitions in legacy bindings target the consumer device instead of the ICE driver, causing recursive PM calls and deadlocks.
- [High] The ICE core clock remains permanently enabled, negating the power savings of runtime PM.
- [High] Probe function unconditionally holds a PM runtime usage count, causing underflows on removal or premature suspends during active consumer use.
- [High] Missing PM runtime usage count decrement on error path in `qcom_ice_resume()`.
- [Low] Unconditional assignment of PM operations produces dead code and compiler warnings when PM is disabled.
--
commit 06d65ce92438c9c71440b4209d19b0ada46013d3
Author: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
soc: qcom: ice: Enable PM runtime for ICE driver
The QCOM ICE driver manages the ICE core clock through direct calls to
clk_prepare_enable() and clk_disable_unprepare(), which limits integration
with platforms that rely on firmware-managed resources or platform-specific
power management mechanisms.
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index b203bc685cadd..6f9d679b530cc 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
[ ... ]
> @@ -310,8 +311,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
> struct device *dev = ice->dev;
> int err;
>
> - err = clk_prepare_enable(ice->core_clk);
> - if (err) {
> + err = pm_runtime_resume_and_get(dev);
> + if (err < 0) {
> dev_err(dev, "failed to enable core clock (%d)\n",
> err);
> return err;
For legacy devicetree bindings, the ICE instance is instantiated using the
consumer device, so dev points directly to the UFS device.
If qcom_ice_resume() is invoked from within the UFS controller's own
runtime_resume callback, does this cause a deadlock?
rpm_resume() would detect that the device is already in the RPM_RESUMING
state, and the thread would block waiting for itself to finish.
Also, if qcom_ice_wait_bist_status(ice) fails later in this function:
drivers/soc/qcom/ice.c:qcom_ice_resume() {
...
qcom_ice_hwkm_init(ice);
return qcom_ice_wait_bist_status(ice);
}
Does this leak the PM usage count, since pm_runtime_put() is not called
on the error path?
> @@ -323,7 +324,7 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
>
> int qcom_ice_suspend(struct qcom_ice *ice)
> {
> - clk_disable_unprepare(ice->core_clk);
> + pm_runtime_put_sync(ice->dev);
Does this cause a PM usage count underflow for legacy bindings?
Since ice->dev is the consumer device, calling this during the UFS
controller's runtime_suspend callback might drop the usage count below 0
because the consumer's usage count is already 0.
[ ... ]
> @@ -716,24 +717,69 @@ EXPORT_SYMBOL_GPL(devm_of_qcom_ice_get);
>
> static int qcom_ice_probe(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
[ ... ]
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret < 0) {
> + dev_warn(dev, "Runtime PM fails to resume, ret: %d\n", ret);
> + return ret;
> + }
> +
> return 0;
> }
>
> +static void qcom_ice_remove(struct platform_device *pdev)
> +{
> + pm_runtime_put_sync(&pdev->dev);
> +}
Does this create a race condition with the consumer device?
The probe function holds a PM runtime usage count, and the driver seems to
expect the consumer's suspend routine to drop this initial reference.
If the UFS driver suspends, the ICE count drops to 0. If the ICE driver is
subsequently unbound, qcom_ice_remove() will call pm_runtime_put_sync(),
dropping the usage count to -1.
Conversely, if ICE is unbound while UFS is resumed (count is 1),
qcom_ice_remove() drops the count to 0, suspending the ICE hardware while
the active UFS consumer is still relying on it for crypto operations.
[ ... ]
> +static int ice_runtime_suspend(struct device *dev)
> +{
> + struct qcom_ice *ice = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(ice->core_clk);
> + return 0;
> +}
Does the ICE core clock remain permanently enabled anyway?
If the core clock is acquired in qcom_ice_create() using
devm_clk_get_enabled() or devm_clk_get_optional_enabled(), it holds a
permanent enable reference on the clock.
When ice_runtime_suspend() calls clk_disable_unprepare(), it would only
decrement the clock's enable count from 2 back to 1, leaving the physical
hardware clock continuously running.
[ ... ]
> static struct platform_driver qcom_ice_driver = {
> .probe = qcom_ice_probe,
> + .remove = qcom_ice_remove,
> .driver = {
> .name = "qcom-ice",
> + .pm = &ice_pm_ops,
This isn't a bug, but should this use pm_ptr(&ice_pm_ops)?
When CONFIG_PM is disabled, SET_RUNTIME_PM_OPS expands to nothing,
which leaves the static functions ice_runtime_suspend and
ice_runtime_resume unreferenced and may cause unused-function warnings.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver
2026-05-12 3:37 ` [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver Linlin Zhang
2026-05-13 4:21 ` sashiko-bot
@ 2026-05-14 14:06 ` Krzysztof Kozlowski
2026-05-15 14:22 ` Linlin Zhang
1 sibling, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-05-14 14:06 UTC (permalink / raw)
To: Linlin Zhang, Rob Herring, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel, Neeraj Soni, Deepti Jaggi
On 12/05/2026 05:37, Linlin Zhang wrote:
> The QCOM ICE driver manages the ICE core clock through direct calls to
> clk_prepare_enable() and clk_disable_unprepare(), which limits integration
No, it does not limit any integration.
> with platforms that rely on firmware-managed resources or platform-specific
> power management mechanisms.
Nope. It's perfectly correct way of managing clocks. Adding runtime PM
ONLY to toggle clocks is absolute killer, pointless overhead without
benefits.
>
> Replace direct clock management with runtime PM support by moving clock
> enable and disable into runtime PM callbacks. Use
> pm_runtime_resume_and_get() and pm_runtime_put_sync() in qcom_ice_resume()
> and qcom_ice_suspend() to drive power state transitions, and enable runtime
> PM in qcom_ice_probe().
>
> Reviewed-by: Neeraj Soni <neeraj.soni@oss.qualcomm.com>
> Reviewed-by: Deepti Jaggi <deepti.jaggi@oss.qualcomm.com>
> Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
> ---
> drivers/soc/qcom/ice.c | 58 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index b203bc685cad..6f9d679b530c 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -16,6 +16,7 @@
> #include <linux/of.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>
> #include <linux/firmware/qcom/qcom_scm.h>
>
> @@ -310,8 +311,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
> struct device *dev = ice->dev;
> int err;
>
> - err = clk_prepare_enable(ice->core_clk);
> - if (err) {
> + err = pm_runtime_resume_and_get(dev);
> + if (err < 0) {
> dev_err(dev, "failed to enable core clock (%d)\n",
> err);
> return err;
> @@ -323,7 +324,7 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
>
> int qcom_ice_suspend(struct qcom_ice *ice)
> {
> - clk_disable_unprepare(ice->core_clk);
> + pm_runtime_put_sync(ice->dev);
> ice->hwkm_init_complete = false;
>
> return 0;
This is pretty pointless change. At least by quick glance. You changed
nothing here for PM, except adding indirection layer and more locks.
Clocks will be gated the same way, no energy savings. But on the other
hand introducing runtime PM subsystem is huge bunch of code with its own
locks, completely unnecessary here.
This itself is poor choice and has NEGATIVE impact on all existing
platforms without any benefit.
I am surprised you went through SIX internal reviews, collected two
internal review tags and no one suggested that using runtime PM ONLY to
toggle clocks is pretty pointless and undesired.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver
2026-05-14 14:06 ` Krzysztof Kozlowski
@ 2026-05-15 14:22 ` Linlin Zhang
2026-05-15 14:48 ` Krzysztof Kozlowski
0 siblings, 1 reply; 15+ messages in thread
From: Linlin Zhang @ 2026-05-15 14:22 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel, Neeraj Soni, Deepti Jaggi,
bjorn.andersson
Hi Krzysztof,
Thanks for the review.
For the SCMI-based platforms (e.g. sa8255p), the ICE resources such as
clocks are not controlled directly by the ICE driver. Instead, they are
managed by remote firmware and exposed to Linux via power domains. As a
result, the ICE driver cannot use clk_prepare_enable() directly to
control the hardware clock.
The intention of moving the clock handling into runtime PM callbacks is
to align the ICE driver with the power domain framework used on these
platforms. When the ICE device is attached to a power domain, invoking
pm_runtime_resume_and_get() will trigger the provider (remote firmware
via SCMI) to power up the device, which in turn enables the underlying
clock and other resources.
This design follows the guidance where the runtime PM framework is
used as the common mechanism to abstract both:
- direct clock control on non-SCMI platforms, and
- firmware-controlled resources via power domains on SCMI platforms.
In both cases, the runtime PM callbacks are responsible for performing
the actual resource enable/disable:
- for legacy platforms: clk_prepare_enable()/disable_unprepare()
- for SCMI platforms: power domain on/off handled by firmware
So while it may look like an additional layer on legacy platforms, this
approach provides a unified mechanism without requiring separate driver
entry points or special handling in the upper layers (e.g. UFS driver).
That said, I understand your concern that introducing runtime PM solely
for clock gating can be seen as unnecessary overhead on existing
platforms. I will revisit the implementation to ensure that:
- the runtime PM integration does not introduce regressions for legacy
platforms, and
- the design clearly justifies the common abstraction for both SCMI
and non-SCMI cases.
In addition, I rewrite the commit message as the following to make the
intention more clear.
On some platforms the ICE device is placed in a firmware-managed power
domain. In those cases the ICE core resources (including the clock) are
not directly controllable by Linux and are instead toggled by the power
domain provider (e.g. remote firmware via SCMI).
Wire the ICE device into runtime PM so that a single pm_runtime
transition is used to bring the ICE device up/down. When the device is
attached to a PM domain, pm_runtime_resume_and_get()/pm_runtime_put_sync()
will invoke the PM domain callbacks and let the provider manage the
resources. On platforms without a PM domain the runtime PM callbacks
continue to perform the existing clock enable/disable locally.
No functional change is intended for non-firmware-managed platforms; the
change provides a common control point that allows ICE to operate when
resources are owned by a PM domain provider.
Thanks again for the feedback. I would appreciate your further review
and comments.
Best regards,
Linlin
On 5/14/2026 10:06 PM, Krzysztof Kozlowski wrote:
> On 12/05/2026 05:37, Linlin Zhang wrote:
>> The QCOM ICE driver manages the ICE core clock through direct calls to
>> clk_prepare_enable() and clk_disable_unprepare(), which limits integration
>
> No, it does not limit any integration.
>
>> with platforms that rely on firmware-managed resources or platform-specific
>> power management mechanisms.
>
> Nope. It's perfectly correct way of managing clocks. Adding runtime PM
> ONLY to toggle clocks is absolute killer, pointless overhead without
> benefits.
>
>>
>> Replace direct clock management with runtime PM support by moving clock
>> enable and disable into runtime PM callbacks. Use
>> pm_runtime_resume_and_get() and pm_runtime_put_sync() in qcom_ice_resume()
>> and qcom_ice_suspend() to drive power state transitions, and enable runtime
>> PM in qcom_ice_probe().
>>
>> Reviewed-by: Neeraj Soni <neeraj.soni@oss.qualcomm.com>
>> Reviewed-by: Deepti Jaggi <deepti.jaggi@oss.qualcomm.com>
>> Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
>> ---
>> drivers/soc/qcom/ice.c | 58 ++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 53 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
>> index b203bc685cad..6f9d679b530c 100644
>> --- a/drivers/soc/qcom/ice.c
>> +++ b/drivers/soc/qcom/ice.c
>> @@ -16,6 +16,7 @@
>> #include <linux/of.h>
>> #include <linux/of_platform.h>
>> #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>
>> #include <linux/firmware/qcom/qcom_scm.h>
>>
>> @@ -310,8 +311,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
>> struct device *dev = ice->dev;
>> int err;
>>
>> - err = clk_prepare_enable(ice->core_clk);
>> - if (err) {
>> + err = pm_runtime_resume_and_get(dev);
>> + if (err < 0) {
>> dev_err(dev, "failed to enable core clock (%d)\n",
>> err);
>> return err;
>> @@ -323,7 +324,7 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
>>
>> int qcom_ice_suspend(struct qcom_ice *ice)
>> {
>> - clk_disable_unprepare(ice->core_clk);
>> + pm_runtime_put_sync(ice->dev);
>> ice->hwkm_init_complete = false;
>>
>> return 0;
>
>
> This is pretty pointless change. At least by quick glance. You changed
> nothing here for PM, except adding indirection layer and more locks.
> Clocks will be gated the same way, no energy savings. But on the other
> hand introducing runtime PM subsystem is huge bunch of code with its own
> locks, completely unnecessary here.
>
> This itself is poor choice and has NEGATIVE impact on all existing
> platforms without any benefit.
>
> I am surprised you went through SIX internal reviews, collected two
> internal review tags and no one suggested that using runtime PM ONLY to
> toggle clocks is pretty pointless and undesired.>
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver
2026-05-15 14:22 ` Linlin Zhang
@ 2026-05-15 14:48 ` Krzysztof Kozlowski
2026-05-15 14:49 ` Krzysztof Kozlowski
0 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-05-15 14:48 UTC (permalink / raw)
To: Linlin Zhang, Rob Herring, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel, Neeraj Soni, Deepti Jaggi,
bjorn.andersson
On 15/05/2026 16:22, Linlin Zhang wrote:
>
> Hi Krzysztof,
>
> Thanks for the review.
>
> For the SCMI-based platforms (e.g. sa8255p), the ICE resources such as
> clocks are not controlled directly by the ICE driver. Instead, they are
> managed by remote firmware and exposed to Linux via power domains. As a
> result, the ICE driver cannot use clk_prepare_enable() directly to
> control the hardware clock.
>
> The intention of moving the clock handling into runtime PM callbacks is
> to align the ICE driver with the power domain framework used on these
> platforms. When the ICE device is attached to a power domain, invoking
> pm_runtime_resume_and_get() will trigger the provider (remote firmware
> via SCMI) to power up the device, which in turn enables the underlying
> clock and other resources.
>
> This design follows the guidance where the runtime PM framework is
> used as the common mechanism to abstract both:
> - direct clock control on non-SCMI platforms, and
> - firmware-controlled resources via power domains on SCMI platforms.
>
> In both cases, the runtime PM callbacks are responsible for performing
> the actual resource enable/disable:
> - for legacy platforms: clk_prepare_enable()/disable_unprepare()
> - for SCMI platforms: power domain on/off handled by firmware
>
> So while it may look like an additional layer on legacy platforms, this
> approach provides a unified mechanism without requiring separate driver
> entry points or special handling in the upper layers (e.g. UFS driver).
>
> That said, I understand your concern that introducing runtime PM solely
> for clock gating can be seen as unnecessary overhead on existing
> platforms. I will revisit the implementation to ensure that:
> - the runtime PM integration does not introduce regressions for legacy
> platforms, and
> - the design clearly justifies the common abstraction for both SCMI
> and non-SCMI cases.
>
> In addition, I rewrite the commit message as the following to make the
> intention more clear.
>
> On some platforms the ICE device is placed in a firmware-managed power
> domain. In those cases the ICE core resources (including the clock) are
> not directly controllable by Linux and are instead toggled by the power
> domain provider (e.g. remote firmware via SCMI).
>
> Wire the ICE device into runtime PM so that a single pm_runtime
> transition is used to bring the ICE device up/down. When the device is
> attached to a PM domain, pm_runtime_resume_and_get()/pm_runtime_put_sync()
> will invoke the PM domain callbacks and let the provider manage the
> resources. On platforms without a PM domain the runtime PM callbacks
> continue to perform the existing clock enable/disable locally.
>
> No functional change is intended for non-firmware-managed platforms; the
> change provides a common control point that allows ICE to operate when
> resources are owned by a PM domain provider.
>
Nothing here resolves the comments. Also, it's top posted. Honestly, I
won't be talking through you with LLM, so consider patch NAKed.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver
2026-05-15 14:48 ` Krzysztof Kozlowski
@ 2026-05-15 14:49 ` Krzysztof Kozlowski
0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-05-15 14:49 UTC (permalink / raw)
To: Linlin Zhang, Rob Herring, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel, Neeraj Soni, Deepti Jaggi,
bjorn.andersson
On 15/05/2026 16:48, Krzysztof Kozlowski wrote:
> On 15/05/2026 16:22, Linlin Zhang wrote:
>>
>> Hi Krzysztof,
>>
>> Thanks for the review.
>>
>> For the SCMI-based platforms (e.g. sa8255p), the ICE resources such as
>> clocks are not controlled directly by the ICE driver. Instead, they are
>> managed by remote firmware and exposed to Linux via power domains. As a
>> result, the ICE driver cannot use clk_prepare_enable() directly to
>> control the hardware clock.
>>
>> The intention of moving the clock handling into runtime PM callbacks is
>> to align the ICE driver with the power domain framework used on these
>> platforms. When the ICE device is attached to a power domain, invoking
>> pm_runtime_resume_and_get() will trigger the provider (remote firmware
>> via SCMI) to power up the device, which in turn enables the underlying
>> clock and other resources.
>>
>> This design follows the guidance where the runtime PM framework is
>> used as the common mechanism to abstract both:
>> - direct clock control on non-SCMI platforms, and
>> - firmware-controlled resources via power domains on SCMI platforms.
>>
>> In both cases, the runtime PM callbacks are responsible for performing
>> the actual resource enable/disable:
>> - for legacy platforms: clk_prepare_enable()/disable_unprepare()
>> - for SCMI platforms: power domain on/off handled by firmware
>>
>> So while it may look like an additional layer on legacy platforms, this
>> approach provides a unified mechanism without requiring separate driver
>> entry points or special handling in the upper layers (e.g. UFS driver).
>>
>> That said, I understand your concern that introducing runtime PM solely
>> for clock gating can be seen as unnecessary overhead on existing
>> platforms. I will revisit the implementation to ensure that:
>> - the runtime PM integration does not introduce regressions for legacy
>> platforms, and
>> - the design clearly justifies the common abstraction for both SCMI
>> and non-SCMI cases.
>>
>> In addition, I rewrite the commit message as the following to make the
>> intention more clear.
>>
>> On some platforms the ICE device is placed in a firmware-managed power
>> domain. In those cases the ICE core resources (including the clock) are
>> not directly controllable by Linux and are instead toggled by the power
>> domain provider (e.g. remote firmware via SCMI).
>>
>> Wire the ICE device into runtime PM so that a single pm_runtime
>> transition is used to bring the ICE device up/down. When the device is
>> attached to a PM domain, pm_runtime_resume_and_get()/pm_runtime_put_sync()
>> will invoke the PM domain callbacks and let the provider manage the
>> resources. On platforms without a PM domain the runtime PM callbacks
>> continue to perform the existing clock enable/disable locally.
>>
>> No functional change is intended for non-firmware-managed platforms; the
>> change provides a common control point that allows ICE to operate when
>> resources are owned by a PM domain provider.
>>
>
>
> Nothing here resolves the comments. Also, it's top posted. Honestly, I
> won't be talking through you with LLM, so consider patch NAKed.
Plus you completely ignored all the comments I posted in review. Great.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] soc: qcom: ice: Add SCMI support for sa8255p based targets
2026-05-12 3:37 [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource Linlin Zhang
2026-05-12 3:37 ` [PATCH v2 1/3] dt-bindings: crypto: qcom,ice: Add sa8255p support Linlin Zhang
2026-05-12 3:37 ` [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver Linlin Zhang
@ 2026-05-12 3:37 ` Linlin Zhang
2026-05-13 5:16 ` sashiko-bot
2026-05-14 12:52 ` [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource Krzysztof Kozlowski
3 siblings, 1 reply; 15+ messages in thread
From: Linlin Zhang @ 2026-05-12 3:37 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel, Neeraj Soni, Deepti Jaggi
The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
The driver requests resources operations over SCMI using power
and performance protocols.
The SCMI power protocol enables or disables resources like clocks,
interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
such as resume/suspend, to control power states(on/off).
The SCMI performance protocol manages ICE clock, with a power domain
set for ICE clock. The driver uses runtime PM framework APIs to
request power on/off status of the clock.
Reviewed-by: Neeraj Soni <neeraj.soni@oss.qualcomm.com>
Reviewed-by: Deepti Jaggi <quic_djaggi@quicinc.com>
Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
---
drivers/soc/qcom/ice.c | 64 ++++++++++++++++++++++++++++--------------
1 file changed, 43 insertions(+), 21 deletions(-)
diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
index 6f9d679b530c..cf185a6e1973 100644
--- a/drivers/soc/qcom/ice.c
+++ b/drivers/soc/qcom/ice.c
@@ -68,6 +68,10 @@ union crypto_cfg {
};
};
+struct engine_desc {
+ bool fw_managed;
+};
+
/* QCOM ICE HWKM (Hardware Key Manager) registers */
#define HWKM_OFFSET 0x8000
@@ -554,6 +558,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
void __iomem *base)
{
struct qcom_ice *engine;
+ const struct engine_desc *engine_cfg = NULL;
if (!qcom_scm_is_available())
return ERR_PTR(-EPROBE_DEFER);
@@ -570,20 +575,23 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
engine->dev = dev;
engine->base = base;
- /*
- * Legacy DT binding uses different clk names for each consumer,
- * so lets try those first. If none of those are a match, it means
- * the we only have one clock and it is part of the dedicated DT node.
- * Also, enable the clock before we check what HW version the driver
- * supports.
- */
- 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 (IS_ERR(engine->core_clk))
- return ERR_CAST(engine->core_clk);
+ engine_cfg = device_get_match_data(dev);
+ if (!engine_cfg || !engine_cfg->fw_managed) {
+ /*
+ * Legacy DT binding uses different clk names for each consumer,
+ * so lets try those first. If none of those are a match, it means
+ * the we only have one clock and it is part of the dedicated DT node.
+ * Also, enable the clock before we check what HW version the driver
+ * supports.
+ */
+ 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 (IS_ERR(engine->core_clk))
+ return ERR_CAST(engine->core_clk);
+ }
if (!qcom_ice_check_supported(engine))
return ERR_PTR(-EOPNOTSUPP);
@@ -756,13 +764,17 @@ static void qcom_ice_remove(struct platform_device *pdev)
static int ice_runtime_resume(struct device *dev)
{
- struct qcom_ice *ice = dev_get_drvdata(dev);
+ struct engine_desc *engine_cfg = device_get_match_data(dev);
int err = 0;
- err = clk_prepare_enable(ice->core_clk);
- if (err) {
- dev_err(dev, "failed to enable core clock (%d)\n",
- err);
+ if (!engine_cfg || !engine_cfg->fw_managed) {
+ struct qcom_ice *ice = dev_get_drvdata(dev);
+
+ err = clk_prepare_enable(ice->core_clk);
+ if (err) {
+ dev_err(dev, "failed to enable core clock (%d)\n",
+ err);
+ }
}
return err;
@@ -770,9 +782,14 @@ static int ice_runtime_resume(struct device *dev)
static int ice_runtime_suspend(struct device *dev)
{
- struct qcom_ice *ice = dev_get_drvdata(dev);
+ const struct engine_desc *engine_cfg = device_get_match_data(dev);
+
+ if (!engine_cfg || !engine_cfg->fw_managed) {
+ struct qcom_ice *ice = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(ice->core_clk);
+ }
- clk_disable_unprepare(ice->core_clk);
return 0;
}
@@ -780,8 +797,13 @@ static const struct dev_pm_ops ice_pm_ops = {
SET_RUNTIME_PM_OPS(ice_runtime_suspend, ice_runtime_resume, NULL)
};
+static const struct engine_desc cfg_fw_managed = {
+ .fw_managed = true,
+};
+
static const struct of_device_id qcom_ice_of_match_table[] = {
{ .compatible = "qcom,inline-crypto-engine" },
+ { .compatible = "qcom,sa8255p-inline-crypto-engine", .data = &cfg_fw_managed },
{ },
};
MODULE_DEVICE_TABLE(of, qcom_ice_of_match_table);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/3] soc: qcom: ice: Add SCMI support for sa8255p based targets
2026-05-12 3:37 ` [PATCH v2 3/3] soc: qcom: ice: Add SCMI support for sa8255p based targets Linlin Zhang
@ 2026-05-13 5:16 ` sashiko-bot
0 siblings, 0 replies; 15+ messages in thread
From: sashiko-bot @ 2026-05-13 5:16 UTC (permalink / raw)
To: Linlin Zhang; +Cc: krzk+dt, robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Type confusion in `qcom_ice_create` causes system crash for legacy consumers.
- [Critical] Premature hardware access on `fw_managed` devices before SCMI PM domain is resumed.
- [Low] Discarded `const` qualifier on `device_get_match_data()` return value.
--
commit f67a1074aa6c150b2ffc9f1fb18d4a18e025ed13
Author: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
soc: qcom: ice: Add SCMI support for sa8255p based targets
This commit adds SCMI power and performance protocol support for managing
ICE clocks on the Qualcomm SA8255p SoC. It introduces a firmware-managed
flag via match data to skip explicit clock enablement, relying on runtime
PM framework APIs instead.
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 6f9d679b530cc..cf185a6e19731 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
[ ... ]
> @@ -570,20 +575,23 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
> engine->dev = dev;
> engine->base = base;
>
> - /*
> - * Legacy DT binding uses different clk names for each consumer,
> - * so lets try those first. If none of those are a match, it means
> - * the we only have one clock and it is part of the dedicated DT node.
> - * Also, enable the clock before we check what HW version the driver
> - * supports.
> - */
> - 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 (IS_ERR(engine->core_clk))
> - return ERR_CAST(engine->core_clk);
> + engine_cfg = device_get_match_data(dev);
> + if (!engine_cfg || !engine_cfg->fw_managed) {
When qcom_ice_create() is called via the legacy fallback path
(of_qcom_ice_get()), the dev parameter points to the consumer device (e.g.,
the SDHCI or UFS host controller), not the dedicated ICE device.
Will calling device_get_match_data(dev) here retrieve the match data of the
consumer driver rather than struct engine_desc?
If the consumer's match data structure happens to have a non-zero byte where
fw_managed is expected, this could falsely evaluate to true, skipping clock
initialization and leading to a crash.
> + /*
> + * Legacy DT binding uses different clk names for each consumer,
> + * so lets try those first. If none of those are a match, it means
> + * the we only have one clock and it is part of the dedicated DT node.
> + * Also, enable the clock before we check what HW version the driver
> + * supports.
> + */
> + 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 (IS_ERR(engine->core_clk))
> + return ERR_CAST(engine->core_clk);
> + }
>
> if (!qcom_ice_check_supported(engine))
> return ERR_PTR(-EOPNOTSUPP);
For firmware-managed devices, the driver skips explicitly enabling the
ICE core clock. However, qcom_ice_check_supported() immediately reads the
QCOM_ICE_REG_VERSION hardware register.
Could this access cause a bus fault on firmware-managed devices?
The runtime PM framework is responsible for the SCMI power and performance
domain, but devm_pm_runtime_enable() is called in probe() after
qcom_ice_create() returns. Reading hardware registers before the PM
domain is active might violate power management ordering.
[ ... ]
> @@ -756,13 +764,17 @@ static void qcom_ice_remove(struct platform_device *pdev)
>
> static int ice_runtime_resume(struct device *dev)
> {
> - struct qcom_ice *ice = dev_get_drvdata(dev);
> + struct engine_desc *engine_cfg = device_get_match_data(dev);
This isn't a bug, but device_get_match_data() returns a const void pointer.
Assigning it to a non-const struct engine_desc pointer discards the const
qualifier and triggers a compiler warning. Could this be declared as
const struct engine_desc *engine_cfg instead, similar to how it is handled
in ice_runtime_suspend()?
> int err = 0;
>
> - err = clk_prepare_enable(ice->core_clk);
> - if (err) {
> - dev_err(dev, "failed to enable core clock (%d)\n",
> - err);
> + if (!engine_cfg || !engine_cfg->fw_managed) {
> + struct qcom_ice *ice = dev_get_drvdata(dev);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource
2026-05-12 3:37 [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource Linlin Zhang
` (2 preceding siblings ...)
2026-05-12 3:37 ` [PATCH v2 3/3] soc: qcom: ice: Add SCMI support for sa8255p based targets Linlin Zhang
@ 2026-05-14 12:52 ` Krzysztof Kozlowski
2026-05-15 11:35 ` Linlin Zhang
3 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-05-14 12:52 UTC (permalink / raw)
To: Linlin Zhang
Cc: Rob Herring, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel
On Mon, May 11, 2026 at 08:37:47PM -0700, Linlin Zhang wrote:
> From: linlzhan <linlzhan@qti.qualcomm.com>
>
> The Qualcomm automotive SA8255p SoC relies on firmware to configure
> platform resources, including clocks, interconnects and TLMM (GPIOs).
> These resources are controlled by the driver via SCMI power and
> performance protocols.
>
> The SCMI power protocol is used to enable and disable platform
> resources, including clocks, interconnect paths, and TLMM, by mapping
> resource state transitions to the runtime PM framework?s
> resume/suspend callbacks.
>
> In this design, the ICE driver acts as an SCMI client, with clocks and
> power domains abstracted and controlled by the SCMI server in firmware.
> This implementation depends on pm_runtime_resume_and_get() and
> pm_runtime_put_sync(), which are available in the OPP tree?s
> linux-next branch.
>
> v2:
> -- rebase the patchset
> -- update to/cc lists
> -- Link to v1: https://lore.kernel.org/all/20260430032136.3058773-1-linlin.zhang@oss.qualcomm.com/
>
> -- To Linux Community
>
> v6:
> - Protect calling clock API with fw_managed flag in ICE runtime OPS callbacks.
> - Link to v5: http://shc-kerarch-hyd:8080/kernel_archive/20260324095703.1306437-1-linlin.zhang@oss.qualcomm.com/T/#t
Please do not include non-working links in public postings.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource
2026-05-14 12:52 ` [PATCH v2 0/3] soc: qcom: ice: Enable firmware managed resource Krzysztof Kozlowski
@ 2026-05-15 11:35 ` Linlin Zhang
0 siblings, 0 replies; 15+ messages in thread
From: Linlin Zhang @ 2026-05-15 11:35 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Herbert Xu, David S . Miller, devicetree, linux-crypto,
linux-arm-msm, linux-kernel
On 5/14/2026 8:52 PM, Krzysztof Kozlowski wrote:
> On Mon, May 11, 2026 at 08:37:47PM -0700, Linlin Zhang wrote:
>> From: linlzhan <linlzhan@qti.qualcomm.com>
>>
>> The Qualcomm automotive SA8255p SoC relies on firmware to configure
>> platform resources, including clocks, interconnects and TLMM (GPIOs).
>> These resources are controlled by the driver via SCMI power and
>> performance protocols.
>>
>> The SCMI power protocol is used to enable and disable platform
>> resources, including clocks, interconnect paths, and TLMM, by mapping
>> resource state transitions to the runtime PM framework?s
>> resume/suspend callbacks.
>>
>> In this design, the ICE driver acts as an SCMI client, with clocks and
>> power domains abstracted and controlled by the SCMI server in firmware.
>> This implementation depends on pm_runtime_resume_and_get() and
>> pm_runtime_put_sync(), which are available in the OPP tree?s
>> linux-next branch.
>>
>> v2:
>> -- rebase the patchset
>> -- update to/cc lists
>> -- Link to v1: https://lore.kernel.org/all/20260430032136.3058773-1-linlin.zhang@oss.qualcomm.com/
>>
>> -- To Linux Community
>>
>> v6:
>> - Protect calling clock API with fw_managed flag in ICE runtime OPS callbacks.
>> - Link to v5: http://shc-kerarch-hyd:8080/kernel_archive/20260324095703.1306437-1-linlin.zhang@oss.qualcomm.com/T/#t
>
> Please do not include non-working links in public postings.
Thanks for your comment!
I'll remove them in next patch
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 15+ messages in thread