[RFC 0/2] Add support for SCMI QTI Memlat Vendor Protocol

[RFC 0/2] Add support for SCMI QTI Memlat Vendor Protocol

[Sibi Sankar]

The patch series documents the bindings and adds support for the
SCMI QTI memlat (memory latency) vendor protocol. The protocol takes
in several tuneables including the IPM ratio (Instructions Per Miss),
bus bandwidth requirements and PMU maps to enable frequency scaling
of various buses (L3/LLCC/DDR). The scaling is performed by the HW
memory latency governor running on the CPUSS Control Processor.

Depends on CPUCP mailbox driver:
https://patchwork.kernel.org/project/linux-arm-msm/cover/1663135386-26270-1-git-send-email-quic_sibis@quicinc.com/

Sibi Sankar (2):
  dt-bindings: firmware: arm,scmi: Add support for memlat vendor
    protocol
  firmware: arm_scmi: Add SCMI QTI Memlat vendor protocol

 .../devicetree/bindings/firmware/arm,scmi.yaml     | 164 +++++++++++++
 drivers/firmware/arm_scmi/Kconfig                  |  10 +
 drivers/firmware/arm_scmi/Makefile                 |   1 +
 drivers/firmware/arm_scmi/qcom_memlat_vendor.c     | 269 +++++++++++++++++++++
 include/linux/scmi_protocol.h                      |  36 +++
 5 files changed, 480 insertions(+)
 create mode 100644 drivers/firmware/arm_scmi/qcom_memlat_vendor.c

-- 
2.7.4

[RFC 1/2] dt-bindings: firmware: arm,scmi: Add support for memlat vendor protocol

[Sibi Sankar]

Add bindings support for the SCMI QTI memlat (memory latency) vendor
protocol. The memlat vendor protocol enables the frequency scaling of
various buses (L3/LLCC/DDR) based on the memory latency governor
running on the CPUSS Control Processor.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml     | 164 +++++++++++++++++++++
 1 file changed, 164 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 1c0388da6721..efc8a5a8bffe 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -189,6 +189,47 @@ properties:
       reg:
         const: 0x18
 
+  protocol@80:
+    type: object
+    properties:
+      reg:
+        const: 0x80
+
+      qcom,bus-type:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        items:
+          minItems: 1
+        description:
+          Identifier of the bus type to be scaled by the memlat protocol.
+
+      cpu-map:
+        type: object
+        description:
+          The list of all cpu cluster configurations to be tracked by the memlat protocol
+
+        patternProperties:
+          '^cluster[0-9]':
+            type: object
+            description:
+              Each cluster node describes the frequency domain associated with the
+              CPUFREQ HW engine and bandwidth requirements of the buses to be scaled.
+
+            properties:
+              operating-points-v2: true
+
+              qcom,freq-domain:
+                $ref: /schemas/types.yaml#/definitions/phandle-array
+                description:
+                  Reference to the frequency domain of the CPUFREQ HW engine
+                items:
+                  - items:
+                      - description: phandle to CPUFREQ HW engine
+                      - description: frequency domain associated with the cluster
+
+            required:
+              - qcom,freq-domain
+              - operating-points-v2
+
 additionalProperties: false
 
 patternProperties:
@@ -429,4 +470,127 @@ examples:
         };
     };
 
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    firmware {
+        scmi {
+            compatible = "arm,scmi";
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            mboxes = <&cpucp_mbox>;
+            mbox-names = "tx";
+            shmem = <&cpu_scp_lpri>;
+
+            scmi_memlat: protocol@80 {
+                reg = <0x80>;
+                qcom,bus-type = <0x2>;
+
+                cpu-map {
+                    cluster0 {
+                        qcom,freq-domain = <&cpufreq_hw 0>;
+                        operating-points-v2 = <&cpu0_opp_table>;
+                    };
+
+                    cluster1 {
+                        qcom,freq-domain = <&cpufreq_hw 1>;
+                        operating-points-v2 = <&cpu4_opp_table>;
+                    };
+
+                    cluster2 {
+                        qcom,freq-domain = <&cpufreq_hw 2>;
+                        operating-points-v2 = <&cpu7_opp_table>;
+                    };
+                };
+            };
+        };
+
+        cpu0_opp_table: opp-table-cpu0 {
+            compatible = "operating-points-v2";
+
+            cpu0_opp_300mhz: opp-300000000 {
+                opp-hz = /bits/ 64 <300000000>;
+                opp-peak-kBps = <9600000>;
+            };
+
+            cpu0_opp_1325mhz: opp-1324800000 {
+                opp-hz = /bits/ 64 <1324800000>;
+                opp-peak-kBps = <33792000>;
+            };
+
+            cpu0_opp_2016mhz: opp-2016000000 {
+                opp-hz = /bits/ 64 <2016000000>;
+                opp-peak-kBps = <48537600>;
+            };
+        };
+
+        cpu4_opp_table: opp-table-cpu4 {
+            compatible = "operating-points-v2";
+
+            cpu4_opp_691mhz: opp-691200000 {
+                opp-hz = /bits/ 64 <691200000>;
+                opp-peak-kBps = <9600000>;
+            };
+
+            cpu4_opp_941mhz: opp-940800000 {
+                opp-hz = /bits/ 64 <940800000>;
+                opp-peak-kBps = <17817600>;
+            };
+
+            cpu4_opp_2611mhz: opp-2611200000 {
+                opp-hz = /bits/ 64 <2611200000>;
+                opp-peak-kBps = <48537600>;
+            };
+        };
+
+        cpu7_opp_table: opp-table-cpu7 {
+            compatible = "operating-points-v2";
+
+            cpu7_opp_806mhz: opp-806400000 {
+                opp-hz = /bits/ 64 <806400000>;
+                opp-peak-kBps = <9600000>;
+            };
+
+            cpu7_opp_2381mhz: opp-2380800000 {
+                opp-hz = /bits/ 64 <2380800000>;
+                opp-peak-kBps = <44851200>;
+            };
+
+            cpu7_opp_2515mhz: opp-2515200000 {
+                opp-hz = /bits/ 64 <2515200000>;
+                opp-peak-kBps = <48537600>;
+            };
+        };
+    };
+
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        cpucp_mbox: mailbox@17400000 {
+            compatible = "qcom,cpucp-mbox";
+            reg =   <0x0 0x17c00000 0x0 0x10>, <0x0 0x18590300 0x0 0x700>;
+            interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
+            #mbox-cells = <0>;
+        };
+
+        sram@18509400 {
+            compatible = "mmio-sram";
+            reg = <0x0 0x18509400 0x0 0x400>;
+            no-memory-wc;
+
+            #address-cells = <1>;
+            #size-cells = <1>;
+            ranges = <0x0 0x0 0x18509400 0x400>;
+
+            cpu_scp_lpri: scp-sram-section@0 {
+                compatible = "arm,scmi-shmem";
+                reg = <0x0 0x80>;
+            };
+        };
+    };
+
 ...
-- 
2.7.4

Re: [RFC 1/2] dt-bindings: firmware: arm,scmi: Add support for memlat vendor protocol

[Sudeep Holla]

On Thu, Nov 03, 2022 at 10:28:31AM +0530, Sibi Sankar wrote:
> Add bindings support for the SCMI QTI memlat (memory latency) vendor
> protocol. The memlat vendor protocol enables the frequency scaling of
> various buses (L3/LLCC/DDR) based on the memory latency governor
> running on the CPUSS Control Processor.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  .../devicetree/bindings/firmware/arm,scmi.yaml     | 164 +++++++++++++++++++++
>  1 file changed, 164 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 1c0388da6721..efc8a5a8bffe 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -189,6 +189,47 @@ properties:
>        reg:
>          const: 0x18
>  
> +  protocol@80:
> +    type: object
> +    properties:
> +      reg:
> +        const: 0x80
> +
> +      qcom,bus-type:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          minItems: 1
> +        description:
> +          Identifier of the bus type to be scaled by the memlat protocol.
> +

Why is this part of the provider of the service ?

> +      cpu-map:
> +        type: object
> +        description:
> +          The list of all cpu cluster configurations to be tracked by the memlat protocol
> +
> +        patternProperties:
> +          '^cluster[0-9]':
> +            type: object
> +            description:
> +              Each cluster node describes the frequency domain associated with the
> +              CPUFREQ HW engine and bandwidth requirements of the buses to be scaled.
> +
> +            properties:
> +              operating-points-v2: true
> +
> +              qcom,freq-domain:
> +                $ref: /schemas/types.yaml#/definitions/phandle-array
> +                description:
> +                  Reference to the frequency domain of the CPUFREQ HW engine
> +                items:
> +                  - items:
> +                      - description: phandle to CPUFREQ HW engine
> +                      - description: frequency domain associated with the cluster
> +
> +            required:
> +              - qcom,freq-domain
> +              - operating-points-v2
> +

I would avoid all these here as part of provider node. It should be part
of the consumer to have all these details and do what it needs to do with
any such information.

-- 
Regards,
Sudeep

Re: [RFC 1/2] dt-bindings: firmware: arm,scmi: Add support for memlat vendor protocol

[Rob Herring]

On Thu, 03 Nov 2022 10:28:31 +0530, Sibi Sankar wrote:
> Add bindings support for the SCMI QTI memlat (memory latency) vendor
> protocol. The memlat vendor protocol enables the frequency scaling of
> various buses (L3/LLCC/DDR) based on the memory latency governor
> running on the CPUSS Control Processor.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  .../devicetree/bindings/firmware/arm,scmi.yaml     | 164 +++++++++++++++++++++
>  1 file changed, 164 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/firmware/arm,scmi.example.dtb: scmi: mbox-names: ['tx'] is too short
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
Documentation/devicetree/bindings/firmware/arm,scmi.example.dtb:0:0: /example-3/soc/mailbox@17400000: failed to match any schema with compatible: ['qcom,cpucp-mbox']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [RFC 1/2] dt-bindings: firmware: arm,scmi: Add support for memlat vendor protocol

[Rob Herring]

On Thu, Nov 03, 2022 at 10:28:31AM +0530, Sibi Sankar wrote:
> Add bindings support for the SCMI QTI memlat (memory latency) vendor
> protocol. The memlat vendor protocol enables the frequency scaling of
> various buses (L3/LLCC/DDR) based on the memory latency governor
> running on the CPUSS Control Processor.

I thought the interconnect binding was what provided details for bus 
scaling.

> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  .../devicetree/bindings/firmware/arm,scmi.yaml     | 164 +++++++++++++++++++++
>  1 file changed, 164 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 1c0388da6721..efc8a5a8bffe 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -189,6 +189,47 @@ properties:
>        reg:
>          const: 0x18
>  
> +  protocol@80:
> +    type: object
> +    properties:
> +      reg:
> +        const: 0x80
> +
> +      qcom,bus-type:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          minItems: 1
> +        description:
> +          Identifier of the bus type to be scaled by the memlat protocol.
> +
> +      cpu-map:

cpu-map only goes under /cpus node.

> +        type: object
> +        description:
> +          The list of all cpu cluster configurations to be tracked by the memlat protocol
> +
> +        patternProperties:
> +          '^cluster[0-9]':
> +            type: object
> +            description:
> +              Each cluster node describes the frequency domain associated with the
> +              CPUFREQ HW engine and bandwidth requirements of the buses to be scaled.
> +
> +            properties:

cpu-map nodes don't have properties.

> +              operating-points-v2: true
> +
> +              qcom,freq-domain:

Please don't add new users of this. Use the performance-domains binding 
instead.

> +                $ref: /schemas/types.yaml#/definitions/phandle-array
> +                description:
> +                  Reference to the frequency domain of the CPUFREQ HW engine
> +                items:
> +                  - items:
> +                      - description: phandle to CPUFREQ HW engine
> +                      - description: frequency domain associated with the cluster
> +
> +            required:
> +              - qcom,freq-domain
> +              - operating-points-v2
> +
>  additionalProperties: false
>  
>  patternProperties:
> @@ -429,4 +470,127 @@ examples:
>          };
>      };
>  
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    firmware {
> +        scmi {
> +            compatible = "arm,scmi";
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            mboxes = <&cpucp_mbox>;
> +            mbox-names = "tx";
> +            shmem = <&cpu_scp_lpri>;
> +
> +            scmi_memlat: protocol@80 {
> +                reg = <0x80>;
> +                qcom,bus-type = <0x2>;
> +
> +                cpu-map {
> +                    cluster0 {
> +                        qcom,freq-domain = <&cpufreq_hw 0>;
> +                        operating-points-v2 = <&cpu0_opp_table>;
> +                    };
> +
> +                    cluster1 {
> +                        qcom,freq-domain = <&cpufreq_hw 1>;
> +                        operating-points-v2 = <&cpu4_opp_table>;
> +                    };
> +
> +                    cluster2 {
> +                        qcom,freq-domain = <&cpufreq_hw 2>;
> +                        operating-points-v2 = <&cpu7_opp_table>;
> +                    };
> +                };
> +            };
> +        };
> +
> +        cpu0_opp_table: opp-table-cpu0 {
> +            compatible = "operating-points-v2";
> +
> +            cpu0_opp_300mhz: opp-300000000 {
> +                opp-hz = /bits/ 64 <300000000>;
> +                opp-peak-kBps = <9600000>;
> +            };
> +
> +            cpu0_opp_1325mhz: opp-1324800000 {
> +                opp-hz = /bits/ 64 <1324800000>;
> +                opp-peak-kBps = <33792000>;
> +            };
> +
> +            cpu0_opp_2016mhz: opp-2016000000 {
> +                opp-hz = /bits/ 64 <2016000000>;
> +                opp-peak-kBps = <48537600>;
> +            };
> +        };
> +
> +        cpu4_opp_table: opp-table-cpu4 {
> +            compatible = "operating-points-v2";
> +
> +            cpu4_opp_691mhz: opp-691200000 {
> +                opp-hz = /bits/ 64 <691200000>;
> +                opp-peak-kBps = <9600000>;
> +            };
> +
> +            cpu4_opp_941mhz: opp-940800000 {
> +                opp-hz = /bits/ 64 <940800000>;
> +                opp-peak-kBps = <17817600>;
> +            };
> +
> +            cpu4_opp_2611mhz: opp-2611200000 {
> +                opp-hz = /bits/ 64 <2611200000>;
> +                opp-peak-kBps = <48537600>;
> +            };
> +        };
> +
> +        cpu7_opp_table: opp-table-cpu7 {
> +            compatible = "operating-points-v2";
> +
> +            cpu7_opp_806mhz: opp-806400000 {
> +                opp-hz = /bits/ 64 <806400000>;
> +                opp-peak-kBps = <9600000>;
> +            };
> +
> +            cpu7_opp_2381mhz: opp-2380800000 {
> +                opp-hz = /bits/ 64 <2380800000>;
> +                opp-peak-kBps = <44851200>;
> +            };
> +
> +            cpu7_opp_2515mhz: opp-2515200000 {
> +                opp-hz = /bits/ 64 <2515200000>;
> +                opp-peak-kBps = <48537600>;
> +            };
> +        };
> +    };
> +
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        cpucp_mbox: mailbox@17400000 {
> +            compatible = "qcom,cpucp-mbox";
> +            reg =   <0x0 0x17c00000 0x0 0x10>, <0x0 0x18590300 0x0 0x700>;
> +            interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
> +            #mbox-cells = <0>;
> +        };
> +
> +        sram@18509400 {
> +            compatible = "mmio-sram";
> +            reg = <0x0 0x18509400 0x0 0x400>;
> +            no-memory-wc;
> +
> +            #address-cells = <1>;
> +            #size-cells = <1>;
> +            ranges = <0x0 0x0 0x18509400 0x400>;
> +
> +            cpu_scp_lpri: scp-sram-section@0 {
> +                compatible = "arm,scmi-shmem";
> +                reg = <0x0 0x80>;
> +            };
> +        };
> +    };
> +
>  ...
> -- 
> 2.7.4
> 
> 

Re: [RFC 1/2] dt-bindings: firmware: arm,scmi: Add support for memlat vendor protocol

[Sibi Sankar]

Hey Rob,
Thanks for taking time to review the series.

On 11/4/22 23:33, Rob Herring wrote:
> On Thu, Nov 03, 2022 at 10:28:31AM +0530, Sibi Sankar wrote:
>> Add bindings support for the SCMI QTI memlat (memory latency) vendor
>> protocol. The memlat vendor protocol enables the frequency scaling of
>> various buses (L3/LLCC/DDR) based on the memory latency governor
>> running on the CPUSS Control Processor.
> 
> I thought the interconnect binding was what provided details for bus
> scaling.

The bus scaling in this particular case is done by SCP FW and not
from any kernel client. The SCMI vendor protocol would be used to
pass on the bandwidth requirements during initialization and SCP FW
would vote on it independently after it is

> 
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   .../devicetree/bindings/firmware/arm,scmi.yaml     | 164 +++++++++++++++++++++
>>   1 file changed, 164 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 1c0388da6721..efc8a5a8bffe 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -189,6 +189,47 @@ properties:
>>         reg:
>>           const: 0x18
>>   
>> +  protocol@80:
>> +    type: object
>> +    properties:
>> +      reg:
>> +        const: 0x80
>> +
>> +      qcom,bus-type:
>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>> +        items:
>> +          minItems: 1
>> +        description:
>> +          Identifier of the bus type to be scaled by the memlat protocol.
>> +
>> +      cpu-map:
> 
> cpu-map only goes under /cpus node.

sure will use a qcom specific node instead

> 
>> +        type: object
>> +        description:
>> +          The list of all cpu cluster configurations to be tracked by the memlat protocol
>> +
>> +        patternProperties:
>> +          '^cluster[0-9]':
>> +            type: object
>> +            description:
>> +              Each cluster node describes the frequency domain associated with the
>> +              CPUFREQ HW engine and bandwidth requirements of the buses to be scaled.
>> +
>> +            properties:
> 
> cpu-map nodes don't have properties.

ack

> 
>> +              operating-points-v2: true
>> +
>> +              qcom,freq-domain:
> 
> Please don't add new users of this. Use the performance-domains binding
> instead.

The plan was to re-use the ^^ to determine frequency domain of
the cpus since they are already present in the dts. I guess using
performance-domains bindings would require a corresponding change in
qcom-cpufreq-hw driver as well. Ack.

> 
>> +                $ref: /schemas/types.yaml#/definitions/phandle-array
>> +                description:
>> +                  Reference to the frequency domain of the CPUFREQ HW engine
>> +                items:
>> +                  - items:
>> +                      - description: phandle to CPUFREQ HW engine
>> +                      - description: frequency domain associated with the cluster
>> +
>> +            required:
>> +              - qcom,freq-domain
>> +              - operating-points-v2
>> +
>>   additionalProperties: false
>>   
>>   patternProperties:
>> @@ -429,4 +470,127 @@ examples:
>>           };
>>       };
>>   
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    firmware {
>> +        scmi {
>> +            compatible = "arm,scmi";
>> +
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            mboxes = <&cpucp_mbox>;
>> +            mbox-names = "tx";
>> +            shmem = <&cpu_scp_lpri>;
>> +
>> +            scmi_memlat: protocol@80 {
>> +                reg = <0x80>;
>> +                qcom,bus-type = <0x2>;
>> +
>> +                cpu-map {
>> +                    cluster0 {
>> +                        qcom,freq-domain = <&cpufreq_hw 0>;
>> +                        operating-points-v2 = <&cpu0_opp_table>;
>> +                    };
>> +
>> +                    cluster1 {
>> +                        qcom,freq-domain = <&cpufreq_hw 1>;
>> +                        operating-points-v2 = <&cpu4_opp_table>;
>> +                    };
>> +
>> +                    cluster2 {
>> +                        qcom,freq-domain = <&cpufreq_hw 2>;
>> +                        operating-points-v2 = <&cpu7_opp_table>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +
>> +        cpu0_opp_table: opp-table-cpu0 {
>> +            compatible = "operating-points-v2";
>> +
>> +            cpu0_opp_300mhz: opp-300000000 {
>> +                opp-hz = /bits/ 64 <300000000>;
>> +                opp-peak-kBps = <9600000>;
>> +            };
>> +
>> +            cpu0_opp_1325mhz: opp-1324800000 {
>> +                opp-hz = /bits/ 64 <1324800000>;
>> +                opp-peak-kBps = <33792000>;
>> +            };
>> +
>> +            cpu0_opp_2016mhz: opp-2016000000 {
>> +                opp-hz = /bits/ 64 <2016000000>;
>> +                opp-peak-kBps = <48537600>;
>> +            };
>> +        };
>> +
>> +        cpu4_opp_table: opp-table-cpu4 {
>> +            compatible = "operating-points-v2";
>> +
>> +            cpu4_opp_691mhz: opp-691200000 {
>> +                opp-hz = /bits/ 64 <691200000>;
>> +                opp-peak-kBps = <9600000>;
>> +            };
>> +
>> +            cpu4_opp_941mhz: opp-940800000 {
>> +                opp-hz = /bits/ 64 <940800000>;
>> +                opp-peak-kBps = <17817600>;
>> +            };
>> +
>> +            cpu4_opp_2611mhz: opp-2611200000 {
>> +                opp-hz = /bits/ 64 <2611200000>;
>> +                opp-peak-kBps = <48537600>;
>> +            };
>> +        };
>> +
>> +        cpu7_opp_table: opp-table-cpu7 {
>> +            compatible = "operating-points-v2";
>> +
>> +            cpu7_opp_806mhz: opp-806400000 {
>> +                opp-hz = /bits/ 64 <806400000>;
>> +                opp-peak-kBps = <9600000>;
>> +            };
>> +
>> +            cpu7_opp_2381mhz: opp-2380800000 {
>> +                opp-hz = /bits/ 64 <2380800000>;
>> +                opp-peak-kBps = <44851200>;
>> +            };
>> +
>> +            cpu7_opp_2515mhz: opp-2515200000 {
>> +                opp-hz = /bits/ 64 <2515200000>;
>> +                opp-peak-kBps = <48537600>;
>> +            };
>> +        };
>> +    };
>> +
>> +
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        cpucp_mbox: mailbox@17400000 {
>> +            compatible = "qcom,cpucp-mbox";
>> +            reg =   <0x0 0x17c00000 0x0 0x10>, <0x0 0x18590300 0x0 0x700>;
>> +            interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>> +            #mbox-cells = <0>;
>> +        };
>> +
>> +        sram@18509400 {
>> +            compatible = "mmio-sram";
>> +            reg = <0x0 0x18509400 0x0 0x400>;
>> +            no-memory-wc;
>> +
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +            ranges = <0x0 0x0 0x18509400 0x400>;
>> +
>> +            cpu_scp_lpri: scp-sram-section@0 {
>> +                compatible = "arm,scmi-shmem";
>> +                reg = <0x0 0x80>;
>> +            };
>> +        };
>> +    };
>> +
>>   ...
>> -- 
>> 2.7.4
>>
>>

[RFC 2/2] firmware: arm_scmi: Add SCMI QTI Memlat vendor protocol

[Sibi Sankar]

Add support for the SCMI QTI memlat (memory latency) vendor protocol.
The QTI memlat vendor protocol takes in several tuneables including the
IPM ratio (Instructions Per Miss), bus bandwidth requirements and PMU
maps to enable frequency scaling of various buses (L3/LLCC/DDR) performed
by the memory latency governor running on the CPUSS Control Processor.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 drivers/firmware/arm_scmi/Kconfig              |  10 +
 drivers/firmware/arm_scmi/Makefile             |   1 +
 drivers/firmware/arm_scmi/qcom_memlat_vendor.c | 269 +++++++++++++++++++++++++
 include/linux/scmi_protocol.h                  |  36 ++++
 4 files changed, 316 insertions(+)
 create mode 100644 drivers/firmware/arm_scmi/qcom_memlat_vendor.c

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index a14f65444b35..814a3fc37dc1 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -136,6 +136,16 @@ config ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
 
 endif #ARM_SCMI_PROTOCOL
 
+config QTI_SCMI_MEMLAT_PROTOCOL
+	tristate "Qualcomm Technologies, Inc. SCMI MEMLAT vendor Protocol"
+	depends on ARM_SCMI_PROTOCOL && QCOM_CPUCP_MBOX
+	help
+	  The SCMI QTI memlat vendor protocol adds support for the frequency
+	  scaling of buses (L3/LLCC/DDR) by the QTI HW memlat governor running
+	  on the CPUSS Control Processor (CPUCP).
+
+	  Say Y here if you want to build this driver.
+
 config ARM_SCMI_POWER_DOMAIN
 	tristate "SCMI power domain driver"
 	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 9ea86f8cc8f7..78e6d72fb9bb 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -11,6 +11,7 @@ scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o volt
 scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
 		    $(scmi-transport-y)
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
+obj-$(CONFIG_QTI_SCMI_MEMLAT_PROTOCOL) += qcom_memlat_vendor.o
 obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
 obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
 
diff --git a/drivers/firmware/arm_scmi/qcom_memlat_vendor.c b/drivers/firmware/arm_scmi/qcom_memlat_vendor.c
new file mode 100644
index 000000000000..4b7db309e633
--- /dev/null
+++ b/drivers/firmware/arm_scmi/qcom_memlat_vendor.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/scmi_protocol.h>
+
+#include "protocols.h"
+
+#define MAX_MAP_ENTRIES 14
+#define MAX_PMU_ENTRIES 24
+
+enum scmi_memlat_protocol_cmd {
+	MEMLAT_SET_CPU_GROUP = 0x10,
+	MEMLAT_SET_MONITOR = 0x11,
+	MEMLAT_COMMON_PMU_MAP = 0x12,
+	MEMLAT_MON_PMU_MAP = 0x13,
+	MEMLAT_IPM_RATIO = 0x14,
+	MEMLAT_STALL_RATIO = 0x15,
+	MEMLAT_SAMPLE_MS = 0x18,
+	MEMLAT_MON_FREQ_MAP = 0x19,
+	MEMLAT_START_MONITOR = 0x1c,
+	MEMLAT_STOP_MONITOR = 0x1d,
+};
+
+struct node_msg {
+	u32 cpumask;
+	u32 mon_type;
+};
+
+struct scalar_param_msg {
+	u32 cpumask;
+	u32 mon_type;
+	u32 val;
+};
+
+struct map_table {
+	u32 v1;
+	u32 v2;
+};
+
+struct map_param_msg {
+	u32 cpumask;
+	u32 mon_type;
+	u32 nr_rows;
+	struct map_table tbl[MAX_MAP_ENTRIES];
+};
+
+struct pmu_map_msg {
+	u32 cpumask;
+	u32 mon_type;
+	u32 nr_entries;
+	u32 pmu[MAX_PMU_ENTRIES];
+};
+
+static int scmi_set_cpugrp_mon(const struct scmi_protocol_handle *ph,
+			       u32 cpus_mpidr, u32 mon_type, u32 msg_id)
+{
+	int ret = 0;
+	struct scmi_xfer *t;
+	struct node_msg *msg;
+
+	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->cpumask = cpu_to_le32(cpus_mpidr);
+	msg->mon_type = cpu_to_le32(mon_type);
+	ret = ph->xops->do_xfer(ph, t);
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_set_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
+{
+	return scmi_set_cpugrp_mon(ph, cpus_mpidr, mon_type, MEMLAT_SET_MONITOR);
+}
+
+static int scmi_set_cpu_grp(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
+{
+	return scmi_set_cpugrp_mon(ph, cpus_mpidr, mon_type, MEMLAT_SET_CPU_GROUP);
+}
+
+static int scmi_send_pmu_map_command(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+				     u32 mon_type, u32 nr_entries, void *buf, u32 msg_id)
+{
+	u32 *dst;
+	int ret, i = 0;
+	struct scmi_xfer *t;
+	struct pmu_map_msg *msg;
+	struct map_table *src = buf;
+
+	if (nr_entries > MAX_PMU_ENTRIES)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->cpumask = cpu_to_le32(cpus_mpidr);
+	msg->mon_type = cpu_to_le32(mon_type);
+	msg->nr_entries = cpu_to_le32(nr_entries);
+	dst = msg->pmu;
+
+	for (i = 0; i < nr_entries; i++)
+		dst[i] = cpu_to_le32(src[i].v2);
+
+	ret = ph->xops->do_xfer(ph, t);
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static int scmi_common_pmu_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			       u32 mon_type, u32 nr_entries, void *buf)
+{
+	return scmi_send_pmu_map_command(ph, cpus_mpidr, mon_type, nr_entries,
+					 buf, MEMLAT_COMMON_PMU_MAP);
+}
+
+static int scmi_mon_pmu_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			    u32 mon_type, u32 nr_entries, void *buf)
+{
+	return scmi_send_pmu_map_command(ph, cpus_mpidr, mon_type, nr_entries,
+					 buf, MEMLAT_MON_PMU_MAP);
+}
+
+static int scmi_freq_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			 u32 mon_type, u32 nr_rows, void *buf)
+{
+	int ret, i = 0;
+	struct scmi_xfer *t;
+	struct map_param_msg *msg;
+	struct map_table *tbl, *src = buf;
+
+	if (nr_rows > MAX_MAP_ENTRIES)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, MEMLAT_MON_FREQ_MAP, sizeof(*msg),
+				      sizeof(*msg), &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->cpumask = cpu_to_le32(cpus_mpidr);
+	msg->mon_type = cpu_to_le32(mon_type);
+	msg->nr_rows = cpu_to_le32(nr_rows);
+	tbl = msg->tbl;
+
+	for (i = 0; i < nr_rows; i++) {
+		tbl[i].v1 = cpu_to_le32(src[i].v1);
+		tbl[i].v2 = cpu_to_le32(src[i].v2);
+	}
+
+	ret = ph->xops->do_xfer(ph, t);
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static int scmi_set_tunable(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			    u32 msg_id, u32 mon_type, u32 val)
+{
+	int ret = 0;
+	struct scmi_xfer *t;
+	struct scalar_param_msg *msg;
+
+	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->cpumask = cpu_to_le32(cpus_mpidr);
+	msg->mon_type = cpu_to_le32(mon_type);
+	msg->val = cpu_to_le32(val);
+	ret = ph->xops->do_xfer(ph, t);
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_ipm_ratio(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			  u32 mon_type, u32 val)
+{
+	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_IPM_RATIO, mon_type, val);
+}
+
+static int scmi_stall_ratio(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			    u32 mon_type, u32 val)
+{
+	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_STALL_RATIO, mon_type, val);
+}
+
+static int scmi_sample_ms(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			  u32 mon_type, u32 val)
+{
+	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_SAMPLE_MS, mon_type, val);
+}
+
+static int scmi_send_start_stop(const struct scmi_protocol_handle *ph,
+				u32 cpus_mpidr, u32 mon_type, u32 msg_id)
+{
+	int ret = 0;
+	struct scmi_xfer *t;
+	struct scalar_param_msg *msg;
+
+	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->cpumask = cpu_to_le32(cpus_mpidr);
+	msg->mon_type = cpu_to_le32(mon_type);
+	ret = ph->xops->do_xfer(ph, t);
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_stop_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
+{
+	return scmi_send_start_stop(ph, cpus_mpidr, mon_type, MEMLAT_STOP_MONITOR);
+}
+
+static int scmi_start_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
+{
+	return scmi_send_start_stop(ph, cpus_mpidr, mon_type, MEMLAT_START_MONITOR);
+}
+
+static struct scmi_vendor_memlat_ops memlat_ops = {
+	.set_cpu_grp = scmi_set_cpu_grp,
+	.freq_map = scmi_freq_map,
+	.set_mon = scmi_set_mon,
+	.common_pmu_map = scmi_common_pmu_map,
+	.mon_pmu_map = scmi_mon_pmu_map,
+	.ipm_ratio = scmi_ipm_ratio,
+	.stall_ratio = scmi_stall_ratio,
+	.sample_ms = scmi_sample_ms,
+	.start_monitor = scmi_start_mon,
+	.stop_monitor = scmi_stop_mon,
+};
+
+static int scmi_vendor_memlat_protocol_init(const struct scmi_protocol_handle *ph)
+{
+	int ret;
+	u32 version;
+
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
+
+	dev_dbg(ph->dev, "Memlat Version %d.%d\n",
+		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+	return 0;
+}
+
+static const struct scmi_protocol scmi_vendor_memlat = {
+	.id = SCMI_VENDOR_PROTOCOL_MEMLAT,
+	.owner = THIS_MODULE,
+	.instance_init = &scmi_vendor_memlat_protocol_init,
+	.ops = &memlat_ops,
+};
+module_scmi_protocol(scmi_vendor_memlat);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. SCMI Memlat Protocol");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 4f765bc788ff..57abb5be45c9 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -677,6 +677,40 @@ struct scmi_powercap_proto_ops {
 };
 
 /**
+ * struct scmi_vendor_memlat_ops - represents the various operations provided
+ * by SCMI QTI HW Memlat Vendor Protocol
+ *
+ * @cpu_grp: set the cpugrp
+ * @set_mon: set the supported monitors
+ * @common_pmu_map: sets the common PMU map supported by governor
+ * @mon_pmu_map: sets the additional PMU map supported by governor
+ * @ipm_ratio: sets the ratio_ceil needed for hw memlat governor
+ * @stall_ratio: sets the stall_floor needed for hw memlat governor
+ * @sample_ms: sets the poll iterval of the governor
+ * @freq_map: sets the freq_map of the governor
+ * @start_mon: starts the monitor in firmware
+ * @stop_mon: stops the monitor in firmware
+ */
+struct scmi_vendor_memlat_ops {
+	int (*set_cpu_grp)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
+	int (*set_mon)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
+	int (*common_pmu_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
+			      u32 nr_rows, void *buf);
+	int (*mon_pmu_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
+			   u32 nr_rows, void *buf);
+	int (*ipm_ratio)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			 u32 mon_type, u32 val);
+	int (*stall_ratio)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			   u32 mon_type, u32 val);
+	int (*sample_ms)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
+			 u32 mon_type, u32 val);
+	int (*freq_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
+			u32 nr_rows, void *buf);
+	int (*start_monitor)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
+	int (*stop_monitor)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
+};
+
+/**
  * struct scmi_notify_ops  - represents notifications' operations provided by
  * SCMI core
  * @devm_event_notifier_register: Managed registration of a notifier_block for
@@ -785,6 +819,8 @@ enum scmi_std_protocol {
 	SCMI_PROTOCOL_POWERCAP = 0x18,
 };
 
+#define SCMI_VENDOR_PROTOCOL_MEMLAT    0x80
+
 enum scmi_system_events {
 	SCMI_SYSTEM_SHUTDOWN,
 	SCMI_SYSTEM_COLDRESET,
-- 
2.7.4

Re: [RFC 2/2] firmware: arm_scmi: Add SCMI QTI Memlat vendor protocol

[Sudeep Holla]

On Thu, Nov 03, 2022 at 10:28:32AM +0530, Sibi Sankar wrote:
> Add support for the SCMI QTI memlat (memory latency) vendor protocol.
> The QTI memlat vendor protocol takes in several tuneables including the
> IPM ratio (Instructions Per Miss), bus bandwidth requirements and PMU
> maps to enable frequency scaling of various buses (L3/LLCC/DDR) performed
> by the memory latency governor running on the CPUSS Control Processor.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  drivers/firmware/arm_scmi/Kconfig              |  10 +
>  drivers/firmware/arm_scmi/Makefile             |   1 +
>  drivers/firmware/arm_scmi/qcom_memlat_vendor.c | 269 +++++++++++++++++++++++++
>  include/linux/scmi_protocol.h                  |  36 ++++
>  4 files changed, 316 insertions(+)
>  create mode 100644 drivers/firmware/arm_scmi/qcom_memlat_vendor.c
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index a14f65444b35..814a3fc37dc1 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -136,6 +136,16 @@ config ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
>  
>  endif #ARM_SCMI_PROTOCOL
>  
> +config QTI_SCMI_MEMLAT_PROTOCOL
> +	tristate "Qualcomm Technologies, Inc. SCMI MEMLAT vendor Protocol"
> +	depends on ARM_SCMI_PROTOCOL && QCOM_CPUCP_MBOX

If you have set the transport correctly, there should be no need for any
such dependency.

> +	help
> +	  The SCMI QTI memlat vendor protocol adds support for the frequency
> +	  scaling of buses (L3/LLCC/DDR) by the QTI HW memlat governor running
> +	  on the CPUSS Control Processor (CPUCP).
> +
> +	  Say Y here if you want to build this driver.
> +

I don't think it is scalable to have a config option for each vendor+protocol
Kconfig. IMO just one config for all qcom vendor protocol please.

--
Regards,
Sudeep

Re: [RFC 2/2] firmware: arm_scmi: Add SCMI QTI Memlat vendor protocol

[Sudeep Holla]

On Thu, Nov 03, 2022 at 10:24:44AM +0000, Sudeep Holla wrote:
> On Thu, Nov 03, 2022 at 10:28:32AM +0530, Sibi Sankar wrote:
> > Add support for the SCMI QTI memlat (memory latency) vendor protocol.
> > The QTI memlat vendor protocol takes in several tuneables including the
> > IPM ratio (Instructions Per Miss), bus bandwidth requirements and PMU
> > maps to enable frequency scaling of various buses (L3/LLCC/DDR) performed
> > by the memory latency governor running on the CPUSS Control Processor.
> > 
> > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> > ---
> >  drivers/firmware/arm_scmi/Kconfig              |  10 +
> >  drivers/firmware/arm_scmi/Makefile             |   1 +
> >  drivers/firmware/arm_scmi/qcom_memlat_vendor.c | 269 +++++++++++++++++++++++++
> >  include/linux/scmi_protocol.h                  |  36 ++++
> >  4 files changed, 316 insertions(+)
> >  create mode 100644 drivers/firmware/arm_scmi/qcom_memlat_vendor.c
> > 
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > index a14f65444b35..814a3fc37dc1 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -136,6 +136,16 @@ config ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
> >  
> >  endif #ARM_SCMI_PROTOCOL
> >  
> > +config QTI_SCMI_MEMLAT_PROTOCOL
> > +	tristate "Qualcomm Technologies, Inc. SCMI MEMLAT vendor Protocol"
> > +	depends on ARM_SCMI_PROTOCOL && QCOM_CPUCP_MBOX
> 
> If you have set the transport correctly, there should be no need for any
> such dependency.
> 
> > +	help
> > +	  The SCMI QTI memlat vendor protocol adds support for the frequency
> > +	  scaling of buses (L3/LLCC/DDR) by the QTI HW memlat governor running
> > +	  on the CPUSS Control Processor (CPUCP).
> > +
> > +	  Say Y here if you want to build this driver.
> > +
> 
> I don't think it is scalable to have a config option for each vendor+protocol
> Kconfig. IMO just one config for all qcom vendor protocol please.
> 

Sorry pressed send too early before I could write the main part :(.
Can you please also add the driver using this protocol in the next revision.
What framework does that fit in ? Devfreq ? I am very much interested in
that as it helps in distributing the responsibility across these correctly.
I think that could be one of the reason I don't like all the information
dump you have in the DT binding proposed in the provider node. It needs to
move out but in order to understand where to, we need full picture here.
So please provide the same. 

Also it doesn't hurt to describe in detail: what theses "several tuneables"
are and where are they expected to arrive from or targeted to ?
Is CPUSS Control Processor responsible for CPU DVFS or not ?
Does it just control DVFS of L3/LLCC and DDR or is there a bigger list ?
All these information matters as your current DT proposal seem to be
tightly coupled with only few of these.

--
Regards,
Sudeep

Re: [RFC 2/2] firmware: arm_scmi: Add SCMI QTI Memlat vendor protocol

[Sibi Sankar]

Hey Sudeep,

Thanks for taking time to review the series.

On 11/3/22 16:07, Sudeep Holla wrote:
> On Thu, Nov 03, 2022 at 10:24:44AM +0000, Sudeep Holla wrote:
>> On Thu, Nov 03, 2022 at 10:28:32AM +0530, Sibi Sankar wrote:
>>> Add support for the SCMI QTI memlat (memory latency) vendor protocol.
>>> The QTI memlat vendor protocol takes in several tuneables including the
>>> IPM ratio (Instructions Per Miss), bus bandwidth requirements and PMU
>>> maps to enable frequency scaling of various buses (L3/LLCC/DDR) performed
>>> by the memory latency governor running on the CPUSS Control Processor.
>>>
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>> ---
>>>   drivers/firmware/arm_scmi/Kconfig              |  10 +
>>>   drivers/firmware/arm_scmi/Makefile             |   1 +
>>>   drivers/firmware/arm_scmi/qcom_memlat_vendor.c | 269 +++++++++++++++++++++++++
>>>   include/linux/scmi_protocol.h                  |  36 ++++
>>>   4 files changed, 316 insertions(+)
>>>   create mode 100644 drivers/firmware/arm_scmi/qcom_memlat_vendor.c
>>>
>>> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
>>> index a14f65444b35..814a3fc37dc1 100644
>>> --- a/drivers/firmware/arm_scmi/Kconfig
>>> +++ b/drivers/firmware/arm_scmi/Kconfig
>>> @@ -136,6 +136,16 @@ config ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
>>>   
>>>   endif #ARM_SCMI_PROTOCOL
>>>   
>>> +config QTI_SCMI_MEMLAT_PROTOCOL
>>> +	tristate "Qualcomm Technologies, Inc. SCMI MEMLAT vendor Protocol"
>>> +	depends on ARM_SCMI_PROTOCOL && QCOM_CPUCP_MBOX
>>
>> If you have set the transport correctly, there should be no need for any
>> such dependency.

ack

>>
>>> +	help
>>> +	  The SCMI QTI memlat vendor protocol adds support for the frequency
>>> +	  scaling of buses (L3/LLCC/DDR) by the QTI HW memlat governor running
>>> +	  on the CPUSS Control Processor (CPUCP).
>>> +
>>> +	  Say Y here if you want to build this driver.
>>> +
>>
>> I don't think it is scalable to have a config option for each vendor+protocol
>> Kconfig. IMO just one config for all qcom vendor protocol please.

ditto

>>
> 
> Sorry pressed send too early before I could write the main part :(.
> Can you please also add the driver using this protocol in the next revision.
> What framework does that fit in ? Devfreq ? I am very much interested in
> that as it helps in distributing the responsibility across these correctly.
> I think that could be one of the reason I don't like all the information
> dump you have in the DT binding proposed in the provider node. It needs to
> move out but in order to understand where to, we need full picture here.
> So please provide the same.
> 

As of now it would just be a simple SoC platform driver which passes on
the tuneables and starts the memlat governor on the SCP firmware. I'll
include it in the next re-spin.


> Also it doesn't hurt to describe in detail: what theses "several tuneables"
> are and where are they expected to arrive from or targeted to ?

ack

> Is CPUSS Control Processor responsible for CPU DVFS or not ?
> Does it just control DVFS of L3/LLCC and DDR or is there a bigger list ?

CPUSS CP is responsible for CPU DVFS but it's not done through
SCMI (it's done through the qcom-cpufreq-hw driver). Memory latency
protocol is meant to control the DVFS of memory buses. So it could
be one or all of them and the supported list for that particular
SoC was meant to be passed through the qcom,bus-type array as
described in the bindings.

- Sibi

> All these information matters as your current DT proposal seem to be
> tightly coupled with only few of these.
> 
> --
> Regards,
> Sudeep

Re: [RFC 2/2] firmware: arm_scmi: Add SCMI QTI Memlat vendor protocol

[Matthias Kaehlcke]

Hi Sibi,

On Thu, Nov 03, 2022 at 10:28:32AM +0530, Sibi Sankar wrote:
> Add support for the SCMI QTI memlat (memory latency) vendor protocol.
> The QTI memlat vendor protocol takes in several tuneables including the
> IPM ratio (Instructions Per Miss), bus bandwidth requirements and PMU
> maps to enable frequency scaling of various buses (L3/LLCC/DDR) performed
> by the memory latency governor running on the CPUSS Control Processor.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  drivers/firmware/arm_scmi/Kconfig              |  10 +
>  drivers/firmware/arm_scmi/Makefile             |   1 +
>  drivers/firmware/arm_scmi/qcom_memlat_vendor.c | 269 +++++++++++++++++++++++++
>  include/linux/scmi_protocol.h                  |  36 ++++
>  4 files changed, 316 insertions(+)
>  create mode 100644 drivers/firmware/arm_scmi/qcom_memlat_vendor.c
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index a14f65444b35..814a3fc37dc1 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -136,6 +136,16 @@ config ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
>  
>  endif #ARM_SCMI_PROTOCOL
>  
> +config QTI_SCMI_MEMLAT_PROTOCOL
> +	tristate "Qualcomm Technologies, Inc. SCMI MEMLAT vendor Protocol"
> +	depends on ARM_SCMI_PROTOCOL && QCOM_CPUCP_MBOX
> +	help
> +	  The SCMI QTI memlat vendor protocol adds support for the frequency
> +	  scaling of buses (L3/LLCC/DDR) by the QTI HW memlat governor running
> +	  on the CPUSS Control Processor (CPUCP).
> +
> +	  Say Y here if you want to build this driver.
> +
>  config ARM_SCMI_POWER_DOMAIN
>  	tristate "SCMI power domain driver"
>  	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index 9ea86f8cc8f7..78e6d72fb9bb 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -11,6 +11,7 @@ scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o volt
>  scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
>  		    $(scmi-transport-y)
>  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
> +obj-$(CONFIG_QTI_SCMI_MEMLAT_PROTOCOL) += qcom_memlat_vendor.o
>  obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
>  obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
>  
> diff --git a/drivers/firmware/arm_scmi/qcom_memlat_vendor.c b/drivers/firmware/arm_scmi/qcom_memlat_vendor.c
> new file mode 100644
> index 000000000000..4b7db309e633
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/qcom_memlat_vendor.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +
> +#include "protocols.h"
> +
> +#define MAX_MAP_ENTRIES 14
> +#define MAX_PMU_ENTRIES 24
> +
> +enum scmi_memlat_protocol_cmd {
> +	MEMLAT_SET_CPU_GROUP = 0x10,
> +	MEMLAT_SET_MONITOR = 0x11,
> +	MEMLAT_COMMON_PMU_MAP = 0x12,
> +	MEMLAT_MON_PMU_MAP = 0x13,
> +	MEMLAT_IPM_RATIO = 0x14,
> +	MEMLAT_STALL_RATIO = 0x15,
> +	MEMLAT_SAMPLE_MS = 0x18,
> +	MEMLAT_MON_FREQ_MAP = 0x19,
> +	MEMLAT_START_MONITOR = 0x1c,
> +	MEMLAT_STOP_MONITOR = 0x1d,
> +};
> +
> +struct node_msg {
> +	u32 cpumask;
> +	u32 mon_type;
> +};
> +
> +struct scalar_param_msg {
> +	u32 cpumask;
> +	u32 mon_type;
> +	u32 val;
> +};
> +
> +struct map_table {
> +	u32 v1;
> +	u32 v2;
> +};
> +
> +struct map_param_msg {
> +	u32 cpumask;
> +	u32 mon_type;
> +	u32 nr_rows;
> +	struct map_table tbl[MAX_MAP_ENTRIES];
> +};
> +
> +struct pmu_map_msg {
> +	u32 cpumask;
> +	u32 mon_type;
> +	u32 nr_entries;
> +	u32 pmu[MAX_PMU_ENTRIES];
> +};
> +
> +static int scmi_set_cpugrp_mon(const struct scmi_protocol_handle *ph,
> +			       u32 cpus_mpidr, u32 mon_type, u32 msg_id)
> +{
> +	int ret = 0;

no need to initialize

> +	struct scmi_xfer *t;
> +	struct node_msg *msg;
> +
> +	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
> +	if (ret)
> +		return ret;
> +
> +	msg = t->tx.buf;
> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
> +	msg->mon_type = cpu_to_le32(mon_type);
> +	ret = ph->xops->do_xfer(ph, t);
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_set_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
> +{
> +	return scmi_set_cpugrp_mon(ph, cpus_mpidr, mon_type, MEMLAT_SET_MONITOR);
> +}
> +
> +static int scmi_set_cpu_grp(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
> +{
> +	return scmi_set_cpugrp_mon(ph, cpus_mpidr, mon_type, MEMLAT_SET_CPU_GROUP);
> +}
> +
> +static int scmi_send_pmu_map_command(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +				     u32 mon_type, u32 nr_entries, void *buf, u32 msg_id)
> +{
> +	u32 *dst;
> +	int ret, i = 0;

initialization is not needed

> +	struct scmi_xfer *t;
> +	struct pmu_map_msg *msg;
> +	struct map_table *src = buf;
> +
> +	if (nr_entries > MAX_PMU_ENTRIES)
> +		return -EINVAL;
> +
> +	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
> +	if (ret)
> +		return ret;
> +
> +	msg = t->tx.buf;
> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
> +	msg->mon_type = cpu_to_le32(mon_type);

The above 7 lines are a recurring pattern. Might be worth to have a wrapper for
it. The datatype of 'msg' varies though, so it would have to be a macro :(

> +	msg->nr_entries = cpu_to_le32(nr_entries);
> +	dst = msg->pmu;
> +
> +	for (i = 0; i < nr_entries; i++)
> +		dst[i] = cpu_to_le32(src[i].v2);
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	ph->xops->xfer_put(ph, t);
> +	return ret;

This above 3 lines also recurring, consider a wrapper. With that the above
would become:

	return scmi_do_xfer(ph, t);
> +}
> +
> +static int scmi_common_pmu_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			       u32 mon_type, u32 nr_entries, void *buf)
> +{
> +	return scmi_send_pmu_map_command(ph, cpus_mpidr, mon_type, nr_entries,
> +					 buf, MEMLAT_COMMON_PMU_MAP);
> +}
> +
> +static int scmi_mon_pmu_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			    u32 mon_type, u32 nr_entries, void *buf)
> +{
> +	return scmi_send_pmu_map_command(ph, cpus_mpidr, mon_type, nr_entries,
> +					 buf, MEMLAT_MON_PMU_MAP);
> +}
> +
> +static int scmi_freq_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			 u32 mon_type, u32 nr_rows, void *buf)
> +{
> +	int ret, i = 0;

initialization is unnecessary

> +	struct scmi_xfer *t;
> +	struct map_param_msg *msg;
> +	struct map_table *tbl, *src = buf;
> +
> +	if (nr_rows > MAX_MAP_ENTRIES)
> +		return -EINVAL;
> +
> +	ret = ph->xops->xfer_get_init(ph, MEMLAT_MON_FREQ_MAP, sizeof(*msg),
> +				      sizeof(*msg), &t);
> +	if (ret)
> +		return ret;
> +
> +	msg = t->tx.buf;
> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
> +	msg->mon_type = cpu_to_le32(mon_type);
> +	msg->nr_rows = cpu_to_le32(nr_rows);
> +	tbl = msg->tbl;
> +
> +	for (i = 0; i < nr_rows; i++) {
> +		tbl[i].v1 = cpu_to_le32(src[i].v1);
> +		tbl[i].v2 = cpu_to_le32(src[i].v2);
> +	}
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	ph->xops->xfer_put(ph, t);
> +	return ret;
> +}
> +
> +static int scmi_set_tunable(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			    u32 msg_id, u32 mon_type, u32 val)
> +{
> +	int ret = 0;

drop initialization

> +	struct scmi_xfer *t;
> +	struct scalar_param_msg *msg;
> +
> +	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
> +	if (ret)
> +		return ret;
> +
> +	msg = t->tx.buf;
> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
> +	msg->mon_type = cpu_to_le32(mon_type);
> +	msg->val = cpu_to_le32(val);
> +	ret = ph->xops->do_xfer(ph, t);
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_ipm_ratio(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			  u32 mon_type, u32 val)
> +{
> +	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_IPM_RATIO, mon_type, val);
> +}
> +
> +static int scmi_stall_ratio(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			    u32 mon_type, u32 val)
> +{
> +	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_STALL_RATIO, mon_type, val);
> +}
> +
> +static int scmi_sample_ms(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			  u32 mon_type, u32 val)
> +{
> +	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_SAMPLE_MS, mon_type, val);
> +}
> +
> +static int scmi_send_start_stop(const struct scmi_protocol_handle *ph,
> +				u32 cpus_mpidr, u32 mon_type, u32 msg_id)
> +{
> +	int ret = 0;

drop init

> +	struct scmi_xfer *t;
> +	struct scalar_param_msg *msg;
> +
> +	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
> +	if (ret)
> +		return ret;
> +
> +	msg = t->tx.buf;
> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
> +	msg->mon_type = cpu_to_le32(mon_type);
> +	ret = ph->xops->do_xfer(ph, t);
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_stop_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
> +{
> +	return scmi_send_start_stop(ph, cpus_mpidr, mon_type, MEMLAT_STOP_MONITOR);
> +}
> +
> +static int scmi_start_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
> +{
> +	return scmi_send_start_stop(ph, cpus_mpidr, mon_type, MEMLAT_START_MONITOR);
> +}
> +
> +static struct scmi_vendor_memlat_ops memlat_ops = {
> +	.set_cpu_grp = scmi_set_cpu_grp,
> +	.freq_map = scmi_freq_map,
> +	.set_mon = scmi_set_mon,
> +	.common_pmu_map = scmi_common_pmu_map,
> +	.mon_pmu_map = scmi_mon_pmu_map,
> +	.ipm_ratio = scmi_ipm_ratio,
> +	.stall_ratio = scmi_stall_ratio,
> +	.sample_ms = scmi_sample_ms,
> +	.start_monitor = scmi_start_mon,
> +	.stop_monitor = scmi_stop_mon,
> +};
> +
> +static int scmi_vendor_memlat_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> +	int ret;
> +	u32 version;
> +
> +	ret = ph->xops->version_get(ph, &version);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(ph->dev, "Memlat Version %d.%d\n",
> +		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> +	return 0;
> +}
> +
> +static const struct scmi_protocol scmi_vendor_memlat = {
> +	.id = SCMI_VENDOR_PROTOCOL_MEMLAT,
> +	.owner = THIS_MODULE,
> +	.instance_init = &scmi_vendor_memlat_protocol_init,
> +	.ops = &memlat_ops,
> +};
> +module_scmi_protocol(scmi_vendor_memlat);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. SCMI Memlat Protocol");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 4f765bc788ff..57abb5be45c9 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -677,6 +677,40 @@ struct scmi_powercap_proto_ops {
>  };
>  
>  /**
> + * struct scmi_vendor_memlat_ops - represents the various operations provided
> + * by SCMI QTI HW Memlat Vendor Protocol
> + *
> + * @cpu_grp: set the cpugrp
> + * @set_mon: set the supported monitors
> + * @common_pmu_map: sets the common PMU map supported by governor
> + * @mon_pmu_map: sets the additional PMU map supported by governor
> + * @ipm_ratio: sets the ratio_ceil needed for hw memlat governor
> + * @stall_ratio: sets the stall_floor needed for hw memlat governor
> + * @sample_ms: sets the poll iterval of the governor
> + * @freq_map: sets the freq_map of the governor
> + * @start_mon: starts the monitor in firmware
> + * @stop_mon: stops the monitor in firmware
> + */
> +struct scmi_vendor_memlat_ops {
> +	int (*set_cpu_grp)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
> +	int (*set_mon)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
> +	int (*common_pmu_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
> +			      u32 nr_rows, void *buf);
> +	int (*mon_pmu_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
> +			   u32 nr_rows, void *buf);
> +	int (*ipm_ratio)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			 u32 mon_type, u32 val);
> +	int (*stall_ratio)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			   u32 mon_type, u32 val);
> +	int (*sample_ms)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
> +			 u32 mon_type, u32 val);
> +	int (*freq_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
> +			u32 nr_rows, void *buf);
> +	int (*start_monitor)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
> +	int (*stop_monitor)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
> +};
> +
> +/**
>   * struct scmi_notify_ops  - represents notifications' operations provided by
>   * SCMI core
>   * @devm_event_notifier_register: Managed registration of a notifier_block for
> @@ -785,6 +819,8 @@ enum scmi_std_protocol {
>  	SCMI_PROTOCOL_POWERCAP = 0x18,
>  };
>  
> +#define SCMI_VENDOR_PROTOCOL_MEMLAT    0x80
> +
>  enum scmi_system_events {
>  	SCMI_SYSTEM_SHUTDOWN,
>  	SCMI_SYSTEM_COLDRESET,
> -- 
> 2.7.4
> 

Re: [RFC 2/2] firmware: arm_scmi: Add SCMI QTI Memlat vendor protocol

[Sibi Sankar]

Hey Matthias,

Thanks for taking time to review the series.

On 11/4/22 01:32, Matthias Kaehlcke wrote:
> Hi Sibi,
> 
> On Thu, Nov 03, 2022 at 10:28:32AM +0530, Sibi Sankar wrote:
>> Add support for the SCMI QTI memlat (memory latency) vendor protocol.
>> The QTI memlat vendor protocol takes in several tuneables including the
>> IPM ratio (Instructions Per Miss), bus bandwidth requirements and PMU
>> maps to enable frequency scaling of various buses (L3/LLCC/DDR) performed
>> by the memory latency governor running on the CPUSS Control Processor.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   drivers/firmware/arm_scmi/Kconfig              |  10 +
>>   drivers/firmware/arm_scmi/Makefile             |   1 +
>>   drivers/firmware/arm_scmi/qcom_memlat_vendor.c | 269 +++++++++++++++++++++++++
>>   include/linux/scmi_protocol.h                  |  36 ++++
>>   4 files changed, 316 insertions(+)
>>   create mode 100644 drivers/firmware/arm_scmi/qcom_memlat_vendor.c
>>
>> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
>> index a14f65444b35..814a3fc37dc1 100644
>> --- a/drivers/firmware/arm_scmi/Kconfig
>> +++ b/drivers/firmware/arm_scmi/Kconfig
>> @@ -136,6 +136,16 @@ config ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
>>   
>>   endif #ARM_SCMI_PROTOCOL
>>   
>> +config QTI_SCMI_MEMLAT_PROTOCOL
>> +	tristate "Qualcomm Technologies, Inc. SCMI MEMLAT vendor Protocol"
>> +	depends on ARM_SCMI_PROTOCOL && QCOM_CPUCP_MBOX
>> +	help
>> +	  The SCMI QTI memlat vendor protocol adds support for the frequency
>> +	  scaling of buses (L3/LLCC/DDR) by the QTI HW memlat governor running
>> +	  on the CPUSS Control Processor (CPUCP).
>> +
>> +	  Say Y here if you want to build this driver.
>> +
>>   config ARM_SCMI_POWER_DOMAIN
>>   	tristate "SCMI power domain driver"
>>   	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
>> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
>> index 9ea86f8cc8f7..78e6d72fb9bb 100644
>> --- a/drivers/firmware/arm_scmi/Makefile
>> +++ b/drivers/firmware/arm_scmi/Makefile
>> @@ -11,6 +11,7 @@ scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o volt
>>   scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
>>   		    $(scmi-transport-y)
>>   obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
>> +obj-$(CONFIG_QTI_SCMI_MEMLAT_PROTOCOL) += qcom_memlat_vendor.o
>>   obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
>>   obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
>>   
>> diff --git a/drivers/firmware/arm_scmi/qcom_memlat_vendor.c b/drivers/firmware/arm_scmi/qcom_memlat_vendor.c
>> new file mode 100644
>> index 000000000000..4b7db309e633
>> --- /dev/null
>> +++ b/drivers/firmware/arm_scmi/qcom_memlat_vendor.c
>> @@ -0,0 +1,269 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/scmi_protocol.h>
>> +
>> +#include "protocols.h"
>> +
>> +#define MAX_MAP_ENTRIES 14
>> +#define MAX_PMU_ENTRIES 24
>> +
>> +enum scmi_memlat_protocol_cmd {
>> +	MEMLAT_SET_CPU_GROUP = 0x10,
>> +	MEMLAT_SET_MONITOR = 0x11,
>> +	MEMLAT_COMMON_PMU_MAP = 0x12,
>> +	MEMLAT_MON_PMU_MAP = 0x13,
>> +	MEMLAT_IPM_RATIO = 0x14,
>> +	MEMLAT_STALL_RATIO = 0x15,
>> +	MEMLAT_SAMPLE_MS = 0x18,
>> +	MEMLAT_MON_FREQ_MAP = 0x19,
>> +	MEMLAT_START_MONITOR = 0x1c,
>> +	MEMLAT_STOP_MONITOR = 0x1d,
>> +};
>> +
>> +struct node_msg {
>> +	u32 cpumask;
>> +	u32 mon_type;
>> +};
>> +
>> +struct scalar_param_msg {
>> +	u32 cpumask;
>> +	u32 mon_type;
>> +	u32 val;
>> +};
>> +
>> +struct map_table {
>> +	u32 v1;
>> +	u32 v2;
>> +};
>> +
>> +struct map_param_msg {
>> +	u32 cpumask;
>> +	u32 mon_type;
>> +	u32 nr_rows;
>> +	struct map_table tbl[MAX_MAP_ENTRIES];
>> +};
>> +
>> +struct pmu_map_msg {
>> +	u32 cpumask;
>> +	u32 mon_type;
>> +	u32 nr_entries;
>> +	u32 pmu[MAX_PMU_ENTRIES];
>> +};
>> +
>> +static int scmi_set_cpugrp_mon(const struct scmi_protocol_handle *ph,
>> +			       u32 cpus_mpidr, u32 mon_type, u32 msg_id)
>> +{
>> +	int ret = 0;
> 
> no need to initialize
> 
>> +	struct scmi_xfer *t;
>> +	struct node_msg *msg;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msg = t->tx.buf;
>> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
>> +	msg->mon_type = cpu_to_le32(mon_type);
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static int scmi_set_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
>> +{
>> +	return scmi_set_cpugrp_mon(ph, cpus_mpidr, mon_type, MEMLAT_SET_MONITOR);
>> +}
>> +
>> +static int scmi_set_cpu_grp(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
>> +{
>> +	return scmi_set_cpugrp_mon(ph, cpus_mpidr, mon_type, MEMLAT_SET_CPU_GROUP);
>> +}
>> +
>> +static int scmi_send_pmu_map_command(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +				     u32 mon_type, u32 nr_entries, void *buf, u32 msg_id)
>> +{
>> +	u32 *dst;
>> +	int ret, i = 0;
> 
> initialization is not needed
> 
>> +	struct scmi_xfer *t;
>> +	struct pmu_map_msg *msg;
>> +	struct map_table *src = buf;
>> +
>> +	if (nr_entries > MAX_PMU_ENTRIES)
>> +		return -EINVAL;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msg = t->tx.buf;
>> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
>> +	msg->mon_type = cpu_to_le32(mon_type);
> 
> The above 7 lines are a recurring pattern. Might be worth to have a wrapper for
> it. The datatype of 'msg' varies though, so it would have to be a macro :(
> 
>> +	msg->nr_entries = cpu_to_le32(nr_entries);
>> +	dst = msg->pmu;
>> +
>> +	for (i = 0; i < nr_entries; i++)
>> +		dst[i] = cpu_to_le32(src[i].v2);
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	ph->xops->xfer_put(ph, t);
>> +	return ret;
> 
> This above 3 lines also recurring, consider a wrapper. With that the above
> would become:
> 
> 	return scmi_do_xfer(ph, t);

Ack. Will drop the unnecessary initialisations during the next re-spin
as well.

>> +}
>> +
>> +static int scmi_common_pmu_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			       u32 mon_type, u32 nr_entries, void *buf)
>> +{
>> +	return scmi_send_pmu_map_command(ph, cpus_mpidr, mon_type, nr_entries,
>> +					 buf, MEMLAT_COMMON_PMU_MAP);
>> +}
>> +
>> +static int scmi_mon_pmu_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			    u32 mon_type, u32 nr_entries, void *buf)
>> +{
>> +	return scmi_send_pmu_map_command(ph, cpus_mpidr, mon_type, nr_entries,
>> +					 buf, MEMLAT_MON_PMU_MAP);
>> +}
>> +
>> +static int scmi_freq_map(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			 u32 mon_type, u32 nr_rows, void *buf)
>> +{
>> +	int ret, i = 0;
> 
> initialization is unnecessary
> 
>> +	struct scmi_xfer *t;
>> +	struct map_param_msg *msg;
>> +	struct map_table *tbl, *src = buf;
>> +
>> +	if (nr_rows > MAX_MAP_ENTRIES)
>> +		return -EINVAL;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, MEMLAT_MON_FREQ_MAP, sizeof(*msg),
>> +				      sizeof(*msg), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msg = t->tx.buf;
>> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
>> +	msg->mon_type = cpu_to_le32(mon_type);
>> +	msg->nr_rows = cpu_to_le32(nr_rows);
>> +	tbl = msg->tbl;
>> +
>> +	for (i = 0; i < nr_rows; i++) {
>> +		tbl[i].v1 = cpu_to_le32(src[i].v1);
>> +		tbl[i].v2 = cpu_to_le32(src[i].v2);
>> +	}
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	ph->xops->xfer_put(ph, t);
>> +	return ret;
>> +}
>> +
>> +static int scmi_set_tunable(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			    u32 msg_id, u32 mon_type, u32 val)
>> +{
>> +	int ret = 0;
> 
> drop initialization
> 
>> +	struct scmi_xfer *t;
>> +	struct scalar_param_msg *msg;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msg = t->tx.buf;
>> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
>> +	msg->mon_type = cpu_to_le32(mon_type);
>> +	msg->val = cpu_to_le32(val);
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static int scmi_ipm_ratio(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			  u32 mon_type, u32 val)
>> +{
>> +	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_IPM_RATIO, mon_type, val);
>> +}
>> +
>> +static int scmi_stall_ratio(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			    u32 mon_type, u32 val)
>> +{
>> +	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_STALL_RATIO, mon_type, val);
>> +}
>> +
>> +static int scmi_sample_ms(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			  u32 mon_type, u32 val)
>> +{
>> +	return scmi_set_tunable(ph, cpus_mpidr, MEMLAT_SAMPLE_MS, mon_type, val);
>> +}
>> +
>> +static int scmi_send_start_stop(const struct scmi_protocol_handle *ph,
>> +				u32 cpus_mpidr, u32 mon_type, u32 msg_id)
>> +{
>> +	int ret = 0;
> 
> drop init
> 
>> +	struct scmi_xfer *t;
>> +	struct scalar_param_msg *msg;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, msg_id, sizeof(*msg), sizeof(*msg), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msg = t->tx.buf;
>> +	msg->cpumask = cpu_to_le32(cpus_mpidr);
>> +	msg->mon_type = cpu_to_le32(mon_type);
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static int scmi_stop_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
>> +{
>> +	return scmi_send_start_stop(ph, cpus_mpidr, mon_type, MEMLAT_STOP_MONITOR);
>> +}
>> +
>> +static int scmi_start_mon(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type)
>> +{
>> +	return scmi_send_start_stop(ph, cpus_mpidr, mon_type, MEMLAT_START_MONITOR);
>> +}
>> +
>> +static struct scmi_vendor_memlat_ops memlat_ops = {
>> +	.set_cpu_grp = scmi_set_cpu_grp,
>> +	.freq_map = scmi_freq_map,
>> +	.set_mon = scmi_set_mon,
>> +	.common_pmu_map = scmi_common_pmu_map,
>> +	.mon_pmu_map = scmi_mon_pmu_map,
>> +	.ipm_ratio = scmi_ipm_ratio,
>> +	.stall_ratio = scmi_stall_ratio,
>> +	.sample_ms = scmi_sample_ms,
>> +	.start_monitor = scmi_start_mon,
>> +	.stop_monitor = scmi_stop_mon,
>> +};
>> +
>> +static int scmi_vendor_memlat_protocol_init(const struct scmi_protocol_handle *ph)
>> +{
>> +	int ret;
>> +	u32 version;
>> +
>> +	ret = ph->xops->version_get(ph, &version);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_dbg(ph->dev, "Memlat Version %d.%d\n",
>> +		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct scmi_protocol scmi_vendor_memlat = {
>> +	.id = SCMI_VENDOR_PROTOCOL_MEMLAT,
>> +	.owner = THIS_MODULE,
>> +	.instance_init = &scmi_vendor_memlat_protocol_init,
>> +	.ops = &memlat_ops,
>> +};
>> +module_scmi_protocol(scmi_vendor_memlat);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. SCMI Memlat Protocol");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
>> index 4f765bc788ff..57abb5be45c9 100644
>> --- a/include/linux/scmi_protocol.h
>> +++ b/include/linux/scmi_protocol.h
>> @@ -677,6 +677,40 @@ struct scmi_powercap_proto_ops {
>>   };
>>   
>>   /**
>> + * struct scmi_vendor_memlat_ops - represents the various operations provided
>> + * by SCMI QTI HW Memlat Vendor Protocol
>> + *
>> + * @cpu_grp: set the cpugrp
>> + * @set_mon: set the supported monitors
>> + * @common_pmu_map: sets the common PMU map supported by governor
>> + * @mon_pmu_map: sets the additional PMU map supported by governor
>> + * @ipm_ratio: sets the ratio_ceil needed for hw memlat governor
>> + * @stall_ratio: sets the stall_floor needed for hw memlat governor
>> + * @sample_ms: sets the poll iterval of the governor
>> + * @freq_map: sets the freq_map of the governor
>> + * @start_mon: starts the monitor in firmware
>> + * @stop_mon: stops the monitor in firmware
>> + */
>> +struct scmi_vendor_memlat_ops {
>> +	int (*set_cpu_grp)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
>> +	int (*set_mon)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
>> +	int (*common_pmu_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
>> +			      u32 nr_rows, void *buf);
>> +	int (*mon_pmu_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
>> +			   u32 nr_rows, void *buf);
>> +	int (*ipm_ratio)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			 u32 mon_type, u32 val);
>> +	int (*stall_ratio)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			   u32 mon_type, u32 val);
>> +	int (*sample_ms)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr,
>> +			 u32 mon_type, u32 val);
>> +	int (*freq_map)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type,
>> +			u32 nr_rows, void *buf);
>> +	int (*start_monitor)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
>> +	int (*stop_monitor)(const struct scmi_protocol_handle *ph, u32 cpus_mpidr, u32 mon_type);
>> +};
>> +
>> +/**
>>    * struct scmi_notify_ops  - represents notifications' operations provided by
>>    * SCMI core
>>    * @devm_event_notifier_register: Managed registration of a notifier_block for
>> @@ -785,6 +819,8 @@ enum scmi_std_protocol {
>>   	SCMI_PROTOCOL_POWERCAP = 0x18,
>>   };
>>   
>> +#define SCMI_VENDOR_PROTOCOL_MEMLAT    0x80
>> +
>>   enum scmi_system_events {
>>   	SCMI_SYSTEM_SHUTDOWN,
>>   	SCMI_SYSTEM_COLDRESET,
>> -- 
>> 2.7.4
>>
> 
> 

Re: [RFC 0/2] Add support for SCMI QTI Memlat Vendor Protocol

[Cristian Marussi]

On Thu, Nov 03, 2022 at 10:28:30AM +0530, Sibi Sankar wrote:
> The patch series documents the bindings and adds support for the
> SCMI QTI memlat (memory latency) vendor protocol. The protocol takes
> in several tuneables including the IPM ratio (Instructions Per Miss),
> bus bandwidth requirements and PMU maps to enable frequency scaling
> of various buses (L3/LLCC/DDR). The scaling is performed by the HW
> memory latency governor running on the CPUSS Control Processor.
> 
> Depends on CPUCP mailbox driver:
> https://patchwork.kernel.org/project/linux-arm-msm/cover/1663135386-26270-1-git-send-email-quic_sibis@quicinc.com/
> 

[+ CC: souvik.chakravarty@arm.com ]

Hi Sibi,

Nice to see vendor protocols starting to make their way into upstream !

I only glanced through the series as of now, and I'd have a few
questions before going on with the review:

 - why this protocol is dependent on a specific transport ?
   Is it to compile it only on platform supoprting it without adding
   a per-protocol Kconfig ?

Protocols are anyway enumerated at SCMI stack probe time so even if it
is not there it just won't be activated...I maybe missing something.

Thanks,
Cristian

Re: [RFC 0/2] Add support for SCMI QTI Memlat Vendor Protocol

[Sibi Sankar]

Hey Cristian,
Thanks for taking time to review the series.

On 11/3/22 15:11, Cristian Marussi wrote:
> On Thu, Nov 03, 2022 at 10:28:30AM +0530, Sibi Sankar wrote:
>> The patch series documents the bindings and adds support for the
>> SCMI QTI memlat (memory latency) vendor protocol. The protocol takes
>> in several tuneables including the IPM ratio (Instructions Per Miss),
>> bus bandwidth requirements and PMU maps to enable frequency scaling
>> of various buses (L3/LLCC/DDR). The scaling is performed by the HW
>> memory latency governor running on the CPUSS Control Processor.
>>
>> Depends on CPUCP mailbox driver:
>> https://patchwork.kernel.org/project/linux-arm-msm/cover/1663135386-26270-1-git-send-email-quic_sibis@quicinc.com/
>>
> 
> [+ CC: souvik.chakravarty@arm.com ]
> 
> Hi Sibi,
> 
> Nice to see vendor protocols starting to make their way into upstream !
> 
> I only glanced through the series as of now, and I'd have a few
> questions before going on with the review:
> 
>   - why this protocol is dependent on a specific transport ?
>     Is it to compile it only on platform supoprting it without adding
>     a per-protocol Kconfig ?

It was done just to compile it on platforms supporting it but it doesn't
have to done that way. I remove the dependency during the next re-spin.

- Sibi

> 
> Protocols are anyway enumerated at SCMI stack probe time so even if it
> is not there it just won't be activated...I maybe missing something.
> 
> Thanks,
> Cristian
> 
> 
>