Re: [PATCH] net: fsl_pq_mdio: fix non tbi phy access

On Nov 14, 2011, at 11:17 PM, Baruch Siach wrote:

Hi Andy,
=20
On Mon, Nov 14, 2011 at 09:04:47PM +0000, Fleming Andy-AFLEMING wrote:

quoted

Well, this got applied quickly, so I guess I can't NAK, but this =

requires discussion.

quoted

=20
On Nov 14, 2011, at 0:22, "Baruch Siach" [off-list ref] wrote:
=20

quoted

Since 952c5ca1 (fsl_pq_mdio: Clean up tbi address configuration) =

.probe returns

quoted

quoted

-EBUSY when the "tbi-phy" node is missing. Fix this.

=20
It returns an error because it finds no tbi node. Because without the =

tbi=20

quoted

node, there is no way for the driver to determine which address to =

set.

quoted

=20
Your solution is to ignore the error, and hope. That's a broken =

approach. =20

quoted

The real solution for a p1010 should be to have a tbi node in the =

dts.

=20
Can you elaborate a bit on why this approach is broken? The PHY used =

to work=20

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.

=20

quoted

And looking at the p1010si.dtsi, I see that it's automatically there =

for=20

quoted

you.
=20
How were you breaking?

=20
Adding linuxppc to Cc.
=20
My board is P1011 based, the single core version of P1020, not P1010. =

In=20

p1020si.dtsi I see no tbi node. In p1020rdb.dts I see a tbi node but =

only for=20

mdio@25000, not mdio@24000, which is what I'm using.
=20
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 =3D <0x11>;
                                device_type =3D "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=