Messages in this thread | | | Date | Mon, 24 Jun 2019 11:22:24 +0100 | From | Russell King - ARM Linux admin <> | Subject | Re: [PATCH v4 2/5] net: macb: add support for sgmii MAC-PHY interface |
| |
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
| |