Re: [PATCH net-next v9 10/11] net: phy: ncn26000: Support for onsemi's S2500 internal phy
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-10 19:40:29
Also in:
linux-devicetree, linux-doc, lkml
Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin. Low priority / nit picks for your consideration: - [Low] In this patch, the new S2500 entry in ncn26000_driver[] (drivers/net/phy/ncn26000.c, lines 172-191) has no .set_loopback… - [Low] The help text for the NCN26000_PHY Kconfig option (drivers/net/phy/Kconfig, lines 393-398) still says "Currently… -- net: phy: ncn26000: Support for onsemi's S2500 internal phy This patch adds a phy_driver entry for the internal PHY of the onsemi S2500 MAC-PHY to the ncn26000 driver. The NCN26000 PLCA TO_TIMER workaround in ncn26000_config_init() now runs only for the NCN26000 model.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c index 6d433f4e2c45d..a33ebb2a7ecef 100644 --- a/drivers/net/phy/ncn26000.c +++ b/drivers/net/phy/ncn26000.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -163,6 +169,26 @@ static int ncn26000_config_intr(struct phy_device *phydev) } static struct phy_driver ncn26000_driver[] = { + { + PHY_ID_MATCH_MODEL(PHY_ID_S2500), + .name = "S2500", + .features = PHY_BASIC_T1S_P2MP_FEATURES, + .config_init = ncn26000_config_init, + .config_intr = ncn26000_config_intr, + .config_aneg = ncn26000_config_aneg, + .read_status = ncn26000_read_status, + .handle_interrupt = ncn26000_handle_interrupt, + .set_plca_cfg = ncn26000_c45_plca_set_cfg, + .get_plca_cfg = genphy_c45_plca_get_cfg, + .get_plca_status = genphy_c45_plca_get_status, + .soft_reset = genphy_soft_reset, + .get_sqi = genphy_c45_oatc14_get_sqi, + .get_sqi_max = genphy_c45_oatc14_get_sqi_max, + .read_mmd = genphy_read_mmd_c45, + .write_mmd = genphy_write_mmd_c45, + .cable_test_get_status = genphy_c45_oatc14_cable_test_get_status, + .cable_test_start = genphy_c45_oatc14_cable_test_start, + },
[Severity: Low]
The S2500 entry has no .set_loopback callback, so phy_loopback() falls
back to genphy_loopback(). That function overwrites all of BMCR:
drivers/net/phy/phy_device.c:genphy_loopback() {
...
phy_modify(phydev, MII_BMCR, ~0, ctl);
ret = phy_read_poll_timeout(phydev, MII_BMSR, val,
val & BMSR_LSTATUS,
5000, 500000, true);
...
}
This driver uses that BMCR bit for link control:
#define NCN26000_BCMR_LINK_CTRL_BIT BMCR_ANENABLE
At this commit, does the PHY loopback selftest from "onsemi: s2500: Added
selftest support to onsemi's S2500 driver" fail on S2500? The path is
ethtool -t -> net_selftest -> phy_loopback() -> genphy_loopback().
This is not a regression against the pre-patch state. Before this patch
the generic PHY driver bound to 0x180FF411 and used the same fallback.
The later patch in this series, "net: phy: ncn26000: Support for
loopback", adds genphy_loopback_no_speed() and sets it as .set_loopback
on the S2500 entry. That fixes this by the end of the series.
[ ... ]
quoted hunk ↗ jump to hunk
static const struct mdio_device_id __maybe_unused ncn26000_tbl[] = { { PHY_ID_MATCH_MODEL(PHY_ID_NCN26000) }, + { PHY_ID_MATCH_MODEL(PHY_ID_S2500) }, { } };
[Severity: Low] Should the NCN26000_PHY help text in drivers/net/phy/Kconfig be updated as well? It still reads: Currently supports the NCN26000 10BASE-T1S Industrial PHY with MII interface. With this patch the driver also binds the S2500 internal PHY. That PHY is not a discrete MII-attached part; it is reached over the OA TC6 MDIO bus with C45 accessors. No later patch in the series changes Kconfig. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-s2500-mac-phy-support-v9-0-dcefe1d0bf0d%40onsemi.com