RE: [PATCH net-next 2/3] net: stmmac: Start adding phylink support
From: Russell King - ARM Linux admin > 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. Thanks for the feedback. Please see previous discussion here that lead to the introduction of this patch [1]. I can squash it into 3/3 but the diff of that patch will look even worse ... [1] https://patchwork.ozlabs.org/patch/1110489/ Thanks, Jose Miguel Abreu
Re: [PATCH net-next 2/3] net: stmmac: Start adding phylink support
On Tue, Jun 11, 2019 at 05:18:46PM +0200, Jose Abreu wrote:
> Start adding the phylink callbacks.
>
> Signed-off-by: Jose Abreu
> Cc: Joao Pinto
> Cc: David S. Miller
> Cc: Giuseppe Cavallaro
> Cc: Alexandre Torgue
> Cc: Russell King
> Cc: Andrew Lunn
> Cc: Florian Fainelli
> Cc: Heiner Kallweit
> ---
> 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
> #include
> #include
> +#include
linux/phy.h is unnecessary when you include phylink.h
> #include
> #include "common.h"
> #include
> @@ -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
> #endif /* CONFIG_DEBUG_FS */
> #include
> +#include
> #include
> #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
[PATCH net-next 2/3] net: stmmac: Start adding phylink support
Start adding the phylink callbacks.
Signed-off-by: Jose Abreu
Cc: Joao Pinto
Cc: David S. Miller
Cc: Giuseppe Cavallaro
Cc: Alexandre Torgue
Cc: Russell King
Cc: Andrew Lunn
Cc: Florian Fainelli
Cc: Heiner Kallweit
---
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
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
#include
#include
+#include
#include
#include "common.h"
#include
@@ -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
#endif /* CONFIG_DEBUG_FS */
#include
+#include
#include
#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);
+ }
+
+ 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 */
+};
+
/**
* stmmac_adjust_link - adjusts the link parameters
* @dev: net device structure
--
2.21.0
