Received-SPF: pass (google.com: domain of linux-kernel+bounces-70434-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <3c2f1c61-89b4-4103-ac45-a2a541de215e@sifive.com>
Date: Sun, 18 Feb 2024 09:50:59 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 3/6] dt-bindings: cache: Add SiFive Extensible Cache
 controller
Content-Language: en-US
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
 Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Eric Lin <eric.lin@sifive.com>, Conor Dooley <conor@kernel.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, Paul Walmsley <paul.walmsley@sifive.com>,
 linux-riscv@lists.infradead.org, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 linux-arm-kernel@lists.infradead.org
References: <20240216000837.1868917-1-samuel.holland@sifive.com>
 <20240216000837.1868917-4-samuel.holland@sifive.com>
 <d655b72e-3094-4b6b-bee8-9677b7c987ce@linaro.org>
From: Samuel Holland <samuel.holland@sifive.com>
In-Reply-To: <d655b72e-3094-4b6b-bee8-9677b7c987ce@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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@sifive.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@30040000 {
>> +        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
>