Re: [RFC PATCH v3 03/37] dt-bindings: Add binding for kalray,kv3-1-intc

[Krzysztof Kozlowski <krzk@kernel.org>]

On 31/07/2024 16:47, Yann Sionneau wrote:
>>> +description: |
>>> +  The Kalray Core Interrupt Controller is tightly integrated in each kv3 core
>>> +  present in the Coolidge SoC.
>>> +
>>> +  It provides the following features:
>>> +  - 32 independent interrupt sources
>>> +  - 2-bit configurable priority level
>>> +  - 2-bit configurable ownership level
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: kalray,kv3-1-intc
>> What is the SoC name/model? You use "Kalray Core" and "Coolidge" and
>> "kv3-1".
> 
> The SoC name is Coolidge , it contains "Kalray cores". Kalray being the name of the company.
> 
> The exact core name is kv3-1. The core is part of the kvx family: next core generation will most likely be called kv4 or kv4-1.
> 
> The question whether to use "coolidge" (soc name) or "kv3-1" (cpu core name) for our IPs compatible is indeed a good one, we talked about it internally at Kalray.
> 
> We ended up using "kv3-1" at the beginning of the compatible when the hw IP is very closed/tightly integrated into the CPU core.
> 
> We ended up using "coolidge" when the hw IP is just some general IP inside the SoC (like the IOMMU, the dma_noc).
> 
> The "intc" or "core intc", is very tightly integrated into the cpu core.
> 
> The "core intc" is configured directly using core registers named SFR (System Function Registers) using special SFR handling instructions. It is not memory mapped.
> 
> So we ended up using "kv3-1" in the compatible. Is this OK?

Comaptibles are supposed to be based on SoC model name, thus kv3-1 is
fine (based in your explanation). However unified naming in title or
description would be helpful, e.g. in title:
"Kalray Coolidge kv3-1 Core Interrupt Controller"

And all other compatibles and titles need fixing. For example in other
places you use "coolidge".

> 
>>
>>> +
>>> +  "#interrupt-cells":
>>> +    const: 1
>>> +    description:
>>> +      The IRQ number.
>>> +
>>> +  "#address-cells":
>>> +    const: 0
>>> +
>>> +  interrupt-controller: true
>>> +
>>> +additionalProperties: false
>> Please put it after "required:" block. See example-schema. This applies
>> everywhere.
> Oops, ok, I will do this.
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - "#interrupt-cells"
>>> +  - "#address-cells"
>>> +  - interrupt-controller
>>> +
>>> +examples:
>>> +  - |
>>> +    intc: interrupt-controller {
>> No resources? How does it talk with the hardware?
> CPU configures the core intc using special instructions to access SFR (System Function Registers).

Ack.


Best regards,
Krzysztof