Re: [PATCH v2 3/3] net: macb: add support for high speed interface

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Mon, Dec 16, 2019 at 01:09:08PM +0000, Russell King - ARM Linux admin wrote:
> On Mon, Dec 16, 2019 at 12:49:59PM +0000, Milind Parab wrote:
> > >> > +	if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
> > >>
> > >> Why bp->phy_interface and not state->interface?
> > 
> > okay, this needs to change to state->interface
> > 
> > >>
> > >> If you don't support selecting between USXGMII and other modes at
> > >> runtime, should macb_validate() be allowing ethtool link modes for
> > >> it when it's different from the configured setting?
> > 
> > We have separate SGMII and USXGMII PCS, which are enabled and programmed 
> > by MAC driver. Also, there are separate low speed (up to 1G) and high 
> > speed MAC which can be programmed though MAC driver. 
> > As long as, PHY (PMA, external to Cadence MAC controller) can handle 
> > this change, GEM can work with interface changes at a runtime.
> > 
> > >>
> > >> > +		if (gem_mac_usx_configure(bp, state) < 0) {
> > >> > +			spin_unlock_irqrestore(&bp->lock, flags);
> > >> > +			phylink_mac_change(bp->phylink, false);
> > >>
> > >> I guess this is the reason you're waiting for the USXGMII block
> > >> to lock - do you not have any way to raise an interrupt when
> > >> something changes with the USXGMII (or for that matter SGMII)
> > >> blocks?  Without that, you're fixed to a single speed.
> > 
> > Yes, we need to wait (poll) until USXGMII block lock is set.
> > Interrupt for USXGMII block lock set event is not supported.
> 
> You should poll for that status. We already have some polling support
> in phylink (in the case of a fixed link using the callback, or a GPIO
> that has no interrupt support) so it probably makes sense to extend
> that functionality for MACs that do not provide status interrupts.

And here's that extension (untested):

8<===
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: phylink: add support for polling MAC PCS

Some MAC PCS blocks are unable to provide interrupts when their status
changes. As we already have support in phylink for polling status, use
this to provide a hook for MACs to enable polling mode.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 Documentation/networking/sfp-phylink.rst |  8 +++--
 drivers/net/phy/phylink.c                | 44 ++++++++++++++++++------
 include/linux/phylink.h                  |  1 +
 3 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/Documentation/networking/sfp-phylink.rst b/Documentation/networking/sfp-phylink.rst
index a5e00a159d21..5695204be09a 100644
--- a/Documentation/networking/sfp-phylink.rst
+++ b/Documentation/networking/sfp-phylink.rst
@@ -115,7 +115,7 @@ this documentation.
     * - Original function
       - Replacement function
     * - phy_start(phydev)
-      - phylink_start(priv->phylink)
+      - phylink_start(priv->phylink) or phylink_start_poll(priv->phylink)
     * - phy_stop(phydev)
       - phylink_stop(priv->phylink)
     * - phy_mii_ioctl(phydev, ifr, cmd)
@@ -251,7 +251,9 @@ this documentation.
 	phylink_mac_change(priv->phylink, link_is_up);
 
     where ``link_is_up`` is true if the link is currently up or false
-    otherwise.
+    otherwise. If a MAC is uanble to provide these interrupts, then
+    :c:func:`phylink_start_poll` should be used in step 5.
+
 
 11. Verify that the driver does not call::
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index fc45657bb9c9..616d208b348e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -964,15 +964,7 @@ static irqreturn_t phylink_link_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-/**
- * phylink_start() - start a phylink instance
- * @pl: a pointer to a &struct phylink returned from phylink_create()
- *
- * Start the phylink instance specified by @pl, configuring the MAC for the
- * desired link mode(s) and negotiation style. This should be called from the
- * network device driver's &struct net_device_ops ndo_open() method.
- */
-void phylink_start(struct phylink *pl)
+static void __phylink_start(struct phylink *pl, bool poll)
 {
 	ASSERT_RTNL();
 
@@ -1014,15 +1006,47 @@ void phylink_start(struct phylink *pl)
 		if (irq <= 0)
 			mod_timer(&pl->link_poll, jiffies + HZ);
 	}
-	if (pl->cfg_link_an_mode == MLO_AN_FIXED && pl->get_fixed_state)
+	if ((pl->cfg_link_an_mode == MLO_AN_FIXED && pl->get_fixed_state) ||
+	    poll)
 		mod_timer(&pl->link_poll, jiffies + HZ);
 	if (pl->phydev)
 		phy_start(pl->phydev);
 	if (pl->sfp_bus)
 		sfp_upstream_start(pl->sfp_bus);
 }
+
+/**
+ * phylink_start() - start a phylink instance
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * Start the phylink instance specified by @pl, configuring the MAC for the
+ * desired link mode(s) and negotiation style. This should be called from the
+ * network device driver's &struct net_device_ops ndo_open() method.
+ */
+void phylink_start(struct phylink *pl)
+{
+	__phylink_start(pl, false);
+}
 EXPORT_SYMBOL_GPL(phylink_start);
 
+/**
+ * phylink_start_poll() - start a phylink instance with polling
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * Start the phylink instance specified by @pl, as per phylink_start(), but
+ * also enable polling mode. This should be used if the MAC has no support
+ * for MAC PCS status interrupts.
+ *
+ * The MAC PCS must ensure that if it detects the link going down, that must
+ * be reported via mac_pcs_get_state() method to ensure proper update of the
+ * MAC.
+ */
+void phylink_start_poll(struct phylink *pl)
+{
+	__phylink_start(pl, true);
+}
+EXPORT_SYMBOL_GPL(phylink_start_poll);
+
 /**
  * phylink_stop() - stop a phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index bedff1a217fe..529fb5ce440d 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -250,6 +250,7 @@ int phylink_fixed_state_cb(struct phylink *,
 void phylink_mac_change(struct phylink *, bool up);
 
 void phylink_start(struct phylink *);
+void phylink_start_poll(struct phylink *pl);
 void phylink_stop(struct phylink *);
 
 void phylink_ethtool_get_wol(struct phylink *, struct ethtool_wolinfo *);
-- 
2.20.1


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