[PATCH v2] net: ethernet: xilinx_emaclite: set protocol selector bits when writing ANAR

[PATCH v2] net: ethernet: xilinx_emaclite: set protocol selector bits when writing ANAR

[renner]

This patch sets the protocol selector bits (4:0) of the PHY's MII_ADVERTISE
register (ANAR) when writing ADVERTISE_ALL. The protocol selector bits are
indicating IEEE 803.3u support and are fixed / read-only on some PHYs. Not
setting them correctly on others (like TI DP83630) makes the PHY fall back
to 10M HDX mode which should be avoided.

Tested for TI DP83630 PHY on Microblaze platform.

Signed-off-by: Jens Renner <renner@efe-gmbh.de>
---
Changes in v2:
- set ADVERTISE_ALL and protocol selector ADVERTISE_CSMA while all other
  bits will be cleared
- adapted patch title & description (original title: "net: ethernet:
  xilinx_emaclite: keep protocol selector bits by reading ANAR")

>>> David Laight <David.Laight@ACULAB.COM> hat am 28. Mai 2013 um 18:22
>>> geschrieben:
[...]
>>> 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.

drivers/net/ethernet/xilinx/xilinx_emaclite.c   |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
index 919b983..b7268b3 100644
--- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
+++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
@@ -946,7 +946,8 @@ 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);
+               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL |
+                         ADVERTISE_CSMA);

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

Re: [PATCH v2] net: ethernet: xilinx_emaclite: set protocol selector bits when writing ANAR

[Michal Simek]

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

On 05/30/2013 12:36 AM, renner@efe-gmbh.de wrote:
> This patch sets the protocol selector bits (4:0) of the PHY's MII_ADVERTISE
> register (ANAR) when writing ADVERTISE_ALL. The protocol selector bits are
> indicating IEEE 803.3u support and are fixed / read-only on some PHYs. Not
> setting them correctly on others (like TI DP83630) makes the PHY fall back
> to 10M HDX mode which should be avoided.
> 
> Tested for TI DP83630 PHY on Microblaze platform.
> 
> Signed-off-by: Jens Renner <renner@efe-gmbh.de>
> ---
> Changes in v2:
> - set ADVERTISE_ALL and protocol selector ADVERTISE_CSMA while all other
>   bits will be cleared
> - adapted patch title & description (original title: "net: ethernet:
>   xilinx_emaclite: keep protocol selector bits by reading ANAR")
> 
>>>> David Laight <David.Laight@ACULAB.COM> hat am 28. Mai 2013 um 18:22
>>>> geschrieben:
> [...]
>>>> 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.
> 
> drivers/net/ethernet/xilinx/xilinx_emaclite.c   |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
> b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
> index 919b983..b7268b3 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
> @@ -946,7 +946,8 @@ 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);
> +               phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL |
> +                         ADVERTISE_CSMA);
> 
>                 /* Restart auto negotiation */
>                 bmcr = phy_read(lp->phy_dev, MII_BMCR);
> 

I have tested it on sp605 and there is no problem with autonegotiation.
On others xilinx boards behaviour should be the same.
Tested-by: Michal Simek <monstr@monstr.eu>

BTW: Going to send v2 for emaclite patches, I will CC you and will
be good if you can test them.

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 v2] net: ethernet: xilinx_emaclite: set protocol selector bits when writing ANAR

[David Miller]

From: "renner@efe-gmbh.de" <renner@efe-gmbh.de>
Date: Thu, 30 May 2013 00:36:18 +0200 (CEST)

> This patch sets the protocol selector bits (4:0) of the PHY's MII_ADVERTISE
> register (ANAR) when writing ADVERTISE_ALL. The protocol selector bits are
> indicating IEEE 803.3u support and are fixed / read-only on some PHYs. Not
> setting them correctly on others (like TI DP83630) makes the PHY fall back
> to 10M HDX mode which should be avoided.
> 
> Tested for TI DP83630 PHY on Microblaze platform.
> 
> Signed-off-by: Jens Renner <renner@efe-gmbh.de>

This patch has been corrupted by your email client, among other things it
has corrupted the whitespace in the patch, changing TAB characters into
sequences of spaces.

Please read Documentation/email-clients.txt in the kernel sources, and then
send a test patch to yourself.  Do not resubmit this patch to the list until
you can successfully apply the test patch you email to yourself.

Thanks.