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

Commit Message

Klaus Kudielka Dec. 28, 2018, 11:14 a.m. UTC

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.

Comments

Andrew Lunn Dec. 28, 2018, 11:28 p.m. UTC | #1

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

Patch

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