| Message ID | 20241007061023.1978380-2-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm_scmi: vendors: Qualcomm Generic Vendor Extensions | expand |
On Mon, Oct 07, 2024 at 11:40:19AM GMT, Sibi Sankar wrote: > Document the various memory buses that can be monitored and scaled by > the memory latency governor hosted by the QCOM SCMI Generic Extension > Protocol v1.0. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > > v3: > * Restructure the bindings to mimic IMX [Christian] > > .../bindings/firmware/arm,scmi.yaml | 1 + > .../bindings/firmware/qcom,scmi-memlat.yaml | 246 ++++++++++++++++++ > .../dt-bindings/firmware/qcom,scmi-memlat.h | 22 ++ > 3 files changed, 269 insertions(+) > create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > create mode 100644 include/dt-bindings/firmware/qcom,scmi-memlat.h > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > index 54d7d11bfed4..1d405f429168 100644 > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > @@ -24,6 +24,7 @@ description: | > > anyOf: > - $ref: /schemas/firmware/nxp,imx95-scmi.yaml > + - $ref: /schemas/firmware/qcom,scmi-memlat.yaml > > properties: > $nodename: > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > new file mode 100644 > index 000000000000..0e8ea6dacd6a > --- /dev/null > +++ b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > @@ -0,0 +1,246 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/firmware/qcom,scmi-memlat.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm SCMI Memory Bus nodes > + > +maintainers: > + - Sibi Sankar <quic_sibis@quicinc.com> > + > +description: > + This binding describes the various memory buses that can be monitored and scaled > + by memory latency governor running on the CPU Control Processor (SCMI controller). > + > +properties: > + protocol@80: > + $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node' > + unevaluatedProperties: false > + > + properties: > + reg: > + const: 0x80 > + > + patternProperties: > + '^memory-[0-9]$': > + type: object > + unevaluatedProperties: false > + description: > + The list of all memory buses that can be monitored and scaled by the > + memory latency governor running on the SCMI controller. > + > + properties: > + qcom,memory-type: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2] > + description: | > + Memory Bus Identifier > + 0 = QCOM_MEM_TYPE_DDR > + 1 = QCOM_MEM_TYPE_LLCC > + 2 = QCOM_MEM_TYPE_DDR_QOS I'm sorry if this has been discussed and frowned upon, but can you squash memory type into device node? protocol@80 { ddr { }; llcc { }; ddr-qos { }; }; > + > + freq-table-hz: > + items: > + items: > + - description: Minimum frequency of the memory bus in Hz > + - description: Maximum frequency of the memory bus in Hz Does it make sense for the DDR-QOS type? Can we hardcode those values and drop freq-table-hz from the DDR-QOS node? Also, can we drop this completely by adding one extra OPP entry with the minimum memory bus frequency? > + > + patternProperties: > + '^monitor-[0-9]$': > + type: object > + unevaluatedProperties: false > + description: > + The list of all monitors detecting the memory latency bound workloads using > + various counters. > + > + properties: > + qcom,compute-type: > + description: > + Monitors of type compute perform bus dvfs based on a rudimentary CPU > + frequency to memory frequency map. > + type: boolean This seems to be redundant. If there is no qcom,ipm-ceil property, then it's qcom,compute-type, isn't it? > + > + qcom,ipm-ceil: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Monitors having this property perform bus dvfs based on the same > + rudimentary table but the scaling is performed only if the calculated > + IPM (Instruction Per Misses) exceeds the given ceiling. > + > + cpus: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: > + Should be a list of phandles to CPU nodes (as described in > + Documentation/devicetree/bindings/arm/cpus.yaml). Which CPU nodes? I see that the examples list all CPUs here. Do we really need them? > + > + operating-points-v2: true > + opp-table: > + type: object > + > + required: > + - cpus > + - operating-points-v2 > + > + oneOf: > + - required: [ 'qcom,compute-type' ] > + - required: [ 'qcom,ipm-ceil' ] > + > + required: > + - qcom,memory-type > + - freq-table-hz > + > +additionalProperties: true > + > +examples: > + - | > + #include <dt-bindings/firmware/qcom,scmi-memlat.h> > + > + firmware { > + scmi { > + compatible = "arm,scmi"; > + mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>; > + mbox-names = "tx", "rx"; > + shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + protocol@80 { > + reg = <0x80>; > + > + memory-0 { > + qcom,memory-type = <QCOM_MEM_TYPE_DDR>; > + freq-table-hz = /bits/ 64 <200000000 4224000000>; > + > + monitor-0 { Hmm. Can we say that each memory type can have at most one IPM and one compute aka "passive" memlat monitor? Does it make sense to use them as node names and drop the extra monitor-M names? > + qcom,ipm-ceil = <20000000>; > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > + &CPU8 &CPU9 &CPU10 &CPU11>; Are CPU lists different between monitors? Can they be different? Can they be different between different memory types? > + operating-points-v2 = <&memory0_monitor0_opp_table>; > + > + memory0_monitor0_opp_table: opp-table { sensible names are better: ddr_ipm_opp_table: opp-table { }; > + compatible = "operating-points-v2"; > + > + opp-999000000 { > + opp-hz = /bits/ 64 <999000000 547000000>; > + }; > + > + opp-1440000000 { > + opp-hz = /bits/ 64 <1440000000 768000000>; > + }; > + > + opp-1671000000 { > + opp-hz = /bits/ 64 <1671000000 1555000000>; > + }; > + > + opp-2189000000 { > + opp-hz = /bits/ 64 <2189000000 2092000000>; > + }; > + > + opp-2516000000 { > + opp-hz = /bits/ 64 <2516000000 3187000000>; > + }; > + > + opp-3860000000 { > + opp-hz = /bits/ 64 <3860000000 4224000000>; > + }; > + }; > + }; > + > + monitor-1 { > + qcom,compute-type; > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > + &CPU8 &CPU9 &CPU10 &CPU11>; > + operating-points-v2 = <&memory0_monitor1_opp_table>; > + > + memory0_monitor1_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-1440000000 { > + opp-hz = /bits/ 64 <1440000000 200000000>; > + }; > + > + opp-2189000000 { > + opp-hz = /bits/ 64 <2189000000 768000000>; > + }; > + > + opp-2516000000 { > + opp-hz = /bits/ 64 <2516000000 1555000000>; > + }; > + > + opp-3860000000 { > + opp-hz = /bits/ 64 <3860000000 4224000000>; > + }; > + }; > + }; > + }; > + > + memory-1 { > + qcom,memory-type = <QCOM_MEM_TYPE_LLCC>; > + freq-table-hz = /bits/ 64 <300000000 1067000000>; > + > + monitor-0 { > + qcom,ipm-ceil = <20000000>; > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > + &CPU8 &CPU9 &CPU10 &CPU11>; > + operating-points-v2 = <&memory1_monitor0_opp_table>; > + > + memory1_monitor0_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-999000000 { > + opp-hz = /bits/ 64 <999000000 300000000>; > + }; > + > + opp-1440000000 { > + opp-hz = /bits/ 64 <1440000000 466000000>; > + }; > + > + opp-1671000000 { > + opp-hz = /bits/ 64 <1671000000 600000000>; > + }; > + > + opp-2189000000 { > + opp-hz = /bits/ 64 <2189000000 806000000>; > + }; > + > + opp-2516000000 { > + opp-hz = /bits/ 64 <2516000000 933000000>; > + }; > + > + opp-3860000000 { > + opp-hz = /bits/ 64 <3860000000 1066000000>; > + }; > + }; > + }; > + }; > + > + memory-2 { > + qcom,memory-type = <QCOM_MEM_TYPE_DDR_QOS>; > + freq-table-hz = /bits/ 64 <QCOM_DDR_LEVEL_AUTO QCOM_DDR_LEVEL_PERF>; This is definitely not 'frequency of the memory bys in Hz' > + > + monitor-0 { > + qcom,ipm-ceil = <20000000>; > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > + &CPU8 &CPU9 &CPU10 &CPU11>; > + operating-points-v2 = <&memory2_monitor0_opp_table>; > + > + memory2_monitor0_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-2189000000 { > + opp-hz = /bits/ 64 <2189000000>; > + opp-level = <QCOM_DDR_LEVEL_AUTO>; > + }; > + > + opp-3860000000 { > + opp-hz = /bits/ 64 <3860000000>; > + opp-level = <QCOM_DDR_LEVEL_PERF>; > + }; > + }; > + }; > + }; > + }; > + }; > + }; > diff --git a/include/dt-bindings/firmware/qcom,scmi-memlat.h b/include/dt-bindings/firmware/qcom,scmi-memlat.h > new file mode 100644 > index 000000000000..7ae8d8d5623b > --- /dev/null > +++ b/include/dt-bindings/firmware/qcom,scmi-memlat.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > +/* > + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. > + */ > +#ifndef __DT_BINDINGS_QCOM_SCMI_VENDOR_H > +#define __DT_BINDINGS_QCOM_SCMI_VENDOR > + > +/* Memory IDs */ > +#define QCOM_MEM_TYPE_DDR 0x0 > +#define QCOM_MEM_TYPE_LLCC 0x1 > +#define QCOM_MEM_TYPE_DDR_QOS 0x2 > + > +/* > + * QCOM_MEM_TYPE_DDR_QOS supports the following states. > + * > + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled > + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled > + */ > +#define QCOM_DDR_LEVEL_AUTO 0x0 > +#define QCOM_DDR_LEVEL_PERF 0x1 > + > +#endif /* __DT_BINDINGS_QCOM_SCMI_VENDOR_H */ > -- > 2.34.1 >
On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote: > Document the various memory buses that can be monitored and scaled by > the memory latency governor hosted by the QCOM SCMI Generic Extension > Protocol v1.0. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > > v3: > * Restructure the bindings to mimic IMX [Christian] > > .../bindings/firmware/arm,scmi.yaml | 1 + > .../bindings/firmware/qcom,scmi-memlat.yaml | 246 ++++++++++++++++++ > .../dt-bindings/firmware/qcom,scmi-memlat.h | 22 ++ > 3 files changed, 269 insertions(+) > create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > create mode 100644 include/dt-bindings/firmware/qcom,scmi-memlat.h > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > index 54d7d11bfed4..1d405f429168 100644 > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > @@ -24,6 +24,7 @@ description: | > > anyOf: > - $ref: /schemas/firmware/nxp,imx95-scmi.yaml > + - $ref: /schemas/firmware/qcom,scmi-memlat.yaml > > properties: > $nodename: > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > new file mode 100644 > index 000000000000..0e8ea6dacd6a > --- /dev/null > +++ b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > @@ -0,0 +1,246 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/firmware/qcom,scmi-memlat.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm SCMI Memory Bus nodes > + > +maintainers: > + - Sibi Sankar <quic_sibis@quicinc.com> > + > +description: > + This binding describes the various memory buses that can be monitored and scaled Drop "This binding" and describe the hardware/firmware, not binding. Also, not wrapped according to Linux coding style. > + by memory latency governor running on the CPU Control Processor (SCMI controller). > + > +properties: > + protocol@80: > + $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node' > + unevaluatedProperties: false > + > + properties: > + reg: > + const: 0x80 > + > + patternProperties: > + '^memory-[0-9]$': > + type: object > + unevaluatedProperties: false > + description: > + The list of all memory buses that can be monitored and scaled by the > + memory latency governor running on the SCMI controller. > + > + properties: > + qcom,memory-type: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2] > + description: | > + Memory Bus Identifier > + 0 = QCOM_MEM_TYPE_DDR > + 1 = QCOM_MEM_TYPE_LLCC > + 2 = QCOM_MEM_TYPE_DDR_QOS You need to describe these. Why "Quality of service" should be a separate bus? To me it looks like you are re-defining interconnects. > + > + freq-table-hz: > + items: > + items: " - items:" no? > + - description: Minimum frequency of the memory bus in Hz > + - description: Maximum frequency of the memory bus in Hz > + > + patternProperties: > + '^monitor-[0-9]$': > + type: object > + unevaluatedProperties: false > + description: > + The list of all monitors detecting the memory latency bound workloads using > + various counters. > + > + properties: > + qcom,compute-type: > + description: > + Monitors of type compute perform bus dvfs based on a rudimentary CPU > + frequency to memory frequency map. I don't understand why you need to describe what type of monitor the SCMI has. Commit msg is pretty vague, so does not help me, either. > + type: boolean > + > + qcom,ipm-ceil: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Monitors having this property perform bus dvfs based on the same > + rudimentary table but the scaling is performed only if the calculated > + IPM (Instruction Per Misses) exceeds the given ceiling. > + > + cpus: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: > + Should be a list of phandles to CPU nodes (as described in > + Documentation/devicetree/bindings/arm/cpus.yaml). > + > + operating-points-v2: true > + opp-table: > + type: object > + > + required: > + - cpus > + - operating-points-v2 > + > + oneOf: > + - required: [ 'qcom,compute-type' ] > + - required: [ 'qcom,ipm-ceil' ] > + > + required: > + - qcom,memory-type > + - freq-table-hz > + > +additionalProperties: true > + > +examples: > + - | > + #include <dt-bindings/firmware/qcom,scmi-memlat.h> > + > + firmware { > + scmi { > + compatible = "arm,scmi"; > + mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>; > + mbox-names = "tx", "rx"; > + shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + protocol@80 { > + reg = <0x80>; > + > + memory-0 { > + qcom,memory-type = <QCOM_MEM_TYPE_DDR>; > + freq-table-hz = /bits/ 64 <200000000 4224000000>; > + > + monitor-0 { > + qcom,ipm-ceil = <20000000>; > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > + &CPU8 &CPU9 &CPU10 &CPU11>; Labels are lowercase. > + operating-points-v2 = <&memory0_monitor0_opp_table>; > + > + memory0_monitor0_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-999000000 { > + opp-hz = /bits/ 64 <999000000 547000000>; > + }; > + > + opp-1440000000 { > + opp-hz = /bits/ 64 <1440000000 768000000>; > + }; > + > + opp-1671000000 { > + opp-hz = /bits/ 64 <1671000000 1555000000>; > + }; > + > + opp-2189000000 { > + opp-hz = /bits/ 64 <2189000000 2092000000>; > + }; > + > + opp-2516000000 { > + opp-hz = /bits/ 64 <2516000000 3187000000>; > + }; > + > + opp-3860000000 { > + opp-hz = /bits/ 64 <3860000000 4224000000>; > + }; > + }; > + }; > + > + monitor-1 { > + qcom,compute-type; > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > + &CPU8 &CPU9 &CPU10 &CPU11>; > + operating-points-v2 = <&memory0_monitor1_opp_table>; > + > + memory0_monitor1_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-1440000000 { > + opp-hz = /bits/ 64 <1440000000 200000000>; > + }; > + > + opp-2189000000 { > + opp-hz = /bits/ 64 <2189000000 768000000>; > + }; > + > + opp-2516000000 { > + opp-hz = /bits/ 64 <2516000000 1555000000>; > + }; > + > + opp-3860000000 { > + opp-hz = /bits/ 64 <3860000000 4224000000>; > + }; > + }; > + }; > + }; > + > + memory-1 { > + qcom,memory-type = <QCOM_MEM_TYPE_LLCC>; > + freq-table-hz = /bits/ 64 <300000000 1067000000>; > + > + monitor-0 { > + qcom,ipm-ceil = <20000000>; > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > + &CPU8 &CPU9 &CPU10 &CPU11>; > + operating-points-v2 = <&memory1_monitor0_opp_table>; > + > + memory1_monitor0_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-999000000 { > + opp-hz = /bits/ 64 <999000000 300000000>; > + }; > + > + opp-1440000000 { > + opp-hz = /bits/ 64 <1440000000 466000000>; > + }; > + > + opp-1671000000 { > + opp-hz = /bits/ 64 <1671000000 600000000>; > + }; > + > + opp-2189000000 { > + opp-hz = /bits/ 64 <2189000000 806000000>; > + }; > + > + opp-2516000000 { > + opp-hz = /bits/ 64 <2516000000 933000000>; > + }; > + > + opp-3860000000 { > + opp-hz = /bits/ 64 <3860000000 1066000000>; > + }; > + }; > + }; > + }; > + > + memory-2 { > + qcom,memory-type = <QCOM_MEM_TYPE_DDR_QOS>; > + freq-table-hz = /bits/ 64 <QCOM_DDR_LEVEL_AUTO QCOM_DDR_LEVEL_PERF>; > + > + monitor-0 { > + qcom,ipm-ceil = <20000000>; > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > + &CPU8 &CPU9 &CPU10 &CPU11>; > + operating-points-v2 = <&memory2_monitor0_opp_table>; > + > + memory2_monitor0_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-2189000000 { > + opp-hz = /bits/ 64 <2189000000>; > + opp-level = <QCOM_DDR_LEVEL_AUTO>; > + }; > + > + opp-3860000000 { > + opp-hz = /bits/ 64 <3860000000>; > + opp-level = <QCOM_DDR_LEVEL_PERF>; > + }; > + }; > + }; > + }; > + }; > + }; > + }; > diff --git a/include/dt-bindings/firmware/qcom,scmi-memlat.h b/include/dt-bindings/firmware/qcom,scmi-memlat.h > new file mode 100644 > index 000000000000..7ae8d8d5623b > --- /dev/null > +++ b/include/dt-bindings/firmware/qcom,scmi-memlat.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > +/* > + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. > + */ > +#ifndef __DT_BINDINGS_QCOM_SCMI_VENDOR_H > +#define __DT_BINDINGS_QCOM_SCMI_VENDOR > + > +/* Memory IDs */ > +#define QCOM_MEM_TYPE_DDR 0x0 > +#define QCOM_MEM_TYPE_LLCC 0x1 > +#define QCOM_MEM_TYPE_DDR_QOS 0x2 Use decimal. > + > +/* > + * QCOM_MEM_TYPE_DDR_QOS supports the following states. > + * > + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled > + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled What is "%"? > + */ > +#define QCOM_DDR_LEVEL_AUTO 0x0 > +#define QCOM_DDR_LEVEL_PERF 0x1 Decimal. > + > +#endif /* __DT_BINDINGS_QCOM_SCMI_VENDOR_H */ > -- > 2.34.1 >
On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote: > +/* > + * QCOM_MEM_TYPE_DDR_QOS supports the following states. > + * > + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled > + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled > + */ > +#define QCOM_DDR_LEVEL_AUTO 0x0 > +#define QCOM_DDR_LEVEL_PERF 0x1 I could not find any driver using these. Can you point me to usage in the drivers? Best regards, Krzysztof
On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote: > On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote: > > +/* > > + * QCOM_MEM_TYPE_DDR_QOS supports the following states. > > + * > > + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled > > + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled > > + */ > > +#define QCOM_DDR_LEVEL_AUTO 0x0 > > +#define QCOM_DDR_LEVEL_PERF 0x1 > > I could not find any driver using these. Can you point me to usage in > the drivers? It's well hidden. These are the raw values used for DDR_QOS memory. > > Best regards, > Krzysztof >
On 08/10/2024 14:10, Dmitry Baryshkov wrote: > On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote: >> On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote: >>> +/* >>> + * QCOM_MEM_TYPE_DDR_QOS supports the following states. >>> + * >>> + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled >>> + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled >>> + */ >>> +#define QCOM_DDR_LEVEL_AUTO 0x0 >>> +#define QCOM_DDR_LEVEL_PERF 0x1 >> >> I could not find any driver using these. Can you point me to usage in >> the drivers? > > It's well hidden. These are the raw values used for DDR_QOS memory. So not a binding? Then these should be dropped. Best regards, Krzysztof
On 10/7/24 23:36, Dmitry Baryshkov wrote: > On Mon, Oct 07, 2024 at 11:40:19AM GMT, Sibi Sankar wrote: >> Document the various memory buses that can be monitored and scaled by >> the memory latency governor hosted by the QCOM SCMI Generic Extension >> Protocol v1.0. >> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> Hey Dmitry, Thanks for taking time to review the series! >> --- >> >> v3: >> * Restructure the bindings to mimic IMX [Christian] >> >> .../bindings/firmware/arm,scmi.yaml | 1 + >> .../bindings/firmware/qcom,scmi-memlat.yaml | 246 ++++++++++++++++++ >> .../dt-bindings/firmware/qcom,scmi-memlat.h | 22 ++ >> 3 files changed, 269 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml >> create mode 100644 include/dt-bindings/firmware/qcom,scmi-memlat.h >> >> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> index 54d7d11bfed4..1d405f429168 100644 >> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> @@ -24,6 +24,7 @@ description: | >> >> anyOf: >> - $ref: /schemas/firmware/nxp,imx95-scmi.yaml >> + - $ref: /schemas/firmware/qcom,scmi-memlat.yaml >> >> properties: >> $nodename: >> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml >> new file mode 100644 >> index 000000000000..0e8ea6dacd6a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml >> @@ -0,0 +1,246 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/firmware/qcom,scmi-memlat.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm SCMI Memory Bus nodes >> + >> +maintainers: >> + - Sibi Sankar <quic_sibis@quicinc.com> >> + >> +description: >> + This binding describes the various memory buses that can be monitored and scaled >> + by memory latency governor running on the CPU Control Processor (SCMI controller). >> + >> +properties: >> + protocol@80: >> + $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node' >> + unevaluatedProperties: false >> + >> + properties: >> + reg: >> + const: 0x80 >> + >> + patternProperties: >> + '^memory-[0-9]$': >> + type: object >> + unevaluatedProperties: false >> + description: >> + The list of all memory buses that can be monitored and scaled by the >> + memory latency governor running on the SCMI controller. >> + >> + properties: >> + qcom,memory-type: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1, 2] >> + description: | >> + Memory Bus Identifier >> + 0 = QCOM_MEM_TYPE_DDR >> + 1 = QCOM_MEM_TYPE_LLCC >> + 2 = QCOM_MEM_TYPE_DDR_QOS > > I'm sorry if this has been discussed and frowned upon, but can you > squash memory type into device node? I don't think anyone had any strong opinions on how the nodes is to be named. We went with a generic node name since it could accomodate multiple instances of llcc or ddr in the future. We didn't want it be named ddr-0/ddr-1 and so on. So I'll continue to stick with the current naming unless you have a strong reason other than readability. > > protocol@80 { > ddr { > }; > > llcc { > }; > > ddr-qos { > }; > }; > >> + >> + freq-table-hz: >> + items: >> + items: >> + - description: Minimum frequency of the memory bus in Hz >> + - description: Maximum frequency of the memory bus in Hz > > Does it make sense for the DDR-QOS type? Can we hardcode those values > and drop freq-table-hz from the DDR-QOS node? > > Also, can we drop this completely by adding one extra OPP entry with the > minimum memory bus frequency? the map table doesn't necessarily list all the supported frequencies. It was made that way so that the table is flexible enough that it doesn't have to be changed a lot across platforms. Hence a need for a separate property to list min/max freq. > >> + >> + patternProperties: >> + '^monitor-[0-9]$': >> + type: object >> + unevaluatedProperties: false >> + description: >> + The list of all monitors detecting the memory latency bound workloads using >> + various counters. >> + >> + properties: >> + qcom,compute-type: >> + description: >> + Monitors of type compute perform bus dvfs based on a rudimentary CPU >> + frequency to memory frequency map. >> + type: boolean > > This seems to be redundant. If there is no qcom,ipm-ceil property, then > it's qcom,compute-type, isn't it? ack > >> + >> + qcom,ipm-ceil: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Monitors having this property perform bus dvfs based on the same >> + rudimentary table but the scaling is performed only if the calculated >> + IPM (Instruction Per Misses) exceeds the given ceiling. >> + >> + cpus: >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: >> + Should be a list of phandles to CPU nodes (as described in >> + Documentation/devicetree/bindings/arm/cpus.yaml). > > Which CPU nodes? I see that the examples list all CPUs here. Do we > really need them? This observation is only valid for X1E where all the cpus have identical freq charecteristics. Even with this case we need to list them to handle cases where cpus gets disabled by the bootloader on lower cored X1E parts i.e. we use this to figure out the actual physical mask. > >> + >> + operating-points-v2: true >> + opp-table: >> + type: object >> + >> + required: >> + - cpus >> + - operating-points-v2 >> + >> + oneOf: >> + - required: [ 'qcom,compute-type' ] >> + - required: [ 'qcom,ipm-ceil' ] >> + >> + required: >> + - qcom,memory-type >> + - freq-table-hz >> + >> +additionalProperties: true >> + >> +examples: >> + - | >> + #include <dt-bindings/firmware/qcom,scmi-memlat.h> >> + >> + firmware { >> + scmi { >> + compatible = "arm,scmi"; >> + mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>; >> + mbox-names = "tx", "rx"; >> + shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + protocol@80 { >> + reg = <0x80>; >> + >> + memory-0 { >> + qcom,memory-type = <QCOM_MEM_TYPE_DDR>; >> + freq-table-hz = /bits/ 64 <200000000 4224000000>; >> + >> + monitor-0 { > > Hmm. Can we say that each memory type can have at most one IPM and one > compute aka "passive" memlat monitor? Does it make sense to use them as > node names and drop the extra monitor-M names? Again this observation is valid only for X1E where the cpu freq across cpu's are identical across clusters and is not true for other mobile SoCs. So each memory can have more than 2 monitors i.e. atleast one active/passibe monitor for each cluster. > >> + qcom,ipm-ceil = <20000000>; >> + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 >> + &CPU8 &CPU9 &CPU10 &CPU11>; > > Are CPU lists different between monitors? Can they be different? Can > they be different between different memory types? same explanation. > >> + operating-points-v2 = <&memory0_monitor0_opp_table>; >> + >> + memory0_monitor0_opp_table: opp-table { > > sensible names are better: I think I just picked these names up from a cpufreq table upstream. > > ddr_ipm_opp_table: opp-table { > }; > >> + compatible = "operating-points-v2"; >> + >> + opp-999000000 { >> + opp-hz = /bits/ 64 <999000000 547000000>; >> + }; >> + >> + opp-1440000000 { >> + opp-hz = /bits/ 64 <1440000000 768000000>; >> + }; >> + >> + opp-1671000000 { >> + opp-hz = /bits/ 64 <1671000000 1555000000>; >> + }; >> + >> + opp-2189000000 { >> + opp-hz = /bits/ 64 <2189000000 2092000000>; >> + }; >> + >> + opp-2516000000 { >> + opp-hz = /bits/ 64 <2516000000 3187000000>; >> + }; >> + >> + opp-3860000000 { >> + opp-hz = /bits/ 64 <3860000000 4224000000>; >> + }; >> + }; >> + }; >> + >> + monitor-1 { >> + qcom,compute-type; >> + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 >> + &CPU8 &CPU9 &CPU10 &CPU11>; >> + operating-points-v2 = <&memory0_monitor1_opp_table>; >> + >> + memory0_monitor1_opp_table: opp-table { >> + compatible = "operating-points-v2"; >> + >> + opp-1440000000 { >> + opp-hz = /bits/ 64 <1440000000 200000000>; >> + }; >> + >> + opp-2189000000 { >> + opp-hz = /bits/ 64 <2189000000 768000000>; >> + }; >> + >> + opp-2516000000 { >> + opp-hz = /bits/ 64 <2516000000 1555000000>; >> + }; >> + >> + opp-3860000000 { >> + opp-hz = /bits/ 64 <3860000000 4224000000>; >> + }; >> + }; >> + }; >> + }; >> + >> + memory-1 { >> + qcom,memory-type = <QCOM_MEM_TYPE_LLCC>; >> + freq-table-hz = /bits/ 64 <300000000 1067000000>; >> + >> + monitor-0 { >> + qcom,ipm-ceil = <20000000>; >> + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 >> + &CPU8 &CPU9 &CPU10 &CPU11>; >> + operating-points-v2 = <&memory1_monitor0_opp_table>; >> + >> + memory1_monitor0_opp_table: opp-table { >> + compatible = "operating-points-v2"; >> + >> + opp-999000000 { >> + opp-hz = /bits/ 64 <999000000 300000000>; >> + }; >> + >> + opp-1440000000 { >> + opp-hz = /bits/ 64 <1440000000 466000000>; >> + }; >> + >> + opp-1671000000 { >> + opp-hz = /bits/ 64 <1671000000 600000000>; >> + }; >> + >> + opp-2189000000 { >> + opp-hz = /bits/ 64 <2189000000 806000000>; >> + }; >> + >> + opp-2516000000 { >> + opp-hz = /bits/ 64 <2516000000 933000000>; >> + }; >> + >> + opp-3860000000 { >> + opp-hz = /bits/ 64 <3860000000 1066000000>; >> + }; >> + }; >> + }; >> + }; >> + >> + memory-2 { >> + qcom,memory-type = <QCOM_MEM_TYPE_DDR_QOS>; >> + freq-table-hz = /bits/ 64 <QCOM_DDR_LEVEL_AUTO QCOM_DDR_LEVEL_PERF>; > > This is definitely not 'frequency of the memory bys in Hz' ack, will change it. -Sibi > >> + >> + monitor-0 { >> + qcom,ipm-ceil = <20000000>; >> + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 >> + &CPU8 &CPU9 &CPU10 &CPU11>; >> + operating-points-v2 = <&memory2_monitor0_opp_table>; >> + >> + memory2_monitor0_opp_table: opp-table { >> + compatible = "operating-points-v2"; >> + >> + opp-2189000000 { >> + opp-hz = /bits/ 64 <2189000000>; >> + opp-level = <QCOM_DDR_LEVEL_AUTO>; >> + }; >> + >> + opp-3860000000 { >> + opp-hz = /bits/ 64 <3860000000>; >> + opp-level = <QCOM_DDR_LEVEL_PERF>; >> + }; >> + }; >> + }; >> + }; >> + }; >> + }; >> + }; >> diff --git a/include/dt-bindings/firmware/qcom,scmi-memlat.h b/include/dt-bindings/firmware/qcom,scmi-memlat.h >> new file mode 100644 >> index 000000000000..7ae8d8d5623b >> --- /dev/null >> +++ b/include/dt-bindings/firmware/qcom,scmi-memlat.h >> @@ -0,0 +1,22 @@ >> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ >> +/* >> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> +#ifndef __DT_BINDINGS_QCOM_SCMI_VENDOR_H >> +#define __DT_BINDINGS_QCOM_SCMI_VENDOR >> + >> +/* Memory IDs */ >> +#define QCOM_MEM_TYPE_DDR 0x0 >> +#define QCOM_MEM_TYPE_LLCC 0x1 >> +#define QCOM_MEM_TYPE_DDR_QOS 0x2 >> + >> +/* >> + * QCOM_MEM_TYPE_DDR_QOS supports the following states. >> + * >> + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled >> + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled >> + */ >> +#define QCOM_DDR_LEVEL_AUTO 0x0 >> +#define QCOM_DDR_LEVEL_PERF 0x1 >> + >> +#endif /* __DT_BINDINGS_QCOM_SCMI_VENDOR_H */ >> -- >> 2.34.1 >> >
On 10/8/24 17:41, Krzysztof Kozlowski wrote: > On 08/10/2024 14:10, Dmitry Baryshkov wrote: >> On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote: >>> On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote: >>>> +/* >>>> + * QCOM_MEM_TYPE_DDR_QOS supports the following states. >>>> + * >>>> + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled >>>> + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled >>>> + */ >>>> +#define QCOM_DDR_LEVEL_AUTO 0x0 >>>> +#define QCOM_DDR_LEVEL_PERF 0x1 >>> >>> I could not find any driver using these. Can you point me to usage in >>> the drivers? >> >> It's well hidden. These are the raw values used for DDR_QOS memory. > > So not a binding? Then these should be dropped. I am not sure why the term "well hidden" was even considered :( The driver just reads them from dt and passes them along. If you want the dt to list magic numbers 0/1 instead I can do that as well. -Sibi > > Best regards, > Krzysztof >
On 22/10/2024 09:25, Sibi Sankar wrote: > > > On 10/8/24 17:41, Krzysztof Kozlowski wrote: >> On 08/10/2024 14:10, Dmitry Baryshkov wrote: >>> On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote: >>>> On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote: >>>>> +/* >>>>> + * QCOM_MEM_TYPE_DDR_QOS supports the following states. >>>>> + * >>>>> + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled >>>>> + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled >>>>> + */ >>>>> +#define QCOM_DDR_LEVEL_AUTO 0x0 >>>>> +#define QCOM_DDR_LEVEL_PERF 0x1 >>>> >>>> I could not find any driver using these. Can you point me to usage in >>>> the drivers? >>> >>> It's well hidden. These are the raw values used for DDR_QOS memory. >> >> So not a binding? Then these should be dropped. > > I am not sure why the term "well hidden" was even considered :( > The driver just reads them from dt and passes them along. If you > want the dt to list magic numbers 0/1 instead I can do that as well. > If these are used by FW, then it's fine, although please document it clearly in comment. Best regards, Krzysztof
On Thu, Oct 24, 2024 at 03:29:05PM +0200, Krzysztof Kozlowski wrote: > On 22/10/2024 09:25, Sibi Sankar wrote: > > > > > > On 10/8/24 17:41, Krzysztof Kozlowski wrote: > >> On 08/10/2024 14:10, Dmitry Baryshkov wrote: > >>> On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote: > >>>> On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote: > >>>>> +/* > >>>>> + * QCOM_MEM_TYPE_DDR_QOS supports the following states. > >>>>> + * > >>>>> + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled > >>>>> + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled > >>>>> + */ > >>>>> +#define QCOM_DDR_LEVEL_AUTO 0x0 > >>>>> +#define QCOM_DDR_LEVEL_PERF 0x1 > >>>> > >>>> I could not find any driver using these. Can you point me to usage in > >>>> the drivers? > >>> > >>> It's well hidden. These are the raw values used for DDR_QOS memory. > >> > >> So not a binding? Then these should be dropped. > > > > I am not sure why the term "well hidden" was even considered :( > > The driver just reads them from dt and passes them along. If you > > want the dt to list magic numbers 0/1 instead I can do that as well. > > > > If these are used by FW, then it's fine, although please document it > clearly in comment. If they are used by FW but they are common between all platforms, it should go to the driver instead of being stuffed into DT.
On Tue, Oct 22, 2024 at 12:55:19PM +0530, Sibi Sankar wrote: > > > On 10/8/24 17:41, Krzysztof Kozlowski wrote: > > On 08/10/2024 14:10, Dmitry Baryshkov wrote: > > > On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote: > > > > On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote: > > > > > +/* > > > > > + * QCOM_MEM_TYPE_DDR_QOS supports the following states. > > > > > + * > > > > > + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled > > > > > + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled > > > > > + */ > > > > > +#define QCOM_DDR_LEVEL_AUTO 0x0 > > > > > +#define QCOM_DDR_LEVEL_PERF 0x1 > > > > > > > > I could not find any driver using these. Can you point me to usage in > > > > the drivers? > > > > > > It's well hidden. These are the raw values used for DDR_QOS memory. > > > > So not a binding? Then these should be dropped. > > I am not sure why the term "well hidden" was even considered :( > The driver just reads them from dt and passes them along. If you > want the dt to list magic numbers 0/1 instead I can do that as well. Well, it is well hidden, because e.g. Krzysztof could not find them being used. No, nobody asks for the magic numbers. Please drop them from DT and move to the driver instead. And this is one additional bonus point for the non-generic node names. ddr-qos is _different_ from all other "memory" types. It doesn't have min/max values.
On Tue, Oct 22, 2024 at 12:43:09PM +0530, Sibi Sankar wrote: > > > On 10/7/24 23:36, Dmitry Baryshkov wrote: > > On Mon, Oct 07, 2024 at 11:40:19AM GMT, Sibi Sankar wrote: > > > Document the various memory buses that can be monitored and scaled by > > > the memory latency governor hosted by the QCOM SCMI Generic Extension > > > Protocol v1.0. > > > > > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > > Hey Dmitry, > > Thanks for taking time to review the series! > > > > --- > > > > > > v3: > > > * Restructure the bindings to mimic IMX [Christian] > > > > > > .../bindings/firmware/arm,scmi.yaml | 1 + > > > .../bindings/firmware/qcom,scmi-memlat.yaml | 246 ++++++++++++++++++ > > > .../dt-bindings/firmware/qcom,scmi-memlat.h | 22 ++ > > > 3 files changed, 269 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > > > create mode 100644 include/dt-bindings/firmware/qcom,scmi-memlat.h > > > > > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > > > index 54d7d11bfed4..1d405f429168 100644 > > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > > > @@ -24,6 +24,7 @@ description: | > > > anyOf: > > > - $ref: /schemas/firmware/nxp,imx95-scmi.yaml > > > + - $ref: /schemas/firmware/qcom,scmi-memlat.yaml > > > properties: > > > $nodename: > > > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > > > new file mode 100644 > > > index 000000000000..0e8ea6dacd6a > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml > > > @@ -0,0 +1,246 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/firmware/qcom,scmi-memlat.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Qualcomm SCMI Memory Bus nodes > > > + > > > +maintainers: > > > + - Sibi Sankar <quic_sibis@quicinc.com> > > > + > > > +description: > > > + This binding describes the various memory buses that can be monitored and scaled > > > + by memory latency governor running on the CPU Control Processor (SCMI controller). > > > + > > > +properties: > > > + protocol@80: > > > + $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node' > > > + unevaluatedProperties: false > > > + > > > + properties: > > > + reg: > > > + const: 0x80 > > > + > > > + patternProperties: > > > + '^memory-[0-9]$': > > > + type: object > > > + unevaluatedProperties: false > > > + description: > > > + The list of all memory buses that can be monitored and scaled by the > > > + memory latency governor running on the SCMI controller. > > > + > > > + properties: > > > + qcom,memory-type: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [0, 1, 2] > > > + description: | > > > + Memory Bus Identifier > > > + 0 = QCOM_MEM_TYPE_DDR > > > + 1 = QCOM_MEM_TYPE_LLCC > > > + 2 = QCOM_MEM_TYPE_DDR_QOS > > > > I'm sorry if this has been discussed and frowned upon, but can you > > squash memory type into device node? > > I don't think anyone had any strong opinions on how the > nodes is to be named. We went with a generic node name since > it could accomodate multiple instances of llcc or ddr in the > future. We didn't want it be named ddr-0/ddr-1 and so on. So > I'll continue to stick with the current naming unless you have > a strong reason other than readability. As I wrote in the other email, the memory types are not equal. They have different properties, etc. Having non-generic names allows describing that in schema. Last, but not least, please consider how reserved memory nodes are handled nowadays: they have non-generic names, each one describing the purpose / kind. > > protocol@80 { > > ddr { > > }; > > > > llcc { > > }; > > > > ddr-qos { > > }; > > }; > > > > > + > > > + freq-table-hz: > > > + items: > > > + items: > > > + - description: Minimum frequency of the memory bus in Hz > > > + - description: Maximum frequency of the memory bus in Hz > > > > Does it make sense for the DDR-QOS type? Can we hardcode those values > > and drop freq-table-hz from the DDR-QOS node? > > > > Also, can we drop this completely by adding one extra OPP entry with the > > minimum memory bus frequency? > > the map table doesn't necessarily list all the supported > frequencies. It was made that way so that the table is flexible > enough that it doesn't have to be changed a lot across platforms. > Hence a need for a separate property to list min/max freq. Please use opp-supported-hw or other similar techniques to describe supported frequencies. > > > > > > + > > > + patternProperties: > > > + '^monitor-[0-9]$': > > > + type: object > > > + unevaluatedProperties: false > > > + description: > > > + The list of all monitors detecting the memory latency bound workloads using > > > + various counters. > > > + > > > + properties: > > > + qcom,compute-type: > > > + description: > > > + Monitors of type compute perform bus dvfs based on a rudimentary CPU > > > + frequency to memory frequency map. > > > + type: boolean > > > > This seems to be redundant. If there is no qcom,ipm-ceil property, then > > it's qcom,compute-type, isn't it? > > ack > > > > > > + > > > + qcom,ipm-ceil: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: > > > + Monitors having this property perform bus dvfs based on the same > > > + rudimentary table but the scaling is performed only if the calculated > > > + IPM (Instruction Per Misses) exceeds the given ceiling. > > > + > > > + cpus: > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > + description: > > > + Should be a list of phandles to CPU nodes (as described in > > > + Documentation/devicetree/bindings/arm/cpus.yaml). > > > > Which CPU nodes? I see that the examples list all CPUs here. Do we > > really need them? > > This observation is only valid for X1E where all the cpus have > identical freq charecteristics. Even with this case we need to > list them to handle cases where cpus gets disabled by the bootloader > on lower cored X1E parts i.e. we use this to figure out the actual > physical mask. This should be a part of the description. BTW, why do you need to remove bootloader-removed cores? Can you simply ignore non-existing CPUs instead? > > > > > > + > > > + operating-points-v2: true > > > + opp-table: > > > + type: object > > > + > > > + required: > > > + - cpus > > > + - operating-points-v2 > > > + > > > + oneOf: > > > + - required: [ 'qcom,compute-type' ] > > > + - required: [ 'qcom,ipm-ceil' ] > > > + > > > + required: > > > + - qcom,memory-type > > > + - freq-table-hz > > > + > > > +additionalProperties: true > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/firmware/qcom,scmi-memlat.h> > > > + > > > + firmware { > > > + scmi { > > > + compatible = "arm,scmi"; > > > + mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>; > > > + mbox-names = "tx", "rx"; > > > + shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>; > > > + > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + protocol@80 { > > > + reg = <0x80>; > > > + > > > + memory-0 { > > > + qcom,memory-type = <QCOM_MEM_TYPE_DDR>; > > > + freq-table-hz = /bits/ 64 <200000000 4224000000>; > > > + > > > + monitor-0 { > > > > Hmm. Can we say that each memory type can have at most one IPM and one > > compute aka "passive" memlat monitor? Does it make sense to use them as > > node names and drop the extra monitor-M names? > > Again this observation is valid only for X1E where the cpu freq > across cpu's are identical across clusters and is not true for > other mobile SoCs. So each memory can have more than 2 monitors > i.e. atleast one active/passibe monitor for each cluster. Description or commit message, please. > > > > > > + qcom,ipm-ceil = <20000000>; > > > + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 > > > + &CPU8 &CPU9 &CPU10 &CPU11>; > > > > Are CPU lists different between monitors? Can they be different? Can > > they be different between different memory types? > > same explanation. > > > > > > + operating-points-v2 = <&memory0_monitor0_opp_table>; > > > + > > > + memory0_monitor0_opp_table: opp-table { > > > > sensible names are better: > > I think I just picked these names up from a cpufreq table upstream. Doesn't mean that you can't be better than that :-D > > > > > ddr_ipm_opp_table: opp-table { > > }; > >
On 10/7/2024 12:10 AM, Sibi Sankar wrote: > diff --git a/include/dt-bindings/firmware/qcom,scmi-memlat.h b/include/dt-bindings/firmware/qcom,scmi-memlat.h > new file mode 100644 > index 000000000000..7ae8d8d5623b > --- /dev/null > +++ b/include/dt-bindings/firmware/qcom,scmi-memlat.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > +/* > + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. > + */ > +#ifndef __DT_BINDINGS_QCOM_SCMI_VENDOR_H > +#define __DT_BINDINGS_QCOM_SCMI_VENDOR The #define does not match the #ifndef (missing "_H") > + > +/* Memory IDs */ > +#define QCOM_MEM_TYPE_DDR 0x0 > +#define QCOM_MEM_TYPE_LLCC 0x1 > +#define QCOM_MEM_TYPE_DDR_QOS 0x2 > + > +/* > + * QCOM_MEM_TYPE_DDR_QOS supports the following states. > + * > + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled > + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled > + */ > +#define QCOM_DDR_LEVEL_AUTO 0x0 > +#define QCOM_DDR_LEVEL_PERF 0x1 > + > +#endif /* __DT_BINDINGS_QCOM_SCMI_VENDOR_H */
On 11/7/24 03:48, Jeffrey Hugo wrote: > On 10/7/2024 12:10 AM, Sibi Sankar wrote: >> diff --git a/include/dt-bindings/firmware/qcom,scmi-memlat.h >> b/include/dt-bindings/firmware/qcom,scmi-memlat.h >> new file mode 100644 >> index 000000000000..7ae8d8d5623b >> --- /dev/null >> +++ b/include/dt-bindings/firmware/qcom,scmi-memlat.h >> @@ -0,0 +1,22 @@ >> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ >> +/* >> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights >> reserved. >> + */ >> +#ifndef __DT_BINDINGS_QCOM_SCMI_VENDOR_H >> +#define __DT_BINDINGS_QCOM_SCMI_VENDOR > > The #define does not match the #ifndef (missing "_H") Thanks for catching this. Will fix it in the next re-spin. -Sibi > >> + >> +/* Memory IDs */ >> +#define QCOM_MEM_TYPE_DDR 0x0 >> +#define QCOM_MEM_TYPE_LLCC 0x1 >> +#define QCOM_MEM_TYPE_DDR_QOS 0x2 >> + >> +/* >> + * QCOM_MEM_TYPE_DDR_QOS supports the following states. >> + * >> + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled >> + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled >> + */ >> +#define QCOM_DDR_LEVEL_AUTO 0x0 >> +#define QCOM_DDR_LEVEL_PERF 0x1 >> + >> +#endif /* __DT_BINDINGS_QCOM_SCMI_VENDOR_H */ >
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml index 54d7d11bfed4..1d405f429168 100644 --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml @@ -24,6 +24,7 @@ description: | anyOf: - $ref: /schemas/firmware/nxp,imx95-scmi.yaml + - $ref: /schemas/firmware/qcom,scmi-memlat.yaml properties: $nodename: diff --git a/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml new file mode 100644 index 000000000000..0e8ea6dacd6a --- /dev/null +++ b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml @@ -0,0 +1,246 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/firmware/qcom,scmi-memlat.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm SCMI Memory Bus nodes + +maintainers: + - Sibi Sankar <quic_sibis@quicinc.com> + +description: + This binding describes the various memory buses that can be monitored and scaled + by memory latency governor running on the CPU Control Processor (SCMI controller). + +properties: + protocol@80: + $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node' + unevaluatedProperties: false + + properties: + reg: + const: 0x80 + + patternProperties: + '^memory-[0-9]$': + type: object + unevaluatedProperties: false + description: + The list of all memory buses that can be monitored and scaled by the + memory latency governor running on the SCMI controller. + + properties: + qcom,memory-type: + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2] + description: | + Memory Bus Identifier + 0 = QCOM_MEM_TYPE_DDR + 1 = QCOM_MEM_TYPE_LLCC + 2 = QCOM_MEM_TYPE_DDR_QOS + + freq-table-hz: + items: + items: + - description: Minimum frequency of the memory bus in Hz + - description: Maximum frequency of the memory bus in Hz + + patternProperties: + '^monitor-[0-9]$': + type: object + unevaluatedProperties: false + description: + The list of all monitors detecting the memory latency bound workloads using + various counters. + + properties: + qcom,compute-type: + description: + Monitors of type compute perform bus dvfs based on a rudimentary CPU + frequency to memory frequency map. + type: boolean + + qcom,ipm-ceil: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Monitors having this property perform bus dvfs based on the same + rudimentary table but the scaling is performed only if the calculated + IPM (Instruction Per Misses) exceeds the given ceiling. + + cpus: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: + Should be a list of phandles to CPU nodes (as described in + Documentation/devicetree/bindings/arm/cpus.yaml). + + operating-points-v2: true + opp-table: + type: object + + required: + - cpus + - operating-points-v2 + + oneOf: + - required: [ 'qcom,compute-type' ] + - required: [ 'qcom,ipm-ceil' ] + + required: + - qcom,memory-type + - freq-table-hz + +additionalProperties: true + +examples: + - | + #include <dt-bindings/firmware/qcom,scmi-memlat.h> + + firmware { + scmi { + compatible = "arm,scmi"; + mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>; + mbox-names = "tx", "rx"; + shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>; + + #address-cells = <1>; + #size-cells = <0>; + + protocol@80 { + reg = <0x80>; + + memory-0 { + qcom,memory-type = <QCOM_MEM_TYPE_DDR>; + freq-table-hz = /bits/ 64 <200000000 4224000000>; + + monitor-0 { + qcom,ipm-ceil = <20000000>; + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 + &CPU8 &CPU9 &CPU10 &CPU11>; + operating-points-v2 = <&memory0_monitor0_opp_table>; + + memory0_monitor0_opp_table: opp-table { + compatible = "operating-points-v2"; + + opp-999000000 { + opp-hz = /bits/ 64 <999000000 547000000>; + }; + + opp-1440000000 { + opp-hz = /bits/ 64 <1440000000 768000000>; + }; + + opp-1671000000 { + opp-hz = /bits/ 64 <1671000000 1555000000>; + }; + + opp-2189000000 { + opp-hz = /bits/ 64 <2189000000 2092000000>; + }; + + opp-2516000000 { + opp-hz = /bits/ 64 <2516000000 3187000000>; + }; + + opp-3860000000 { + opp-hz = /bits/ 64 <3860000000 4224000000>; + }; + }; + }; + + monitor-1 { + qcom,compute-type; + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 + &CPU8 &CPU9 &CPU10 &CPU11>; + operating-points-v2 = <&memory0_monitor1_opp_table>; + + memory0_monitor1_opp_table: opp-table { + compatible = "operating-points-v2"; + + opp-1440000000 { + opp-hz = /bits/ 64 <1440000000 200000000>; + }; + + opp-2189000000 { + opp-hz = /bits/ 64 <2189000000 768000000>; + }; + + opp-2516000000 { + opp-hz = /bits/ 64 <2516000000 1555000000>; + }; + + opp-3860000000 { + opp-hz = /bits/ 64 <3860000000 4224000000>; + }; + }; + }; + }; + + memory-1 { + qcom,memory-type = <QCOM_MEM_TYPE_LLCC>; + freq-table-hz = /bits/ 64 <300000000 1067000000>; + + monitor-0 { + qcom,ipm-ceil = <20000000>; + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 + &CPU8 &CPU9 &CPU10 &CPU11>; + operating-points-v2 = <&memory1_monitor0_opp_table>; + + memory1_monitor0_opp_table: opp-table { + compatible = "operating-points-v2"; + + opp-999000000 { + opp-hz = /bits/ 64 <999000000 300000000>; + }; + + opp-1440000000 { + opp-hz = /bits/ 64 <1440000000 466000000>; + }; + + opp-1671000000 { + opp-hz = /bits/ 64 <1671000000 600000000>; + }; + + opp-2189000000 { + opp-hz = /bits/ 64 <2189000000 806000000>; + }; + + opp-2516000000 { + opp-hz = /bits/ 64 <2516000000 933000000>; + }; + + opp-3860000000 { + opp-hz = /bits/ 64 <3860000000 1066000000>; + }; + }; + }; + }; + + memory-2 { + qcom,memory-type = <QCOM_MEM_TYPE_DDR_QOS>; + freq-table-hz = /bits/ 64 <QCOM_DDR_LEVEL_AUTO QCOM_DDR_LEVEL_PERF>; + + monitor-0 { + qcom,ipm-ceil = <20000000>; + cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 + &CPU8 &CPU9 &CPU10 &CPU11>; + operating-points-v2 = <&memory2_monitor0_opp_table>; + + memory2_monitor0_opp_table: opp-table { + compatible = "operating-points-v2"; + + opp-2189000000 { + opp-hz = /bits/ 64 <2189000000>; + opp-level = <QCOM_DDR_LEVEL_AUTO>; + }; + + opp-3860000000 { + opp-hz = /bits/ 64 <3860000000>; + opp-level = <QCOM_DDR_LEVEL_PERF>; + }; + }; + }; + }; + }; + }; + }; diff --git a/include/dt-bindings/firmware/qcom,scmi-memlat.h b/include/dt-bindings/firmware/qcom,scmi-memlat.h new file mode 100644 index 000000000000..7ae8d8d5623b --- /dev/null +++ b/include/dt-bindings/firmware/qcom,scmi-memlat.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ +/* + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. + */ +#ifndef __DT_BINDINGS_QCOM_SCMI_VENDOR_H +#define __DT_BINDINGS_QCOM_SCMI_VENDOR + +/* Memory IDs */ +#define QCOM_MEM_TYPE_DDR 0x0 +#define QCOM_MEM_TYPE_LLCC 0x1 +#define QCOM_MEM_TYPE_DDR_QOS 0x2 + +/* + * QCOM_MEM_TYPE_DDR_QOS supports the following states. + * + * %QCOM_DDR_LEVEL_AUTO: DDR operates with LPM enabled + * %QCOM_DDR_LEVEL_PERF: DDR operates with LPM disabled + */ +#define QCOM_DDR_LEVEL_AUTO 0x0 +#define QCOM_DDR_LEVEL_PERF 0x1 + +#endif /* __DT_BINDINGS_QCOM_SCMI_VENDOR_H */
Document the various memory buses that can be monitored and scaled by the memory latency governor hosted by the QCOM SCMI Generic Extension Protocol v1.0. Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> --- v3: * Restructure the bindings to mimic IMX [Christian] .../bindings/firmware/arm,scmi.yaml | 1 + .../bindings/firmware/qcom,scmi-memlat.yaml | 246 ++++++++++++++++++ .../dt-bindings/firmware/qcom,scmi-memlat.h | 22 ++ 3 files changed, 269 insertions(+) create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml create mode 100644 include/dt-bindings/firmware/qcom,scmi-memlat.h