[PATCH net v4 0/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[PATCH net v4 0/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[Horatiu Vultur]

The first patch will update the PHYs VSC8584, VSC8582, VSC8575 and VSC856X
to use PHY_ID_MATCH_MODEL because only rev B exists for these PHYs.
But for the PHYs VSC8574 and VSC8572 exists rev A, B, C, D and E.
This is just a preparation for the second patch to allow the VSC8574 and
VSC8572 to use the function vsc8584_probe().

We want to use vsc8584_probe() for VSC8574 and VSC8572 because this
function does the correct PTP initialization. This change is in the second
patch.

v3->v4:
- rebase on net-main
v2->v3:
- split into a series, first patch will update VSC8584, VSC8582, VSC8575
  and VSC856X to use PHY_ID_MATCH_MODEL, second patch will do the actual
  fix
- improve commit message and start use vsc8584_probe()
v1->v2:
- rename vsc8574_probe to vsc8552_probe and introduce a new probe
  function called vsc8574_probe and make sure that vsc8504 and vsc8552
  will use vsc8552_probe.

Horatiu Vultur (2):
  phy: mscc: Use PHY_ID_MATCH_MODEL for VSC8584, VSC8582, VSC8575,
    VSC856X
  phy: mscc: Fix PTP for VSC8574 and VSC8572

 drivers/net/phy/mscc/mscc.h      |  8 ++++----
 drivers/net/phy/mscc/mscc_main.c | 29 +++++++----------------------
 2 files changed, 11 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH net v4 1/2] phy: mscc: Use PHY_ID_MATCH_MODEL for VSC8584, VSC8582, VSC8575, VSC856X

[Horatiu Vultur]

As the PHYs VSC8584, VSC8582, VSC8575 and VSC856X exists only as rev B,
we can use PHY_ID_MATCH_MODEL to match exactly on revision B of the PHY.
Because of this change then there is not need the check if it is a
different revision than rev B in the function vsc8584_probe() as we
already know that this will never happen.
These changes are a preparation for the next patch because in that patch
we will make the PHYs VSC8574 and VSC8572 to use vsc8584_probe() and
these PHYs have multiple revision.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/phy/mscc/mscc.h      |  8 ++++----
 drivers/net/phy/mscc/mscc_main.c | 23 ++++-------------------
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 2d8eca54c40a2..2eef5956b9cc5 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -289,12 +289,12 @@ enum rgmii_clock_delay {
 #define PHY_ID_VSC8540			  0x00070760
 #define PHY_ID_VSC8541			  0x00070770
 #define PHY_ID_VSC8552			  0x000704e0
-#define PHY_ID_VSC856X			  0x000707e0
+#define PHY_ID_VSC856X			  0x000707e1
 #define PHY_ID_VSC8572			  0x000704d0
 #define PHY_ID_VSC8574			  0x000704a0
-#define PHY_ID_VSC8575			  0x000707d0
-#define PHY_ID_VSC8582			  0x000707b0
-#define PHY_ID_VSC8584			  0x000707c0
+#define PHY_ID_VSC8575			  0x000707d1
+#define PHY_ID_VSC8582			  0x000707b1
+#define PHY_ID_VSC8584			  0x000707c1
 #define PHY_VENDOR_MSCC			0x00070400
 
 #define MSCC_VDDMAC_1500		  1500
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index ef0ef1570d392..d05f6ed052ad0 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1724,12 +1724,6 @@ static int vsc8584_config_init(struct phy_device *phydev)
 	 * in this pre-init function.
 	 */
 	if (phy_package_init_once(phydev)) {
-		/* The following switch statement assumes that the lowest
-		 * nibble of the phy_id_mask is always 0. This works because
-		 * the lowest nibble of the PHY_ID's below are also 0.
-		 */
-		WARN_ON(phydev->drv->phy_id_mask & 0xf);
-
 		switch (phydev->phy_id & phydev->drv->phy_id_mask) {
 		case PHY_ID_VSC8504:
 		case PHY_ID_VSC8552:
@@ -2290,11 +2284,6 @@ static int vsc8584_probe(struct phy_device *phydev)
 	   VSC8531_DUPLEX_COLLISION};
 	int ret;
 
-	if ((phydev->phy_id & MSCC_DEV_REV_MASK) != VSC8584_REVB) {
-		dev_err(&phydev->mdio.dev, "Only VSC8584 revB is supported.\n");
-		return -ENOTSUPP;
-	}
-
 	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
 	if (!vsc8531)
 		return -ENOMEM;
@@ -2587,9 +2576,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_inband  = vsc85xx_config_inband,
 },
 {
-	.phy_id		= PHY_ID_VSC856X,
+	PHY_ID_MATCH_MODEL(PHY_ID_VSC856X),
 	.name		= "Microsemi GE VSC856X SyncE",
-	.phy_id_mask	= 0xfffffff0,
 	/* PHY_GBIT_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
 	.config_init    = &vsc8584_config_init,
@@ -2667,9 +2655,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_inband  = vsc85xx_config_inband,
 },
 {
-	.phy_id		= PHY_ID_VSC8575,
+	PHY_ID_MATCH_MODEL(PHY_ID_VSC8575),
 	.name		= "Microsemi GE VSC8575 SyncE",
-	.phy_id_mask	= 0xfffffff0,
 	/* PHY_GBIT_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
 	.config_init    = &vsc8584_config_init,
@@ -2693,9 +2680,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_inband  = vsc85xx_config_inband,
 },
 {
-	.phy_id		= PHY_ID_VSC8582,
+	PHY_ID_MATCH_MODEL(PHY_ID_VSC8582),
 	.name		= "Microsemi GE VSC8582 SyncE",
-	.phy_id_mask	= 0xfffffff0,
 	/* PHY_GBIT_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
 	.config_init    = &vsc8584_config_init,
@@ -2719,9 +2705,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_inband  = vsc85xx_config_inband,
 },
 {
-	.phy_id		= PHY_ID_VSC8584,
+	PHY_ID_MATCH_MODEL(PHY_ID_VSC8584),
 	.name		= "Microsemi GE VSC8584 SyncE",
-	.phy_id_mask	= 0xfffffff0,
 	/* PHY_GBIT_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
 	.config_init    = &vsc8584_config_init,
-- 
2.34.1

[PATCH net v4 2/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[Horatiu Vultur]

The PTP initialization is two-step. First part are the function
vsc8584_ptp_probe_once() and vsc8584_ptp_probe() at probe time which
initialize the locks, queues, creates the PTP device. The second part is
the function vsc8584_ptp_init() at config_init() time which initialize
PTP in the HW.

For VSC8574 and VSC8572, the PTP initialization is incomplete. It is
missing the first part but it makes the second part. Meaning that the
ptp_clock_register() is never called.

There is no crash without the first part when enabling PTP but this is
unexpected because some PHys have PTP functionality exposed by the
driver and some don't even though they share the same PTP clock PTP.

Fixes: 774626fa440e ("net: phy: mscc: Add PTP support for 2 more VSC PHYs")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/phy/mscc/mscc_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index d05f6ed052ad0..90b62b8fd4af6 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -2613,7 +2613,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.remove		= &vsc85xx_remove,
-	.probe		= &vsc8574_probe,
+	.probe		= &vsc8584_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
@@ -2636,12 +2636,12 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_aneg    = &vsc85xx_config_aneg,
 	.aneg_done	= &genphy_aneg_done,
 	.read_status	= &vsc85xx_read_status,
-	.handle_interrupt = vsc85xx_handle_interrupt,
+	.handle_interrupt = vsc8584_handle_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.remove		= &vsc85xx_remove,
-	.probe		= &vsc8574_probe,
+	.probe		= &vsc8584_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
-- 
2.34.1

Re: [PATCH net v4 2/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[Maxime Chevallier]

Hi Horatiu,

On 17/10/2025 08:48, Horatiu Vultur wrote:
> The PTP initialization is two-step. First part are the function
> vsc8584_ptp_probe_once() and vsc8584_ptp_probe() at probe time which
> initialize the locks, queues, creates the PTP device. The second part is
> the function vsc8584_ptp_init() at config_init() time which initialize
> PTP in the HW.
> 
> For VSC8574 and VSC8572, the PTP initialization is incomplete. It is
> missing the first part but it makes the second part. Meaning that the
> ptp_clock_register() is never called.
> 
> There is no crash without the first part when enabling PTP but this is
> unexpected because some PHys have PTP functionality exposed by the
> driver and some don't even though they share the same PTP clock PTP.
> 
> Fixes: 774626fa440e ("net: phy: mscc: Add PTP support for 2 more VSC PHYs")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Re: [PATCH net v4 1/2] phy: mscc: Use PHY_ID_MATCH_MODEL for VSC8584, VSC8582, VSC8575, VSC856X

[Maxime Chevallier]

Hi Horatiu,

On 17/10/2025 08:48, Horatiu Vultur wrote:
> As the PHYs VSC8584, VSC8582, VSC8575 and VSC856X exists only as rev B,
> we can use PHY_ID_MATCH_MODEL to match exactly on revision B of the PHY.
> Because of this change then there is not need the check if it is a
> different revision than rev B in the function vsc8584_probe() as we
> already know that this will never happen.
> These changes are a preparation for the next patch because in that patch
> we will make the PHYs VSC8574 and VSC8572 to use vsc8584_probe() and
> these PHYs have multiple revision.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Re: [PATCH net v4 2/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[Jakub Kicinski]

On Fri, 17 Oct 2025 08:48:19 +0200 Horatiu Vultur wrote:
> For VSC8574 and VSC8572, the PTP initialization is incomplete. It is
> missing the first part but it makes the second part. Meaning that the
> ptp_clock_register() is never called.
> 
> There is no crash without the first part when enabling PTP but this is
> unexpected because some PHys have PTP functionality exposed by the
> driver and some don't even though they share the same PTP clock PTP.

I'm tempted to queue this to net-next, sounds like a "never worked 
in an obvious way" case.  I'd appreciate a second opinion.. Andrew?

Re: [PATCH net v4 2/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[Paolo Abeni]

On 10/21/25 1:53 AM, Jakub Kicinski wrote:
> On Fri, 17 Oct 2025 08:48:19 +0200 Horatiu Vultur wrote:
>> For VSC8574 and VSC8572, the PTP initialization is incomplete. It is
>> missing the first part but it makes the second part. Meaning that the
>> ptp_clock_register() is never called.
>>
>> There is no crash without the first part when enabling PTP but this is
>> unexpected because some PHys have PTP functionality exposed by the
>> driver and some don't even though they share the same PTP clock PTP.
> 
> I'm tempted to queue this to net-next, sounds like a "never worked 
> in an obvious way" case.  I'd appreciate a second opinion.. Andrew?

FTR, I agree with the above, as (out of sheer ignorance) I think/fear
the first patch can potentially cause regressions.

/P

Re: [PATCH net v4 1/2] phy: mscc: Use PHY_ID_MATCH_MODEL for VSC8584, VSC8582, VSC8575, VSC856X

[Russell King (Oracle)]

On Fri, Oct 17, 2025 at 08:48:18AM +0200, Horatiu Vultur wrote:
> As the PHYs VSC8584, VSC8582, VSC8575 and VSC856X exists only as rev B,
> we can use PHY_ID_MATCH_MODEL to match exactly on revision B of the PHY.

I don't follow this. PHY_ID_MATCH_MODEL() uses a mask of bits 31:4,
omitting the revision field. So that is equivalent to a .phy_id_mask
of 0xfffffff0, which is what the code already uses.

> Because of this change then there is not need the check if it is a
> different revision than rev B in the function vsc8584_probe() as we
> already know that this will never happen.

Since bits 3:0 are masked out, this statement seems to be false.

> @@ -2587,9 +2576,8 @@ static struct phy_driver vsc85xx_driver[] = {
>  	.config_inband  = vsc85xx_config_inband,
>  },
>  {
> -	.phy_id		= PHY_ID_VSC856X,
> +	PHY_ID_MATCH_MODEL(PHY_ID_VSC856X),
>  	.name		= "Microsemi GE VSC856X SyncE",
> -	.phy_id_mask	= 0xfffffff0,
>  	/* PHY_GBIT_FEATURES */
>  	.soft_reset	= &genphy_soft_reset,
>  	.config_init    = &vsc8584_config_init,
> @@ -2667,9 +2655,8 @@ static struct phy_driver vsc85xx_driver[] = {
>  	.config_inband  = vsc85xx_config_inband,
>  },
>  {
> -	.phy_id		= PHY_ID_VSC8575,
> +	PHY_ID_MATCH_MODEL(PHY_ID_VSC8575),
>  	.name		= "Microsemi GE VSC8575 SyncE",
> -	.phy_id_mask	= 0xfffffff0,
>  	/* PHY_GBIT_FEATURES */
>  	.soft_reset	= &genphy_soft_reset,
>  	.config_init    = &vsc8584_config_init,
> @@ -2693,9 +2680,8 @@ static struct phy_driver vsc85xx_driver[] = {
>  	.config_inband  = vsc85xx_config_inband,
>  },
>  {
> -	.phy_id		= PHY_ID_VSC8582,
> +	PHY_ID_MATCH_MODEL(PHY_ID_VSC8582),
>  	.name		= "Microsemi GE VSC8582 SyncE",
> -	.phy_id_mask	= 0xfffffff0,
>  	/* PHY_GBIT_FEATURES */
>  	.soft_reset	= &genphy_soft_reset,
>  	.config_init    = &vsc8584_config_init,
> @@ -2719,9 +2705,8 @@ static struct phy_driver vsc85xx_driver[] = {
>  	.config_inband  = vsc85xx_config_inband,
>  },
>  {
> -	.phy_id		= PHY_ID_VSC8584,
> +	PHY_ID_MATCH_MODEL(PHY_ID_VSC8584),
>  	.name		= "Microsemi GE VSC8584 SyncE",
> -	.phy_id_mask	= 0xfffffff0,
>  	/* PHY_GBIT_FEATURES */
>  	.soft_reset	= &genphy_soft_reset,
>  	.config_init    = &vsc8584_config_init,

Due to what I've said above, the above part of the patch is a cleanup,
and functionally is a no-op.

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

Re: [PATCH net v4 1/2] phy: mscc: Use PHY_ID_MATCH_MODEL for VSC8584, VSC8582, VSC8575, VSC856X

[Jakub Kicinski]

On Fri, 17 Oct 2025 08:48:18 +0200 Horatiu Vultur wrote:
> -	if ((phydev->phy_id & MSCC_DEV_REV_MASK) != VSC8584_REVB) {

I think MSCC_DEV_REV_MASK is no longer used after this patch?

Re: [PATCH net v4 2/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[Jakub Kicinski]

On Tue, 21 Oct 2025 11:07:20 +0200 Paolo Abeni wrote:
> On 10/21/25 1:53 AM, Jakub Kicinski wrote:
> > On Fri, 17 Oct 2025 08:48:19 +0200 Horatiu Vultur wrote:  
> >> For VSC8574 and VSC8572, the PTP initialization is incomplete. It is
> >> missing the first part but it makes the second part. Meaning that the
> >> ptp_clock_register() is never called.
> >>
> >> There is no crash without the first part when enabling PTP but this is
> >> unexpected because some PHys have PTP functionality exposed by the
> >> driver and some don't even though they share the same PTP clock PTP.  
> > 
> > I'm tempted to queue this to net-next, sounds like a "never worked 
> > in an obvious way" case.  I'd appreciate a second opinion.. Andrew?  
> 
> FTR, I agree with the above, as (out of sheer ignorance) I think/fear
> the first patch can potentially cause regressions.

Thanks, let's rephrase the commits message on patch 1 (per Russell's
comments) and get this reposted for net-next (without the Fixes tag).

Re: [PATCH net v4 1/2] phy: mscc: Use PHY_ID_MATCH_MODEL for VSC8584, VSC8582, VSC8575, VSC856X

[Horatiu Vultur]

The 10/21/2025 10:09, Russell King (Oracle) wrote:

Hi,

> 
> On Fri, Oct 17, 2025 at 08:48:18AM +0200, Horatiu Vultur wrote:
> > As the PHYs VSC8584, VSC8582, VSC8575 and VSC856X exists only as rev B,
> > we can use PHY_ID_MATCH_MODEL to match exactly on revision B of the PHY.
> 
> I don't follow this. PHY_ID_MATCH_MODEL() uses a mask of bits 31:4,
> omitting the revision field. So that is equivalent to a .phy_id_mask
> of 0xfffffff0, which is what the code already uses.

I totally understand why you don't understand this as this is I made big
mistake!
I was supposed to use PHY_ID_MATCH_EXACT instead of PHY_ID_MATCH_MODEL.

> 
> > Because of this change then there is not need the check if it is a
> > different revision than rev B in the function vsc8584_probe() as we
> > already know that this will never happen.
> 
> Since bits 3:0 are masked out, this statement seems to be false.
> 
> > @@ -2587,9 +2576,8 @@ static struct phy_driver vsc85xx_driver[] = {
> >       .config_inband  = vsc85xx_config_inband,
> >  },
> >  {
> > -     .phy_id         = PHY_ID_VSC856X,
> > +     PHY_ID_MATCH_MODEL(PHY_ID_VSC856X),
> >       .name           = "Microsemi GE VSC856X SyncE",
> > -     .phy_id_mask    = 0xfffffff0,
> >       /* PHY_GBIT_FEATURES */
> >       .soft_reset     = &genphy_soft_reset,
> >       .config_init    = &vsc8584_config_init,
> > @@ -2667,9 +2655,8 @@ static struct phy_driver vsc85xx_driver[] = {
> >       .config_inband  = vsc85xx_config_inband,
> >  },
> >  {
> > -     .phy_id         = PHY_ID_VSC8575,
> > +     PHY_ID_MATCH_MODEL(PHY_ID_VSC8575),
> >       .name           = "Microsemi GE VSC8575 SyncE",
> > -     .phy_id_mask    = 0xfffffff0,
> >       /* PHY_GBIT_FEATURES */
> >       .soft_reset     = &genphy_soft_reset,
> >       .config_init    = &vsc8584_config_init,
> > @@ -2693,9 +2680,8 @@ static struct phy_driver vsc85xx_driver[] = {
> >       .config_inband  = vsc85xx_config_inband,
> >  },
> >  {
> > -     .phy_id         = PHY_ID_VSC8582,
> > +     PHY_ID_MATCH_MODEL(PHY_ID_VSC8582),
> >       .name           = "Microsemi GE VSC8582 SyncE",
> > -     .phy_id_mask    = 0xfffffff0,
> >       /* PHY_GBIT_FEATURES */
> >       .soft_reset     = &genphy_soft_reset,
> >       .config_init    = &vsc8584_config_init,
> > @@ -2719,9 +2705,8 @@ static struct phy_driver vsc85xx_driver[] = {
> >       .config_inband  = vsc85xx_config_inband,
> >  },
> >  {
> > -     .phy_id         = PHY_ID_VSC8584,
> > +     PHY_ID_MATCH_MODEL(PHY_ID_VSC8584),
> >       .name           = "Microsemi GE VSC8584 SyncE",
> > -     .phy_id_mask    = 0xfffffff0,
> >       /* PHY_GBIT_FEATURES */
> >       .soft_reset     = &genphy_soft_reset,
> >       .config_init    = &vsc8584_config_init,
> 
> Due to what I've said above, the above part of the patch is a cleanup,
> and functionally is a no-op.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

-- 
/Horatiu