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

Frank Wunderlich posted 14 patches 3 months, 1 week ago
There is a newer version of this series
[PATCH v7 01/14] dt-bindings: net: mediatek,net: allow irq names
Posted by Frank Wunderlich 3 months, 1 week ago
From: Frank Wunderlich <frank-w@public-files.de>

In preparation for MT7988 and RSS/LRO allow the interrupt-names
property. Also increase the maximum IRQ count to 8 (4 FE + 4 RSS),
but set boundaries for all compatibles same as irq count.

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
+
         clocks:
           minItems: 15
           maxItems: 15
@@ -350,6 +383,9 @@ allOf:
         interrupts:
           minItems: 4
 
+        interrupt-names:
+          minItems: 4
+
         clocks:
           minItems: 24
           maxItems: 24
-- 
2.43.0
Re: [PATCH v7 01/14] dt-bindings: net: mediatek,net: allow irq names
Posted by Krzysztof Kozlowski 3 months, 1 week ago
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?

> property. Also increase the maximum IRQ count to 8 (4 FE + 4 RSS),

Why? There is no user of 8 items.

> but set boundaries for all compatibles same as irq count.

Your patch does not do it.

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

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
Re: [PATCH v7 01/14] dt-bindings: net: mediatek,net: allow irq names
Posted by Frank Wunderlich 3 months, 1 week ago
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.

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

I hope this explanation makes things clearer...

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

>> 
>> 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
to not run into problems supporting old devicetree
when adding RSS/LRO to driver.

>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
Re: [PATCH v7 01/14] dt-bindings: net: mediatek,net: allow irq names
Posted by Krzysztof Kozlowski 3 months, 1 week ago
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
Re: [PATCH v7 01/14] dt-bindings: net: mediatek,net: allow irq names
Posted by Frank Wunderlich (linux) 3 months ago
Am 2025-07-02 08:27, schrieb Krzysztof Kozlowski:
> 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

ok, so i need to define descriptions for all interrupts instead of only 
increasing the count. Ok, was not clear to me.

so something like this:

item0: on SoCs with shared IRQ (mt762[18]) used for RX+TX, on other free 
to be used
item1: on non-shared SoCs used for TX
item2: on non-shared SoCs used for RX (except RSS/LRO is used)
item3: reserved / currently unused
item4-7: IRQs for RSS/LRO

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

MT7988 is basicly new because there is no devicetree there yet...only 
driver and
this (incomplete) binding.

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

i thought i can limit is by setting the MaxItems in the soc-spcific 
blocks below.
What reason does MaxItems have there if not this? of course if there is 
any Soc not
having a specific block it is open.But this is also the case for 
interrupts property
handles the same way before.

I only left it open on mt7988 (only set minitems because all 8 can be 
used, but 4 are required).

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

limited here to the same as interrupts.

>>>>          clocks:
>>>>            minItems: 4
>>>>            maxItems: 4
>>>> @@ -166,6 +182,9 @@ allOf:
>>>>          interrupts:
>>>>            maxItems: 1
>>>> 
>>>> +        interrupt-namess:
>>>> +          maxItems: 1
dito

>>>>          clocks:
>>>>            minItems: 2
>>>>            maxItems: 2
>>>> @@ -192,6 +211,10 @@ allOf:
>>>>            minItems: 3
>>>>            maxItems: 3
>>>> 
>>>> +        interrupt-names:
>>>> +          minItems: 3
>>>> +          maxItems: 3
dito and so on

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

The original binding excluded the 4 RSS/LRO IRQs as this is an optional 
feature not
yet available in driver. It is needed to get the full speed on the 10G 
interfaces.
MT7988 is the first SoC which has 10G MACs. Older Socs like mt7986 and 
mt7981 can also
support RSS/LRO to reduce cpu load. But here we will run into the "new 
kernel - old
devicetree" issue, if we try to upstream this. Maybe we do not add this 
because these
only have 2.5G MACs.

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

if i upstream the ethernet-node now with only 4 IRQS, we have to extend 
them later and
have to deal with only 4 IRQs in driver to be compatible with older DTS. 
So newer
kernel with RSS/LRO support cannot work with older DT.
To avoid this i add all related IRQs now (from DT perspective a new 
device - there is
no mt7988 device with ethernet node in devicetree yet).

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

i tried to explain :(
i have a working dts for mt7988 and try to upstream it in this series 
and thats the
cause i have to update the binding. Imho i cannot increase the 
interrupt-count in SoC
specific block (e.g. setting count globally to 4 but to 8 for only 
mt7988).

I added interrupt-names to get it cleaner in driver (access via name 
instead of
index). And also in Devicetree we see the meaning of IRQs without 
looking through
driver. I see really no reason for not adding the interrupt names.

>>> Best regards,
>>> Krzysztof

regards Frank
Re: [PATCH v7 01/14] dt-bindings: net: mediatek,net: allow irq names
Posted by Daniel Golle 3 months ago
On Thu, Jul 03, 2025 at 01:01:40PM +0200, Frank Wunderlich (linux) wrote:
> Am 2025-07-02 08:27, schrieb Krzysztof Kozlowski:
> > 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
> 
> ok, so i need to define descriptions for all interrupts instead of only
> increasing the count. Ok, was not clear to me.
> 
> so something like this:
> 
> item0: on SoCs with shared IRQ (mt762[18]) used for RX+TX, on other free to
> be used
> item1: on non-shared SoCs used for TX
> item2: on non-shared SoCs used for RX (except RSS/LRO is used)
> item3: reserved / currently unused
> item4-7: IRQs for RSS/LRO

These descriptions match the current *software* use of those interrupts,
however, DT should describe the hardware and esp. item0 up to item3 could
be used in different ways in the future (by programming MTK_FE_INT_GRP
register differently).

I think using interrupt-names fe0...fe3 and pdma0...pdma3 is still the
best option, so the driver can request the interrupts by name which is
much more readable in the driver code and SoC's dtsi than relying on a
specific order.

> > 
> > 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?
> 
> The original binding excluded the 4 RSS/LRO IRQs as this is an optional
> feature not
> yet available in driver. It is needed to get the full speed on the 10G
> interfaces.
> MT7988 is the first SoC which has 10G MACs. Older Socs like mt7986 and
> mt7981 can also
> support RSS/LRO to reduce cpu load. But here we will run into the "new
> kernel - old
> devicetree" issue, if we try to upstream this. Maybe we do not add this
> because these
> only have 2.5G MACs.

It might be important to note that

MT7621, MT7628: 1 IRQ
MT7622, MT7623: 3 IRQs (only two used by the driver for now)
MT7981, MT7986: 4 IRQs (only two used by the driver for now)

While older SoCs MT7981 and MT7986 have limited support for *either LRO
or RSS* in hardware, only MT7988 got 4 frame-engine IRQs like MT7981 and
MT7986 and an additional 4 IRQs for the 4 RX DMA rings on top of that,
so a total of 8, and can do both RSS and LRO.