Re: [PATCH net-next 2/3] net: stmmac: Start adding phylink support

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

On Tue, Jun 11, 2019 at 05:18:46PM +0200, Jose Abreu wrote:
> Start adding the phylink callbacks.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/Kconfig   |  1 +
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  4 ++
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 48 +++++++++++++++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index 0b5c8d74c683..cf0c9f4f347a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -4,6 +4,7 @@ config STMMAC_ETH
>  	depends on HAS_IOMEM && HAS_DMA
>  	select MII
>  	select PHYLIB
> +	select PHYLINK

Please replace PHYLIB with PHYLINK here, there's no need to select both.

>  	select CRC32
>  	imply PTP_1588_CLOCK
>  	select RESET_CONTROLLER
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index a16ada8b8507..b8386778f6c6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -25,6 +25,7 @@
>  #include <linux/clk.h>
>  #include <linux/stmmac.h>
>  #include <linux/phy.h>
> +#include <linux/phylink.h>

linux/phy.h is unnecessary when you include phylink.h

>  #include <linux/pci.h>
>  #include "common.h"
>  #include <linux/ptp_clock_kernel.h>
> @@ -155,6 +156,9 @@ struct stmmac_priv {
>  	struct mii_bus *mii;
>  	int mii_irq[PHY_MAX_ADDR];
>  
> +	struct phylink_config phylink_config;
> +	struct phylink *phylink;
> +
>  	struct stmmac_extra_stats xstats ____cacheline_aligned_in_smp;
>  	struct stmmac_safety_stats sstats;
>  	struct plat_stmmacenet_data *plat;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6a2f072c0ce3..e2e69cb08fef 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -45,6 +45,7 @@
>  #include <linux/seq_file.h>
>  #endif /* CONFIG_DEBUG_FS */
>  #include <linux/net_tstamp.h>
> +#include <linux/phylink.h>
>  #include <net/pkt_cls.h>
>  #include "stmmac_ptp.h"
>  #include "stmmac.h"
> @@ -848,6 +849,39 @@ static void stmmac_mac_flow_ctrl(struct stmmac_priv *priv, u32 duplex)
>  			priv->pause, tx_cnt);
>  }
>  
> +static void stmmac_validate(struct phylink_config *config,
> +			    unsigned long *supported,
> +			    struct phylink_link_state *state)
> +{
> +	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +	int tx_cnt = priv->plat->tx_queues_to_use;
> +	int max_speed = priv->plat->max_speed;
> +
> +	/* Cut down 1G if asked to */
> +	if ((max_speed > 0) && (max_speed < 1000)) {
> +		phylink_set(mask, 1000baseT_Full);
> +		phylink_set(mask, 1000baseX_Full);
> +	}
> +
> +	/* Half-Duplex can only work with single queue */
> +	if (tx_cnt > 1) {
> +		phylink_set(mask, 10baseT_Half);
> +		phylink_set(mask, 100baseT_Half);
> +		phylink_set(mask, 1000baseT_Half);
> +	}

The logic here looks a little weird - if max_speed is less than 1000, we
can end up with 1000baseT-HD enabled.  Surely this is not desirable.

> +
> +	bitmap_andnot(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +	bitmap_andnot(state->advertising, state->advertising, mask,
> +		      __ETHTOOL_LINK_MODE_MASK_NBITS);
> +}
> +
> +static int stmmac_mac_link_state(struct phylink_config *config,
> +				 struct phylink_link_state *state)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  static void stmmac_mac_config(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> @@ -900,6 +934,11 @@ static void stmmac_mac_config(struct net_device *dev)
>  	writel(ctrl, priv->ioaddr + MAC_CTRL_REG);
>  }
>  
> +static void stmmac_mac_an_restart(struct phylink_config *config)
> +{
> +	/* Not Supported */
> +}
> +
>  static void stmmac_mac_link_down(struct net_device *dev, bool autoneg)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> @@ -914,6 +953,15 @@ static void stmmac_mac_link_up(struct net_device *dev, bool autoneg)
>  	stmmac_mac_set(priv, priv->ioaddr, true);
>  }
>  
> +static const struct phylink_mac_ops __maybe_unused stmmac_phylink_mac_ops = {
> +	.validate = stmmac_validate,
> +	.mac_link_state = stmmac_mac_link_state,
> +	.mac_config = NULL, /* TO BE FILLED */
> +	.mac_an_restart = stmmac_mac_an_restart,
> +	.mac_link_down = NULL, /* TO BE FILLED */
> +	.mac_link_up = NULL, /* TO BE FILLED */
> +};
> +

If this is not used, I don't really see the point of splitting this from
the rest of the patch.  Also, I don't see the point of all those NULL
initialisers either.

>  /**
>   * stmmac_adjust_link - adjusts the link parameters
>   * @dev: net device structure
> -- 
> 2.21.0
> 
> 

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