[PATCH net-next v2 0/3] Add support for SFPs behind PHYs

[PATCH net-next v2 0/3] Add support for SFPs behind PHYs

[Russell King - ARM Linux admin]

This series adds partial support for SFP cages connected to PHYs,
specifically optical SFPs.

We add core infrastructure to phylib for this, and arrange for
minimal code in the PHY driver - currently, this is code to verify
that the module is one that we can support for Marvell 10G PHYs.

v2: add yaml binding patch

 .../bindings/net/ethernet-controller.yaml          |  5 ++
 .../devicetree/bindings/net/ethernet-phy.yaml      |  5 ++
 drivers/net/phy/marvell10g.c                       | 25 +++++++-
 drivers/net/phy/phy.c                              |  7 +++
 drivers/net/phy/phy_device.c                       | 66 ++++++++++++++++++++++
 include/linux/phy.h                                | 11 ++++
 6 files changed, 118 insertions(+), 1 deletion(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH net-next v2 1/3] dt-bindings: net: add ethernet controller and phy sfp property

[Russell King]

Document the missing sfp property for ethernet controllers (which
has existed for some time) which is being extended to ethernet PHYs.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/net/ethernet-controller.yaml         | 5 +++++
 Documentation/devicetree/bindings/net/ethernet-phy.yaml      | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 0e7c31794ae6..ac471b60ed6a 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -121,6 +121,11 @@ title: Ethernet Controller Generic Binding
       and is useful for determining certain configuration settings
       such as flow control thresholds.
 
+  sfp:
+    $ref: /schemas/types.yaml#definitions/phandle
+    description:
+      Specifies a reference to a node representing a SFP cage.
+
   tx-fifo-depth:
     $ref: /schemas/types.yaml#definitions/uint32
     description:
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index f70f18ff821f..8927941c74bb 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -153,6 +153,11 @@ title: Ethernet PHY Generic Binding
       Delay after the reset was deasserted in microseconds. If
       this property is missing the delay will be skipped.
 
+  sfp:
+    $ref: /schemas/types.yaml#definitions/phandle
+    description:
+      Specifies a reference to a node representing a SFP cage.
+
 required:
   - reg
 
-- 
2.20.1

[PATCH net-next v2 2/3] net: phy: add core phylib sfp support

[Russell King]

Add core phylib help for supporting SFP sockets on PHYs.  This provides
a mechanism to inform the SFP layer about PHY up/down events, and also
unregister the SFP bus when the PHY is going away.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c        |  7 ++++
 drivers/net/phy/phy_device.c | 66 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 11 ++++++
 3 files changed, 84 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0a314cf45408..cb9ab24ee898 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -23,6 +23,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
+#include <linux/sfp.h>
 #include <linux/workqueue.h>
 #include <linux/mdio.h>
 #include <linux/io.h>
@@ -866,6 +867,9 @@ void phy_stop(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
+	if (phydev->sfp_bus)
+		sfp_upstream_stop(phydev->sfp_bus);
+
 	phydev->state = PHY_HALTED;
 
 	mutex_unlock(&phydev->lock);
@@ -900,6 +904,9 @@ void phy_start(struct phy_device *phydev)
 		goto out;
 	}
 
+	if (phydev->sfp_bus)
+		sfp_upstream_start(phydev->sfp_bus);
+
 	/* if phy was suspended, bring the physical link up again */
 	__phy_resume(phydev);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9ddfb8b0ce04..ff47ec245100 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -27,6 +27,7 @@
 #include <linux/bitmap.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
+#include <linux/sfp.h>
 #include <linux/mdio.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
@@ -1174,6 +1175,65 @@ phy_standalone_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(phy_standalone);
 
+/**
+ * phy_sfp_attach - attach the SFP bus to the PHY upstream network device
+ * @upstream: pointer to the phy device
+ * @bus: sfp bus representing cage being attached
+ *
+ * This is used to fill in the sfp_upstream_ops .attach member.
+ */
+void phy_sfp_attach(void *upstream, struct sfp_bus *bus)
+{
+	struct phy_device *phydev = upstream;
+
+	if (phydev->attached_dev)
+		phydev->attached_dev->sfp_bus = bus;
+	phydev->sfp_bus_attached = true;
+}
+EXPORT_SYMBOL(phy_sfp_attach);
+
+/**
+ * phy_sfp_detach - detach the SFP bus from the PHY upstream network device
+ * @upstream: pointer to the phy device
+ * @bus: sfp bus representing cage being attached
+ *
+ * This is used to fill in the sfp_upstream_ops .detach member.
+ */
+void phy_sfp_detach(void *upstream, struct sfp_bus *bus)
+{
+	struct phy_device *phydev = upstream;
+
+	if (phydev->attached_dev)
+		phydev->attached_dev->sfp_bus = NULL;
+	phydev->sfp_bus_attached = false;
+}
+EXPORT_SYMBOL(phy_sfp_detach);
+
+/**
+ * phy_sfp_probe - probe for a SFP cage attached to this PHY device
+ * @phydev: Pointer to phy_device
+ * @ops: SFP's upstream operations
+ */
+int phy_sfp_probe(struct phy_device *phydev,
+		  const struct sfp_upstream_ops *ops)
+{
+	struct sfp_bus *bus;
+	int ret;
+
+	if (phydev->mdio.dev.fwnode) {
+		bus = sfp_bus_find_fwnode(phydev->mdio.dev.fwnode);
+		if (IS_ERR(bus))
+			return PTR_ERR(bus);
+
+		phydev->sfp_bus = bus;
+
+		ret = sfp_bus_add_upstream(bus, phydev, ops);
+		sfp_bus_put(bus);
+	}
+	return 0;
+}
+EXPORT_SYMBOL(phy_sfp_probe);
+
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
@@ -1249,6 +1309,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (dev) {
 		phydev->attached_dev = dev;
 		dev->phydev = phydev;
+
+		if (phydev->sfp_bus_attached)
+			dev->sfp_bus = phydev->sfp_bus;
 	}
 
 	/* Some Ethernet drivers try to connect to a PHY device before
@@ -2333,6 +2396,9 @@ static int phy_remove(struct device *dev)
 	phydev->state = PHY_DOWN;
 	mutex_unlock(&phydev->lock);
 
+	sfp_bus_del_upstream(phydev->sfp_bus);
+	phydev->sfp_bus = NULL;
+
 	if (phydev->drv && phydev->drv->remove) {
 		phydev->drv->remove(phydev);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1f9a141ccfb4..9753d9f28d73 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -203,6 +203,8 @@ static inline const char *phy_modes(phy_interface_t interface)
 
 struct device;
 struct phylink;
+struct sfp_bus;
+struct sfp_upstream_ops;
 struct sk_buff;
 
 /*
@@ -342,6 +344,8 @@ struct phy_c45_device_ids {
  * dev_flags: Device-specific flags used by the PHY driver.
  * irq: IRQ number of the PHY's interrupt (-1 if none)
  * phy_timer: The timer for handling the state machine
+ * sfp_bus_attached: flag indicating whether the SFP bus has been attached
+ * sfp_bus: SFP bus attached to this PHY's fiber port
  * attached_dev: The attached enet driver's device instance ptr
  * adjust_link: Callback for the enet controller to respond to
  * changes in the link state.
@@ -430,6 +434,9 @@ struct phy_device {
 
 	struct mutex lock;
 
+	/* This may be modified under the rtnl lock */
+	bool sfp_bus_attached;
+	struct sfp_bus *sfp_bus;
 	struct phylink *phylink;
 	struct net_device *attached_dev;
 
@@ -1015,6 +1022,10 @@ int phy_suspend(struct phy_device *phydev);
 int phy_resume(struct phy_device *phydev);
 int __phy_resume(struct phy_device *phydev);
 int phy_loopback(struct phy_device *phydev, bool enable);
+void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
+void phy_sfp_detach(void *upstream, struct sfp_bus *bus);
+int phy_sfp_probe(struct phy_device *phydev,
+	          const struct sfp_upstream_ops *ops);
 struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
 			      phy_interface_t interface);
 struct phy_device *phy_find_first(struct mii_bus *bus);
-- 
2.20.1

[PATCH net-next v2 3/3] net: phy: marvell10g: add SFP+ support

[Russell King]

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(-)

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)
-- 
2.20.1

Re: [PATCH net-next v2 1/3] dt-bindings: net: add ethernet controller and phy sfp property

[Rob Herring]

On Fri, Nov 15, 2019 at 1:56 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:
>
> Document the missing sfp property for ethernet controllers (which
> has existed for some time) which is being extended to ethernet PHYs.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/net/ethernet-controller.yaml         | 5 +++++
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml      | 5 +++++
>  2 files changed, 10 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH net-next v2 3/3] net: phy: marvell10g: add SFP+ support

[Andrew Lunn]

> +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

Re: [PATCH net-next v2 1/3] dt-bindings: net: add ethernet controller and phy sfp property

[Andrew Lunn]

On Fri, Nov 15, 2019 at 07:56:46PM +0000, Russell King wrote:
> Document the missing sfp property for ethernet controllers (which
> has existed for some time) which is being extended to ethernet PHYs.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 2/3] net: phy: add core phylib sfp support

[Andrew Lunn]

On Fri, Nov 15, 2019 at 07:56:51PM +0000, Russell King wrote:
> Add core phylib help for supporting SFP sockets on PHYs.  This provides
> a mechanism to inform the SFP layer about PHY up/down events, and also
> unregister the SFP bus when the PHY is going away.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 3/3] net: phy: marvell10g: add SFP+ support

[Russell King - ARM Linux admin]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 3/3] net: phy: marvell10g: add SFP+ support

[Andrew Lunn]

> 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

Re: [PATCH net-next v2 3/3] net: phy: marvell10g: add SFP+ support

[Russell King - ARM Linux admin]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 0/3] Add support for SFPs behind PHYs

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Fri, 15 Nov 2019 19:53:39 +0000

> This series adds partial support for SFP cages connected to PHYs,
> specifically optical SFPs.
> 
> We add core infrastructure to phylib for this, and arrange for
> minimal code in the PHY driver - currently, this is code to verify
> that the module is one that we can support for Marvell 10G PHYs.
> 
> v2: add yaml binding patch

I've applied this series to net-next.

Andrew, if you still have concerns about capability detection etc.
please keep following up in this patch #3 thread.

Thanks.