[net PATCH 1/1] net: phy: fix phy link up when limiting speed via device tree

[net PATCH 1/1] net: phy: fix phy link up when limiting speed via device tree

[Mugunthan V N]

When limiting phy link speed using "max-speed" to 100mbps or less on a
giga bit phy, phy never completes auto negotiation and phy state
machine is held in PHY_AN. Fixing this issue by comparing the giga
bit advertise though phydev->supported doesn't have it but phy has
BMSR_ESTATEN set. So that auto negotiation is restarted as old and
new advertise are different and link comes up fine.

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/net/phy/phy_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bdfe51f..d551df6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -796,10 +796,11 @@ static int genphy_config_advert(struct phy_device *phydev)
 	if (phydev->supported & (SUPPORTED_1000baseT_Half |
 				 SUPPORTED_1000baseT_Full)) {
 		adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
-		if (adv != oldadv)
-			changed = 1;
 	}
 
+	if (adv != oldadv)
+		changed = 1;
+
 	err = phy_write(phydev, MII_CTRL1000, adv);
 	if (err < 0)
 		return err;
-- 
2.4.4.409.g5b1d901

Re: [net PATCH 1/1] net: phy: fix phy link up when limiting speed via device tree

[Florian Fainelli]

2015-06-25 9:51 GMT-07:00 Mugunthan V N <mugunthanvnm@ti.com>:
> When limiting phy link speed using "max-speed" to 100mbps or less on a
> giga bit phy, phy never completes auto negotiation and phy state
> machine is held in PHY_AN. Fixing this issue by comparing the giga
> bit advertise though phydev->supported doesn't have it but phy has
> BMSR_ESTATEN set. So that auto negotiation is restarted as old and
> new advertise are different and link comes up fine.

This looks valid, however, I am curious why this part of the code:

        oldadv = adv;
        adv &= ~(ADVERTISE_ALL | ADVERTISE_100BASE4 | ADVERTISE_PAUSE_CAP |
                 ADVERTISE_PAUSE_ASYM);
        adv |= ethtool_adv_to_mii_adv_t(advertise);

        if (adv != oldadv) {
                err = phy_write(phydev, MII_ADVERTISE, adv);

                if (err < 0)
                        return err;
                changed = 1;
        }

Is not flagging this as a change already?

>
> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  drivers/net/phy/phy_device.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index bdfe51f..d551df6 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -796,10 +796,11 @@ static int genphy_config_advert(struct phy_device *phydev)
>         if (phydev->supported & (SUPPORTED_1000baseT_Half |
>                                  SUPPORTED_1000baseT_Full)) {
>                 adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
> -               if (adv != oldadv)
> -                       changed = 1;
>         }
>
> +       if (adv != oldadv)
> +               changed = 1;
> +
>         err = phy_write(phydev, MII_CTRL1000, adv);
>         if (err < 0)
>                 return err;
> --
> 2.4.4.409.g5b1d901
>



-- 
Florian

Re: [net PATCH 1/1] net: phy: fix phy link up when limiting speed via device tree

[Mugunthan V N]

On Thursday 25 June 2015 11:01 PM, Florian Fainelli wrote:
> 2015-06-25 9:51 GMT-07:00 Mugunthan V N <mugunthanvnm@ti.com>:
>> > When limiting phy link speed using "max-speed" to 100mbps or less on a
>> > giga bit phy, phy never completes auto negotiation and phy state
>> > machine is held in PHY_AN. Fixing this issue by comparing the giga
>> > bit advertise though phydev->supported doesn't have it but phy has
>> > BMSR_ESTATEN set. So that auto negotiation is restarted as old and
>> > new advertise are different and link comes up fine.
> This looks valid, however, I am curious why this part of the code:
> 
>         oldadv = adv;
>         adv &= ~(ADVERTISE_ALL | ADVERTISE_100BASE4 | ADVERTISE_PAUSE_CAP |
>                  ADVERTISE_PAUSE_ASYM);
>         adv |= ethtool_adv_to_mii_adv_t(advertise);
> 
>         if (adv != oldadv) {
>                 err = phy_write(phydev, MII_ADVERTISE, adv);
> 
>                 if (err < 0)
>                         return err;
>                 changed = 1;
>         }
> 
> Is not flagging this as a change already?
> 

This can flag a change when the selected limit is 10mbps, if 100mbps is
selected, at this instant oldadv and adv are same so change is not
flagged here.

Regards
Mugunthan V N

Re: [net PATCH 1/1] net: phy: fix phy link up when limiting speed via device tree

[Florian Fainelli]

Le 06/25/15 09:51, Mugunthan V N a écrit :
> When limiting phy link speed using "max-speed" to 100mbps or less on a
> giga bit phy, phy never completes auto negotiation and phy state
> machine is held in PHY_AN. Fixing this issue by comparing the giga
> bit advertise though phydev->supported doesn't have it but phy has
> BMSR_ESTATEN set. So that auto negotiation is restarted as old and
> new advertise are different and link comes up fine.
> 
> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  drivers/net/phy/phy_device.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index bdfe51f..d551df6 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -796,10 +796,11 @@ static int genphy_config_advert(struct phy_device *phydev)
>  	if (phydev->supported & (SUPPORTED_1000baseT_Half |
>  				 SUPPORTED_1000baseT_Full)) {
>  		adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
> -		if (adv != oldadv)
> -			changed = 1;
>  	}
>  
> +	if (adv != oldadv)
> +		changed = 1;
> +
>  	err = phy_write(phydev, MII_CTRL1000, adv);
>  	if (err < 0)
>  		return err;
> 


-- 
Florian

Re: [net PATCH 1/1] net: phy: fix phy link up when limiting speed via device tree

[Mugunthan V N]

On Friday 26 June 2015 08:33 AM, Florian Fainelli wrote:
> Le 06/25/15 09:51, Mugunthan V N a écrit :
>> > When limiting phy link speed using "max-speed" to 100mbps or less on a
>> > giga bit phy, phy never completes auto negotiation and phy state
>> > machine is held in PHY_AN. Fixing this issue by comparing the giga
>> > bit advertise though phydev->supported doesn't have it but phy has
>> > BMSR_ESTATEN set. So that auto negotiation is restarted as old and
>> > new advertise are different and link comes up fine.
>> > 
>> > Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 

Dave

Forgot to tag the patch to stable@vger.kernel.org for v4.1 lts, should I
send a separate patch to Greg KH or you still hold a branch to be sent
for v4.1?

Regards
Mugunthan V N

Re: [net PATCH 1/1] net: phy: fix phy link up when limiting speed via device tree

[David Miller]

From: Mugunthan V N <mugunthanvnm@ti.com>
Date: Thu, 25 Jun 2015 22:21:02 +0530

> When limiting phy link speed using "max-speed" to 100mbps or less on a
> giga bit phy, phy never completes auto negotiation and phy state
> machine is held in PHY_AN. Fixing this issue by comparing the giga
> bit advertise though phydev->supported doesn't have it but phy has
> BMSR_ESTATEN set. So that auto negotiation is restarted as old and
> new advertise are different and link comes up fine.
> 
> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>

Applied and queued up for -stable, thanks.