drivers/net/phy/mscc/mscc_main.c | 51 +++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 4 deletions(-)
When trying to enable PTP on vsc8574 and vsc8572 it is not working even
if the function vsc8584_ptp_init it says that it has support for PHY
timestamping. It is not working because there is no PTP device.
So, to fix this make sure to create a PTP device also for this PHYs as
they have the same PTP IP as the other vsc PHYs.
Fixes: 774626fa440e ("net: phy: mscc: Add PTP support for 2 more VSC PHYs")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
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.
---
drivers/net/phy/mscc/mscc_main.c | 51 +++++++++++++++++++++++++++++---
1 file changed, 47 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index ef0ef1570d392..e9a8dc6096868 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -2253,7 +2253,7 @@ static int vsc8514_probe(struct phy_device *phydev)
return vsc85xx_dt_led_modes_get(phydev, default_mode);
}
-static int vsc8574_probe(struct phy_device *phydev)
+static int vsc8552_probe(struct phy_device *phydev)
{
struct vsc8531_private *vsc8531;
u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
@@ -2282,6 +2282,49 @@ static int vsc8574_probe(struct phy_device *phydev)
return vsc85xx_dt_led_modes_get(phydev, default_mode);
}
+static int vsc8574_probe(struct phy_device *phydev)
+{
+ struct vsc8531_private *vsc8531;
+ u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
+ VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
+ VSC8531_DUPLEX_COLLISION};
+ int ret;
+
+ vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
+ if (!vsc8531)
+ return -ENOMEM;
+
+ phydev->priv = vsc8531;
+
+ vsc8584_get_base_addr(phydev);
+ ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
+ vsc8531->base_addr,
+ sizeof(struct vsc85xx_shared_private));
+ if (ret)
+ return ret;
+
+ vsc8531->nleds = 4;
+ vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
+ vsc8531->hw_stats = vsc8584_hw_stats;
+ vsc8531->nstats = ARRAY_SIZE(vsc8584_hw_stats);
+ vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
+ sizeof(u64), GFP_KERNEL);
+ if (!vsc8531->stats)
+ return -ENOMEM;
+
+ if (phy_package_probe_once(phydev)) {
+ ret = vsc8584_ptp_probe_once(phydev);
+ if (ret)
+ return ret;
+ }
+
+ ret = vsc8584_ptp_probe(phydev);
+ if (ret)
+ return ret;
+
+ return vsc85xx_dt_led_modes_get(phydev, default_mode);
+}
+
static int vsc8584_probe(struct phy_device *phydev)
{
struct vsc8531_private *vsc8531;
@@ -2426,7 +2469,7 @@ static struct phy_driver vsc85xx_driver[] = {
.config_intr = &vsc85xx_config_intr,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
- .probe = &vsc8574_probe,
+ .probe = &vsc8552_probe,
.set_wol = &vsc85xx_wol_set,
.get_wol = &vsc85xx_wol_get,
.get_tunable = &vsc85xx_get_tunable,
@@ -2573,7 +2616,7 @@ static struct phy_driver vsc85xx_driver[] = {
.config_intr = &vsc85xx_config_intr,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
- .probe = &vsc8574_probe,
+ .probe = &vsc8552_probe,
.set_wol = &vsc85xx_wol_set,
.get_wol = &vsc85xx_wol_get,
.get_tunable = &vsc85xx_get_tunable,
@@ -2648,7 +2691,7 @@ 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,
--
2.34.1
On Wed, 17 Sep 2025 13:33:16 +0200 Horatiu Vultur wrote: > When trying to enable PTP on vsc8574 and vsc8572 it is not working even > if the function vsc8584_ptp_init it says that it has support for PHY > timestamping. It is not working because there is no PTP device. > So, to fix this make sure to create a PTP device also for this PHYs as > they have the same PTP IP as the other vsc PHYs. May be useful to proof read your commit message, or run it thru a grammar checker. Copy & paste into a Google Doc would be enough.. Regarding the patch the new function looks like a spitting image of vsc8584_probe(), minus the revision check. -- pw-bot: cr
On Thu, Sep 18, 2025 at 04:09:42PM -0700, Jakub Kicinski wrote: > On Wed, 17 Sep 2025 13:33:16 +0200 Horatiu Vultur wrote: > > When trying to enable PTP on vsc8574 and vsc8572 it is not working even > > if the function vsc8584_ptp_init it says that it has support for PHY > > timestamping. It is not working because there is no PTP device. > > So, to fix this make sure to create a PTP device also for this PHYs as > > they have the same PTP IP as the other vsc PHYs. > > May be useful to proof read your commit message, or run it thru > a grammar checker. Copy & paste into a Google Doc would be enough.. I agree, and I did not understand the problem from the commit message. I would suggest something like below (maybe not identical). The PTP initialization is two-step: first we have vsc8584_ptp_probe_once() / vsc8584_ptp_probe() at probe() time, then we have vsc8584_ptp_init() at config_init() time. For VSC8574 and VSC8572, the PTP initialization is incomplete. We are making the second step without having previously made the first one. This means, for example, that ptp_clock_register() is never called. Nothing crashes as a result of this, but it is unexpected that some PHY generations have PTP functionality exposed by the driver and some do not, even though they share the same PTP clock IP. > Regarding the patch the new function looks like a spitting image > of vsc8584_probe(), minus the revision check. > -- > pw-bot: cr Also, even without this patch, vsc8574_probe() and vsc8584_probe() are structurally very similar and could use some consolidation. Would it make sense to create a static int vsc8531_probe_common(struct phy_device *phydev, bool ptp) and call it from multiple wrappers? The VSC8584_REVB check can go in the vsc8584_probe() wrapper. The "size_t priv_size" argument of devm_phy_package_join() can be set based on the "bool ptp" argument, because struct vsc85xx_shared_private is used only in PTP code. You can make a preparatory change in 'net' patch sets, even without a Fixes: tag, if you clearly explain what it's for.
The 09/22/2025 15:15, Vladimir Oltean wrote: Hi, > > On Thu, Sep 18, 2025 at 04:09:42PM -0700, Jakub Kicinski wrote: > > On Wed, 17 Sep 2025 13:33:16 +0200 Horatiu Vultur wrote: > > > When trying to enable PTP on vsc8574 and vsc8572 it is not working even > > > if the function vsc8584_ptp_init it says that it has support for PHY > > > timestamping. It is not working because there is no PTP device. > > > So, to fix this make sure to create a PTP device also for this PHYs as > > > they have the same PTP IP as the other vsc PHYs. > > > > May be useful to proof read your commit message, or run it thru > > a grammar checker. Copy & paste into a Google Doc would be enough.. > > I agree, and I did not understand the problem from the commit message. > > I would suggest something like below (maybe not identical). > > The PTP initialization is two-step: first we have vsc8584_ptp_probe_once() / > vsc8584_ptp_probe() at probe() time, then we have vsc8584_ptp_init() at > config_init() time. > > For VSC8574 and VSC8572, the PTP initialization is incomplete. We are > making the second step without having previously made the first one. > This means, for example, that ptp_clock_register() is never called. > > Nothing crashes as a result of this, but it is unexpected that some PHY > generations have PTP functionality exposed by the driver and some do > not, even though they share the same PTP clock IP. I agree, I need to be more carefull and more clear in the commit messages. Thanks for the great example. > > > Regarding the patch the new function looks like a spitting image > > of vsc8584_probe(), minus the revision check. > > -- > > pw-bot: cr > > Also, even without this patch, vsc8574_probe() and vsc8584_probe() are > structurally very similar and could use some consolidation. > > Would it make sense to create a static int vsc8531_probe_common(struct > phy_device *phydev, bool ptp) and call it from multiple wrappers? The > VSC8584_REVB check can go in the vsc8584_probe() wrapper. The "size_t > priv_size" argument of devm_phy_package_join() can be set based on the > "bool ptp" argument, because struct vsc85xx_shared_private is used only > in PTP code. > > You can make a preparatory change in 'net' patch sets, even without > a Fixes: tag, if you clearly explain what it's for. 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; } } --- -- /Horatiu
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!
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).
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!
The 09/26/2025 11:19, Russell King (Oracle) wrote: Hi, > > 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. Thanks for the explanation and for suggestion. I will use your suggestion in the next version. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! -- /Horatiu
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). 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? 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?
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. > > 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
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
On Fri, Sep 26, 2025 at 09:11:11AM +0200, Horatiu Vultur wrote: > 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? This sounds good, however I don't exactly understand how it fits in with your response to Russell to replace phydev->phy_id with phydev->drv->phy_id in the next revision. If the revision check in vsc8584_probe() goes away, where will you use phydev->drv->phy_id?
The 09/26/2025 15:20, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, Sep 26, 2025 at 09:11:11AM +0200, Horatiu Vultur wrote: > > 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? > > This sounds good, however I don't exactly understand how it fits in with > your response to Russell to replace phydev->phy_id with phydev->drv->phy_id > in the next revision. If the revision check in vsc8584_probe() goes away, > where will you use phydev->drv->phy_id? I got a little bit confused here. Because no one answer to this email, I thought it might not be OK, so that is the reason why I said that I will go with Russell approach. But if you think that this approach that I proposed here is OK (as you seem to be). Then I will go with this and then I will not do Russell suggestion because it is not needed anymore. -- /Horatiu
On Fri, Sep 26, 2025 at 02:21:16PM +0200, Horatiu Vultur wrote: > The 09/26/2025 15:20, Vladimir Oltean wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Fri, Sep 26, 2025 at 09:11:11AM +0200, Horatiu Vultur wrote: > > > 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? > > > > This sounds good, however I don't exactly understand how it fits in with > > your response to Russell to replace phydev->phy_id with phydev->drv->phy_id > > in the next revision. If the revision check in vsc8584_probe() goes away, > > where will you use phydev->drv->phy_id? > > I got a little bit confused here. > Because no one answer to this email, I thought it might not be OK, so > that is the reason why I said that I will go with Russell approach. > But if you think that this approach that I proposed here is OK (as you seem > to be). Then I will go with this and then I will not do Russell > suggestion because it is not needed anymore. Yes, sorry, I am in the middle of some work and I'm not as responsive as I should be.
The 09/26/2025 15:27, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, Sep 26, 2025 at 02:21:16PM +0200, Horatiu Vultur wrote: > > The 09/26/2025 15:20, Vladimir Oltean wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > > > On Fri, Sep 26, 2025 at 09:11:11AM +0200, Horatiu Vultur wrote: > > > > 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? > > > > > > This sounds good, however I don't exactly understand how it fits in with > > > your response to Russell to replace phydev->phy_id with phydev->drv->phy_id > > > in the next revision. If the revision check in vsc8584_probe() goes away, > > > where will you use phydev->drv->phy_id? > > > > I got a little bit confused here. > > Because no one answer to this email, I thought it might not be OK, so > > that is the reason why I said that I will go with Russell approach. > > But if you think that this approach that I proposed here is OK (as you seem > > to be). Then I will go with this and then I will not do Russell > > suggestion because it is not needed anymore. > > Yes, sorry, I am in the middle of some work and I'm not as responsive as > I should be. No worries. Thanks for all the help you provided to this patch and all the previous ones! -- /Horatiu
© 2016 - 2025 Red Hat, Inc.