Re: [PATCH v1 3/6] dt-bindings: cache: Add SiFive Extensible Cache controller

Hi Krzysztof,

On 2024-02-17 3:09 AM, Krzysztof Kozlowski wrote:

On 16/02/2024 01:08, Samuel Holland wrote:

quoted

From: Eric Lin <redacted>

Add YAML DT binding documentation for the SiFive Extensible Cache
controller. The Extensible Cache controller interleaves cache blocks
across a number of heterogeneous independently-programmed slices. Each
slice contains an MMIO interface for configuration, cache maintenance,
error reporting, and performance monitoring.

+allOf:
+  - $ref: /schemas/cache-controller.yaml#
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - sifive,extensiblecache0
+
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - const: sifive,extensiblecache0
+      - const: cache
+
+  "#address-cells": true

const or enum: [1, 2], depending on the addressing you need here.

quoted

+  "#size-cells": true

ditto

quoted

+  ranges: true
+
+  interrupts:
+    maxItems: 1
+
+  cache-block-size:
+    const: 64
+
+  cache-level: true

5 is acceptable? I would argue this should be even const.

quoted

+  cache-sets: true
+  cache-size: true

Some constraints on any of these?

Thanks for the feedback. I will add the various constraints in v2, though some
constraints will be somewhat loose as the topology is highly configurable.

quoted

+  cache-unified: true
+
+patternProperties:
+  "^cache-controller@[0-9a-f]+$":
+    type: object
+    additionalProperties: false

What is this object supposed to represent? Add description.

I will add a description in v2.

This object represents a single slice of the cache. Requests from clients are
interleaved between cache slices depending on the client, the address, etc.

Since there is no strong relationship between client (i.e. CPU) and cache slice,
the next-level-cache property must point to the top-level EC node, not a slice.

quoted

+    properties:
+      reg:
+        maxItems: 1
+
+      cache-block-size:
+        const: 64
+
+      cache-sets: true
+      cache-size: true
+      cache-unified: true

cache-level

I will add this in v2. It seemed redundant since the value cannot differ between
slices.

Regards,
Samuel

quoted

+
+      sifive,bm-event-counters:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        default: 0
+        description: Number of bucket monitor registers in this slice
+
+      sifive,cache-ways:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: Number of ways in this slice (independent of cache size)
+
+      sifive,perfmon-counters:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        default: 0
+        description: Number of PMU counter registers in this slice
+
+    required:
+      - reg
+      - cache-block-size
+      - cache-sets
+      - cache-size
+      - cache-unified
+      - sifive,cache-ways
+
+required:
+  - compatible
+  - ranges
+  - interrupts
+  - cache-block-size
+  - cache-level
+  - cache-sets
+  - cache-size
+  - cache-unified
+
+additionalProperties: false
+
+examples:
+  - |
+    cache-controller@30040000 {
+        compatible = "sifive,extensiblecache0", "cache";
+        ranges = <0x30040000 0x30040000 0x10000>;
+        interrupts = <0x4>;

You use hex as interrupt numbers on your platforms?

quoted

+        cache-block-size = <0x40>;
+        cache-level = <3>;
+        cache-sets = <0x800>;

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel