Re: [PATCH net-next 6/7] net: txgbe: support copper NIC with external PHY

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Jul 25, 2023 at 06:04:49PM +0800, Jiawen Wu wrote:
> On Tuesday, July 25, 2023 4:03 PM, Russell King (Oracle) wrote:
> > On Tue, Jul 25, 2023 at 10:41:46AM +0800, Jiawen Wu wrote:
> > > On Monday, July 24, 2023 6:43 PM, Russell King (Oracle) wrote:
> > > > On Mon, Jul 24, 2023 at 06:23:40PM +0800, Jiawen Wu wrote:
> > > > > @@ -22,6 +25,9 @@ static int txgbe_get_link_ksettings(struct net_device *netdev,
> > > > >  {
> > > > >  	struct txgbe *txgbe = netdev_to_txgbe(netdev);
> > > > >
> > > > > +	if (txgbe->wx->media_type == sp_media_copper)
> > > > > +		return phy_ethtool_get_link_ksettings(netdev, cmd);
> > > >
> > > > Why? If a PHY is attached via phylink, then phylink will automatically
> > > > forward the call below to phylib.
> > >
> > > No, there is no phylink implemented for sp_media_copper.
> > >
> > > > > +
> > > > >  	return phylink_ethtool_ksettings_get(txgbe->phylink, cmd);
> > > >
> > > > If you implement it correctly, you also don't need two entirely
> > > > separate paths to configure the MAC/PCS for the results of the PHY's
> > > > negotiation, because phylink gives you a _generic_ set of interfaces
> > > > between whatever is downstream from the MAC and the MAC.
> > >
> > > For sp_media_copper, only mii bus is registered for attaching PHY.
> > > Most MAC/PCS configuration is done in firmware, so it is not necessary
> > > to implement phylink as sp_media_fiber.
> > 
> > If you do implement phylink for copper, then you don't need all these
> > conditionals and the additional adjust_link implementation. In other
> > words, you can re-use a lot of the code you've already added.
> > 
> > You don't have to provide a PCS to phylink provided you don't tell
> > phylink that it's "in-band".
> 
> Do I need to create a separate software node? That would seem to
> break more code of fiber initialization flow. I could try, but I'd like to
> keep the two flows separate.

You don't need any of the swnodes to be registered, so
txgbe_swnodes_register() can be skipped. You also don't need
txgbe_mdio_pcs_init() as you said firmware will look after that.

You will need txgbe_phylink_init() to select phy_mode depending on
whether your configuration is for SFP or not, so something like:

	if (txgbe->wx->media_type == sp_media_copper) {
		phy_mode = PHY_INTERFACE_MODE_XAUI;
		fwnode = NULL;
	} else {
		phy_mode = PHY_INTERFACE_MODE_10GBASER;
		fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_PHYLINK]);
	}

	__set_bit(phy_mode, config->supported_interfaces);
	phylink = phylink_create(config, fwnode, phy_mode, &txgbe_mac_ops);

You can then use phylink_connect_phy() to add the phydev to phylink.

You'll probably need to make txgbe_phylink_mac_select() check whether
txgbe->xpcs is non-NULL to prevent a NULL pointer dereference as I
don't believe you have the XPCS in this setup - or alternatively:

	if (interface == PHY_INTERFACE_MODE_10GBASER)
		return &txgbe->xpcs->pcs;

	return NULL;

and that should be about all that would be required. phylink will
then forward all the appropriate calls onto phylib for you, take care
of reading the phy's status, and calling the mac_link_up/mac_link_down
functions as the PHY status indicates the link changes state.

Phylink will call mac_prepare()/mac_config()/mac_finish() when the
netdev is opened, and will also limit the PHY's advertisement
according to the capabilities supplied in mac_capabilities, so you
shouldn't need to remove unsupported link modes from the PHY.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!