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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 18 Feb 2024 09:50:59 -0600
From: Samuel Holland <samuel.holland@...ive.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Eric Lin <eric.lin@...ive.com>, Conor Dooley <conor@...nel.org>
Cc: Palmer Dabbelt <palmer@...belt.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Paul Walmsley <paul.walmsley@...ive.com>,
 linux-riscv@...ts.infradead.org, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 linux-arm-kernel@...ts.infradead.org
Subject: 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:
>> From: Eric Lin <eric.lin@...ive.com>
>>
>> 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.
> 
>> +  "#size-cells": true
> 
> ditto
> 
>> +  ranges: true
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  cache-block-size:
>> +    const: 64
>> +
>> +  cache-level: true
> 
> 5 is acceptable? I would argue this should be even const.
> 
>> +  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.

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

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

>> +
>> +      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@...40000 {
>> +        compatible = "sifive,extensiblecache0", "cache";
>> +        ranges = <0x30040000 0x30040000 0x10000>;
>> +        interrupts = <0x4>;
> 
> You use hex as interrupt numbers on your platforms?
> 
>> +        cache-block-size = <0x40>;
>> +        cache-level = <3>;
>> +        cache-sets = <0x800>;
> 
> Best regards,
> Krzysztof
>