Date: Tue, 02 Jul 2024 10:11:07 +0200
From: Romain Gantois <romain.gantois@...tlin.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
 Russell King <linux@...linux.org.uk>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

Hello Andrew, thanks for the review!

I think this particular patch warrants that I explain myself a bit more.

Some SGMII SFP modules will work fine once they're inserted and the appropriate 
probe() function has been called by the SFP PHY driver. However, this is not 
necessarily the case, as some SFP PHYs require further configuration before the 
link can be brought up (e.g. calling phy_init_hw() on them which will 
eventually call things like config_init()).

This configuration usually doesn't happen before the PHY device is attached to 
a network device. In this case, the DP83869 PHY is placed between the MAC and 
the SFP PHY. Thus, the DP83869 is attached to a network device while the SFP 
PHY is not. This means that the DP83869 driver basically takes on the role of 
the MAC driver in some aspects.

In this patch, I used the connect_phy() callback as a way to get a handle to 
the downstream SFP PHY. This callback is only implemented by phylink so far.

I used the module_start() callback to initialize the SFP PHY hardware and 
start it's state machine.

On lundi 1 juillet 2024 19:00:29 UTC+2 Andrew Lunn wrote:
> > +static int dp83869_connect_phy(void *upstream, struct phy_device *phy)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +	struct dp83869_private *dp83869;
> > +
> > +	dp83869 = phydev->priv;
> > +
> > +	if (dp83869->mode != DP83869_RGMII_SGMII_BRIDGE)
> > +		return 0;
> > +
> > +	if (!phy->drv) {
> > +		dev_warn(&phy->mdio.dev, "No driver bound to SFP module phy!
\n");
> 
> more instances which could be phydev_{err|warn|info}().
> 
> > +		return 0;
> > +	}
> > +
> > +	phy_support_asym_pause(phy);
> 
> That is unusual. This is normally used by a MAC driver to indicate it
> supports asym pause. It is the MAC which implements pause, but the PHY
> negotiates it. So this tells phylib what to advertise to the link
> partner. Why would a PHY do this? What if the MAC does not support
> asym pause?
> 

The idea here was that the downstream SFP PHY should advertise pause 
capabilities if the upstream MAC and intermediate DP83869 PHY supported them. 
However, the way I implemented this is indeed flawed, since the DP83869 driver 
should check that the MAC wants to advertise pause capabilities before calling 
this on the downstream PHY.

I suggest the following logic instead:

if (pause bits are set in dp83869 advertising mask)
	copy pause bits from sfp_phy->supported to sfp_phy->advertising

> > +	linkmode_set_bit(PHY_INTERFACE_MODE_SGMII, phy->host_interfaces);
> > +	phy->interface = PHY_INTERFACE_MODE_SGMII;
> > +	phy->port = PORT_TP;
> > +
> > +	phy->speed = SPEED_UNKNOWN;
> > +	phy->duplex = DUPLEX_UNKNOWN;
> > +	phy->pause = MLO_PAUSE_NONE;
> > +	phy->interrupts = PHY_INTERRUPT_DISABLED;
> > +	phy->irq = PHY_POLL;
> > +	phy->phy_link_change = &dp83869_sfp_phy_change;
> > +	phy->state = PHY_READY;
> 
> I don't know of any other PHY which messes with the state machine like
> this. This needs some explanation.

phylink_sfp_connect_phy() does something similar. The reasoning behind setting 
PHY_READY is that the later call to phy_start() will fail if the PHY isn't in 
the PHY_READY or PHY_HALTED state.

No other PHY driver does this because as of now, phylink is the only implementer 
of the connect_phy() callback. Other PHY drivers don't seem to support handling 
the initial configuration of a downstream SFP PHY.

> 
> > +
> > +	dp83869->mod_phy = phy;
> > +
> > +	return 0;
> > +}
> > +
> > +static void dp83869_disconnect_phy(void *upstream)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +	struct dp83869_private *dp83869;
> > +
> > +	dp83869 = phydev->priv;
> > +	dp83869->mod_phy = NULL;
> > +}
> > +
> > +static int dp83869_module_start(void *upstream)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +	struct dp83869_private *dp83869;
> > +	struct phy_device *mod_phy;
> > +	int ret;
> > +
> > +	dp83869 = phydev->priv;
> > +	mod_phy = dp83869->mod_phy;
> > +	if (!mod_phy)
> > +		return 0;
> > +
> > +	ret = phy_init_hw(mod_phy);
> > +	if (ret) {
> > +		dev_err(&mod_phy->mdio.dev, "Failed to initialize PHY hardware: 
error
> > %d", ret); +		return ret;
> > +	}
> > +
> > +	phy_start(mod_phy);
> 
> Something else no other PHY driver does....

phy_init_hw() is necessary here to ensure that the SFP PHY is configured properly before 
the link is brought up. phy_start() is used to start the phylib state machine, which is what 
would happen if the SFP PHY was directly connected to a MAC.

As with the connect_phy() case, no other PHY  driver currently implements module_start(), 
only phylink does, which is why this code might look out of place.

Of course, there could be flaws in my understanding of phylib or the SFP core, please let 
me know what you think.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists