Hi Dave,

On 27/08/2025 17:22, Dave Martin wrote:

On Fri, Aug 22, 2025 at 03:29:50PM +0000, James Morse wrote:

quoted

From: Rob Herring <robh@kernel.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.

Me neither!

quoted

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

[...]

quoted

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

[...]

quoted

+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 [off-list ref]
+
+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.

It's a more-specific compatible, I think these are usually things like:
| compatible = "acme,mega-cache-9000", "arm,mpam-msc"

Where the driver can key errata-workaround on the vendor specific bit when needed.

In this case - I think they're examples, but Rob said they were supposed to be in some
other list of compatible. (not sure what/where that is)

quoted

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

That is newer than this document. On DT platforms PCC is spelled SCMI, and is
discoverable. Andre P prototyped this, (patches in the extras branch) but no-one
has come out of the woodwork to say they actually need it yet.

ACPI PCC is a definite maybe.

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.

I'm pretty sure its discoverable on DT/SCMI platforms.

quoted

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

quoted

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

Looks like yes. Fixed.

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.)

quoted

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

What about the architecture's name for fields?
This number goes in MPAMCFG_PART_SEL.RIS.

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

quoted

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

Nit: What is "bulk storage"?

Probably to distinguish it from other storage a cache may have, such as tag-ram.

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

I figure these are comments, I'll remove them...

quoted

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

By the phandle this is a description for.

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

Yes,

quoted

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

[...]

quoted

+examples:
+  - |
+    L3: cache-controller@30000000 {
+        compatible = "arm,dsu-l3-cache", "cache";
+        cache-level = <3>;
+        cache-unified;
+
+        ranges = <0x0 0x30000000 0x800000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        msc@10000 {
+            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?)

The MSC's parent cache node can be used to find the affinity.
I'll make it singular and drop the 's

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.

I'll move it to the end of the list of properties so it doesn't look like it belongs to
the one below it.

quoted

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

Thanks,

James