| Message ID | 20230217-topic-cpr3h-v14-3-9fd23241493d@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Core Power Reduction v3, v4 and Hardened | expand |
On Tue, Aug 29, 2023 at 01:01:44PM +0200, Ulf Hansson wrote: > On Mon, 28 Aug 2023 at 13:42, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > > > Add the bindings for the CPR3 driver to the documentation. > > > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > [Konrad: Make binding check pass; update AGdR's email] > > Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > --- > > .../devicetree/bindings/soc/qcom/qcom,cpr3.yaml | 286 +++++++++++++++++++++ > > 1 file changed, 286 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml > > new file mode 100644 > > index 000000000000..acf2e294866b > > [...] > > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/qcom,gcc-msm8998.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + > > + cpus { > > + #address-cells = <2>; > > + #size-cells = <0>; > > + > > + cpu@0 { > > + compatible = "qcom,kryo280"; > > + device_type = "cpu"; > > + reg = <0x0 0x0>; > > + operating-points-v2 = <&cpu0_opp_table>; > > + power-domains = <&apc_cprh 0>; > > + power-domain-names = "cprh"; > > Rather than using a Qcom specific power-domain-name, perhaps a common > power-domain-name for cpus, that can be used for "the performance > domain" would be a good idea here? > > I have suggested using "perf" for the SCMI performance domain [1], > perhaps that description should be extended to cover this and other > performance domains too? Better yet, nothing. There's no value to -names when there is only 1 entry. Rob
On 31.08.2023 18:28, Rob Herring wrote: > On Tue, Aug 29, 2023 at 01:01:44PM +0200, Ulf Hansson wrote: >> On Mon, 28 Aug 2023 at 13:42, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >>> >>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> >>> >>> Add the bindings for the CPR3 driver to the documentation. >>> >>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> >>> [Konrad: Make binding check pass; update AGdR's email] >>> Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>> --- >>> .../devicetree/bindings/soc/qcom/qcom,cpr3.yaml | 286 +++++++++++++++++++++ >>> 1 file changed, 286 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml >>> new file mode 100644 >>> index 000000000000..acf2e294866b >> >> [...] >> >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/clock/qcom,gcc-msm8998.h> >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + >>> + cpus { >>> + #address-cells = <2>; >>> + #size-cells = <0>; >>> + >>> + cpu@0 { >>> + compatible = "qcom,kryo280"; >>> + device_type = "cpu"; >>> + reg = <0x0 0x0>; >>> + operating-points-v2 = <&cpu0_opp_table>; >>> + power-domains = <&apc_cprh 0>; >>> + power-domain-names = "cprh"; >> >> Rather than using a Qcom specific power-domain-name, perhaps a common >> power-domain-name for cpus, that can be used for "the performance >> domain" would be a good idea here? >> >> I have suggested using "perf" for the SCMI performance domain [1], >> perhaps that description should be extended to cover this and other >> performance domains too? > > Better yet, nothing. There's no value to -names when there is only 1 > entry. As of today, it's required for devm_pm_opp_attach_genpd() Ulf, is there a better way to do this that doesn't require names? Konrad
On Thu, 31 Aug 2023 at 18:28, Rob Herring <robh@kernel.org> wrote: > > On Tue, Aug 29, 2023 at 01:01:44PM +0200, Ulf Hansson wrote: > > On Mon, 28 Aug 2023 at 13:42, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > > > From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > > > > > Add the bindings for the CPR3 driver to the documentation. > > > > > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > > [Konrad: Make binding check pass; update AGdR's email] > > > Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > --- > > > .../devicetree/bindings/soc/qcom/qcom,cpr3.yaml | 286 +++++++++++++++++++++ > > > 1 file changed, 286 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml > > > new file mode 100644 > > > index 000000000000..acf2e294866b > > > > [...] > > > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/clock/qcom,gcc-msm8998.h> > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > + > > > + cpus { > > > + #address-cells = <2>; > > > + #size-cells = <0>; > > > + > > > + cpu@0 { > > > + compatible = "qcom,kryo280"; > > > + device_type = "cpu"; > > > + reg = <0x0 0x0>; > > > + operating-points-v2 = <&cpu0_opp_table>; > > > + power-domains = <&apc_cprh 0>; > > > + power-domain-names = "cprh"; > > > > Rather than using a Qcom specific power-domain-name, perhaps a common > > power-domain-name for cpus, that can be used for "the performance > > domain" would be a good idea here? > > > > I have suggested using "perf" for the SCMI performance domain [1], > > perhaps that description should be extended to cover this and other > > performance domains too? > > Better yet, nothing. There's no value to -names when there is only 1 > entry. The above is just an example of a consumer node for a cpu. The bindings for cpus are in Documentation/devicetree/bindings/arm/cpus.yaml. Please have a look there. We don't even have a maxItems specified for it. Kind regards Uffe
On Thu, 31 Aug 2023 at 18:40, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 31.08.2023 18:28, Rob Herring wrote: > > On Tue, Aug 29, 2023 at 01:01:44PM +0200, Ulf Hansson wrote: > >> On Mon, 28 Aug 2023 at 13:42, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > >>> > >>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > >>> > >>> Add the bindings for the CPR3 driver to the documentation. > >>> > >>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > >>> [Konrad: Make binding check pass; update AGdR's email] > >>> Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >>> --- > >>> .../devicetree/bindings/soc/qcom/qcom,cpr3.yaml | 286 +++++++++++++++++++++ > >>> 1 file changed, 286 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml > >>> new file mode 100644 > >>> index 000000000000..acf2e294866b > >> > >> [...] > >> > >>> + > >>> +examples: > >>> + - | > >>> + #include <dt-bindings/clock/qcom,gcc-msm8998.h> > >>> + #include <dt-bindings/interrupt-controller/irq.h> > >>> + > >>> + cpus { > >>> + #address-cells = <2>; > >>> + #size-cells = <0>; > >>> + > >>> + cpu@0 { > >>> + compatible = "qcom,kryo280"; > >>> + device_type = "cpu"; > >>> + reg = <0x0 0x0>; > >>> + operating-points-v2 = <&cpu0_opp_table>; > >>> + power-domains = <&apc_cprh 0>; > >>> + power-domain-names = "cprh"; > >> > >> Rather than using a Qcom specific power-domain-name, perhaps a common > >> power-domain-name for cpus, that can be used for "the performance > >> domain" would be a good idea here? > >> > >> I have suggested using "perf" for the SCMI performance domain [1], > >> perhaps that description should be extended to cover this and other > >> performance domains too? > > > > Better yet, nothing. There's no value to -names when there is only 1 > > entry. > As of today, it's required for devm_pm_opp_attach_genpd() > > Ulf, is there a better way to do this that doesn't require names? In my opinion I think using names is valuable from a future and flexibility point of view. To pick the proper name is another question. Anyway, in this case I think you should consider the case of potentially having multiple power-domains for the cpu. Having both a cpr(h) (for performance-scaling) and a psci (for power) power-domain sounds like a combination that should already exist. Maybe not upstream wise, but at least this is what I have been told to exist several years ago by Qcom engineers. Kind regards Uffe
On 1.09.2023 00:02, Ulf Hansson wrote: > On Thu, 31 Aug 2023 at 18:40, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> On 31.08.2023 18:28, Rob Herring wrote: >>> On Tue, Aug 29, 2023 at 01:01:44PM +0200, Ulf Hansson wrote: >>>> On Mon, 28 Aug 2023 at 13:42, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >>>>> >>>>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> >>>>> >>>>> Add the bindings for the CPR3 driver to the documentation. >>>>> >>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> >>>>> [Konrad: Make binding check pass; update AGdR's email] >>>>> Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com> >>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>> --- >>>>> .../devicetree/bindings/soc/qcom/qcom,cpr3.yaml | 286 +++++++++++++++++++++ >>>>> 1 file changed, 286 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml >>>>> new file mode 100644 >>>>> index 000000000000..acf2e294866b >>>> >>>> [...] >>>> >>>>> + >>>>> +examples: >>>>> + - | >>>>> + #include <dt-bindings/clock/qcom,gcc-msm8998.h> >>>>> + #include <dt-bindings/interrupt-controller/irq.h> >>>>> + >>>>> + cpus { >>>>> + #address-cells = <2>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + cpu@0 { >>>>> + compatible = "qcom,kryo280"; >>>>> + device_type = "cpu"; >>>>> + reg = <0x0 0x0>; >>>>> + operating-points-v2 = <&cpu0_opp_table>; >>>>> + power-domains = <&apc_cprh 0>; >>>>> + power-domain-names = "cprh"; >>>> >>>> Rather than using a Qcom specific power-domain-name, perhaps a common >>>> power-domain-name for cpus, that can be used for "the performance >>>> domain" would be a good idea here? >>>> >>>> I have suggested using "perf" for the SCMI performance domain [1], >>>> perhaps that description should be extended to cover this and other >>>> performance domains too? >>> >>> Better yet, nothing. There's no value to -names when there is only 1 >>> entry. >> As of today, it's required for devm_pm_opp_attach_genpd() >> >> Ulf, is there a better way to do this that doesn't require names? > > In my opinion I think using names is valuable from a future and > flexibility point of view. To pick the proper name is another > question. > > Anyway, in this case I think you should consider the case of > potentially having multiple power-domains for the cpu. Having both a > cpr(h) (for performance-scaling) and a psci (for power) power-domain > sounds like a combination that should already exist. Maybe not > upstream wise, but at least this is what I have been told to exist > several years ago by Qcom engineers. Riiight I completely forgot about PSCI cpuidle.. Konrad
diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml new file mode 100644 index 000000000000..acf2e294866b --- /dev/null +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,cpr3.yaml @@ -0,0 +1,286 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/soc/qcom/qcom,cpr3.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Core Power Reduction v3/v4/Hardened (CPR3, CPR4, CPRh) + +description: + CPR (Core Power Reduction) is a technology to reduce core power of a CPU + (or another device). Each OPP of a device corresponds to a "corner" that + has a range of valid voltages for a particular frequency. + The CPR monitors dynamic factors such as temperature, etc. and suggests + or (in the CPR-hardened case) applies voltage adjustments to save power + and meet silicon characteristic requirements for a given chip unit. + +maintainers: + - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> + +properties: + compatible: + oneOf: + - const: qcom,cpr3 + - const: qcom,cpr4 + - items: + - enum: + - qcom,msm8998-cprh + - qcom,sdm630-cprh + - const: qcom,cprh + + reg: + items: + - description: Register space of the CPR controller0 + - description: Register space of the CPR controller1 + + interrupts: + maxItems: 1 + + clocks: + items: + - description: CPR reference clock + + vdd-supply: + description: Autonomous Phase Control (APC) or other power supply + + '#power-domain-cells': + const: 1 + + qcom,acc: + $ref: /schemas/types.yaml#/definitions/phandle + description: phandle to syscon for writing ACC settings + + nvmem-cells: + description: Cells containing the fuse corners and revision data + maxItems: 32 + + nvmem-cell-names: + maxItems: 32 + + operating-points-v2: true + + power-domains: + maxItems: 1 + +required: + - compatible + - reg + - clocks + - operating-points-v2 + - "#power-domain-cells" + - nvmem-cells + - nvmem-cell-names + +additionalProperties: false + +allOf: + - if: + properties: + compatible: + contains: + enum: + - qcom,msm8998-cprh + then: + properties: + nvmem-cell-names: + items: + - const: cpr_speed_bin + - const: cpr_fuse_revision + - const: cpr0_quotient1 + - const: cpr0_quotient2 + - const: cpr0_quotient3 + - const: cpr0_quotient4 + - const: cpr0_quotient_offset2 + - const: cpr0_quotient_offset3 + - const: cpr0_quotient_offset4 + - const: cpr0_init_voltage1 + - const: cpr0_init_voltage2 + - const: cpr0_init_voltage3 + - const: cpr0_init_voltage4 + - const: cpr0_ring_osc1 + - const: cpr0_ring_osc2 + - const: cpr0_ring_osc3 + - const: cpr0_ring_osc4 + - const: cpr1_quotient1 + - const: cpr1_quotient2 + - const: cpr1_quotient3 + - const: cpr1_quotient4 + - const: cpr1_quotient_offset2 + - const: cpr1_quotient_offset3 + - const: cpr1_quotient_offset4 + - const: cpr1_init_voltage1 + - const: cpr1_init_voltage2 + - const: cpr1_init_voltage3 + - const: cpr1_init_voltage4 + - const: cpr1_ring_osc1 + - const: cpr1_ring_osc2 + - const: cpr1_ring_osc3 + - const: cpr1_ring_osc4 + +examples: + - | + #include <dt-bindings/clock/qcom,gcc-msm8998.h> + #include <dt-bindings/interrupt-controller/irq.h> + + cpus { + #address-cells = <2>; + #size-cells = <0>; + + cpu@0 { + compatible = "qcom,kryo280"; + device_type = "cpu"; + reg = <0x0 0x0>; + operating-points-v2 = <&cpu0_opp_table>; + power-domains = <&apc_cprh 0>; + power-domain-names = "cprh"; + }; + + cpu@100 { + compatible = "qcom,kryo280"; + device_type = "cpu"; + reg = <0x0 0x100>; + operating-points-v2 = <&cpu4_opp_table>; + power-domains = <&apc_cprh 1>; + power-domain-names = "cprh"; + }; + }; + + cpu0_opp_table: opp-table-cpu0 { + compatible = "operating-points-v2"; + opp-shared; + + opp-1843200000 { + opp-hz = /bits/ 64 <1843200000>; + required-opps = <&cprh_opp3>; + }; + + opp-1094400000 { + opp-hz = /bits/ 64 <1094400000>; + required-opps = <&cprh_opp2>; + }; + + opp-300000000 { + opp-hz = /bits/ 64 <300000000>; + required-opps = <&cprh_opp1>; + }; + }; + + cpu4_opp_table: opp-table-cpu4 { + compatible = "operating-points-v2"; + opp-shared; + + opp-2208000000 { + opp-hz = /bits/ 64 <2208000000>; + required-opps = <&cprh_opp3>; + }; + + opp-1113600000 { + opp-hz = /bits/ 64 <1113600000>; + required-opps = <&cprh_opp2>; + }; + + opp-300000000 { + opp-hz = /bits/ 64 <300000000>; + required-opps = <&cprh_opp1>; + }; + }; + + cprh_opp_table: opp-table-cprh { + compatible = "operating-points-v2-qcom-level"; + + cprh_opp1: opp-1 { + opp-level = <1>; + qcom,opp-fuse-level = <1>; + qcom,opp-cloop-vadj = <0>; + qcom,opp-oloop-vadj = <0>; + }; + + cprh_opp2: opp-2 { + opp-level = <2>; + qcom,opp-fuse-level = <2>; + qcom,opp-cloop-vadj = <0>; + qcom,opp-oloop-vadj = <0>; + }; + + cprh_opp3: opp-3 { + opp-level = <3>; + qcom,opp-fuse-level = <2 3>; + qcom,opp-cloop-vadj = <0>; + qcom,opp-oloop-vadj = <0>; + }; + }; + + apc_cprh: power-controller@179c8000 { + compatible = "qcom,msm8998-cprh", "qcom,cprh"; + reg = <0x0179c8000 0x4000>, <0x0179c4000 0x4000>; + clocks = <&gcc GCC_HMSS_RBCPR_CLK>; + + operating-points-v2 = <&cprh_opp_table>; + #power-domain-cells = <1>; + + nvmem-cells = <&cpr_efuse_speedbin>, + <&cpr_fuse_revision>, + <&cpr_quot0_pwrcl>, + <&cpr_quot1_pwrcl>, + <&cpr_quot2_pwrcl>, + <&cpr_quot3_pwrcl>, + <&cpr_quot_offset1_pwrcl>, + <&cpr_quot_offset2_pwrcl>, + <&cpr_quot_offset3_pwrcl>, + <&cpr_init_voltage0_pwrcl>, + <&cpr_init_voltage1_pwrcl>, + <&cpr_init_voltage2_pwrcl>, + <&cpr_init_voltage3_pwrcl>, + <&cpr_ro_sel0_pwrcl>, + <&cpr_ro_sel1_pwrcl>, + <&cpr_ro_sel2_pwrcl>, + <&cpr_ro_sel3_pwrcl>, + <&cpr_quot0_perfcl>, + <&cpr_quot1_perfcl>, + <&cpr_quot2_perfcl>, + <&cpr_quot3_perfcl>, + <&cpr_quot_offset1_perfcl>, + <&cpr_quot_offset2_perfcl>, + <&cpr_quot_offset3_perfcl>, + <&cpr_init_voltage0_perfcl>, + <&cpr_init_voltage1_perfcl>, + <&cpr_init_voltage2_perfcl>, + <&cpr_init_voltage3_perfcl>, + <&cpr_ro_sel0_perfcl>, + <&cpr_ro_sel1_perfcl>, + <&cpr_ro_sel2_perfcl>, + <&cpr_ro_sel3_perfcl>; + nvmem-cell-names = "cpr_speed_bin", + "cpr_fuse_revision", + "cpr0_quotient1", + "cpr0_quotient2", + "cpr0_quotient3", + "cpr0_quotient4", + "cpr0_quotient_offset2", + "cpr0_quotient_offset3", + "cpr0_quotient_offset4", + "cpr0_init_voltage1", + "cpr0_init_voltage2", + "cpr0_init_voltage3", + "cpr0_init_voltage4", + "cpr0_ring_osc1", + "cpr0_ring_osc2", + "cpr0_ring_osc3", + "cpr0_ring_osc4", + "cpr1_quotient1", + "cpr1_quotient2", + "cpr1_quotient3", + "cpr1_quotient4", + "cpr1_quotient_offset2", + "cpr1_quotient_offset3", + "cpr1_quotient_offset4", + "cpr1_init_voltage1", + "cpr1_init_voltage2", + "cpr1_init_voltage3", + "cpr1_init_voltage4", + "cpr1_ring_osc1", + "cpr1_ring_osc2", + "cpr1_ring_osc3", + "cpr1_ring_osc4"; + }; +...