[RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[Klaus Kudielka]

Hello,

I own a Turris Omnia, which has basic support included in the
Linux kernel (armada-385-turris-omnia.dts).

Apart from the 88E1514 PHY specified for eth2, the device also
features an SFP cage, which uses the same SGMII of the Armada
385 SoC (a 2:1 multiplexer is driven by the MOD_DEF0 signal).

Details can be found on page 5 of
https://doc.turris.cz/doc/_media/rtrom01-schema.pdf

As far as I understand the situation, phylink does not support
such an architecture at the moment. This may be the reason why
the SFP cage was never specified in the Turris Omnia DTS.

Questions:
- Is anybody already working on a solution?
- If not, would something like the patch below be acceptable?
- Any other suggestions, how to get this supported?

Summary of the patch, which is based on the device tree
published by Tomas (and my interpretation of phylink.c):

I'm specifying *both* the SFP cage and the PHY in the node of
the eth2 MAC. In phylink_of_connect, if SFP module is already
present: Ignore any PHY specified by the devicetree. After
SFP removal, restore the original (device tree based)
configuration, which was determined by phylink_create et al.
(I don't think that this would harm the generic use case)

With this, "hot" insertion and removal work, as long as the
network interface is down. I tested this successfully on my
Turris Omnia, on a relatively recent openwrt-master
(4.14 kernel). SFP module is a TP-LINK TL-SM321B.

Insertion/removal in the "up" state would require more
work. If this is considered necessary, I can give it a try.

I am sorry, the patch is against the 4.14 (openwrt) kernel,
but you get the basic idea.

--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -115,6 +115,16 @@
              };
          };
      };
+
+    sfp: sfp {
+        compatible = "sff,sfp";
+        i2c-bus = <&i2csfp>;
+        tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
+        tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
+        rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
+        los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
+        mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
+    };
  };

  /* Connected to 88E6176 switch, port 6 */
@@ -148,6 +158,7 @@
      status = "okay";
      phy-mode = "sgmii";
      phy = <&phy1>;
+    sfp = <&sfp>;
  };

  &i2c0 {
@@ -210,7 +221,7 @@
              /* routed to PCIe2 connector (CN62A) */
          };

-        i2c@4 {
+        i2csfp: i2c@4 {
              #address-cells = <1>;
              #size-cells = <0>;
              reg = <4>;
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -58,6 +58,12 @@ struct phylink {
      bool mac_link_dropped;

      struct sfp_bus *sfp_bus;
+
+    /* Things to remember across a module insertion/removal cycle */
+    u8 default_an_mode;
+    u8 default_port;
+    struct phylink_link_state default_config;
+    bool sfp_module_present;
  };

  static inline void linkmode_zero(unsigned long *dst)
@@ -680,6 +686,10 @@ int phylink_of_phy_connect(struct phylin
      if (pl->link_an_mode == MLO_AN_FIXED)
          return 0;

+    /* If SFP module present, we do not need a separate PHY */
+    if (pl->sfp_module_present)
+        return 0;
+
      phy_node = of_parse_phandle(dn, "phy-handle", 0);
      if (!phy_node)
          phy_node = of_parse_phandle(dn, "phy", 0);
@@ -1371,6 +1381,11 @@ static int phylink_sfp_module_insert(voi
      if (mode == MLO_AN_8023Z && pl->phydev)
          return -EINVAL;

+    pl->default_an_mode = pl->link_an_mode;
+    pl->default_port = pl->link_port;
+    pl->default_config = pl->link_config;
+    pl->sfp_module_present = true;
+
      changed = !bitmap_equal(pl->supported, support,
                  __ETHTOOL_LINK_MODE_MASK_NBITS);
      if (changed) {
@@ -1399,6 +1414,16 @@ static int phylink_sfp_module_insert(voi
      return ret;
  }

+static void phylink_sfp_module_remove(void *upstream)
+{
+    struct phylink *pl = upstream;
+
+    pl->link_an_mode = pl->default_an_mode;
+    pl->link_port = pl->default_port;
+    pl->link_config = pl->default_config;
+    pl->sfp_module_present = false;
+}
+
  static void phylink_sfp_link_down(void *upstream)
  {
      struct phylink *pl = upstream;
@@ -1432,6 +1457,7 @@ static void phylink_sfp_disconnect_phy(v

  static const struct sfp_upstream_ops sfp_phylink_ops = {
      .module_insert = phylink_sfp_module_insert,
+    .module_remove = phylink_sfp_module_remove,
      .link_up = phylink_sfp_link_up,
      .link_down = phylink_sfp_link_down,
      .connect_phy = phylink_sfp_connect_phy,

Re: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[Andrew Lunn]

On Sun, Dec 30, 2018 at 09:25:53AM +0100, Klaus Kudielka wrote:
> Hello,

Hi Klaus

Thanks for reposting on the correct list.

> Apart from the 88E1514 PHY specified for eth2, the device also
> features an SFP cage, which uses the same SGMII of the Armada
> 385 SoC (a 2:1 multiplexer is driven by the MOD_DEF0 signal).

Some of the Marvell Ethernet switches have a similar setup. Some ports
have both an internal PHY and a SERDES port which can be connected to
an SFP cage. Whichever gets link first is connected to the MAC.  I can
also imagine a slightly different setup to what you have, the MUX is
controlled via a GPIO, not MOD_DEF0, so again, you want both active
until one gets link, although SFP LOS is not very reliable for this.

So we need a generic solution to this. In your case, the PHY should be
configured down when the SFP is present, but then made active when the
SFP is unplugged. For the Marvell switch, we need both SFP and PHY
active until one gets a link, and then the other configured down. So
it sounds like we need some board specific code involved to implement
the board specific parts.

    Andrew

Re: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[Florian Fainelli]

Le 12/30/18 à 1:51 AM, Andrew Lunn a écrit :
> On Sun, Dec 30, 2018 at 09:25:53AM +0100, Klaus Kudielka wrote:
>> Hello,
> 
> Hi Klaus
> 
> Thanks for reposting on the correct list.
> 
>> Apart from the 88E1514 PHY specified for eth2, the device also
>> features an SFP cage, which uses the same SGMII of the Armada
>> 385 SoC (a 2:1 multiplexer is driven by the MOD_DEF0 signal).
> 
> Some of the Marvell Ethernet switches have a similar setup. Some ports
> have both an internal PHY and a SERDES port which can be connected to
> an SFP cage. Whichever gets link first is connected to the MAC.  I can
> also imagine a slightly different setup to what you have, the MUX is
> controlled via a GPIO, not MOD_DEF0, so again, you want both active
> until one gets link, although SFP LOS is not very reliable for this.
> 
> So we need a generic solution to this. In your case, the PHY should be
> configured down when the SFP is present, but then made active when the
> SFP is unplugged. For the Marvell switch, we need both SFP and PHY
> active until one gets a link, and then the other configured down. So
> it sounds like we need some board specific code involved to implement
> the board specific parts.

There are at least two user space configuration knobs that we could
extend to support dynamically switching between copper and fiber interface:

ethtool -s <iface> port tp|mii|fiber..

or

devlink port type eth media copper|sfp

Would that work here?
-- 
Florian

Re: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[Andrew Lunn]

On Sun, Dec 30, 2018 at 02:35:32PM -0800, Florian Fainelli wrote:
> Le 12/30/18 à 1:51 AM, Andrew Lunn a écrit :
> > On Sun, Dec 30, 2018 at 09:25:53AM +0100, Klaus Kudielka wrote:
> >> Hello,
> > 
> > Hi Klaus
> > 
> > Thanks for reposting on the correct list.
> > 
> >> Apart from the 88E1514 PHY specified for eth2, the device also
> >> features an SFP cage, which uses the same SGMII of the Armada
> >> 385 SoC (a 2:1 multiplexer is driven by the MOD_DEF0 signal).
> > 
> > Some of the Marvell Ethernet switches have a similar setup. Some ports
> > have both an internal PHY and a SERDES port which can be connected to
> > an SFP cage. Whichever gets link first is connected to the MAC.  I can
> > also imagine a slightly different setup to what you have, the MUX is
> > controlled via a GPIO, not MOD_DEF0, so again, you want both active
> > until one gets link, although SFP LOS is not very reliable for this.
> > 
> > So we need a generic solution to this. In your case, the PHY should be
> > configured down when the SFP is present, but then made active when the
> > SFP is unplugged. For the Marvell switch, we need both SFP and PHY
> > active until one gets a link, and then the other configured down. So
> > it sounds like we need some board specific code involved to implement
> > the board specific parts.
> 
> There are at least two user space configuration knobs that we could
> extend to support dynamically switching between copper and fiber interface:
> 
> ethtool -s <iface> port tp|mii|fiber..
> 
> or
> 
> devlink port type eth media copper|sfp
> 
> Would that work here?

Hi Florian

I don't think so for the Turris Omnia. There is no software control
over this, the multiplexor is directly controlled via the SFP
hardware.

For the Marvel switch, it probably is an option. We have software
control of the two 'PHYS', so can control which is up and can get
link.

	Andrew

Re: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[Russell King - ARM Linux]

On Sun, Dec 30, 2018 at 02:35:32PM -0800, Florian Fainelli wrote:
> There are at least two user space configuration knobs that we could
> extend to support dynamically switching between copper and fiber interface:
> 
> ethtool -s <iface> port tp|mii|fiber..

How does that work if you have a SFP plugged in that gives you a RJ45
socket (and hence shows "IP" in the ethtool output) ?

-- 
RMK's Patch system: http://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: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[Klaus Kudielka]

On 30.12.18 10:51, Andrew Lunn wrote:

> Some of the Marvell Ethernet switches have a similar setup. Some 
> ports have both an internal PHY and a SERDES port which can be 
> connected to an SFP cage. Whichever gets link first is connected to 
> the MAC.

This decision is taken by hardware? I am just wondering how the outcome
would be detected unambiguously by software. (Any documentation?)

> I can also imagine a slightly different setup to what you have, the 
> MUX is controlled via a GPIO, not MOD_DEF0, so again, you want both 
> active until one gets link, although SFP LOS is not very reliable for
> this.

Okay, here the decision for the MUX would be taken by software.

> So we need a generic solution to this. In your case, the PHY should 
> be configured down when the SFP is present, but then made active when
> the SFP is unplugged.

That would be the ideal solution for the Omnia.

> For the Marvell switch, we need both SFP and PHY active until one 
> gets a link, and then the other configured down. So it sounds like
> we need some board specific code involved to implement the board 
> specific parts.

Such a "generic" solution would be restricted (per MAC) to a maximum of 
one SFP (fiber or copper), and one separate PHY, right? The main 
difference between boards would be the switching logic.

Klaus

Re: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[Andrew Lunn]

On Mon, Dec 31, 2018 at 12:24:18PM +0100, Klaus Kudielka wrote:
> On 30.12.18 10:51, Andrew Lunn wrote:
> 
> >Some of the Marvell Ethernet switches have a similar setup. Some ports
> >have both an internal PHY and a SERDES port which can be connected to an
> >SFP cage. Whichever gets link first is connected to the MAC.
> 
> This decision is taken by hardware? I am just wondering how the outcome
> would be detected unambiguously by software. (Any documentation?)

The Marvell documentation is not public. I would have to check, but i
think there is a bit which tells you. But as Florian pointed out, this
can be indirectly controlled from software, in that a PHY which is
configured down will never get link, in the same way an SFP with its
receiver disabled will never get link. So software to enable one or
the other would work.

> Such a "generic" solution would be restricted (per MAC) to a maximum of one
> SFP (fiber or copper), and one separate PHY, right? The main difference
> between boards would be the switching logic.

Yes, that seems a sensible restriction.

     Andrew

Re: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)

[Klaus Kudielka]

On 31.12.18 18:43, Andrew Lunn wrote:
> 
> The Marvell documentation is not public. I would have to check, but
> i think there is a bit which tells you. But as Florian pointed out,
> this can be indirectly controlled from software, in that a PHY which
> is configured down will never get link, in the same way an SFP with
> its receiver disabled will never get link. So software to enable one
> or the other would work.
> 
>> Such a "generic" solution would be restricted (per MAC) to a
>> maximum of one SFP (fiber or copper), and one separate PHY, right?
>> The main difference between boards would be the switching logic.
> 
> Yes, that seems a sensible restriction.
> 

I might be able come up with a re-work of phylink.c, such that it can 
manage two independent link configurations, and reconfigure the MAC upon 
a switch. The actual switching would be modeled in the various existing 
callbacks, based on a board-specific recipe (for the moment 
sfp-module-present or first-link). ethtool will probably just see the 
"active" link (i.e. the PHY/SFP connected to the MAC).

As far as I can see, this would make both the Omnia SFP and the Marvell 
switches work as they were designed, without any interface change.

Additional userspace-driven control could be added at a later time, if 
it turns out to be necessary.

[RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)?

[Klaus Kudielka]

Hello,

I own a Turris Omnia, which has basic support included in the
Linux kernel (armada-385-turris-omnia.dts).

Apart from the 88E1514 PHY specified for eth2, the device also
features an SFP cage, which uses the same SGMII of the Armada
385 SoC, using a 2:1 multiplexer driven by the MOD_DEF0 signal.
Details can be found on page 5 of
https://doc.turris.cz/doc/_media/rtrom01-schema.pdf.

As far as I understand the situation, phylink does not support
such an architecture at the moment, which is the reason why the
SFP cage was never specified in the Turris Omnia's DTS.

Questions:
- Is anybody already working on a solution?
- If not, would something like the patch below be acceptable?
- Any other suggestions, how to get this supported?

Summary of the patch below, whis is based on device tree
published by Tomas (and my interpretation of phylink.c):

I'm specifying *both* the SFP cage and the PHY in the node of
the eth2 MAC. In phylink_of_connect, if SFP module is already
present: Ignore any PHY specified by the devicetree. After
SFP removal, restore the original (device tree based)
configuration, which was determined by phylink_create et al.
(I don't think that this would harm the generic use case)

With this, "hot" insertion and removal work, as long as the
network interface is down. I tested this successfully on my
Turris Omnia, on a relatively recent openwrt-master
(4.14 kernel). SFP module is a TP-LINK TL-SM321B.

"Hot" insertion/removal in the "up" state would require more
work.

I am sorry, the patch below is against the 4.14 (openwrt)
kernel, but you get the basic idea.

--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -115,6 +115,16 @@
              };
          };
      };
+
+    sfp: sfp {
+        compatible = "sff,sfp";
+        i2c-bus = <&i2csfp>;
+        tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
+        tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
+        rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
+        los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
+        mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
+    };
  };

  /* Connected to 88E6176 switch, port 6 */
@@ -148,6 +158,7 @@
      status = "okay";
      phy-mode = "sgmii";
      phy = <&phy1>;
+    sfp = <&sfp>;
  };

  &i2c0 {
@@ -210,7 +221,7 @@
              /* routed to PCIe2 connector (CN62A) */
          };

-        i2c@4 {
+        i2csfp: i2c@4 {
              #address-cells = <1>;
              #size-cells = <0>;
              reg = <4>;
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -58,6 +58,12 @@ struct phylink {
      bool mac_link_dropped;

      struct sfp_bus *sfp_bus;
+
+    /* Things to remember across a module insertion/removal cycle */
+    u8 default_an_mode;
+    u8 default_port;
+    struct phylink_link_state default_config;
+    bool sfp_module_present;
  };

  static inline void linkmode_zero(unsigned long *dst)
@@ -680,6 +686,10 @@ int phylink_of_phy_connect(struct phylin
      if (pl->link_an_mode == MLO_AN_FIXED)
          return 0;

+    /* If SFP module present, we do not need a separate PHY */
+    if (pl->sfp_module_present)
+        return 0;
+
      phy_node = of_parse_phandle(dn, "phy-handle", 0);
      if (!phy_node)
          phy_node = of_parse_phandle(dn, "phy", 0);
@@ -1371,6 +1381,11 @@ static int phylink_sfp_module_insert(voi
      if (mode == MLO_AN_8023Z && pl->phydev)
          return -EINVAL;

+    pl->default_an_mode = pl->link_an_mode;
+    pl->default_port = pl->link_port;
+    pl->default_config = pl->link_config;
+    pl->sfp_module_present = true;
+
      changed = !bitmap_equal(pl->supported, support,
                  __ETHTOOL_LINK_MODE_MASK_NBITS);
      if (changed) {
@@ -1399,6 +1414,16 @@ static int phylink_sfp_module_insert(voi
      return ret;
  }

+static void phylink_sfp_module_remove(void *upstream)
+{
+    struct phylink *pl = upstream;
+
+    pl->link_an_mode = pl->default_an_mode;
+    pl->link_port = pl->default_port;
+    pl->link_config = pl->default_config;
+    pl->sfp_module_present = false;
+}
+
  static void phylink_sfp_link_down(void *upstream)
  {
      struct phylink *pl = upstream;
@@ -1432,6 +1457,7 @@ static void phylink_sfp_disconnect_phy(v

  static const struct sfp_upstream_ops sfp_phylink_ops = {
      .module_insert = phylink_sfp_module_insert,
+    .module_remove = phylink_sfp_module_remove,
      .link_up = phylink_sfp_link_up,
      .link_down = phylink_sfp_link_down,
      .connect_phy = phylink_sfp_connect_phy,


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY (e.g. Turris Omnia)?

[Andrew Lunn]

On Fri, Dec 28, 2018 at 12:14:55PM +0100, Klaus Kudielka wrote:
> Hello,

Hi Klaus

This is a networking issue, not an ARM issue. Please discuss this on
the netdev list, and Cc: the phylib and phylink maintainers.

     Thanks
	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel