diff mbox

net: mvneta: properly disable HW PHY polling and ensure adjust_link() works

Message ID 1378304478-21237-1-git-send-email-thomas.petazzoni@free-electrons.com
State New
Headers show

Commit Message

Thomas Petazzoni Sept. 4, 2013, 2:21 p.m. UTC
This commit fixes a long-standing bug that has been reported by many
users: on some Armada 370 platforms, only the network interface that
has been used in U-Boot to tftp the kernel works properly in
Linux. The other network interfaces can see a 'link up', but are
unable to transmit data. The reports were generally made on the Armada
370-based Mirabox, but have also been given on the Armada 370-RD
board.

The network MAC in the Armada 370/XP (supported by the mvneta driver
in Linux) has a functionality that allows it to continuously poll the
PHY and directly update the MAC configuration accordingly (speed,
duplex, etc.). The very first versions of the driver submitted for
review were using this hardware mechanism, but due to this, the driver
was not integrated with the kernel phylib. Following reviews, the
driver was changed to use the phylib, and therefore a software based
polling. In software based polling, Linux regularly talks to the PHY
over the MDIO bus, and sees if the link status has changed. If it's
the case then the adjust_link() callback of the driver is called to
update the MAC configuration accordingly.

However, it turns out that the adjust_link() callback was not
configuring the hardware in a completely correct way: while it was
setting the speed and duplex bits correctly, it wasn't telling the
hardware to actually take into account those bits rather than what the
hardware-based PHY polling mechanism has concluded. So, in fact the
adjust_link() callback was basically a no-op.

However, the network happened to be working because on the network
interfaces used by U-Boot for tftp on Armada 370 platforms because the
hardware PHY polling was enabled by the bootloader, and left enabled
by Linux. However, the second network interface not used for tftp (or
both network interfaces if the kernel is loaded from USB, NAND or SD
card) didn't had the hardware PHY polling enabled.

This patch fixes this situation by:

 (1) Making sure that the hardware PHY polling is disabled by clearing
     the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
     register in the driver ->probe() function.

 (2) Making sure that the duplex and speed selections made by the
     adjust_link() callback are taken into account by clearing the
     MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
     MVNETA_GMAC_AUTONEG_CONFIG register.

This patch has been tested on Armada 370 Mirabox, and now both network
interfaces are usable after boot.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Willy Tarreau <w@1wt.eu>
Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
Cc: Peter Sanford <psanford@nearbuy.io>
Cc: Ethan Tuttle <ethan@ethantuttle.com>
Cc: Chény Yves-Gael <yves@cheny.fr>
Cc: Ryan Press <ryan@presslab.us>
Cc: Simon Guinot <simon.guinot@sequanux.org>
Cc: vdonnefort@lacie.com
Cc: stable@vger.kernel.org
---
David, this patch is a fix for a problem that has been here since 3.8
(when the mvneta driver was introduced), so I've Cc'ed stable@ and if
possible I'd like to patch to be included for 3.12.
---
 drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Jason Cooper Sept. 4, 2013, 2:50 p.m. UTC | #1
On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
> This commit fixes a long-standing bug that has been reported by many
> users: on some Armada 370 platforms, only the network interface that
> has been used in U-Boot to tftp the kernel works properly in
> Linux. The other network interfaces can see a 'link up', but are
> unable to transmit data. The reports were generally made on the Armada
> 370-based Mirabox, but have also been given on the Armada 370-RD
> board.
> 
> The network MAC in the Armada 370/XP (supported by the mvneta driver
> in Linux) has a functionality that allows it to continuously poll the
> PHY and directly update the MAC configuration accordingly (speed,
> duplex, etc.). The very first versions of the driver submitted for
> review were using this hardware mechanism, but due to this, the driver
> was not integrated with the kernel phylib. Following reviews, the
> driver was changed to use the phylib, and therefore a software based
> polling. In software based polling, Linux regularly talks to the PHY
> over the MDIO bus, and sees if the link status has changed. If it's
> the case then the adjust_link() callback of the driver is called to
> update the MAC configuration accordingly.
> 
> However, it turns out that the adjust_link() callback was not
> configuring the hardware in a completely correct way: while it was
> setting the speed and duplex bits correctly, it wasn't telling the
> hardware to actually take into account those bits rather than what the
> hardware-based PHY polling mechanism has concluded. So, in fact the
> adjust_link() callback was basically a no-op.
> 
> However, the network happened to be working because on the network
> interfaces used by U-Boot for tftp on Armada 370 platforms because the
> hardware PHY polling was enabled by the bootloader, and left enabled
> by Linux. However, the second network interface not used for tftp (or
> both network interfaces if the kernel is loaded from USB, NAND or SD
> card) didn't had the hardware PHY polling enabled.
> 
> This patch fixes this situation by:
> 
>  (1) Making sure that the hardware PHY polling is disabled by clearing
>      the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
>      register in the driver ->probe() function.
> 
>  (2) Making sure that the duplex and speed selections made by the
>      adjust_link() callback are taken into account by clearing the
>      MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
>      MVNETA_GMAC_AUTONEG_CONFIG register.
> 
> This patch has been tested on Armada 370 Mirabox, and now both network
> interfaces are usable after boot.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Willy Tarreau <w@1wt.eu>
> Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
> Cc: Peter Sanford <psanford@nearbuy.io>
> Cc: Ethan Tuttle <ethan@ethantuttle.com>
> Cc: Chény Yves-Gael <yves@cheny.fr>
> Cc: Ryan Press <ryan@presslab.us>
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> Cc: vdonnefort@lacie.com
> Cc: stable@vger.kernel.org
> ---
> David, this patch is a fix for a problem that has been here since 3.8
> (when the mvneta driver was introduced), so I've Cc'ed stable@ and if
> possible I'd like to patch to be included for 3.12.

David,

Offending patch is:

  c5aff18 net: mvneta: driver for Marvell Armada 370/XP network unit

Applies and builds cleanly against v3.8.13, v3.9.11, v3.10.10, and v3.11

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

> ---
>  drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index b017818..90ab292 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -138,7 +138,9 @@
>  #define      MVNETA_GMAC_FORCE_LINK_PASS         BIT(1)
>  #define      MVNETA_GMAC_CONFIG_MII_SPEED        BIT(5)
>  #define      MVNETA_GMAC_CONFIG_GMII_SPEED       BIT(6)
> +#define      MVNETA_GMAC_AN_SPEED_EN             BIT(7)
>  #define      MVNETA_GMAC_CONFIG_FULL_DUPLEX      BIT(12)
> +#define      MVNETA_GMAC_AN_DUPLEX_EN            BIT(13)
>  #define MVNETA_MIB_COUNTERS_BASE                 0x3080
>  #define      MVNETA_MIB_LATE_COLLISION           0x7c
>  #define MVNETA_DA_FILT_SPEC_MCAST                0x3400
> @@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
>  	/* Assign port SDMA configuration */
>  	mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
>  
> +	/* Disable PHY polling in hardware, since we're using the
> +	 * kernel phylib to do this.
> +	 */
> +	val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
> +	val &= ~MVNETA_PHY_POLLING_ENABLE;
> +	mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
> +
>  	mvneta_set_ucast_table(pp, -1);
>  	mvneta_set_special_mcast_table(pp, -1);
>  	mvneta_set_other_mcast_table(pp, -1);
> @@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
>  			val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
>  			val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
>  				 MVNETA_GMAC_CONFIG_GMII_SPEED |
> -				 MVNETA_GMAC_CONFIG_FULL_DUPLEX);
> +				 MVNETA_GMAC_CONFIG_FULL_DUPLEX |
> +				 MVNETA_GMAC_AN_SPEED_EN |
> +				 MVNETA_GMAC_AN_DUPLEX_EN);
>  
>  			if (phydev->duplex)
>  				val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Vincent Donnefort Sept. 4, 2013, 3:20 p.m. UTC | #2
On Wed, Sep 04, 2013 at 10:50:51AM -0400, Jason Cooper wrote:
> On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
> > This commit fixes a long-standing bug that has been reported by many
> > users: on some Armada 370 platforms, only the network interface that
> > has been used in U-Boot to tftp the kernel works properly in
> > Linux. The other network interfaces can see a 'link up', but are
> > unable to transmit data. The reports were generally made on the Armada
> > 370-based Mirabox, but have also been given on the Armada 370-RD
> > board.
> > 
> > The network MAC in the Armada 370/XP (supported by the mvneta driver
> > in Linux) has a functionality that allows it to continuously poll the
> > PHY and directly update the MAC configuration accordingly (speed,
> > duplex, etc.). The very first versions of the driver submitted for
> > review were using this hardware mechanism, but due to this, the driver
> > was not integrated with the kernel phylib. Following reviews, the
> > driver was changed to use the phylib, and therefore a software based
> > polling. In software based polling, Linux regularly talks to the PHY
> > over the MDIO bus, and sees if the link status has changed. If it's
> > the case then the adjust_link() callback of the driver is called to
> > update the MAC configuration accordingly.
> > 
> > However, it turns out that the adjust_link() callback was not
> > configuring the hardware in a completely correct way: while it was
> > setting the speed and duplex bits correctly, it wasn't telling the
> > hardware to actually take into account those bits rather than what the
> > hardware-based PHY polling mechanism has concluded. So, in fact the
> > adjust_link() callback was basically a no-op.
> > 
> > However, the network happened to be working because on the network
> > interfaces used by U-Boot for tftp on Armada 370 platforms because the
> > hardware PHY polling was enabled by the bootloader, and left enabled
> > by Linux. However, the second network interface not used for tftp (or
> > both network interfaces if the kernel is loaded from USB, NAND or SD
> > card) didn't had the hardware PHY polling enabled.
> > 
> > This patch fixes this situation by:
> > 
> >  (1) Making sure that the hardware PHY polling is disabled by clearing
> >      the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
> >      register in the driver ->probe() function.
> > 
> >  (2) Making sure that the duplex and speed selections made by the
> >      adjust_link() callback are taken into account by clearing the
> >      MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
> >      MVNETA_GMAC_AUTONEG_CONFIG register.
> > 
> > This patch has been tested on Armada 370 Mirabox, and now both network
> > interfaces are usable after boot.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Willy Tarreau <w@1wt.eu>
> > Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
> > Cc: Peter Sanford <psanford@nearbuy.io>
> > Cc: Ethan Tuttle <ethan@ethantuttle.com>
> > Cc: Chény Yves-Gael <yves@cheny.fr>
> > Cc: Ryan Press <ryan@presslab.us>
> > Cc: Simon Guinot <simon.guinot@sequanux.org>
> > Cc: vdonnefort@lacie.com
> > Cc: stable@vger.kernel.org
> > ---
> > David, this patch is a fix for a problem that has been here since 3.8
> > (when the mvneta driver was introduced), so I've Cc'ed stable@ and if
> > possible I'd like to patch to be included for 3.12.
> 
> David,
> 
> Offending patch is:
> 
>   c5aff18 net: mvneta: driver for Marvell Armada 370/XP network unit
> 
> Applies and builds cleanly against v3.8.13, v3.9.11, v3.10.10, and v3.11
> 
> Acked-by: Jason Cooper <jason@lakedaemon.net>
> 
> thx,
> 
> Jason.

Works with the armada370-rd board.

Tested-by: Vincent Donnefort <vdonnefort@gmail.com>

Thank you!

Vincent.

> 
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index b017818..90ab292 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -138,7 +138,9 @@
> >  #define      MVNETA_GMAC_FORCE_LINK_PASS         BIT(1)
> >  #define      MVNETA_GMAC_CONFIG_MII_SPEED        BIT(5)
> >  #define      MVNETA_GMAC_CONFIG_GMII_SPEED       BIT(6)
> > +#define      MVNETA_GMAC_AN_SPEED_EN             BIT(7)
> >  #define      MVNETA_GMAC_CONFIG_FULL_DUPLEX      BIT(12)
> > +#define      MVNETA_GMAC_AN_DUPLEX_EN            BIT(13)
> >  #define MVNETA_MIB_COUNTERS_BASE                 0x3080
> >  #define      MVNETA_MIB_LATE_COLLISION           0x7c
> >  #define MVNETA_DA_FILT_SPEC_MCAST                0x3400
> > @@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
> >  	/* Assign port SDMA configuration */
> >  	mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
> >  
> > +	/* Disable PHY polling in hardware, since we're using the
> > +	 * kernel phylib to do this.
> > +	 */
> > +	val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
> > +	val &= ~MVNETA_PHY_POLLING_ENABLE;
> > +	mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
> > +
> >  	mvneta_set_ucast_table(pp, -1);
> >  	mvneta_set_special_mcast_table(pp, -1);
> >  	mvneta_set_other_mcast_table(pp, -1);
> > @@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
> >  			val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
> >  			val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
> >  				 MVNETA_GMAC_CONFIG_GMII_SPEED |
> > -				 MVNETA_GMAC_CONFIG_FULL_DUPLEX);
> > +				 MVNETA_GMAC_CONFIG_FULL_DUPLEX |
> > +				 MVNETA_GMAC_AN_SPEED_EN |
> > +				 MVNETA_GMAC_AN_DUPLEX_EN);
> >  
> >  			if (phydev->duplex)
> >  				val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
> > -- 
> > 1.8.1.2
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
yves@cheny.fr Sept. 4, 2013, 4 p.m. UTC | #3
Le 2013-09-04 17:20, Vincent Donnefort a écrit :
> On Wed, Sep 04, 2013 at 10:50:51AM -0400, Jason Cooper wrote:
>> On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
>> > This commit fixes a long-standing bug that has been reported by many
>> > users: on some Armada 370 platforms, only the network interface that
>> > has been used in U-Boot to tftp the kernel works properly in
>> > Linux. The other network interfaces can see a 'link up', but are
>> > unable to transmit data. The reports were generally made on the Armada
>> > 370-based Mirabox, but have also been given on the Armada 370-RD
>> > board.
>> >
>> > The network MAC in the Armada 370/XP (supported by the mvneta driver
>> > in Linux) has a functionality that allows it to continuously poll the
>> > PHY and directly update the MAC configuration accordingly (speed,
>> > duplex, etc.). The very first versions of the driver submitted for
>> > review were using this hardware mechanism, but due to this, the driver
>> > was not integrated with the kernel phylib. Following reviews, the
>> > driver was changed to use the phylib, and therefore a software based
>> > polling. In software based polling, Linux regularly talks to the PHY
>> > over the MDIO bus, and sees if the link status has changed. If it's
>> > the case then the adjust_link() callback of the driver is called to
>> > update the MAC configuration accordingly.
>> >
>> > However, it turns out that the adjust_link() callback was not
>> > configuring the hardware in a completely correct way: while it was
>> > setting the speed and duplex bits correctly, it wasn't telling the
>> > hardware to actually take into account those bits rather than what the
>> > hardware-based PHY polling mechanism has concluded. So, in fact the
>> > adjust_link() callback was basically a no-op.
>> >
>> > However, the network happened to be working because on the network
>> > interfaces used by U-Boot for tftp on Armada 370 platforms because the
>> > hardware PHY polling was enabled by the bootloader, and left enabled
>> > by Linux. However, the second network interface not used for tftp (or
>> > both network interfaces if the kernel is loaded from USB, NAND or SD
>> > card) didn't had the hardware PHY polling enabled.
>> >
>> > This patch fixes this situation by:
>> >
>> >  (1) Making sure that the hardware PHY polling is disabled by clearing
>> >      the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
>> >      register in the driver ->probe() function.
>> >
>> >  (2) Making sure that the duplex and speed selections made by the
>> >      adjust_link() callback are taken into account by clearing the
>> >      MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
>> >      MVNETA_GMAC_AUTONEG_CONFIG register.
>> >
>> > This patch has been tested on Armada 370 Mirabox, and now both network
>> > interfaces are usable after boot.
>> >
>> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> > Cc: Willy Tarreau <w@1wt.eu>
>> > Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
>> > Cc: Peter Sanford <psanford@nearbuy.io>
>> > Cc: Ethan Tuttle <ethan@ethantuttle.com>
>> > Cc: Chény Yves-Gael <yves@cheny.fr>
>> > Cc: Ryan Press <ryan@presslab.us>
>> > Cc: Simon Guinot <simon.guinot@sequanux.org>
>> > Cc: vdonnefort@lacie.com
>> > Cc: stable@vger.kernel.org
>> > ---
>> > David, this patch is a fix for a problem that has been here since 3.8
>> > (when the mvneta driver was introduced), so I've Cc'ed stable@ and if
>> > possible I'd like to patch to be included for 3.12.
>> 
>> David,
>> 
>> Offending patch is:
>> 
>>   c5aff18 net: mvneta: driver for Marvell Armada 370/XP network unit
>> 
>> Applies and builds cleanly against v3.8.13, v3.9.11, v3.10.10, and 
>> v3.11
>> 
>> Acked-by: Jason Cooper <jason@lakedaemon.net>
>> 
>> thx,
>> 
>> Jason.
> 
> Works with the armada370-rd board.
> 
> Tested-by: Vincent Donnefort <vdonnefort@gmail.com>
> 
> Thank you!
> 
> Vincent.
> 


Works with the mirabox armada370 devkit.

Tested-by: Yves-Gael Cheny <yves@cheny.fr>

Many thx,

Yves-Gaël .



>> 
>> > ---
>> >  drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
>> >  1 file changed, 12 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>> > index b017818..90ab292 100644
>> > --- a/drivers/net/ethernet/marvell/mvneta.c
>> > +++ b/drivers/net/ethernet/marvell/mvneta.c
>> > @@ -138,7 +138,9 @@
>> >  #define      MVNETA_GMAC_FORCE_LINK_PASS         BIT(1)
>> >  #define      MVNETA_GMAC_CONFIG_MII_SPEED        BIT(5)
>> >  #define      MVNETA_GMAC_CONFIG_GMII_SPEED       BIT(6)
>> > +#define      MVNETA_GMAC_AN_SPEED_EN             BIT(7)
>> >  #define      MVNETA_GMAC_CONFIG_FULL_DUPLEX      BIT(12)
>> > +#define      MVNETA_GMAC_AN_DUPLEX_EN            BIT(13)
>> >  #define MVNETA_MIB_COUNTERS_BASE                 0x3080
>> >  #define      MVNETA_MIB_LATE_COLLISION           0x7c
>> >  #define MVNETA_DA_FILT_SPEC_MCAST                0x3400
>> > @@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
>> >  	/* Assign port SDMA configuration */
>> >  	mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
>> >
>> > +	/* Disable PHY polling in hardware, since we're using the
>> > +	 * kernel phylib to do this.
>> > +	 */
>> > +	val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
>> > +	val &= ~MVNETA_PHY_POLLING_ENABLE;
>> > +	mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
>> > +
>> >  	mvneta_set_ucast_table(pp, -1);
>> >  	mvneta_set_special_mcast_table(pp, -1);
>> >  	mvneta_set_other_mcast_table(pp, -1);
>> > @@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
>> >  			val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
>> >  			val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
>> >  				 MVNETA_GMAC_CONFIG_GMII_SPEED |
>> > -				 MVNETA_GMAC_CONFIG_FULL_DUPLEX);
>> > +				 MVNETA_GMAC_CONFIG_FULL_DUPLEX |
>> > +				 MVNETA_GMAC_AN_SPEED_EN |
>> > +				 MVNETA_GMAC_AN_DUPLEX_EN);
>> >
>> >  			if (phydev->duplex)
>> >  				val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
>> > --
>> > 1.8.1.2
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Gregory CLEMENT Sept. 4, 2013, 4:08 p.m. UTC | #4
On 04/09/2013 16:21, Thomas Petazzoni wrote:
> This commit fixes a long-standing bug that has been reported by many
> users: on some Armada 370 platforms, only the network interface that
> has been used in U-Boot to tftp the kernel works properly in
> Linux. The other network interfaces can see a 'link up', but are
> unable to transmit data. The reports were generally made on the Armada
> 370-based Mirabox, but have also been given on the Armada 370-RD
> board.
> 
> The network MAC in the Armada 370/XP (supported by the mvneta driver
> in Linux) has a functionality that allows it to continuously poll the
> PHY and directly update the MAC configuration accordingly (speed,
> duplex, etc.). The very first versions of the driver submitted for
> review were using this hardware mechanism, but due to this, the driver
> was not integrated with the kernel phylib. Following reviews, the
> driver was changed to use the phylib, and therefore a software based
> polling. In software based polling, Linux regularly talks to the PHY
> over the MDIO bus, and sees if the link status has changed. If it's
> the case then the adjust_link() callback of the driver is called to
> update the MAC configuration accordingly.
> 
> However, it turns out that the adjust_link() callback was not
> configuring the hardware in a completely correct way: while it was
> setting the speed and duplex bits correctly, it wasn't telling the
> hardware to actually take into account those bits rather than what the
> hardware-based PHY polling mechanism has concluded. So, in fact the
> adjust_link() callback was basically a no-op.
> 
> However, the network happened to be working because on the network
> interfaces used by U-Boot for tftp on Armada 370 platforms because the
> hardware PHY polling was enabled by the bootloader, and left enabled
> by Linux. However, the second network interface not used for tftp (or
> both network interfaces if the kernel is loaded from USB, NAND or SD
> card) didn't had the hardware PHY polling enabled.
> 
> This patch fixes this situation by:
> 
>  (1) Making sure that the hardware PHY polling is disabled by clearing
>      the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
>      register in the driver ->probe() function.
> 
>  (2) Making sure that the duplex and speed selections made by the
>      adjust_link() callback are taken into account by clearing the
>      MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
>      MVNETA_GMAC_AUTONEG_CONFIG register.
> 
> This patch has been tested on Armada 370 Mirabox, and now both network
> interfaces are usable after boot.
> 

Well done Thomas!

I have successfully tested it on Armada 370 Mirabox:

Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Willy Tarreau <w@1wt.eu>
> Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
> Cc: Peter Sanford <psanford@nearbuy.io>
> Cc: Ethan Tuttle <ethan@ethantuttle.com>
> Cc: Chény Yves-Gael <yves@cheny.fr>
> Cc: Ryan Press <ryan@presslab.us>
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> Cc: vdonnefort@lacie.com
> Cc: stable@vger.kernel.org
> ---
> David, this patch is a fix for a problem that has been here since 3.8
> (when the mvneta driver was introduced), so I've Cc'ed stable@ and if
> possible I'd like to patch to be included for 3.12.
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index b017818..90ab292 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -138,7 +138,9 @@
>  #define      MVNETA_GMAC_FORCE_LINK_PASS         BIT(1)
>  #define      MVNETA_GMAC_CONFIG_MII_SPEED        BIT(5)
>  #define      MVNETA_GMAC_CONFIG_GMII_SPEED       BIT(6)
> +#define      MVNETA_GMAC_AN_SPEED_EN             BIT(7)
>  #define      MVNETA_GMAC_CONFIG_FULL_DUPLEX      BIT(12)
> +#define      MVNETA_GMAC_AN_DUPLEX_EN            BIT(13)
>  #define MVNETA_MIB_COUNTERS_BASE                 0x3080
>  #define      MVNETA_MIB_LATE_COLLISION           0x7c
>  #define MVNETA_DA_FILT_SPEC_MCAST                0x3400
> @@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
>  	/* Assign port SDMA configuration */
>  	mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
>  
> +	/* Disable PHY polling in hardware, since we're using the
> +	 * kernel phylib to do this.
> +	 */
> +	val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
> +	val &= ~MVNETA_PHY_POLLING_ENABLE;
> +	mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
> +
>  	mvneta_set_ucast_table(pp, -1);
>  	mvneta_set_special_mcast_table(pp, -1);
>  	mvneta_set_other_mcast_table(pp, -1);
> @@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
>  			val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
>  			val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
>  				 MVNETA_GMAC_CONFIG_GMII_SPEED |
> -				 MVNETA_GMAC_CONFIG_FULL_DUPLEX);
> +				 MVNETA_GMAC_CONFIG_FULL_DUPLEX |
> +				 MVNETA_GMAC_AN_SPEED_EN |
> +				 MVNETA_GMAC_AN_DUPLEX_EN);
>  
>  			if (phydev->duplex)
>  				val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
>
Willy Tarreau Sept. 4, 2013, 4:32 p.m. UTC | #5
Hi Thomas!

On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
> This commit fixes a long-standing bug that has been reported by many
> users: on some Armada 370 platforms, only the network interface that
> has been used in U-Boot to tftp the kernel works properly in
> Linux. The other network interfaces can see a 'link up', but are
> unable to transmit data. The reports were generally made on the Armada
> 370-based Mirabox, but have also been given on the Armada 370-RD
> board.
(...)
> This patch has been tested on Armada 370 Mirabox, and now both network
> interfaces are usable after boot.

Just as a complementary check, I can also confirm that the OpenBlocks
AX3 continues to work fine after this change.

Best regards!
Willy
Ethan Tuttle Sept. 5, 2013, 4:14 a.m. UTC | #6
Just booted with the patch on my Mirabox.  Both interfaces work!
Thank you Thomas.

One remaining issue is that the interface which uBoot didn't configure
is still getting a random mac address:

mvneta d0070000.ethernet eth0: Using hardware mac address f0:ad:4e:01:dc:97
mvneta d0074000.ethernet eth1: Using random mac address d2:35:dd:c8:99:48

This is on 3.11 plus Thomas' patch, which includes the previous fix to
"read MAC address from hardware when available".  Perhaps that fix
isn't working with the phylib?

Thanks again,

Ethan

On Wed, Sep 4, 2013 at 9:32 AM, Willy Tarreau <w@1wt.eu> wrote:
>
> Hi Thomas!
>
> On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
> > This commit fixes a long-standing bug that has been reported by many
> > users: on some Armada 370 platforms, only the network interface that
> > has been used in U-Boot to tftp the kernel works properly in
> > Linux. The other network interfaces can see a 'link up', but are
> > unable to transmit data. The reports were generally made on the Armada
> > 370-based Mirabox, but have also been given on the Armada 370-RD
> > board.
> (...)
> > This patch has been tested on Armada 370 Mirabox, and now both network
> > interfaces are usable after boot.
>
> Just as a complementary check, I can also confirm that the OpenBlocks
> AX3 continues to work fine after this change.
>
> Best regards!
> Willy
>
Willy Tarreau Sept. 5, 2013, 5:12 a.m. UTC | #7
Hi,

On Wed, Sep 04, 2013 at 09:14:23PM -0700, Ethan Tuttle wrote:
> Just booted with the patch on my Mirabox.  Both interfaces work!
> Thank you Thomas.
> 
> One remaining issue is that the interface which uBoot didn't configure
> is still getting a random mac address:
> 
> mvneta d0070000.ethernet eth0: Using hardware mac address f0:ad:4e:01:dc:97
> mvneta d0074000.ethernet eth1: Using random mac address d2:35:dd:c8:99:48
> 
> This is on 3.11 plus Thomas' patch, which includes the previous fix to
> "read MAC address from hardware when available".  Perhaps that fix
> isn't working with the phylib?

No this is unrelated. The MAC isn't configured by the boot loader when
the NIC is not used. U-boot only passes it as a non-standard ATAG. I
have some patches to reinject the non-standard atags into the device
tree if you absolutely need this, but they're not suited for mainline
inclusion from what I understood last time I proposed them (they use
a marvell-specific atag header).

Regards,
Willy
Ethan Tuttle Sept. 5, 2013, 5:22 a.m. UTC | #8
Understood.  Ultimately, I'll use this board as a router, and stable
mac addresses would be better than random.  So I would be interested
to try your atag -> device tree patches.  Have they been posted
somewhere I can find them?

Thanks Willy.

Ethan

On Wed, Sep 4, 2013 at 10:12 PM, Willy Tarreau <w@1wt.eu> wrote:
> Hi,
>
> On Wed, Sep 04, 2013 at 09:14:23PM -0700, Ethan Tuttle wrote:
>> Just booted with the patch on my Mirabox.  Both interfaces work!
>> Thank you Thomas.
>>
>> One remaining issue is that the interface which uBoot didn't configure
>> is still getting a random mac address:
>>
>> mvneta d0070000.ethernet eth0: Using hardware mac address f0:ad:4e:01:dc:97
>> mvneta d0074000.ethernet eth1: Using random mac address d2:35:dd:c8:99:48
>>
>> This is on 3.11 plus Thomas' patch, which includes the previous fix to
>> "read MAC address from hardware when available".  Perhaps that fix
>> isn't working with the phylib?
>
> No this is unrelated. The MAC isn't configured by the boot loader when
> the NIC is not used. U-boot only passes it as a non-standard ATAG. I
> have some patches to reinject the non-standard atags into the device
> tree if you absolutely need this, but they're not suited for mainline
> inclusion from what I understood last time I proposed them (they use
> a marvell-specific atag header).
>
> Regards,
> Willy
>
yves@cheny.fr Sept. 5, 2013, 6:23 a.m. UTC | #9
Hi Willy,
i would be interested too !

thx
Yves

Le 2013-09-05 07:22, Ethan Tuttle a écrit :
> Understood.  Ultimately, I'll use this board as a router, and stable
> mac addresses would be better than random.  So I would be interested
> to try your atag -> device tree patches.  Have they been posted
> somewhere I can find them?
> 
> Thanks Willy.
> 
> Ethan
> 
> On Wed, Sep 4, 2013 at 10:12 PM, Willy Tarreau <w@1wt.eu> wrote:
>> Hi,
>> 
>> On Wed, Sep 04, 2013 at 09:14:23PM -0700, Ethan Tuttle wrote:
>>> Just booted with the patch on my Mirabox.  Both interfaces work!
>>> Thank you Thomas.
>>> 
>>> One remaining issue is that the interface which uBoot didn't 
>>> configure
>>> is still getting a random mac address:
>>> 
>>> mvneta d0070000.ethernet eth0: Using hardware mac address 
>>> f0:ad:4e:01:dc:97
>>> mvneta d0074000.ethernet eth1: Using random mac address 
>>> d2:35:dd:c8:99:48
>>> 
>>> This is on 3.11 plus Thomas' patch, which includes the previous fix 
>>> to
>>> "read MAC address from hardware when available".  Perhaps that fix
>>> isn't working with the phylib?
>> 
>> No this is unrelated. The MAC isn't configured by the boot loader when
>> the NIC is not used. U-boot only passes it as a non-standard ATAG. I
>> have some patches to reinject the non-standard atags into the device
>> tree if you absolutely need this, but they're not suited for mainline
>> inclusion from what I understood last time I proposed them (they use
>> a marvell-specific atag header).
>> 
>> Regards,
>> Willy
>>
Ethan Tuttle Sept. 5, 2013, 6:52 a.m. UTC | #10
Works like a charm!  Thanks for sharing these.

It may be a while until an alternative to Marvell's uboot is available
for the Mirabox, so it's a shame these patches won't be making it into
the mainline kernel.

Regards,

Ethan

On Wed, Sep 4, 2013 at 11:40 PM, Willy Tarreau <w@1wt.eu> wrote:
> On Thu, Sep 05, 2013 at 08:23:12AM +0200, yves@cheny.fr wrote:
>> Hi Willy,
>> i would be interested too !
>>
>> thx
>> Yves
>>
>> Le 2013-09-05 07:22, Ethan Tuttle a écrit :
>> >Understood.  Ultimately, I'll use this board as a router, and stable
>> >mac addresses would be better than random.  So I would be interested
>> >to try your atag -> device tree patches.  Have they been posted
>> >somewhere I can find them?
>
> OK guys, here they come. Note that they're now simplified since the
> eth* aliases have been added to the dts.
>
> Willy
>
Thomas Petazzoni Sept. 5, 2013, 7:28 a.m. UTC | #11
Hello,

On Thu, 5 Sep 2013 07:12:23 +0200, Willy Tarreau wrote:

> > This is on 3.11 plus Thomas' patch, which includes the previous fix to
> > "read MAC address from hardware when available".  Perhaps that fix
> > isn't working with the phylib?
> 
> No this is unrelated. The MAC isn't configured by the boot loader when
> the NIC is not used. U-boot only passes it as a non-standard ATAG. I
> have some patches to reinject the non-standard atags into the device
> tree if you absolutely need this, but they're not suited for mainline
> inclusion from what I understood last time I proposed them (they use
> a marvell-specific atag header).

I indeed submitted a revised/improved version of your patches some time
ago, but they were rejected. See
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173201.html.
Since this has been rejected, the available options are:

 (1) Use a DT-capable bootloader that will properly set the MAC
     addresses in the DT. Such DT capable bootloaders will soon be made
     available by Marvell, but I am really unsure Globalscale will
     provide an update for the Mirabox bootloader, rebased on the new
     Marvell bootloader that will be DT capable.

     As an alternative, we (mainly Sebastian Hesselbarth and myself)
     have started adding Armada 370/XP support in the Barebox
     bootloader. We can already start Barebox on the Mirabox, but for
     now it's quite useless since only the serial port is supported,
     there is still no support for the network, SD card, USB or NAND.
     This will probably come over time, but it's not going to happen
     overnight.

 (2) Use the "impedance matcher" code written by Daniel Mack and
     extended by Jason Cooper, available at
     https://github.com/zonque/pxa-impedance-matcher. Essentially, it
     inserts a small binary between the installed bootloader and the
     kernel, that for example allows to choose a particular DTB amongst
     several, depending on the board that is detected. I believe it
     could probably be extended to cover other use cases such as
     modifying the DTB to add the MAC addresses where appropriate. I've
     added Jason Cooper in the Cc list if he wants to comment on that.

 (3) Continue to manually apply the patches from Willy that add support
     for the Marvell-specific ATAGs.

Best regards,

Thomas
Willy Tarreau Sept. 5, 2013, 7:44 a.m. UTC | #12
Hi Thomas,

On Thu, Sep 05, 2013 at 09:28:08AM +0200, Thomas Petazzoni wrote:
> I indeed submitted a revised/improved version of your patches some time
> ago, but they were rejected. See
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173201.html.

Thanks for the link, that's indeed what I was referring to. Well, at least
the most boring part that constantly needed to be rebased was the DT patch.
Now it's much easier to keep the small remaining patches in one's local tree.

> Since this has been rejected, the available options are:
> 
>  (1) Use a DT-capable bootloader that will properly set the MAC
>      addresses in the DT. Such DT capable bootloaders will soon be made
>      available by Marvell, but I am really unsure Globalscale will
>      provide an update for the Mirabox bootloader, rebased on the new
>      Marvell bootloader that will be DT capable.

One simpler solution for them could be to slightly modify the boot loader
so that it sets the MAC address on the two ethernet controllers prior to
boot. Then your code which checks if a MAC is already set will simply
work.

>      As an alternative, we (mainly Sebastian Hesselbarth and myself)
>      have started adding Armada 370/XP support in the Barebox
>      bootloader. We can already start Barebox on the Mirabox, but for
>      now it's quite useless since only the serial port is supported,
>      there is still no support for the network, SD card, USB or NAND.
>      This will probably come over time, but it's not going to happen
>      overnight.

And we must keep in mind that people are generally scared by boot loader
upgrades, especially when it's for a different one. At least on this
platform now we have a solution to reflash even after complete failures
so this is less of a problem.

>  (2) Use the "impedance matcher" code written by Daniel Mack and
>      extended by Jason Cooper, available at
>      https://github.com/zonque/pxa-impedance-matcher. Essentially, it
>      inserts a small binary between the installed bootloader and the
>      kernel, that for example allows to choose a particular DTB amongst
>      several, depending on the board that is detected. I believe it
>      could probably be extended to cover other use cases such as
>      modifying the DTB to add the MAC addresses where appropriate. I've
>      added Jason Cooper in the Cc list if he wants to comment on that.

Could be a nice solution as well, indeed.

A last one would be to have the mvneta module accept an array of addresses
as a module parameter. This way it would just require a minor change in the
kernel's cmdline to pass the MAC addresses. I remember seeing this in the
past, I don't remember the platform (maybe the NSLU2 but I could be wrong).

Cheers,
Willy
Thomas Petazzoni Sept. 5, 2013, 8:26 a.m. UTC | #13
Dear Willy Tarreau,

On Thu, 5 Sep 2013 09:44:26 +0200, Willy Tarreau wrote:

> On Thu, Sep 05, 2013 at 09:28:08AM +0200, Thomas Petazzoni wrote:
> > I indeed submitted a revised/improved version of your patches some time
> > ago, but they were rejected. See
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173201.html.
> 
> Thanks for the link, that's indeed what I was referring to. Well, at least
> the most boring part that constantly needed to be rebased was the DT patch.
> Now it's much easier to keep the small remaining patches in one's local tree.

Right.

> > Since this has been rejected, the available options are:
> > 
> >  (1) Use a DT-capable bootloader that will properly set the MAC
> >      addresses in the DT. Such DT capable bootloaders will soon be made
> >      available by Marvell, but I am really unsure Globalscale will
> >      provide an update for the Mirabox bootloader, rebased on the new
> >      Marvell bootloader that will be DT capable.
> 
> One simpler solution for them could be to slightly modify the boot loader
> so that it sets the MAC address on the two ethernet controllers prior to
> boot. Then your code which checks if a MAC is already set will simply
> work.

This works when the network driver is compiled 'statically' inside the
kernel. When compiled as a module, then the gatable clock of the
network interface will be gated at the end of the kernel boot, before
the mvneta module is probe. And gating the network interface clocks
means that it will loose its state, including its MAC address. So it's
not an entirely perfect solution either, but I admit that on such
platforms, the network driver is most likely compiled statically, so it
would probably suit the needs of most people.

Note that this can be done without doing any change in the bootloader.
For example, on a Mirabox, you can do:

   mw.l 0xD0072414 0x5C93; mw.l 0xD0072418 0xF0AD4E01; mw.l 0xD0076414 0x5C94; mw.l 0xD0076418 0xF0AD4E01; bootm

to boot your kernel. This will program the MAC addresses for both
network interfaces in the network controllers, so that when booting
Linux, you get:

[   42.122881] mvneta d0070000.ethernet eth0: Using hardware mac address f0:ad:4e:01:5c:93
[   42.385398] mvneta d0074000.ethernet eth1: Using hardware mac address f0:ad:4e:01:5c:94

You add that to your default U-Boot boot script, and that's it, you
have stable MAC addresses. Of course, as explained above, that doesn't
work if you build mvneta as a module.

> >      As an alternative, we (mainly Sebastian Hesselbarth and myself)
> >      have started adding Armada 370/XP support in the Barebox
> >      bootloader. We can already start Barebox on the Mirabox, but for
> >      now it's quite useless since only the serial port is supported,
> >      there is still no support for the network, SD card, USB or NAND.
> >      This will probably come over time, but it's not going to happen
> >      overnight.
> 
> And we must keep in mind that people are generally scared by boot loader
> upgrades, especially when it's for a different one. At least on this
> platform now we have a solution to reflash even after complete failures
> so this is less of a problem.

Right, with kwboot working very reliably on Mirabox, I believe this is
not really an issue.

> >  (2) Use the "impedance matcher" code written by Daniel Mack and
> >      extended by Jason Cooper, available at
> >      https://github.com/zonque/pxa-impedance-matcher. Essentially, it
> >      inserts a small binary between the installed bootloader and the
> >      kernel, that for example allows to choose a particular DTB amongst
> >      several, depending on the board that is detected. I believe it
> >      could probably be extended to cover other use cases such as
> >      modifying the DTB to add the MAC addresses where appropriate. I've
> >      added Jason Cooper in the Cc list if he wants to comment on that.
> 
> Could be a nice solution as well, indeed.
> 
> A last one would be to have the mvneta module accept an array of addresses
> as a module parameter. This way it would just require a minor change in the
> kernel's cmdline to pass the MAC addresses. I remember seeing this in the
> past, I don't remember the platform (maybe the NSLU2 but I could be wrong).

The situation of module parameters to pass MAC addresses was a bit
fuzzy. There was once a proposal to add a generic kernel parameter to
do this, but it was rejected by David Miller (I believe not on specific
implementation details, but on the general idea). However, there are
numerous drivers in the tree that do provide a custom module parameter
to set MAC addresses.

However, with the above suggestion of U-Boot scripting, I believe we
have a relatively easy solution for people to use.

Best regards,

Thomas
Willy Tarreau Sept. 5, 2013, 9:11 a.m. UTC | #14
On Thu, Sep 05, 2013 at 10:26:59AM +0200, Thomas Petazzoni wrote:
> > One simpler solution for them could be to slightly modify the boot loader
> > so that it sets the MAC address on the two ethernet controllers prior to
> > boot. Then your code which checks if a MAC is already set will simply
> > work.
> 
> This works when the network driver is compiled 'statically' inside the
> kernel. When compiled as a module, then the gatable clock of the
> network interface will be gated at the end of the kernel boot, before
> the mvneta module is probe. And gating the network interface clocks
> means that it will loose its state, including its MAC address. So it's
> not an entirely perfect solution either, but I admit that on such
> platforms, the network driver is most likely compiled statically, so it
> would probably suit the needs of most people.

Agreed.

> Note that this can be done without doing any change in the bootloader.
> For example, on a Mirabox, you can do:
> 
>    mw.l 0xD0072414 0x5C93; mw.l 0xD0072418 0xF0AD4E01; mw.l 0xD0076414 0x5C94; mw.l 0xD0076418 0xF0AD4E01; bootm
> 
> to boot your kernel. This will program the MAC addresses for both
> network interfaces in the network controllers, so that when booting
> Linux, you get:
> 
> [   42.122881] mvneta d0070000.ethernet eth0: Using hardware mac address f0:ad:4e:01:5c:93
> [   42.385398] mvneta d0074000.ethernet eth1: Using hardware mac address f0:ad:4e:01:5c:94
> 
> You add that to your default U-Boot boot script, and that's it, you
> have stable MAC addresses.

Hmmm that's quite interesting. Unfortunately I don't see an easy way to
make this directly rely on the ethaddr/eth1addr so that end users can
simply cut-n-paste a few lines into the u-boot config. But anyway that
can be useful.

> > A last one would be to have the mvneta module accept an array of addresses
> > as a module parameter. This way it would just require a minor change in the
> > kernel's cmdline to pass the MAC addresses. I remember seeing this in the
> > past, I don't remember the platform (maybe the NSLU2 but I could be wrong).
> 
> The situation of module parameters to pass MAC addresses was a bit
> fuzzy. There was once a proposal to add a generic kernel parameter to
> do this, but it was rejected by David Miller (I believe not on specific
> implementation details, but on the general idea). However, there are
> numerous drivers in the tree that do provide a custom module parameter
> to set MAC addresses.

Yes, I remember using this with the sunhme driver many years ago when
we did not have access to the onboard rom to retrieve the MAC address.

> However, with the above suggestion of U-Boot scripting, I believe we
> have a relatively easy solution for people to use.

We could provide a script to do it more conveniently for the user :-)

Best regards,
Willy
Thomas Petazzoni Sept. 5, 2013, 9:24 a.m. UTC | #15
Dear Willy Tarreau,

On Thu, 5 Sep 2013 11:11:47 +0200, Willy Tarreau wrote:

> 
> > Note that this can be done without doing any change in the bootloader.
> > For example, on a Mirabox, you can do:
> > 
> >    mw.l 0xD0072414 0x5C93; mw.l 0xD0072418 0xF0AD4E01; mw.l 0xD0076414 0x5C94; mw.l 0xD0076418 0xF0AD4E01; bootm
> > 
> > to boot your kernel. This will program the MAC addresses for both
> > network interfaces in the network controllers, so that when booting
> > Linux, you get:
> > 
> > [   42.122881] mvneta d0070000.ethernet eth0: Using hardware mac address f0:ad:4e:01:5c:93
> > [   42.385398] mvneta d0074000.ethernet eth1: Using hardware mac address f0:ad:4e:01:5c:94
> > 
> > You add that to your default U-Boot boot script, and that's it, you
> > have stable MAC addresses.
> 
> Hmmm that's quite interesting. Unfortunately I don't see an easy way to
> make this directly rely on the ethaddr/eth1addr so that end users can
> simply cut-n-paste a few lines into the u-boot config. But anyway that
> can be useful.

I thought about this as well, but I don't think that's possible, the
U-Boot scripting/parsing capabilities seems to be too limited to
achieve that, unfortunately.

Best regards,

Thomas
Jason Cooper Sept. 5, 2013, 11:36 a.m. UTC | #16
On Thu, Sep 05, 2013 at 09:28:08AM +0200, Thomas Petazzoni wrote:
>  (2) Use the "impedance matcher" code written by Daniel Mack and
>      extended by Jason Cooper, available at
>      https://github.com/zonque/pxa-impedance-matcher. Essentially, it
>      inserts a small binary between the installed bootloader and the
>      kernel, that for example allows to choose a particular DTB amongst
>      several, depending on the board that is detected. I believe it
>      could probably be extended to cover other use cases such as
>      modifying the DTB to add the MAC addresses where appropriate. I've
>      added Jason Cooper in the Cc list if he wants to comment on that.

Yes, I'm hoping to add the dtb editing support sometime over the next
few weeks.  That way, we can use Willy's atags patch in there, and the
kernel never knows the difference.

It isn't high on my priority list atm, so if someone else is motivated,
I've been borrowing code from barebox to flesh out impedance-matcher.
Patches are welcomed ;-)

thx,

Jason.
Jason Cooper Sept. 5, 2013, 11:38 a.m. UTC | #17
On Thu, Sep 05, 2013 at 10:26:59AM +0200, Thomas Petazzoni wrote:
> On Thu, 5 Sep 2013 09:44:26 +0200, Willy Tarreau wrote:
> > One simpler solution for them could be to slightly modify the boot loader
> > so that it sets the MAC address on the two ethernet controllers prior to
> > boot. Then your code which checks if a MAC is already set will simply
> > work.
> 
> This works when the network driver is compiled 'statically' inside the
> kernel. When compiled as a module, then the gatable clock of the
> network interface will be gated at the end of the kernel boot, before
> the mvneta module is probe. And gating the network interface clocks
> means that it will loose its state, including its MAC address. So it's
> not an entirely perfect solution either, but I admit that on such
> platforms, the network driver is most likely compiled statically, so it
> would probably suit the needs of most people.

Up until sleep and standby modes are supported.  Proper power savings
would include gating the clock...

thx,

Jason.
David Miller Sept. 5, 2013, 6:51 p.m. UTC | #18
From: Jason Cooper <jason@lakedaemon.net>
Date: Wed, 4 Sep 2013 10:50:51 -0400

> On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
>> This commit fixes a long-standing bug that has been reported by many
>> users: on some Armada 370 platforms, only the network interface that
>> has been used in U-Boot to tftp the kernel works properly in
>> Linux. The other network interfaces can see a 'link up', but are
>> unable to transmit data. The reports were generally made on the Armada
>> 370-based Mirabox, but have also been given on the Armada 370-RD
>> board.
 ...
> 
> David,
> 
> Offending patch is:
> 
>   c5aff18 net: mvneta: driver for Marvell Armada 370/XP network unit
> 
> Applies and builds cleanly against v3.8.13, v3.9.11, v3.10.10, and v3.11
> 
> Acked-by: Jason Cooper <jason@lakedaemon.net>

Applied.
diff mbox

Patch

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b017818..90ab292 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -138,7 +138,9 @@ 
 #define      MVNETA_GMAC_FORCE_LINK_PASS         BIT(1)
 #define      MVNETA_GMAC_CONFIG_MII_SPEED        BIT(5)
 #define      MVNETA_GMAC_CONFIG_GMII_SPEED       BIT(6)
+#define      MVNETA_GMAC_AN_SPEED_EN             BIT(7)
 #define      MVNETA_GMAC_CONFIG_FULL_DUPLEX      BIT(12)
+#define      MVNETA_GMAC_AN_DUPLEX_EN            BIT(13)
 #define MVNETA_MIB_COUNTERS_BASE                 0x3080
 #define      MVNETA_MIB_LATE_COLLISION           0x7c
 #define MVNETA_DA_FILT_SPEC_MCAST                0x3400
@@ -915,6 +917,13 @@  static void mvneta_defaults_set(struct mvneta_port *pp)
 	/* Assign port SDMA configuration */
 	mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
 
+	/* Disable PHY polling in hardware, since we're using the
+	 * kernel phylib to do this.
+	 */
+	val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
+	val &= ~MVNETA_PHY_POLLING_ENABLE;
+	mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
+
 	mvneta_set_ucast_table(pp, -1);
 	mvneta_set_special_mcast_table(pp, -1);
 	mvneta_set_other_mcast_table(pp, -1);
@@ -2307,7 +2316,9 @@  static void mvneta_adjust_link(struct net_device *ndev)
 			val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
 			val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
 				 MVNETA_GMAC_CONFIG_GMII_SPEED |
-				 MVNETA_GMAC_CONFIG_FULL_DUPLEX);
+				 MVNETA_GMAC_CONFIG_FULL_DUPLEX |
+				 MVNETA_GMAC_AN_SPEED_EN |
+				 MVNETA_GMAC_AN_DUPLEX_EN);
 
 			if (phydev->duplex)
 				val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;