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
> 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
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/[email protected]/
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
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/[email protected]/
>
> 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