Re: [PATCH v5 01/13] dt-bindings: net: mediatek,net: update for mt7988

[Krzysztof Kozlowski <krzk@kernel.org>]

On 22/06/2025 13:44, Frank Wunderlich wrote:
> Hi,
> 
> Thank you for review.
> 
> Am 22. Juni 2025 13:10:31 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>> On Fri, Jun 20, 2025 at 10:35:32AM +0200, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> Update binding for mt7988 which has 3 gmac and 2 reg items.
>>
>> Why?
> 
> I guess this is for reg? Socs toll mt7986 afair
>  get the SRAM register by offset to the MAC
>  register.
> On mt7988 we started defining it directly.

This should be explained in commit msg. Why are you doing the changes...

> 
>>> MT7988 has 4 FE IRQs (currently only 2 are used) and the 4 IRQs for
>>> use with RSS/LRO later.
>>>
>>> Add interrupt-names to make them accessible by name.
>>>
> ...
>>>    reg:
>>> -    maxItems: 1
>>> +    items:
>>> +      - description: Register for accessing the MACs.
>>> +      - description: SoC internal SRAM used for DMA operations.
>>
>> SRAM like mmio-sram?
> 
> Not sure,but as far as i understand the driver
>  the sram is used to handle tx packets directly
>  on the soc (less dram operations).
> 
> As mt7988 is the first 10Gbit/s capable SoC
>  there are some changes. But do we really need 
>  a new binding? We also thing abour adding
>  RSS/LRO to mt7986 too,so we come into
>  similar situation regarding the Interrupts/  
>  -names.

If it is mmio-sram, then it is definitely not reg property.

Anyway
wrap emails
according to list
discussion rules.

> 
>>> +    minItems: 1
>>>  
>>>    clocks:
>>>      minItems: 2
>>> @@ -40,7 +43,11 @@ properties:
>>>  
>>>    interrupts:
>>>      minItems: 1
>>> -    maxItems: 4
>>> +    maxItems: 8
>>> +
>>> +  interrupt-names:
>>> +    minItems: 1
>>> +    maxItems: 8
>>
>> So now all variants get unspecified names? You need to define it. Or
>> just drop.
> 
> Most socs using the Fe-irqs like mt7988,some
>  specify only 3 and 2 soc (mt762[18]) have only
>  1 shared irq. But existing dts not yet using the
>  irq-names.
> Thats why i leave it undefined here and
>  defining it only for mt7988 below. But leaving it
>  open to add irq names to other socs like filogic
>  socs (mt798x) where we are considering
>  adding rss/lro support too.
> 

I explained this is wrong. Your binding must be specific, not flexible.

>>>  
>>>    power-domains:
>>>      maxItems: 1
>>> @@ -348,7 +355,19 @@ allOf:
>>>      then:
>>>        properties:
>>>          interrupts:
>>> -          minItems: 4
>>> +          minItems: 2
>>
>> Why? Didn't you say it has 4?
> 
> Sorry missed to change it after adding the 2
>  reserved fe irqs back again (i tried adding only used irqs - rx+tx,but got info that all irqs can be used - for future functions - so added all available).
> 
>>> +
>>> +        interrupt-names:
>>> +          minItems: 2
>>> +          items:
>>> +            - const: fe0
>>> +            - const: fe1
>>> +            - const: fe2
>>> +            - const: fe3
>>> +            - const: pdma0
>>> +            - const: pdma1
>>> +            - const: pdma2
>>> +            - const: pdma3
>>>  
>>>          clocks:
>>>            minItems: 24
>>> @@ -381,8 +400,11 @@ allOf:
>>>              - const: xgp2
>>>              - const: xgp3
>>>  
>>> +        reg:
>>> +          minItems: 2
>>
>>
>> And all else? Why they got 2 reg and 8 interrupts now? All variants are
>> now affected/changed. We have been here: you need to write specific
>> bindings.
> 
> Mt7988 is more powerful and we wanted to add
>  all irqs available to have less problems when
>  adding rss support later. E.g. mt7986 also have
>  the pdma irqs,but they are not part of
>  binding+dts yet. Thats 1 reason why
>  introducing irq-names now. And this block is
>  for mt7988 only...the other still have a regcount of 1 (min-items).

This explains me nothing. Why do you change other hardware? Why when
doing something for MT7988 you also state that other SoCs have different
number of interrupts?



Best regards,
Krzysztof