diff mbox series

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

Message ID 20241007061023.1978380-2-quic_sibis@quicinc.com
State New
Headers show
Series arm_scmi: vendors: Qualcomm Generic Vendor Extensions | expand

Commit Message

Sibi Sankar Oct. 7, 2024, 6:10 a.m. UTC
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@quicinc.com>
---

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

Comments

Krzysztof Kozlowski Oct. 8, 2024, 6:47 a.m. UTC | #1
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@quicinc.com>
> ---
> 
> 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@quicinc.com>
> +
> +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
>
Krzysztof Kozlowski Oct. 8, 2024, 6:49 a.m. UTC | #2
On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote:
> +/*
> + * 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

I could not find any driver using these. Can you point me to usage in
the drivers?

Best regards,
Krzysztof
Dmitry Baryshkov Oct. 8, 2024, 12:10 p.m. UTC | #3
On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote:
> On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote:
> > +/*
> > + * 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
> 
> I could not find any driver using these. Can you point me to usage in
> the drivers?

It's well hidden. These are the raw values used for DDR_QOS memory.

> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Oct. 8, 2024, 12:11 p.m. UTC | #4
On 08/10/2024 14:10, Dmitry Baryshkov wrote:
> On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote:
>> On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote:
>>> +/*
>>> + * 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
>>
>> I could not find any driver using these. Can you point me to usage in
>> the drivers?
> 
> It's well hidden. These are the raw values used for DDR_QOS memory.

So not a binding? Then these should be dropped.

Best regards,
Krzysztof
Dmitry Baryshkov Oct. 24, 2024, 7:46 p.m. UTC | #5
On Thu, Oct 24, 2024 at 03:29:05PM +0200, Krzysztof Kozlowski wrote:
> On 22/10/2024 09:25, Sibi Sankar wrote:
> > 
> > 
> > On 10/8/24 17:41, Krzysztof Kozlowski wrote:
> >> On 08/10/2024 14:10, Dmitry Baryshkov wrote:
> >>> On Tue, Oct 08, 2024 at 08:49:27AM GMT, Krzysztof Kozlowski wrote:
> >>>> On Mon, Oct 07, 2024 at 11:40:19AM +0530, Sibi Sankar wrote:
> >>>>> +/*
> >>>>> + * 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
> >>>>
> >>>> I could not find any driver using these. Can you point me to usage in
> >>>> the drivers?
> >>>
> >>> It's well hidden. These are the raw values used for DDR_QOS memory.
> >>
> >> So not a binding? Then these should be dropped.
> > 
> > I am not sure why the term "well hidden" was even considered :(
> > The driver just reads them from dt and passes them along. If you
> > want the dt to list magic numbers 0/1 instead I can do that as well.
> > 
> 
> If these are used by FW, then it's fine, although please document it
> clearly in comment.

If they are used by FW but they are common between all platforms, it
should go to the driver instead of being stuffed into DT.
Dmitry Baryshkov Oct. 24, 2024, 7:54 p.m. UTC | #6
On Tue, Oct 22, 2024 at 12:43:09PM +0530, Sibi Sankar wrote:
> 
> 
> On 10/7/24 23:36, Dmitry Baryshkov wrote:
> > On Mon, Oct 07, 2024 at 11:40:19AM GMT, Sibi Sankar wrote:
> > > Document the various memory buses that can be monitored and scaled by
> > > the memory latency governor hosted by the QCOM SCMI Generic Extension
> > > Protocol v1.0.
> > > 
> > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> 
> Hey Dmitry,
> 
> Thanks for taking time to review the series!
> 
> > > ---
> > > 
> > > v3:
> > > * Restructure the bindings to mimic IMX [Christian]
> > > 
> > >   .../bindings/firmware/arm,scmi.yaml           |   1 +
> > >   .../bindings/firmware/qcom,scmi-memlat.yaml   | 246 ++++++++++++++++++
> > >   .../dt-bindings/firmware/qcom,scmi-memlat.h   |  22 ++
> > >   3 files changed, 269 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml
> > >   create mode 100644 include/dt-bindings/firmware/qcom,scmi-memlat.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > index 54d7d11bfed4..1d405f429168 100644
> > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > @@ -24,6 +24,7 @@ description: |
> > >   anyOf:
> > >     - $ref: /schemas/firmware/nxp,imx95-scmi.yaml
> > > +  - $ref: /schemas/firmware/qcom,scmi-memlat.yaml
> > >   properties:
> > >     $nodename:
> > > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml
> > > new file mode 100644
> > > index 000000000000..0e8ea6dacd6a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/firmware/qcom,scmi-memlat.yaml
> > > @@ -0,0 +1,246 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/firmware/qcom,scmi-memlat.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Qualcomm SCMI Memory Bus nodes
> > > +
> > > +maintainers:
> > > +  - Sibi Sankar <quic_sibis@quicinc.com>
> > > +
> > > +description:
> > > +  This binding describes the various memory buses that can be monitored and scaled
> > > +  by memory latency governor running on the CPU Control Processor (SCMI controller).
> > > +
> > > +properties:
> > > +  protocol@80:
> > > +    $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node'
> > > +    unevaluatedProperties: false
> > > +
> > > +    properties:
> > > +      reg:
> > > +        const: 0x80
> > > +
> > > +    patternProperties:
> > > +      '^memory-[0-9]$':
> > > +        type: object
> > > +        unevaluatedProperties: false
> > > +        description:
> > > +          The list of all memory buses that can be monitored and scaled by the
> > > +          memory latency governor running on the SCMI controller.
> > > +
> > > +        properties:
> > > +          qcom,memory-type:
> > > +            $ref: /schemas/types.yaml#/definitions/uint32
> > > +            enum: [0, 1, 2]
> > > +            description: |
> > > +              Memory Bus Identifier
> > > +              0 = QCOM_MEM_TYPE_DDR
> > > +              1 = QCOM_MEM_TYPE_LLCC
> > > +              2 = QCOM_MEM_TYPE_DDR_QOS
> > 
> > I'm sorry if this has been discussed and frowned upon, but can you
> > squash memory type into device node?
> 
> I don't think anyone had any strong opinions on how the
> nodes is to be named. We went with a generic node name since
> it could accomodate multiple instances of llcc or ddr in the
> future. We didn't want it be named ddr-0/ddr-1 and so on. So
> I'll continue to stick with the current naming unless you have
> a strong reason other than readability.

As I wrote in the other email, the memory types are not equal. They have
different properties, etc. Having non-generic names allows describing
that in schema.

Last, but not least, please consider how reserved memory nodes are
handled nowadays: they have non-generic names, each one describing the
purpose / kind.

> > protocol@80 {
> > 	ddr {
> > 	};
> > 
> > 	llcc {
> > 	};
> > 
> > 	ddr-qos {
> > 	};
> > };
> > 
> > > +
> > > +          freq-table-hz:
> > > +            items:
> > > +              items:
> > > +                - description: Minimum frequency of the memory bus in Hz
> > > +                - description: Maximum frequency of the memory bus in Hz
> > 
> > Does it make sense for the DDR-QOS type? Can we hardcode those values
> > and drop freq-table-hz from the DDR-QOS node?
> > 
> > Also, can we drop this completely by adding one extra OPP entry with the
> > minimum memory bus frequency?
> 
> the map table doesn't necessarily list all the supported
> frequencies. It was made that way so that the table is flexible
> enough that it doesn't have to be changed a lot across platforms.
> Hence a need for a separate property to list min/max freq.

Please use opp-supported-hw or other similar techniques to describe
supported frequencies.

> 
> > 
> > > +
> > > +        patternProperties:
> > > +          '^monitor-[0-9]$':
> > > +            type: object
> > > +            unevaluatedProperties: false
> > > +            description:
> > > +              The list of all monitors detecting the memory latency bound workloads using
> > > +              various counters.
> > > +
> > > +            properties:
> > > +              qcom,compute-type:
> > > +                description:
> > > +                  Monitors of type compute perform bus dvfs based on a rudimentary CPU
> > > +                  frequency to memory frequency map.
> > > +                type: boolean
> > 
> > This seems to be redundant. If there is no qcom,ipm-ceil property, then
> > it's qcom,compute-type, isn't it?
> 
> ack
> 
> > 
> > > +
> > > +              qcom,ipm-ceil:
> > > +                $ref: /schemas/types.yaml#/definitions/uint32
> > > +                description:
> > > +                  Monitors having this property perform bus dvfs based on the same
> > > +                  rudimentary table but the scaling is performed only if the calculated
> > > +                  IPM (Instruction Per Misses) exceeds the given ceiling.
> > > +
> > > +              cpus:
> > > +                $ref: /schemas/types.yaml#/definitions/phandle-array
> > > +                description:
> > > +                  Should be a list of phandles to CPU nodes (as described in
> > > +                  Documentation/devicetree/bindings/arm/cpus.yaml).
> > 
> > Which CPU nodes? I see that the examples list all CPUs here. Do we
> > really need them?
> 
> This observation is only valid for X1E where all the cpus have
> identical freq charecteristics. Even with this case we need to
> list them to handle cases where cpus gets disabled by the bootloader
> on lower cored X1E parts i.e. we use this to figure out the actual
> physical mask.

This should be a part of the description.

BTW, why do you need to remove bootloader-removed cores? Can you simply
ignore non-existing CPUs instead?

> 
> > 
> > > +
> > > +              operating-points-v2: true
> > > +              opp-table:
> > > +                type: object
> > > +
> > > +            required:
> > > +              - cpus
> > > +              - operating-points-v2
> > > +
> > > +            oneOf:
> > > +              - required: [ 'qcom,compute-type' ]
> > > +              - required: [ 'qcom,ipm-ceil' ]
> > > +
> > > +        required:
> > > +          - qcom,memory-type
> > > +          - freq-table-hz
> > > +
> > > +additionalProperties: true
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/firmware/qcom,scmi-memlat.h>
> > > +
> > > +    firmware {
> > > +        scmi {
> > > +            compatible = "arm,scmi";
> > > +            mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>;
> > > +            mbox-names = "tx", "rx";
> > > +            shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>;
> > > +
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            protocol@80 {
> > > +                reg = <0x80>;
> > > +
> > > +                memory-0 {
> > > +                    qcom,memory-type = <QCOM_MEM_TYPE_DDR>;
> > > +                    freq-table-hz = /bits/ 64 <200000000 4224000000>;
> > > +
> > > +                    monitor-0 {
> > 
> > Hmm. Can we say that each memory type can have at most one IPM and one
> > compute aka "passive" memlat monitor? Does it make sense to use them as
> > node names and drop the extra monitor-M names?
> 
> Again this observation is valid only for X1E where the cpu freq
> across cpu's are identical across clusters and is not true for
> other mobile SoCs. So each memory can have more than 2 monitors
> i.e. atleast one active/passibe monitor for each cluster.

Description or commit message, please.

> 
> > 
> > > +                        qcom,ipm-ceil = <20000000>;
> > > +                        cpus = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7
> > > +                                &CPU8 &CPU9 &CPU10 &CPU11>;
> > 
> > Are CPU lists different between monitors? Can they be different? Can
> > they be different between different memory types?
> 
> same explanation.
> 
> > 
> > > +                        operating-points-v2 = <&memory0_monitor0_opp_table>;
> > > +
> > > +                        memory0_monitor0_opp_table: opp-table {
> > 
> > sensible names are better:
> 
> I think I just picked these names up from a cpufreq table upstream.

Doesn't mean that you can't be better than that :-D

> 
> > 
> > ddr_ipm_opp_table: opp-table {
> > };
> >
Jeffrey Hugo Nov. 6, 2024, 10:18 p.m. UTC | #7
On 10/7/2024 12:10 AM, Sibi Sankar wrote:
> 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

The #define does not match the #ifndef (missing "_H")

> +
> +/* 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 */
Sibi Sankar Nov. 14, 2024, 4:17 a.m. UTC | #8
On 11/7/24 03:48, Jeffrey Hugo wrote:
> On 10/7/2024 12:10 AM, Sibi Sankar wrote:
>> 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
> 
> The #define does not match the #ifndef (missing "_H")

Thanks for catching this. Will fix it in the next re-spin.

-Sibi

> 
>> +
>> +/* 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 */
>
diff mbox series

Patch

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