* Re: [PATCH] net: fsl_pq_mdio: fix non tbi phy access
2011-11-15 5:17 ` [PATCH] net: fsl_pq_mdio: fix non tbi phy access Baruch Siach
@ 2011-11-15 15:06 ` Andy Fleming
2011-11-15 15:44 ` Baruch Siach
0 siblings, 1 reply; 3+ messages in thread
From: Andy Fleming @ 2011-11-15 15:06 UTC (permalink / raw)
To: Baruch Siach; +Cc: netdev@vger.kernel.org, linuxppc-dev
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:
>> Well, this got applied quickly, so I guess I can't NAK, but this =
requires discussion.
>>=20
>> On Nov 14, 2011, at 0:22, "Baruch Siach" <baruch@tkos.co.il> wrote:
>>=20
>>> Since 952c5ca1 (fsl_pq_mdio: Clean up tbi address configuration) =
.probe returns
>>> -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
>> node, there is no way for the driver to determine which address to =
set.
>>=20
>> Your solution is to ignore the error, and hope. That's a broken =
approach. =20
>> 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
>> And looking at the p1010si.dtsi, I see that it's automatically there =
for=20
>> 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=
^ permalink raw reply [flat|nested] 3+ messages in thread