| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <911483df-ee09-033d-7fae-4090bf8a3713@quicinc.com>
Date: Tue, 22 Oct 2024 12:43:09 +0530
From: Sibi Sankar <quic_sibis@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <sudeep.holla@....com>, <cristian.marussi@....com>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <quic_rgottimu@...cinc.com>,
<quic_kshivnan@...cinc.com>, <conor+dt@...nel.org>,
<arm-scmi@...r.kernel.org>
Subject: Re: [PATCH V4 1/5] dt-bindings: firmware: Document bindings for QCOM
SCMI Generic Extension
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@...cinc.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@...cinc.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
>>
>
Powered by blists - more mailing lists