Ethernet support broken for Wandboard Quad on master
Subsystems (2)
Hello,
ethernet support for Wandboard Quad seems to be broken on current master (576aacdb):
U-Boot 2013.07-rc3-00021-g576aacd (Jul 18 2013 - 08:34:32)
CPU: Freescale i.MX6Q rev1.2 at 792 MHz Reset cause: POR Board: Wandboard DRAM: 2 GiB MMC: FSL_SDHC: 0, FSL_SDHC: 1
*** Warning - bad CRC, using default environment
In: serial Out: serial Err: serial Net: FEC [PRIME] Warning: FEC using MAC address from net device
Hit any key to stop autoboot: 0
=> setenv autoload no && dhcp && tftp ${fdt_addr} /wandboard/imx6q-wandboard.dtb && fdt addr ${fdt_addr} && tftpboot ${loadaddr} /wandboard/zImage && setenv bootargs console=${console}, ${baudrate} rootwait && bootz ${loadaddr} - ${fdt_addr}
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
BOOTP broadcast 5
Retry count exceeded; starting again
[...]
It used to work as of e6c7f86f:
U-Boot 2013.04-00603-ge6c7f86 (Jul 18 2013 - 08:42:04)
CPU: Freescale i.MX6Q rev1.2 at 792 MHz Reset cause: POR Board: Wandboard DRAM: 2 GiB MMC: FSL_SDHC: 0, FSL_SDHC: 1
*** Warning - bad CRC, using default environment
In: serial Out: serial Err: serial Net: FEC [PRIME] Warning: FEC using MAC address from net device
Hit any key to stop autoboot: 0
=> dhcp
FEC Waiting for PHY auto negotiation to complete. done BOOTP broadcast 1
DHCP client bound to address 192.168.2.154
*** Warning: no boot file name; using 'C0A8029A.img' Using FEC device
TFTP from server 192.168.2.1; our IP address is 192.168.2.154 Filename 'C0A8029A.img'.
Load address: 0x12000000
Loading: *
TFTP error: 'File not found' (1)
Not retrying...
=>
I've started a git bisect run (booting via USB OTG), but it's going to take a while as git bisect is throwing lots of commits without Wandboard Quad support at me and seems to be only narrowing down the commits one by one rather than on a log-2 basis.
Sascha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130718/724dd5ee/attachment.pgp>
Quoted 4 lines · Sascha Silbe
I've started a git bisect run (booting via USB OTG), but it's going to take a while as git bisect is throwing lots of commits without Wandboard Quad support at me and seems to be only narrowing down the commits one by one rather than on a log-2 basis.
A slightly more refined approach (cherry-picking the Wandboard Quad support patch each time the build fails) enabled git bisect to track the breakage down to:
commit de1d786edf01035f60a87e1e0f917a4169dc6964
Author: Charles Coldwell <[email protected]> Date: Thu Feb 21 08:25:52 2013 -0500
add support for Xilinx 1000BASE-X phy (GTX)
commit 39695029bc15041c809df3db4ba19bd729c447fa
Author: Charles Coldwell <[email protected]>
Date: Tue Feb 19 08:27:33 2013 -0500
Changes to support the Xilinx 1000BASE-X phy (GTX/MGT)
:040000 040000 459dbd37eb25461cc71d6e59b91a4b4994dca297 ee62742c09ab981c458ee61a1c827ee6baefb08b M drivers
:040000 040000 f8ebebe7282a5a69ed50b1f4ed687f8e8fc6a9ce ffd40f9c2a640f9aa74bbef92fc4a3e2925b04e0 M include
Reverting the commit fixes ethernet support on Wandboard Quad.
Charles, do you have any idea why your commit breaks ethernet support on Wandboard Quad? This is what I'm seeing:
U-Boot 2013.07-rc3-00021-g576aacd (Jul 18 2013 - 08:34:32)
CPU: Freescale i.MX6Q rev1.2 at 792 MHz Reset cause: POR Board: Wandboard DRAM: 2 GiB MMC: FSL_SDHC: 0, FSL_SDHC: 1
*** Warning - bad CRC, using default environment
In: serial Out: serial Err: serial Net: FEC [PRIME] Warning: FEC using MAC address from net device
Hit any key to stop autoboot: 0
=> setenv autoload no && dhcp && tftp ${fdt_addr} /wandboard/imx6q-wandboard.dtb && fdt addr ${fdt_addr} && tftpboot ${loadaddr} /wandboard/zImage && setenv bootargs console=${console},
${baudrate} rootwait && bootz ${loadaddr} - ${fdt_addr}
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
BOOTP broadcast 5
Retry count exceeded; starting again
[...]
Sascha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130718/48b4a209/attachment.pgp>
Hi Sascha,
Quoted 17 lines · Sascha Silbe
Quoted 4 lines · Sascha Silbe
I've started a git bisect run (booting via USB OTG), but it's going to take a while as git bisect is throwing lots of commits without Wandboard Quad support at me and seems to be only narrowing down the commits one by one rather than on a log-2 basis.
A slightly more refined approach (cherry-picking the Wandboard Quad support patch each time the build fails) enabled git bisect to track the breakage down to:
commit
de1d786edf01035f60a87e1e0f917a4169dc6964Author: Charles Coldwell <[email protected]> Date: Thu Feb 21 08:25:52 2013 -0500add support for Xilinx 1000BASE-X phy (GTX)
Quoted 7 lines
commit
39695029bc15041c809df3db4ba19bd729c447fa
Author: Charles Coldwell <[email protected]>
Date: Tue Feb 19 08:27:33 2013 -0500Changes to support the Xilinx 1000BASE-X phy (GTX/MGT)
Signed-off-by Charles Coldwell <[email protected]>
I confirm the same here: currently ethernet is broken in U-boot master and reverting such commit make it work again.
Thanks,
Fabio Estevam
Quoted 24 lines · Fabio Estevam
Hi Sascha,
Quoted 21 lines · Sascha Silbe
Quoted 4 lines · Sascha Silbe
I've started a git bisect run (booting via USB OTG), but it's going to take a while as git bisect is throwing lots of commits without Wandboard Quad support at me and seems to be only narrowing down the commits one by one rather than on a log-2 basis.
A slightly more refined approach (cherry-picking the Wandboard Quad support patch each time the build fails) enabled git bisect to track the breakage down to:
commit
de1d786edf01035f60a87e1e0f917a4169dc6964Author: Charles Coldwell <[email protected]> Date: Thu Feb 21 08:25:52 2013 -0500add support for Xilinx 1000BASE-X phy (GTX)
commit
39695029bc15041c809df3db4ba19bd729c447fa
Author: Charles Coldwell <[email protected]>
Date: Tue Feb 19 08:27:33 2013 -0500
Quoted 6 lines
Quoted 3 lines
Changes to support the Xilinx 1000BASE-X phy (GTX/MGT)
Signed-off-by Charles Coldwell <[email protected]>I confirm the same here: currently ethernet is broken in U-boot master and reverting such commit make it work again.
I have tried:
drivers/net/phy/phy.c +1 -1
@@ -404,7 +404,7 @@ int genphy_config(struct phy_device *phydev)if (val &ESTATUS_1000_XFULL ) features |= SUPPORTED_1000baseX_Full;if (val &ESTATUS_1000_XHALF ) - features |= SUPPORTED_1000baseX_Full; + features |= SUPPORTED_1000baseX_Half; } phydev->supported = features;
,but it still did not fix the issue.
Charles/Joe,
Any ideas?
Joe,
Quoted 14 lines · Fabio Estevam
I have tried:
drivers/net/phy/phy.c +1
-1@@ -404,7 +404,7 @@ int genphy_config(struct phy_device *phydev)if (val &ESTATUS_1000_XFULL ) features |= SUPPORTED_1000baseX_Full;if (val &ESTATUS_1000_XHALF ) - features |= SUPPORTED_1000baseX_Full; + features |= SUPPORTED_1000baseX_Half; } phydev->supported = features;
Quoted 5 lines
,but it still did not fix the issue.
Charles/Joe,
Any ideas?
Also tested on a mx6qsabresd and ethernet is also broken there as well.
The following patch fixes it:
drivers/net/phy/phy.c +2 -6
@@ -337,12 +337,8 @@ int genphy_parse_link(struct phy_device *phydev) estatus =phy_read (phydev,MDIO_DEVAD_NONE ,MII_ESTATUS ); -if (estatus & (ESTATUS_1000_XFULL |ESTATUS_1000_XHALF | -ESTATUS_1000_TFULL |ESTATUS_1000_THALF )) { - phydev->speed =SPEED_1000 ; -if (estatus & (ESTATUS_1000_XFULL |ESTATUS_1000_TFULL )) - phydev->duplex =DUPLEX_FULL ; - } +if (estatus & (ESTATUS_1000_XFULL |ESTATUS_1000_TFULL )) + phydev->duplex =DUPLEX_FULL ; }else {u32 bmcr =phy_read (phydev,MDIO_DEVAD_NONE ,MII_BMCR );
Is this the correct fix?
Quoted 21 lines · Fabio Estevam
The following patch fixes it:
drivers/net/phy/phy.c +2
-6@@ -337,12 +337,8 @@ int genphy_parse_link(struct phy_device *phydev) estatus =phy_read (phydev,MDIO_DEVAD_NONE ,MII_ESTATUS ); -if (estatus & (ESTATUS_1000_XFULL |ESTATUS_1000_XHALF | -ESTATUS_1000_TFULL |ESTATUS_1000_THALF )) { - phydev->speed =SPEED_1000 ; -if (estatus & (ESTATUS_1000_XFULL |ESTATUS_1000_TFULL )) - phydev->duplex =DUPLEX_FULL ; - } +if (estatus & (ESTATUS_1000_XFULL |ESTATUS_1000_TFULL )) + phydev->duplex =DUPLEX_FULL ; }else {u32 bmcr =phy_read (phydev,MDIO_DEVAD_NONE ,MII_BMCR );
Quoted 1 line
Is this the correct fix?
No, I don't think so.
In the previous version, it would have set both phydev->speed and phydev->duplex; after your patch it only sets phydev->duplex.
What happens if you just remove the line that assigns phydev->speed in the previous version?
--
Charles M. Coldwell, W1CMC
Belmont, Massachusetts, New England
"Turn on, log in, tune out"
Quoted 2 lines · Charles Coldwell
What happens if you just remove the line that assigns
phydev->speedin the previous version?
It works fine if I do:
drivers/net/phy/phy.c +0 -1
@@ -339,7 +339,6 @@ int genphy_parse_link(struct phy_device *phydev)if (estatus & (ESTATUS_1000_XFULL |ESTATUS_1000_XHALF |ESTATUS_1000_TFULL |ESTATUS_1000_THALF )) { - phydev->speed =SPEED_1000 ;if (estatus & (ESTATUS_1000_XFULL |ESTATUS_1000_TFULL )) phydev->duplex =DUPLEX_FULL ; }
Is this the correct fix?
Quoted 4 lines · Sascha Silbe
Charles, do you have any idea why your commit breaks ethernet support on Wandboard Quad?
I've never heard of the Wandboard Quad, so I suppose the short answer is "no". However, the philosophy of the patch I submitted was:
1. Check the ESTATEN (Extended Status Enabled) bit in the BMSR (Basic Mode Status Register at address 0x01) MII register.
2. If it is set, read the ESTATUS (Extended Status) register
3. If the ESTATUS register indicates that it supports 1000BASE-X or 1000BASE-T, set the corresponding speed and duplex fields of the phy_device structure.
The MDIO registers in the Xilinx 1000BASE-X phy are documented here
http://www.xilinx.com/support/documentation/user_guides/ug194.pdf
starting on page 122.
ESTATEN is bit 8 of the BMSR at address 0x01; ESTATUS is at register address 15 and should always hold the value 0x8000 on the Xilinx 1000BASE-X phy since the only mode it supports is 1000BASE-X full duplex.
So, AFAICS, the patch should function correctly if both phys are using the same register maps. However, I don't think the Xilinx register map is blessed by the MDIO standards bodies, and therein might lie the trouble.
Can somebody look up the phy register maps for the i.MX6 phy (if it's built in) or whatever the Wandboard is using?
--
Charles M. Coldwell, W1CMC
Belmont, Massachusetts, New England
"Turn on, log in, tune out"
Quoted 2 lines · Charles Coldwell
Can somebody look up the phy register maps for the i.MX6 phy (if it's built in) or whatever the Wandboard is using?
mx6qsabresd and wandboard both use AR8031 PHY.
Quoted 12 lines · Charles Coldwell
Quoted 4 lines · Sascha Silbe
Charles, do you have any idea why your commit breaks ethernet support on Wandboard Quad?
I've never heard of the Wandboard Quad, so I suppose the short answer is "no". However, the philosophy of the patch I submitted was:
1. Check the ESTATEN (Extended Status Enabled) bit in the BMSR (Basic Mode Status Register at address
0x01) MII register.
Quoted 7 lines
2. If it is set, read the ESTATUS (Extended Status) register
3. If the ESTATUS register indicates that it supports 1000BASE-X or 1000BASE-T, set the corresponding speed and duplex fields of the
phy_devicestructure.The MDIO registers in the Xilinx 1000BASE-X phy are documented here
Which board uses the Xilinx 1000BASE-X in U-boot?
Hello Charles,
[CC += a few people that were CC'ed on the revert of Charles' patch]
Quoted 2 lines · Charles Coldwell
I've never heard of the Wandboard Quad, so I suppose the short answer is "no". However, the philosophy of the patch I submitted was:
[...]
Thanks for the description and the pointer to the Xilinx register description. I think I got to the bottom of it.
The Xilinx PHY supports the GMII basic register set (registers 0, 1 and 15), but not the full extended register set (registers 2-14). Especially the MASTER-SLAVE Control and Status registers (IEEE 802.3 terminology) are missing. Bit 0 (Extended Capability) of the (non-Extended) Status register is correctly set to 0 to indicate this lack of support.
Without the MASTER-SLAVE Status register, we can't tell whether the _peer_ also supports 1Gbps operation. Your patch ends up enabling it anyway, even for 10/100Mbps peers.
Can you try the patch below, please? It restricts Extended Status processing to the PHYs that don't support the MASTER-SLAVE Control and Status registers, like the Xilinx one you use. The other PHYs should continue to work as before your patch. Tested successfully on Wandboard Quad.
Sascha
-- >8 --
From: Sascha Silbe <[email protected]>
Date: Fri, 19 Jul 2013 11:37:54 +0200
Subject: [PATCH] phy: fix 10/100Mbps operation on 1Gbps-capable links
de1d786 [add support for Xilinx 1000BASE-X phy (GTX)] introduced a check for the extended status register in order to support 1Gbps-capable PHYs that don't have the 1000BASE-T registers. Since Extended Status only indicates what the PHY (i.e. the local side) is capable of, this broke communication with non-1Gbps peers.
Only check the extended status if the 1000BASE-T registers are actually missing so we don't end up setting speed to 1Gbps even though the previous test (for the combination of local and peer support for 1Gbps) already indicated we can't do 1Gbps with the current peer.
--- drivers/net/phy/phy.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/net/phy/phy.c +8 -1
@@ -333,7 +333,14 @@ int genphy_parse_link(struct phy_device *phydev) }else if (lpa &LPA_10FULL ) phydev->duplex =DUPLEX_FULL ; -if (mii_reg &BMSR_ESTATEN ) +/* Extended status may indicate that the PHY supports +* 1000BASE-T/X even though the 1000BASE-T registers +* are missing. In this case we can't tell whether the +* peer also supports it, so we only check extended +* status if the 1000BASE-T registers are actually +* missing. +*/ +if ((mii_reg &BMSR_ESTATEN ) && !(mii_reg &BMSR_ERCAP )) estatus =phy_read (phydev,MDIO_DEVAD_NONE ,MII_ESTATUS );
Signature · 9 lines
1.8.3.2 -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 489 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130719/369ef7ab/attachment.pgp>
Fabio Estevam
Tested-byQuoted 23 lines · Sascha Silbe
Hello Charles,
[CC += a few people that were CC'ed on the revert of Charles' patch]
Quoted 2 lines · Charles Coldwell
I've never heard of the Wandboard Quad, so I suppose the short answer is "no". However, the philosophy of the patch I submitted was:
[...]
Thanks for the description and the pointer to the Xilinx register description. I think I got to the bottom of it.
The Xilinx PHY supports the GMII basic register set (registers 0, 1 and 15), but not the full extended register set (registers 2-14). Especially the MASTER-SLAVE Control and Status registers (IEEE 802.3 terminology) are missing. Bit 0 (Extended Capability) of the (non-Extended) Status register is correctly set to 0 to indicate this lack of support.
Without the MASTER-SLAVE Status register, we can't tell whether the _peer_ also supports 1Gbps operation. Your patch ends up enabling it anyway, even for 10/100Mbps peers.
Quoted 5 lines
Can you try the patch below, please? It restricts Extended Status processing to the PHYs that don't support the MASTER-SLAVE Control and Status registers, like the Xilinx one you use. The other PHYs should continue to work as before your patch. Tested successfully on Wandboard Quad.
Thanks for your detailed analysis, Sascha.
On a mx6qsabresd:
Hopefully we can get it applied into the upcoming 2013.07 in order to avoid the regression on some mx6 boards.
Quoted 30 lines · Fabio Estevam
Quoted 23 lines · Sascha Silbe
Hello Charles,
[CC += a few people that were CC'ed on the revert of Charles' patch]
Quoted 2 lines · Charles Coldwell
I've never heard of the Wandboard Quad, so I suppose the short answer is "no". However, the philosophy of the patch I submitted was:
[...]
Thanks for the description and the pointer to the Xilinx register description. I think I got to the bottom of it.
The Xilinx PHY supports the GMII basic register set (registers 0, 1 and 15), but not the full extended register set (registers 2-14). Especially the MASTER-SLAVE Control and Status registers (IEEE 802.3 terminology) are missing. Bit 0 (Extended Capability) of the (non-Extended) Status register is correctly set to 0 to indicate this lack of support.
Without the MASTER-SLAVE Status register, we can't tell whether the _peer_ also supports 1Gbps operation. Your patch ends up enabling it anyway, even for 10/100Mbps peers.
Quoted 5 lines
Can you try the patch below, please? It restricts Extended Status processing to the PHYs that don't support the MASTER-SLAVE Control and Status registers, like the Xilinx one you use. The other PHYs should continue to work as before your patch. Tested successfully on Wandboard Quad.
Quoted 8 lines
Thanks for your detailed analysis, Sascha.
On a mx6qsabresd:
Tested-by Fabio Estevam <[email protected]>Hopefully we can get it applied into the upcoming 2013.07 in order to avoid the regression on some mx6 boards.
Can we also get this tested on the board Charles has? Thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130719/c40dd077/attachment.pgp>
It's difficult. The board has been taken away from me. It was a Virtex 6 FPGA running Microblaze Linux; I might be able to test on a Virtex 7 but not for several days.
Sascha Silbe's fix looks good to me, however.
Quoted 44 lines · Tom Rini
Quoted 30 lines · Fabio Estevam
Quoted 23 lines · Sascha Silbe
Hello Charles,
[CC += a few people that were CC'ed on the revert of Charles' patch]
Quoted 2 lines · Charles Coldwell
I've never heard of the Wandboard Quad, so I suppose the short answer is "no". However, the philosophy of the patch I submitted was:
[...]
Thanks for the description and the pointer to the Xilinx register description. I think I got to the bottom of it.
The Xilinx PHY supports the GMII basic register set (registers 0, 1 and 15), but not the full extended register set (registers 2-14). Especially the MASTER-SLAVE Control and Status registers (IEEE 802.3 terminology) are missing. Bit 0 (Extended Capability) of the (non-Extended) Status register is correctly set to 0 to indicate this lack of support.
Without the MASTER-SLAVE Status register, we can't tell whether the _peer_ also supports 1Gbps operation. Your patch ends up enabling it anyway, even for 10/100Mbps peers.
Quoted 5 lines
Can you try the patch below, please? It restricts Extended Status processing to the PHYs that don't support the MASTER-SLAVE Control and Status registers, like the Xilinx one you use. The other PHYs should continue to work as before your patch. Tested successfully on Wandboard Quad.
Quoted 8 lines
Thanks for your detailed analysis, Sascha.
On a mx6qsabresd:
Tested-by Fabio Estevam <[email protected]>Hopefully we can get it applied into the upcoming 2013.07 in order to avoid the regression on some mx6 boards.
Can we also get this tested on the board Charles has? Thanks!
--
Tom
Signature · 3 lines
Charles M. Coldwell, W1CMC Belmont, Massachusetts, New England "Turn on, log in, tune out"
Joe Hershberger
Reviewed-byQuoted 10 lines · Sascha Silbe
From: Sascha Silbe <[email protected]>
Date: Fri, 19 Jul 2013 11:37:54 +0200
Subject: [PATCH] phy: fix 10/100Mbps operation on 1Gbps-capable linksde1d786 [add support for Xilinx 1000BASE-X phy (GTX)] introduced a check for the extended status register in order to support 1Gbps-capable PHYs that don't have the 1000BASE-T registers. Since Extended Status only indicates what the PHY (i.e. the local side) is capable of, this broke communication with non-1Gbps peers.
Quoted 6 lines
Only check the extended status if the 1000BASE-T registers are actually missing so we don't end up setting speed to 1Gbps even though the previous test (for the combination of local and peer support for 1Gbps) already indicated we can't do 1Gbps with the current peer.
Signed-off-by Sascha Silbe <[email protected]>
Quoted 8 lines · Sascha Silbe
Subject: [PATCH] phy: fix 10/100Mbps operation on 1Gbps-capable links
de1d786 [add support for Xilinx 1000BASE-X phy (GTX)] introduced a check for the extended status register in order to support 1Gbps-capable PHYs that don't have the 1000BASE-T registers. Since Extended Status only indicates what the PHY (i.e. the local side) is capable of, this broke communication with non-1Gbps peers.
Quoted 6 lines
Only check the extended status if the 1000BASE-T registers are actually missing so we don't end up setting speed to 1Gbps even though the previous test (for the combination of local and peer support for 1Gbps) already indicated we can't do 1Gbps with the current peer.
Signed-off-by Sascha Silbe <[email protected]>
Applied to u-boot/master, after fixing the comment style, thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130719/54868a99/attachment.pgp>