From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4AC0EC433E0 for ; Thu, 11 Jun 2020 08:58:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 15D8620656 for ; Thu, 11 Jun 2020 08:58:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="U+bauA+Y" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726947AbgFKI65 (ORCPT ); Thu, 11 Jun 2020 04:58:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36688 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726833AbgFKI64 (ORCPT ); Thu, 11 Jun 2020 04:58:56 -0400 Received: from mail-ej1-x643.google.com (mail-ej1-x643.google.com [IPv6:2a00:1450:4864:20::643]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0D554C03E96F; Thu, 11 Jun 2020 01:58:56 -0700 (PDT) Received: by mail-ej1-x643.google.com with SMTP id gl26so5625845ejb.11; Thu, 11 Jun 2020 01:58:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=zRc4m9iOEbI5hoMKMjsFhJKGdjgdHacxqemabglyW6c=; b=U+bauA+Y9j54Fn9NvJEb/9mduwBYK+x47gECfqiSXtMVhMLdxialK1Ktu+HkJKbqqg qXVwJhXxkLNSlsGPmy41g0DGdNwztzGGKYre7jtPz9QkI2HnGlCsonGoGFoeJy4Zz10o tA9tnoflZ6fVRIAVVT9cBt6vjuNEwh1O0yrycWo1LOrZ4krv0e7YkG0qZaAQ5iVEvmsi 3hulYoxqEslAeZD/hbXCSE8Yg0OFt6Gb/oA1Jnv8kq2pFgZa5kpqnyr9jItb91t9k0zY HcpSCfmxSokDIlS6QSB2eoKCyBjWHwy2i0rFs2iH0JRK0CfcxMor+QrxfEcjxxY74jqx fl7A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zRc4m9iOEbI5hoMKMjsFhJKGdjgdHacxqemabglyW6c=; b=T4X3ZvVnqRiRBwLfIbhiC/7gzO+nTOdpKKibzhFym8I6GPrd3N0vdpnOgL+TyK6dOQ ONVbNpNc4UMcqENns/siEV2ppdEAjsuT0e1MCw9jw0XJXL6S1hMbelQbjvA6xwBgpC1y bZF6eMuQ+iN7WtaQlB+EDBmM25DrYRRTsImLzqt9Tkj/0IqBtezy6TT83RqH1+1vn42X MCEiHa0CnyZlnWRDewDnzMEoVpx7KVNSC2zBt3JaBf93jJwXHX1l6689qbOM3gfHg7MO XvME1cjZxCDyNFAbzD/LpV0x2iQnsxuBmcUP/MZRR5rLi+BTR+x08YOuH20Xc4UwaMbr jofw== X-Gm-Message-State: AOAM531udz/aKXA3LV25l527j4VGeedgCr+014J3FMz+6jVh4B4FhOFG Llq2AGjZtIoJTXDFZJcqfXCRa1KyeVccAo0ap+eZU8vz X-Google-Smtp-Source: ABdhPJzn2MdWdohld+76c4IyAD8s0MWErlY1Ynriclp0c/arf06GAP4KYcJy4dggnX7tCZaP3K0BA/1AC0IkSe+US/Q= X-Received: by 2002:a17:906:1d56:: with SMTP id o22mr7276427ejh.406.1591865934537; Thu, 11 Jun 2020 01:58:54 -0700 (PDT) MIME-Version: 1.0 References: <78519bc421a1cb7000a68d05e43c4208b26f37e5.1591816172.git.noodles@earth.li> In-Reply-To: <78519bc421a1cb7000a68d05e43c4208b26f37e5.1591816172.git.noodles@earth.li> From: Vladimir Oltean Date: Thu, 11 Jun 2020 11:58:43 +0300 Message-ID: Subject: Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB To: Jonathan McDowell Cc: Russell King - ARM Linux admin , Andrew Lunn , Vivien Didelot , Florian Fainelli , "David S. Miller" , Jakub Kicinski , netdev , lkml Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jonathan, On Wed, 10 Jun 2020 at 23:19, Jonathan McDowell wrote: > > Update the driver to use the new PHYLINK callbacks, removing the > legacy adjust_link callback. > > Signed-off-by: Jonathan McDowell > --- > drivers/net/dsa/qca8k.c | 309 +++++++++++++++++++++++++++------------- > 1 file changed, 212 insertions(+), 97 deletions(-) > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > index d2b5ab403e06..dcd9e8fa99b6 100644 > --- a/drivers/net/dsa/qca8k.c > +++ b/drivers/net/dsa/qca8k.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -418,55 +419,6 @@ qca8k_mib_init(struct qca8k_priv *priv) > mutex_unlock(&priv->reg_mutex); > } > > -static int > -qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode) > -{ > - u32 reg, val; > - > - switch (port) { > - case 0: > - reg = QCA8K_REG_PORT0_PAD_CTRL; > - break; > - case 6: > - reg = QCA8K_REG_PORT6_PAD_CTRL; > - break; > - default: > - pr_err("Can't set PAD_CTRL on port %d\n", port); > - return -EINVAL; > - } > - > - /* Configure a port to be directly connected to an external > - * PHY or MAC. > - */ > - switch (mode) { > - case PHY_INTERFACE_MODE_RGMII: > - /* RGMII mode means no delay so don't enable the delay */ > - val = QCA8K_PORT_PAD_RGMII_EN; > - qca8k_write(priv, reg, val); > - break; > - case PHY_INTERFACE_MODE_RGMII_ID: > - /* RGMII_ID needs internal delay. This is enabled through > - * PORT5_PAD_CTRL for all ports, rather than individual port > - * registers > - */ > - qca8k_write(priv, reg, > - QCA8K_PORT_PAD_RGMII_EN | > - QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) | > - QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY)); > - qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL, > - QCA8K_PORT_PAD_RGMII_RX_DELAY_EN); > - break; > - case PHY_INTERFACE_MODE_SGMII: > - qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN); > - break; > - default: > - pr_err("xMII mode %d not supported\n", mode); > - return -EINVAL; > - } > - > - return 0; > -} > - > static void > qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable) > { > @@ -639,9 +591,7 @@ static int > qca8k_setup(struct dsa_switch *ds) > { > struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv; > - phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA; > int ret, i; > - u32 mask; > > /* Make sure that port 0 is the cpu port */ > if (!dsa_is_cpu_port(ds, 0)) { > @@ -661,24 +611,9 @@ qca8k_setup(struct dsa_switch *ds) > if (ret) > return ret; > > - /* Initialize CPU port pad mode (xMII type, delays...) */ > - ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode); > - if (ret) { > - pr_err("Can't find phy-mode for master device\n"); > - return ret; > - } > - ret = qca8k_set_pad_ctrl(priv, QCA8K_CPU_PORT, phy_mode); > - if (ret < 0) > - return ret; > - > - /* Enable CPU Port, force it to maximum bandwidth and full-duplex */ > - mask = QCA8K_PORT_STATUS_SPEED_1000 | QCA8K_PORT_STATUS_TXFLOW | > - QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_DUPLEX; > - qca8k_write(priv, QCA8K_REG_PORT_STATUS(QCA8K_CPU_PORT), mask); > + /* Enable CPU Port */ > qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0, > QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN); > - qca8k_port_set_status(priv, QCA8K_CPU_PORT, 1); > - priv->port_sts[QCA8K_CPU_PORT].enabled = 1; > > /* Enable MIB counters */ > qca8k_mib_init(priv); > @@ -693,10 +628,9 @@ qca8k_setup(struct dsa_switch *ds) > qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i), > QCA8K_PORT_LOOKUP_MEMBER, 0); > > - /* Disable MAC by default on all user ports */ > + /* Disable MAC by default on all ports */ > for (i = 1; i < QCA8K_NUM_PORTS; i++) > - if (dsa_is_user_port(ds, i)) > - qca8k_port_set_status(priv, i, 0); > + qca8k_port_set_status(priv, i, 0); > > /* Forward all unknown frames to CPU port for Linux processing */ > qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1, > @@ -713,7 +647,7 @@ qca8k_setup(struct dsa_switch *ds) > QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds)); > } > > - /* Invividual user ports get connected to CPU port only */ > + /* Individual user ports get connected to CPU port only */ > if (dsa_is_user_port(ds, i)) { > int shift = 16 * (i % 2); > > @@ -743,44 +677,223 @@ qca8k_setup(struct dsa_switch *ds) > } > > static void > -qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy) > +qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, > + const struct phylink_link_state *state) > { > struct qca8k_priv *priv = ds->priv; > u32 reg; > > - /* Force fixed-link setting for CPU port, skip others. */ > - if (!phy_is_pseudo_fixed_link(phy)) > + switch (port) { > + case 0: /* 1st CPU port */ > + if (state->interface != PHY_INTERFACE_MODE_RGMII && > + state->interface != PHY_INTERFACE_MODE_RGMII_ID && > + state->interface != PHY_INTERFACE_MODE_SGMII) > + return; > + > + reg = QCA8K_REG_PORT0_PAD_CTRL; > + break; > + case 1: > + case 2: > + case 3: > + case 4: > + case 5: > + /* Internal PHY, nothing to do */ > + return; > + case 6: /* 2nd CPU port / external PHY */ > + if (state->interface != PHY_INTERFACE_MODE_RGMII && > + state->interface != PHY_INTERFACE_MODE_RGMII_ID && > + state->interface != PHY_INTERFACE_MODE_SGMII && > + state->interface != PHY_INTERFACE_MODE_1000BASEX) > + return; > + > + reg = QCA8K_REG_PORT6_PAD_CTRL; > + break; > + default: > + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); > + return; > + } > + > + if (port != 6 && phylink_autoneg_inband(mode)) { > + dev_err(ds->dev, "%s: in-band negotiation unsupported\n", > + __func__); > + return; > + } > + > + switch (state->interface) { > + case PHY_INTERFACE_MODE_RGMII: > + /* RGMII mode means no delay so don't enable the delay */ > + qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN); > + break; > + case PHY_INTERFACE_MODE_RGMII_ID: > + /* RGMII_ID needs internal delay. This is enabled through > + * PORT5_PAD_CTRL for all ports, rather than individual port > + * registers > + */ > + qca8k_write(priv, reg, > + QCA8K_PORT_PAD_RGMII_EN | > + QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) | > + QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY)); 3 points here: - Should you prevalidate the device tree bindings that in case rgmii* mode are used, same delay settings are applied to all ports? - Can any RGMII port be connected to a PHY? If it can, won't the PHY enable delays too for RGMII_ID? Will the link work in that case? - Should you treat RGMII_TX_DELAY and RGMII_RX_DELAY independently for the case where there may be PCB traces? > + qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL, > + QCA8K_PORT_PAD_RGMII_RX_DELAY_EN); > + break; > + case PHY_INTERFACE_MODE_SGMII: > + case PHY_INTERFACE_MODE_1000BASEX: > + /* Enable SGMII on the port */ > + qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN); > + break; > + default: > + dev_err(ds->dev, "xMII mode %s not supported for port %d\n", > + phy_modes(state->interface), port); > return; > + } > +} > > - /* Set port speed */ > - switch (phy->speed) { > - case 10: > - reg = QCA8K_PORT_STATUS_SPEED_10; > +static void > +qca8k_phylink_validate(struct dsa_switch *ds, int port, > + unsigned long *supported, > + struct phylink_link_state *state) > +{ > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > + > + switch (port) { > + case 0: /* 1st CPU port */ > + if (state->interface != PHY_INTERFACE_MODE_NA && > + state->interface != PHY_INTERFACE_MODE_RGMII && > + state->interface != PHY_INTERFACE_MODE_RGMII_ID && > + state->interface != PHY_INTERFACE_MODE_SGMII) > + goto unsupported; > break; > - case 100: > - reg = QCA8K_PORT_STATUS_SPEED_100; > + case 1: > + case 2: > + case 3: > + case 4: > + case 5: > + /* Internal PHY */ > + if (state->interface != PHY_INTERFACE_MODE_NA && > + state->interface != PHY_INTERFACE_MODE_GMII) > + goto unsupported; > break; > - case 1000: > - reg = QCA8K_PORT_STATUS_SPEED_1000; > + case 6: /* 2nd CPU port / external PHY */ > + if (state->interface != PHY_INTERFACE_MODE_NA && > + state->interface != PHY_INTERFACE_MODE_RGMII && > + state->interface != PHY_INTERFACE_MODE_RGMII_ID && > + state->interface != PHY_INTERFACE_MODE_SGMII && > + state->interface != PHY_INTERFACE_MODE_1000BASEX) > + goto unsupported; > break; > default: > - dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n", > - port, phy->speed); > + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); phylink has a better validation error message than this, I'd say this is unnecessary. > +unsupported: > + linkmode_zero(supported); > return; > } > > - /* Set duplex mode */ > - if (phy->duplex == DUPLEX_FULL) > - reg |= QCA8K_PORT_STATUS_DUPLEX; > + phylink_set_port_modes(mask); > + phylink_set(mask, Autoneg); > + > + phylink_set(mask, 1000baseT_Full); > + phylink_set(mask, 10baseT_Half); > + phylink_set(mask, 10baseT_Full); > + phylink_set(mask, 100baseT_Half); > + phylink_set(mask, 100baseT_Full); > + > + if (state->interface == PHY_INTERFACE_MODE_1000BASEX) > + phylink_set(mask, 1000baseX_Full); > + > + phylink_set(mask, Pause); > + phylink_set(mask, Asym_Pause); > + > + linkmode_and(supported, supported, mask); > + linkmode_and(state->advertising, state->advertising, mask); > +} > + > +static int > +qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port, > + struct phylink_link_state *state) > +{ > + struct qca8k_priv *priv = ds->priv; > + u32 reg; > > - /* Force flow control */ > - if (dsa_is_cpu_port(ds, port)) > - reg |= QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_TXFLOW; > + reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port)); > + > + state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP); > + state->an_complete = state->link; > + state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO); > + state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL : > + DUPLEX_HALF; > + > + switch (reg & QCA8K_PORT_STATUS_SPEED) { > + case QCA8K_PORT_STATUS_SPEED_10: > + state->speed = SPEED_10; > + break; > + case QCA8K_PORT_STATUS_SPEED_100: > + state->speed = SPEED_100; > + break; > + case QCA8K_PORT_STATUS_SPEED_1000: > + state->speed = SPEED_1000; > + break; > + default: > + state->speed = SPEED_UNKNOWN; > + break; > + } > + > + state->pause = MLO_PAUSE_NONE; > + if (reg & QCA8K_PORT_STATUS_RXFLOW) > + state->pause |= MLO_PAUSE_RX; > + if (reg & QCA8K_PORT_STATUS_TXFLOW) > + state->pause |= MLO_PAUSE_TX; > + > + return 1; > +} > + > +static void > +qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, > + phy_interface_t interface) > +{ > + struct qca8k_priv *priv = ds->priv; > > - /* Force link down before changing MAC options */ > qca8k_port_set_status(priv, port, 0); > +} > + > +static void > +qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode, > + phy_interface_t interface, struct phy_device *phydev, > + int speed, int duplex, bool tx_pause, bool rx_pause) > +{ > + struct qca8k_priv *priv = ds->priv; > + u32 reg; > + > + if (phylink_autoneg_inband(mode)) { > + reg = QCA8K_PORT_STATUS_LINK_AUTO; > + } else { > + switch (speed) { > + case SPEED_10: > + reg = QCA8K_PORT_STATUS_SPEED_10; > + break; > + case SPEED_100: > + reg = QCA8K_PORT_STATUS_SPEED_100; > + break; > + case SPEED_1000: > + reg = QCA8K_PORT_STATUS_SPEED_1000; > + break; > + default: > + reg = QCA8K_PORT_STATUS_LINK_AUTO; > + break; > + } > + > + if (duplex == DUPLEX_FULL) > + reg |= QCA8K_PORT_STATUS_DUPLEX; > + > + if (rx_pause | dsa_is_cpu_port(ds, port)) I think it is odd to do bitwise OR on booleans. > + reg |= QCA8K_PORT_STATUS_RXFLOW; > + > + if (tx_pause | dsa_is_cpu_port(ds, port)) > + reg |= QCA8K_PORT_STATUS_TXFLOW; > + } > + > + reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC; > + > qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg); > - qca8k_port_set_status(priv, port, 1); > } > > static void > @@ -937,13 +1050,11 @@ qca8k_port_enable(struct dsa_switch *ds, int port, > { > struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv; > > - if (!dsa_is_user_port(ds, port)) > - return 0; > - > qca8k_port_set_status(priv, port, 1); > priv->port_sts[port].enabled = 1; > > - phy_support_asym_pause(phy); > + if (dsa_is_user_port(ds, port)) > + phy_support_asym_pause(phy); > > return 0; > } > @@ -1026,7 +1137,6 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port, > static const struct dsa_switch_ops qca8k_switch_ops = { > .get_tag_protocol = qca8k_get_tag_protocol, > .setup = qca8k_setup, > - .adjust_link = qca8k_adjust_link, > .get_strings = qca8k_get_strings, > .get_ethtool_stats = qca8k_get_ethtool_stats, > .get_sset_count = qca8k_get_sset_count, > @@ -1040,6 +1150,11 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > .port_fdb_add = qca8k_port_fdb_add, > .port_fdb_del = qca8k_port_fdb_del, > .port_fdb_dump = qca8k_port_fdb_dump, > + .phylink_validate = qca8k_phylink_validate, > + .phylink_mac_link_state = qca8k_phylink_mac_link_state, > + .phylink_mac_config = qca8k_phylink_mac_config, > + .phylink_mac_link_down = qca8k_phylink_mac_link_down, > + .phylink_mac_link_up = qca8k_phylink_mac_link_up, > }; > > static int > -- > 2.20.1 > Cheers, -Vladimir