Messages in this thread | | | Date | Wed, 8 Mar 2023 13:09:19 +0100 | From | Simon Horman <> | Subject | Re: [PATCH net] net: dsa: mt7530: permit port 5 to work without port 6 on MT7621 SoC |
| |
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>
| |