[PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[Jens Renner (EFE)]

This patch reads the PHY's MII_ADVERTISE register (ANAR) before modifying it.
Hence, the protocol selector bits (4:0) which indicate IEEE 803.3u support are
prevented from being cleared. While the selector bits are fixed / read-only on
some PHYs, not setting them correctly on others (like TI DP83630) makes the
PHY fall back to 10/half mode which should be avoided.

Signed-off-by: Jens Renner <renner@efe-gmbh.de>
---
drivers/net/ethernet/xilinx/xilinx_emaclite.c               |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
index 919b983..4a67af1 100644
--- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
+++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
@@ -922,7 +922,7 @@ void xemaclite_adjust_link(struct net_device *ndev)
 static int xemaclite_open(struct net_device *dev)
 {
        struct net_local *lp = netdev_priv(dev);
-       int retval;
+       int retval, adv;

        /* Just to be safe, stop the device first */
        xemaclite_disable_interrupts(lp);
@@ -946,7 +946,10 @@ static int xemaclite_open(struct net_device *dev)
                phy_write(lp->phy_dev, MII_CTRL1000, 0);

                /* Advertise only 10 and 100mbps full/half duplex speeds */
-               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL);
+               adv = phy_read(lp->phy_dev, MII_ADVERTISE);
+               if (adv < 0)
+                       return adv;
+               phy_write(lp->phy_dev, MII_ADVERTISE, adv | ADVERTISE_ALL);

                /* Restart auto negotiation */
                bmcr = phy_read(lp->phy_dev, MII_BMCR);

RE: [PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[David Laight]

> This patch reads the PHY's MII_ADVERTISE register (ANAR) before modifying it.
> Hence, the protocol selector bits (4:0) which indicate IEEE 803.3u support are
> prevented from being cleared.
> ...
>                 /* Advertise only 10 and 100mbps full/half duplex speeds */
> -               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL);
> +               adv = phy_read(lp->phy_dev, MII_ADVERTISE);
> +               if (adv < 0)
> +                       return adv;
> +               phy_write(lp->phy_dev, MII_ADVERTISE, adv | ADVERTISE_ALL);

Given the comment shouldn't this be removing some bits?

It is a long time since I read the definitions of the ANAR (etc).
But I remember it having at least 5 bits defined for 10M and 100M
operation (including 100T4), so the reference to keeping bits (4:0)
seems confusing.

	David

RE: [PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[renner]

> David Laight <David.Laight@ACULAB.COM> hat am 28. Mai 2013 um 18:22
> geschrieben:
>
>
> > This patch reads the PHY's MII_ADVERTISE register (ANAR) before modifying
> > it.
> > Hence, the protocol selector bits (4:0) which indicate IEEE 803.3u support
> > are
> > prevented from being cleared.
> > ...
> >                 /* Advertise only 10 and 100mbps full/half duplex speeds */
> > -               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL);
> > +               adv = phy_read(lp->phy_dev, MII_ADVERTISE);
> > +               if (adv < 0)
> > +                       return adv;
> > +               phy_write(lp->phy_dev, MII_ADVERTISE, adv | ADVERTISE_ALL);
>
> Given the comment shouldn't this be removing some bits?
>
> It is a long time since I read the definitions of the ANAR (etc).
> But I remember it having at least 5 bits defined for 10M and 100M
> operation (including 100T4), so the reference to keeping bits (4:0)
> seems confusing.
>
>    David
>

David, thanks for your feedback!
The bits I mentioned (the 5 least bits in ANAR) are the so-called protocol
selection bits (at least that's what they call it at Intel, TI, Marvell, Maxim,
etc.). The only value that seems actually to be used is 1 (0b00001) which means
802.3 / fast ethernet capability (maybe 0 for slower devices?). 1 is the
default value for all the 10/100(/1000) PHYs I know of, in some PHYs it is even
hardcoded (i.e. read-only). It must not be confused with ANAR bits 9:5 where
the actual speed and duplex mode can be selected.
ADVERTISE_ALL as defined in mii.h lacks this bit, in contrast to ADVERTISE_FULL
where it is set through ADVERTISE_CSMA. Whether this is correct or not; at
least it makes some sense to me to read what's actually written in these
register bits and not to clear them. Otherwise some PHYs might fall back to
10-half as seen for the TI DP83640 which has been used to test this patch.
I hope this explains the reason for this patch.

Jens.

RE: [PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[David Laight]

> > David Laight <David.Laight@ACULAB.COM> hat am 28. Mai 2013 um 18:22
> > geschrieben:
> >
> >
> > > This patch reads the PHY's MII_ADVERTISE register (ANAR) before modifying
> > > it.
> > > Hence, the protocol selector bits (4:0) which indicate IEEE 803.3u support
> > > are
> > > prevented from being cleared.
> > > ...
> > >                 /* Advertise only 10 and 100mbps full/half duplex speeds */
> > > -               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL);
> > > +               adv = phy_read(lp->phy_dev, MII_ADVERTISE);
> > > +               if (adv < 0)
> > > +                       return adv;
> > > +               phy_write(lp->phy_dev, MII_ADVERTISE, adv | ADVERTISE_ALL);
> >
> > Given the comment shouldn't this be removing some bits?
> >
> > It is a long time since I read the definitions of the ANAR (etc).
> > But I remember it having at least 5 bits defined for 10M and 100M
> > operation (including 100T4), so the reference to keeping bits (4:0)
> > seems confusing.
> >
> >    David
> >
> 
> David, thanks for your feedback!
> The bits I mentioned (the 5 least bits in ANAR) are the so-called protocol
> selection bits (at least that's what they call it at Intel, TI, Marvell, Maxim,
> etc.). The only value that seems actually to be used is 1 (0b00001) which means
> 802.3 / fast ethernet capability (maybe 0 for slower devices?). 1 is the
> default value for all the 10/100(/1000) PHYs I know of, in some PHYs it is even
> hardcoded (i.e. read-only). It must not be confused with ANAR bits 9:5 where
> the actual speed and duplex mode can be selected.
> ADVERTISE_ALL as defined in mii.h lacks this bit, in contrast to ADVERTISE_FULL
> where it is set through ADVERTISE_CSMA. Whether this is correct or not; at
> least it makes some sense to me to read what's actually written in these
> register bits and not to clear them. Otherwise some PHYs might fall back to
> 10-half as seen for the TI DP83640 which has been used to test this patch.
> I hope this explains the reason for this patch.

Part of my confusion was that I'd forgotten that the MII registers are 16bit!
(I said it had been a long time!)

However if you want to advertise 10M/100M HDX and FDX you need to unset
the other speed bits (100T4 was assigned a bit, and I'm sure there were
extra bits reserved for higher speeds - which also need clearing).
Given that the ANAR is defined by 803.x you shouldn't really have a problem!

My real bugbear is that none of the standard MII registers actually tells
you the mode the PHY is working in! Not sure how that got through the
standards body.

	David

Re: [PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[Jens Renner (EFE)]

Am 29.05.2013 10:51, schrieb David Laight:
>>> David Laight <David.Laight@ACULAB.COM> hat am 28. Mai 2013 um 18:22
>>> geschrieben:
>>>
>>>
>>>> This patch reads the PHY's MII_ADVERTISE register (ANAR) before modifying
>>>> it.
>>>> Hence, the protocol selector bits (4:0) which indicate IEEE 803.3u support
>>>> are
>>>> prevented from being cleared.
>>>> ...
>>>>                 /* Advertise only 10 and 100mbps full/half duplex speeds */
>>>> -               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL);
>>>> +               adv = phy_read(lp->phy_dev, MII_ADVERTISE);
>>>> +               if (adv < 0)
>>>> +                       return adv;
>>>> +               phy_write(lp->phy_dev, MII_ADVERTISE, adv | ADVERTISE_ALL);
>>>
>>> Given the comment shouldn't this be removing some bits?
>>>
>>> It is a long time since I read the definitions of the ANAR (etc).
>>> But I remember it having at least 5 bits defined for 10M and 100M
>>> operation (including 100T4), so the reference to keeping bits (4:0)
>>> seems confusing.
>>>
>>>    David
>>>
>>
>> David, thanks for your feedback!
>> The bits I mentioned (the 5 least bits in ANAR) are the so-called protocol
>> selection bits (at least that's what they call it at Intel, TI, Marvell, Maxim,
>> etc.). The only value that seems actually to be used is 1 (0b00001) which means
>> 802.3 / fast ethernet capability (maybe 0 for slower devices?). 1 is the
>> default value for all the 10/100(/1000) PHYs I know of, in some PHYs it is even
>> hardcoded (i.e. read-only). It must not be confused with ANAR bits 9:5 where
>> the actual speed and duplex mode can be selected.
>> ADVERTISE_ALL as defined in mii.h lacks this bit, in contrast to ADVERTISE_FULL
>> where it is set through ADVERTISE_CSMA. Whether this is correct or not; at
>> least it makes some sense to me to read what's actually written in these
>> register bits and not to clear them. Otherwise some PHYs might fall back to
>> 10-half as seen for the TI DP83640 which has been used to test this patch.
>> I hope this explains the reason for this patch.
> 

[...]

> However if you want to advertise 10M/100M HDX and FDX you need to unset
> the other speed bits (100T4 was assigned a bit, and I'm sure there were
> extra bits reserved for higher speeds - which also need clearing).
> Given that the ANAR is defined by 803.x you shouldn't really have a problem!

[...]

You are right. The 1000BASE-T capability is advertised in another register and
already gets cleared one code line before (see original list mail), while the
100BASE-T4 bit now remains unchanged with my patch which is not intended.
Consequently the new patch would look like this:
-               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL);
+               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL |
+                         ADVERTISE_CSMA);
This only advertises HDX & FDX for both 10M / 100M, sets the 802.3 selector
bits as defined in ADVERTISE_CSMA, and clears all other bits which should be
safe. It is done a similar way in some other drivers.
I will prepare a patch resend then.

Jens.

Re: [PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

On 05/28/2013 06:10 PM, Jens Renner (EFE) wrote:
> This patch reads the PHY's MII_ADVERTISE register (ANAR) before modifying it.
> Hence, the protocol selector bits (4:0) which indicate IEEE 803.3u support are
> prevented from being cleared. While the selector bits are fixed / read-only on
> some PHYs, not setting them correctly on others (like TI DP83630) makes the
> PHY fall back to 10/half mode which should be avoided.
> 
> Signed-off-by: Jens Renner <renner@efe-gmbh.de>
> ---
> drivers/net/ethernet/xilinx/xilinx_emaclite.c               |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
> index 919b983..4a67af1 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
> @@ -922,7 +922,7 @@ void xemaclite_adjust_link(struct net_device *ndev)
>  static int xemaclite_open(struct net_device *dev)
>  {
>         struct net_local *lp = netdev_priv(dev);
> -       int retval;
> +       int retval, adv;
> 
>         /* Just to be safe, stop the device first */
>         xemaclite_disable_interrupts(lp);
> @@ -946,7 +946,10 @@ static int xemaclite_open(struct net_device *dev)
>                 phy_write(lp->phy_dev, MII_CTRL1000, 0);
> 
>                 /* Advertise only 10 and 100mbps full/half duplex speeds */
> -               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL);
> +               adv = phy_read(lp->phy_dev, MII_ADVERTISE);
> +               if (adv < 0)
> +                       return adv;
> +               phy_write(lp->phy_dev, MII_ADVERTISE, adv | ADVERTISE_ALL);
> 
>                 /* Restart auto negotiation */
>                 bmcr = phy_read(lp->phy_dev, MII_BMCR);

Acked-by: Michal Simek <monstr@monstr.eu>

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[Jens Renner (EFE)]

Am 29.05.2013 17:23, schrieb Michal Simek:
> On 05/28/2013 06:10 PM, Jens Renner (EFE) wrote:
>> This patch reads the PHY's MII_ADVERTISE register (ANAR) before modifying
>> it. Hence, the protocol selector bits (4:0) which indicate IEEE 803.3u
>> support are prevented from being cleared. While the selector bits are
>> fixed / read-only on some PHYs, not setting them correctly on others
>> (like TI DP83630) makes the PHY fall back to 10/half mode which should be
>> avoided.
>> 
>> Signed-off-by: Jens Renner <renner@efe-gmbh.de> --- 
>> drivers/net/ethernet/xilinx/xilinx_emaclite.c               |    7
>> +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
>> b/drivers/net/ethernet/xilinx/xilinx_emaclite.c index 919b983..4a67af1
>> 100644 --- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c +++
>> b/drivers/net/ethernet/xilinx/xilinx_emaclite.c @@ -922,7 +922,7 @@ void
>> xemaclite_adjust_link(struct net_device *ndev) static int
>> xemaclite_open(struct net_device *dev) { struct net_local *lp =
>> netdev_priv(dev); -       int retval; +       int retval, adv;
>> 
>> /* Just to be safe, stop the device first */ 
>> xemaclite_disable_interrupts(lp); @@ -946,7 +946,10 @@ static int
>> xemaclite_open(struct net_device *dev) phy_write(lp->phy_dev,
>> MII_CTRL1000, 0);
>> 
>> /* Advertise only 10 and 100mbps full/half duplex speeds */ -
>> phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL); +               adv
>> = phy_read(lp->phy_dev, MII_ADVERTISE); +               if (adv < 0) +
>> return adv; +               phy_write(lp->phy_dev, MII_ADVERTISE, adv |
>> ADVERTISE_ALL);
>> 
>> /* Restart auto negotiation */ bmcr = phy_read(lp->phy_dev, MII_BMCR);
> 
> Acked-by: Michal Simek <monstr@monstr.eu>
> 
> Thanks, Michal
> 
> 

Michal, please mind the follow-ups to my original list mail:
http://lists.openwall.net/netdev/2013/05/28/126
http://lists.openwall.net/netdev/2013/05/29/140
There will be a patch v2 later this day (I will put you in CC).
Thanks,
Jens.

Re: [PATCH] net: ethernet: xilinx_emaclite: keep protocol selector bits by reading ANAR

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

On 05/29/2013 06:54 PM, Jens Renner (EFE) wrote:
> Am 29.05.2013 17:23, schrieb Michal Simek:
>> On 05/28/2013 06:10 PM, Jens Renner (EFE) wrote:
>>> This patch reads the PHY's MII_ADVERTISE register (ANAR) before modifying
>>> it. Hence, the protocol selector bits (4:0) which indicate IEEE 803.3u
>>> support are prevented from being cleared. While the selector bits are
>>> fixed / read-only on some PHYs, not setting them correctly on others
>>> (like TI DP83630) makes the PHY fall back to 10/half mode which should be
>>> avoided.
>>>
>>> Signed-off-by: Jens Renner <renner@efe-gmbh.de> --- 
>>> drivers/net/ethernet/xilinx/xilinx_emaclite.c               |    7
>>> +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
>>> b/drivers/net/ethernet/xilinx/xilinx_emaclite.c index 919b983..4a67af1
>>> 100644 --- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c +++
>>> b/drivers/net/ethernet/xilinx/xilinx_emaclite.c @@ -922,7 +922,7 @@ void
>>> xemaclite_adjust_link(struct net_device *ndev) static int
>>> xemaclite_open(struct net_device *dev) { struct net_local *lp =
>>> netdev_priv(dev); -       int retval; +       int retval, adv;
>>>
>>> /* Just to be safe, stop the device first */ 
>>> xemaclite_disable_interrupts(lp); @@ -946,7 +946,10 @@ static int
>>> xemaclite_open(struct net_device *dev) phy_write(lp->phy_dev,
>>> MII_CTRL1000, 0);
>>>
>>> /* Advertise only 10 and 100mbps full/half duplex speeds */ -
>>> phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL); +               adv
>>> = phy_read(lp->phy_dev, MII_ADVERTISE); +               if (adv < 0) +
>>> return adv; +               phy_write(lp->phy_dev, MII_ADVERTISE, adv |
>>> ADVERTISE_ALL);
>>>
>>> /* Restart auto negotiation */ bmcr = phy_read(lp->phy_dev, MII_BMCR);
>>
>> Acked-by: Michal Simek <monstr@monstr.eu>
>>
>> Thanks, Michal
>>
>>
> 
> Michal, please mind the follow-ups to my original list mail:
> http://lists.openwall.net/netdev/2013/05/28/126
> http://lists.openwall.net/netdev/2013/05/29/140
> There will be a patch v2 later this day (I will put you in CC).

Ok. I have tested it on xilinx qemu model and your patch doesn't
break our existing driver usage that's why I gave you my ACK.
And yes, please CC me I will retest your v2.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]