| Message ID | 1383611239-14556-1-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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); > >
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+
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>
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+
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+
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);
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(+)