Hello Russell, On 01/06/22 15:25, Russell King (Oracle) wrote: > 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. Thank you for reviewing the patch. I will send v2 for this series implementing the fix suggested above. Thanks, Siddharth.
