Re: [PATCH v2 1/5] net: ag71xx: port to phylink
Hi,
On Fri, Oct 18, 2019 at 11:39:25AM +0200, Oleksij Rempel wrote:
> The port to phylink was done as close as possible to initial
> functionality.
> Theoretically this HW can support flow control, practically seems to be not
> enough to just enable it. So, more work should be done.
>
> Signed-off-by: Oleksij Rempel
> ---
> drivers/net/ethernet/atheros/Kconfig | 2 +-
> drivers/net/ethernet/atheros/ag71xx.c | 146 +++---
> 2 files changed, 87 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/net/ethernet/atheros/Kconfig
> b/drivers/net/ethernet/atheros/Kconfig
> index 0058051ba925..2720bde5034e 100644
> --- a/drivers/net/ethernet/atheros/Kconfig
> +++ b/drivers/net/ethernet/atheros/Kconfig
> @@ -20,7 +20,7 @@ if NET_VENDOR_ATHEROS
> config AG71XX
> tristate "Atheros AR7XXX/AR9XXX built-in ethernet mac support"
> depends on ATH79
> - select PHYLIB
> + select PHYLINK
> help
> If you wish to compile a kernel for AR7XXX/91XXX and enable
> ethernet support, then you should always answer Y to this.
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c
> b/drivers/net/ethernet/atheros/ag71xx.c
> index 1b1a09095c0d..4ad587d6a8e8 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -314,6 +315,8 @@ struct ag71xx {
> dma_addr_t stop_desc_dma;
>
> int phy_if_mode;
> + struct phylink *phylink;
> + struct phylink_config phylink_config;
>
> struct delayed_work restart_work;
> struct timer_list oom_timer;
> @@ -845,24 +848,20 @@ static void ag71xx_hw_start(struct ag71xx *ag)
> netif_wake_queue(ag->ndev);
> }
>
> -static void ag71xx_link_adjust(struct ag71xx *ag, bool update)
> +static void ag71xx_mac_config(struct phylink_config *config, unsigned int
> mode,
> + const struct phylink_link_state *state)
> {
> - struct phy_device *phydev = ag->ndev->phydev;
> + struct ag71xx *ag = netdev_priv(to_net_dev(config->dev));
> u32 cfg2;
> u32 ifctl;
> u32 fifo5;
>
> - if (!phydev->link && update) {
> - ag71xx_hw_stop(ag);
> - return;
> - }
> -
> if (!ag71xx_is(ag, AR7100) && !ag71xx_is(ag, AR9130))
> ag71xx_fast_reset(ag);
>
> cfg2 = ag71xx_rr(ag, AG71XX_REG_MAC_CFG2);
> cfg2 &= ~(MAC_CFG2_IF_1000 | MAC_CFG2_IF_10_100 | MAC_CFG2_FDX);
> - cfg2 |= (phydev->duplex) ? MAC_CFG2_FDX : 0;
> + cfg2 |= (state->duplex) ? MAC_CFG2_FDX : 0;
>
> ifctl = ag71xx_rr(ag, AG71XX_REG_MAC_IFCTL);
> ifctl &= ~(MAC_IFCTL_SPEED);
> @@ -870,7 +869,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool
> update)
> fifo5 = ag71xx_rr(ag, AG71XX_REG_FIFO_CFG5);
> fifo5 &= ~FIFO_CFG5_BM;
>
> - switch (phydev->speed) {
> + switch (state->speed) {
Please see the documentation for the mac_config() method in
include/linux/phylink.h wrt state->speed and state->duplex validity.
> case SPEED_1000:
> cfg2 |= MAC_CFG2_IF_1000;
> fifo5 |= FIFO_CFG5_BM;
> @@ -883,7 +882,6 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool
> update)
> cfg2 |= MAC_CFG2_IF_10_100;
> break;
> default:
> - WARN(1, "not supported speed %i\n", phydev->speed);
> return;
> }
>
> @@ -897,58 +895,78 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool
> update)
> ag71xx_wr(ag, AG71XX_REG_MAC_CFG2, cfg2);
> ag71xx_wr(ag, AG71XX_REG_FIFO_CFG5, fifo5);
> ag71xx_wr(ag, AG71XX_REG_MAC_IFCTL, ifctl);
> -
> - ag71xx_hw_start(ag);
> -
> - if (update)
> - phy_print_status(phydev);
> }
>
> -static void ag71xx_phy_link_adjust(struct net_device *ndev)
> +static void ag71xx_mac_validate(struct phylink_config *config,
> + unsigned long *supported,
> + struct phylink_link_state *state)
> {
> - struct ag71xx *ag = netdev_priv(ndev);
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> + if (state->interface != PHY_INTERFACE_MODE_NA &&
> + state->interface != PHY_INTERFACE_MODE_GMII &&
> + state->interface != PHY_INTERFACE_MODE_MII) {
> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> + return;
> + }
> +
> + phylink_set(mask, MII);
> +
> + /* flow control is not supported */
>
> - ag71xx_link_adjust(ag, true);
> + 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_GMII) {
This is an assignment, is always true, and will produce a compiler
warning.
Please see the documentation for the validate() method in
include/
[PATCH v2 1/5] net: ag71xx: port to phylink
The port to phylink was done as close as possible to initial
functionality.
Theoretically this HW can support flow control, practically seems to be not
enough to just enable it. So, more work should be done.
Signed-off-by: Oleksij Rempel
---
drivers/net/ethernet/atheros/Kconfig | 2 +-
drivers/net/ethernet/atheros/ag71xx.c | 146 +++---
2 files changed, 87 insertions(+), 61 deletions(-)
diff --git a/drivers/net/ethernet/atheros/Kconfig
b/drivers/net/ethernet/atheros/Kconfig
index 0058051ba925..2720bde5034e 100644
--- a/drivers/net/ethernet/atheros/Kconfig
+++ b/drivers/net/ethernet/atheros/Kconfig
@@ -20,7 +20,7 @@ if NET_VENDOR_ATHEROS
config AG71XX
tristate "Atheros AR7XXX/AR9XXX built-in ethernet mac support"
depends on ATH79
- select PHYLIB
+ select PHYLINK
help
If you wish to compile a kernel for AR7XXX/91XXX and enable
ethernet support, then you should always answer Y to this.
diff --git a/drivers/net/ethernet/atheros/ag71xx.c
b/drivers/net/ethernet/atheros/ag71xx.c
index 1b1a09095c0d..4ad587d6a8e8 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -314,6 +315,8 @@ struct ag71xx {
dma_addr_t stop_desc_dma;
int phy_if_mode;
+ struct phylink *phylink;
+ struct phylink_config phylink_config;
struct delayed_work restart_work;
struct timer_list oom_timer;
@@ -845,24 +848,20 @@ static void ag71xx_hw_start(struct ag71xx *ag)
netif_wake_queue(ag->ndev);
}
-static void ag71xx_link_adjust(struct ag71xx *ag, bool update)
+static void ag71xx_mac_config(struct phylink_config *config, unsigned int mode,
+ const struct phylink_link_state *state)
{
- struct phy_device *phydev = ag->ndev->phydev;
+ struct ag71xx *ag = netdev_priv(to_net_dev(config->dev));
u32 cfg2;
u32 ifctl;
u32 fifo5;
- if (!phydev->link && update) {
- ag71xx_hw_stop(ag);
- return;
- }
-
if (!ag71xx_is(ag, AR7100) && !ag71xx_is(ag, AR9130))
ag71xx_fast_reset(ag);
cfg2 = ag71xx_rr(ag, AG71XX_REG_MAC_CFG2);
cfg2 &= ~(MAC_CFG2_IF_1000 | MAC_CFG2_IF_10_100 | MAC_CFG2_FDX);
- cfg2 |= (phydev->duplex) ? MAC_CFG2_FDX : 0;
+ cfg2 |= (state->duplex) ? MAC_CFG2_FDX : 0;
ifctl = ag71xx_rr(ag, AG71XX_REG_MAC_IFCTL);
ifctl &= ~(MAC_IFCTL_SPEED);
@@ -870,7 +869,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool
update)
fifo5 = ag71xx_rr(ag, AG71XX_REG_FIFO_CFG5);
fifo5 &= ~FIFO_CFG5_BM;
- switch (phydev->speed) {
+ switch (state->speed) {
case SPEED_1000:
cfg2 |= MAC_CFG2_IF_1000;
fifo5 |= FIFO_CFG5_BM;
@@ -883,7 +882,6 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool
update)
cfg2 |= MAC_CFG2_IF_10_100;
break;
default:
- WARN(1, "not supported speed %i\n", phydev->speed);
return;
}
@@ -897,58 +895,78 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool
update)
ag71xx_wr(ag, AG71XX_REG_MAC_CFG2, cfg2);
ag71xx_wr(ag, AG71XX_REG_FIFO_CFG5, fifo5);
ag71xx_wr(ag, AG71XX_REG_MAC_IFCTL, ifctl);
-
- ag71xx_hw_start(ag);
-
- if (update)
- phy_print_status(phydev);
}
-static void ag71xx_phy_link_adjust(struct net_device *ndev)
+static void ag71xx_mac_validate(struct phylink_config *config,
+ unsigned long *supported,
+ struct phylink_link_state *state)
{
- struct ag71xx *ag = netdev_priv(ndev);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+ if (state->interface != PHY_INTERFACE_MODE_NA &&
+ state->interface != PHY_INTERFACE_MODE_GMII &&
+ state->interface != PHY_INTERFACE_MODE_MII) {
+ bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ return;
+ }
+
+ phylink_set(mask, MII);
+
+ /* flow control is not supported */
- ag71xx_link_adjust(ag, true);
+ 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_GMII) {
+ phylink_set(mask, 1000baseT_Full);
+ phylink_set(mask, 1000baseX_Full);
+ }
+
+ bitmap_and(supported, supported, mask,
+ __ETHTOOL_LINK_MODE_MASK_NBITS);
+ bitmap_and(state->advertising, state->advertising, mask,
+ __ETHTOOL_LINK_MODE_MASK_NBITS);
}
-static int ag71xx_phy_connect(struct ag71xx *ag)
+static void ag71xx_mac_link_down(struct phylink_config *config,
+
