Message-ID: <742fe06b-81ab-4402-ae6a-19d6734507ae@arm.com>
Date: Tue, 5 Aug 2025 18:08:27 +0100
From: James Morse <james.morse@....com>
To: Rob Herring <robh@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 Ben Horgan <ben.horgan@....com>, Rohit Mathew <rohit.mathew@....com>,
 Shanker Donthineni <sdonthineni@...dia.com>, Zeng Heng
 <zengheng4@...wei.com>, Lecopzer Chen <lecopzerc@...dia.com>,
 Carl Worth <carl@...amperecomputing.com>,
 shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>
Subject: Re: [RFC PATCH 11/36] dt-bindings: arm: Add MPAM MSC binding

Hi Rob,

On 11/07/2025 22:43, Rob Herring wrote:
> On Fri, Jul 11, 2025 at 06:36:23PM +0000, James Morse wrote:
>> From: Rob Herring <robh@...nel.org>
>>
>> The binding is designed around the assumption that an MSC will be a
>> sub-block of something else such as a memory controller, cache controller,
>> or IOMMU. However, it's certainly possible a design does not have that
>> association or has a mixture of both, so the binding illustrates how we can
>> support that with RIS child nodes.
>>
>> A key part of MPAM is we need to know about all of the MSCs in the system
>> before it can be enabled. This drives the need for the genericish
>> 'arm,mpam-msc' compatible. Though we can't assume an MSC is accessible
>> until a h/w specific driver potentially enables the h/w.

> Is there any DT based h/w using this? I'm not aware of any.

I'm told there is. I'll let them come out of the wood work to confirm it ...


> I would 
> prefer not merging this until there is. I have little insight whether 
> these genericish compatibles will be sufficient, but I have lots of 
> experience to say they won't be. I would also suspect that if anyone has 
> started using this, they've just extended/modified it however they 
> wanted and no feedback got to me.

Sure, what are you looking for here, Reviewed-by tags from someone with a hardware
platform that is going to ship with DT?


>> diff --git a/Documentation/devicetree/bindings/arm/arm,mpam-msc.yaml b/Documentation/devicetree/bindings/arm/arm,mpam-msc.yaml
>> new file mode 100644
>> index 000000000000..9d542ecb1a7d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/arm,mpam-msc.yaml
>> @@ -0,0 +1,227 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/arm,mpam-msc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Arm Memory System Resource Partitioning and Monitoring (MPAM)
>> +
>> +description: |
>> +  The Arm MPAM specification can be found here:
>> +
>> +  https://developer.arm.com/documentation/ddi0598/latest
>> +
>> +maintainers:
>> +  - Rob Herring <robh@...nel.org>
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: arm,mpam-msc                   # Further details are discoverable
>> +      - const: arm,mpam-memory-controller-msc
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: A memory region containing registers as defined in the MPAM
>> +      specification.
>> +
>> +  interrupts:
>> +    minItems: 1
>> +    items:
>> +      - description: error (optional)
>> +      - description: overflow (optional, only for monitoring)
>> +
>> +  interrupt-names:
>> +    oneOf:
>> +      - items:
>> +          - enum: [ error, overflow ]
>> +      - items:
>> +          - const: error
>> +          - const: overflow
>> +
>> +  arm,not-ready-us:
>> +    description: The maximum time in microseconds for monitoring data to be
>> +      accurate after a settings change. For more information, see the
>> +      Not-Ready (NRDY) bit description in the MPAM specification.
>> +
>> +  numa-node-id: true # see NUMA binding
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +patternProperties:
>> +  '^ris@[0-9a-f]$':
>> +    type: object
>> +    additionalProperties: false
>> +    description: |
> 
> '|' can be dropped.
> 
>> +      RIS nodes for each RIS in an MSC. These nodes are required for each RIS
>> +      implementing known MPAM controls
>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +            # Bulk storage for cache
>> +          - arm,mpam-cache
>> +            # Memory bandwidth
>> +          - arm,mpam-memory
>> +
>> +      reg:
>> +        minimum: 0
>> +        maximum: 0xf
>> +
>> +      cpus:
>> +        $ref: '/schemas/types.yaml#/definitions/phandle-array'

> Don't need the type. It's in the core schemas now.

(I assume the type is that '$ref' line)


>> +        description:
>> +          Phandle(s) to the CPU node(s) this RIS belongs to. By default, the parent
>> +          device's affinity is used.
>> +
>> +      arm,mpam-device:
>> +        $ref: '/schemas/types.yaml#/definitions/phandle'
> 
> Don't need quotes. This should be a warning, but no testing happened 
> because the DT list and maintainers weren't CCed.

Yup, its an RFC - I only CC'd the folk that have previously expressed an interest for the
first pass. (git send-email put you on CC!)


>> +        description:
>> +          By default, the MPAM enabled device associated with a RIS is the MSC's
>> +          parent node. It is possible for each RIS to be associated with different
>> +          devices in which case 'arm,mpam-device' should be used.
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +dependencies:
>> +  interrupts: [ interrupt-names ]
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    /*
>> +    cpus {
>> +        cpu@0 {
>> +            next-level-cache = <&L2_0>;
>> +        };
>> +        cpu@100 {
>> +            next-level-cache = <&L2_1>;
>> +        };
>> +    };
>> +    */
>> +    L2_0: cache-controller-0 {
>> +        compatible = "cache";
>> +        cache-level = <2>;
>> +        cache-unified;
>> +        next-level-cache = <&L3>;
>> +
>> +    };
>> +
>> +    L2_1: cache-controller-1 {
>> +        compatible = "cache";
>> +        cache-level = <2>;
>> +        cache-unified;
>> +        next-level-cache = <&L3>;
>> +
>> +    };
> 
> All the above should be dropped. Not part of this binding.
> 
>> +
>> +    L3: cache-controller@...00000 {
>> +        compatible = "arm,dsu-l3-cache", "cache";
> 
> Pretty sure this is a warning because that compatible doesn't exist.

Not sure what to do with this. I see plenty of DT in the tree with 'cache', and you've got
'foo,a-memory-controller' below ...


>> +        cache-level = <3>;
>> +        cache-unified;
>> +
>> +        ranges = <0x0 0x30000000 0x800000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +
>> +        msc@...00 {
>> +            compatible = "arm,mpam-msc";
>> +
>> +            /* CPU affinity implied by parent cache node's  */
>> +            reg = <0x10000 0x2000>;
>> +            interrupts = <1>, <2>;
>> +            interrupt-names = "error", "overflow";
>> +            arm,not-ready-us = <1>;
>> +        };
>> +    };
>> +
>> +    mem: memory-controller@...00 {
>> +        compatible = "foo,a-memory-controller";
>> +        reg = <0x20000 0x1000>;
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges;
>> +
>> +        msc@...00 {
>> +            compatible = "arm,mpam-memory-controller-msc", "arm,mpam-msc";
>> +            reg = <0x21000 0x1000>;
>> +            interrupts = <3>;
>> +            interrupt-names = "error";
>> +            arm,not-ready-us = <1>;
>> +            numa-node-id = <1>;
>> +        };
>> +    };


James

Powered by blists - more mailing lists