C22/C45 decision in phy_restart_aneg()

C22/C45 decision in phy_restart_aneg()

[markus.stockhausen]

Hi,

@Russell: Sorry for the inconvenience of my first mail. After a lengthy
analysis session, I focused too much on the initial C45 enhancement 
of the below function that you wrote. I sent it off too hastily.

So, once again, here is some interesting finding regarding the 
limitations of the Realtek MDIO driver. Remember that it can only run 
in either C22 or C45. This time it is about autonegotiation restart.

int phy_restart_aneg(struct phy_device *phydev) {
  int ret;

  if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0))
    ret = genphy_c45_restart_aneg(phydev);
  else
    ret = genphy_restart_aneg(phydev);

  return ret;
}

I assume that BIT(0) means MDIO_DEVS_C22_PRESENT. As I understand it,
this basically uses C22 commands for C45 PHYs if C22 support is detected.
Currently, I have no reasonable explanation for this additional check.
 
In our case, this function fails for C45 PHYs because the bus and PHY are 
locked down to this single mode. It's stupid, of course, but that's how 
it is. I see two options for fixing the issue:

1) Mask the C22 presence in the bus for all PHYs when running in C45.
2) Drop the C22 condition check in the above function.

Any advice?

Thanks in advance.

Markus

Re: C22/C45 decision in phy_restart_aneg()

[Russell King (Oracle)]

On Sun, Aug 31, 2025 at 09:45:46AM +0200, markus.stockhausen@gmx.de wrote:
> @Russell: Sorry for the inconvenience of my first mail. After a lengthy
> analysis session, I focused too much on the initial C45 enhancement 
> of the below function that you wrote. I sent it off too hastily.

It's important to send messages to the right people for a proper
discussion. Andrew may have some input on this - maybe as a result
of my ideas below.

> So, once again, here is some interesting finding regarding the 
> limitations of the Realtek MDIO driver. Remember that it can only run 
> in either C22 or C45. This time it is about autonegotiation restart.
> 
> int phy_restart_aneg(struct phy_device *phydev) {
>   int ret;
> 
>   if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0))
>     ret = genphy_c45_restart_aneg(phydev);
>   else
>     ret = genphy_restart_aneg(phydev);
> 
>   return ret;
> }
> 
> I assume that BIT(0) means MDIO_DEVS_C22_PRESENT. As I understand it,
> this basically uses C22 commands for C45 PHYs if C22 support is detected.
> Currently, I have no reasonable explanation for this additional check.

There was discussion and delving into the spec when this was added.
See https://lore.kernel.org/all/20170601102327.GF27796@n2100.armlinux.org.uk/

(side note: google is now useless for finding that, searching for "hook
up clause 45 autonegotiation restart" returns very few results, none of
them with the full history. I've had to search my mailboxes for it, find
the message ID, and then look it up on lore. We now have various web
high-bandwidth crawlers that do not respect robots.txt to thank for this
as sites have ended up blocking all crawlers using stuff like Anubis
proof of work.)

It's been a long time, and so I've forgotten the details now, so all
there is to go on are the above emails.

You may notice that the initial version I submitted just used
phydev->is_c45.

> In our case, this function fails for C45 PHYs because the bus and PHY are 
> locked down to this single mode. It's stupid, of course, but that's how 
> it is. I see two options for fixing the issue:
> 
> 1) Mask the C22 presence in the bus for all PHYs when running in C45.
> 2) Drop the C22 condition check in the above function.

I guess we also need to make these kinds of tests conditional on
whether the bus supports C45 or not.

static bool mdiobus_supports_c22(struct mii_bus *bus)
{
	return bus->read && bus->write;
}

static bool mdiobus_supports_c45(struct mii_bus *bus)
{
	return bus->read_c45 && bus->write_c45;
}

This should be fine, because we already require MII bus drivers to only
populate these methods only when they're supported (see commit
fbfe97597c77 ("net: phy: Decide on C45 capabilities based on presence
of method").

	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0))

can then become:

	if (phydev->is_c45 &&
	    !(mdiobus_supports_c22(phydev->mdio.bus) &&
	      phydev->c45_ids.devices_in_package & MDIO_DEVS_C22PRESENT))

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: C22/C45 decision in phy_restart_aneg()

[Andrew Lunn]

> > In our case, this function fails for C45 PHYs because the bus and PHY are 
> > locked down to this single mode. It's stupid, of course, but that's how 
> > it is. I see two options for fixing the issue:
> > 
> > 1) Mask the C22 presence in the bus for all PHYs when running in C45.
> > 2) Drop the C22 condition check in the above function.
> 
> I guess we also need to make these kinds of tests conditional on
> whether the bus supports C45 or not.
> 
> static bool mdiobus_supports_c22(struct mii_bus *bus)
> {
> 	return bus->read && bus->write;
> }
> 
> static bool mdiobus_supports_c45(struct mii_bus *bus)
> {
> 	return bus->read_c45 && bus->write_c45;
> }

Something i've said in the past is that we have a mess regarding what
does phydev->is_c45 mean? And really, this is an extension of that
mess.

There are two separate C45 concepts which get mixed up in is_c45.

One concept is, does the PHY have the C45 register address space?  The
second concept is, how do you access the C45 address space? Via C45
bus transactions, or C45 over C22 bus transactions.

The extension here is, does the PHY have the C22 address space, and
what sort of bus transactions do you use to access the C22 address
space. It is however simplified, there is no C22 over C45 as far as i
know. So it is actually, can you do C22 bus transactions or not?

I've tried in the past to get developers to sort out this mess, but it
never got very far. Maybe we can use this as an opportunity to address
a little bit of the problem?

> This should be fine, because we already require MII bus drivers to only
> populate these methods only when they're supported (see commit
> fbfe97597c77 ("net: phy: Decide on C45 capabilities based on presence
> of method").
> 
> 	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0))
> 
> can then become:
> 
> 	if (phydev->is_c45 &&
> 	    !(mdiobus_supports_c22(phydev->mdio.bus) &&
> 	      phydev->c45_ids.devices_in_package & MDIO_DEVS_C22PRESENT))

So what we are asking here is, can we do C22 bus transfers, and does
the PHY have the C22 address space? And looking in the code, there are
at least three places this is needed:

int phy_restart_aneg(struct phy_device *phydev)
{
        int ret;

        if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
                ret = genphy_c45_restart_aneg(phydev);
        else
                ret = genphy_restart_aneg(phydev);

        return ret;
}

int phy_aneg_done(struct phy_device *phydev)
{
        if (phydev->drv && phydev->drv->aneg_done)
                return phydev->drv->aneg_done(phydev);
        else if (phydev->is_c45)
                return genphy_c45_aneg_done(phydev);
        else
                return genphy_aneg_done(phydev);
}

This i _think_ is currently broken, i would expect the same condition
to apply.

int phy_config_aneg(struct phy_device *phydev)
{
        if (phydev->drv->config_aneg)
                return phydev->drv->config_aneg(phydev);

        /* Clause 45 PHYs that don't implement Clause 22 registers are not
         * allowed to call genphy_config_aneg()
         */
        if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
                return genphy_c45_config_aneg(phydev);

        return genphy_config_aneg(phydev);
}

So maybe we should add new members to phydev?

@bus_has_c22: The bus can perform C22 transactions
@phy_has_c22_regs: The PHY has the C22 register address space.

During probe, we set bus_has_c22 based on Russells
mdiobus_supports_c22() above. phy_has_c22_regs should be set if we
discover the PHY by C22 ID registers, or
phydev->c45_ids.devices_in_package indicates the PHY has C22
registers. We also need to handle the PHY is instantiated via ID
values in DT. After the PHY has probed, we can read C22 registers 2
and 3, and see if the ID look valid, i.e. mostly not F, same as we do
in get_phy_c22_id().

The condition then becomes:

        if (phydev->is_c45 && !(phydev->bus_has_c22 && phydev->phy_has_c22_regs))

Then maybe some time in the future, the is_c45 will get replaced with
something similar?

	Andrew