Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-25 Thread Simon Horman 
On Fri, Sep 21, 2018 at 03:01:44PM +0200, Andrew Lunn wrote:
> > Thanks Andrew,
> > 
> > it seems that removing Aysm Pause does the trick.
> 
> Great.
> 
> I will submit the patch today.
> 
> I see two possible followups:
> 
> 1) Figure out why auto-neg does not complete when Pause is
> advertised. Is this a problem with the local PHY or the link partner?
> The Micrel we have some control over, but the link partner in the
> switch we have to treat as a black box.

I may have limited scope to configure the link partner,
but it will be limited at best.

> 2) If we can get negotiation to work correctly, then implement Pause
> in the MAC driver. When phylib calls the adjust_link callback
> phydev->pause and phydev->asym_pause tells you want the partner can
> do. You can then decide how to program the MAC. There is also a
> get/set for ethtool.
> 
> It really requires somebody with the hardware to do this.

I have the hardware but I'm not sure how easily I can get
it to others.

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-21 Thread Andrew Lunn 
> Thanks Andrew,
> 
> it seems that removing Aysm Pause does the trick.

Great.

I will submit the patch today.

I see two possible followups:

1) Figure out why auto-neg does not complete when Pause is
advertised. Is this a problem with the local PHY or the link partner?
The Micrel we have some control over, but the link partner in the
switch we have to treat as a black box.

2) If we can get negotiation to work correctly, then implement Pause
in the MAC driver. When phylib calls the adjust_link callback
phydev->pause and phydev->asym_pause tells you want the partner can
do. You can then decide how to program the MAC. There is also a
get/set for ethtool.

It really requires somebody with the hardware to do this.

   Andrew

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-21 Thread Simon Horman 
On Thu, Sep 20, 2018 at 03:25:30PM +0200, Andrew Lunn wrote:
> > 1. net-next: cf7d97e1e54d ("net: mdio: remove duplicated include from 
> > mdio_bus.c")
> > 
> > # mii-tool -vv eth0
> > Using SIOCGMIIPHY=0x8947
> > eth0: no link
> >   registers for MII PHY 0: 
> > 1140 7949 0022 1622 0d81 c1e1 000f 
> >  0300      3000
> >    0078 7002   0200
> >    0528    
> >   product info: vendor 00:08:85, model 34 rev 2
> >   basic mode:   autonegotiation enabled
> >   basic status: no link
> >   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 
> > 10baseT-FD 10baseT-HD
> >   advertising:  100baseTx-FD 100baseTx-HD flow-control
> >   link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> > 
> > 2. net-next with this patch reverted
> > 
> > # mii-tool -vv eth0
> > Using SIOCGMIIPHY=0x8947
> > eth0: negotiated 100baseTx-FD, link ok
> >   registers for MII PHY 0: 
> > 1140 796d 0022 1622 0181 c1e1 000f 
> 
> Hi Simon
> 
> Word 5 is what we are advertising. Bits 10 and 11 are Pause and Asym
> Pause. In the good case here, neither are set. In this bad case above,
> both bits are set.
> 
> The patch i asked you to try only cleared the Pause bit, not the
> Asymmetric Pause bit. mii-tool only saying 'flow-control' did not
> help.
> 
> Word 6 is what the partner is advertising. c1e1 indicates the partner
> does not support flow control, both bits are 0. I don't see why this
> is preventing auto-net though. But in the bad case, the status
> register indicates auto-neg has not completed.
> 
> Anyway, please can you try this patch, which also removes Aysm Pause.

Thanks Andrew,

it seems that removing Aysm Pause does the trick.


* net-next [5678cb3c96ee ("net-next: mscc: remove unused ocelot_dev_gmii.h")]
+ Your patch to disable Pause and Asym_Pause (and 10baseT)
  => Success!

# ip link set up dev eth0; sleep 10; mii-tool -vv eth0
[   13.418522] Micrel KSZ9031 Gigabit PHY e680.ethernet-:00: 
attached PHY driver [Micrel KSZ9031 Gigabit PHY] 
(mii_bus:phy_addr=e680.ethernet-:00, irq=204)
[   16.399410] ravb e680.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control off
Using SIOCGMIIPHY=0x8947
eth0: negotiated 100baseTx-FD, link ok
  registers for MII PHY 0: 
1140 796d 0022 1622 0181 c1e1 000f 
 0300 3800     3000
   0c7e 54fe   0200
   0500    
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: autonegotiation complete, link ok
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 
10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD
  link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

* net-next [5678cb3c96ee ("net-next: mscc: remove unused ocelot_dev_gmii.h")]
+ Your patch modified to only disable Asym_Pause (and 10baseT)
  => Success!

# ip link set up dev eth0; sleep 10; mii-tool -vv eth0
[   86.414460] Micrel KSZ9031 Gigabit PHY e680.ethernet-:00: 
attached PHY driver [Micrel KSZ9031 Gigabit PHY] 
(mii_bus:phy_addr=e680.ethernet-:00, irq=204)
[   89.414651] ravb e680.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control off
Using SIOCGMIIPHY=0x8947
eth0: negotiated 100baseTx-FD, link ok
  registers for MII PHY 0: 
1140 796d 0022 1622 0581 c1e1 000f 
 0300 3800     3000
   087e 44fe   0200
   0500    
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: autonegotiation complete, link ok
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 
10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD flow-control
  link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

* net-next [5678cb3c96ee ("net-next: mscc: remove unused ocelot_dev_gmii.h")]
  + Your patch modified to only disable Pause (and 10baseT)
  => Fail (same test and result as yesterday)

# ip link set up dev eth0; sleep 10; mii-tool -vv eth0
[   52.518742] Micrel KSZ9031 Gigabit PHY e680.ethernet-:00: 
attached PHY driver [Micrel KSZ9031 Gigabit PHY] 
(mii_bus:phy_addr=e680.ethernet-:00, irq=204)
Using SIOCGMIIPHY=0x8947
eth0: no link
  registers for MII PHY 0: 
1140 7949 0022 1622 0981 c1e1 000f 
 0300      3000
   0878 7002   0200
   0528    
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: no link
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 
10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD
  link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

> 
> Thanks
>   Andrew
> 
> >From

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-20 Thread Andrew Lunn 
> 1. net-next: cf7d97e1e54d ("net: mdio: remove duplicated include from 
> mdio_bus.c")
> 
> # mii-tool -vv eth0
> Using SIOCGMIIPHY=0x8947
> eth0: no link
>   registers for MII PHY 0: 
> 1140 7949 0022 1622 0d81 c1e1 000f 
>  0300      3000
>    0078 7002   0200
>    0528    
>   product info: vendor 00:08:85, model 34 rev 2
>   basic mode:   autonegotiation enabled
>   basic status: no link
>   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 
> 10baseT-FD 10baseT-HD
>   advertising:  100baseTx-FD 100baseTx-HD flow-control
>   link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> 
> 2. net-next with this patch reverted
> 
> # mii-tool -vv eth0
> Using SIOCGMIIPHY=0x8947
> eth0: negotiated 100baseTx-FD, link ok
>   registers for MII PHY 0: 
> 1140 796d 0022 1622 0181 c1e1 000f 

Hi Simon

Word 5 is what we are advertising. Bits 10 and 11 are Pause and Asym
Pause. In the good case here, neither are set. In this bad case above,
both bits are set.

The patch i asked you to try only cleared the Pause bit, not the
Asymmetric Pause bit. mii-tool only saying 'flow-control' did not
help.

Word 6 is what the partner is advertising. c1e1 indicates the partner
does not support flow control, both bits are 0. I don't see why this
is preventing auto-net though. But in the bad case, the status
register indicates auto-neg has not completed.

Anyway, please can you try this patch, which also removes Aysm Pause.

Thanks
Andrew

>From 00a061304af51831ca1dc86bf6ce23d01f724229 Mon Sep 17 00:00:00 2001
From: Andrew Lunn 
Date: Tue, 18 Sep 2018 18:12:54 -0500
Subject: [PATCH] ravb: Disable Pause Advertisement

The previous commit to ravb had the side effect of making the PHY
advertise Pause. This previously did not happen, and it appears the
MAC does not support Pause. By default, phydev->supported has Pause
enabled, but phydev->advertising does not. Rather than rely on this,
be explicit, and remove the Pause link mode.

Reported-by: Simon Horman 
Fixes: 41124fa64d4b ("net: ethernet: Add helper to remove a supported link 
mode")
Signed-off-by: Andrew Lunn 
---
 drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
b/drivers/net/ethernet/renesas/ravb_main.c
index fb2a1125780d..b0f2612ad226 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1073,9 +1073,11 @@ static int ravb_phy_init(struct net_device *ndev)
netdev_info(ndev, "limited PHY to 100Mbit/s\n");
}
 
-   /* 10BASE is not supported */
+   /* 10BASE, Pause and Asym Pause is not supported */
phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
+   phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
+   phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
 
phy_attached_info(phydev);
 
-- 
2.19.0.rc1

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-18 Thread Andrew Lunn 
> Hi Andrew,

Hi Simon

Thanks for the dumps
 
> 1. net-next: cf7d97e1e54d ("net: mdio: remove duplicated include from 
> mdio_bus.c")
> 
>   basic status: no link
>   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 
> 10baseT-FD 10baseT-HD
>   advertising:  100baseTx-FD 100baseTx-HD flow-control
>   link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> 
> 2. net-next with this patch reverted
> 
>   basic status: autonegotiation complete, link ok
>   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 
> 10baseT-FD 10baseT-HD
>   advertising:  100baseTx-FD 100baseTx-HD
>   link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

So flow-control is not present here.

>   basic status: autonegotiation complete, link ok
>   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 
> 10baseT-FD 10baseT-HD
>   advertising:  100baseTx-FD 100baseTx-HD
>   link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

And here also.

Looking at the code, i see:

/* E-MAC init function */
static void ravb_emac_init(struct net_device *ndev)
{
struct ravb_private *priv = netdev_priv(ndev);

/* Receive frame limit set register */
ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);

/* EMAC Mode: PAUSE prohibition; Duplex; RX Checksum; TX; RX */
ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
   (ndev->features & NETIF_F_RXCSUM ? ECMR_RCSC : 0) |
   ECMR_TE | ECMR_RE, ECMR);

Does this mean Pause is not supported in the hardware?

 Thanks
Andrew

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-18 Thread Simon Horman 
On Mon, Sep 17, 2018 at 05:38:11PM +0200, Andrew Lunn wrote:
> On Mon, Sep 17, 2018 at 05:13:07PM +0200, Simon Horman wrote:
> > On Wed, Sep 12, 2018 at 01:53:14AM +0200, Andrew Lunn wrote:
> > > Some MAC hardware cannot support a subset of link modes. e.g. often
> > > 1Gbps Full duplex is supported, but Half duplex is not. Add a helper
> > > to remove such a link mode.
> > > 
> > > Signed-off-by: Andrew Lunn 
> > > Reviewed-by: Florian Fainelli 
> > > ---
> > >  drivers/net/ethernet/apm/xgene/xgene_enet_hw.c |  6 +++---
> > >  drivers/net/ethernet/cadence/macb_main.c   |  5 ++---
> > >  drivers/net/ethernet/freescale/fec_main.c  |  3 ++-
> > >  drivers/net/ethernet/microchip/lan743x_main.c  |  2 +-
> > >  drivers/net/ethernet/renesas/ravb_main.c   |  3 ++-
> > >  .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 12 
> > >  drivers/net/phy/phy_device.c   | 18 ++
> > >  drivers/net/usb/lan78xx.c  |  2 +-
> > >  include/linux/phy.h|  1 +
> > >  9 files changed, 38 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c 
> > > b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> > > index 078a04dc1182..4831f9de5945 100644
> > 
> > ...
> > 
> > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
> > > b/drivers/net/ethernet/renesas/ravb_main.c
> > > index aff5516b781e..fb2a1125780d 100644
> > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > @@ -1074,7 +1074,8 @@ static int ravb_phy_init(struct net_device *ndev)
> > >   }
> > >  
> > >   /* 10BASE is not supported */
> > > - phydev->supported &= ~PHY_10BT_FEATURES;
> > > + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> > > + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> > >  
> > >   phy_attached_info(phydev);
> > >  
> > 
> > ...
> > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index db1172db1e7c..e9ca83a438b0 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -1765,6 +1765,24 @@ int phy_set_max_speed(struct phy_device *phydev, 
> > > u32 max_speed)
> > >  }
> > >  EXPORT_SYMBOL(phy_set_max_speed);
> > >  
> > > +/**
> > > + * phy_remove_link_mode - Remove a supported link mode
> > > + * @phydev: phy_device structure to remove link mode from
> > > + * @link_mode: Link mode to be removed
> > > + *
> > > + * Description: Some MACs don't support all link modes which the PHY
> > > + * does.  e.g. a 1G MAC often does not support 1000Half. Add a helper
> > > + * to remove a link mode.
> > > + */
> > > +void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode)
> > > +{
> > > + WARN_ON(link_mode > 31);
> > > +
> > > + phydev->supported &= ~BIT(link_mode);
> > > + phydev->advertising = phydev->supported;
> > > +}
> > > +EXPORT_SYMBOL(phy_remove_link_mode);
> > > +
> > >  static void of_set_phy_supported(struct phy_device *phydev)
> > >  {
> > >   struct device_node *node = phydev->mdio.dev.of_node;
> > 
> > Hi Andrew,
> > 
> > I believe that for the RAVB the overall effect of this change is that
> > 10-BaseT modes are no longer advertised (although both with and without
> > this patch they are not supported).
> > 
> > Unfortunately on R-Car Gen3 M3-W (r8a7796) based Salvator-X board
> > I have observed that this results in the link no longer being negotiated
> > on one switch (the one I usually use) while it seemed fine on another.
> 
> Hi Simon
> 
> Thanks for testing this.
> 
> Could you dump the PHY registers with and without this patch:
> 
> $ mii-tool -vv eth0
> 
> Once difference is that phy_remove_link_mode() does
> phydev->advertising = phydev->supported where as the old code does
> not. I though phylib would do this anyway, it does at some point in
> time, but i didn't check when. It could be you are actually
> advertising 10, even if you don't support it.

Hi Andrew,

here are the results. I ran them with the device connected to the switch
which doesn't allow successful link negotiation when this patch is present.

1. net-next: cf7d97e1e54d ("net: mdio: remove duplicated include from 
mdio_bus.c")

# mii-tool -vv eth0
Using SIOCGMIIPHY=0x8947
eth0: no link
  registers for MII PHY 0: 
1140 7949 0022 1622 0d81 c1e1 000f 
 0300      3000
   0078 7002   0200
   0528    
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: no link
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 
10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD flow-control
  link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

2. net-next with this patch reverted

# mii-tool -vv eth0
Using SIOCGMIIPHY=0x8947
eth0: negotiated 100baseTx-FD, link ok
  registers for

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-17 Thread Andrew Lunn 
On Mon, Sep 17, 2018 at 05:13:07PM +0200, Simon Horman wrote:
> On Wed, Sep 12, 2018 at 01:53:14AM +0200, Andrew Lunn wrote:
> > Some MAC hardware cannot support a subset of link modes. e.g. often
> > 1Gbps Full duplex is supported, but Half duplex is not. Add a helper
> > to remove such a link mode.
> > 
> > Signed-off-by: Andrew Lunn 
> > Reviewed-by: Florian Fainelli 
> > ---
> >  drivers/net/ethernet/apm/xgene/xgene_enet_hw.c |  6 +++---
> >  drivers/net/ethernet/cadence/macb_main.c   |  5 ++---
> >  drivers/net/ethernet/freescale/fec_main.c  |  3 ++-
> >  drivers/net/ethernet/microchip/lan743x_main.c  |  2 +-
> >  drivers/net/ethernet/renesas/ravb_main.c   |  3 ++-
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 12 
> >  drivers/net/phy/phy_device.c   | 18 ++
> >  drivers/net/usb/lan78xx.c  |  2 +-
> >  include/linux/phy.h|  1 +
> >  9 files changed, 38 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c 
> > b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> > index 078a04dc1182..4831f9de5945 100644
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index aff5516b781e..fb2a1125780d 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -1074,7 +1074,8 @@ static int ravb_phy_init(struct net_device *ndev)
> > }
> >  
> > /* 10BASE is not supported */
> > -   phydev->supported &= ~PHY_10BT_FEATURES;
> > +   phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> > +   phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> >  
> > phy_attached_info(phydev);
> >  
> 
> ...
> 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index db1172db1e7c..e9ca83a438b0 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1765,6 +1765,24 @@ int phy_set_max_speed(struct phy_device *phydev, u32 
> > max_speed)
> >  }
> >  EXPORT_SYMBOL(phy_set_max_speed);
> >  
> > +/**
> > + * phy_remove_link_mode - Remove a supported link mode
> > + * @phydev: phy_device structure to remove link mode from
> > + * @link_mode: Link mode to be removed
> > + *
> > + * Description: Some MACs don't support all link modes which the PHY
> > + * does.  e.g. a 1G MAC often does not support 1000Half. Add a helper
> > + * to remove a link mode.
> > + */
> > +void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode)
> > +{
> > +   WARN_ON(link_mode > 31);
> > +
> > +   phydev->supported &= ~BIT(link_mode);
> > +   phydev->advertising = phydev->supported;
> > +}
> > +EXPORT_SYMBOL(phy_remove_link_mode);
> > +
> >  static void of_set_phy_supported(struct phy_device *phydev)
> >  {
> > struct device_node *node = phydev->mdio.dev.of_node;
> 
> Hi Andrew,
> 
> I believe that for the RAVB the overall effect of this change is that
> 10-BaseT modes are no longer advertised (although both with and without
> this patch they are not supported).
> 
> Unfortunately on R-Car Gen3 M3-W (r8a7796) based Salvator-X board
> I have observed that this results in the link no longer being negotiated
> on one switch (the one I usually use) while it seemed fine on another.

Hi Simon

Thanks for testing this.

Could you dump the PHY registers with and without this patch:

$ mii-tool -vv eth0

Once difference is that phy_remove_link_mode() does
phydev->advertising = phydev->supported where as the old code does
not. I though phylib would do this anyway, it does at some point in
time, but i didn't check when. It could be you are actually
advertising 10, even if you don't support it.

Thanks
Andrew

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-17 Thread Simon Horman 
On Wed, Sep 12, 2018 at 01:53:14AM +0200, Andrew Lunn wrote:
> Some MAC hardware cannot support a subset of link modes. e.g. often
> 1Gbps Full duplex is supported, but Half duplex is not. Add a helper
> to remove such a link mode.
> 
> Signed-off-by: Andrew Lunn 
> Reviewed-by: Florian Fainelli 
> ---
>  drivers/net/ethernet/apm/xgene/xgene_enet_hw.c |  6 +++---
>  drivers/net/ethernet/cadence/macb_main.c   |  5 ++---
>  drivers/net/ethernet/freescale/fec_main.c  |  3 ++-
>  drivers/net/ethernet/microchip/lan743x_main.c  |  2 +-
>  drivers/net/ethernet/renesas/ravb_main.c   |  3 ++-
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 12 
>  drivers/net/phy/phy_device.c   | 18 ++
>  drivers/net/usb/lan78xx.c  |  2 +-
>  include/linux/phy.h|  1 +
>  9 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c 
> b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> index 078a04dc1182..4831f9de5945 100644

...

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
> b/drivers/net/ethernet/renesas/ravb_main.c
> index aff5516b781e..fb2a1125780d 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1074,7 +1074,8 @@ static int ravb_phy_init(struct net_device *ndev)
>   }
>  
>   /* 10BASE is not supported */
> - phydev->supported &= ~PHY_10BT_FEATURES;
> + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
>  
>   phy_attached_info(phydev);
>  

...

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index db1172db1e7c..e9ca83a438b0 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1765,6 +1765,24 @@ int phy_set_max_speed(struct phy_device *phydev, u32 
> max_speed)
>  }
>  EXPORT_SYMBOL(phy_set_max_speed);
>  
> +/**
> + * phy_remove_link_mode - Remove a supported link mode
> + * @phydev: phy_device structure to remove link mode from
> + * @link_mode: Link mode to be removed
> + *
> + * Description: Some MACs don't support all link modes which the PHY
> + * does.  e.g. a 1G MAC often does not support 1000Half. Add a helper
> + * to remove a link mode.
> + */
> +void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode)
> +{
> + WARN_ON(link_mode > 31);
> +
> + phydev->supported &= ~BIT(link_mode);
> + phydev->advertising = phydev->supported;
> +}
> +EXPORT_SYMBOL(phy_remove_link_mode);
> +
>  static void of_set_phy_supported(struct phy_device *phydev)
>  {
>   struct device_node *node = phydev->mdio.dev.of_node;

Hi Andrew,

I believe that for the RAVB the overall effect of this change is that
10-BaseT modes are no longer advertised (although both with and without
this patch they are not supported).

Unfortunately on R-Car Gen3 M3-W (r8a7796) based Salvator-X board
I have observed that this results in the link no longer being negotiated
on one switch (the one I usually use) while it seemed fine on another.

[PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

2018-09-11 Thread Andrew Lunn 
Some MAC hardware cannot support a subset of link modes. e.g. often
1Gbps Full duplex is supported, but Half duplex is not. Add a helper
to remove such a link mode.

Signed-off-by: Andrew Lunn 
Reviewed-by: Florian Fainelli 
---
 drivers/net/ethernet/apm/xgene/xgene_enet_hw.c |  6 +++---
 drivers/net/ethernet/cadence/macb_main.c   |  5 ++---
 drivers/net/ethernet/freescale/fec_main.c  |  3 ++-
 drivers/net/ethernet/microchip/lan743x_main.c  |  2 +-
 drivers/net/ethernet/renesas/ravb_main.c   |  3 ++-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 12 
 drivers/net/phy/phy_device.c   | 18 ++
 drivers/net/usb/lan78xx.c  |  2 +-
 include/linux/phy.h|  1 +
 9 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c 
b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
index 078a04dc1182..4831f9de5945 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
@@ -895,9 +895,9 @@ int xgene_enet_phy_connect(struct net_device *ndev)
}
 
pdata->phy_speed = SPEED_UNKNOWN;
-   phy_dev->supported &= ~SUPPORTED_10baseT_Half &
- ~SUPPORTED_100baseT_Half &
- ~SUPPORTED_1000baseT_Half;
+   phy_remove_link_mode(phy_dev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
+   phy_remove_link_mode(phy_dev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+   phy_remove_link_mode(phy_dev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
phy_dev->supported |= SUPPORTED_Pause |
  SUPPORTED_Asym_Pause;
phy_dev->advertising = phy_dev->supported;
diff --git a/drivers/net/ethernet/cadence/macb_main.c 
b/drivers/net/ethernet/cadence/macb_main.c
index bd4095c3a031..96ae8c992810 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -549,9 +549,8 @@ static int macb_mii_probe(struct net_device *dev)
phy_set_max_speed(phydev, SPEED_100);
 
if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
-   phydev->supported &= ~SUPPORTED_1000baseT_Half;
-
-   phydev->advertising = phydev->supported;
+   phy_remove_link_mode(phydev,
+ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
 
bp->link = 0;
bp->speed = 0;
diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 5e849510c689..0c6fd77b6599 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1947,7 +1947,8 @@ static int fec_enet_mii_probe(struct net_device *ndev)
/* mask with MAC supported features */
if (fep->quirks & FEC_QUIRK_HAS_GBIT) {
phy_set_max_speed(phy_dev, 1000);
-   phy_dev->supported &= ~SUPPORTED_1000baseT_Half;
+   phy_remove_link_mode(phy_dev,
+ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
 #if !defined(CONFIG_M5272)
phy_dev->supported |= SUPPORTED_Pause;
 #endif
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c 
b/drivers/net/ethernet/microchip/lan743x_main.c
index e7dce79ff2c9..048307959c01 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1013,7 +1013,7 @@ static int lan743x_phy_open(struct lan743x_adapter 
*adapter)
goto return_error;
 
/* MAC doesn't support 1000T Half */
-   phydev->supported &= ~SUPPORTED_1000baseT_Half;
+   phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
 
/* support both flow controls */
phy->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
b/drivers/net/ethernet/renesas/ravb_main.c
index aff5516b781e..fb2a1125780d 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1074,7 +1074,8 @@ static int ravb_phy_init(struct net_device *ndev)
}
 
/* 10BASE is not supported */
-   phydev->supported &= ~PHY_10BT_FEATURES;
+   phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
+   phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
 
phy_attached_info(phydev);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3d7aec7a050b..3715a0a4af3c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -993,10 +993,14 @@ static int stmmac_init_phy(struct net_device *dev)
 * Half-duplex mode not supported with multiqueue
 * half-duplex can only works with single queue
 */
-   if (tx_cnt > 1)
-   phydev->supported &= ~(SUPPORTED_1000baseT_Half |
-