Re: [PATCH v4 2/5] net: macb: add support for sgmii MAC-PHY interface

[Russell King - ARM Linux admin]

On Mon, Jun 24, 2019 at 10:14:41AM +0000, Parshuram Raju Thombare wrote:
> >> >I still don't think this makes much sense, splitting the interface
> >> > configuration between here and below.
> >> Do you mean splitting mac_config in two *_configure functions ?
> >> This was done as per Andrew's suggestion to make code mode readable
> >> and easy to manage by splitting MAC configuration for different interfaces.
> >No, I mean here you disable SGMII if we're switching away from SGMII
> >mode.... (note, this means there is more to come for this sentence)
> Sorry, I misunderstood your original question. I think disabling old interface
> and enabling new one can be done in single place. I will do this change.
> 
> >> >This will only be executed when we are not using inband mode, which
> >> >basically means it's not possible to switch to SGMII in-band mode.
> >> SGMII is used in default PHY mode. And above code is to program MAC to
> >> select PCS and SGMII interface.
> >... and here you enable it for SGMII mode, but only for non-inband
> >modes.
> >
> >Why not:
> >	if (change_interface)  {
> >		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> >			// Enable SGMII mode and PCS
> >			gem_writel(bp, NCFGR, ncfgr | GEM_BIT(SGMIIEN) |
> >				   GEM_BIT(PCSSEL));
> >		} else {
> >			// Disable SGMII mode and PCS
> >			gem_writel(bp, NCFGR, ncfgr & ~(GEM_BIT(SGMIIEN)
> >				   GEM_BIT(PCSSEL)));
> >			// Reset PCS
> >			gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL)
> >				   GEM_BIT(PCS_CTRL_RST));
> >		}
> >	}
> >	if (!phylink_autoneg_inband(mode) &&
> >	    (bp->speed != state->speed || bp->duplex != state->duplex)) {
> >?
> Ok
> 
> >> >> +
> >> >> +		if (!interface_supported) {
> >> >> +			netdev_err(dev, "Phy mode %s not supported",
> >> >> +				   phy_modes(phy_mode));
> >> >> +			goto err_out_free_netdev;
> >> >> +		}
> >> >> +
> >> >>  		bp->phy_interface = phy_mode;
> >> >> +	} else {
> >> >> +		bp->phy_interface = phy_mode;
> >> >> +	}
> >> >If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and
> >> > mac_config()
> >> >is called with state->interface = PHY_INTERFACE_MODE_SGMII, then
> >> >mac_config() won't configure the MAC for the interface type - is that
> >> >intentional?
> >> In mac_config configure MAC for non in-band mode, there is also check for
> >> speed, duplex
> >> changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN
> >> and DUPLEX_UNKNOWN
> >> values so it is expected that for non in band mode state contains valid speed
> >> and duplex mode
> >> which are different from *_UNKNOWN values.
> 
> >Sorry, this reply doesn't answer my question.  I'm not asking about
> >bp->speed and bp->duplex.  I'm asking:
> >1) why you are initialising bp->phy_interface here
> >2) you to consider the impact that has on the mac_config() implementation
> >  you are proposing
> > because I think it's buggy.
> bp->phy_interface is to store phy mode value from device tree. This is used later 
> to know what phy interface user has selected for PHY-MAC. Same is used
> to configure MAC correctly and based on your suggestion code is
> added to handle PHY dynamically changing phy interface, in which 
> case bp->phy_interface is also updated. Though it may not be what user want, 
> if phy interface is totally decided by PHY and is anyway going to be different from what user
> has selected in DT, initializing it here doesn't make sense.
> But in case of PHY not changing phy_interface dynamically bp->phy_interface need to be
> initialized with value from DT.

When phylink_start() is called, you will receive a mac_config() call to
configure the MAC for the initial operating settings, which will include
the current PHY interface mode.  This will initially be whatever
interface mode was passed in to phylink_create().

-- 
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