Re: [PATCH V4 1/5] dt-bindings: firmware: Document bindings for QCOM SCMI Generic Extension

[Krzysztof Kozlowski]

On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote:
> Document the various memory buses that can be monitored and scaled by
> the memory latency governor hosted by the QCOM SCMI Generic Extension
> Protocol v1.0.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> ---
> 
> v3:
> * Restructure the bindings to mimic IMX [Christian]
> 
>  .../bindings/firmware/arm,scmi.yaml           |   1 +
>  .../bindings/firmware/qcom,scmi-memlat.yaml   | 246 ++++++++++++++++++
>  .../dt-bindings/firmware/qcom,scmi-memlat.h   |  22 ++
>  3 files changed, 269 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml
>  create mode 100644 include/dt-bindings/firmware/qcom,scmi-memlat.h
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 54d7d11bfed4..1d405f429168 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -24,6 +24,7 @@ description: |
>  
>  anyOf:
>    - $ref: /schemas/firmware/nxp,imx95-scmi.yaml
> +  - $ref: /schemas/firmware/qcom,scmi-memlat.yaml
>  
>  properties:
>    $nodename:
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml
> new file mode 100644
> index 000000000000..0e8ea6dacd6a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml
> @@ -0,0 +1,246 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/qcom,scmi-memlat.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SCMI Memory Bus nodes
> +
> +maintainers:
> +  - Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> +
> +description:
> +  This binding describes the various memory buses that can be monitored and scaled

Drop "This binding" and describe the hardware/firmware, not binding.

Also, not wrapped according to Linux coding style.


> +  by memory latency governor running on the CPU Control Processor (SCMI controller).
> +
> +properties:
> +  protocol@80:
> +    $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        const: 0x80
> +
> +    patternProperties:
> +      '^memory-[0-9]$':
> +        type: object
> +        unevaluatedProperties: false
> +        description:
> +          The list of all memory buses that can be monitored and scaled by the
> +          memory latency governor running on the SCMI controller.
> +
> +        properties:
> +          qcom,memory-type:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2]
> +            description: |
> +              Memory Bus Identifier
> +              0 = QCOM_MEM_TYPE_DDR
> +              1 = QCOM_MEM_TYPE_LLCC
> +              2 = QCOM_MEM_TYPE_DDR_QOS

You need to describe these. Why "Quality of service" should be a separate
bus?

To me it looks like you are re-defining interconnects.

> +
> +          freq-table-hz:
> +            items:
> +              items:

" - items:"
no?

> +                - description: Minimum frequency of the memory bus in Hz
> +                - description: Maximum frequency of the memory bus in Hz
> +
> +        patternProperties:
> +          '^monitor-[0-9]$':
> +            type: object
> +            unevaluatedProperties: false
> +            description:
> +              The list of all monitors detecting the memory latency bound workloads using
> +              various counters.
> +
> +            properties:
> +              qcom,compute-type:
> +                description:
> +                  Monitors of type compute perform bus dvfs based on a rudimentary CPU
> +                  frequency to memory frequency map.

I don't understand why you need to describe what type of monitor the
SCMI has. Commit msg is pretty vague, so does not help me, either.

> +                type: boolean
> +
> +              qcom,ipm-ceil:
> +                $ref: /schemas/types.yaml#/definitions/uint32
> +                description:
> +                  Monitors having this property perform bus dvfs based on the same
> +                  rudimentary table but the scaling is performed only if the calculated
> +                  IPM (Instruction Per Misses) exceeds the given ceiling.
> +
> +              cpus:
> +                $ref: /schemas/types.yaml#/definitions/phandle-array
> +                description:
> +                  Should be a list of phandles to CPU nodes (as described in
> +                  Documentation/devicetree/bindings/arm/cpus.yaml).
> +
> +              operating-points-v2: true
> +              opp-table:
> +                type: object
> +
> +            required:
> +              - cpus
> +              - operating-points-v2
> +
> +            oneOf:
> +              - required: [ 'qcom,compute-type' ]
> +              - required: [ 'qcom,ipm-ceil' ]
> +
> +        required:
> +          - qcom,memory-type
> +          - freq-table-hz
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    #include <dt-bindings/firmware/qcom,scmi-memlat.h>
> +
> +    firmware {
> +        scmi {
> +            compatible = "arm,scmi";
> +            mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>;
> +            mbox-names = "tx", "rx";
> +            shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            protocol@80 {
> +                reg = <0x80>;
> +
> +                memory-0 {
> +                    qcom,memory-type = <QCOM_MEM_TYPE_DDR>;
> +                    freq-table-hz = /bits/ 64 <200000000 4224000000>;
> +
> +                    monitor-0 {
> +                        qcom,ipm-ceil = <20000000>;
> +                        cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7
> +                                &CPU8 &CPU9 &CPU10 &CPU11>;

Labels are lowercase.

> +                        operating-points-v2 = <&memory0_monitor0_opp_table>;
> +
> +                        memory0_monitor0_opp_table: opp-table {
> +                            compatible = "operating-points-v2";
> +
> +                            opp-999000000 {
> +                                opp-hz = /bits/ 64 <999000000 547000000>;
> +                            };
> +
> +                            opp-1440000000 {
> +                                opp-hz = /bits/ 64 <1440000000 768000000>;
> +                            };
> +
> +                            opp-1671000000 {
> +                                opp-hz = /bits/ 64 <1671000000 1555000000>;
> +                            };
> +
> +                            opp-2189000000 {
> +                                opp-hz = /bits/ 64 <2189000000 2092000000>;
> +                            };
> +
> +                            opp-2516000000 {
> +                                opp-hz = /bits/ 64 <2516000000 3187000000>;
> +                            };
> +
> +                            opp-3860000000 {
> +                                opp-hz = /bits/ 64 <3860000000 4224000000>;
> +                            };
> +                        };
> +                    };
> +
> +                    monitor-1 {
> +                        qcom,compute-type;
> +                        cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7
> +                                &CPU8 &CPU9 &CPU10 &CPU11>;
> +                        operating-points-v2 = <&memory0_monitor1_opp_table>;
> +
> +                        memory0_monitor1_opp_table: opp-table {
> +                            compatible = "operating-points-v2";
> +
> +                            opp-1440000000 {
> +                                    opp-hz = /bits/ 64 <1440000000 200000000>;
> +                            };
> +
> +                            opp-2189000000 {
> +                                    opp-hz = /bits/ 64 <2189000000 768000000>;
> +                            };
> +
> +                            opp-2516000000 {
> +                                    opp-hz = /bits/ 64 <2516000000 1555000000>;
> +                            };
> +
> +                            opp-3860000000 {
> +                                    opp-hz = /bits/ 64 <3860000000 4224000000>;
> +                            };
> +                        };
> +                    };
> +                };
> +
> +                memory-1 {
> +                    qcom,memory-type = <QCOM_MEM_TYPE_LLCC>;
> +                    freq-table-hz = /bits/ 64 <300000000 1067000000>;
> +
> +                    monitor-0 {
> +                        qcom,ipm-ceil = <20000000>;
> +                        cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7
> +                                &CPU8 &CPU9 &CPU10 &CPU11>;
> +                        operating-points-v2 = <&memory1_monitor0_opp_table>;
> +
> +                        memory1_monitor0_opp_table: opp-table {
> +                            compatible = "operating-points-v2";
> +
> +                            opp-999000000 {
> +                                opp-hz = /bits/ 64 <999000000 300000000>;
> +                            };
> +
> +                            opp-1440000000 {
> +                                opp-hz = /bits/ 64 <1440000000 466000000>;
> +                            };
> +
> +                            opp-1671000000 {
> +                                opp-hz = /bits/ 64 <1671000000 600000000>;
> +                            };
> +
> +                            opp-2189000000 {
> +                                opp-hz = /bits/ 64 <2189000000 806000000>;
> +                            };
> +
> +                            opp-2516000000 {
> +                                opp-hz = /bits/ 64 <2516000000 933000000>;
> +                            };
> +
> +                            opp-3860000000 {
> +                                opp-hz = /bits/ 64 <3860000000 1066000000>;
> +                            };
> +                        };
> +                    };
> +                };
> +
> +                memory-2 {
> +                    qcom,memory-type = <QCOM_MEM_TYPE_DDR_QOS>;
> +                    freq-table-hz = /bits/ 64 <QCOM_DDR_LEVEL_AUTO QCOM_DDR_LEVEL_PERF>;
> +
> +                    monitor-0 {
> +                        qcom,ipm-ceil = <20000000>;
> +                        cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7
> +                                &CPU8 &CPU9 &CPU10 &CPU11>;
> +                        operating-points-v2 = <&memory2_monitor0_opp_table>;
> +
> +                        memory2_monitor0_opp_table: opp-table {
> +                            compatible = "operating-points-v2";
> +
> +                            opp-2189000000 {
> +                                opp-hz = /bits/ 64 <2189000000>;
> +                                opp-level = <QCOM_DDR_LEVEL_AUTO>;
> +                            };
> +
> +                            opp-3860000000 {
> +                                opp-hz = /bits/ 64 <3860000000>;
> +                                opp-level = <QCOM_DDR_LEVEL_PERF>;
> +                            };
> +                        };
> +                    };
> +                };
> +            };
> +        };
> +    };
> diff --git a/include/dt-bindings/firmware/qcom,scmi-memlat.h b/include/dt-bindings/firmware/qcom,scmi-memlat.h
> new file mode 100644
> index 000000000000..7ae8d8d5623b
> --- /dev/null
> +++ b/include/dt-bindings/firmware/qcom,scmi-memlat.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#ifndef __DT_BINDINGS_QCOM_SCMI_VENDOR_H
> +#define __DT_BINDINGS_QCOM_SCMI_VENDOR
> +
> +/* Memory IDs */
> +#define QCOM_MEM_TYPE_DDR	0x0
> +#define QCOM_MEM_TYPE_LLCC	0x1
> +#define QCOM_MEM_TYPE_DDR_QOS	0x2

Use decimal.

> +
> +/*
> + * QCOM_MEM_TYPE_DDR_QOS supports the following states.
> + *
> + * %QCOM_DDR_LEVEL_AUTO:	DDR operates with LPM enabled
> + * %QCOM_DDR_LEVEL_PERF:	DDR operates with LPM disabled

What is "%"?

> + */
> +#define QCOM_DDR_LEVEL_AUTO	0x0
> +#define QCOM_DDR_LEVEL_PERF	0x1

Decimal.

> +
> +#endif /* __DT_BINDINGS_QCOM_SCMI_VENDOR_H */
> -- 
> 2.34.1
>