Re: [PATCH net v2] phy: mscc: Fix PTP for vsc8574 and VSC8572

[Horatiu Vultur <horatiu.vultur@microchip.com>]

The 09/23/2025 09:19, Horatiu Vultur wrote:

Hi,

> The 09/22/2025 16:28, Vladimir Oltean wrote:
> > 
> > On Mon, Sep 22, 2025 at 02:33:01PM +0200, Horatiu Vultur wrote:
> > > Thanks for the advice.
> > > What about to make the PHY_ID_VSC8572 and PHY_ID_VSC8574 to use
> > > vsc8584_probe() and then in this function just have this check:
> > >
> > > ---
> > > if ((phydev->phy_id & 0xfffffff0) != PHY_ID_VSC8572 &&
> > >     (phydev->phy_id & 0xfffffff0) != PHY_ID_VSC8574) {
> > >       if ((phydev->phy_id & MSCC_DEV_REV_MASK) != VSC8584_REVB) {
> > >               dev_err(&phydev->mdio.dev, "Only VSC8584 revB is supported.\n");
> > >               return -ENOTSUPP;
> > >       }
> > > }
> > 
> > Personally, I think you are making the code harder to understand what
> > PHYs the test is referring to, and why it exists in the first place.
> > 
> > Ideally this test would have not existed. Instead of the open-coded
> > phy_id and phy_id_mask fields from the struct phy_driver array entries,
> > one could have used PHY_ID_MATCH_MODEL() for those entries where the
> > bits 3:0 of the PHY ID do not matter, and PHY_ID_MATCH_EXACT() where
> > they do. Instead of failing the probe, just not match the device with
> > this driver and let the system handle it some other way (Generic PHY).
> 
> Yes, I can see your point. That would be a nicer fix.
> 
> > 
> > I'm not sure if this is intended or not, but the combined effect of:
> > - commit a5afc1678044 ("net: phy: mscc: add support for VSC8584 PHY")
> > - commit 75a1ccfe6c72 ("mscc.c: Add support for additional VSC PHYs")
> > 
> > is that for VSC856X, VSC8575, VSC8582, VSC8584, the driver will only
> > probe on Rev B silicon, and fail otherwise. Initially, the revision test
> > was only there for VSC8584, and it transferred to the others by virtue
> > of reusing the same vsc8584_probe() function. I don't see signs that
> > this was 100% intentional. I say this because when probing e.g. on
> > VSC8575 revA, the kernel will print "Only VSC8584 revB is supported."
> > which looks more like an error than someone's actual intention.
> > 
> > By excluding VSC8574 and VSC8572 from the above revision test, it feels
> > like a double workaround rather than using the conventional PHY ID match
> > helpers as intended.
> > 
> > As a Microchip employee, maybe you have access to some info regarding
> > whether the limitations mentioned by Quentin Schulz for VSC8584 revA
> > are valid for all the other PHYs for which they are currently imposed.
> > What makes VSC8574/VSC8572 unlike the others in this regard?
> 
> Let me start by asking my colleagues and figure out which revisions were
> produced for which PHYs.
> Thanks for the advice.

I have been asking around about these revisions of the PHYs and what is
available:
vsc856x - only rev B exists
vsc8575 - only rev B exists
vsc8582 - only rev B exists
vsc8584 - only rev B exists
vsc8574 - rev A,B,C,D,E exists
vsc8572 - rev A,B,C,D,E exists

For vsc856x, vsc8575, vsc8582, vsc8584 the lower 4 bits in register 3
will have a value of 1.
For vsc8574 and vsc8572 the lower 4 bits in register 3 will have a value
of 0 for rev A, 1 for rev B and C, 2 for D and E.

Based on this information, I think both commits a5afc1678044 and
75a1ccfe6c72 are correct regarding the revision check.

So, now to be able to fix the PTP for vsc8574 and vsc8572, I can do the
following:
- start to use PHY_ID_MATCH_MODEL for vsc856x, vsc8575, vsc8582, vsc8584
- because of this change I will need to remove also the WARN_ON() in the
  function vsc8584_config_init()
- then I can drop the check for revision in vsc8584_probe()
- then I can make vsc8574 and vsc8572 to use vsc8584_probe()

What do you think about this?

> 
> > 
> > It looks like the review comments to clean things up are getting bigger.
> > I'm not sure this is all adequate for 'net' any longer.
> > On the other hand, you said PTP never worked for VSC8574/VSC8572,
> > without any crash, it was just not enabled. Maybe this can all be
> > reconsidered as new functionality for net-next, and there we have more
> > space for shuffling things around?
> 
> -- 
> /Horatiu

-- 
/Horatiu