| Message ID | b693b45a689d42b60a8a9e29dcdd2fb27e20df82.1321251618.git.baruch@tkos.co.il |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Baruch Siach <baruch@tkos.co.il> Date: Mon, 14 Nov 2011 08:21:30 +0200 > Since 952c5ca1 (fsl_pq_mdio: Clean up tbi address configuration) .probe returns > -EBUSY when the "tbi-phy" node is missing. Fix this. > > Cc: Andy Fleming <afleming@freescale.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Well, this got applied quickly, so I guess I can't NAK, but this requires discussion. On Nov 14, 2011, at 0:22, "Baruch Siach" <baruch@tkos.co.il> wrote: > Since 952c5ca1 (fsl_pq_mdio: Clean up tbi address configuration) .probe returns > -EBUSY when the "tbi-phy" node is missing. Fix this. It returns an error because it finds no tbi node. Because without the tbi node, there is no way for the driver to determine which address to set. Your solution is to ignore the error, and hope. That's a broken approach. The real solution for a p1010 should be to have a tbi node in the dts. And looking at the p1010si.dtsi, I see that it's automatically there for you. How were you breaking? Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On Mon, Nov 14, 2011 at 09:04:47PM +0000, Fleming Andy-AFLEMING wrote: > Well, this got applied quickly, so I guess I can't NAK, but this requires discussion. > > On Nov 14, 2011, at 0:22, "Baruch Siach" <baruch@tkos.co.il> wrote: > > > Since 952c5ca1 (fsl_pq_mdio: Clean up tbi address configuration) .probe returns > > -EBUSY when the "tbi-phy" node is missing. Fix this. > > It returns an error because it finds no tbi node. Because without the tbi > node, there is no way for the driver to determine which address to set. > > Your solution is to ignore the error, and hope. That's a broken approach. > The real solution for a p1010 should be to have a tbi node in the dts. Can you elaborate a bit on why this approach is broken? The PHY used to work for me until 952c5ca1, and with this applied. > And looking at the p1010si.dtsi, I see that it's automatically there for > you. > > How were you breaking? Adding linuxppc to Cc. My board is P1011 based, the single core version of P1020, not P1010. In p1020si.dtsi I see no tbi node. In p1020rdb.dts I see a tbi node but only for mdio@25000, not mdio@24000, which is what I'm using. Am I missing something? baruch
On Nov 14, 2011, at 11:17 PM, Baruch Siach wrote: > Hi Andy, > > On Mon, Nov 14, 2011 at 09:04:47PM +0000, Fleming Andy-AFLEMING wrote: >> Well, this got applied quickly, so I guess I can't NAK, but this requires discussion. >> >> On Nov 14, 2011, at 0:22, "Baruch Siach" <baruch@tkos.co.il> wrote: >> >>> Since 952c5ca1 (fsl_pq_mdio: Clean up tbi address configuration) .probe returns >>> -EBUSY when the "tbi-phy" node is missing. Fix this. >> >> It returns an error because it finds no tbi node. Because without the tbi >> node, there is no way for the driver to determine which address to set. >> >> Your solution is to ignore the error, and hope. That's a broken approach. >> The real solution for a p1010 should be to have a tbi node in the dts. > > Can you elaborate a bit on why this approach is broken? The PHY used to work > for me until 952c5ca1, and with this applied. Yes, well, just because a problem goes away when a patch is applied does not mean that the patch is correct, or that it made things work. An explanation: In order to support certain types of serial data interfaces with external PHYs (like SGMII), it is necessary to translate the MAC's data signaling into the serialized signaling. On Freescale parts, this is done via a SerDes block, but the SerDes link needs a small amount of management. To perform this management, we have an onboard "TBI" PHY. This PHY is highly integrated with the MAC and MDIO devices. Each MAC has two relevant components: 1) a TBIPA register, which declares the address of the TBI PHY 2) an associated MDIO controller. In order to configure the SerDes link, it is necessary to communicate via the "local" MDIO controller with the TBI PHY. For most of the MACs, this is simple: Choose an address for TBIPA, and then use that address to communicate with the TBI PHY. However, the *first* MDIO controller is also used to communicate with external PHYs. On this controller, we have to be fairly particular about which address we put in TBIPA, because all transactions to that address will go to the TBI PHY. On older parts, this value defaulted to "0", but it now defaults to "31", I believe. Ok, so now we're at this code. The of_mdiobus_register() function will parse the device tree, and find all of the PHYs on the MDIO bus, and register them as devices. In order to ensure that all of those PHYs are accessible, we *MUST* set TBIPA to something that won't conflict with any existing addresses. The mechanism we have chosen for this is to assign the address in the device tree, via a tbi-phy node. My recent patch changed the behavior, because we used to try to find a free address via scanning, but this was somewhat ugly, and failed (as you noticed) due to uninitialized mutexes. The reason your latest patch is wrong is because it doesn't set the TBIPA register at all if there is no tbi-phy node. Instead, it just relies on luck, hoping that the TBIPA register was set to something that doesn't conflict already. It will work if 0x1f or 0 aren't necessary PHY addresses for your board, or if the firmware set it to something sensible. > >> And looking at the p1010si.dtsi, I see that it's automatically there for >> you. >> >> How were you breaking? > > Adding linuxppc to Cc. > > My board is P1011 based, the single core version of P1020, not P1010. In > p1020si.dtsi I see no tbi node. In p1020rdb.dts I see a tbi node but only for > mdio@25000, not mdio@24000, which is what I'm using. > > Am I missing something? Well, that's a bug. In truth, the silicon dtsi trees should not have tbi nodes, as that's highly machine-specific. The p1020rdb is apparently relying on the old behavior, which is broken, and due to the fact that the first ethernet interface doesn't *use* the TBI PHY. You should add this to your board tree: mdio@24000 { tbi0: tbi-phy@11 { reg = <0x11>; device_type = "tbi-phy"; }; }; And add the PHYs you use, as well as set reg (and the value after the "@") to something that makes sense for your board. I am going to go right now, and add tbi nodes for all of the Freescale platforms. I will also modify the fsl_pq_mdio code to be more explicit about its reason for failure. Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On Tue, Nov 15, 2011 at 09:06:03AM -0600, Andy Fleming wrote: > On Nov 14, 2011, at 11:17 PM, Baruch Siach wrote: > > On Mon, Nov 14, 2011 at 09:04:47PM +0000, Fleming Andy-AFLEMING wrote: [snip] > >> And looking at the p1010si.dtsi, I see that it's automatically there for > >> you. > >> > >> How were you breaking? > > > > Adding linuxppc to Cc. > > > > My board is P1011 based, the single core version of P1020, not P1010. In > > p1020si.dtsi I see no tbi node. In p1020rdb.dts I see a tbi node but only for > > mdio@25000, not mdio@24000, which is what I'm using. > > > > Am I missing something? > > Well, that's a bug. In truth, the silicon dtsi trees should not have tbi > nodes, as that's highly machine-specific. The p1020rdb is apparently relying > on the old behavior, which is broken, and due to the fact that the first > ethernet interface doesn't *use* the TBI PHY. > > You should add this to your board tree: > > mdio@24000 { > > tbi0: tbi-phy@11 { > reg = <0x11>; > device_type = "tbi-phy"; > }; > }; > > And add the PHYs you use, as well as set reg (and the value after the "@") > to something that makes sense for your board. Thanks for your detailed explanation and prompt response. I've added a tbi node, dropped my patch, and now my board works as expected. > I am going to go right now, and add tbi nodes for all of the Freescale > platforms. I will also modify the fsl_pq_mdio code to be more explicit about > its reason for failure. Please Cc me on these. Thanks, baruch
diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c index 4d9f84b..8dee1ae 100644 --- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c +++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c @@ -356,16 +356,16 @@ static int fsl_pq_mdio_probe(struct platform_device *ofdev) if (prop) tbiaddr = *prop; - } - if (tbiaddr == -1) { - err = -EBUSY; + if (tbiaddr == -1) { + err = -EBUSY; - goto err_free_irqs; + goto err_free_irqs; + } else { + out_be32(tbipa, tbiaddr); + } } - out_be32(tbipa, tbiaddr); - err = of_mdiobus_register(new_bus, np); if (err) { printk (KERN_ERR "%s: Cannot register as MDIO bus\n",
Since 952c5ca1 (fsl_pq_mdio: Clean up tbi address configuration) .probe returns -EBUSY when the "tbi-phy" node is missing. Fix this. Cc: Andy Fleming <afleming@freescale.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- drivers/net/ethernet/freescale/fsl_pq_mdio.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)