All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
@ 2021-05-17 15:54 Sudeep Holla
  2021-05-17 19:17 ` Rob Herring
                   ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Sudeep Holla @ 2021-05-17 15:54 UTC (permalink / raw)
  To: linux-kernel, devicetree
  Cc: Sudeep Holla, Rob Herring, Manivannan Sadhasivam, Hector Yuan,
	Viresh Kumar, Bjorn Andersson, Rob Herring

The CLKSCREW attack [0] exposed security vulnerabilities in energy management
implementations where untrusted software had direct access to clock and
voltage hardware controls. In this attack, the malicious software was able to
place the platform into unsafe overclocked or undervolted configurations. Such
configurations then enabled the injection of predictable faults to reveal
secrets.

Many Arm-based systems used to or still use voltage regulator and clock
frameworks in the kernel. These frameworks allow callers to independently
manipulate frequency and voltage settings. Such implementations can render
systems susceptible to this form of attack.

Attacks such as CLKSCREW are now being mitigated by not having direct and
independent control of clock and voltage in the kernel and moving that
control to a trusted entity, such as the SCP firmware or secure world
firmware/software which are to perform sanity checking on the requested
performance levels, thereby preventing any attempted malicious programming.

With the advent of such an abstraction, there is a need to replace the
generic clock and regulator bindings used by such devices with a generic
performance domains bindings.

[0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang

Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
Cc: Rob Herring <robh+dt@kernel.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
Hi All,

Sorry for yet another delay, I don't want to mist this for v5.14 as Mediatek
cpufreq driver was depending on this IIRC.

v3[3]->v4:
	- Dropped unnecessary phandle-array reference
	- Added maxItems = 1 for the property

v2[2]->v3[3]:
	- Dropped required properties
	- Added non cpu device example
	- Updated cpu bindings too

v1[1]->v2[2]:
	- Changed to Dual License
	- Added select: true, enum for #performance-domain-cells and
	  $ref for performance-domain
	- Changed the example to use real existing compatibles instead
	  of made-up ones

Regards,
Sudeep

[1] https://lore.kernel.org/r/20201105173539.1426301-1-sudeep.holla@arm.com
[2] https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
[3] https://lore.kernel.org/r/20210407135913.2067694-1-sudeep.holla@arm.com

 .../devicetree/bindings/arm/cpus.yaml         |  7 ++
 .../bindings/dvfs/performance-domain.yaml     | 74 +++++++++++++++++++
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dvfs/performance-domain.yaml

diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
index f3c7249c73d6..9a2432a88074 100644
--- a/Documentation/devicetree/bindings/arm/cpus.yaml
+++ b/Documentation/devicetree/bindings/arm/cpus.yaml
@@ -257,6 +257,13 @@ description: |+

       where voltage is in V, frequency is in MHz.

+  performance-domains:
+    maxItems: 1
+    description:
+      List of phandles and performance domain specifiers, as defined by
+      bindings of the performance domain provider. See also
+      dvfs/performance-domain.yaml.
+
   power-domains:
     description:
       List of phandles and PM domain specifiers, as defined by bindings of the
diff --git a/Documentation/devicetree/bindings/dvfs/performance-domain.yaml b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
new file mode 100644
index 000000000000..c8b91207f34d
--- /dev/null
+++ b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dvfs/performance-domain.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic performance domains
+
+maintainers:
+  - Sudeep Holla <sudeep.holla@arm.com>
+
+description: |+
+  This binding is intended for performance management of groups of devices or
+  CPUs that run in the same performance domain. Performance domains must not
+  be confused with power domains. A performance domain is defined by a set
+  of devices that always have to run at the same performance level. For a given
+  performance domain, there is a single point of control that affects all the
+  devices in the domain, making it impossible to set the performance level of
+  an individual device in the domain independently from other devices in
+  that domain. For example, a set of CPUs that share a voltage domain, and
+  have a common frequency control, is said to be in the same performance
+  domain.
+
+  This device tree binding can be used to bind performance domain consumer
+  devices with their performance domains provided by performance domain
+  providers. A performance domain provider can be represented by any node in
+  the device tree and can provide one or more performance domains. A consumer
+  node can refer to the provider by a phandle and a set of phandle arguments
+  (so called performance domain specifiers) of length specified by the
+  \#performance-domain-cells property in the performance domain provider node.
+
+select: true
+
+properties:
+  "#performance-domain-cells":
+    description:
+      Number of cells in a performance domain specifier. Typically 0 for nodes
+      representing a single performance domain and 1 for nodes providing
+      multiple performance domains (e.g. performance controllers), but can be
+      any value as specified by device tree binding documentation of particular
+      provider.
+    enum: [ 0, 1 ]
+
+  performance-domains:
+    $ref: '/schemas/types.yaml#/definitions/phandle-array'
+    maxItems: 1
+    description:
+      A phandle and performance domain specifier as defined by bindings of the
+      performance controller/provider specified by phandle.
+
+additionalProperties: true
+
+examples:
+  - |
+    performance: performance-controller@12340000 {
+        compatible = "qcom,cpufreq-hw";
+        reg = <0x12340000 0x1000>;
+        #performance-domain-cells = <1>;
+    };
+
+    // The node above defines a performance controller that is a performance
+    // domain provider and expects one cell as its phandle argument.
+
+    cpus {
+        #address-cells = <2>;
+        #size-cells = <0>;
+
+        cpu@0 {
+            device_type = "cpu";
+            compatible = "arm,cortex-a57";
+            reg = <0x0 0x0>;
+            performance-domains = <&performance 1>;
+        };
+    };
--
2.25.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-17 15:54 [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains Sudeep Holla
@ 2021-05-17 19:17 ` Rob Herring
  2021-05-19 11:23   ` Sudeep Holla
  2021-05-17 20:45 ` Rob Herring
  2021-10-14 10:56 ` Ulf Hansson
  2 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2021-05-17 19:17 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-kernel, devicetree, Manivannan Sadhasivam, Hector Yuan,
	Viresh Kumar, Bjorn Andersson

On Mon, May 17, 2021 at 10:55 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> implementations where untrusted software had direct access to clock and
> voltage hardware controls. In this attack, the malicious software was able to
> place the platform into unsafe overclocked or undervolted configurations. Such
> configurations then enabled the injection of predictable faults to reveal
> secrets.
>
> Many Arm-based systems used to or still use voltage regulator and clock
> frameworks in the kernel. These frameworks allow callers to independently
> manipulate frequency and voltage settings. Such implementations can render
> systems susceptible to this form of attack.
>
> Attacks such as CLKSCREW are now being mitigated by not having direct and
> independent control of clock and voltage in the kernel and moving that
> control to a trusted entity, such as the SCP firmware or secure world
> firmware/software which are to perform sanity checking on the requested
> performance levels, thereby preventing any attempted malicious programming.
>
> With the advent of such an abstraction, there is a need to replace the
> generic clock and regulator bindings used by such devices with a generic
> performance domains bindings.
>
> [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
>
> Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> Cc: Rob Herring <robh+dt@kernel.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-17 15:54 [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains Sudeep Holla
  2021-05-17 19:17 ` Rob Herring
@ 2021-05-17 20:45 ` Rob Herring
  2021-05-19 11:20   ` Sudeep Holla
  2021-10-14 10:56 ` Ulf Hansson
  2 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2021-05-17 20:45 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Rob Herring, linux-kernel, Bjorn Andersson, Hector Yuan,
	Manivannan Sadhasivam, Viresh Kumar, devicetree

On Mon, 17 May 2021 16:54:58 +0100, Sudeep Holla wrote:
> The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> implementations where untrusted software had direct access to clock and
> voltage hardware controls. In this attack, the malicious software was able to
> place the platform into unsafe overclocked or undervolted configurations. Such
> configurations then enabled the injection of predictable faults to reveal
> secrets.
> 
> Many Arm-based systems used to or still use voltage regulator and clock
> frameworks in the kernel. These frameworks allow callers to independently
> manipulate frequency and voltage settings. Such implementations can render
> systems susceptible to this form of attack.
> 
> Attacks such as CLKSCREW are now being mitigated by not having direct and
> independent control of clock and voltage in the kernel and moving that
> control to a trusted entity, such as the SCP firmware or secure world
> firmware/software which are to perform sanity checking on the requested
> performance levels, thereby preventing any attempted malicious programming.
> 
> With the advent of such an abstraction, there is a need to replace the
> generic clock and regulator bindings used by such devices with a generic
> performance domains bindings.
> 
> [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
> 
> Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> Cc: Rob Herring <robh+dt@kernel.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> Hi All,
> 
> Sorry for yet another delay, I don't want to mist this for v5.14 as Mediatek
> cpufreq driver was depending on this IIRC.
> 
> v3[3]->v4:
> 	- Dropped unnecessary phandle-array reference
> 	- Added maxItems = 1 for the property
> 
> v2[2]->v3[3]:
> 	- Dropped required properties
> 	- Added non cpu device example
> 	- Updated cpu bindings too
> 
> v1[1]->v2[2]:
> 	- Changed to Dual License
> 	- Added select: true, enum for #performance-domain-cells and
> 	  $ref for performance-domain
> 	- Changed the example to use real existing compatibles instead
> 	  of made-up ones
> 
> Regards,
> Sudeep
> 
> [1] https://lore.kernel.org/r/20201105173539.1426301-1-sudeep.holla@arm.com
> [2] https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> [3] https://lore.kernel.org/r/20210407135913.2067694-1-sudeep.holla@arm.com
> 
>  .../devicetree/bindings/arm/cpus.yaml         |  7 ++
>  .../bindings/dvfs/performance-domain.yaml     | 74 +++++++++++++++++++
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dvfs/performance-domain.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/dvfs/performance-domain.example.dt.yaml:0:0: /example-0/performance-controller@12340000: failed to match any schema with compatible: ['qcom,cpufreq-hw']

See https://patchwork.ozlabs.org/patch/1479615

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-17 20:45 ` Rob Herring
@ 2021-05-19 11:20   ` Sudeep Holla
  2021-05-20 19:43     ` Rob Herring
  0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2021-05-19 11:20 UTC (permalink / raw)
  To: Rob Herring
  Cc: Rob Herring, linux-kernel, Bjorn Andersson, Hector Yuan,
	Sudeep Holla, Manivannan Sadhasivam, Viresh Kumar, devicetree

Hi Rob,

On Mon, May 17, 2021 at 03:45:11PM -0500, Rob Herring wrote:

[...]

> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/dvfs/performance-domain.example.dt.yaml:0:0: /example-0/performance-controller@12340000: failed to match any schema with compatible: ['qcom,cpufreq-hw']
>
> See https://patchwork.ozlabs.org/patch/1479615

IIUC, such errors due to the fact that the compatible used in the example
is not in any yaml schema(as it is still in the old txt format). I also
assume such errors are allowed until the transition is complete and I
need not fix anything as part of this patch ?

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-17 19:17 ` Rob Herring
@ 2021-05-19 11:23   ` Sudeep Holla
  2021-05-20  3:54     ` Viresh Kumar
  0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2021-05-19 11:23 UTC (permalink / raw)
  To: Rob Herring, Viresh Kumar
  Cc: linux-kernel, devicetree, Sudeep Holla, Manivannan Sadhasivam,
	Hector Yuan, Viresh Kumar, Bjorn Andersson

On Mon, May 17, 2021 at 02:17:05PM -0500, Rob Herring wrote:
> On Mon, May 17, 2021 at 10:55 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> > implementations where untrusted software had direct access to clock and
> > voltage hardware controls. In this attack, the malicious software was able to
> > place the platform into unsafe overclocked or undervolted configurations. Such
> > configurations then enabled the injection of predictable faults to reveal
> > secrets.
> >
> > Many Arm-based systems used to or still use voltage regulator and clock
> > frameworks in the kernel. These frameworks allow callers to independently
> > manipulate frequency and voltage settings. Such implementations can render
> > systems susceptible to this form of attack.
> >
> > Attacks such as CLKSCREW are now being mitigated by not having direct and
> > independent control of clock and voltage in the kernel and moving that
> > control to a trusted entity, such as the SCP firmware or secure world
> > firmware/software which are to perform sanity checking on the requested
> > performance levels, thereby preventing any attempted malicious programming.
> >
> > With the advent of such an abstraction, there is a need to replace the
> > generic clock and regulator bindings used by such devices with a generic
> > performance domains bindings.
> >
> > [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
> >
> > Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks Rob.

Viresh,

I noticed I haven't cc-ed linux-pm list, do you need me to post there or
are you happy to pick it up from here when Hector's mediatek cpufreq drivers
using this are ready to be merged ?

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-19 11:23   ` Sudeep Holla
@ 2021-05-20  3:54     ` Viresh Kumar
  0 siblings, 0 replies; 24+ messages in thread
From: Viresh Kumar @ 2021-05-20  3:54 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Rob Herring, linux-kernel, devicetree, Manivannan Sadhasivam,
	Hector Yuan, Bjorn Andersson

On 19-05-21, 12:23, Sudeep Holla wrote:
> I noticed I haven't cc-ed linux-pm list, do you need me to post there or
> are you happy to pick it up from here when Hector's mediatek cpufreq drivers
> using this are ready to be merged ?

Applied. Thanks.

-- 
viresh

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-19 11:20   ` Sudeep Holla
@ 2021-05-20 19:43     ` Rob Herring
  2021-05-21  4:08       ` Viresh Kumar
  0 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2021-05-20 19:43 UTC (permalink / raw)
  To: Sudeep Holla, Viresh Kumar
  Cc: linux-kernel, Bjorn Andersson, Hector Yuan,
	Manivannan Sadhasivam, devicetree

On Wed, May 19, 2021 at 6:20 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Hi Rob,
>
> On Mon, May 17, 2021 at 03:45:11PM -0500, Rob Herring wrote:
>
> [...]
>
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > Documentation/devicetree/bindings/dvfs/performance-domain.example.dt.yaml:0:0: /example-0/performance-controller@12340000: failed to match any schema with compatible: ['qcom,cpufreq-hw']
> >
> > See https://patchwork.ozlabs.org/patch/1479615
>
> IIUC, such errors due to the fact that the compatible used in the example
> is not in any yaml schema(as it is still in the old txt format). I also
> assume such errors are allowed until the transition is complete and I
> need not fix anything as part of this patch ?

Not allowed because I can't turn this check on by default until we get
rid of the existing 80 or so. But it is a new check and Viresh already
applied, so oh well.

BTW, one of the 80 is 'operating-points-v2' compatible. Hint, hint.

Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-20 19:43     ` Rob Herring
@ 2021-05-21  4:08       ` Viresh Kumar
  2021-05-21 15:24         ` Sudeep Holla
  0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2021-05-21  4:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: Sudeep Holla, linux-kernel, Bjorn Andersson, Hector Yuan,
	Manivannan Sadhasivam, devicetree

On 20-05-21, 14:43, Rob Herring wrote:
> Not allowed because I can't turn this check on by default until we get
> rid of the existing 80 or so. But it is a new check and Viresh already
> applied, so oh well.

I can always drop it :)

-- 
viresh

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-21  4:08       ` Viresh Kumar
@ 2021-05-21 15:24         ` Sudeep Holla
  2021-05-24  9:17           ` Viresh Kumar
  0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2021-05-21 15:24 UTC (permalink / raw)
  To: Viresh Kumar, Rob Herring
  Cc: linux-kernel, Sudeep Holla, Bjorn Andersson, Hector Yuan,
	Manivannan Sadhasivam, devicetree

On Fri, May 21, 2021 at 09:38:34AM +0530, Viresh Kumar wrote:
> On 20-05-21, 14:43, Rob Herring wrote:
> > Not allowed because I can't turn this check on by default until we get
> > rid of the existing 80 or so. But it is a new check and Viresh already
> > applied, so oh well.
>
> I can always drop it :)
>

While I really don't care(evident by rate at which I worked on this 😉)
I think Hector Yuan won't be happy to wait I guess. As a quick fix, you
can update "qcom,cpufreq-hw" to "mediatek,cpufreq-hw". You will still
get warning with this patch + update alone, but once you have Hector's
mediatek cpufreq driver applied, the warnings must disappear.

What do you think ?

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-21 15:24         ` Sudeep Holla
@ 2021-05-24  9:17           ` Viresh Kumar
  2021-05-24 10:05             ` Sudeep Holla
  0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2021-05-24  9:17 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Rob Herring, linux-kernel, Bjorn Andersson, Hector Yuan,
	Manivannan Sadhasivam, devicetree

On 21-05-21, 16:24, Sudeep Holla wrote:
> On Fri, May 21, 2021 at 09:38:34AM +0530, Viresh Kumar wrote:
> > On 20-05-21, 14:43, Rob Herring wrote:
> > > Not allowed because I can't turn this check on by default until we get
> > > rid of the existing 80 or so. But it is a new check and Viresh already
> > > applied, so oh well.
> >
> > I can always drop it :)
> >
> 
> While I really don't care(evident by rate at which I worked on this 😉)
> I think Hector Yuan won't be happy to wait I guess. As a quick fix, you
> can update "qcom,cpufreq-hw" to "mediatek,cpufreq-hw". You will still
> get warning with this patch + update alone, but once you have Hector's
> mediatek cpufreq driver applied, the warnings must disappear.
> 
> What do you think ?

I guess you can send a fix later then.

-- 
viresh

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-24  9:17           ` Viresh Kumar
@ 2021-05-24 10:05             ` Sudeep Holla
  0 siblings, 0 replies; 24+ messages in thread
From: Sudeep Holla @ 2021-05-24 10:05 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rob Herring, Sudeep Holla, linux-kernel, Bjorn Andersson,
	Hector Yuan, Manivannan Sadhasivam, devicetree

On Mon, May 24, 2021 at 02:47:15PM +0530, Viresh Kumar wrote:
> On 21-05-21, 16:24, Sudeep Holla wrote:
> > On Fri, May 21, 2021 at 09:38:34AM +0530, Viresh Kumar wrote:
> > > On 20-05-21, 14:43, Rob Herring wrote:
> > > > Not allowed because I can't turn this check on by default until we get
> > > > rid of the existing 80 or so. But it is a new check and Viresh already
> > > > applied, so oh well.
> > >
> > > I can always drop it :)
> > >
> > 
> > While I really don't care(evident by rate at which I worked on this 😉)
> > I think Hector Yuan won't be happy to wait I guess. As a quick fix, you
> > can update "qcom,cpufreq-hw" to "mediatek,cpufreq-hw". You will still
> > get warning with this patch + update alone, but once you have Hector's
> > mediatek cpufreq driver applied, the warnings must disappear.
> > 
> > What do you think ?
> 
> I guess you can send a fix later then.

Sure 😄

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-05-17 15:54 [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains Sudeep Holla
  2021-05-17 19:17 ` Rob Herring
  2021-05-17 20:45 ` Rob Herring
@ 2021-10-14 10:56 ` Ulf Hansson
  2021-10-14 14:55   ` Sudeep Holla
  2 siblings, 1 reply; 24+ messages in thread
From: Ulf Hansson @ 2021-10-14 10:56 UTC (permalink / raw)
  To: Sudeep Holla, Rob Herring, Viresh Kumar
  Cc: Linux Kernel Mailing List, DTML, Manivannan Sadhasivam,
	Hector Yuan, Bjorn Andersson, Rob Herring

On Mon, 17 May 2021 at 18:14, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> implementations where untrusted software had direct access to clock and
> voltage hardware controls. In this attack, the malicious software was able to
> place the platform into unsafe overclocked or undervolted configurations. Such
> configurations then enabled the injection of predictable faults to reveal
> secrets.
>
> Many Arm-based systems used to or still use voltage regulator and clock
> frameworks in the kernel. These frameworks allow callers to independently
> manipulate frequency and voltage settings. Such implementations can render
> systems susceptible to this form of attack.
>
> Attacks such as CLKSCREW are now being mitigated by not having direct and
> independent control of clock and voltage in the kernel and moving that
> control to a trusted entity, such as the SCP firmware or secure world
> firmware/software which are to perform sanity checking on the requested
> performance levels, thereby preventing any attempted malicious programming.
>
> With the advent of such an abstraction, there is a need to replace the
> generic clock and regulator bindings used by such devices with a generic
> performance domains bindings.
>
> [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
>
> Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> Cc: Rob Herring <robh+dt@kernel.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Hi Sudeep/Viresh/Rob,

I noticed this binding recently got accepted, so I guess I have missed
the opportunity to provide you with a few comments.

In any case, I would like to ask a few questions. In particular, am I
trying to understand why the power-domains bindings [1] can't be used
for this?

The power-domains are capable of dealing with "performance" through
the "operating-points-v2" DT property, which maps to the generic OPP
bindings [2]. I wonder why that isn't sufficient here? Can you please
elaborate?

[1]
Documentation/devicetree/bindings/power/power-domain.yaml

[2]
Documentation/devicetree/bindings/opp/opp-v2-base.yaml

Kind regards
Uffe

> ---
> Hi All,
>
> Sorry for yet another delay, I don't want to mist this for v5.14 as Mediatek
> cpufreq driver was depending on this IIRC.
>
> v3[3]->v4:
>         - Dropped unnecessary phandle-array reference
>         - Added maxItems = 1 for the property
>
> v2[2]->v3[3]:
>         - Dropped required properties
>         - Added non cpu device example
>         - Updated cpu bindings too
>
> v1[1]->v2[2]:
>         - Changed to Dual License
>         - Added select: true, enum for #performance-domain-cells and
>           $ref for performance-domain
>         - Changed the example to use real existing compatibles instead
>           of made-up ones
>
> Regards,
> Sudeep
>
> [1] https://lore.kernel.org/r/20201105173539.1426301-1-sudeep.holla@arm.com
> [2] https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> [3] https://lore.kernel.org/r/20210407135913.2067694-1-sudeep.holla@arm.com
>
>  .../devicetree/bindings/arm/cpus.yaml         |  7 ++
>  .../bindings/dvfs/performance-domain.yaml     | 74 +++++++++++++++++++
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dvfs/performance-domain.yaml
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> index f3c7249c73d6..9a2432a88074 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> @@ -257,6 +257,13 @@ description: |+
>
>        where voltage is in V, frequency is in MHz.
>
> +  performance-domains:
> +    maxItems: 1
> +    description:
> +      List of phandles and performance domain specifiers, as defined by
> +      bindings of the performance domain provider. See also
> +      dvfs/performance-domain.yaml.
> +
>    power-domains:
>      description:
>        List of phandles and PM domain specifiers, as defined by bindings of the
> diff --git a/Documentation/devicetree/bindings/dvfs/performance-domain.yaml b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
> new file mode 100644
> index 000000000000..c8b91207f34d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dvfs/performance-domain.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic performance domains
> +
> +maintainers:
> +  - Sudeep Holla <sudeep.holla@arm.com>
> +
> +description: |+
> +  This binding is intended for performance management of groups of devices or
> +  CPUs that run in the same performance domain. Performance domains must not
> +  be confused with power domains. A performance domain is defined by a set
> +  of devices that always have to run at the same performance level. For a given
> +  performance domain, there is a single point of control that affects all the
> +  devices in the domain, making it impossible to set the performance level of
> +  an individual device in the domain independently from other devices in
> +  that domain. For example, a set of CPUs that share a voltage domain, and
> +  have a common frequency control, is said to be in the same performance
> +  domain.
> +
> +  This device tree binding can be used to bind performance domain consumer
> +  devices with their performance domains provided by performance domain
> +  providers. A performance domain provider can be represented by any node in
> +  the device tree and can provide one or more performance domains. A consumer
> +  node can refer to the provider by a phandle and a set of phandle arguments
> +  (so called performance domain specifiers) of length specified by the
> +  \#performance-domain-cells property in the performance domain provider node.
> +
> +select: true
> +
> +properties:
> +  "#performance-domain-cells":
> +    description:
> +      Number of cells in a performance domain specifier. Typically 0 for nodes
> +      representing a single performance domain and 1 for nodes providing
> +      multiple performance domains (e.g. performance controllers), but can be
> +      any value as specified by device tree binding documentation of particular
> +      provider.
> +    enum: [ 0, 1 ]
> +
> +  performance-domains:
> +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> +    maxItems: 1
> +    description:
> +      A phandle and performance domain specifier as defined by bindings of the
> +      performance controller/provider specified by phandle.
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    performance: performance-controller@12340000 {
> +        compatible = "qcom,cpufreq-hw";
> +        reg = <0x12340000 0x1000>;
> +        #performance-domain-cells = <1>;
> +    };
> +
> +    // The node above defines a performance controller that is a performance
> +    // domain provider and expects one cell as its phandle argument.
> +
> +    cpus {
> +        #address-cells = <2>;
> +        #size-cells = <0>;
> +
> +        cpu@0 {
> +            device_type = "cpu";
> +            compatible = "arm,cortex-a57";
> +            reg = <0x0 0x0>;
> +            performance-domains = <&performance 1>;
> +        };
> +    };
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-14 10:56 ` Ulf Hansson
@ 2021-10-14 14:55   ` Sudeep Holla
  2021-10-15  9:17     ` Ulf Hansson
  0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2021-10-14 14:55 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rob Herring, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Sudeep Holla, Hector Yuan,
	Bjorn Andersson, Rob Herring

On Thu, Oct 14, 2021 at 12:56:46PM +0200, Ulf Hansson wrote:
> On Mon, 17 May 2021 at 18:14, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> > implementations where untrusted software had direct access to clock and
> > voltage hardware controls. In this attack, the malicious software was able to
> > place the platform into unsafe overclocked or undervolted configurations. Such
> > configurations then enabled the injection of predictable faults to reveal
> > secrets.
> >
> > Many Arm-based systems used to or still use voltage regulator and clock
> > frameworks in the kernel. These frameworks allow callers to independently
> > manipulate frequency and voltage settings. Such implementations can render
> > systems susceptible to this form of attack.
> >
> > Attacks such as CLKSCREW are now being mitigated by not having direct and
> > independent control of clock and voltage in the kernel and moving that
> > control to a trusted entity, such as the SCP firmware or secure world
> > firmware/software which are to perform sanity checking on the requested
> > performance levels, thereby preventing any attempted malicious programming.
> >
> > With the advent of such an abstraction, there is a need to replace the
> > generic clock and regulator bindings used by such devices with a generic
> > performance domains bindings.
> >
> > [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
> >
> > Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Hi Sudeep/Viresh/Rob,
>
> I noticed this binding recently got accepted, so I guess I have missed
> the opportunity to provide you with a few comments.
>

Sorry for not cc-ing you, wasn't aware of the below mentioned intersection,
so assumed you are not one of the interested parties.

> In any case, I would like to ask a few questions. In particular, am I
> trying to understand why the power-domains bindings [1] can't be used
> for this?
>

One reason I can think of is on some platforms, the power domains are
completely controlled by the firmware and not exposed to the OSPM.
This is mostly applicable for CPU devices(Platform co-ordinated PSCI)

> The power-domains are capable of dealing with "performance" through
> the "operating-points-v2" DT property, which maps to the generic OPP
> bindings [2]. I wonder why that isn't sufficient here? Can you please
> elaborate?
>

Even if the power domains are exposed to the OSPM, the OPPs can be
firmware enumerated rather than DT. Not sure if it is possible to
represent such systems in the above mentioned bindings. IIUC, the genpd
uses clock and regulator apis to drive the performance, but these
platforms have f/w interface to drive the OPPs(abstracted).

I am happy to know if there are ways to support such systems with the
options you have mentioned above.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-14 14:55   ` Sudeep Holla
@ 2021-10-15  9:17     ` Ulf Hansson
  2021-10-19  7:24       ` Viresh Kumar
                         ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Ulf Hansson @ 2021-10-15  9:17 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Rob Herring, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson, Rob Herring

On Thu, 14 Oct 2021 at 16:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 14, 2021 at 12:56:46PM +0200, Ulf Hansson wrote:
> > On Mon, 17 May 2021 at 18:14, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> > > implementations where untrusted software had direct access to clock and
> > > voltage hardware controls. In this attack, the malicious software was able to
> > > place the platform into unsafe overclocked or undervolted configurations. Such
> > > configurations then enabled the injection of predictable faults to reveal
> > > secrets.
> > >
> > > Many Arm-based systems used to or still use voltage regulator and clock
> > > frameworks in the kernel. These frameworks allow callers to independently
> > > manipulate frequency and voltage settings. Such implementations can render
> > > systems susceptible to this form of attack.
> > >
> > > Attacks such as CLKSCREW are now being mitigated by not having direct and
> > > independent control of clock and voltage in the kernel and moving that
> > > control to a trusted entity, such as the SCP firmware or secure world
> > > firmware/software which are to perform sanity checking on the requested
> > > performance levels, thereby preventing any attempted malicious programming.
> > >
> > > With the advent of such an abstraction, there is a need to replace the
> > > generic clock and regulator bindings used by such devices with a generic
> > > performance domains bindings.
> > >
> > > [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
> > >
> > > Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >
> > Hi Sudeep/Viresh/Rob,
> >
> > I noticed this binding recently got accepted, so I guess I have missed
> > the opportunity to provide you with a few comments.
> >
>
> Sorry for not cc-ing you, wasn't aware of the below mentioned intersection,
> so assumed you are not one of the interested parties.
>
> > In any case, I would like to ask a few questions. In particular, am I
> > trying to understand why the power-domains bindings [1] can't be used
> > for this?
> >
>
> One reason I can think of is on some platforms, the power domains are
> completely controlled by the firmware and not exposed to the OSPM.
> This is mostly applicable for CPU devices(Platform co-ordinated PSCI)

See below.

>
> > The power-domains are capable of dealing with "performance" through
> > the "operating-points-v2" DT property, which maps to the generic OPP
> > bindings [2]. I wonder why that isn't sufficient here? Can you please
> > elaborate?
> >
>
> Even if the power domains are exposed to the OSPM, the OPPs can be
> firmware enumerated rather than DT. Not sure if it is possible to
> represent such systems in the above mentioned bindings. IIUC, the genpd
> uses clock and regulator apis to drive the performance, but these
> platforms have f/w interface to drive the OPPs(abstracted).

Genpd doesn't operate on clock rates or voltage levels. Instead
"performance" is just an integer value for genpd. What a performance
index means, is genpd provider specific.

In other words, it becomes the responsibility for the genpd provider
to map a performance state index to an OPP, for example. So far,
providers have used the generic OPP DT bindings to do this, but for
sure, we don't have to limit ourselves to this. So, if OPP tables can
be enumerated by FW, rather than specified in DT, that should
certainly be possible to support.

BTW, these are genpd provider callbacks, that needs to be implemented
to let it control performance. Perhaps that helps to understand
things.

int (*set_performance_state)(struct generic_pm_domain *genpd, unsigned
int state);
unsigned int (*opp_to_performance_state)(struct generic_pm_domain
*genpd, struct dev_pm_opp *opp);

>
> I am happy to know if there are ways to support such systems with the
> options you have mentioned above.

As far as I understand, the "performance domains" DT bindings that
$subject patch introduces, allows us to group devices into domains, to
let them be "performance controlled" together. Right?

Unless I am missing something, it looks like power domains DT bindings
already offer this for us. Yes, certainly, the DT doc [1] needs an
updated description to better explain this, but other than that we
should be fine, don't you think?

[1]
Documentation/devicetree/bindings/power/power-domain.yaml

>
> --
> Regards,
> Sudeep

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-15  9:17     ` Ulf Hansson
@ 2021-10-19  7:24       ` Viresh Kumar
  2021-10-19 13:58         ` Ulf Hansson
  2021-10-20 10:25       ` Sudeep Holla
  2021-10-20 12:11       ` Rob Herring
  2 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2021-10-19  7:24 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Sudeep Holla, Rob Herring, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson, Rob Herring

On 15-10-21, 11:17, Ulf Hansson wrote:
> As far as I understand, the "performance domains" DT bindings that
> $subject patch introduces, allows us to group devices into domains, to
> let them be "performance controlled" together. Right?

This and it also provides a reg space where we can get/set the
performance state.

> Unless I am missing something, it looks like power domains DT bindings
> already offer this for us. Yes, certainly, the DT doc [1] needs an
> updated description to better explain this, but other than that we
> should be fine, don't you think?

I think yes we can make it work through that as well, but I am not
sure if we will be able to use required-opp n stuff here as the DT
doesn't have the OPP table for the CPUs.

The CPU's freq table is generated at runtime, see
drivers/cpufreq/mediatek-cpufreq-hw.c for example.

-- 
viresh

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-19  7:24       ` Viresh Kumar
@ 2021-10-19 13:58         ` Ulf Hansson
  2021-10-20  6:24           ` Viresh Kumar
  0 siblings, 1 reply; 24+ messages in thread
From: Ulf Hansson @ 2021-10-19 13:58 UTC (permalink / raw)
  To: Viresh Kumar, Sudeep Holla
  Cc: Rob Herring, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson, Rob Herring

On Tue, 19 Oct 2021 at 09:24, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 15-10-21, 11:17, Ulf Hansson wrote:
> > As far as I understand, the "performance domains" DT bindings that
> > $subject patch introduces, allows us to group devices into domains, to
> > let them be "performance controlled" together. Right?
>
> This and it also provides a reg space where we can get/set the
> performance state.

Right. So that can be done for power-domains too.

>
> > Unless I am missing something, it looks like power domains DT bindings
> > already offer this for us. Yes, certainly, the DT doc [1] needs an
> > updated description to better explain this, but other than that we
> > should be fine, don't you think?
>
> I think yes we can make it work through that as well, but I am not
> sure if we will be able to use required-opp n stuff here as the DT
> doesn't have the OPP table for the CPUs.
>
> The CPU's freq table is generated at runtime, see
> drivers/cpufreq/mediatek-cpufreq-hw.c for example.

To me, this looks doable from a genpd provider too. Of course, we may
need to extend the genpd interface a bit to make it fit well for this
new use case, of course. And I am happy to help, if that is needed.

One thing though; how is the aggregation of the OPP votes expected to
be done? Is that entirely managed by FW - or is it expected that the
cpufreq driver, in this case, keeps track of the aggregated votes too?

Don't get me wrong, I am not pushing for these DT bindings to be
deprecated (at least not yet :-)), but I would certainly like to
understand more about them. In the end, we haven't walked this far, by
extending genpd and inventing new DT bindings to enable it to support
"performance management" - then just to just forget about them again.
:-)

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-19 13:58         ` Ulf Hansson
@ 2021-10-20  6:24           ` Viresh Kumar
  0 siblings, 0 replies; 24+ messages in thread
From: Viresh Kumar @ 2021-10-20  6:24 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Sudeep Holla, Rob Herring, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson, Rob Herring

On 19-10-21, 15:58, Ulf Hansson wrote:
> To me, this looks doable from a genpd provider too. Of course, we may
> need to extend the genpd interface a bit to make it fit well for this
> new use case, of course. And I am happy to help, if that is needed.
> 
> One thing though; how is the aggregation of the OPP votes expected to
> be done? Is that entirely managed by FW - or is it expected that the
> cpufreq driver, in this case, keeps track of the aggregated votes too?

In case of cpufreq drivers, the voting is never required since there
is always a single request from cpufreq core to change the freq for a
group of devices (CPUs). And these genpds will only have a bunch of
CPUs to serve.

> Don't get me wrong, I am not pushing for these DT bindings to be
> deprecated (at least not yet :-)), but I would certainly like to
> understand more about them. In the end, we haven't walked this far, by
> extending genpd and inventing new DT bindings to enable it to support
> "performance management" - then just to just forget about them again.
> :-)

I am happy to move to genpd if this can be done in a sane way there :)

-- 
viresh

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-15  9:17     ` Ulf Hansson
  2021-10-19  7:24       ` Viresh Kumar
@ 2021-10-20 10:25       ` Sudeep Holla
  2021-10-21 13:34         ` Ulf Hansson
  2021-10-20 12:11       ` Rob Herring
  2 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2021-10-20 10:25 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rob Herring, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson, Rob Herring

On Fri, Oct 15, 2021 at 11:17:18AM +0200, Ulf Hansson wrote:
> On Thu, 14 Oct 2021 at 16:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 14, 2021 at 12:56:46PM +0200, Ulf Hansson wrote:
> > > On Mon, 17 May 2021 at 18:14, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> > > > implementations where untrusted software had direct access to clock and
> > > > voltage hardware controls. In this attack, the malicious software was able to
> > > > place the platform into unsafe overclocked or undervolted configurations. Such
> > > > configurations then enabled the injection of predictable faults to reveal
> > > > secrets.
> > > >
> > > > Many Arm-based systems used to or still use voltage regulator and clock
> > > > frameworks in the kernel. These frameworks allow callers to independently
> > > > manipulate frequency and voltage settings. Such implementations can render
> > > > systems susceptible to this form of attack.
> > > >
> > > > Attacks such as CLKSCREW are now being mitigated by not having direct and
> > > > independent control of clock and voltage in the kernel and moving that
> > > > control to a trusted entity, such as the SCP firmware or secure world
> > > > firmware/software which are to perform sanity checking on the requested
> > > > performance levels, thereby preventing any attempted malicious programming.
> > > >
> > > > With the advent of such an abstraction, there is a need to replace the
> > > > generic clock and regulator bindings used by such devices with a generic
> > > > performance domains bindings.
> > > >
> > > > [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
> > > >
> > > > Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > >
> > > Hi Sudeep/Viresh/Rob,
> > >
> > > I noticed this binding recently got accepted, so I guess I have missed
> > > the opportunity to provide you with a few comments.
> > >
> >
> > Sorry for not cc-ing you, wasn't aware of the below mentioned intersection,
> > so assumed you are not one of the interested parties.
> >
> > > In any case, I would like to ask a few questions. In particular, am I
> > > trying to understand why the power-domains bindings [1] can't be used
> > > for this?
> > >
> >
> > One reason I can think of is on some platforms, the power domains are
> > completely controlled by the firmware and not exposed to the OSPM.
> > This is mostly applicable for CPU devices(Platform co-ordinated PSCI)
> 
> See below.
> 
> >
> > > The power-domains are capable of dealing with "performance" through
> > > the "operating-points-v2" DT property, which maps to the generic OPP
> > > bindings [2]. I wonder why that isn't sufficient here? Can you please
> > > elaborate?
> > >
> >
> > Even if the power domains are exposed to the OSPM, the OPPs can be
> > firmware enumerated rather than DT. Not sure if it is possible to
> > represent such systems in the above mentioned bindings. IIUC, the genpd
> > uses clock and regulator apis to drive the performance, but these
> > platforms have f/w interface to drive the OPPs(abstracted).
> 
> Genpd doesn't operate on clock rates or voltage levels. Instead
> "performance" is just an integer value for genpd. What a performance
> index means, is genpd provider specific.
>

Understood.

> In other words, it becomes the responsibility for the genpd provider
> to map a performance state index to an OPP, for example. So far,
> providers have used the generic OPP DT bindings to do this, but for
> sure, we don't have to limit ourselves to this. So, if OPP tables can
> be enumerated by FW, rather than specified in DT, that should
> certainly be possible to support.
>
> BTW, these are genpd provider callbacks, that needs to be implemented
> to let it control performance. Perhaps that helps to understand
> things.
>
> int (*set_performance_state)(struct generic_pm_domain *genpd, unsigned
> int state);
> unsigned int (*opp_to_performance_state)(struct generic_pm_domain
> *genpd, struct dev_pm_opp *opp);
>

Looks like this can be used for devices but what about CPUs ?

> >
> > I am happy to know if there are ways to support such systems with the
> > options you have mentioned above.
>
> As far as I understand, the "performance domains" DT bindings that
> $subject patch introduces, allows us to group devices into domains, to
> let them be "performance controlled" together. Right?
>

Or independently. It doesn't matter.

> Unless I am missing something, it looks like power domains DT bindings
> already offer this for us. Yes, certainly, the DT doc [1] needs an
> updated description to better explain this, but other than that we
> should be fine, don't you think?
>

As I mentioned about, the main question is what if firmware doesn't
want to expose power domain details to OSPM like PC co-ordinated PSCI
idle states while it wants to either group CPUs or leave them as
individual in order to get per-CPU DVFS requests and aggregate them
in the firmware. It does something similar for idle states already.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-15  9:17     ` Ulf Hansson
  2021-10-19  7:24       ` Viresh Kumar
  2021-10-20 10:25       ` Sudeep Holla
@ 2021-10-20 12:11       ` Rob Herring
  2021-10-21 13:13         ` Ulf Hansson
  2 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2021-10-20 12:11 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Sudeep Holla, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson

On Fri, Oct 15, 2021 at 4:17 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 14 Oct 2021 at 16:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 14, 2021 at 12:56:46PM +0200, Ulf Hansson wrote:
> > > On Mon, 17 May 2021 at 18:14, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> > > > implementations where untrusted software had direct access to clock and
> > > > voltage hardware controls. In this attack, the malicious software was able to
> > > > place the platform into unsafe overclocked or undervolted configurations. Such
> > > > configurations then enabled the injection of predictable faults to reveal
> > > > secrets.
> > > >
> > > > Many Arm-based systems used to or still use voltage regulator and clock
> > > > frameworks in the kernel. These frameworks allow callers to independently
> > > > manipulate frequency and voltage settings. Such implementations can render
> > > > systems susceptible to this form of attack.
> > > >
> > > > Attacks such as CLKSCREW are now being mitigated by not having direct and
> > > > independent control of clock and voltage in the kernel and moving that
> > > > control to a trusted entity, such as the SCP firmware or secure world
> > > > firmware/software which are to perform sanity checking on the requested
> > > > performance levels, thereby preventing any attempted malicious programming.
> > > >
> > > > With the advent of such an abstraction, there is a need to replace the
> > > > generic clock and regulator bindings used by such devices with a generic
> > > > performance domains bindings.
> > > >
> > > > [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
> > > >
> > > > Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > >
> > > Hi Sudeep/Viresh/Rob,
> > >
> > > I noticed this binding recently got accepted, so I guess I have missed
> > > the opportunity to provide you with a few comments.
> > >
> >
> > Sorry for not cc-ing you, wasn't aware of the below mentioned intersection,
> > so assumed you are not one of the interested parties.
> >
> > > In any case, I would like to ask a few questions. In particular, am I
> > > trying to understand why the power-domains bindings [1] can't be used
> > > for this?
> > >
> >
> > One reason I can think of is on some platforms, the power domains are
> > completely controlled by the firmware and not exposed to the OSPM.
> > This is mostly applicable for CPU devices(Platform co-ordinated PSCI)
>
> See below.
>
> >
> > > The power-domains are capable of dealing with "performance" through
> > > the "operating-points-v2" DT property, which maps to the generic OPP
> > > bindings [2]. I wonder why that isn't sufficient here? Can you please
> > > elaborate?
> > >
> >
> > Even if the power domains are exposed to the OSPM, the OPPs can be
> > firmware enumerated rather than DT. Not sure if it is possible to
> > represent such systems in the above mentioned bindings. IIUC, the genpd
> > uses clock and regulator apis to drive the performance, but these
> > platforms have f/w interface to drive the OPPs(abstracted).
>
> Genpd doesn't operate on clock rates or voltage levels. Instead
> "performance" is just an integer value for genpd. What a performance
> index means, is genpd provider specific.
>
> In other words, it becomes the responsibility for the genpd provider
> to map a performance state index to an OPP, for example. So far,
> providers have used the generic OPP DT bindings to do this, but for
> sure, we don't have to limit ourselves to this. So, if OPP tables can
> be enumerated by FW, rather than specified in DT, that should
> certainly be possible to support.
>
> BTW, these are genpd provider callbacks, that needs to be implemented
> to let it control performance. Perhaps that helps to understand
> things.
>
> int (*set_performance_state)(struct generic_pm_domain *genpd, unsigned
> int state);
> unsigned int (*opp_to_performance_state)(struct generic_pm_domain
> *genpd, struct dev_pm_opp *opp);
>
> >
> > I am happy to know if there are ways to support such systems with the
> > options you have mentioned above.
>
> As far as I understand, the "performance domains" DT bindings that
> $subject patch introduces, allows us to group devices into domains, to
> let them be "performance controlled" together. Right?
>
> Unless I am missing something, it looks like power domains DT bindings
> already offer this for us. Yes, certainly, the DT doc [1] needs an
> updated description to better explain this, but other than that we
> should be fine, don't you think?

'power domains' in DT is supposed to mean physical power islands in
the h/w where as genpd can be whatever you want. Are power and
performance domains always 1:1?

Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-20 12:11       ` Rob Herring
@ 2021-10-21 13:13         ` Ulf Hansson
  2021-10-21 13:33           ` Sudeep Holla
  0 siblings, 1 reply; 24+ messages in thread
From: Ulf Hansson @ 2021-10-21 13:13 UTC (permalink / raw)
  To: Rob Herring
  Cc: Sudeep Holla, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson

On Wed, 20 Oct 2021 at 14:11, Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 15, 2021 at 4:17 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 14 Oct 2021 at 16:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Oct 14, 2021 at 12:56:46PM +0200, Ulf Hansson wrote:
> > > > On Mon, 17 May 2021 at 18:14, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> > > > > implementations where untrusted software had direct access to clock and
> > > > > voltage hardware controls. In this attack, the malicious software was able to
> > > > > place the platform into unsafe overclocked or undervolted configurations. Such
> > > > > configurations then enabled the injection of predictable faults to reveal
> > > > > secrets.
> > > > >
> > > > > Many Arm-based systems used to or still use voltage regulator and clock
> > > > > frameworks in the kernel. These frameworks allow callers to independently
> > > > > manipulate frequency and voltage settings. Such implementations can render
> > > > > systems susceptible to this form of attack.
> > > > >
> > > > > Attacks such as CLKSCREW are now being mitigated by not having direct and
> > > > > independent control of clock and voltage in the kernel and moving that
> > > > > control to a trusted entity, such as the SCP firmware or secure world
> > > > > firmware/software which are to perform sanity checking on the requested
> > > > > performance levels, thereby preventing any attempted malicious programming.
> > > > >
> > > > > With the advent of such an abstraction, there is a need to replace the
> > > > > generic clock and regulator bindings used by such devices with a generic
> > > > > performance domains bindings.
> > > > >
> > > > > [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
> > > > >
> > > > > Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > >
> > > > Hi Sudeep/Viresh/Rob,
> > > >
> > > > I noticed this binding recently got accepted, so I guess I have missed
> > > > the opportunity to provide you with a few comments.
> > > >
> > >
> > > Sorry for not cc-ing you, wasn't aware of the below mentioned intersection,
> > > so assumed you are not one of the interested parties.
> > >
> > > > In any case, I would like to ask a few questions. In particular, am I
> > > > trying to understand why the power-domains bindings [1] can't be used
> > > > for this?
> > > >
> > >
> > > One reason I can think of is on some platforms, the power domains are
> > > completely controlled by the firmware and not exposed to the OSPM.
> > > This is mostly applicable for CPU devices(Platform co-ordinated PSCI)
> >
> > See below.
> >
> > >
> > > > The power-domains are capable of dealing with "performance" through
> > > > the "operating-points-v2" DT property, which maps to the generic OPP
> > > > bindings [2]. I wonder why that isn't sufficient here? Can you please
> > > > elaborate?
> > > >
> > >
> > > Even if the power domains are exposed to the OSPM, the OPPs can be
> > > firmware enumerated rather than DT. Not sure if it is possible to
> > > represent such systems in the above mentioned bindings. IIUC, the genpd
> > > uses clock and regulator apis to drive the performance, but these
> > > platforms have f/w interface to drive the OPPs(abstracted).
> >
> > Genpd doesn't operate on clock rates or voltage levels. Instead
> > "performance" is just an integer value for genpd. What a performance
> > index means, is genpd provider specific.
> >
> > In other words, it becomes the responsibility for the genpd provider
> > to map a performance state index to an OPP, for example. So far,
> > providers have used the generic OPP DT bindings to do this, but for
> > sure, we don't have to limit ourselves to this. So, if OPP tables can
> > be enumerated by FW, rather than specified in DT, that should
> > certainly be possible to support.
> >
> > BTW, these are genpd provider callbacks, that needs to be implemented
> > to let it control performance. Perhaps that helps to understand
> > things.
> >
> > int (*set_performance_state)(struct generic_pm_domain *genpd, unsigned
> > int state);
> > unsigned int (*opp_to_performance_state)(struct generic_pm_domain
> > *genpd, struct dev_pm_opp *opp);
> >
> > >
> > > I am happy to know if there are ways to support such systems with the
> > > options you have mentioned above.
> >
> > As far as I understand, the "performance domains" DT bindings that
> > $subject patch introduces, allows us to group devices into domains, to
> > let them be "performance controlled" together. Right?
> >
> > Unless I am missing something, it looks like power domains DT bindings
> > already offer this for us. Yes, certainly, the DT doc [1] needs an
> > updated description to better explain this, but other than that we
> > should be fine, don't you think?
>
> 'power domains' in DT is supposed to mean physical power islands in
> the h/w where as genpd can be whatever you want. Are power and
> performance domains always 1:1?

I wouldn't say that "power domains" *must* correspond to physical
power islands. At least, that's not the way the bindings are being
used. For example, if it makes better sense to keep some of the logic
in FW, rather than describing a complete topology in DT, that should
be perfectly fine.

Additionally, I am not suggesting that "performance domains" and
"power domains" must map 1:1. A device can be performance managed
through one domain and power managed by another, that would be
perfectly fine to me.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-21 13:13         ` Ulf Hansson
@ 2021-10-21 13:33           ` Sudeep Holla
  2021-10-21 16:01             ` Ulf Hansson
  0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2021-10-21 13:33 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rob Herring, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Sudeep Holla, Manivannan Sadhasivam, Hector Yuan,
	Bjorn Andersson

On Thu, Oct 21, 2021 at 03:13:30PM +0200, Ulf Hansson wrote:
> On Wed, 20 Oct 2021 at 14:11, Rob Herring <robh@kernel.org> wrote:
> >

[...]

> > 'power domains' in DT is supposed to mean physical power islands in
> > the h/w where as genpd can be whatever you want. Are power and
> > performance domains always 1:1?
>
> I wouldn't say that "power domains" *must* correspond to physical
> power islands. At least, that's not the way the bindings are being
> used. For example, if it makes better sense to keep some of the logic
> in FW, rather than describing a complete topology in DT, that should
> be perfectly fine.
>

I agree. The DT must either have h/w view or f/w view of the topology
and not both(that is inviting more trouble in terms of bindings as well
as handling it in the OSPM).

> Additionally, I am not suggesting that "performance domains" and
> "power domains" must map 1:1. A device can be performance managed
> through one domain and power managed by another, that would be
> perfectly fine to me.

I don't understand what you mean by that. Do you expect to create a genpd
with no power domain ops and just performance ops to deal with scenario
I have been presenting(i.e. power domains for a set of devices(CPUs in
particular) aren't exposed to OSPM while performance domains are).

I really don't like to create psuedo/dummy power domains with no useful
info(as f/w hides or abstracts it) just to represent the performance
domains.

Also with CPUs you can imagine all sort of combinations like:
1. cluster level perf domain + cpu level power domains
3. cluster level perf domain + cluster level power domains
2. cpu level perf domain + cpu level power domains
4. cpu level perf domain + cluster level power domains

+ power domains not available to OSPM in all the 4 combo.

So I am really struggling to visualise a way to represent these based
on your suggestion.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-20 10:25       ` Sudeep Holla
@ 2021-10-21 13:34         ` Ulf Hansson
  2021-10-21 15:35           ` Sudeep Holla
  0 siblings, 1 reply; 24+ messages in thread
From: Ulf Hansson @ 2021-10-21 13:34 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Rob Herring, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson, Rob Herring

On Wed, 20 Oct 2021 at 12:25, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Oct 15, 2021 at 11:17:18AM +0200, Ulf Hansson wrote:
> > On Thu, 14 Oct 2021 at 16:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Oct 14, 2021 at 12:56:46PM +0200, Ulf Hansson wrote:
> > > > On Mon, 17 May 2021 at 18:14, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > The CLKSCREW attack [0] exposed security vulnerabilities in energy management
> > > > > implementations where untrusted software had direct access to clock and
> > > > > voltage hardware controls. In this attack, the malicious software was able to
> > > > > place the platform into unsafe overclocked or undervolted configurations. Such
> > > > > configurations then enabled the injection of predictable faults to reveal
> > > > > secrets.
> > > > >
> > > > > Many Arm-based systems used to or still use voltage regulator and clock
> > > > > frameworks in the kernel. These frameworks allow callers to independently
> > > > > manipulate frequency and voltage settings. Such implementations can render
> > > > > systems susceptible to this form of attack.
> > > > >
> > > > > Attacks such as CLKSCREW are now being mitigated by not having direct and
> > > > > independent control of clock and voltage in the kernel and moving that
> > > > > control to a trusted entity, such as the SCP firmware or secure world
> > > > > firmware/software which are to perform sanity checking on the requested
> > > > > performance levels, thereby preventing any attempted malicious programming.
> > > > >
> > > > > With the advent of such an abstraction, there is a need to replace the
> > > > > generic clock and regulator bindings used by such devices with a generic
> > > > > performance domains bindings.
> > > > >
> > > > > [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
> > > > >
> > > > > Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
> > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > >
> > > > Hi Sudeep/Viresh/Rob,
> > > >
> > > > I noticed this binding recently got accepted, so I guess I have missed
> > > > the opportunity to provide you with a few comments.
> > > >
> > >
> > > Sorry for not cc-ing you, wasn't aware of the below mentioned intersection,
> > > so assumed you are not one of the interested parties.
> > >
> > > > In any case, I would like to ask a few questions. In particular, am I
> > > > trying to understand why the power-domains bindings [1] can't be used
> > > > for this?
> > > >
> > >
> > > One reason I can think of is on some platforms, the power domains are
> > > completely controlled by the firmware and not exposed to the OSPM.
> > > This is mostly applicable for CPU devices(Platform co-ordinated PSCI)
> >
> > See below.
> >
> > >
> > > > The power-domains are capable of dealing with "performance" through
> > > > the "operating-points-v2" DT property, which maps to the generic OPP
> > > > bindings [2]. I wonder why that isn't sufficient here? Can you please
> > > > elaborate?
> > > >
> > >
> > > Even if the power domains are exposed to the OSPM, the OPPs can be
> > > firmware enumerated rather than DT. Not sure if it is possible to
> > > represent such systems in the above mentioned bindings. IIUC, the genpd
> > > uses clock and regulator apis to drive the performance, but these
> > > platforms have f/w interface to drive the OPPs(abstracted).
> >
> > Genpd doesn't operate on clock rates or voltage levels. Instead
> > "performance" is just an integer value for genpd. What a performance
> > index means, is genpd provider specific.
> >
>
> Understood.
>
> > In other words, it becomes the responsibility for the genpd provider
> > to map a performance state index to an OPP, for example. So far,
> > providers have used the generic OPP DT bindings to do this, but for
> > sure, we don't have to limit ourselves to this. So, if OPP tables can
> > be enumerated by FW, rather than specified in DT, that should
> > certainly be possible to support.
> >
> > BTW, these are genpd provider callbacks, that needs to be implemented
> > to let it control performance. Perhaps that helps to understand
> > things.
> >
> > int (*set_performance_state)(struct generic_pm_domain *genpd, unsigned
> > int state);
> > unsigned int (*opp_to_performance_state)(struct generic_pm_domain
> > *genpd, struct dev_pm_opp *opp);
> >
>
> Looks like this can be used for devices but what about CPUs ?

Yes, that should work. dev_pm_genpd_set_performance_state() takes a
struct device* as an in-parameter.

The struct device to use would typically be the one that you receive
from dev_pm_domain_attach_by_name(). We already do this for some other
cpufreq drivers, so this works fine.

>
> > >
> > > I am happy to know if there are ways to support such systems with the
> > > options you have mentioned above.
> >
> > As far as I understand, the "performance domains" DT bindings that
> > $subject patch introduces, allows us to group devices into domains, to
> > let them be "performance controlled" together. Right?
> >
>
> Or independently. It doesn't matter.
>
> > Unless I am missing something, it looks like power domains DT bindings
> > already offer this for us. Yes, certainly, the DT doc [1] needs an
> > updated description to better explain this, but other than that we
> > should be fine, don't you think?
> >
>
> As I mentioned about, the main question is what if firmware doesn't
> want to expose power domain details to OSPM like PC co-ordinated PSCI
> idle states while it wants to either group CPUs or leave them as
> individual in order to get per-CPU DVFS requests and aggregate them
> in the firmware. It does something similar for idle states already.

Yes, that can be modeled too.

Just let each CPU node point to its own separate power-domain and also
*don't* model the parent power-domain, instead leave this to be
managed by the FW.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-21 13:34         ` Ulf Hansson
@ 2021-10-21 15:35           ` Sudeep Holla
  0 siblings, 0 replies; 24+ messages in thread
From: Sudeep Holla @ 2021-10-21 15:35 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rob Herring, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Sudeep Holla, Hector Yuan,
	Bjorn Andersson, Rob Herring

On Thu, Oct 21, 2021 at 03:34:17PM +0200, Ulf Hansson wrote:
> On Wed, 20 Oct 2021 at 12:25, Sudeep Holla <sudeep.holla@arm.com> wrote:

[...]

> >
> > Looks like this can be used for devices but what about CPUs ?
>
> Yes, that should work. dev_pm_genpd_set_performance_state() takes a
> struct device* as an in-parameter.
>
> The struct device to use would typically be the one that you receive
> from dev_pm_domain_attach_by_name(). We already do this for some other
> cpufreq drivers, so this works fine.
>

I totally understand this is possible but the question is: is it really
necessary ?

> >
> > > >
> > > > I am happy to know if there are ways to support such systems with the
> > > > options you have mentioned above.
> > >
> > > As far as I understand, the "performance domains" DT bindings that
> > > $subject patch introduces, allows us to group devices into domains, to
> > > let them be "performance controlled" together. Right?
> > >
> >
> > Or independently. It doesn't matter.
> >
> > > Unless I am missing something, it looks like power domains DT bindings
> > > already offer this for us. Yes, certainly, the DT doc [1] needs an
> > > updated description to better explain this, but other than that we
> > > should be fine, don't you think?
> > >
> >
> > As I mentioned about, the main question is what if firmware doesn't
> > want to expose power domain details to OSPM like PC co-ordinated PSCI
> > idle states while it wants to either group CPUs or leave them as
> > individual in order to get per-CPU DVFS requests and aggregate them
> > in the firmware. It does something similar for idle states already.
> 
> Yes, that can be modeled too.
> 
> Just let each CPU node point to its own separate power-domain and also
> *don't* model the parent power-domain, instead leave this to be
> managed by the FW.
>

Why ? IMO it is unnecessary indirection and useless as we don't need the
genpd created for these for no useful reasons. All we need is to get the
domain id for the device and request the firmware passing that id to set
the performance. Creating a psuedo power domain with no power domain
info except the performance domain id and then creating genpd domains
for the same just because we can is something I am failing to understand
here. I agree if the CPU domains were real(as in what f/w or h/w represents),
then may be, again I really don't like them disconnected from real CPU
power domains just because perf and power are not 1:1. Creating one set
of genpd for power and another for performance is also not okay IMO for
reasons stated above.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains
  2021-10-21 13:33           ` Sudeep Holla
@ 2021-10-21 16:01             ` Ulf Hansson
  0 siblings, 0 replies; 24+ messages in thread
From: Ulf Hansson @ 2021-10-21 16:01 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Rob Herring, Viresh Kumar, Linux Kernel Mailing List, DTML,
	Manivannan Sadhasivam, Hector Yuan, Bjorn Andersson

On Thu, 21 Oct 2021 at 15:33, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 21, 2021 at 03:13:30PM +0200, Ulf Hansson wrote:
> > On Wed, 20 Oct 2021 at 14:11, Rob Herring <robh@kernel.org> wrote:
> > >
>
> [...]
>
> > > 'power domains' in DT is supposed to mean physical power islands in
> > > the h/w where as genpd can be whatever you want. Are power and
> > > performance domains always 1:1?
> >
> > I wouldn't say that "power domains" *must* correspond to physical
> > power islands. At least, that's not the way the bindings are being
> > used. For example, if it makes better sense to keep some of the logic
> > in FW, rather than describing a complete topology in DT, that should
> > be perfectly fine.
> >
>
> I agree. The DT must either have h/w view or f/w view of the topology
> and not both(that is inviting more trouble in terms of bindings as well
> as handling it in the OSPM).
>
> > Additionally, I am not suggesting that "performance domains" and
> > "power domains" must map 1:1. A device can be performance managed
> > through one domain and power managed by another, that would be
> > perfectly fine to me.
>
> I don't understand what you mean by that. Do you expect to create a genpd
> with no power domain ops and just performance ops to deal with scenario
> I have been presenting(i.e. power domains for a set of devices(CPUs in
> particular) aren't exposed to OSPM while performance domains are).

Yes, but only if that would make sense, of course.

If it matters, the performance states and idle states in genpd are
supported as two orthogonal states.

>
> I really don't like to create psuedo/dummy power domains with no useful
> info(as f/w hides or abstracts it) just to represent the performance
> domains.

I assume you mean creating dummy *genpds* - and yes that seems very
silly, I agree.

We shouldn't do that, unless we can make use of them in some clever
way to avoid open coding, but that's an entirely different question,
unrelated to DT.

>
> Also with CPUs you can imagine all sort of combinations like:
> 1. cluster level perf domain + cpu level power domains
> 3. cluster level perf domain + cluster level power domains
> 2. cpu level perf domain + cpu level power domains
> 4. cpu level perf domain + cluster level power domains
>
> + power domains not available to OSPM in all the 4 combo.
>
> So I am really struggling to visualise a way to represent these based
> on your suggestion.

For perf domains, we could model this in DT as one power domain for
performance, per CPU, but without a cluster power domain for the
performance, as that seems to be managed in FW. Note that, this
doesn't mean we need to create genpds and hook up devices to them.

I guess this would map 1:1 towards how the "performance-domains"
binding is intended to be used, right?

One thing though, for PSCI we distinguish the power domains, by
specifying "power-domain-names = "psci" in DT. Similar to this, we
would then need to come up with another power-domain-name, to map it
to a "performance domain". Mostly to cover future compatibility
issues.

So, to summarize (thanks for a good discussion!), I will not insist on
deprecating the recently introduced "performance domains" binding. I
leave the call to you! However, to me, it still looks like the
power-domains binding could be used to support "performance" as well.
Especially, as we already have other cpufreq drivers [1] supporting
them.

Kind regards
Uffe

[1]
Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-10-21 16:02 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-17 15:54 [PATCH v4] dt-bindings: dvfs: Add support for generic performance domains Sudeep Holla
2021-05-17 19:17 ` Rob Herring
2021-05-19 11:23   ` Sudeep Holla
2021-05-20  3:54     ` Viresh Kumar
2021-05-17 20:45 ` Rob Herring
2021-05-19 11:20   ` Sudeep Holla
2021-05-20 19:43     ` Rob Herring
2021-05-21  4:08       ` Viresh Kumar
2021-05-21 15:24         ` Sudeep Holla
2021-05-24  9:17           ` Viresh Kumar
2021-05-24 10:05             ` Sudeep Holla
2021-10-14 10:56 ` Ulf Hansson
2021-10-14 14:55   ` Sudeep Holla
2021-10-15  9:17     ` Ulf Hansson
2021-10-19  7:24       ` Viresh Kumar
2021-10-19 13:58         ` Ulf Hansson
2021-10-20  6:24           ` Viresh Kumar
2021-10-20 10:25       ` Sudeep Holla
2021-10-21 13:34         ` Ulf Hansson
2021-10-21 15:35           ` Sudeep Holla
2021-10-20 12:11       ` Rob Herring
2021-10-21 13:13         ` Ulf Hansson
2021-10-21 13:33           ` Sudeep Holla
2021-10-21 16:01             ` Ulf Hansson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.