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

[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 :-)

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