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@xxxxxxxxxxx> > >>> --- > >>> .../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@xxxxxxxxx> > >>> + > >>> +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 >
