linux-kernel - Re: [PATCH v2 1/3] dt-bindings: power: Introduce 'assigned-performance-states' property

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

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

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

Message-ID: <YLYV3ov/iBffZMg4@gerhold.net>
Date:   Tue, 1 Jun 2021 13:12:45 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Rajendra Nayak <rnayak@...eaurora.org>
Cc:     ulf.hansson@...aro.org, robh+dt@...nel.org,
        bjorn.andersson@...aro.org, viresh.kumar@...aro.org,
        linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        swboyd@...omium.org, rojay@...eaurora.org
Subject: Re: [PATCH v2 1/3] dt-bindings: power: Introduce
 'assigned-performance-states' property

Hi,

On Thu, May 27, 2021 at 11:42:27AM +0530, Rajendra Nayak wrote:
> While most devices within power-domains which support performance states,
> scale the performance state dynamically, some devices might want to
> set a static/default performance state while the device is active.
> These devices typically would also run off a fixed clock and not support
> dynamically scaling the device's performance, also known as DVFS techniques.
> Add a property 'assigned-performance-states' which client devices can
> use to set this default performance state on their power-domains.
> 
> Signed-off-by: Rajendra Nayak <rnayak@...eaurora.org>
> ---
>  .../devicetree/bindings/power/power-domain.yaml    | 50 ++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/power-domain.yaml b/Documentation/devicetree/bindings/power/power-domain.yaml
> index aed51e9..88cebf2 100644
> --- a/Documentation/devicetree/bindings/power/power-domain.yaml
> +++ b/Documentation/devicetree/bindings/power/power-domain.yaml
> @@ -66,6 +66,19 @@ properties:
>        by the given provider should be subdomains of the domain specified
>        by this binding.
>  
> +  assigned-performance-states:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +       Some devices might need to configure their power domains in a default
> +       performance state while the device is active. These devices typically
> +       would also run off a fixed clock and not support dynamically scaling the
> +       device's performance, also known as DVFS techniques. The list of performance
> +       state values should correspond to the list of power domains specified as part
> +       of the power-domains property. Each cell corresponds to one power-domain.
> +       A value of 0 can be used for power-domains with no performance state
> +       requirement. In case the power-domains have OPP tables associated, the values
> +       here would typically match with one of the entries in the OPP table.
> +

Is it just me or is this actually in the wrong place here?
Given that #power-domain-cells is required this looks like the bindings
for power domain providers, not consumers. :)

It looks like the consumer bindings are still in
Documentation/devicetree/bindings/power/power_domain.txt

>  required:
>    - "#power-domain-cells"
>  
> @@ -131,3 +144,40 @@ examples:
>              min-residency-us = <7000>;
>          };
>      };
> +
> +  - |
> +    parent4: power-controller@...40000 {
> +        compatible = "foo,power-controller";
> +        reg = <0x12340000 0x1000>;
> +        #power-domain-cells = <0>;
> +    };
> +
> +    parent5: power-controller@...10000 {
> +        compatible = "foo,power-controller";
> +        reg = <0x43210000 0x1000>;
> +        #power-domain-cells = <0>;
> +        operating-points-v2 = <&power_opp_table>;
> +
> +        power_opp_table: opp-table {
> +            compatible = "operating-points-v2";
> +
> +            power_opp_low: opp1 {
> +                opp-level = <16>;
> +            };
> +
> +            rpmpd_opp_ret: opp2 {
> +                opp-level = <64>;
> +            };
> +
> +            rpmpd_opp_svs: opp3 {
> +                opp-level = <256>;
> +            };
> +        };
> +    };
> +
> +    child4: consumer@...41000 {
> +        compatible = "foo,consumer";
> +        reg = <0x12341000 0x1000>;
> +        power-domains = <&parent4>, <&parent5>;
> +        assigned-performance-states = <0>, <256>;
> +    };

Bjorn already asked this in v1 [1]:

> May I ask how this is different from saying something like:
>
> 	required-opps = <&??>, <&rpmpd_opp_svs>;

and maybe this was already discussed further elsewhere. But I think at
the very least we need some clarification in the commit message + the
binding documentation how your new property relates to the existing
"required-opps" binding.

Because even if it might not be implemented at the moment,
Documentation/devicetree/bindings/power/power_domain.txt actually also
specifies "required-opps" for device nodes e.g. with the following example:

	leaky-device0@...50000 {
		compatible = "foo,i-leak-current";
		reg = <0x12350000 0x1000>;
		power-domains = <&power 0>;
		required-opps = <&domain0_opp_0>;
	};

It looks like Viresh added that in commit e856f078bcf1
("OPP: Introduce "required-opp" property").

And in general I think it's a bit inconsistent that we usually refer to
performance states with phandles into the OPP table, but the
assigned-performance-states suddenly use "raw numbers".

Stephan

[1]: https://lore.kernel.org/linux-arm-msm/YAG%2FpNXQOS+C2zLr@builder.lan/