Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 Aug 2022 18:59:05 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc:     davem@davemloft.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com,
        Andrew Lunn <andrew@lunn.ch>, Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH net-next 2/2] net: altera: tse: convert to phylink
Message-ID: <YwUVabmOJNDgf/JK@shell.armlinux.org.uk>
References: <20220823140517.3091239-1-maxime.chevallier@bootlin.com>
 <20220823140517.3091239-3-maxime.chevallier@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220823140517.3091239-3-maxime.chevallier@bootlin.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Tue, Aug 23, 2022 at 04:05:17PM +0200, Maxime Chevallier wrote:
> This commit converts the Altera Triple Speed Ethernet Controller to
> phylink. This controller supports MII, GMII and RGMII with its MAC, and
> SGMII + 1000BaseX through a small embedded PCS.
> 
> The PCS itself has a register set very similar to what is found in a
> typical 802.3 ethernet PHY, but this register set memory-mapped instead
> of lying on an mdio bus.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

This needs some work.

> +static void alt_tse_mac_link_state(struct phylink_config *config,
> +				   struct phylink_link_state *state)
> +{
> +	struct net_device *ndev = to_net_dev(config->dev);
> +	struct altera_tse_private *priv = netdev_priv(ndev);
> +
> +	u16 bmsr, lpa;
> +
> +	bmsr = sgmii_pcs_read(priv, MII_BMSR);
> +	lpa = sgmii_pcs_read(priv, MII_LPA);
> +
> +	phylink_mii_c22_pcs_decode_state(state, bmsr, lpa);
> +}
> +
> +static void alt_tse_mac_an_restart(struct phylink_config *config)
> +{
> +	struct net_device *ndev = to_net_dev(config->dev);
> +	struct altera_tse_private *priv = netdev_priv(ndev);
> +	u16 bmcr;
> +
> +	bmcr = sgmii_pcs_read(priv, MII_BMCR);
> +	bmcr |= BMCR_ANRESTART;
> +	sgmii_pcs_write(priv, MII_BMCR, bmcr);
> +}
> +
> +static void alt_tse_pcs_config(struct net_device *ndev,
> +			       const struct phylink_link_state *state)
> +{
> +	struct altera_tse_private *priv = netdev_priv(ndev);
> +	u32 ctrl, if_mode;
> +
> +	if (state->interface != PHY_INTERFACE_MODE_SGMII &&
> +	    state->interface != PHY_INTERFACE_MODE_1000BASEX)
> +		return;
> +
> +	ctrl = sgmii_pcs_read(priv, MII_BMCR);
> +	if_mode = sgmii_pcs_read(priv, SGMII_PCS_IF_MODE);
> +
> +	/* Set link timer to 1.6ms, as per the MegaCore Function User Guide */
> +	sgmii_pcs_write(priv, SGMII_PCS_LINK_TIMER_0, 0x0D40);
> +	sgmii_pcs_write(priv, SGMII_PCS_LINK_TIMER_1, 0x03);
> +
> +	if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> +		if_mode |= PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA;
> +	} else if (state->interface == PHY_INTERFACE_MODE_1000BASEX) {
> +		if_mode &= ~(PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA);
> +		if_mode |= PCS_IF_MODE_SGMI_SPEED_1000;
> +	}
> +
> +	ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE);
> +
> +	sgmii_pcs_write(priv, MII_BMCR, ctrl);
> +	sgmii_pcs_write(priv, SGMII_PCS_IF_MODE, if_mode);
> +
> +	sgmii_pcs_reset(priv);
> +}

These look like they can be plugged directly into the phylink_pcs
support - please use that in preference to bolting it ino the MAC
ops - as every other ethernet driver (with the exception of
mtk_eth_soc) does today.

> +
> +static void alt_tse_mac_config(struct phylink_config *config, unsigned int mode,
> +			       const struct phylink_link_state *state)
> +{
> +	struct net_device *ndev = to_net_dev(config->dev);
> +	struct altera_tse_private *priv = netdev_priv(ndev);
> +	u32 ctrl;
> +
> +	ctrl = csrrd32(priv->mac_dev, tse_csroffs(command_config));
> +	ctrl &= ~(MAC_CMDCFG_ENA_10 | MAC_CMDCFG_ETH_SPEED | MAC_CMDCFG_HD_ENA);
> +
> +	if (state->duplex == DUPLEX_HALF)
> +		ctrl |= MAC_CMDCFG_HD_ENA;

Using state->duplex in mac_config has always been a problem, it's not
well defined, and there are paths through phylink where state->duplex
does not reflect the state of the link when this function is called.
This is why it's always been clearly documented that this shall not be
used in mac_config.

> +
> +	if (state->speed == SPEED_1000)
> +		ctrl |= MAC_CMDCFG_ETH_SPEED;
> +	else if (state->speed == SPEED_10)
> +		ctrl |= MAC_CMDCFG_ENA_10;

Using state->speed brings with it the same problems as state->duplex.

Please instead use mac_link_up() (and pcs_link_up()) - which are the
only callbacks from phylink that you can be sure give you the
speed, duplex and pause settings for the link.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!