Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Siddharth Vadapalli <s-vadapalli@ti.com>
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, vladimir.oltean@nxp.com,
	grygorii.strashko@ti.com, vigneshr@ti.com, nsekhar@ti.com,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, kishon@ti.com
Subject: Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location
Date: Wed, 1 Jun 2022 10:55:39 +0100	[thread overview]
Message-ID: <Ypc3myH2SgGwUmMF@shell.armlinux.org.uk> (raw)
In-Reply-To: <41277985-28c9-9bf0-8b24-6acc40391ef2@ti.com>

On Wed, Jun 01, 2022 at 02:59:47PM +0530, Siddharth Vadapalli wrote:
> Hello Russell,
> 
> On 01/06/22 13:59, Russell King (Oracle) wrote:
> > On Wed, Jun 01, 2022 at 11:39:57AM +0530, Siddharth Vadapalli wrote:
> >> Hello Russell,
> >>
> >> On 31/05/22 17:25, Russell King (Oracle) wrote:
> >>> On Tue, May 31, 2022 at 05:00:58PM +0530, Siddharth Vadapalli wrote:
> >>>> In TI's J7200 SoC CPSW5G ports, each of the 4 ports can be configured
> >>>> as a QSGMII main or QSGMII-SUB port. This configuration is performed
> >>>> by phy-gmii-sel driver on invoking the phy_set_mode_ext() function.
> >>>>
> >>>> It is necessary for the QSGMII main port to be configured before any of
> >>>> the QSGMII-SUB interfaces are brought up. Currently, the QSGMII-SUB
> >>>> interfaces come up before the QSGMII main port is configured.
> >>>>
> >>>> Fix this by moving the call to phy_set_mode_ext() from
> >>>> am65_cpsw_nuss_ndo_slave_open() to am65_cpsw_nuss_init_slave_ports(),
> >>>> thereby ensuring that the QSGMII main port is configured before any of
> >>>> the QSGMII-SUB ports are brought up.
> >>>
> >>> This sounds like "if we're configured via port->slave.phy_if to be in
> >>> QSGMII mode, then the serdes PHY needs to be configured before any of
> >>> the QSGMII ports are used". Doesn't that mean that if
> >>> port->slave.phy_if is QSGMII, then the port _only_ supports QSGMII
> >>> mode, and conversely, the port doesn't support QSGMII unless firmware
> >>> said it could be.
> >>>
> >>> So, doesn't that mean am65_cpsw_nuss_init_port_ndev() should indicate
> >>> only QSGMII, or only the RGMII modes, but never both together?
> >>
> >> The phy-gmii-sel driver called by phy_set_mode_ext() configures the CPSW5G MAC
> >> rather than the SerDes Phy. In the CPSW5G MAC, the QSGMII mode is further split
> >> up as two modes that are TI SoC specific, namely QSGMII main and QSGMII-SUB. Of
> >> the 4 ports present in CPSW5G (4 external ports), only one can be the main port
> >> while the rest are the QSGMII-SUB ports. Only the QSGMII main interface is
> >> responsible for auto-negotiation between the MAC and PHY. For this reason, the
> >> writes to the CPSW5G MAC, mentioning which of the interfaces is the QSGMII main
> >> interface and which ones are the QSGMII-SUB interfaces has to be done before any
> >> of the interfaces are brought up. Otherwise, it would result in a QSGMII-SUB
> >> interface being brought up before the QSGMII main interface is determined,
> >> resulting in the failure of auto-negotiation process, thereby making the
> >> QSGMII-SUB interfaces non-functional.
> > 
> > That confirms my suspicion - if an interface is in QSGMII mode, then
> > RGMII should not be marked as a supported interface to phylink. If the
> 
> CPSW5G MAC supports both RGMII and QSGMII modes, so wouldn't it be correct to
> mark both RGMII and QSGMII modes as supported? The mode is specified in the
> device-tree and configured in CPSW5G MAC accordingly.
> 
> > "QSGMII main interface" were to be switched to RGMII mode, then this
> > would break the other ports. So RGMII isn't supported if in QSGMII
> > mode.
> 
> Yes, if the QSGMII main interface were to be switched to RGMII mode, then it
> would break the other ports. However, the am65-cpsw driver currently has no
> provision to dynamically change the port modes once the driver is initialized.

If there is no provision to change the port mode, then as far as
phylink is concerned, you should not advertise that it supports
anything but the current mode - because if phylink were to request
the driver change the mode, the driver can't do it.

So, you want there, at the very least:

	if (phy_interface_mode_is_rgmii(port->slave.phy_if))
		phy_interface_set_rgmii(port->slave.phylink_config.supported_interfaces);
	else
		__set_bit(PHY_INTERFACE_MODE_QSGMII, port->slave.phylink_config.supported_interfaces);

which will still ensure that port->slave.phy_if is either a RGMII
mode or QSGMII.

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