Message-ID: <aK8w1L3gHBk2Fz1k@e133380.arm.com>
Date: Wed, 27 Aug 2025 17:22:44 +0100
From: Dave Martin <Dave.Martin@....com>
To: James Morse <james.morse@....com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-acpi@...r.kernel.org, devicetree@...r.kernel.org,
	D Scott Phillips OS <scott@...amperecomputing.com>,
	carl@...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>, Koba Ko <kobak@...dia.com>,
	Shanker Donthineni <sdonthineni@...dia.com>, fenghuay@...dia.com,
	baisheng.gao@...soc.com,
	Jonathan Cameron <jonathan.cameron@...wei.com>,
	Rob Herring <robh@...nel.org>, Rohit Mathew <rohit.mathew@....com>,
	Rafael Wysocki <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Hanjun Guo <guohanjun@...wei.com>,
	Sudeep Holla <sudeep.holla@....com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH 09/33] dt-bindings: arm: Add MPAM MSC binding

Hi James,

On Fri, Aug 22, 2025 at 03:29:50PM +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.

I'll leave detailed review to other people for now, since I'm not so up
to speed on all things DT.

A few random comments, below.


[...]

> diff --git a/Documentation/devicetree/bindings/arm/arm,mpam-msc.yaml b/Documentation/devicetree/bindings/arm/arm,mpam-msc.yaml

[...]

> @@ -0,0 +1,200 @@

[...]

> +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

There seems to be no clear statement about how these differ.

> +  reg:
> +    maxItems: 1
> +    description: A memory region containing registers as defined in the MPAM
> +      specification.

There seems to be no handling of PCC-based MSCs here.  Should there be?

If this can be added later in a backwards-compatible way, I guess
that's not a problem (and this is what compatible strings are for, if
all else fails.)

An explicit statement that PCC is not supported here might be helpful,
though.

> +  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

Yeugh.  Is this really the only way to say "one or both of foo"?

(I don't know the answer to this -- though I can believe that it's
true.  Perhaps just not describing this property is another option.
Many bindings seem not to bother.)

> +
> +  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]$':

It this supposed to be '^ris@[0-9a-f]+$' ?

Currently MPAMF_IDR.RIS_MAX is only 4 bits in size and so cannot be
greater than 0xf.  But it is not inconceivable that a future revision
of the architecture might enable more -- and the are 4 RES0 bits
looming over the RIS_MAX field, just waiting to be used...

(In any case, it feels wrong to try to enforce numeric bounds with a
regex, even in the cases where it happens to work straightforwardly.)

> +    type: object
> +    additionalProperties: false
> +    description:
> +      RIS nodes for each RIS in an MSC. These nodes are required for each RIS

The architectural term is "resource instance", not "RIS".

But "RIS nodes" is fine for describing the DT nodes, since we can call
them what we like, and "ris" is widely used inside the MPAM driver.

People writing DTs should not need to be familiar with the driver's
internal naming conventions, though.

(There are other instances, but I won't comment on them all
individually.)

> +      implementing known MPAM controls
> +
> +    properties:
> +      compatible:
> +        enum:
> +            # Bulk storage for cache

Nit: What is "bulk storage"?

The MPAM spec just refers to "cache" or "cache memory".

> +          - arm,mpam-cache
> +            # Memory bandwidth
> +          - arm,mpam-memory
> +
> +      reg:
> +        minimum: 0
> +        maximum: 0xf
> +
> +      cpus:
> +        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
> +        description:
> +          By default, the MPAM enabled device associated with a RIS is the MSC's

Associated how?  Is this the device where the physical resources
managed by the MSC are located?

> +          parent node. It is possible for each RIS to be associated with different
> +          devices in which case 'arm,mpam-device' should be used.

[...]

> +examples:
> +  - |
> +    L3: cache-controller@...00000 {
> +        compatible = "arm,dsu-l3-cache", "cache";
> +        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  */

"node's" -> "nodes".

(or it this supposed to be in the singular -- i.e., the immediately
parent cache node only?)

Anyway, it looks like this is commenting on the "reg" property, which
doesn't seem right.

Is this commnent supposed instead to explain the omission of the "cpus"
property?  If so, that should be made clearer.

> +            reg = <0x10000 0x2000>;
> +            interrupts = <1>, <2>;
> +            interrupt-names = "error", "overflow";
> +            arm,not-ready-us = <1>;
> +        };
> +    };

[...]

(Examples otherwise not reviewed in detail.)

Cheers
---Dave

Powered by blists - more mailing lists