Hello!

This patch series simplifies the code, improves the logic of the switch
hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
MT7988 SoC switches, and introduces the preferring local CPU port
operation.

There's also a patch for fixing the port capabilities of the switch on the
MT7988 SoC.

I have done a bidirectional speed test using iperf3 on all ports of the
MT7530 and MT7531 switches with this patch series applied. I have tested
every possible configuration on the MCM and standalone MT7530 and MT7531
switch. I'll let the name of the dtb files speak for themselves.

MT7621 Unielec:

only-gmac0-mt7621-unielec-u7621-06-16m.dtb
rgmii-only-gmac0-mt7621-unielec-u7621-06-16m.dtb
only-gmac1-mt7621-unielec-u7621-06-16m.dtb
gmac0-and-gmac1-mt7621-unielec-u7621-06-16m.dtb
phy0-muxing-mt7621-unielec-u7621-06-16m.dtb
phy4-muxing-mt7621-unielec-u7621-06-16m.dtb
port5-as-user-mt7621-unielec-u7621-06-16m.dtb

tftpboot 0x80008000 mips-uzImage-next-20230519.bin; tftpboot 0x83000000 mips-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000

MT7622 Bananapi:

only-gmac0-mt7622-bananapi-bpi-r64.dtb
gmac0-and-gmac1-mt7622-bananapi-bpi-r64.dtb
port5-as-user-mt7622-bananapi-bpi-r64.dtb

tftpboot 0x40000000 arm64-Image-next-20230519; tftpboot 0x45000000 arm64-rootfs.cpio.uboot; tftpboot 0x4a000000 $dtb; booti 0x40000000 0x45000000 0x4a000000

MT7623 Bananapi:

only-gmac0-mt7623n-bananapi-bpi-r2.dtb
rgmii-only-gmac0-mt7623n-bananapi-bpi-r2.dtb
only-gmac1-mt7623n-bananapi-bpi-r2.dtb
gmac0-and-gmac1-mt7623n-bananapi-bpi-r2.dtb
phy0-muxing-mt7623n-bananapi-bpi-r2.dtb
phy4-muxing-mt7623n-bananapi-bpi-r2.dtb
port5-as-user-mt7623n-bananapi-bpi-r2.dtb

tftpboot 0x80008000 arm-zImage-next-20230519; tftpboot 0x83000000 arm-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootz 0x80008000 0x83000000 0x83f00000

Arınç

Arınç ÜNAL (30):
  net: dsa: mt7530: add missing @p5_interface to mt7530_priv description
  net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel
  net: dsa: mt7530: properly support MT7531AE and MT7531BE
  net: dsa: mt7530: improve comments regarding port 5 and 6
  net: dsa: mt7530: read XTAL value from correct register
  net: dsa: mt7530: improve code path for setting up port 5
  net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled
  net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured
  net: dsa: mt7530: empty default case on mt7530_setup_port5()
  net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
  net: dsa: mt7530: remove pad_setup function pointer
  net: dsa: mt7530: move XTAL check to mt7530_setup()
  net: dsa: mt7530: move enabling port 6 to mt7530_setup_port6()
  net: dsa: mt7530: switch to if/else statements on mt7530_setup_port6()
  net: dsa: mt7530: set TRGMII RD TAP if trgmii is being used
  net: dsa: mt7530: move lowering port 5 RGMII driving to mt7530_setup()
  net: dsa: mt7530: fix port capabilities for MT7988
  net: dsa: mt7530: remove .mac_port_config for MT7988 and make it optional
  net: dsa: mt7530: set interrupt register only for MT7530
  net: dsa: mt7530: properly reset MT7531 switch
  net: dsa: mt7530: get rid of useless error returns on phylink code path
  net: dsa: mt7530: rename p5_intf_sel and use only for MT7530 switch
  net: dsa: mt7530: run mt7530_pll_setup() only with 40 MHz XTAL
  net: dsa: mt7530: rename MT7530_MFC to MT753X_MFC
  net: dsa: mt7530: properly set MT7531_CPU_PMAP
  net: dsa: mt7530: properly set MT7530_CPU_PORT
  net: dsa: mt7530: introduce BPDU trapping for MT7530 switch
  net: dsa: mt7530: introduce LLDP frame trapping
  net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
  MAINTAINERS: add me as maintainer of MEDIATEK SWITCH DRIVER

 MAINTAINERS                   |   5 +-
 drivers/net/dsa/mt7530-mdio.c |   7 +-
 drivers/net/dsa/mt7530.c      | 470 ++++++++++++++++---------------------
 drivers/net/dsa/mt7530.h      | 107 +++++----
 include/net/dsa.h             |   8 +
 net/dsa/dsa.c                 |  24 +-
 6 files changed, 289 insertions(+), 332 deletions(-)

On Mon, May 22, 2023 at 03:15:02PM +0300, arinc9.unal@gmail.com wrote:
> Hello!
> 
> This patch series simplifies the code, improves the logic of the switch
> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
> MT7988 SoC switches, and introduces the preferring local CPU port
> operation.

Hi Arınç

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

says:

  Avoid sending series longer than 15 patches. Larger series takes
  longer to review as reviewers will defer looking at it until they
  find a large chunk of time. A small series can be reviewed in a
  short time, so Maintainers just do it. As a result, a sequence of
  smaller series gets merged quicker and with better review coverage.

Given you description above, it sounds like this could easily be split
into smaller patch series.

     Andrew

On 22.05.2023 15:25, Andrew Lunn wrote:
> On Mon, May 22, 2023 at 03:15:02PM +0300, arinc9.unal@gmail.com wrote:
>> Hello!
>>
>> This patch series simplifies the code, improves the logic of the switch
>> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
>> MT7988 SoC switches, and introduces the preferring local CPU port
>> operation.
> 
> Hi Arınç
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
> says:
> 
>    Avoid sending series longer than 15 patches. Larger series takes
>    longer to review as reviewers will defer looking at it until they
>    find a large chunk of time. A small series can be reviewed in a
>    short time, so Maintainers just do it. As a result, a sequence of
>    smaller series gets merged quicker and with better review coverage.
> 
> Given you description above, it sounds like this could easily be split
> into smaller patch series.

Later patches require the prior ones to apply properly. I can submit the
first 15 patches, then the remaining once the first submission is applied.
Would that suit you?

Arınç

On Mon, May 22, 2023 at 04:37:28PM +0300, Arınç ÜNAL wrote:
> Later patches require the prior ones to apply properly. I can submit the
> first 15 patches, then the remaining once the first submission is applied.
> Would that suit you?

Please wait for some feedback on (a subset of) what you've submitted thus far.
Thanks.

On Mon, May 22, 2023 at 03:15:02PM +0300, arinc9.unal@gmail.com wrote:
> Hello!
> 
> This patch series simplifies the code, improves the logic of the switch
> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
> MT7988 SoC switches, and introduces the preferring local CPU port
> operation.
> 
> There's also a patch for fixing the port capabilities of the switch on the
> MT7988 SoC.
> 
> I have done a bidirectional speed test using iperf3 on all ports of the
> MT7530 and MT7531 switches with this patch series applied. I have tested
> every possible configuration on the MCM and standalone MT7530 and MT7531
> switch. I'll let the name of the dtb files speak for themselves.

As general feedback for the series, please sort the fixes to be first,
to have as few dependencies as possible, and submit then to the 'net' tree,
leaving cleanup at the end.

The 05/22/2023 15:15, arinc9.unal@gmail.com wrote:

Hi,

> 
> Hello!
> 
> This patch series simplifies the code, improves the logic of the switch
> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
> MT7988 SoC switches, and introduces the preferring local CPU port
> operation.
> 
> There's also a patch for fixing the port capabilities of the switch on the
> MT7988 SoC.
> 

I have noticed that in many patches of the series you have:
Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Where you also have:
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

I think you can drop Tested-by as the SoB will imply that. I think you
got a similar comment some time ago to a different patch series.


-- 
/Horatiu

On Mon, May 22, 2023 at 04:09:17PM +0200, Horatiu Vultur wrote:
> The 05/22/2023 15:15, arinc9.unal@gmail.com wrote:
> 
> Hi,
> 
> > 
> > Hello!
> > 
> > This patch series simplifies the code, improves the logic of the switch
> > hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
> > MT7988 SoC switches, and introduces the preferring local CPU port
> > operation.
> > 
> > There's also a patch for fixing the port capabilities of the switch on the
> > MT7988 SoC.
> > 
> 
> I have noticed that in many patches of the series you have:
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Where you also have:
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> I think you can drop Tested-by as the SoB will imply that. I think you
> got a similar comment some time ago to a different patch series.

Signed-off-by in no way implies a tested-by. Signed-off-by has a very
distinct definition that is in submitting-patches.rst.

Clearly, if one is working on infrastructure where there are numerous
drivers involved, one probably doesn't have all the hardware, and one
may have to send patches that have only been build tested, but never
tested against real hardware.

While we may attempt to elicit testing, most of the time this seems
to be a waste of time and effort - or at least that's my experience.
Even if you Cc people who have recently been active with hardware,
that is no guarantee that there will be any reaction.

That has got to the point now where I just don't bother trying to
elicit help from others to test driver changes. If people want to
test, they need to do so when they see a patch on the mailing list,
preferably before it gets applied. If not, and if it breaks something,
then we'll have to generate a patch to fix the breakage.

So no, please stop thinking that SoB implies that the patch has been
tested.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On Mon, 22 May 2023 19:13:31 +0100 Russell King (Oracle) wrote:
> > I have noticed that in many patches of the series you have:
> > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > 
> > Where you also have:
> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > 
> > I think you can drop Tested-by as the SoB will imply that. I think you
> > got a similar comment some time ago to a different patch series.  
> 
> Signed-off-by in no way implies a tested-by. Signed-off-by has a very
> distinct definition that is in submitting-patches.rst.
> 
> Clearly, if one is working on infrastructure where there are numerous
> drivers involved, one probably doesn't have all the hardware, and one
> may have to send patches that have only been build tested, but never
> tested against real hardware.
> 
> While we may attempt to elicit testing, most of the time this seems
> to be a waste of time and effort - or at least that's my experience.
> Even if you Cc people who have recently been active with hardware,
> that is no guarantee that there will be any reaction.
> 
> That has got to the point now where I just don't bother trying to
> elicit help from others to test driver changes. If people want to
> test, they need to do so when they see a patch on the mailing list,
> preferably before it gets applied. If not, and if it breaks something,
> then we'll have to generate a patch to fix the breakage.
> 
> So no, please stop thinking that SoB implies that the patch has been
> tested.

Dunno, I had the same reaction as Horatiu. Adding "Compile tested only"
in the commit messages of patches which author wasn't able to test seems
more natural than assuming that nothing is tested by default.

It's not a hard requirement, e.g. seems fairly common sense that cross-
-driver work comes with limited testing coverage. But for someone
working on a single driver, assuming not tested by default, again, 
to me - feels odd.

On 22/05/2023 17:09, Horatiu Vultur wrote:
> The 05/22/2023 15:15, arinc9.unal@gmail.com wrote:
> 
> Hi,
> 
>>
>> Hello!
>>
>> This patch series simplifies the code, improves the logic of the switch
>> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
>> MT7988 SoC switches, and introduces the preferring local CPU port
>> operation.
>>
>> There's also a patch for fixing the port capabilities of the switch on the
>> MT7988 SoC.
>>
> 
> I have noticed that in many patches of the series you have:
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Where you also have:
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> I think you can drop Tested-by as the SoB will imply that. I think you
> got a similar comment some time ago to a different patch series.

Yes, that was for the net, therefore the stable tree. The very first 
rule for the patches going to the stable tree is that the patch must be 
tested.

I don't see a clear indication of the patches submitted to net-next 
being tested on the relevant hardware by the author. Therefore, I 
explicitly put my tag to state it. Let me know if you don't agree with this.

Arınç