On Wed, Oct 21, 2020 at 07:44:05PM +0200, Parshuram Thombare wrote: > This patch adds support for 10GBASE-R interface to the linux driver for > Cadence's ethernet controller. > This controller has separate MAC's and PCS'es for low and high speed paths. > High speed PCS supports 100M, 1G, 2.5G, 5G and 10G through rate adaptation > implementation. However, since it doesn't support auto negotiation, linux > driver is modified to support 10GBASE-R insted of USXGMII. > > Signed-off-by: Parshuram Thombare <pthom...@cadence.com> > --- > Changes between v2 and v3: > 1. Replace USXGMII interface by 10GBASE-R interface. > 2. Adopted new phylink pcs_ops for high speed PCS. > 3. Added pcs_get_state for high speed PCS.
Hi, If you're going to support pcs_ops for the 10GBASE-R mode, can you also convert macb to use pcs_ops for the lower speed modes as well? The reason is that when you have pcs_ops in place, there are slight behaviour differences in the way phylink calls the MAC functions, and in the long run I would like to eventually retire the old ways (so we don't have to keep compatible with old in-kernel APIs alive for ever.) I think all that needs to happen is a pcs_ops for the non-10GBASE-R mode which moves macb_mac_pcs_get_state() and macb_mac_an_restart() to it, and implements a stub pcs_config(). So it should be simple to do. Further comments below. > +static int macb_usx_pcs_config(struct phylink_pcs *pcs, > + unsigned int mode, > + phy_interface_t interface, > + const unsigned long *advertising, > + bool permit_pause_to_mac) > +{ > + struct macb *bp = container_of(pcs, struct macb, phylink_pcs); > + u32 val; > + > + val = gem_readl(bp, NCFGR); > + val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val); > + gem_writel(bp, NCFGR, val); This looks like it's configuring the MAC rather than the PCS - it should probably be in mac_prepare() or mac_config(). Note that the order of calls when changing the interface mode will be: - mac_prepare() - mac_config() - pcs_config() - pcs_an_restart() optionally - mac_finish() > +static int macb_mac_prepare(struct phylink_config *config, unsigned int mode, > + phy_interface_t interface) > +{ > + struct net_device *ndev = to_net_dev(config->dev); > + struct macb *bp = netdev_priv(ndev); > + > + if (interface == PHY_INTERFACE_MODE_10GBASER) { > + bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops; > + phylink_set_pcs(bp->phylink, &bp->phylink_pcs); > + } What happens if phylink requests 10GBASE-R and subsequently selects a different interface mode? You end up with the PCS still registered and phylink will use it even when not in 10GBASE-R mode - so its functions will also be called. Note also that if a PCS is registered, phylink will omit to call mac_config() unless the interface mode is being changed. If no PCS is registered, it will call mac_config() in the old way (which includes link up events.) Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!