Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sean Anderson <sean.anderson@seco.com>
To:     netdev@vger.kernel.org, "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Sean Anderson <sean.anderson@seco.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>
Subject: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks
Date:   Mon,  4 Oct 2021 15:15:21 -0400
Message-Id: <20211004191527.1610759-11-sean.anderson@seco.com>
In-Reply-To: <20211004191527.1610759-1-sean.anderson@seco.com>
References: <20211004191527.1610759-1-sean.anderson@seco.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?R1Vw3TWKBOCkImL1fNxmYe/XonikpdEjeZRY19Uw6TDxscv+AWp7EWAzV4gb?=
 =?us-ascii?Q?diV1kbeJ0qcIMqF8cSKfWKBCYFe9625cP17UDuoriKp2og59ojy1nlNmdv6U?=
 =?us-ascii?Q?1gw/nWze595y0jc1Hhp/2JK1SsOEKKZrYyPp3HrC5BNwJ5i2/BFDpPvrmYbk?=
 =?us-ascii?Q?idTO0InM9px9+D+/+xE2lsP7mBK2I94g1sn0F6Ph5LckcgI1CBAXJCE8OKDM?=
 =?us-ascii?Q?XVQU10weu66X8c92MrAEaHcmEvkCrc8ml+j2vQ014EOhRWoS79fRkbIgaKEu?=
 =?us-ascii?Q?IjhY7gNGC/+lVI9Brm9qJ5PE1q/IR9ztc8Db6u+OtI2yTHhXx9FSl8q6XhKt?=
 =?us-ascii?Q?UpBZAA1IB/6HyWyBboLGT3ao44Ej/OHypbCcGFSSYG0IsCcuhy4KWd9B1nut?=
 =?us-ascii?Q?k+9X1Wi5qgie24cayV78uG/8HhKj9aauckFP7pdSaUaARL+eel3hsLeOi+SJ?=
 =?us-ascii?Q?qkKDw/zbwkaPIzWgBsj2zwpUMKdXQZSjNXQBBj5hdNT0OjpAr2mQ8wy1zo4y?=
 =?us-ascii?Q?buMgIknInReABbSKqKxEaVCe6LCbxpmazFMZ1wyLz4vGYyIx39xHbHCfIPwk?=
 =?us-ascii?Q?NwWAJ1QJyx0bhFHwXz3xe86rs3j7Ylaim3z1N6M/wfr8U4Ct4AXhekg0cQ7t?=
 =?us-ascii?Q?Ik33hXnS/oHas5lk665FEyDR31+2VVUNLM3VuCu/cOkmKYUtpIA3Ym1Ai0Et?=
 =?us-ascii?Q?2aBXn7DjKFd9ZDyIE2Q7aRPCRM4Qz3LCQTw486cqZg9YNIA31SQR5aElX5mG?=
 =?us-ascii?Q?Fe7LE4lx4nLkSn0YAb9jjnCNvJm930/eLp+OHGts8ELPPaJ6B8JODXcOdaTe?=
 =?us-ascii?Q?1osANCS0VteWuohU9n4DcP8GxokUVZCWXMDC1VhxZLrC7XP+I2+dKnxOfyFn?=
 =?us-ascii?Q?HYtJqx+T9HX1bsUV3GvjJuNJS6oCgTAk8VlWtEBwL+MgFXeLZpmYGwXsybL7?=
 =?us-ascii?Q?ua99xVIT51+qNfFnMSIRwnAySE8eekB/I8hygmIgG1i/n+6fns5eRAdZvwQW?=
 =?us-ascii?Q?OIA25eLTy5ze4EjP4xxZfKuzZjiVa8IGkaxt29rcnfUgGhc1GIu1mbYT+T8W?=
 =?us-ascii?Q?is8Fe3VC6x5/fER7a6LX2mz8Q6cDDG7OM2sp+JmEGMbGAyqdaUF66qLipWQo?=
 =?us-ascii?Q?CqbBGSPwEtVhtJrZWuH2ZRa5hMh+j/xsntgOUeIeCRGshJYnl1lKZ4xr8Hyi?=
 =?us-ascii?Q?GyXhWBCo8XsEz0zG0g2ttclEHoxZqqscKeH5KOUBVAG62v3TaV6GKhmJW2ms?=
 =?us-ascii?Q?MdSMxN81J7Y2ise4G/O2G5rSiD5Iy8uNfTTIk2VCXVyAC7gYQWhMxyBU7W0b?=
 =?us-ascii?Q?qz19q2z1zKLMOTSYoh24xxdD?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a2a52f0-0529-4ede-2c18-08d9876b683e
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4523.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Oct 2021 19:16:03.2970
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: e5t8PtCzxD8Vkmx0NsP5pw+MqypMNeqj6k1cJhop0xjDRzZLAsIn07/inqdm9gFZWowleBqYeNRE5T03l6hK9w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR03MB7434
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

This moves all PCS-related settings to the pcs callbacks. This makes it
easy to support external PCSs. In addition, support for 1000BASE-X is
added (since we need it to set the SGMII mux).

pcs_config should set all registers necessary to bring the link up. In
addition, it needs to keep track of whether it modified anything so that
phylink can restart autonegotiation. config is also the right time to
veto configuration parameters, such as using the wrong interface. This
catches someone trying to use a slower speed (which could be supported
otherwise) after using a faster speed. We can't support this because
phylink doesn't support detaching PCSs.

pcs_link_up is necessary IFF the mode is not in-band and the speed and
duplex are different from what was set in config. However, because the
PCS supports only fixed speed (SPEED_1000 or SPEED_10000) and full
duplex, there is nothing to configure. Therefore, link_up has been
removed.

Now that the autonegotiation is done in pcs_config, we no longer need to
do it in macb_mac_config.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/net/ethernet/cadence/macb_main.c | 214 +++++++++++++++--------
 1 file changed, 138 insertions(+), 76 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index db7acce42a27..08dcccd94f87 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -543,6 +543,7 @@ static void macb_validate(struct phylink_config *config,
 			goto none;
 		fallthrough;
 	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
 		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
 			phylink_set(mask, 1000baseT_Full);
 			phylink_set(mask, 1000baseX_Full);
@@ -571,25 +572,100 @@ static void macb_validate(struct phylink_config *config,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);
 }
 
-static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
-				 phy_interface_t interface, int speed,
-				 int duplex)
-{
-	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
-	u32 config;
-
-	config = gem_readl(bp, USX_CONTROL);
-	config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, config);
-	config = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, config);
-	config &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
-	config |= GEM_BIT(TX_EN);
-	gem_writel(bp, USX_CONTROL, config);
+static inline struct macb *pcs_to_macb(struct phylink_pcs *pcs)
+{
+	return container_of(pcs, struct macb, phylink_pcs);
+}
+
+static void macb_pcs_get_state(struct phylink_pcs *pcs,
+			       struct phylink_link_state *state)
+{
+	struct macb *bp = pcs_to_macb(pcs);
+
+	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
+		state->interface = PHY_INTERFACE_MODE_SGMII;
+	else
+		state->interface = PHY_INTERFACE_MODE_1000BASEX;
+
+	phylink_mii_c22_pcs_decode_state(state, gem_readl(bp, PCSSTS),
+					 gem_readl(bp, PCSANLPBASE));
+}
+
+/**
+ * macb_pcs_config_an() - Configure autonegotiation settings for PCSs
+ * @bp - The macb to operate on
+ * @mode - The autonegotiation mode
+ * @interface - The interface to use
+ * @advertising - The advertisement mask
+ *
+ * This provides common configuration for PCS autonegotiation.
+ *
+ * Context: Call with @bp->lock held.
+ * Return: 1 if any registers were changed; 0 otherwise
+ */
+static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
+			      phy_interface_t interface,
+			      const unsigned long *advertising)
+{
+	bool changed = false;
+	u16 old, new;
+
+	old = gem_readl(bp, PCSANADV);
+	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
+						       old);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, PCSANADV, new);
+	}
+
+	old = new = gem_readl(bp, PCSCNTRL);
+	if (mode == MLO_AN_INBAND)
+		new |= BMCR_ANENABLE;
+	else
+		new &= ~BMCR_ANENABLE;
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, PCSCNTRL, new);
+	}
+	return changed;
+}
+
+static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+			   phy_interface_t interface,
+			   const unsigned long *advertising,
+			   bool permit_pause_to_mac)
+{
+	bool changed = false;
+	struct macb *bp = pcs_to_macb(pcs);
+	u16 old, new;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bp->lock, flags);
+	old = new = gem_readl(bp, NCFGR);
+	if (interface == PHY_INTERFACE_MODE_SGMII) {
+		new |= GEM_BIT(SGMIIEN);
+	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
+		new &= ~GEM_BIT(SGMIIEN);
+	} else {
+		spin_lock_irqsave(&bp->lock, flags);
+		return -EOPNOTSUPP;
+	}
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, NCFGR, new);
+	}
+
+	if (macb_pcs_config_an(bp, mode, interface, advertising))
+		changed = true;
+
+	spin_unlock_irqrestore(&bp->lock, flags);
+	return changed;
 }
 
 static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
 				   struct phylink_link_state *state)
 {
-	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+	struct macb *bp = pcs_to_macb(pcs);
 	u32 val;
 
 	state->speed = SPEED_10000;
@@ -609,70 +685,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
 			       const unsigned long *advertising,
 			       bool permit_pause_to_mac)
 {
-	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+	bool changed;
+	struct macb *bp = pcs_to_macb(pcs);
+	u16 old, new;
+	unsigned long flags;
 
-	gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
-		   GEM_BIT(SIGNAL_OK));
+	if (interface != PHY_INTERFACE_MODE_10GBASER)
+		return -EOPNOTSUPP;
 
-	return 0;
-}
+	spin_lock_irqsave(&bp->lock, flags);
+	old = new = gem_readl(bp, NCR);
+	new |= GEM_BIT(ENABLE_HS_MAC);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, NCFGR, new);
+	}
 
-static void macb_pcs_get_state(struct phylink_pcs *pcs,
-			       struct phylink_link_state *state)
-{
-	state->link = 0;
-}
+	if (macb_pcs_config_an(bp, mode, interface, advertising))
+		changed = true;
 
-static void macb_pcs_an_restart(struct phylink_pcs *pcs)
-{
-	/* Not supported */
-}
+	old = new = gem_readl(bp, USX_CONTROL);
+	new |= GEM_BIT(SIGNAL_OK);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, USX_CONTROL, new);
+	}
 
-static int macb_pcs_config(struct phylink_pcs *pcs,
-			   unsigned int mode,
-			   phy_interface_t interface,
-			   const unsigned long *advertising,
-			   bool permit_pause_to_mac)
-{
-	return 0;
+	old = new = gem_readl(bp, USX_CONTROL);
+	new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new);
+	new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new);
+	new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
+	new |= GEM_BIT(TX_EN);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, USX_CONTROL, new);
+	}
+
+	spin_unlock_irqrestore(&bp->lock, flags);
+	return changed;
 }
 
 static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
 	.pcs_get_state = macb_usx_pcs_get_state,
 	.pcs_config = macb_usx_pcs_config,
-	.pcs_link_up = macb_usx_pcs_link_up,
 };
 
 static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
 	.pcs_get_state = macb_pcs_get_state,
-	.pcs_an_restart = macb_pcs_an_restart,
 	.pcs_config = macb_pcs_config,
 };
 
 static void macb_mac_config(struct phylink_config *config, unsigned int mode,
 			    const struct phylink_link_state *state)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&bp->lock, flags);
-
-	/* Disable AN for SGMII fixed link configuration, enable otherwise.
-	 * Must be written after PCSSEL is set in NCFGR,
-	 * otherwise writes will not take effect.
-	 */
-	if (macb_is_gem(bp) && state->interface == PHY_INTERFACE_MODE_SGMII) {
-		u32 pcsctrl, old_pcsctrl;
-
-		old_pcsctrl = gem_readl(bp, PCSCNTRL);
-		if (mode == MLO_AN_FIXED)
-			pcsctrl = old_pcsctrl & ~GEM_BIT(PCSAUTONEG);
-		else
-			pcsctrl = old_pcsctrl | GEM_BIT(PCSAUTONEG);
-		if (old_pcsctrl != pcsctrl)
-			gem_writel(bp, PCSCNTRL, pcsctrl);
-	}
-
-	spin_unlock_irqrestore(&bp->lock, flags);
+	/* Nothing to do */
 }
 
 static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
@@ -763,20 +829,23 @@ static void macb_mac_link_up(struct phylink_config *config,
 static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
 			    phy_interface_t interface)
 {
+	int set_pcs = 0;
 	struct net_device *ndev = to_net_dev(config->dev);
 	struct macb *bp = netdev_priv(ndev);
 	unsigned long flags;
 	u32 old_ctrl, ctrl;
 	u32 old_ncr, ncr;
 
-	if (interface == PHY_INTERFACE_MODE_10GBASER)
+	if (interface == PHY_INTERFACE_MODE_10GBASER) {
 		bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops;
-	else if (interface == PHY_INTERFACE_MODE_SGMII)
+		set_pcs = 1;
+	} else if (interface == PHY_INTERFACE_MODE_SGMII ||
+		   interface == PHY_INTERFACE_MODE_1000BASEX) {
 		bp->phylink_pcs.ops = &macb_phylink_pcs_ops;
-	else
-		bp->phylink_pcs.ops = NULL;
+		set_pcs = 1;
+	}
 
-	if (bp->phylink_pcs.ops)
+	if (set_pcs)
 		phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
 
 	spin_lock_irqsave(&bp->lock, flags);
@@ -787,21 +856,14 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
 	if (bp->caps & MACB_CAPS_MACB_IS_EMAC) {
 		if (interface == PHY_INTERFACE_MODE_RMII)
 			ctrl |= MACB_BIT(RM9200_RMII);
-	} else if (macb_is_gem(bp)) {
-		ctrl &= ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
-		ncr &= ~GEM_BIT(ENABLE_HS_MAC);
-
-		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
-			ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
-		} else if (state->interface == PHY_INTERFACE_MODE_10GBASER) {
-			ctrl |= GEM_BIT(PCSSEL);
-			ncr |= GEM_BIT(ENABLE_HS_MAC);
-		} else if (bp->caps & MACB_CAPS_MIIONRGMII &&
-			   bp->phy_interface == PHY_INTERFACE_MODE_MII) {
-			ncr |= MACB_BIT(MIIONRGMII);
-		}
+	} else if (bp->caps & MACB_CAPS_MIIONRGMII &&
+		   bp->phy_interface == PHY_INTERFACE_MODE_MII) {
+		ncr |= MACB_BIT(MIIONRGMII);
 	}
 
+	if (macb_is_gem(bp) && set_pcs)
+		ctrl |= GEM_BIT(PCSSEL);
+
 	/* Apply the new configuration, if any */
 	if (old_ctrl ^ ctrl)
 		macb_or_gem_writel(bp, NCFGR, ctrl);
-- 
2.25.1