| Message ID | toezjti9795.fsf@twin.sascha.silbe.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
On Fri, Jul 19, 2013 at 7:25 AM, Sascha Silbe <t-uboot@infra-silbe.de> wrote: > Hello Charles, > > [CC += a few people that were CC'ed on the revert of Charles' patch] > > Charles Coldwell <coldwell@gmail.com> writes: > >> 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. Thanks for your detailed analysis, Sascha. On a mx6qsabresd: Tested-by: Fabio Estevam <fabio.estevam@freescale.com> Hopefully we can get it applied into the upcoming 2013.07 in order to avoid the regression on some mx6 boards.
On Fri, Jul 19, 2013 at 09:38:42AM -0300, Fabio Estevam wrote: > On Fri, Jul 19, 2013 at 7:25 AM, Sascha Silbe <t-uboot@infra-silbe.de> wrote: > > Hello Charles, > > > > [CC += a few people that were CC'ed on the revert of Charles' patch] > > > > Charles Coldwell <coldwell@gmail.com> writes: > > > >> 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. > > Thanks for your detailed analysis, Sascha. > > On a mx6qsabresd: > > Tested-by: Fabio Estevam <fabio.estevam@freescale.com> > > 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!
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. On Fri, Jul 19, 2013 at 8:42 AM, Tom Rini <trini@ti.com> wrote: > On Fri, Jul 19, 2013 at 09:38:42AM -0300, Fabio Estevam wrote: >> On Fri, Jul 19, 2013 at 7:25 AM, Sascha Silbe <t-uboot@infra-silbe.de> wrote: >> > Hello Charles, >> > >> > [CC += a few people that were CC'ed on the revert of Charles' patch] >> > >> > Charles Coldwell <coldwell@gmail.com> writes: >> > >> >> 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. >> >> Thanks for your detailed analysis, Sascha. >> >> On a mx6qsabresd: >> >> Tested-by: Fabio Estevam <fabio.estevam@freescale.com> >> >> 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
On Fri, Jul 19, 2013 at 5:25 AM, Sascha Silbe <t-uboot@infra-silbe.de> wrote: > From: Sascha Silbe <t-uboot@infra-silbe.de> > 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. > > Signed-off-by: Sascha Silbe <t-uboot@infra-silbe.de> Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
On Fri, Jul 19, 2013 at 12:25:10PM +0200, Sascha Silbe wrote: > 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. > > Signed-off-by: Sascha Silbe <t-uboot@infra-silbe.de> Applied to u-boot/master, after fixing the comment style, thanks!
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 7c0eaec..f803834 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -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);