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@xxxxxxxxxxx> > > 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@xxxxxxxxxxx> > > > + > > > +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 { > > }; > > -- With best wishes Dmitry
