Date:   Sun, 30 Dec 2018 09:25:53 +0100
From:   Klaus Kudielka <klaus.kudielka@...il.com>
To:     netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Uwe Kleine-König <uwe@...ine-koenig.org>,
        Tomas Hlavacek <tmshlvck@...il.com>
Subject: [RFC] phylink: support for devices with MAC sharing SFP cage & PHY
 (e.g. Turris Omnia)

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,

Powered by blists - more mailing lists