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

[PATCH net v3 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.

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 v3 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 v3 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 v3 2/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[Russell King (Oracle)]

On Mon, Sep 29, 2025 at 11:13:02AM +0200, 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.

So, to summarise, you register the PTP clock at probe time? At that
point, you get a /dev/ptp* device. Is that device functional without
the config_init() initialisation?

I would hope it is, because one of the fundamental concepts in kernel
programming is... don't publish devices to userspace that are not
completely ready for operation.

Conversely, don't tear down a published device without first
unpublishing it from userspace.

Thanks.

-- 
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 v3 2/2] phy: mscc: Fix PTP for VSC8574 and VSC8572

[Horatiu Vultur]

The 09/29/2025 14:33, Russell King (Oracle) wrote:

Hi Russell,

> 
> On Mon, Sep 29, 2025 at 11:13:02AM +0200, 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.
> 
> So, to summarise, you register the PTP clock at probe time? At that
> point, you get a /dev/ptp* device. Is that device functional without
> the config_init() initialisation?

That is correct. I register the PTP clock at probe time.
It is not fully functional, for example calling "phc_ctl /dev/ptp0 get", I
always get "clock time is 0.000000000 or Thu Jan  1 00:00:00 1970"

# phc_ctl /dev/ptp0 get
phc_ctl[414.412]: clock time is 0.000000000 or Thu Jan  1 00:00:00 1970

# phc_ctl /dev/ptp0 get
phc_ctl[418.453]: clock time is 0.000000000 or Thu Jan  1 00:00:00 1970

Then after setting up, I can see the time increasing:

# phc_ctl /dev/ptp0 get
phc_ctl[511.841]: clock time is 2.232610928 or Thu Jan  1 00:00:02 1970

# phc_ctl /dev/ptp0 get
phc_ctl[515.113]: clock time is 5.504853240 or Thu Jan  1 00:00:05 1970

> 
> I would hope it is, because one of the fundamental concepts in kernel
> programming is... don't publish devices to userspace that are not
> completely ready for operation.
> 
> Conversely, don't tear down a published device without first
> unpublishing it from userspace.

Thanks for reminding me about this. Unfortunately, the same behaviour is for
all the devices that this driver is covering. So, then maybe I would
need to create a patch in the future to change this to have the correct
behaviour.

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

-- 
/Horatiu

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

[Jakub Kicinski]

On Mon, 29 Sep 2025 11:13:00 +0200 Horatiu Vultur wrote:
> 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.

This doesn't apply cleanly to net. If it was generated against net-next
- perhaps wait a couple of days until trees converge before reposting?
-- 
pw-bot: cr

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

[Horatiu Vultur]

The 09/29/2025 18:47, Jakub Kicinski wrote:
> 
> On Mon, 29 Sep 2025 11:13:00 +0200 Horatiu Vultur wrote:
> > 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.
> 
> This doesn't apply cleanly to net. If it was generated against net-next
> - perhaps wait a couple of days until trees converge before reposting?

Yeah, I have generated this patch series on net-next instead of net.
Yes, I can wait before reposting.

> --
> pw-bot: cr

-- 
/Horatiu