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

From: Russell King - ARM Linux admin
Date: Mon Jun 24 2019 - 06:22:40 EST


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