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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Sep 26, 2025 at 12:52:03PM +0300, Vladimir Oltean wrote:
> On Fri, Sep 26, 2025 at 10:46:47AM +0100, Russell King (Oracle) 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;
> > > 	}
> > > }
> > 
> > Please, no, not like this. Have a look how the driver already compares
> > PHY IDs in the rest of the code.
> > 
> > When a PHY driver is matched, the PHY ID is compared using the
> > .phy_id and .phy_id_mask members of the phy_driver structure.
> > 
> > The .phy_id is normally stuff like PHY_ID_VSC8572 and PHY_ID_VSC8574.
> > 
> > When the driver is probed, phydev->drv is set to point at the
> > appropriate phy_driver structure. Thus, the tests can be simplified
> > to merely looking at phydev->drv->phy_id:
> > 
> > 	if (phydev->drv->phy_id != PHY_ID_VSC8572 &&
> > 	    phydev->drv->phy_id != PHY_ID_VSC8574 &&
> > 	    (phydev->phy_id & MSCC_DEV_REV_MASK) != VSC8584_REVB) {
> > ...
> > 
> > Alternatively, please look at the phy_id*() and phydev_id_compare()
> > families of functions.
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 
> The phydev->phy_id comparisons are problematic with clause 45 PHYs where
> this field is zero, but with clause 22 they should technically work.
> It's good to know coming from a phylib maintainer that it's preferable
> to use phydev->drv->phy_id everywhere (I also wanted to comment on this
> aspect, but because technically nothing is broken, I didn't).

Yes indeed, which is another reason to use phydev->drv->* as these
get matched against the C22 and C45 IDs during PHY probe.

If we wish to get more clever (in terms of wanthing to know the
revision without knowing how the driver was matched) we could store
the matched ID in phydev, as read from hardware. This would also get
around the problem that where the ID is provided in the DT compatible,
we would have the real hardware-read ID to check things like the
revision against, rather than a fixed value in DT.

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