| Message ID | E1iVhiC-0007bG-Cm@rmk-PC.armlinux.org.uk |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add support for SFPs behind PHYs | expand |
> +static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id) > +{ > + struct phy_device *phydev = upstream; > + __ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, }; > + phy_interface_t iface; > + > + sfp_parse_support(phydev->sfp_bus, id, support); > + iface = sfp_select_interface(phydev->sfp_bus, id, support); > + > + if (iface != PHY_INTERFACE_MODE_10GKR) { > + dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n"); > + return -EINVAL; > + } Hi Russell Is it possible to put an SFP module into an SFP+ cage? sfp_select_interface() would then say 1000Base-X or 2500Base-X. The SFP+ cage has a single SERDES pair, so electrically, would it be possible to do 1000Base-X? Should mv3310_sfp_insert() be reconfiguring the PHY so the SFP side swaps to 1000Base-X? Andrew
On Sat, Nov 16, 2019 at 05:06:35PM +0100, Andrew Lunn wrote: > > +static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id) > > +{ > > + struct phy_device *phydev = upstream; > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, }; > > + phy_interface_t iface; > > + > > + sfp_parse_support(phydev->sfp_bus, id, support); > > + iface = sfp_select_interface(phydev->sfp_bus, id, support); > > + > > + if (iface != PHY_INTERFACE_MODE_10GKR) { > > + dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n"); > > + return -EINVAL; > > + } > > Hi Russell > > Is it possible to put an SFP module into an SFP+ cage? > sfp_select_interface() would then say 1000Base-X or 2500Base-X. The > SFP+ cage has a single SERDES pair, so electrically, would it be > possible to do 1000Base-X? Should mv3310_sfp_insert() be reconfiguring > the PHY so the SFP side swaps to 1000Base-X? The answer is... it depends. Some SFP+ cages have stronger pull-ups and run the I2C bus at 400kHz. SFP modules limit the pullups to 4.7k minimum and a bus speed of 100kHz. Some SFP modules will stand the faster bus speed and the stronger pull-ups. Others will not. Others may end up with EEPROM corruption. It is possible to reconfigure the 3310 to 1000base-X (I don't think 2500base-X is possible on the fiber port) but it requires a PHY reset. I have code to do it, but I haven't tested it - as the pullups on the board I have to test with are too strong to allow the EEPROM in SFP modules to be read. If I get around to replacing the resistor packs, then I can test it, but I'm not going to contribute completely untested code! So, this patch reflects what can be done with the SFP+ slots on Macchiatobin boards today.
> The answer is... it depends. Hi Russell One issue we have had with phylink is people using the interfaces wrongly. When asking this question, i was thinking about documentation. Your answer suggests this method is not simply about the validation you are doing here, it could also be about configuration of the PHY to fit the module. Maybe it would be good to add documentation somewhere about the range of things this call can do? > So, this patch reflects what can be done with the SFP+ slots on > Macchiatobin boards today. This all sounds very hardware dependent. So we are going to need some more DT properties i guess. Have you thought about this? Maybe we need to add compatibles sff,sfp+ and sff,sff+ ? Indicate the board is capable of the higher speeds? And when sfp+/sff+ is used, maybe a boolean to indicate it is also sff/sfp compatible? sfp_select_interface() can then look at these properties and return PHY_INTERFACE_MODE_NA if the board is not capable of supporting the module? Would it even make sense to make the PHY interface more like the MAC interface? A validate function to indicate what it is capable of? A configure function to configure its mode towards the SFP? Andrew
On Sun, Nov 17, 2019 at 08:25:29PM +0100, Andrew Lunn wrote: > > The answer is... it depends. > > Hi Russell > > One issue we have had with phylink is people using the interfaces > wrongly. When asking this question, i was thinking about > documentation. Your answer suggests this method is not simply about > the validation you are doing here, it could also be about > configuration of the PHY to fit the module. Err. Is that not what phylink_sfp_module_insert() is doing - validating that the module can be supported by the MAC and setting the MAC up for it. The implementation for marvell10g is no different - I'm not sure why you're thinking something else is going on here. > Maybe it would be good to add documentation somewhere about the range > of things this call can do? > > > So, this patch reflects what can be done with the SFP+ slots on > > Macchiatobin boards today. > > This all sounds very hardware dependent. So we are going to need some > more DT properties i guess. Have you thought about this? I don't see how DT properties help. DT properties can't stop you plugging in a SFP module into a SFP+ slot with too strong pullups and corrupting the EEPROM on the SFP module. There's nothing that really tells you that the module is SFP or SFP+, and if we wanted to guess, we'd need to read the EEPROM... but reading the EEPROM might corrupt it - catch-22. > Maybe we need to add compatibles sff,sfp+ and sff,sff+ ? Indicate the > board is capable of the higher speeds? And when sfp+/sff+ is used, > maybe a boolean to indicate it is also sff/sfp compatible? I've had a patch like that hanging around for a few years. I've yet to find anything that could benefit from it or use it to make some kind of decision in the code. > sfp_select_interface() can then look at these properties and return > PHY_INTERFACE_MODE_NA if the board is not capable of supporting the > module? > > Would it even make sense to make the PHY interface more like the MAC > interface? A validate function to indicate what it is capable of? A > configure function to configure its mode towards the SFP? I don't see how any of that helps. The problem is not whether the PHY interface mode is supported it not - on the Macchiatobin DS, the 88x3310 PHY certainly supports 10GBASE-R and 1000BASE-X via the fiber port. So it's entirely possible to configure the 3310 to drive a 1G fiber SFP. The problem on the Macchiatobin is the I2C pull-up resistors, which either prevent a SFP module being readable or end up corrupting the SFP module EEPROM in the process of trying (and probably failing) to read it. There is nothing the kernel can do to work around that.
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 3b99882692e3..1bf13017d288 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -26,6 +26,7 @@ #include <linux/hwmon.h> #include <linux/marvell_phy.h> #include <linux/phy.h> +#include <linux/sfp.h> #define MV_PHY_ALASKA_NBT_QUIRK_MASK 0xfffffffe #define MV_PHY_ALASKA_NBT_QUIRK_REV (MARVELL_PHY_ID_88X3310 | 0xa) @@ -206,6 +207,28 @@ static int mv3310_hwmon_probe(struct phy_device *phydev) } #endif +static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id) +{ + struct phy_device *phydev = upstream; + __ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, }; + phy_interface_t iface; + + sfp_parse_support(phydev->sfp_bus, id, support); + iface = sfp_select_interface(phydev->sfp_bus, id, support); + + if (iface != PHY_INTERFACE_MODE_10GKR) { + dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n"); + return -EINVAL; + } + return 0; +} + +static const struct sfp_upstream_ops mv3310_sfp_ops = { + .attach = phy_sfp_attach, + .detach = phy_sfp_detach, + .module_insert = mv3310_sfp_insert, +}; + static int mv3310_probe(struct phy_device *phydev) { struct mv3310_priv *priv; @@ -236,7 +259,7 @@ static int mv3310_probe(struct phy_device *phydev) if (ret) return ret; - return 0; + return phy_sfp_probe(phydev, &mv3310_sfp_ops); } static int mv3310_suspend(struct phy_device *phydev)
Add support for SFP+ cages to the Marvell 10G PHY driver. This is slightly complicated by the way phylib works in that we need to use a multi-step process to attach the SFP bus, and we also need to track the phylink state machine to know when the module's transmit disable signal should change state. With appropriate DT changes, this allows the SFP+ canges on the Macchiatobin platform to be functional. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/marvell10g.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)