Received-SPF: pass (google.com: domain of linux-kernel+bounces-215481-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <4d21f140-351e-da3f-57b4-3041fb8684e9@quicinc.com>
Date: Sat, 15 Jun 2024 02:00:12 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [RFC V2 1/4] dt-bindings: firmware: Add support for QCOM Vendor
 Protocol
Content-Language: en-US
To: Rob Herring <robh@kernel.org>
CC: <sudeep.holla@arm.com>, <cristian.marussi@arm.com>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>,
        <quic_rgottimu@quicinc.com>, <quic_kshivnan@quicinc.com>,
        <conor+dt@kernel.org>
References: <20240612183031.219906-1-quic_sibis@quicinc.com>
 <20240612183031.219906-2-quic_sibis@quicinc.com>
 <20240613193625.GA2338851-robh@kernel.org>
From: Sibi Sankar <quic_sibis@quicinc.com>
In-Reply-To: <20240613193625.GA2338851-robh@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 6/14/24 01:06, Rob Herring wrote:
> On Thu, Jun 13, 2024 at 12:00:28AM +0530, Sibi Sankar wrote:
>> Document the SCMI QCOM Vendor protocol v1.0 bindings and the various memory
>> buses that can be monitored and scaled by memory latency governor hosted
>> on it.
>>

Hey Rob,

Thanks for taking time to review the series :)

>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> v1:
>> * Add missing bindings for the protocol. [Konrad/Dmitry]
>> * Use alternate bindings. [Dmitry/Konrad]
>>
>>   .../bindings/firmware/arm,scmi.yaml           |  21 ++
>>   .../bindings/soc/qcom/qcom,scmi-memlat.yaml   | 243 ++++++++++++++++++
>>   include/dt-bindings/soc/qcom,scmi-vendor.h    |  22 ++
>>   3 files changed, 286 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,scmi-memlat.yaml
>>   create mode 100644 include/dt-bindings/soc/qcom,scmi-vendor.h
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 7de2c29606e5..21e4da53d02c 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -278,6 +278,27 @@ properties:
>>       required:
>>         - reg
>>   
>> +  protocol@80:
>> +    $ref: '#/$defs/protocol-node'
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      reg:
>> +        const: 0x80
>> +
>> +      memlat-dvfs:
> 
> I don't see the purpose of this container node.

Thanks, looks like I took a shortcut to show that the memory nodes
are logically placed with this abstraction. I'll should be able to
get it removed and achieve the same functionality.

> 
>> +        type: object
>> +        additionalProperties: false
>> +        description:
>> +          The list of all memory buses that can be monitored and scaled by the
>> +          memory latency governor running on the SCMI controller.
>> +
>> +        patternProperties:
>> +          '^memory-[0-9]$':
>> +            type: object
>> +            $ref: /schemas/soc/qcom/qcom,scmi-memlat.yaml#
> 
> This schema needs to be at the level of the protocol node. See the i.MX
> SCMI pinctrl patches for more details on what it should look like.

Thanks will take a look at that.

> 
>> +            unevaluatedProperties: false
>> +
>>   additionalProperties: false
>>   
>>   $defs:
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,scmi-memlat.yaml
>> new file mode 100644
>> index 000000000000..c6e3d163c4a3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,scmi-memlat.yaml
>> @@ -0,0 +1,243 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/qcom/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: |
> 
> Doesn't need '|' if no formatting.

ack

> 
>> +  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:
>> +  qcom,memory-type:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1, 2]
>> +    description:
> 
> Needs a '|' if you want your formatting preserved.

ack

> 
>> +      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.
>> +
>> +      qcom,cpulist:
>> +        $ref: /schemas/types.yaml#/definitions/phandle-array
>> +        description:
>> +          List of phandles to the CPUs nodes whose frequency and IPM are to be
>> +          monitored.
> 
> We have a standard property for this: cpus

will use ^^ in the next re-spin

> 
>> +
>> +      operating-points-v2: true
>> +      opp-table:
>> +        type: object
>> +
>> +    required:
>> +      - qcom,cpulist
>> +      - operating-points-v2
>> +      - opp-table
>> +
> 
>> +    allOf:
>> +      - if:
>> +          properties:
>> +            qcom,compute-type: false
>> +        then:
>> +          required:
>> +            - qcom,ipm-ceil
>> +
>> +      - if:
>> +          properties:
>> +            qcom,ipm-ceil: false
>> +        then:
>> +          required:
>> +            - qcom,compute-type
> 
> Isn't all this just:
> 
> oneOf:
>    - required: [ qcom,compute-type ]
>    - required: [ qcom,ipm-ceil ]

ack, will use ^^ in the next re-spin.

-Sibi

> 
> 
>> +
>> +required:
>> +  - qcom,memory-type
>> +  - freq-table-hz
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/soc/qcom,scmi-vendor.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>;
>> +
>> +                memlat-dvfs {
>> +                    memory-0 {
>> +                        qcom,memory-type = <QCOM_MEM_TYPE_DDR>;
>> +                        freq-table-hz = /bits/ 64 <200000000 4224000000>;
>> +
>> +                        monitor-0 {
>> +                            qcom,ipm-ceil = <20000000>;
>> +                            qcom,cpulist = <&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;
>> +                            qcom,cpulist = <&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>;
>> +                            qcom,cpulist = <&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>;
>> +                            qcom,cpulist = <&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/soc/qcom,scmi-vendor.h b/include/dt-bindings/soc/qcom,scmi-vendor.h
>> new file mode 100644
>> index 000000000000..7ae8d8d5623b
>> --- /dev/null
>> +++ b/include/dt-bindings/soc/qcom,scmi-vendor.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
>>