| Message ID | 20240701-b4-dp83869-sfp-v1-5-a71d6d0ad5f8@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: dp83869: Add support for downstream SFP cages | expand |
> +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? > + 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. > + > + 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.... Andrew
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,
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. 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. Andrew
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
On Tue, Jul 02, 2024 at 03:21:09PM +0200, Andrew Lunn wrote: > 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. phylink only knows about SFPs that are directly connected to the MAC/PCS. It has no knowledge of SFPs that are behind a PHY (like on the Macchiatobin with 88x3310 PHYs.) Due to the structure of the networking layer, I don't see how we could sanely make stacked PHYs work - we expect the ethtool APIs to target the media PHY, but in the case of a platform such as Macchiatobin, we potentially have _two_ media facing PHYs on one network interface. There's the 88x3310 which has its own RJ45 socket, and then if one plugs in a copper SFP, you get another media-facing PHY with its own RJ45 socket. Which PHY should ethtool ksettings_set interact with? The ethtool API wasn't designed for this kind of thing!
On Tue, Jul 02, 2024 at 04:56:28PM +0200, Maxime Chevallier wrote: > 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). This is not a phylink problem, but a phy*lib* implementation problem. It was decided that phy*lib* would take part in layering violations with various functions called *direct* from the core networking layer bypassing MAC drivers entirely. Even with phy*link* that still happens - as soon as netdev->phydev has been set, the networking core will forward PHY related calls to that phydev bypassing the MAC driver and phylink. If we want to start handling multiple layers of PHYs, then we have to get rid of this layering bypass.
On Tue, Jul 02, 2024 at 07:13:50PM +0100, Russell King (Oracle) wrote: > On Tue, Jul 02, 2024 at 04:56:28PM +0200, Maxime Chevallier wrote: > > 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). > > This is not a phylink problem, but a phy*lib* implementation problem. > It was decided that phy*lib* would take part in layering violations > with various functions called *direct* from the core networking layer > bypassing MAC drivers entirely. > > Even with phy*link* that still happens - as soon as netdev->phydev > has been set, the networking core will forward PHY related calls > to that phydev bypassing the MAC driver and phylink. > > If we want to start handling multiple layers of PHYs, then we have > to get rid of this layering bypass. Or at least minimise them. SQI values probably don't cause an issue in this situation, that seems to be mostly automotive which is unlikely to have multiple PHYs. link_down_events probably should be cleaned up and set as part of ethtool_ops->get_link_ext_stats. All the plca is more automotive, i doubt there will be two PHYs involved there. PHY tunabled we need the work bootlin is doing so we can direct to towards a specific PHY. Going through the MAC does not help us. The same is true for PHY statistics. There are no PHY drivers which implement module_info, so that bypass can be deleted. Work is being done on tsinfo, etc. Cable test ideally wants to be run on the outer PHY, but again, the bootlin work should solve this. PSE and multiple PHYs also seems unlikely. So i don't think the problem is too big. Andrew
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c index a3ccaad738b28..a07ec1be84baf 100644 --- a/drivers/net/phy/dp83869.c +++ b/drivers/net/phy/dp83869.c @@ -153,6 +153,8 @@ struct dp83869_private { bool rxctrl_strap_quirk; int clk_output_sel; int mode; + /* PHY in a downstream SFP module */ + struct phy_device *mod_phy; }; static int dp83869_read_status(struct phy_device *phydev) @@ -845,6 +847,93 @@ static int dp83869_config_init(struct phy_device *phydev) return ret; } +static void dp83869_sfp_phy_change(struct phy_device *phydev, bool up) +{ + /* phylink uses this to populate its own structs from phy_device fields + * we don't actually need this callback but if we don't implement it + * the phy core will crash + */ +} + +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"); + return 0; + } + + phy_support_asym_pause(phy); + 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; + + 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); + + return 0; +} + +static void dp83869_module_stop(void *upstream) +{ + struct phy_device *phydev = upstream; + struct dp83869_private *dp83869; + struct phy_device *mod_phy; + + dp83869 = phydev->priv; + mod_phy = dp83869->mod_phy; + if (!mod_phy) + return; + + phy_stop(mod_phy); +} + static int dp83869_module_insert(void *upstream, const struct sfp_eeprom_id *id) { __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_support); @@ -858,6 +947,13 @@ static int dp83869_module_insert(void *upstream, const struct sfp_eeprom_id *id) phylink_set(phy_support, 1000baseX_Full); phylink_set(phy_support, 100baseFX_Full); phylink_set(phy_support, FIBRE); + phylink_set(phy_support, 10baseT_Full); + phylink_set(phy_support, 100baseT_Full); + phylink_set(phy_support, 1000baseT_Full); + phylink_set(phy_support, Autoneg); + phylink_set(phy_support, Pause); + phylink_set(phy_support, Asym_Pause); + phylink_set(phy_support, TP); linkmode_zero(sfp_support); sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces); @@ -877,6 +973,10 @@ static int dp83869_module_insert(void *upstream, const struct sfp_eeprom_id *id) dp83869 = phydev->priv; switch (interface) { + case PHY_INTERFACE_MODE_SGMII: + dp83869->mode = DP83869_RGMII_SGMII_BRIDGE; + phydev->port = PORT_TP; + break; case PHY_INTERFACE_MODE_100BASEX: dp83869->mode = DP83869_RGMII_100_BASE; phydev->port = PORT_FIBRE; @@ -899,6 +999,10 @@ static const struct sfp_upstream_ops dp83869_sfp_ops = { .attach = phy_sfp_attach, .detach = phy_sfp_detach, .module_insert = dp83869_module_insert, + .module_start = dp83869_module_start, + .module_stop = dp83869_module_stop, + .connect_phy = dp83869_connect_phy, + .disconnect_phy = dp83869_disconnect_phy, }; static int dp83869_probe(struct phy_device *phydev)
The DP83869HM PHY transceiver can be configured in RGMII-SGMII bridge mode to interface an RGMII MAC with a standard SFP module containing an integrated PHY. Additional SFP upstream ops are needed to notify the DP83869 driver of SFP PHY detection so that it can initialize it. Add relevant SFP callbacks to the DP83869 driver to support SGMII modules. Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> --- drivers/net/phy/dp83869.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+)