[v7] further mt7988 devicetree work

[PATCH v7 02/14] dt-bindings: net: mediatek,net: update for mt7988

Posted by Frank Wunderlich 3 months, 1 week ago

From: Frank Wunderlich <frank-w@public-files.de>

Update binding for mt7988 which has 3 gmac and a sram for dma
operations.

MT7988 has 4 FE IRQs (currently only 2 are used) and 4 IRQs for use
with RSS/LRO later.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
v6:
- split out the interrupt-names into separate patch
- update irq(name) min count to 4
- add sram-property
- drop second reg entry and minitems as there is only 1 item left again

v5:
- fix v4 logmessage and change description a bit describing how i get
  the irq count.
- update binding for 8 irqs with different names (rx,tx => fe0..fe3)
  including the 2 reserved irqs which can be used later
- change rx-ringX to pdmaX to be closer to hardware documentation

v4:
- increase max interrupts to 6 because of adding RSS/LRO interrupts (4)
  and dropping 2 reserved irqs (0+3) around rx+tx
- dropped Robs RB due to this change
- allow interrupt names
- add interrupt-names without reserved IRQs on mt7988
  this requires mtk driver patch:
  https://patchwork.kernel.org/project/netdevbpf/patch/20250616080738.117993-2-linux@fw-web.de/

v2:
- change reg to list of items
---
 Documentation/devicetree/bindings/net/mediatek,net.yaml | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
index 6672db206b38..74a139000f60 100644
--- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
+++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
@@ -28,7 +28,8 @@ properties:
       - ralink,rt5350-eth
 
   reg:
-    maxItems: 1
+    items:
+      - description: Register for accessing the MACs.
 
   clocks:
     minItems: 2
@@ -66,6 +67,10 @@ properties:
       - const: gmac
       - const: ppe
 
+  sram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to mmio SRAM
+
   mediatek,ethsys:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
@@ -418,7 +423,7 @@ allOf:
             - const: xgp3
 
 patternProperties:
-  "^mac@[0-1]$":
+  "^mac@[0-2]$":
     type: object
     unevaluatedProperties: false
     allOf:
-- 
2.43.0

Re: [PATCH v7 02/14] dt-bindings: net: mediatek,net: update for mt7988

Posted by Krzysztof Kozlowski 3 months, 1 week ago

On Sat, Jun 28, 2025 at 06:54:37PM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Update binding for mt7988 which has 3 gmac and a sram for dma
> operations.

I asked why you are updating. You claim you update because it has 3
GMAC... but that's irrelevant, because it is easy to answer with: it did
not have 3 GMAC before?

So same question: Provide real reason why you are making updates. That's
why you have commit msg.


> 
> MT7988 has 4 FE IRQs (currently only 2 are used) and 4 IRQs for use

mt7988 or MT7988? gmac or GMAC? SRAM or SRAM? and so on... it is not
easy to read and understand your commit msgs.

> with RSS/LRO later.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> v6:
> - split out the interrupt-names into separate patch
> - update irq(name) min count to 4
> - add sram-property
> - drop second reg entry and minitems as there is only 1 item left again
> 
> v5:
> - fix v4 logmessage and change description a bit describing how i get
>   the irq count.
> - update binding for 8 irqs with different names (rx,tx => fe0..fe3)
>   including the 2 reserved irqs which can be used later
> - change rx-ringX to pdmaX to be closer to hardware documentation
> 
> v4:
> - increase max interrupts to 6 because of adding RSS/LRO interrupts (4)
>   and dropping 2 reserved irqs (0+3) around rx+tx
> - dropped Robs RB due to this change
> - allow interrupt names
> - add interrupt-names without reserved IRQs on mt7988
>   this requires mtk driver patch:
>   https://patchwork.kernel.org/project/netdevbpf/patch/20250616080738.117993-2-linux@fw-web.de/
> 
> v2:
> - change reg to list of items
> ---
>  Documentation/devicetree/bindings/net/mediatek,net.yaml | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
> index 6672db206b38..74a139000f60 100644
> --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
> +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
> @@ -28,7 +28,8 @@ properties:
>        - ralink,rt5350-eth
>  
>    reg:
> -    maxItems: 1
> +    items:
> +      - description: Register for accessing the MACs.

Why making this change? It's redundant and nothing in commit msg
explains that.

Best regards,
Krzysztof

Re: [PATCH v7 02/14] dt-bindings: net: mediatek,net: update for mt7988

Posted by Frank Wunderlich 3 months, 1 week ago

Am 1. Juli 2025 08:41:42 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>On Sat, Jun 28, 2025 at 06:54:37PM +0200, Frank Wunderlich wrote:
>> From: Frank Wunderlich <frank-w@public-files.de>
>> 
>> Update binding for mt7988 which has 3 gmac and a sram for dma
>> operations.
>
>I asked why you are updating. You claim you update because it has 3
>GMAC... but that's irrelevant, because it is easy to answer with: it did
>not have 3 GMAC before?
>
>So same question: Provide real reason why you are making updates. That's
>why you have commit msg.

MT7988 had always 3 gmac,but no dts with ethernet
node till now.
As i try to upstream the dts,i fell over this.

Imho changing the regex for the mac subnodes was
simply forgotten to be updated on initial mt7988
support patch.

I try to rephrase it like this:

Binding was not aware for 3 MAC subnodes because
previous mediatek SoC had only 2. Change this to allow
3 GMAC in mt7988 devicetree.
>
>> 
>> MT7988 has 4 FE IRQs (currently only 2 are used) and 4 IRQs for use
>
>mt7988 or MT7988? gmac or GMAC? SRAM or SRAM? and so on... it is not
>easy to read and understand your commit msgs.

Ok,i always write those names in uppercase.

>> with RSS/LRO later.
>> 
>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>> ---
>> v6:
>> - split out the interrupt-names into separate patch
>> - update irq(name) min count to 4
>> - add sram-property
>> - drop second reg entry and minitems as there is only 1 item left again
>> 
>> v5:
>> - fix v4 logmessage and change description a bit describing how i get
>>   the irq count.
>> - update binding for 8 irqs with different names (rx,tx => fe0..fe3)
>>   including the 2 reserved irqs which can be used later
>> - change rx-ringX to pdmaX to be closer to hardware documentation
>> 
>> v4:
>> - increase max interrupts to 6 because of adding RSS/LRO interrupts (4)
>>   and dropping 2 reserved irqs (0+3) around rx+tx
>> - dropped Robs RB due to this change
>> - allow interrupt names
>> - add interrupt-names without reserved IRQs on mt7988
>>   this requires mtk driver patch:
>>   https://patchwork.kernel.org/project/netdevbpf/patch/20250616080738.117993-2-linux@fw-web.de/
>> 
>> v2:
>> - change reg to list of items
>> ---
>>  Documentation/devicetree/bindings/net/mediatek,net.yaml | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
>> index 6672db206b38..74a139000f60 100644
>> --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
>> +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
>> @@ -28,7 +28,8 @@ properties:
>>        - ralink,rt5350-eth
>>  
>>    reg:
>> -    maxItems: 1
>> +    items:
>> +      - description: Register for accessing the MACs.
>
>Why making this change? It's redundant and nothing in commit msg
>explains that.

I was instructed (where we had 2 regs in previous
Version) to name the regs...but as we have one reg
again,i can drop this change. Thought a description 
is better than a count.

>Best regards,
>Krzysztof
>


regards Frank

Re: [PATCH v7 02/14] dt-bindings: net: mediatek,net: update for mt7988

Posted by Krzysztof Kozlowski 3 months, 1 week ago

On 01/07/2025 12:33, Frank Wunderlich wrote:
> Am 1. Juli 2025 08:41:42 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>> On Sat, Jun 28, 2025 at 06:54:37PM +0200, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> Update binding for mt7988 which has 3 gmac and a sram for dma
>>> operations.
>>
>> I asked why you are updating. You claim you update because it has 3
>> GMAC... but that's irrelevant, because it is easy to answer with: it did
>> not have 3 GMAC before?
>>
>> So same question: Provide real reason why you are making updates. That's
>> why you have commit msg.
> 
> MT7988 had always 3 gmac,but no dts with ethernet
> node till now.
> As i try to upstream the dts,i fell over this.

What does it mean? Are you adding new device or not? Nothing explains
that something was missing.

> 
> Imho changing the regex for the mac subnodes was
> simply forgotten to be updated on initial mt7988
> support patch.

Fix
your
wrapping because
it is
difficult
to follow
such
style.

> 
> I try to rephrase it like this:
> 
> Binding was not aware for 3 MAC subnodes because
> previous mediatek SoC had only 2. Change this to allow
> 3 GMAC in mt7988 devicetree.

So a fix for existing? Than add Fixes tag, describe the issue and fix
ONLY that issue.

Best regards,
Krzysztof

Aw: Re: [PATCH v7 02/14] dt-bindings: net: mediatek,net: update for mt7988

Posted by frank-w@public-files.de 3 months, 1 week ago

> Gesendet: Mittwoch, 2. Juli 2025 um 08:29
> Von: "Krzysztof Kozlowski" <krzk@kernel.org>
> Betreff: Re: [PATCH v7 02/14] dt-bindings: net: mediatek,net: update for mt7988
>
> On 01/07/2025 12:33, Frank Wunderlich wrote:
> > Am 1. Juli 2025 08:41:42 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> >> On Sat, Jun 28, 2025 at 06:54:37PM +0200, Frank Wunderlich wrote:
> >>> From: Frank Wunderlich <frank-w@public-files.de>
> >>>
> >>> Update binding for mt7988 which has 3 gmac and a sram for dma
> >>> operations.
> >>
> >> I asked why you are updating. You claim you update because it has 3
> >> GMAC... but that's irrelevant, because it is easy to answer with: it did
> >> not have 3 GMAC before?
> >>
> >> So same question: Provide real reason why you are making updates. That's
> >> why you have commit msg.
> > 
> > MT7988 had always 3 gmac,but no dts with ethernet
> > node till now.
> > As i try to upstream the dts,i fell over this.
> 
> What does it mean? Are you adding new device or not? Nothing explains
> that something was missing.

The binding already exists, but was incomplete. It was added while changing ethernet driver but was not used
because i'm the first person adding mt7988 Ethernet node to devicetree in this series.

> > 
> > Imho changing the regex for the mac subnodes was
> > simply forgotten to be updated on initial mt7988
> > support patch.
> 
> Fix
> your
> wrapping because
> it is
> difficult
> to follow
> such
> style.

i understand that it is not the best, but i have to manually wrap lines because neither my webmail nor
my Android Mail-App (K9Mail) supports automatic wrapping (created a feature-request some years ago which
got rejected). I try to wrap it as good as possible, but still manually (on phone it is not that easy).

> > 
> > I try to rephrase it like this:
> > 
> > Binding was not aware for 3 MAC subnodes because
> > previous mediatek SoC had only 2. Change this to allow
> > 3 GMAC in mt7988 devicetree.
> 
> So a fix for existing? Than add Fixes tag, describe the issue and fix
> ONLY that issue.

Yes, binding for mt7988 already exists withing the mediatek,net binding, but the pattern for mac subnodes
was not updated while adding. So i had to do it before adding the ethernet node to dts in same series.

But yes, i can separate this change again and add Fixes Tag. So just the sram-Property is added in this patch
and i repharse it like this.

> Best regards,
> Krzysztof

regards Frank