[PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode

[PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode

[Jason Gunthorpe]

Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
present' made the call to phy_scan optional, if the DT has a link to
the phy node.

However phy_scan has the side effect of calling phy_addr_set, which
writes the phy MDIO address to the ethernet controller. If phy_addr_set
is not called, and the bootloader has not set the correct address then
the driver will fail to function.

Tested on Kirkwood.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Lennert Buytenhek <buytenh@wantstofly.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 2c210ec..00e43b5 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2890,6 +2890,7 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 					 PHY_INTERFACE_MODE_GMII);
 		if (!mp->phy)
 			err = -ENODEV;
+		phy_addr_set(mp, mp->phy->addr);
 	} else if (pd->phy_addr != MV643XX_ETH_PHY_NONE) {
 		mp->phy = phy_scan(mp, pd->phy_addr);
 
-- 
1.8.1.2

Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode

[Jason Cooper]

Jason,

On Mon, Nov 04, 2013 at 05:27:19PM -0700, Jason Gunthorpe wrote:
> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node

fyi: set core.abbrev = 12 in your git config, according to Linus, 7/8
was a bad decision...

> present' made the call to phy_scan optional, if the DT has a link to
> the phy node.
> 
> However phy_scan has the side effect of calling phy_addr_set, which
> writes the phy MDIO address to the ethernet controller. If phy_addr_set
> is not called, and the bootloader has not set the correct address then
> the driver will fail to function.
> 
> Tested on Kirkwood.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Fixes: cc9d459894b0 "net: mv643xx_eth: use of_phy_connect if phy_node present"
Acked-by: Jason Cooper <jason@lakedaemon.net>

And it should be suitable for v3.11+

thx,

Jason.

Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode

[Sebastian Hesselbarth]

On 11/05/2013 01:27 AM, Jason Gunthorpe wrote:
> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
> present' made the call to phy_scan optional, if the DT has a link to
> the phy node.
>
> However phy_scan has the side effect of calling phy_addr_set, which
> writes the phy MDIO address to the ethernet controller. If phy_addr_set
> is not called, and the bootloader has not set the correct address then
> the driver will fail to function.
>
> Tested on Kirkwood.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---

Jason,

thanks for catching this! I do my kirkwood testing on Dockstar,
which has PHY addr 0x0 - also the reset default, which may be
why it slipped through.

Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

> ---
>   drivers/net/ethernet/marvell/mv643xx_eth.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index 2c210ec..00e43b5 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
> @@ -2890,6 +2890,7 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
>   					 PHY_INTERFACE_MODE_GMII);
>   		if (!mp->phy)
>   			err = -ENODEV;
> +		phy_addr_set(mp, mp->phy->addr);
>   	} else if (pd->phy_addr != MV643XX_ETH_PHY_NONE) {
>   		mp->phy = phy_scan(mp, pd->phy_addr);
>
>

Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode

[Arnaud Ebalard]

Hi Jason,

Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:

> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
> present' made the call to phy_scan optional, if the DT has a link to
> the phy node.
>
> However phy_scan has the side effect of calling phy_addr_set, which
> writes the phy MDIO address to the ethernet controller. If phy_addr_set
> is not called, and the bootloader has not set the correct address then
> the driver will fail to function.

Thanks *a lot* for fixing this one! I had the issue on my ReadyNAS 102
(Armada 370 based) which I had put on a todo list and temporarily
workarounded by including a 'ping whatever' call in my u-boot env in
order to force it to do the init. Without it, I was unable to properly
use the interface. With your fix, after multiple reboots to test it,
everything works as expected. So, FWIW: 

Tested-by: Arnaud Ebalard <arno@natisbad.org>

Cheers,

a+

Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode

[Sebastian Hesselbarth]

On 11/05/2013 11:12 PM, Arnaud Ebalard wrote:
> Hi Jason,
>
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:
>
>> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
>> present' made the call to phy_scan optional, if the DT has a link to
>> the phy node.
>>
>> However phy_scan has the side effect of calling phy_addr_set, which
>> writes the phy MDIO address to the ethernet controller. If phy_addr_set
>> is not called, and the bootloader has not set the correct address then
>> the driver will fail to function.
>
> Thanks *a lot* for fixing this one! I had the issue on my ReadyNAS 102
> (Armada 370 based) which I had put on a todo list and temporarily

Erm, just to make sure: Armada 370 isn't using mv643xx_eth but mvneta,
are you sure it is (was) related to Jason's fix?

Sebastian

> workarounded by including a 'ping whatever' call in my u-boot env in
> order to force it to do the init. Without it, I was unable to properly
> use the interface. With your fix, after multiple reboots to test it,
> everything works as expected. So, FWIW:
>
> Tested-by: Arnaud Ebalard <arno@natisbad.org>

Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode

[Jason Cooper]

On Tue, Nov 05, 2013 at 11:12:00PM +0100, Arnaud Ebalard wrote:
> Hi Jason,
> 
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:
> 
> > Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
> > present' made the call to phy_scan optional, if the DT has a link to
> > the phy node.
> >
> > However phy_scan has the side effect of calling phy_addr_set, which
> > writes the phy MDIO address to the ethernet controller. If phy_addr_set
> > is not called, and the bootloader has not set the correct address then
> > the driver will fail to function.
> 
> Thanks *a lot* for fixing this one! I had the issue on my ReadyNAS 102
> (Armada 370 based) which I had put on a todo list and temporarily

readynas duo v2?

thx,

Jason.

> workarounded by including a 'ping whatever' call in my u-boot env in
> order to force it to do the init. Without it, I was unable to properly
> use the interface. With your fix, after multiple reboots to test it,
> everything works as expected. So, FWIW: 
> 
> Tested-by: Arnaud Ebalard <arno@natisbad.org>
> 
> Cheers,
> 
> a+

Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode

[Arnaud Ebalard]

Hi,

Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> writes:

> On 11/05/2013 11:12 PM, Arnaud Ebalard wrote:
>> Hi Jason,
>>
>> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:
>>
>>> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
>>> present' made the call to phy_scan optional, if the DT has a link to
>>> the phy node.
>>>
>>> However phy_scan has the side effect of calling phy_addr_set, which
>>> writes the phy MDIO address to the ethernet controller. If phy_addr_set
>>> is not called, and the bootloader has not set the correct address then
>>> the driver will fail to function.
>>
>> Thanks *a lot* for fixing this one! I had the issue on my ReadyNAS 102
>> (Armada 370 based) which I had put on a todo list and temporarily
>
> Erm, just to make sure: Armada 370 isn't using mv643xx_eth but mvneta,
> are you sure it is (was) related to Jason's fix?

Thanks for pointing this, Sebastian and my apologies for the noise.
Jason's fix is indeed for a file which is not compiled for my RN102.

As the problem perfectly matched the issue I had and current kernel w/
the patch applied does indeed fix it, I did not try and do the test w/o
the patch applied. It would have showed the problem was fixed by
something else in 3.12. Well, I spent some time digging the changes on
mvneta.c and: 

commit 714086029116b6b0a34e67ba1dd2f0d1cf26770c
Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Date:   Wed Sep 4 16:21:18 2013 +0200

    net: mvneta: properly disable HW PHY polling and ensure adjust_link() works
    
    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.

    [SNIP]

$ git tag --contains 714086029116
v3.12
v3.12-rc1
v3.12-rc2
v3.12-rc3
v3.12-rc4
v3.12-rc5
v3.12-rc6
v3.12-rc7

So the problem was indeed fixed at the beginning of 3.12 series by Thomas.

Anyway, my bad and thanks again for pointing it out.

Cheers,

a+