Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	Joao Pinto <Joao.Pinto@synopsys.com>,
	"David S . Miller" <davem@davemloft.net>,
	Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH net-next 2/3] net: stmmac: Start adding phylink support
Date: Tue, 11 Jun 2019 16:35:29 +0100
Message-ID: <20190611153529.z6hlkhtrnd5ksx2n@shell.armlinux.org.uk> (raw)
In-Reply-To: <7daa1ac5cf56152b9d6c969c24603bc82e0b7d55.1560266175.git.joabreu@synopsys.com>

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

  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-11 15:18 [PATCH net-next 0/3] net: stmmac: Convert to phylink Jose Abreu
2019-06-11 15:18 ` [PATCH net-next 1/3] net: stmmac: Prepare to convert " Jose Abreu
2019-06-11 15:18 ` [PATCH net-next 2/3] net: stmmac: Start adding phylink support Jose Abreu
2019-06-11 15:35   ` Russell King - ARM Linux admin [this message]
2019-06-11 15:40     ` Jose Abreu
2019-06-11 15:18 ` [PATCH net-next 3/3] net: stmmac: Convert to phylink and remove phylib logic Jose Abreu
2019-06-18  9:30   ` Jon Hunter
2019-06-18  9:35     ` Jose Abreu
2019-06-18  9:42       ` Jon Hunter
2019-06-18  9:46         ` Jose Abreu
2019-06-18 10:18           ` Jon Hunter
2019-06-18 15:20             ` Jon Hunter
2019-06-18 19:44               ` Jon Hunter
2019-06-20 14:05                 ` Jon Hunter
2019-06-25  7:37                   ` Jose Abreu
2019-06-25 11:10                     ` Jon Hunter
2019-06-25 11:25                       ` Jose Abreu
2019-06-13 21:02 ` [PATCH net-next 0/3] net: stmmac: Convert to phylink David Miller
2019-06-14 13:40 ` Corentin Labbe
2019-06-14 14:45   ` Jose Abreu
2019-07-22 12:42 ` Ondřej Jirman
2019-07-22 13:28   ` Jose Abreu
2019-07-22 13:40     ` Andrew Lunn
2019-07-22 13:58       ` Jose Abreu
2019-07-22 14:19         ` Andrew Lunn
2019-07-22 14:26           ` Jose Abreu
2019-07-22 14:39             ` Ondřej Jirman
2019-07-23  9:36               ` Jose Abreu
2019-07-22 13:49     ` Ondřej Jirman

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190611153529.z6hlkhtrnd5ksx2n@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=Joao.Pinto@synopsys.com \
    --cc=Jose.Abreu@synopsys.com \
    --cc=alexandre.torgue@st.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=peppe.cavallaro@st.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox