[net] net: dsa: mt7530: permit port 5 to work without port 6 on MT7621 SoC

  The MT7530 switch from the MT7621 SoC has 2 ports which can be set up as
internal: port 5 and 6. Arınç reports that the GMAC1 attached to port 5
receives corrupted frames, unless port 6 (attached to GMAC0) has been
brought up by the driver. This is true regardless of whether port 5 is
used as a user port or as a CPU port (carrying DSA tags).

Offline debugging (blind for me) which began in the linked thread showed
experimentally that the configuration done by the driver for port 6
contains a step which is needed by port 5 as well - the write to
CORE_GSWPLL_GRP2 (note that I've no idea as to what it does, apart from
the comment "Set core clock into 500Mhz"). Prints put by Arınç show that
the reset value of CORE_GSWPLL_GRP2 is RG_GSWPLL_POSDIV_500M(1) |
RG_GSWPLL_FBKDIV_500M(40) (0x128), both on the MCM MT7530 from the
MT7621 SoC, as well as on the standalone MT7530 from MT7623NI Bananapi
BPI-R2. Apparently, port 5 on the standalone MT7530 can work under both
values of the register, while on the MT7621 SoC it cannot.

The call path that triggers the register write is:

mt753x_phylink_mac_config() for port 6
-> mt753x_pad_setup()
   -> mt7530_pad_clk_setup()

so this fully explains the behavior noticed by Arınç, that bringing port
6 up is necessary.

The simplest fix for the problem is to extract the register writes which
are needed for both port 5 and 6 into a common mt7530_pll_setup()
function, which is called at mt7530_setup() time, immediately after
switch reset. We can argue that this mirrors the code layout introduced
in mt7531_setup() by commit 42bc4fafe359 ("net: mt7531: only do PLL once
after the reset"), in that the PLL setup has the exact same positioning,
and further work to consolidate the separate setup() functions is not
hindered.

Testing confirms that:

- the slight reordering of writes to MT7530_P6ECR and to
  CORE_GSWPLL_GRP1 / CORE_GSWPLL_GRP2 introduced by this change does not
  appear to cause problems for the operation of port 6 on MT7621 and on
  MT7623 (where port 5 also always worked)

- packets sent through port 5 are not corrupted anymore, regardless of
  whether port 6 is enabled by phylink or not (or even present in the
  device tree)

My algorithm for determining the Fixes: tag is as follows. Testing shows
that some logic from mt7530_pad_clk_setup() is needed even for port 5.
Prior to commit ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK
API"), a call did exist for all phy_is_pseudo_fixed_link() ports - so
port 5 included. That commit replaced it with a temporary "Port 5 is not
supported!" comment, and the following commit 38f790a80560 ("net: dsa:
mt7530: Add support for port 5") replaced that comment with a
configuration procedure in mt7530_setup_port5() which was insufficient
for port 5 to work. I'm laying the blame on the patch that claimed
support for port 5, although one would have also needed the change from
commit c3b8e07909db ("net: dsa: mt7530: setup core clock even in TRGMII
mode") for the write to be performed completely independently from port
6's configuration.

Thanks go to Arınç for describing the problem, for debugging and for
testing.

Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Link: https://lore.kernel.org/netdev/f297c2c4-6e7c-57ac-2394-f6025d309b9d@arinc9.com/
Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/mt7530.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)
  

On 7.03.2023 18:54, Vladimir Oltean wrote:
> The MT7530 switch from the MT7621 SoC has 2 ports which can be set up as
> internal: port 5 and 6. Arınç reports that the GMAC1 attached to port 5
> receives corrupted frames, unless port 6 (attached to GMAC0) has been
> brought up by the driver. This is true regardless of whether port 5 is
> used as a user port or as a CPU port (carrying DSA tags).
> 
> Offline debugging (blind for me) which began in the linked thread showed
> experimentally that the configuration done by the driver for port 6
> contains a step which is needed by port 5 as well - the write to
> CORE_GSWPLL_GRP2 (note that I've no idea as to what it does, apart from
> the comment "Set core clock into 500Mhz"). Prints put by Arınç show that
> the reset value of CORE_GSWPLL_GRP2 is RG_GSWPLL_POSDIV_500M(1) |
> RG_GSWPLL_FBKDIV_500M(40) (0x128), both on the MCM MT7530 from the
> MT7621 SoC, as well as on the standalone MT7530 from MT7623NI Bananapi
> BPI-R2. Apparently, port 5 on the standalone MT7530 can work under both
> values of the register, while on the MT7621 SoC it cannot.
> 
> The call path that triggers the register write is:
> 
> mt753x_phylink_mac_config() for port 6
> -> mt753x_pad_setup()
>     -> mt7530_pad_clk_setup()
> 
> so this fully explains the behavior noticed by Arınç, that bringing port
> 6 up is necessary.
> 
> The simplest fix for the problem is to extract the register writes which
> are needed for both port 5 and 6 into a common mt7530_pll_setup()
> function, which is called at mt7530_setup() time, immediately after
> switch reset. We can argue that this mirrors the code layout introduced
> in mt7531_setup() by commit 42bc4fafe359 ("net: mt7531: only do PLL once
> after the reset"), in that the PLL setup has the exact same positioning,
> and further work to consolidate the separate setup() functions is not
> hindered.
> 
> Testing confirms that:
> 
> - the slight reordering of writes to MT7530_P6ECR and to
>    CORE_GSWPLL_GRP1 / CORE_GSWPLL_GRP2 introduced by this change does not
>    appear to cause problems for the operation of port 6 on MT7621 and on
>    MT7623 (where port 5 also always worked)
> 
> - packets sent through port 5 are not corrupted anymore, regardless of
>    whether port 6 is enabled by phylink or not (or even present in the
>    device tree)
> 
> My algorithm for determining the Fixes: tag is as follows. Testing shows
> that some logic from mt7530_pad_clk_setup() is needed even for port 5.
> Prior to commit ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK
> API"), a call did exist for all phy_is_pseudo_fixed_link() ports - so
> port 5 included. That commit replaced it with a temporary "Port 5 is not
> supported!" comment, and the following commit 38f790a80560 ("net: dsa:
> mt7530: Add support for port 5") replaced that comment with a
> configuration procedure in mt7530_setup_port5() which was insufficient
> for port 5 to work. I'm laying the blame on the patch that claimed
> support for port 5, although one would have also needed the change from
> commit c3b8e07909db ("net: dsa: mt7530: setup core clock even in TRGMII
> mode") for the write to be performed completely independently from port
> 6's configuration.
> 
> Thanks go to Arınç for describing the problem, for debugging and for
> testing.
> 
> Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Link: https://lore.kernel.org/netdev/f297c2c4-6e7c-57ac-2394-f6025d309b9d@arinc9.com/
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Cheers.
Arınç
  

On Tue, Mar 07, 2023 at 05:54:11PM +0200, Vladimir Oltean wrote:
> The MT7530 switch from the MT7621 SoC has 2 ports which can be set up as
> internal: port 5 and 6. Arınç reports that the GMAC1 attached to port 5
> receives corrupted frames, unless port 6 (attached to GMAC0) has been
> brought up by the driver. This is true regardless of whether port 5 is
> used as a user port or as a CPU port (carrying DSA tags).
> 
> Offline debugging (blind for me) which began in the linked thread showed
> experimentally that the configuration done by the driver for port 6
> contains a step which is needed by port 5 as well - the write to
> CORE_GSWPLL_GRP2 (note that I've no idea as to what it does, apart from
> the comment "Set core clock into 500Mhz"). Prints put by Arınç show that
> the reset value of CORE_GSWPLL_GRP2 is RG_GSWPLL_POSDIV_500M(1) |
> RG_GSWPLL_FBKDIV_500M(40) (0x128), both on the MCM MT7530 from the
> MT7621 SoC, as well as on the standalone MT7530 from MT7623NI Bananapi
> BPI-R2. Apparently, port 5 on the standalone MT7530 can work under both
> values of the register, while on the MT7621 SoC it cannot.
> 
> The call path that triggers the register write is:
> 
> mt753x_phylink_mac_config() for port 6
> -> mt753x_pad_setup()
>    -> mt7530_pad_clk_setup()
> 
> so this fully explains the behavior noticed by Arınç, that bringing port
> 6 up is necessary.
> 
> The simplest fix for the problem is to extract the register writes which
> are needed for both port 5 and 6 into a common mt7530_pll_setup()
> function, which is called at mt7530_setup() time, immediately after
> switch reset. We can argue that this mirrors the code layout introduced
> in mt7531_setup() by commit 42bc4fafe359 ("net: mt7531: only do PLL once
> after the reset"), in that the PLL setup has the exact same positioning,
> and further work to consolidate the separate setup() functions is not
> hindered.
> 
> Testing confirms that:
> 
> - the slight reordering of writes to MT7530_P6ECR and to
>   CORE_GSWPLL_GRP1 / CORE_GSWPLL_GRP2 introduced by this change does not
>   appear to cause problems for the operation of port 6 on MT7621 and on
>   MT7623 (where port 5 also always worked)
> 
> - packets sent through port 5 are not corrupted anymore, regardless of
>   whether port 6 is enabled by phylink or not (or even present in the
>   device tree)
> 
> My algorithm for determining the Fixes: tag is as follows. Testing shows
> that some logic from mt7530_pad_clk_setup() is needed even for port 5.
> Prior to commit ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK
> API"), a call did exist for all phy_is_pseudo_fixed_link() ports - so
> port 5 included. That commit replaced it with a temporary "Port 5 is not
> supported!" comment, and the following commit 38f790a80560 ("net: dsa:
> mt7530: Add support for port 5") replaced that comment with a
> configuration procedure in mt7530_setup_port5() which was insufficient
> for port 5 to work. I'm laying the blame on the patch that claimed
> support for port 5, although one would have also needed the change from
> commit c3b8e07909db ("net: dsa: mt7530: setup core clock even in TRGMII
> mode") for the write to be performed completely independently from port
> 6's configuration.
> 
> Thanks go to Arınç for describing the problem, for debugging and for
> testing.
> 
> Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Link: https://lore.kernel.org/netdev/f297c2c4-6e7c-57ac-2394-f6025d309b9d@arinc9.com/
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
  

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  7 Mar 2023 17:54:11 +0200 you wrote:
> The MT7530 switch from the MT7621 SoC has 2 ports which can be set up as
> internal: port 5 and 6. Arınç reports that the GMAC1 attached to port 5
> receives corrupted frames, unless port 6 (attached to GMAC0) has been
> brought up by the driver. This is true regardless of whether port 5 is
> used as a user port or as a CPU port (carrying DSA tags).
> 
> Offline debugging (blind for me) which began in the linked thread showed
> experimentally that the configuration done by the driver for port 6
> contains a step which is needed by port 5 as well - the write to
> CORE_GSWPLL_GRP2 (note that I've no idea as to what it does, apart from
> the comment "Set core clock into 500Mhz"). Prints put by Arınç show that
> the reset value of CORE_GSWPLL_GRP2 is RG_GSWPLL_POSDIV_500M(1) |
> RG_GSWPLL_FBKDIV_500M(40) (0x128), both on the MCM MT7530 from the
> MT7621 SoC, as well as on the standalone MT7530 from MT7623NI Bananapi
> BPI-R2. Apparently, port 5 on the standalone MT7530 can work under both
> values of the register, while on the MT7621 SoC it cannot.
> 
> [...]

Here is the summary with links:
  - [net] net: dsa: mt7530: permit port 5 to work without port 6 on MT7621 SoC
    https://git.kernel.org/netdev/net/c/c8b8a3c601f2

You are awesome, thank you!
  

On 7.03.2023 18:54, Vladimir Oltean wrote:
> The MT7530 switch from the MT7621 SoC has 2 ports which can be set up as
> internal: port 5 and 6. Arınç reports that the GMAC1 attached to port 5
> receives corrupted frames, unless port 6 (attached to GMAC0) has been
> brought up by the driver. This is true regardless of whether port 5 is
> used as a user port or as a CPU port (carrying DSA tags).
> 
> Offline debugging (blind for me) which began in the linked thread showed
> experimentally that the configuration done by the driver for port 6
> contains a step which is needed by port 5 as well - the write to
> CORE_GSWPLL_GRP2 (note that I've no idea as to what it does, apart from
> the comment "Set core clock into 500Mhz"). Prints put by Arınç show that
> the reset value of CORE_GSWPLL_GRP2 is RG_GSWPLL_POSDIV_500M(1) |
> RG_GSWPLL_FBKDIV_500M(40) (0x128), both on the MCM MT7530 from the
> MT7621 SoC, as well as on the standalone MT7530 from MT7623NI Bananapi
> BPI-R2. Apparently, port 5 on the standalone MT7530 can work under both
> values of the register, while on the MT7621 SoC it cannot.

I finally found out why. Looking at gsw_mt7623.c [0], setting the core 
clock into 500Mhz is done for 40MHz XTAL. With some dev_info code, I was 
able to confirm that the MCM MT7530 on my MT7621 board runs at 40MHz 
whilst the standalone MT7530 on my Bananapi BPI-R2 runs at 25MHz.

[0] 
https://github.com/BPI-SINOVOIP/BPI-R2-bsp/blob/master/linux-mt/drivers/net/ethernet/mediatek/gsw_mt7623.c#L1039

Arınç