Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Andrew,

On Tue, 2 Jul 2024 15:21:09 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Jul 02, 2024 at 10:11:07AM +0200, Romain Gantois wrote:
> > 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.  
> 
> The SFP PHY is however a PHY which phylib is managing. And you have
> phylink on top of that, which knows about both PHYs. Architecturally,
> i really think phylink should be dealing with all this
> configuration.
> 
> The MAC driver has told phylink its pause capabilities.
> phylink_bringup_phy() will tell phylib these capabilities by calling
> phy_support_asym_pause(). Why does this not work for the SFP PHY?
> 
> phylink knows when the SFP PHY is plugged in, and knows if the link is
> admin up. It should be starting the state machine, not the PHY.

When the SFP upstream is a PHY, phylink isn't involved in managing the
SFP PHY, only the sfp-bus.c code will, using the SFP upstream ops, for
which phylink is one possible provider. When phylink is the SFP
upstream, it does all the mod_phy handling, so other upstream_ops
providers should also do the same.

But I do agree with you in that this logic should not belong to any
specific PHY driver, and be made generic. phylink is one place to
implement that indeed, but so far phylink can't manage both PHYs on the
link (if I'm not mistaken). I don't know how this all fits in phylink
itself, or if this should rather be some phylib / sfp-bus helpers and
extra plumbing to ease writing phy drivers that do SFP and want to
better manage the module PHY.

If I'm not mistaken, Romain's setup uses a MAC that doesn't even use
phylink yet (but the porting guide was updated recently fortunately ;) )

> 
> Do you have access to a macchiatobin? I suggest you play with one, see
> how the marvell PHY driver works when you plug in a copper SFP module.

On my side I have access to one, but it's hard to tell as the 3310 only
really supports 10G copper SFPs so far and I only have one. However from
testing on other boards, it looks like when a PHY is acting as
upstream, copper SFP will work when their default behaviour is to start
autoneg, link establishment and so on automatically when inserted.

The behaviour is currently not consistent between systems that have
phylink as the SFP upstream (no media converter) and systems that have
a media converter.

I think Romain has tested the platform he's using for this series with
multiple copper SFPs, some will work fine, and some just don't. Same
copper SFP work just fine when used on systems that have a straight
MAC <-> SFP link managed by phylink.

Maxime