linux-kernel - Re: [PATCH RFC 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

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: <20241017060045.q2cz3o77aejq4g5m@hu-akhilpo-hyd.qualcomm.com>
Date: Thu, 17 Oct 2024 11:30:45 +0530
From: Akhil P Oommen <quic_akhilpo@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        "Konrad
 Dybcio" <konradybcio@...nel.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Marijn Suijten
	<marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, "Simona
 Vetter" <simona@...ll.ch>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon
	<nm@...com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        <linux-pm@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH RFC 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor
 bindings

On Wed, Oct 16, 2024 at 09:53:58AM +0200, Krzysztof Kozlowski wrote:
> On 15/10/2024 21:13, Akhil P Oommen wrote:
> > On Mon, Oct 14, 2024 at 09:39:01AM +0200, Krzysztof Kozlowski wrote:
> >> On Sat, Oct 12, 2024 at 01:59:29AM +0530, Akhil P Oommen wrote:
> >>> Add a new schema which extends opp-v2 to support a new vendor specific
> >>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
> >>> property called "qcom,opp-acd-level" carries a u32 value recommended
> >>> for each opp needs to be shared to GMU during runtime.
> >>>
> >>> Signed-off-by: Akhil P Oommen <quic_akhilpo@...cinc.com>
> >>> ---
> >>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 84 ++++++++++++++++++++++
> >>>  1 file changed, 84 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>> new file mode 100644
> >>> index 000000000000..9fb828e9da86
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>> @@ -0,0 +1,84 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Qualcomm Adreno compatible OPP supply
> >>> +
> >>> +description:
> >>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> >>> +  ACD related information tailored for the specific chipset. This binding
> >>> +  provides the information needed to describe such a hardware value.
> >>> +
> >>> +maintainers:
> >>> +  - Rob Clark <robdclark@...il.com>
> >>> +
> >>> +allOf:
> >>> +  - $ref: opp-v2-base.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: operating-points-v2-adreno
> >>> +
> >>> +patternProperties:
> >>> +  '^opp-?[0-9]+$':
> >>> +    type: object
> >>> +    additionalProperties: false
> >>> +
> >>> +    properties:
> >>> +      opp-hz: true
> >>> +
> >>> +      opp-level: true
> >>> +
> >>> +      opp-peak-kBps: true
> >>> +
> >>> +      opp-supported-hw: true
> >>> +
> >>> +      qcom,opp-acd-level:
> >>> +        description: |
> >>> +          A positive value representing the acd level associated with this
> >>
> >> What is acd?
> > 
> > Adaptive Clock Distribution, a fancy name for clock throttling during voltage
> > droop. I will update the description to capture this.
> > 
> >>
> >>> +          OPP node. This value is shared to GMU during GPU wake up. It may
> >>
> >> What is GMU?
> > 
> > A co-processor which does power management for Adreno GPU.
> 
> Everything, except obvious GPU, should be explained. GMU is not really
> that obvious:
> https://en.wikipedia.org/wiki/GMU

Will do.

> 
> > 
> >>
> >>> +          not be present for some OPPs and GMU will disable ACD while
> >>
> >> acd or ACD?
> > 
> > should be uppercase everywhere in description.
> > 
> >>
> >>> +          transitioning to that OPP.
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>> +
> >>> +    required:
> >>> +      - opp-hz
> >>> +      - opp-level
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +
> >>
> >> Drop blank line
> >>
> >>> +    #include <dt-bindings/power/qcom-rpmpd.h>
> >>> +
> >>> +    gpu_opp_table: opp-table {
> >>> +        compatible = "operating-points-v2-adreno";
> >>> +
> >>> +        opp-550000000 {
> >>> +                opp-hz = /bits/ 64 <550000000>;
> >>> +                opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
> >>> +                opp-peak-kBps = <6074219>;
> >>> +                qcom,opp-acd-level = <0xc0285ffd>;
> >>> +        };
> >>> +
> >>> +        opp-390000000 {
> >>> +                opp-hz = /bits/ 64 <390000000>;
> >>> +                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> >>> +                opp-peak-kBps = <3000000>;
> >>> +                qcom,opp-acd-level = <0xc0285ffd>;
> >>
> >> That's the same value used everywhere. What's the point? Just encode it
> >> in the driver.
> > 
> > I will update this to keep a different value. In a real implmentation,
> > these values may vary between OPPs. For eg:, please check the DT patch
> > in this series:
> > 
> > https://patchwork.freedesktop.org/patch/619413/
> 
> OK. I still have concerns that it is just some magic hex value. Which
> looks exactly how downstream code. No explanation, no meaning: neither
> in property description nor in actual value (at least I could not spot it).
> 
> And why this is hex? Unit of "level" is either some logical meaning,
> like "high" or "low", or some unit, e.g. Hertz or kBps. None of them are
> hex values in real world.

This value (which is identified after characterization) encodes a voltage
threshold for the ACD hardware and few other knobs required for each OPP.
The intepretation of the bitfields changes between SoCs.

Another point is that ACD is a requirement for higher GPU frequencies to
meet the hw spec. So OPP dt node is the natural place to keep this info,
which also helps to share this data between different OS.

-Akhil

> 
> Best regards,
> Krzysztof
>