Re: [PATCH v7 01/14] dt-bindings: net: mediatek,net: allow irq names

[Krzysztof Kozlowski <krzk@kernel.org>]

On 01/07/2025 12:51, Frank Wunderlich wrote:
> Am 1. Juli 2025 08:44:02 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>> On Sat, Jun 28, 2025 at 06:54:36PM +0200, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> In preparation for MT7988 and RSS/LRO allow the interrupt-names
>>
>> Why? What preparation, what is the purpose of adding the names, what do
>> they solve?
> 
> Devicetree handled by the mtk_eth_soc driver have
> a wild mix of shared and non-shared irq definitions
> accessed by index (shared use index 0,
> non-shared
> using 1+2). Some soc have only 3 FE irqs (like mt7622).
> 
> This makes it unclear which irq is used for what
> on which SoC. Adding names for irq cleans this a bit
> in device tree and driver.

It's implied ABI now, even if the binding did not express that. But
interrupt-names are not necessary to express that at all. Look at other
bindings: we express the list by describing the items:
items:
  - description: foo
  - ... bar

> 
>>> property. Also increase the maximum IRQ count to 8 (4 FE + 4 RSS),
>>
>> Why? There is no user of 8 items.
> 
> MT7988 *with* RSS/LRO (not yet supported by driver
> yet,but i add the irqs in devicetree in this series)
> use 8 irqs,but RSS is optional and 4 irqs get working
> ethernet stack.

That's separate change than fixing ABI and that user MUST BE HERE. You
cannot add some future interrupts for some future device. Adding new
device is the only reason to add more interrupts.

> 
> I hope this explanation makes things clearer...


Commit msg must explain all this, not me asking.

> 
>>> but set boundaries for all compatibles same as irq count.
>>
>> Your patch does not do it.
> 
> I set Min/max-items for interrupt names below like
> interrupts count defined.

No, you don't. It's all fluid and flexible - limited constraints.

> 
>>>
>>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>>> ---
>>> v7: fixed wrong rebase
>>> v6: new patch splitted from the mt7988 changes
>>> ---
>>>  .../devicetree/bindings/net/mediatek,net.yaml | 38 ++++++++++++++++++-
>>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
>>> index 9e02fd80af83..6672db206b38 100644
>>> --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
>>> +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
>>> @@ -40,7 +40,19 @@ properties:
>>>  
>>>    interrupts:
>>>      minItems: 1
>>> -    maxItems: 4
>>> +    maxItems: 8
>>> +
>>> +  interrupt-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: fe0
>>> +      - const: fe1
>>> +      - const: fe2
>>> +      - const: fe3
>>> +      - const: pdma0
>>> +      - const: pdma1
>>> +      - const: pdma2
>>> +      - const: pdma3
>>>  
>>>    power-domains:
>>>      maxItems: 1
>>> @@ -135,6 +147,10 @@ allOf:
>>>            minItems: 3
>>>            maxItems: 3
>>>  
>>> +        interrupt-names:
>>> +          minItems: 3
>>> +          maxItems: 3
>>> +
>>>          clocks:
>>>            minItems: 4
>>>            maxItems: 4
>>> @@ -166,6 +182,9 @@ allOf:
>>>          interrupts:
>>>            maxItems: 1
>>>  
>>> +        interrupt-namess:
>>> +          maxItems: 1
>>> +
>>>          clocks:
>>>            minItems: 2
>>>            maxItems: 2
>>> @@ -192,6 +211,10 @@ allOf:
>>>            minItems: 3
>>>            maxItems: 3
>>>  
>>> +        interrupt-names:
>>> +          minItems: 3
>>> +          maxItems: 3
>>> +
>>>          clocks:
>>>            minItems: 11
>>>            maxItems: 11
>>> @@ -232,6 +255,10 @@ allOf:
>>>            minItems: 3
>>>            maxItems: 3
>>>  
>>> +        interrupt-names:
>>> +          minItems: 3
>>> +          maxItems: 3
>>> +
>>>          clocks:
>>>            minItems: 17
>>>            maxItems: 17
>>> @@ -274,6 +301,9 @@ allOf:
>>>          interrupts:
>>>            minItems: 4
>>>  
>>> +        interrupt-names:
>>> +          minItems: 4
>>> +
>>>          clocks:
>>>            minItems: 15
>>>            maxItems: 15
>>> @@ -312,6 +342,9 @@ allOf:
>>>          interrupts:
>>>            minItems: 4
>>>  
>>> +        interrupt-names:
>>> +          minItems: 4
>>
>> 8 interrupts is now valid?
>>
>>> +
>>>          clocks:
>>>            minItems: 15
>>>            maxItems: 15
>>> @@ -350,6 +383,9 @@ allOf:
>>>          interrupts:
>>>            minItems: 4
>>>  
>>> +        interrupt-names:
>>> +          minItems: 4
>>
>> So why sudenly this device gets 8 interrupts? This makes no sense,
>> nothing explained in the commit msg.
> 
> 4 FrameEngine IRQs are required to be defined (currently 2 are used in driver).
> The other 4 are optional,but added in the devicetree

There were only 4 before and you do not explain why all devices get 8.
You mentioned that MT7988 has 8 but now make 8 for all other variants!

Why you are not answering this question?

> to not run into problems supporting old devicetree
> when adding RSS/LRO to driver.

This is not about driver, it does not matter for the driver. Binding and
DTS are supposed to be complete.

> 
>> I understand nothing from this patch and I already asked you to clearly
>> explain why you are doing things. This patch on its own makes no sense.
>>
>> Best regards,
>> Krzysztof
>>
> 
> 
> regards Frank


Best regards,
Krzysztof