[PATCH v8 02/16] dt-bindings: net: mediatek,net: allow up to 8 IRQs

Frank Wunderlich posted 16 patches 3 months ago
There is a newer version of this series
[PATCH v8 02/16] dt-bindings: net: mediatek,net: allow up to 8 IRQs
Posted by Frank Wunderlich 3 months ago
From: Frank Wunderlich <frank-w@public-files.de>

Increase the maximum IRQ count to 8 (4 FE + 4 RSS/LRO).

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

Mediatek Filogic SoCs (mt798x) have 4 additional IRQs for RSS and/or
LRO.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
v8: separate irq-count change from interrupt-names patch
---
 Documentation/devicetree/bindings/net/mediatek,net.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
index 175d1d011dc6..766224e4ed86 100644
--- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
+++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
@@ -40,7 +40,7 @@ properties:
 
   interrupts:
     minItems: 1
-    maxItems: 4
+    maxItems: 8
 
   power-domains:
     maxItems: 1
-- 
2.43.0
Re: [PATCH v8 02/16] dt-bindings: net: mediatek,net: allow up to 8 IRQs
Posted by Krzysztof Kozlowski 3 months ago
On Sun, Jul 06, 2025 at 03:21:57PM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Increase the maximum IRQ count to 8 (4 FE + 4 RSS/LRO).

Because? Hardware was updated? It was missing before?

> 
> Frame-engine-IRQs (max 4):
> MT7621, MT7628: 1 IRQ
> MT7622, MT7623: 3 IRQs (only two used by the driver for now)
> MT7981, MT7986, MT7988: 4 IRQs (only two used by the driver for now)

You updated commit msg - looks fine - but same problem as before in your
code. Now MT7981 has 4-8 interrupts, even though you say here it has only
4.

> 
> Mediatek Filogic SoCs (mt798x) have 4 additional IRQs for RSS and/or
> LRO.

Although I don't know how to treat this. Just say how many interrupts
are there (MT7981, MT7986, MT7988: 4 FE and 4 RSS), not 4 but later
actually 4+4.

I also do not understand why 7 interrupts is now valid... Are these not
connected physically?

Best regards,
Krzysztof
Re: [PATCH v8 02/16] dt-bindings: net: mediatek,net: allow up to 8 IRQs
Posted by Frank Wunderlich 3 months ago
Am 7. Juli 2025 08:31:11 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>On Sun, Jul 06, 2025 at 03:21:57PM +0200, Frank Wunderlich wrote:
>> From: Frank Wunderlich <frank-w@public-files.de>
>> 
>> Increase the maximum IRQ count to 8 (4 FE + 4 RSS/LRO).
>
>Because? Hardware was updated? It was missing before?

There is no RSS support in driver yet,so IRQs were not added to existing DTS yet.

>> 
>> Frame-engine-IRQs (max 4):
>> MT7621, MT7628: 1 IRQ
>> MT7622, MT7623: 3 IRQs (only two used by the driver for now)
>> MT7981, MT7986, MT7988: 4 IRQs (only two used by the driver for now)
>
>You updated commit msg - looks fine - but same problem as before in your
>code. Now MT7981 has 4-8 interrupts, even though you say here it has only
>4.

Ethernet works with 4,but can be 8 for MT798x.
I cannot increase the MinItems here as it will
throw error because currently only 4 are defined in DTS.same for MT7986.
>> 
>> Mediatek Filogic SoCs (mt798x) have 4 additional IRQs for RSS and/or
>> LRO.
>
>Although I don't know how to treat this. Just say how many interrupts
>are there (MT7981, MT7986, MT7988: 4 FE and 4 RSS), not 4 but later
>actually 4+4.

First block is for Frame Engine IRQs and second for RSS/LRO. Only mention total count 
across all SoCs is imho more confusing.

>I also do not understand why 7 interrupts is now valid... Are these not
>connected physically?

7 does not make sense but i know no way to allow 8 with min 4 without between (5-7).

>Best regards,
>Krzysztof

Hi

Thanks for taking time for review again.

First block are the frame engine IRQs which are max 4 and on all SoCs.
The RSS IRQs are only valid on Filogic (MT798x),so there a total of 8, but on
MT7981 and MT7986 not yet added as i prepare the RSS/LRO driver in background.
We just want to add the IRQs for MT7988 now.
regards Frank
Re: [PATCH v8 02/16] dt-bindings: net: mediatek,net: allow up to 8 IRQs
Posted by AngeloGioacchino Del Regno 3 months ago
Il 07/07/25 09:30, Frank Wunderlich ha scritto:
> Am 7. Juli 2025 08:31:11 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>> On Sun, Jul 06, 2025 at 03:21:57PM +0200, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> Increase the maximum IRQ count to 8 (4 FE + 4 RSS/LRO).
>>
>> Because? Hardware was updated? It was missing before?
> 
> There is no RSS support in driver yet,so IRQs were not added to existing DTS yet.
> 

That's the problem. It's the hardware that you should've described, not the driver.

In short, you should've allowed the interrupts from the get-go, and you wouldn't
be in this situation now :-)

>>>
>>> Frame-engine-IRQs (max 4):
>>> MT7621, MT7628: 1 IRQ
>>> MT7622, MT7623: 3 IRQs (only two used by the driver for now)
>>> MT7981, MT7986, MT7988: 4 IRQs (only two used by the driver for now)
>>
>> You updated commit msg - looks fine - but same problem as before in your
>> code. Now MT7981 has 4-8 interrupts, even though you say here it has only
>> 4.
> 
> Ethernet works with 4,but can be 8 for MT798x.
> I cannot increase the MinItems here as it will
> throw error because currently only 4 are defined in DTS.same for MT7986.
>>>
>>> Mediatek Filogic SoCs (mt798x) have 4 additional IRQs for RSS and/or
>>> LRO.
>>
>> Although I don't know how to treat this. Just say how many interrupts
>> are there (MT7981, MT7986, MT7988: 4 FE and 4 RSS), not 4 but later
>> actually 4+4.
> 
> First block is for Frame Engine IRQs and second for RSS/LRO. Only mention total count
> across all SoCs is imho more confusing.
> 
>> I also do not understand why 7 interrupts is now valid... Are these not
>> connected physically?
> 
> 7 does not make sense but i know no way to allow 8 with min 4 without between (5-7).
> 
>> Best regards,
>> Krzysztof
> 
> Hi
> 
> Thanks for taking time for review again.
> 
> First block are the frame engine IRQs which are max 4 and on all SoCs.
> The RSS IRQs are only valid on Filogic (MT798x),so there a total of 8, but on
> MT7981 and MT7986 not yet added as i prepare the RSS/LRO driver in background.
> We just want to add the IRQs for MT7988 now.
> regards Frank

Again, it's not the driver but the hardware that you're describing.

As long as you are fixing the description of the hardware, even for all three,
I am personally even fine with breaking the ABI, because the hardware description
has been wrong for all that time.

Just don't send those as Fixes commits, but next time you upstream something you
must keep in mind that in bindings/dts you're describing hardware - the driver is
something that should not drive any decision in what you write in bindings.

We're humans, so stuff like this happens - I'm not saying that you shall not make
mistakes - but again please, for the next time, please please please keep in mind
what I just said :-)

Now the options are two:
  - Break the ABI; or
  - Allow 4 or 8 interrupts (not 5, not 6, not 7)

and that - not just on MT7988 but also on 81 and 86 in one go.

Not sure if the second one is feasible, and I'm considering the first option only
because of that; if the second option can be done, act like I never ever considered
the first.

Cheers,
Angelo
Re: [PATCH v8 02/16] dt-bindings: net: mediatek,net: allow up to 8 IRQs
Posted by Frank Wunderlich 3 months ago
Hi Angelo,

Am 7. Juli 2025 12:06:02 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>Il 07/07/25 09:30, Frank Wunderlich ha scritto:
>> Am 7. Juli 2025 08:31:11 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>>> On Sun, Jul 06, 2025 at 03:21:57PM +0200, Frank Wunderlich wrote:
>>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>> 
>>>> Increase the maximum IRQ count to 8 (4 FE + 4 RSS/LRO).
>>> 
>>> Because? Hardware was updated? It was missing before?
>> 
>> There is no RSS support in driver yet,so IRQs were not added to existing DTS yet.
>> 
>
>That's the problem. It's the hardware that you should've described, not the driver.
>
>In short, you should've allowed the interrupts from the get-go, and you wouldn't
>be in this situation now :-)

I have not upstreamed MT7981 or MT7986. I also do not want to say anybody else did this wrong.
I'm happy that MT7986 is working in mainline. It was basicly not taken into account that these IRQs may be needed in future.

The technical documents are often not complete and we get some information step-by-step while testing.
Or it was not seen when documents are too large :) many reasons why it was "forgotten to add".
We use what we get from sdk and docs and try to make it compatible with mainline....no optimal process,but it is like it is.

We are all humans :)

>Again, it's not the driver but the hardware that you're describing.

Frame engine works with 4,but we wanted to do better than mt798[16] and add all known IRQs
for MT7988 and update the older SoCs in next step.

>As long as you are fixing the description of the hardware, even for all three,
>I am personally even fine with breaking the ABI, because the hardware description
>has been wrong for all that time.

As some patches now are applied i can add the missing IRQs for MT798[16] in next round and 
maybe the sram too to not having the binding errors long time.

>Just don't send those as Fixes commits, but next time you upstream something you
>must keep in mind that in bindings/dts you're describing hardware - the driver is
>something that should not drive any decision in what you write in bindings.
>
>We're humans, so stuff like this happens - I'm not saying that you shall not make
>mistakes - but again please, for the next time, please please please keep in mind
>what I just said :-)
>
>Now the options are two:
> - Break the ABI; or
> - Allow 4 or 8 interrupts (not 5, not 6, not 7)
>
>and that - not just on MT7988 but also on 81 and 86 in one go.

I would add dts patches for MT798[16] in next version adding the RSS/LRO IRQs,
interrupt-names and the sram node and set the interrupt minItems to 8 for these.

>Not sure if the second one is feasible, and I'm considering the first option only
>because of that; if the second option can be done, act like I never ever considered
>the first.

Maybe the cleanest way is like i described above? Afaik second is not possible.

My intention is that RSS/LRO is ONLY working with interrupt-names so driver can handle also 
old dtb (skip function if irqs are not found via name).

I hope this is a good way to go...

>Cheers,
>Angelo


regards Frank
Re: [PATCH v8 02/16] dt-bindings: net: mediatek,net: allow up to 8 IRQs
Posted by Krzysztof Kozlowski 2 months, 3 weeks ago
On 07/07/2025 12:43, Frank Wunderlich wrote:
> Hi Angelo,
> 
> Am 7. Juli 2025 12:06:02 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>> Il 07/07/25 09:30, Frank Wunderlich ha scritto:
>>> Am 7. Juli 2025 08:31:11 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>>>> On Sun, Jul 06, 2025 at 03:21:57PM +0200, Frank Wunderlich wrote:
>>>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>>>
>>>>> Increase the maximum IRQ count to 8 (4 FE + 4 RSS/LRO).
>>>>
>>>> Because? Hardware was updated? It was missing before?
>>>
>>> There is no RSS support in driver yet,so IRQs were not added to existing DTS yet.
>>>
>>
>> That's the problem. It's the hardware that you should've described, not the driver.
>>
>> In short, you should've allowed the interrupts from the get-go, and you wouldn't
>> be in this situation now :-)
> 
> I have not upstreamed MT7981 or MT7986. I also do not want to say anybody else did this wrong.
> I'm happy that MT7986 is working in mainline. It was basicly not taken into account that these IRQs may be needed in future.
> 
> The technical documents are often not complete and we get some information step-by-step while testing.
> Or it was not seen when documents are too large :) many reasons why it was "forgotten to add".
> We use what we get from sdk and docs and try to make it compatible with mainline....no optimal process,but it is like it is.


Then explain in the commit msg that hardware description was incomplete
and was missing this and that.

This is the valid reason for doing the change.

Best regards,
Krzysztof