| Message ID | 20241021-gpu-acd-v2-2-9c25a62803bc@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support for GPU ACD feature on Adreno X1-85 | expand |
On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: > On Mon, Oct 21, 2024 at 05:23:43PM +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. >> >> Cc: Rob Clark <robdclark@gmail.com> >> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >> --- >> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ >> 1 file changed, 96 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..6d50c0405ef8 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml >> @@ -0,0 +1,96 @@ >> +# 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@gmail.com> >> + >> +allOf: >> + - $ref: opp-v2-base.yaml# >> + >> +properties: >> + compatible: >> + items: >> + - const: operating-points-v2-adreno >> + - const: operating-points-v2 >> + >> +patternProperties: >> + '^opp-?[0-9]+$': > > '-' should not be optional. opp1 is not expected name. Agree. Will change this to '^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 (Adaptive Clock Distribution, >> + a fancy name for clk throttling during voltage droop) level associated >> + with this OPP node. This value is shared to a co-processor inside GPU >> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not >> + be present for some OPPs and GMU will disable ACD while transitioning >> + to that OPP. This value encodes a voltage threshold and few other knobs >> + which are identified by characterization of the SoC. So, it doesn't have >> + any unit. > > Thanks for explanation and other updates. I am still not happy with this > property. I do not see reason why DT should encode magic values in a > quite generic piece of code. This creates poor ABI, difficult to > maintain or understand. > Configuring GPU ACD block with its respective value is a requirement for each OPP. So OPP node seems like the natural place for this data. If it helps to resolve your concerns, I can elaborate the documentation with details on the GMU HFI interface where this value should be passed on to the hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" in the above doc. > >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> + required: >> + - opp-hz >> + - opp-level >> + >> +required: >> + - compatible >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/power/qcom-rpmpd.h> >> + >> + gpu_opp_table: opp-table { >> + compatible = "operating-points-v2-adreno", "operating-points-v2"; >> + >> + opp-687000000 { >> + opp-hz = /bits/ 64 <687000000>; > > Messed indentation. It seems my text editor got confused here. Will fix. Thanks, -Akhil > > Best regards, > Krzysztof > >
On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: > On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: >> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: >>> On Mon, Oct 21, 2024 at 05:23:43PM +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. >>>> >>>> Cc: Rob Clark <robdclark@gmail.com> >>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>> --- >>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ >>>> 1 file changed, 96 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..6d50c0405ef8 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml >>>> @@ -0,0 +1,96 @@ >>>> +# 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@gmail.com> >>>> + >>>> +allOf: >>>> + - $ref: opp-v2-base.yaml# >>>> + >>>> +properties: >>>> + compatible: >>>> + items: >>>> + - const: operating-points-v2-adreno >>>> + - const: operating-points-v2 >>>> + >>>> +patternProperties: >>>> + '^opp-?[0-9]+$': >>> >>> '-' should not be optional. opp1 is not expected name. >> >> Agree. Will change this to '^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 (Adaptive Clock Distribution, >>>> + a fancy name for clk throttling during voltage droop) level associated >>>> + with this OPP node. This value is shared to a co-processor inside GPU >>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not >>>> + be present for some OPPs and GMU will disable ACD while transitioning >>>> + to that OPP. This value encodes a voltage threshold and few other knobs >>>> + which are identified by characterization of the SoC. So, it doesn't have >>>> + any unit. >>> >>> Thanks for explanation and other updates. I am still not happy with this >>> property. I do not see reason why DT should encode magic values in a >>> quite generic piece of code. This creates poor ABI, difficult to >>> maintain or understand. >>> >> >> Configuring GPU ACD block with its respective value is a requirement for each OPP. >> So OPP node seems like the natural place for this data. >> >> If it helps to resolve your concerns, I can elaborate the documentation with >> details on the GMU HFI interface where this value should be passed on to the >> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" >> in the above doc. > > Usually the preference for DT is to specify data in a sensible way > rather than just the values being programmed to the register. Is it > possible to implement this approach for ACD values? I am still checking about this. Will get back. -Akhil > >> >>> >
On 11/1/2024 9:54 PM, Akhil P Oommen wrote: > On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: >> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: >>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: >>>> On Mon, Oct 21, 2024 at 05:23:43PM +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. >>>>> >>>>> Cc: Rob Clark <robdclark@gmail.com> >>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>>> --- >>>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ >>>>> 1 file changed, 96 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..6d50c0405ef8 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml >>>>> @@ -0,0 +1,96 @@ >>>>> +# 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@gmail.com> >>>>> + >>>>> +allOf: >>>>> + - $ref: opp-v2-base.yaml# >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + items: >>>>> + - const: operating-points-v2-adreno >>>>> + - const: operating-points-v2 >>>>> + >>>>> +patternProperties: >>>>> + '^opp-?[0-9]+$': >>>> >>>> '-' should not be optional. opp1 is not expected name. >>> >>> Agree. Will change this to '^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 (Adaptive Clock Distribution, >>>>> + a fancy name for clk throttling during voltage droop) level associated >>>>> + with this OPP node. This value is shared to a co-processor inside GPU >>>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not >>>>> + be present for some OPPs and GMU will disable ACD while transitioning >>>>> + to that OPP. This value encodes a voltage threshold and few other knobs >>>>> + which are identified by characterization of the SoC. So, it doesn't have >>>>> + any unit. >>>> >>>> Thanks for explanation and other updates. I am still not happy with this >>>> property. I do not see reason why DT should encode magic values in a >>>> quite generic piece of code. This creates poor ABI, difficult to >>>> maintain or understand. >>>> >>> >>> Configuring GPU ACD block with its respective value is a requirement for each OPP. >>> So OPP node seems like the natural place for this data. >>> >>> If it helps to resolve your concerns, I can elaborate the documentation with >>> details on the GMU HFI interface where this value should be passed on to the >>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" >>> in the above doc. >> >> Usually the preference for DT is to specify data in a sensible way >> rather than just the values being programmed to the register. Is it >> possible to implement this approach for ACD values? Krzysztof/Dmitry, BIT(0)-BIT(15) are static configurations which doesn't change between OPPs. We can move it to driver. BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can keep this in the devicetree. And the driver can construct the final value from both data and send it to GMU. If this is acceptable, I will send the v3 revision. -Akhil. > > I am still checking about this. Will get back. > > -Akhil > >> >>> >>>> >> >
On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote: > Hello Akhil, > > On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >> >> On 11/1/2024 9:54 PM, Akhil P Oommen wrote: >>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: >>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: >>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: >>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +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. >>>>>>> >>>>>>> Cc: Rob Clark <robdclark@gmail.com> >>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>>>>> --- >>>>>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ >>>>>>> 1 file changed, 96 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..6d50c0405ef8 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml >>>>>>> @@ -0,0 +1,96 @@ >>>>>>> +# 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@gmail.com> >>>>>>> + >>>>>>> +allOf: >>>>>>> + - $ref: opp-v2-base.yaml# >>>>>>> + >>>>>>> +properties: >>>>>>> + compatible: >>>>>>> + items: >>>>>>> + - const: operating-points-v2-adreno >>>>>>> + - const: operating-points-v2 >>>>>>> + >>>>>>> +patternProperties: >>>>>>> + '^opp-?[0-9]+$': >>>>>> >>>>>> '-' should not be optional. opp1 is not expected name. >>>>> >>>>> Agree. Will change this to '^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 (Adaptive Clock Distribution, >>>>>>> + a fancy name for clk throttling during voltage droop) level associated >>>>>>> + with this OPP node. This value is shared to a co-processor inside GPU >>>>>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not >>>>>>> + be present for some OPPs and GMU will disable ACD while transitioning >>>>>>> + to that OPP. This value encodes a voltage threshold and few other knobs >>>>>>> + which are identified by characterization of the SoC. So, it doesn't have >>>>>>> + any unit. >>>>>> >>>>>> Thanks for explanation and other updates. I am still not happy with this >>>>>> property. I do not see reason why DT should encode magic values in a >>>>>> quite generic piece of code. This creates poor ABI, difficult to >>>>>> maintain or understand. >>>>>> >>>>> >>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP. >>>>> So OPP node seems like the natural place for this data. >>>>> >>>>> If it helps to resolve your concerns, I can elaborate the documentation with >>>>> details on the GMU HFI interface where this value should be passed on to the >>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" >>>>> in the above doc. >>>> >>>> Usually the preference for DT is to specify data in a sensible way >>>> rather than just the values being programmed to the register. Is it >>>> possible to implement this approach for ACD values? >> >> Krzysztof/Dmitry, >> >> BIT(0)-BIT(15) are static configurations which doesn't change between >> OPPs. We can move it to driver. >> >> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can >> keep this in the devicetree. And the driver can construct the final >> value from both data and send it to GMU. >> >> If this is acceptable, I will send the v3 revision. > > Can the upper bitfield have a sensible representation in DT (like uV > or something similar)? Closest approximation is quantized voltage steps. So, unit-less. Converting it to the exact voltage requires identifying the pmic voltage steps and other stuffs which are outside of my expertise. It is convenient if we can abstract it as an integer which correlates with the voltage margin that should be maintained for each regulator corner. -Akhil. > >> >> -Akhil. >> >>> >>> I am still checking about this. Will get back. >>> >>> -Akhil >>> >>>> >>>>> >>>>>> >>>> >>> >> > >
On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > > On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote: > > Hello Akhil, > > > > On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > >> > >> On 11/1/2024 9:54 PM, Akhil P Oommen wrote: > >>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: > >>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: > >>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: > >>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +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. > >>>>>>> > >>>>>>> Cc: Rob Clark <robdclark@gmail.com> > >>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > >>>>>>> --- > >>>>>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ > >>>>>>> 1 file changed, 96 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..6d50c0405ef8 > >>>>>>> --- /dev/null > >>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml > >>>>>>> @@ -0,0 +1,96 @@ > >>>>>>> +# 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@gmail.com> > >>>>>>> + > >>>>>>> +allOf: > >>>>>>> + - $ref: opp-v2-base.yaml# > >>>>>>> + > >>>>>>> +properties: > >>>>>>> + compatible: > >>>>>>> + items: > >>>>>>> + - const: operating-points-v2-adreno > >>>>>>> + - const: operating-points-v2 > >>>>>>> + > >>>>>>> +patternProperties: > >>>>>>> + '^opp-?[0-9]+$': > >>>>>> > >>>>>> '-' should not be optional. opp1 is not expected name. > >>>>> > >>>>> Agree. Will change this to '^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 (Adaptive Clock Distribution, > >>>>>>> + a fancy name for clk throttling during voltage droop) level associated > >>>>>>> + with this OPP node. This value is shared to a co-processor inside GPU > >>>>>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not > >>>>>>> + be present for some OPPs and GMU will disable ACD while transitioning > >>>>>>> + to that OPP. This value encodes a voltage threshold and few other knobs > >>>>>>> + which are identified by characterization of the SoC. So, it doesn't have > >>>>>>> + any unit. > >>>>>> > >>>>>> Thanks for explanation and other updates. I am still not happy with this > >>>>>> property. I do not see reason why DT should encode magic values in a > >>>>>> quite generic piece of code. This creates poor ABI, difficult to > >>>>>> maintain or understand. > >>>>>> > >>>>> > >>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP. > >>>>> So OPP node seems like the natural place for this data. > >>>>> > >>>>> If it helps to resolve your concerns, I can elaborate the documentation with > >>>>> details on the GMU HFI interface where this value should be passed on to the > >>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" > >>>>> in the above doc. > >>>> > >>>> Usually the preference for DT is to specify data in a sensible way > >>>> rather than just the values being programmed to the register. Is it > >>>> possible to implement this approach for ACD values? > >> > >> Krzysztof/Dmitry, > >> > >> BIT(0)-BIT(15) are static configurations which doesn't change between > >> OPPs. We can move it to driver. > >> > >> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can > >> keep this in the devicetree. And the driver can construct the final > >> value from both data and send it to GMU. > >> > >> If this is acceptable, I will send the v3 revision. > > > > Can the upper bitfield have a sensible representation in DT (like uV > > or something similar)? > > Closest approximation is quantized voltage steps. So, unit-less. > Converting it to the exact voltage requires identifying the pmic voltage > steps and other stuffs which are outside of my expertise. > > It is convenient if we can abstract it as an integer which correlates > with the voltage margin that should be maintained for each regulator corner. I'd say, this is up to the DT maintainers then. > > -Akhil. > > > > >> > >> -Akhil. > >> > >>> > >>> I am still checking about this. Will get back. > >>> > >>> -Akhil > >>> > >>>> > >>>>> > >>>>>> > >>>> > >>> > >> > > > > >
On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote: > On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >> >> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote: >>> Hello Akhil, >>> >>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>> >>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote: >>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: >>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: >>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: >>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +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. >>>>>>>>> >>>>>>>>> Cc: Rob Clark <robdclark@gmail.com> >>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>>>>>>> --- >>>>>>>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ >>>>>>>>> 1 file changed, 96 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..6d50c0405ef8 >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml >>>>>>>>> @@ -0,0 +1,96 @@ >>>>>>>>> +# 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@gmail.com> >>>>>>>>> + >>>>>>>>> +allOf: >>>>>>>>> + - $ref: opp-v2-base.yaml# >>>>>>>>> + >>>>>>>>> +properties: >>>>>>>>> + compatible: >>>>>>>>> + items: >>>>>>>>> + - const: operating-points-v2-adreno >>>>>>>>> + - const: operating-points-v2 >>>>>>>>> + >>>>>>>>> +patternProperties: >>>>>>>>> + '^opp-?[0-9]+$': >>>>>>>> >>>>>>>> '-' should not be optional. opp1 is not expected name. >>>>>>> >>>>>>> Agree. Will change this to '^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 (Adaptive Clock Distribution, >>>>>>>>> + a fancy name for clk throttling during voltage droop) level associated >>>>>>>>> + with this OPP node. This value is shared to a co-processor inside GPU >>>>>>>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not >>>>>>>>> + be present for some OPPs and GMU will disable ACD while transitioning >>>>>>>>> + to that OPP. This value encodes a voltage threshold and few other knobs >>>>>>>>> + which are identified by characterization of the SoC. So, it doesn't have >>>>>>>>> + any unit. >>>>>>>> >>>>>>>> Thanks for explanation and other updates. I am still not happy with this >>>>>>>> property. I do not see reason why DT should encode magic values in a >>>>>>>> quite generic piece of code. This creates poor ABI, difficult to >>>>>>>> maintain or understand. >>>>>>>> >>>>>>> >>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP. >>>>>>> So OPP node seems like the natural place for this data. >>>>>>> >>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with >>>>>>> details on the GMU HFI interface where this value should be passed on to the >>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" >>>>>>> in the above doc. >>>>>> >>>>>> Usually the preference for DT is to specify data in a sensible way >>>>>> rather than just the values being programmed to the register. Is it >>>>>> possible to implement this approach for ACD values? >>>> >>>> Krzysztof/Dmitry, >>>> >>>> BIT(0)-BIT(15) are static configurations which doesn't change between >>>> OPPs. We can move it to driver. >>>> >>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can >>>> keep this in the devicetree. And the driver can construct the final >>>> value from both data and send it to GMU. >>>> >>>> If this is acceptable, I will send the v3 revision. >>> >>> Can the upper bitfield have a sensible representation in DT (like uV >>> or something similar)? >> >> Closest approximation is quantized voltage steps. So, unit-less. >> Converting it to the exact voltage requires identifying the pmic voltage >> steps and other stuffs which are outside of my expertise. >> >> It is convenient if we can abstract it as an integer which correlates >> with the voltage margin that should be maintained for each regulator corner. Krzysztof, Could you please confirm if this approach would be acceptable? To reiterate, move the lower 16 bits which is same across OPPs to the driver. Abstract the higher 16 bits as number of quantized voltage margin when ACD mitigation gets triggered. -Akhil. > > I'd say, this is up to the DT maintainers then. > >> >> -Akhil. >> >>> >>>> >>>> -Akhil. >>>> >>>>> >>>>> I am still checking about this. Will get back. >>>>> >>>>> -Akhil >>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>> >>>>> >>>> >>> >>> >> > >
On 4.12.2024 7:18 PM, Akhil P Oommen wrote: > On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote: >> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>> >>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote: >>>> Hello Akhil, >>>> >>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>> >>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote: >>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: >>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: >>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: >>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +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. >>>>>>>>>> >>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com> >>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>>>>>>>> --- >>>>>>>>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ >>>>>>>>>> 1 file changed, 96 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..6d50c0405ef8 >>>>>>>>>> --- /dev/null >>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml >>>>>>>>>> @@ -0,0 +1,96 @@ >>>>>>>>>> +# 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@gmail.com> >>>>>>>>>> + >>>>>>>>>> +allOf: >>>>>>>>>> + - $ref: opp-v2-base.yaml# >>>>>>>>>> + >>>>>>>>>> +properties: >>>>>>>>>> + compatible: >>>>>>>>>> + items: >>>>>>>>>> + - const: operating-points-v2-adreno >>>>>>>>>> + - const: operating-points-v2 >>>>>>>>>> + >>>>>>>>>> +patternProperties: >>>>>>>>>> + '^opp-?[0-9]+$': >>>>>>>>> >>>>>>>>> '-' should not be optional. opp1 is not expected name. >>>>>>>> >>>>>>>> Agree. Will change this to '^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 (Adaptive Clock Distribution, >>>>>>>>>> + a fancy name for clk throttling during voltage droop) level associated >>>>>>>>>> + with this OPP node. This value is shared to a co-processor inside GPU >>>>>>>>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not >>>>>>>>>> + be present for some OPPs and GMU will disable ACD while transitioning >>>>>>>>>> + to that OPP. This value encodes a voltage threshold and few other knobs >>>>>>>>>> + which are identified by characterization of the SoC. So, it doesn't have >>>>>>>>>> + any unit. >>>>>>>>> >>>>>>>>> Thanks for explanation and other updates. I am still not happy with this >>>>>>>>> property. I do not see reason why DT should encode magic values in a >>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to >>>>>>>>> maintain or understand. >>>>>>>>> >>>>>>>> >>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP. >>>>>>>> So OPP node seems like the natural place for this data. >>>>>>>> >>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with >>>>>>>> details on the GMU HFI interface where this value should be passed on to the >>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" >>>>>>>> in the above doc. >>>>>>> >>>>>>> Usually the preference for DT is to specify data in a sensible way >>>>>>> rather than just the values being programmed to the register. Is it >>>>>>> possible to implement this approach for ACD values? >>>>> >>>>> Krzysztof/Dmitry, >>>>> >>>>> BIT(0)-BIT(15) are static configurations which doesn't change between >>>>> OPPs. We can move it to driver. >>>>> >>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can >>>>> keep this in the devicetree. And the driver can construct the final >>>>> value from both data and send it to GMU. >>>>> >>>>> If this is acceptable, I will send the v3 revision. >>>> >>>> Can the upper bitfield have a sensible representation in DT (like uV >>>> or something similar)? >>> >>> Closest approximation is quantized voltage steps. So, unit-less. >>> Converting it to the exact voltage requires identifying the pmic voltage >>> steps and other stuffs which are outside of my expertise. >>> >>> It is convenient if we can abstract it as an integer which correlates >>> with the voltage margin that should be maintained for each regulator corner. > > Krzysztof, > > Could you please confirm if this approach would be acceptable? > > To reiterate, move the lower 16 bits which is same across OPPs to the > driver. Abstract the higher 16 bits as number of quantized voltage > margin when ACD mitigation gets triggered. I know I'm not Krzysztof, but given this is ultimately a magic value passed to the firmware, I'm a bit lukewarm on decomposing it and would rather see the entire 32b passed in a property, so that if a future target needs a different constant in the lower word, we don't have to pull our hair out again, trying to add more spaghetti logic to account for that. Konrad
On Mon, Dec 23, 2024 at 12:31:27PM +0100, Konrad Dybcio wrote: > On 4.12.2024 7:18 PM, Akhil P Oommen wrote: > > On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote: > >> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > >>> > >>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote: > >>>> Hello Akhil, > >>>> > >>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > >>>>> > >>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote: > >>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: > >>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: > >>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: > >>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +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. > >>>>>>>>>> > >>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com> > >>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > >>>>>>>>>> --- > >>>>>>>>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ > >>>>>>>>>> 1 file changed, 96 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..6d50c0405ef8 > >>>>>>>>>> --- /dev/null > >>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml > >>>>>>>>>> @@ -0,0 +1,96 @@ > >>>>>>>>>> +# 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@gmail.com> > >>>>>>>>>> + > >>>>>>>>>> +allOf: > >>>>>>>>>> + - $ref: opp-v2-base.yaml# > >>>>>>>>>> + > >>>>>>>>>> +properties: > >>>>>>>>>> + compatible: > >>>>>>>>>> + items: > >>>>>>>>>> + - const: operating-points-v2-adreno > >>>>>>>>>> + - const: operating-points-v2 > >>>>>>>>>> + > >>>>>>>>>> +patternProperties: > >>>>>>>>>> + '^opp-?[0-9]+$': > >>>>>>>>> > >>>>>>>>> '-' should not be optional. opp1 is not expected name. > >>>>>>>> > >>>>>>>> Agree. Will change this to '^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 (Adaptive Clock Distribution, > >>>>>>>>>> + a fancy name for clk throttling during voltage droop) level associated > >>>>>>>>>> + with this OPP node. This value is shared to a co-processor inside GPU > >>>>>>>>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not > >>>>>>>>>> + be present for some OPPs and GMU will disable ACD while transitioning > >>>>>>>>>> + to that OPP. This value encodes a voltage threshold and few other knobs > >>>>>>>>>> + which are identified by characterization of the SoC. So, it doesn't have > >>>>>>>>>> + any unit. > >>>>>>>>> > >>>>>>>>> Thanks for explanation and other updates. I am still not happy with this > >>>>>>>>> property. I do not see reason why DT should encode magic values in a > >>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to > >>>>>>>>> maintain or understand. > >>>>>>>>> > >>>>>>>> > >>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP. > >>>>>>>> So OPP node seems like the natural place for this data. > >>>>>>>> > >>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with > >>>>>>>> details on the GMU HFI interface where this value should be passed on to the > >>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" > >>>>>>>> in the above doc. > >>>>>>> > >>>>>>> Usually the preference for DT is to specify data in a sensible way > >>>>>>> rather than just the values being programmed to the register. Is it > >>>>>>> possible to implement this approach for ACD values? > >>>>> > >>>>> Krzysztof/Dmitry, > >>>>> > >>>>> BIT(0)-BIT(15) are static configurations which doesn't change between > >>>>> OPPs. We can move it to driver. > >>>>> > >>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can > >>>>> keep this in the devicetree. And the driver can construct the final > >>>>> value from both data and send it to GMU. > >>>>> > >>>>> If this is acceptable, I will send the v3 revision. > >>>> > >>>> Can the upper bitfield have a sensible representation in DT (like uV > >>>> or something similar)? > >>> > >>> Closest approximation is quantized voltage steps. So, unit-less. > >>> Converting it to the exact voltage requires identifying the pmic voltage > >>> steps and other stuffs which are outside of my expertise. > >>> > >>> It is convenient if we can abstract it as an integer which correlates > >>> with the voltage margin that should be maintained for each regulator corner. > > > > Krzysztof, > > > > Could you please confirm if this approach would be acceptable? > > > > To reiterate, move the lower 16 bits which is same across OPPs to the > > driver. Abstract the higher 16 bits as number of quantized voltage > > margin when ACD mitigation gets triggered. > > I know I'm not Krzysztof, but given this is ultimately a magic value > passed to the firmware, I'm a bit lukewarm on decomposing it and would > rather see the entire 32b passed in a property, so that if a future > target needs a different constant in the lower word, we don't have to > pull our hair out again, trying to add more spaghetti logic to account > for that. Also obviously being non-Krzysztof, if we don't have a semantic value for the upper half I'm fine with having the magic value as a single instance instead of spreading it between two places.
On 12/23/2024 5:24 PM, Dmitry Baryshkov wrote: > On Mon, Dec 23, 2024 at 12:31:27PM +0100, Konrad Dybcio wrote: >> On 4.12.2024 7:18 PM, Akhil P Oommen wrote: >>> On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote: >>>> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>> >>>>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote: >>>>>> Hello Akhil, >>>>>> >>>>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>>>> >>>>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote: >>>>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: >>>>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: >>>>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: >>>>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +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. >>>>>>>>>>>> >>>>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com> >>>>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>>>>>>>>>> --- >>>>>>>>>>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ >>>>>>>>>>>> 1 file changed, 96 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..6d50c0405ef8 >>>>>>>>>>>> --- /dev/null >>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml >>>>>>>>>>>> @@ -0,0 +1,96 @@ >>>>>>>>>>>> +# 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@gmail.com> >>>>>>>>>>>> + >>>>>>>>>>>> +allOf: >>>>>>>>>>>> + - $ref: opp-v2-base.yaml# >>>>>>>>>>>> + >>>>>>>>>>>> +properties: >>>>>>>>>>>> + compatible: >>>>>>>>>>>> + items: >>>>>>>>>>>> + - const: operating-points-v2-adreno >>>>>>>>>>>> + - const: operating-points-v2 >>>>>>>>>>>> + >>>>>>>>>>>> +patternProperties: >>>>>>>>>>>> + '^opp-?[0-9]+$': >>>>>>>>>>> >>>>>>>>>>> '-' should not be optional. opp1 is not expected name. >>>>>>>>>> >>>>>>>>>> Agree. Will change this to '^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 (Adaptive Clock Distribution, >>>>>>>>>>>> + a fancy name for clk throttling during voltage droop) level associated >>>>>>>>>>>> + with this OPP node. This value is shared to a co-processor inside GPU >>>>>>>>>>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not >>>>>>>>>>>> + be present for some OPPs and GMU will disable ACD while transitioning >>>>>>>>>>>> + to that OPP. This value encodes a voltage threshold and few other knobs >>>>>>>>>>>> + which are identified by characterization of the SoC. So, it doesn't have >>>>>>>>>>>> + any unit. >>>>>>>>>>> >>>>>>>>>>> Thanks for explanation and other updates. I am still not happy with this >>>>>>>>>>> property. I do not see reason why DT should encode magic values in a >>>>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to >>>>>>>>>>> maintain or understand. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP. >>>>>>>>>> So OPP node seems like the natural place for this data. >>>>>>>>>> >>>>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with >>>>>>>>>> details on the GMU HFI interface where this value should be passed on to the >>>>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" >>>>>>>>>> in the above doc. >>>>>>>>> >>>>>>>>> Usually the preference for DT is to specify data in a sensible way >>>>>>>>> rather than just the values being programmed to the register. Is it >>>>>>>>> possible to implement this approach for ACD values? >>>>>>> >>>>>>> Krzysztof/Dmitry, >>>>>>> >>>>>>> BIT(0)-BIT(15) are static configurations which doesn't change between >>>>>>> OPPs. We can move it to driver. >>>>>>> >>>>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can >>>>>>> keep this in the devicetree. And the driver can construct the final >>>>>>> value from both data and send it to GMU. >>>>>>> >>>>>>> If this is acceptable, I will send the v3 revision. >>>>>> >>>>>> Can the upper bitfield have a sensible representation in DT (like uV >>>>>> or something similar)? >>>>> >>>>> Closest approximation is quantized voltage steps. So, unit-less. >>>>> Converting it to the exact voltage requires identifying the pmic voltage >>>>> steps and other stuffs which are outside of my expertise. >>>>> >>>>> It is convenient if we can abstract it as an integer which correlates >>>>> with the voltage margin that should be maintained for each regulator corner. >>> >>> Krzysztof, >>> >>> Could you please confirm if this approach would be acceptable? >>> >>> To reiterate, move the lower 16 bits which is same across OPPs to the >>> driver. Abstract the higher 16 bits as number of quantized voltage >>> margin when ACD mitigation gets triggered. >> >> I know I'm not Krzysztof, but given this is ultimately a magic value >> passed to the firmware, I'm a bit lukewarm on decomposing it and would >> rather see the entire 32b passed in a property, so that if a future >> target needs a different constant in the lower word, we don't have to >> pull our hair out again, trying to add more spaghetti logic to account >> for that. > > Also obviously being non-Krzysztof, if we don't have a semantic value > for the upper half I'm fine with having the magic value as a single > instance instead of spreading it between two places. If there is a general consensus, I will send out another revision with some minor updates. -Akhil. >
On 23/12/2024 22:31, Akhil P Oommen wrote: > On 12/23/2024 5:24 PM, Dmitry Baryshkov wrote: >> On Mon, Dec 23, 2024 at 12:31:27PM +0100, Konrad Dybcio wrote: >>> On 4.12.2024 7:18 PM, Akhil P Oommen wrote: >>>> On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote: >>>>> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>>> >>>>>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote: >>>>>>> Hello Akhil, >>>>>>> >>>>>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>>>>> >>>>>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote: >>>>>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: >>>>>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: >>>>>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: >>>>>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +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. >>>>>>>>>>>>> >>>>>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com> >>>>>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>>>>>>>>>>> --- >>>>>>>>>>>>> .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ >>>>>>>>>>>>> 1 file changed, 96 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..6d50c0405ef8 >>>>>>>>>>>>> --- /dev/null >>>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml >>>>>>>>>>>>> @@ -0,0 +1,96 @@ >>>>>>>>>>>>> +# 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@gmail.com> >>>>>>>>>>>>> + >>>>>>>>>>>>> +allOf: >>>>>>>>>>>>> + - $ref: opp-v2-base.yaml# >>>>>>>>>>>>> + >>>>>>>>>>>>> +properties: >>>>>>>>>>>>> + compatible: >>>>>>>>>>>>> + items: >>>>>>>>>>>>> + - const: operating-points-v2-adreno >>>>>>>>>>>>> + - const: operating-points-v2 >>>>>>>>>>>>> + >>>>>>>>>>>>> +patternProperties: >>>>>>>>>>>>> + '^opp-?[0-9]+$': >>>>>>>>>>>> >>>>>>>>>>>> '-' should not be optional. opp1 is not expected name. >>>>>>>>>>> >>>>>>>>>>> Agree. Will change this to '^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 (Adaptive Clock Distribution, >>>>>>>>>>>>> + a fancy name for clk throttling during voltage droop) level associated >>>>>>>>>>>>> + with this OPP node. This value is shared to a co-processor inside GPU >>>>>>>>>>>>> + (called Graphics Management Unit a.k.a GMU) during wake up. It may not >>>>>>>>>>>>> + be present for some OPPs and GMU will disable ACD while transitioning >>>>>>>>>>>>> + to that OPP. This value encodes a voltage threshold and few other knobs >>>>>>>>>>>>> + which are identified by characterization of the SoC. So, it doesn't have >>>>>>>>>>>>> + any unit. >>>>>>>>>>>> >>>>>>>>>>>> Thanks for explanation and other updates. I am still not happy with this >>>>>>>>>>>> property. I do not see reason why DT should encode magic values in a >>>>>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to >>>>>>>>>>>> maintain or understand. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP. >>>>>>>>>>> So OPP node seems like the natural place for this data. >>>>>>>>>>> >>>>>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with >>>>>>>>>>> details on the GMU HFI interface where this value should be passed on to the >>>>>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin" >>>>>>>>>>> in the above doc. >>>>>>>>>> >>>>>>>>>> Usually the preference for DT is to specify data in a sensible way >>>>>>>>>> rather than just the values being programmed to the register. Is it >>>>>>>>>> possible to implement this approach for ACD values? >>>>>>>> >>>>>>>> Krzysztof/Dmitry, >>>>>>>> >>>>>>>> BIT(0)-BIT(15) are static configurations which doesn't change between >>>>>>>> OPPs. We can move it to driver. >>>>>>>> >>>>>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can >>>>>>>> keep this in the devicetree. And the driver can construct the final >>>>>>>> value from both data and send it to GMU. >>>>>>>> >>>>>>>> If this is acceptable, I will send the v3 revision. >>>>>>> >>>>>>> Can the upper bitfield have a sensible representation in DT (like uV >>>>>>> or something similar)? >>>>>> >>>>>> Closest approximation is quantized voltage steps. So, unit-less. >>>>>> Converting it to the exact voltage requires identifying the pmic voltage >>>>>> steps and other stuffs which are outside of my expertise. >>>>>> >>>>>> It is convenient if we can abstract it as an integer which correlates >>>>>> with the voltage margin that should be maintained for each regulator corner. >>>> >>>> Krzysztof, >>>> >>>> Could you please confirm if this approach would be acceptable? >>>> >>>> To reiterate, move the lower 16 bits which is same across OPPs to the >>>> driver. Abstract the higher 16 bits as number of quantized voltage >>>> margin when ACD mitigation gets triggered. >>> >>> I know I'm not Krzysztof, but given this is ultimately a magic value >>> passed to the firmware, I'm a bit lukewarm on decomposing it and would >>> rather see the entire 32b passed in a property, so that if a future >>> target needs a different constant in the lower word, we don't have to >>> pull our hair out again, trying to add more spaghetti logic to account >>> for that. I can also imagine future SoC not respecting existing interface and switching to something new, duplicating the effort. All this is "driven by downstream" approach... but sure, let's go with existing approach. Best regards, Krzysztof
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..6d50c0405ef8 --- /dev/null +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml @@ -0,0 +1,96 @@ +# 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@gmail.com> + +allOf: + - $ref: opp-v2-base.yaml# + +properties: + compatible: + items: + - const: operating-points-v2-adreno + - const: operating-points-v2 + +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 (Adaptive Clock Distribution, + a fancy name for clk throttling during voltage droop) level associated + with this OPP node. This value is shared to a co-processor inside GPU + (called Graphics Management Unit a.k.a GMU) during wake up. It may not + be present for some OPPs and GMU will disable ACD while transitioning + to that OPP. This value encodes a voltage threshold and few other knobs + which are identified by characterization of the SoC. So, it doesn't have + any unit. + $ref: /schemas/types.yaml#/definitions/uint32 + + required: + - opp-hz + - opp-level + +required: + - compatible + +additionalProperties: false + +examples: + - | + #include <dt-bindings/power/qcom-rpmpd.h> + + gpu_opp_table: opp-table { + compatible = "operating-points-v2-adreno", "operating-points-v2"; + + opp-687000000 { + opp-hz = /bits/ 64 <687000000>; + opp-level = <RPMH_REGULATOR_LEVEL_SVS_L1>; + opp-peak-kBps = <8171875>; + qcom,opp-acd-level = <0x882e5ffd>; + }; + + 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>; + }; + + opp-300000000 { + opp-hz = /bits/ 64 <300000000>; + opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>; + opp-peak-kBps = <2136719>; + /* Intentionally left out qcom,opp-acd-level property here */ + }; + + };
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. Cc: Rob Clark <robdclark@gmail.com> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> --- .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 ++++++++++++++++++++++ 1 file changed, 96 insertions(+)