Re: [PATCH net-next v9 10/11] net: phy: ncn26000: Support for onsemi's S2500 internal phy

[netdev-bot+sashiko]

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.

> 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

[ ... ]

> @@ -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.

[ ... ]

>  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