mbox series

[net-next,v2,0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5

Message ID 20190821144547.15113-1-opensource@vdorst.com
Headers show
Series net: dsa: mt7530: Convert to PHYLINK and add support for port 5 | expand

Message

René van Dorst Aug. 21, 2019, 2:45 p.m. UTC
1. net: dsa: mt7530: Convert to PHYLINK API
   This patch converts mt7530 to PHYLINK API.
2. dt-bindings: net: dsa: mt7530: Add support for port 5
3. net: dsa: mt7530: Add support for port 5
   These 2 patches adding support for port 5 of the switch.

v1->v2:
 * Mostly phylink improvements after review.
rfc -> v1:
 * Mostly phylink improvements after review.
 * Drop phy isolation patches. Adds no value for now.
René van Dorst (3):
  net: dsa: mt7530: Convert to PHYLINK API
  dt-bindings: net: dsa: mt7530: Add support for port 5
  net: dsa: mt7530: Add support for port 5

 .../devicetree/bindings/net/dsa/mt7530.txt    | 218 ++++++++++
 drivers/net/dsa/mt7530.c                      | 371 +++++++++++++++---
 drivers/net/dsa/mt7530.h                      |  61 ++-
 3 files changed, 577 insertions(+), 73 deletions(-)

Comments

Andrew Lunn Aug. 21, 2019, 9:05 p.m. UTC | #1
On Wed, Aug 21, 2019 at 04:45:44PM +0200, René van Dorst wrote:
> 1. net: dsa: mt7530: Convert to PHYLINK API
>    This patch converts mt7530 to PHYLINK API.
> 2. dt-bindings: net: dsa: mt7530: Add support for port 5
> 3. net: dsa: mt7530: Add support for port 5
>    These 2 patches adding support for port 5 of the switch.
> 
> v1->v2:
>  * Mostly phylink improvements after review.

Hi René

You are addressing comments mostly from Russell King. It would of been
good to Cc: him on the patchset.

Andrew
Frank Wunderlich Aug. 22, 2019, 3:44 p.m. UTC | #2
Hi,

tested on BPI-R2 (mt7623) with 2 Problems (already reported to Rene, just to inform everyone)...maybe anybody has an idea

- linux-next (i know it's not part of the series, but a pitfall on testing other devices) seems to break power-regulator somewhere here:

priv->core_pwr = devm_regulator_get(&mdiodev->dev, "core"); returns 517

#define EPROBE_DEFER517/* Driver requests probe retry */

https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L1726

without linux-next switch came up including dsa-ports

- RX-traffic (run iperf3 -c x.x.x.x -R) is only 780 Mbits/sec (TX=940 Mbits/sec), same measure with 5.3-rc4 gives 940 MBit/s with same devices,
maybe caused by changes for mt76x8?

regards Frank
Frank Wunderlich Aug. 22, 2019, 4:26 p.m. UTC | #3
tested now also on bpi-r64 (mt7622) v0.1 (rtl8367 switch), without linux-next to avoid power-regulator-problems like on bpi-r2

dmesg without warnings/errors caused by this patches
link came up as desired
iperf3 looks good: 943 Mbits/sec in both directions and no other issues

so it is currently only the rx-throughput-problem on mt7623/bpi-r2

regards Frank
David Miller Aug. 22, 2019, 11:20 p.m. UTC | #4
From: René van Dorst <opensource@vdorst.com>
Date: Wed, 21 Aug 2019 16:45:44 +0200

> 1. net: dsa: mt7530: Convert to PHYLINK API
>    This patch converts mt7530 to PHYLINK API.
> 2. dt-bindings: net: dsa: mt7530: Add support for port 5
> 3. net: dsa: mt7530: Add support for port 5
>    These 2 patches adding support for port 5 of the switch.
> 
> v1->v2:
>  * Mostly phylink improvements after review.
> rfc -> v1:
>  * Mostly phylink improvements after review.
>  * Drop phy isolation patches. Adds no value for now.

This definitely needs some review before I'll apply it.

Thanks.
Andrew Lunn Aug. 23, 2019, 1:09 a.m. UTC | #5
On Thu, Aug 22, 2019 at 04:20:47PM -0700, David Miller wrote:
> From: René van Dorst <opensource@vdorst.com>
> Date: Wed, 21 Aug 2019 16:45:44 +0200
> 
> > 1. net: dsa: mt7530: Convert to PHYLINK API
> >    This patch converts mt7530 to PHYLINK API.
> > 2. dt-bindings: net: dsa: mt7530: Add support for port 5
> > 3. net: dsa: mt7530: Add support for port 5
> >    These 2 patches adding support for port 5 of the switch.
> > 
> > v1->v2:
> >  * Mostly phylink improvements after review.
> > rfc -> v1:
> >  * Mostly phylink improvements after review.
> >  * Drop phy isolation patches. Adds no value for now.
> 
> This definitely needs some review before I'll apply it.

That would be Russell.

We should try to improve MAINTAINER so that Russell King gets picked
by the get_maintainer script.

   Andrew
David Miller Aug. 24, 2019, 9:18 p.m. UTC | #6
From: Andrew Lunn <andrew@lunn.ch>
Date: Fri, 23 Aug 2019 03:09:28 +0200

> That would be Russell.
> 
> We should try to improve MAINTAINER so that Russell King gets picked
> by the get_maintainer script.

Shoule he be added to the mt7530 entry?
Andrew Lunn Aug. 24, 2019, 10:15 p.m. UTC | #7
65;5402;1cOn Sat, Aug 24, 2019 at 02:18:03PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Fri, 23 Aug 2019 03:09:28 +0200
> 
> > That would be Russell.
> > 
> > We should try to improve MAINTAINER so that Russell King gets picked
> > by the get_maintainer script.
> 
> Shoule he be added to the mt7530 entry?

Hi David

No. I think we need a phylink entry. And then make use of the K: line
format to list keywords. I hope that even though changes like this
don't touch any files listed as being part of phylink, they will match
the keyword and pickup Russell.

I need to do some testing and see if this actually works.

  Andrew
Russell King (Oracle) Aug. 24, 2019, 10:18 p.m. UTC | #8
On Sat, Aug 24, 2019 at 02:18:03PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Fri, 23 Aug 2019 03:09:28 +0200
> 
> > That would be Russell.
> > 
> > We should try to improve MAINTAINER so that Russell King gets picked
> > by the get_maintainer script.
> 
> Shoule he be added to the mt7530 entry?

Probably some way to make MAINTAINERS pick up on phylink-containing
patches.  Something like:

K:	phylink

maybe?
Russell King (Oracle) Aug. 24, 2019, 10:29 p.m. UTC | #9
On Wed, Aug 21, 2019 at 04:45:44PM +0200, René van Dorst wrote:
> 1. net: dsa: mt7530: Convert to PHYLINK API
>    This patch converts mt7530 to PHYLINK API.
> 2. dt-bindings: net: dsa: mt7530: Add support for port 5
> 3. net: dsa: mt7530: Add support for port 5
>    These 2 patches adding support for port 5 of the switch.
> 
> v1->v2:
>  * Mostly phylink improvements after review.
> rfc -> v1:
>  * Mostly phylink improvements after review.
>  * Drop phy isolation patches. Adds no value for now.
> René van Dorst (3):
>   net: dsa: mt7530: Convert to PHYLINK API
>   dt-bindings: net: dsa: mt7530: Add support for port 5
>   net: dsa: mt7530: Add support for port 5
> 
>  .../devicetree/bindings/net/dsa/mt7530.txt    | 218 ++++++++++
>  drivers/net/dsa/mt7530.c                      | 371 +++++++++++++++---
>  drivers/net/dsa/mt7530.h                      |  61 ++-
>  3 files changed, 577 insertions(+), 73 deletions(-)

Having looked through this set of patches, I don't see anything
from the phylink point of view that concerns me.  So, for the
series from the phylink perspective:

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks.

I did notice a dev_info() in patch 3 that you may like to consider
whether they should be printed at info level or debug level.  You
may keep my ack on the patch when fixing that.

I haven't considered whether the patch passes davem's style
requirements for networking code; what I spotted did look like
the declarations were upside-down christmas tree.
Russell King (Oracle) Aug. 24, 2019, 10:31 p.m. UTC | #10
On Sun, Aug 25, 2019 at 12:15:19AM +0200, Andrew Lunn wrote:
> 65;5402;1cOn Sat, Aug 24, 2019 at 02:18:03PM -0700, David Miller wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > Date: Fri, 23 Aug 2019 03:09:28 +0200
> > 
> > > That would be Russell.
> > > 
> > > We should try to improve MAINTAINER so that Russell King gets picked
> > > by the get_maintainer script.
> > 
> > Shoule he be added to the mt7530 entry?
> 
> Hi David
> 
> No. I think we need a phylink entry. And then make use of the K: line
> format to list keywords. I hope that even though changes like this
> don't touch any files listed as being part of phylink, they will match
> the keyword and pickup Russell.

Note that phylink itself is already covered by

"SFF/SFP/SFP+ MODULE SUPPORT"

but doesn't pick up on keywords.
René van Dorst Aug. 25, 2019, 1:15 p.m. UTC | #11
Hi Russell,

Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:

> On Wed, Aug 21, 2019 at 04:45:44PM +0200, René van Dorst wrote:
>> 1. net: dsa: mt7530: Convert to PHYLINK API
>>    This patch converts mt7530 to PHYLINK API.
>> 2. dt-bindings: net: dsa: mt7530: Add support for port 5
>> 3. net: dsa: mt7530: Add support for port 5
>>    These 2 patches adding support for port 5 of the switch.
>>
>> v1->v2:
>>  * Mostly phylink improvements after review.
>> rfc -> v1:
>>  * Mostly phylink improvements after review.
>>  * Drop phy isolation patches. Adds no value for now.
>> René van Dorst (3):
>>   net: dsa: mt7530: Convert to PHYLINK API
>>   dt-bindings: net: dsa: mt7530: Add support for port 5
>>   net: dsa: mt7530: Add support for port 5
>>
>>  .../devicetree/bindings/net/dsa/mt7530.txt    | 218 ++++++++++
>>  drivers/net/dsa/mt7530.c                      | 371 +++++++++++++++---
>>  drivers/net/dsa/mt7530.h                      |  61 ++-
>>  3 files changed, 577 insertions(+), 73 deletions(-)
>
> Having looked through this set of patches, I don't see anything
> from the phylink point of view that concerns me.  So, for the
> series from the phylink perspective:
>
> Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks and thanks for reviewing.

Greats,

René

>
> Thanks.
>
> I did notice a dev_info() in patch 3 that you may like to consider
> whether they should be printed at info level or debug level.  You
> may keep my ack on the patch when fixing that.
>
> I haven't considered whether the patch passes davem's style
> requirements for networking code; what I spotted did look like
> the declarations were upside-down christmas tree.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up