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

[netdev-bot+sashiko@kernel.org]

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 the existing drivers/net/phy/ncn26000.c header, the patch
  changes the copyright line from 'Copyright 2022 onsemi' to 'Copyright…
- [Low] The commit message only says S2500 internal PHY support is added
  and that the PLCA TO_TIMER default is correct on S2500.
- [Low] This patch makes S2500 use ncn26000_c45_plca_set_cfg().
- [Low] The NCN26000_PHY Kconfig help text is out of date.

--

net: phy: ncn26000: Support for onsemi's S2500 internal phy

This adds a phy_driver entry and an MDIO device table row for the
internal PHY of onsemi's S2500 MAC-PHY. The PLCA TO_TIMER workaround in
ncn26000_config_init() now only runs on NCN26000. It also adds a
MAINTAINERS entry and a MODULE_AUTHOR line.

> diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
> index 6d433f4e2c45d..0006e157917df 100644
> --- a/drivers/net/phy/ncn26000.c
> +++ b/drivers/net/phy/ncn26000.c
> @@ -2,7 +2,7 @@
>  /*
>   *  Driver for the onsemi 10BASE-T1S NCN26000 PHYs family.
>   *
> - * Copyright 2022 onsemi
> + * Copyright 2026 onsemi
>   */

[Severity: Low]
This isn't a bug, but should the 2022 year stay here, for example as
"Copyright 2022-2026 onsemi"?

Most of the file is still the original 2022 code. This patch only adds a
driver entry, a conditional TO_TIMER write and a table row. Neither the
commit message nor the changelog mentions the date change.

[ ... ]

> @@ -163,6 +169,24 @@ 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_aneg           = ncn26000_config_aneg,
> +		.read_status           = ncn26000_read_status,
> +		.set_plca_cfg          = ncn26000_c45_plca_set_cfg,

[Severity: Low]
Is bit 7 of VEND2 register 0x8001 on the S2500 the same ENI bit as on
the NCN26000?

ncn26000_c45_plca_set_cfg() changes that bit on every PLCA enable and
disable:

    eni_cfg = (plca_cfg->enabled) ? NCN26000_PHYCFG1_ENI : 0;
    return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
                          NCN26000_REG_PHYCFG1,
                          NCN26000_PHYCFG1_ENI_MASK, eni_cfg);

The S2500 MAC driver uses a different name for the same MMS4 register
and defines no bits for it:

drivers/net/ethernet/onsemi/s2500/s2500_hw_def.h:
    #define S2500_REG_VS_TWEAKS1		0x8001

It is also listed under OA_TC6_PHY_C45_VS_PLCA_MMS4 in s2500_ethtool.c.

If bit 7 means something else on the S2500, then every
"ethtool --set-plca-cfg ... enable on|off" would silently set or clear
an unrelated vendor tweak bit.

Can the S2500 bit layout be confirmed, or one name used in both drivers?

> +		.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]
This isn't a bug, but the commit message only mentions S2500 support and
the TO_TIMER default. Could it also describe what the S2500 entry does
that the NCN26000 entry does not?

- SQI reporting through genphy_c45_oatc14_get_sqi() and
  genphy_c45_oatc14_get_sqi_max().

- OATC14 cable testing through .cable_test_start and
  .cable_test_get_status.

- Direct C45 MMD access through genphy_read_mmd_c45() and
  genphy_write_mmd_c45(). These helpers are for PHYs that support direct
  C45 access but not C22 indirect MMD access (registers 13 and 14). That
  hardware property seems worth stating.

- No .config_intr or .handle_interrupt, so the PHY always runs in polled
  mode. Only the v10 changelog says this, and that text is dropped when
  the patch is applied.

The patch also includes some changes that the commit message doesn't
mention:

- an unrelated reorder of the local variables in ncn26000_config_intr()
- a new MAINTAINERS M: entry
- a new MODULE_AUTHOR line

[ ... ]

> @@ -183,11 +207,13 @@ module_phy_driver(ncn26000_driver);
>  
>  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 says:

    Currently supports the NCN26000 10BASE-T1S Industrial PHY
    with MII interface.

With this patch the driver also binds PHY_ID_S2500. That is the internal
PHY of the S2500 SPI MAC-PHY, which has no MII interface. S2500_MACPHY
also selects NCN26000_PHY.

The help text is still unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-s2500-mac-phy-support-v10-0-31b5a69b85b7%40onsemi.com