Clause 45 and Clause 22 PHYs on one MDIO bus

Clause 45 and Clause 22 PHYs on one MDIO bus

[Michael Walle]

Hi,

I have a board with a c22 phy (microchip lan8814) and a c45 phy
(intel/maxlinear gyp215) on one bus. If I understand it correctly, both
accesses should be able to coexist on one bus. But the microchip lan8814
actually has a bug and gets confused by c45 accesses. For example it 
will
respond in the middle of another transaction with its own data if it
decodes it as a read. That is something we can see on a logic analyzer.
But we also see random register writes on the lan8814 (which you don't 
see
on the logic analyzer obviously). Fortunately, the GPY215 supports 
indirect
MMD access by the standard c22 registers. Thus as a workaround for the
problem, we could have a c22 only mdio bus.

The SoC I'm using is the LAN9668, which uses the mdio-mscc-mdio driver.
First problem there, it doesn't support C45 (yet) but also doesn't check
for MII_ADDR_C45 and happily reads/writes bogus registers.

I've looked at the mdio subsystem in linux, there is probe_capabilities
(MDIOBUS_C45 and friends) but the mxl-gpy.c is using c45 accesses
nevertheless. I'm not sure if this is a bug or not.

I was thinking of a fallback mechanism for the c45 read access like
in read_mmd. And even if the mdio controller is c45 capable, a PHY
might opt out. In my case, the lan8814.

What do you think?

-michael

Re: Clause 45 and Clause 22 PHYs on one MDIO bus

[Michael Walle]

Am 2022-03-21 12:21, schrieb Michael Walle:
> Hi,
> 
> I have a board with a c22 phy (microchip lan8814) and a c45 phy
> (intel/maxlinear gyp215) on one bus. If I understand it correctly, both
> accesses should be able to coexist on one bus. But the microchip 
> lan8814
> actually has a bug and gets confused by c45 accesses. For example it 
> will
> respond in the middle of another transaction with its own data if it
> decodes it as a read. That is something we can see on a logic analyzer.
> But we also see random register writes on the lan8814 (which you don't 
> see
> on the logic analyzer obviously). Fortunately, the GPY215 supports 
> indirect
> MMD access by the standard c22 registers. Thus as a workaround for the
> problem, we could have a c22 only mdio bus.
> 
> The SoC I'm using is the LAN9668, which uses the mdio-mscc-mdio driver.
> First problem there, it doesn't support C45 (yet) but also doesn't 
> check
> for MII_ADDR_C45 and happily reads/writes bogus registers.
> 
> I've looked at the mdio subsystem in linux, there is probe_capabilities
> (MDIOBUS_C45 and friends) but the mxl-gpy.c is using c45 accesses
> nevertheless. I'm not sure if this is a bug or not.
> 
> I was thinking of a fallback mechanism for the c45 read access like
> in read_mmd. And even if the mdio controller is c45 capable, a PHY
> might opt out. In my case, the lan8814.

Actually, it looks like mdiobus_c45_read() is really c45 only and only
used for PHYs which just support c45 and not c45-over-c22 (?). I was
mistaken by the heavy use of the function in phy_device.c. All the
methods in phy-c45.c use phy_*_mmd() functions. Thus it might only be
the mxl-gpy doing something fishy in its probe function. (And I'm
unsure about get_phy_c45_ids()).

Nevertheless, I'd still need the opt-out of any c45 access. Otherwise,
if someone will ever implement c45 support for the mdio-mscc-mdio
driver, I'll run in the erratic behavior.

-michael

Re: Clause 45 and Clause 22 PHYs on one MDIO bus

[Andrew Lunn]

On Mon, Mar 21, 2022 at 12:21:48PM +0100, Michael Walle wrote:
> Hi,
> 
> I have a board with a c22 phy (microchip lan8814) and a c45 phy
> (intel/maxlinear gyp215) on one bus. If I understand it correctly, both
> accesses should be able to coexist on one bus.

Yes. A C22 PHY should ignore a C45 access and vica versa.

> But the microchip lan8814
> actually has a bug and gets confused by c45 accesses. For example it will
> respond in the middle of another transaction with its own data if it
> decodes it as a read. That is something we can see on a logic analyzer.
> But we also see random register writes on the lan8814 (which you don't see
> on the logic analyzer obviously). Fortunately, the GPY215 supports indirect
> MMD access by the standard c22 registers. Thus as a workaround for the
> problem, we could have a c22 only mdio bus.

That should work, but you loose some performance.

> The SoC I'm using is the LAN9668, which uses the mdio-mscc-mdio driver.
> First problem there, it doesn't support C45 (yet) but also doesn't check
> for MII_ADDR_C45 and happily reads/writes bogus registers.

There are many drivers like that :-(

Whenever a new driver is posted, it is one of the things i ask
for. But older drivers are missing such checks.

> I've looked at the mdio subsystem in linux, there is probe_capabilities
> (MDIOBUS_C45 and friends) but the mxl-gpy.c is using c45 accesses
> nevertheless. I'm not sure if this is a bug or not.

No, that is not a bug. The PHY driver knows the device should be c45
capable. So it will use C45 addresses. The phydev->is_c45 should
indicate if it is possible to perform c45 transactions to the PHY. If
it is not, the core will make use of indirect access via C22,
otherwise the core will perform a direct C45 access.

So it seems like all you need to do is make mdio-mscc-mdio return
-EOPNOTSUPP for C45 and check that phydev->is_c45 is correctly false.

	   Andrew

Re: Clause 45 and Clause 22 PHYs on one MDIO bus

[Andrew Lunn]

> Actually, it looks like mdiobus_c45_read() is really c45 only and only
> used for PHYs which just support c45 and not c45-over-c22 (?). I was
> mistaken by the heavy use of the function in phy_device.c. All the
> methods in phy-c45.c use phy_*_mmd() functions. Thus it might only be
> the mxl-gpy doing something fishy in its probe function.

Yes, there is something odd here. You should search back on the
mailing list.

If i remember correctly, it is something like it responds to both c22
and c45. If it is found via c22, phylib does not set phydev->is_c45,
and everything ends up going indirect. So the probe additionally tries
to find it via c45? Or something like that.

> Nevertheless, I'd still need the opt-out of any c45 access. Otherwise,
> if someone will ever implement c45 support for the mdio-mscc-mdio
> driver, I'll run in the erratic behavior.

Yah, i need to think about that. Are you purely in the DT world, or is
ACPI also an option?

Maybe extend of_mdiobus_register() to look for a DT property to limit
what values probe_capabilities can take?

     Andrew

Re: Clause 45 and Clause 22 PHYs on one MDIO bus

[Michael Walle]

Am 2022-03-21 21:36, schrieb Andrew Lunn:
>> Actually, it looks like mdiobus_c45_read() is really c45 only and only
>> used for PHYs which just support c45 and not c45-over-c22 (?). I was
>> mistaken by the heavy use of the function in phy_device.c. All the
>> methods in phy-c45.c use phy_*_mmd() functions. Thus it might only be
>> the mxl-gpy doing something fishy in its probe function.
> 
> Yes, there is something odd here. You should search back on the
> mailing list.
> 
> If i remember correctly, it is something like it responds to both c22
> and c45. If it is found via c22, phylib does not set phydev->is_c45,
> and everything ends up going indirect. So the probe additionally tries
> to find it via c45? Or something like that.

Yeah, found it: https://lore.kernel.org/netdev/YLaG9cdn6ewdffjV@lunn.ch/

But that means that if the controller is not c45 capable, it will always
fail to probe, no?

I've added the "if (regnum & MII_ADDR_C45) return -EOPNOTSUPP" to the
mdio driver and the gpy phy will then fail to probe - as expected.

Should it check for -EOPNOTSUPP and just ignore that error and continue
probing? Or make it a no-op if probe_capabilities say it has no c45
access so it would take advantage of a quirk flag (derived from dt)?

>> Nevertheless, I'd still need the opt-out of any c45 access. Otherwise,
>> if someone will ever implement c45 support for the mdio-mscc-mdio
>> driver, I'll run in the erratic behavior.
> 
> Yah, i need to think about that. Are you purely in the DT world, or is
> ACPI also an option?

Just DT world.

> Maybe extend of_mdiobus_register() to look for a DT property to limit
> what values probe_capabilities can take?

I'll have to give it a try. First I was thinking that we wouldn't need
it because a broken PHY driver could just set a quirk 
"broken_c45_access"
or similar. But that would mean it has to be probed before any c45 PHY.
Dunno if that will be true for the future. And it sounds rather fragile.
So yes, a dt property might be a better option.

-michael

Re: Clause 45 and Clause 22 PHYs on one MDIO bus

[Michael Walle]

Am 2022-03-21 21:21, schrieb Andrew Lunn:
> On Mon, Mar 21, 2022 at 12:21:48PM +0100, Michael Walle wrote:
>> The SoC I'm using is the LAN9668, which uses the mdio-mscc-mdio 
>> driver.
>> First problem there, it doesn't support C45 (yet) but also doesn't 
>> check
>> for MII_ADDR_C45 and happily reads/writes bogus registers.
> 
> There are many drivers like that :-(
> 
> Whenever a new driver is posted, it is one of the things i ask
> for. But older drivers are missing such checks.

Should that be a patch for net or net-next? One thing to consider;
The gpy215 is probing just fine with a c22 only mdio driver which 
doesn't
check for c45 accesses. It might read fishy registers during its probe,
though. After adding the c45 check in the mdio drivers read and write
it will fail to probe. So depending on the mdio driver it might went
unnoticed that the phy_get_c45_ids() could fail.

If it should go via net, then it should probably be accompanied
by a patch to fix the gpy_probe() (i.e. ignoring -EOPNOTSUPP
error).

-michael

Re: Clause 45 and Clause 22 PHYs on one MDIO bus

[Andrew Lunn]

On Mon, Mar 21, 2022 at 10:41:56PM +0100, Michael Walle wrote:
> Am 2022-03-21 21:36, schrieb Andrew Lunn:
> > > Actually, it looks like mdiobus_c45_read() is really c45 only and only
> > > used for PHYs which just support c45 and not c45-over-c22 (?). I was
> > > mistaken by the heavy use of the function in phy_device.c. All the
> > > methods in phy-c45.c use phy_*_mmd() functions. Thus it might only be
> > > the mxl-gpy doing something fishy in its probe function.
> > 
> > Yes, there is something odd here. You should search back on the
> > mailing list.
> > 
> > If i remember correctly, it is something like it responds to both c22
> > and c45. If it is found via c22, phylib does not set phydev->is_c45,
> > and everything ends up going indirect. So the probe additionally tries
> > to find it via c45? Or something like that.
> 
> Yeah, found it: https://lore.kernel.org/netdev/YLaG9cdn6ewdffjV@lunn.ch/
> 
> But that means that if the controller is not c45 capable, it will always
> fail to probe, no?

The problem is around here:

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-c45.c#L455

phydev->c45_ids.mmds_present needs to be set. Which happens as part of
get_phy_c45_ids(). What probably needs to happen is the
mdiobus_c45_read() in that function need to change to phy_read_mmd()
so that it can use C45 over C22. The devil in the details is making
sure it does actually do C45 if C45 is available, otherwise we could
break devices on a C45 only bus, of which there is a few.

> I'll have to give it a try. First I was thinking that we wouldn't need
> it because a broken PHY driver could just set a quirk "broken_c45_access"
> or similar. But that would mean it has to be probed before any c45 PHY.
> Dunno if that will be true for the future. And it sounds rather fragile.
> So yes, a dt property might be a better option.

This is unfortunately not a PHY property, but a bus property. This bus
has a FUBAR PHY on it, which limits the protocols that can be used on
this bus. And there is no clear relationship between PHYs, you cannot
easily say which PHYs share the same bus. So even if the lan8814 was
to indicate it is FUBAR, you have no idea which other PHYs are
affected.

A DT property is more generic, and if done correct, could actually ban
C22 or it could ban C45.

    Andrew

Re: Clause 45 and Clause 22 PHYs on one MDIO bus

[Andrew Lunn]

On Mon, Mar 21, 2022 at 10:51:29PM +0100, Michael Walle wrote:
> Am 2022-03-21 21:21, schrieb Andrew Lunn:
> > On Mon, Mar 21, 2022 at 12:21:48PM +0100, Michael Walle wrote:
> > > The SoC I'm using is the LAN9668, which uses the mdio-mscc-mdio
> > > driver.
> > > First problem there, it doesn't support C45 (yet) but also doesn't
> > > check
> > > for MII_ADDR_C45 and happily reads/writes bogus registers.
> > 
> > There are many drivers like that :-(
> > 
> > Whenever a new driver is posted, it is one of the things i ask
> > for. But older drivers are missing such checks.
> 
> Should that be a patch for net or net-next? One thing to consider;
> The gpy215 is probing just fine with a c22 only mdio driver which doesn't
> check for c45 accesses. It might read fishy registers during its probe,
> though. After adding the c45 check in the mdio drivers read and write
> it will fail to probe. So depending on the mdio driver it might went
> unnoticed that the phy_get_c45_ids() could fail.
> 
> If it should go via net, then it should probably be accompanied
> by a patch to fix the gpy_probe() (i.e. ignoring -EOPNOTSUPP
> error).

I would suggest net-next first, so it gets some testing. We can then
add it to net later. We just need to keep an eye out for the automagic
bots which magically pick patches to backport. We don't want them to
pickup these patches too soon and only take part of the fix.

       Andrew