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

[Frank Wunderlich]

Hi,

Thank you for review.

Am 22. Juni 2025 13:10:31 MESZ schrieb Krzysztof Kozlowski <krzk at kernel.org>:
>On Fri, Jun 20, 2025 at 10:35:32AM +0200, Frank Wunderlich wrote:
>> From: Frank Wunderlich <frank-w at 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.

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

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

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

But of course i can limit the reg/ irqs/
 irq-names for each compatible.

>https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127
>
>https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml#L39

I take a look into it,but allowing irq names for all matching compatibles does not require it for them. Or am i wrong here?

>Best regards,
>Krzysztof



regards Frank