Re: [PATCH net-next v15] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Kyle

On 9/9/26 09:49, Kyle Switch wrote:
> Add support for Motorcomm YT8824 quad-port 2.5G PHY to the existing
> motorcomm driver, using the phy_package helpers for the shared top
> extended register space.
> It also adds a new exported phylib helper, genphy_c45_template_testmode().
> 

[...]

> +/**
> + * yt8824_resume() - resume the hardware
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_resume(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret = 0;

No need to zero-init

> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = yt8824_power_on(phydev);
> +	mutex_unlock(&priv->shared_lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * yt8824_power_down()  - set utp power down.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * NOTE: need WA like softreset
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_power_down(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +	int r;
> +
> +	if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL) {
> +		/* invalid test mode */
> +		ret = yt8824_utp_invalid_test_mode_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +		/* utp power down */
> +		ret = yt8824_utp_power_down(phydev);
> +		if (ret < 0)
> +			goto retry;
> +		/* normal mode */
> +		ret = yt8824_utp_normal_test_mode_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +	} else {
> +		/* invalid test mode */
> +		ret = yt8824_utp_invalid_test_mode_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		/* sds isolation */
> +		ret = yt8824_sds_isolate_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		/* utp power down */
> +		ret = yt8824_utp_power_down(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		/* normal mode */
> +		ret = yt8824_utp_normal_test_mode_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		/* sds soft reset and disable isolation */
> +		ret = yt8824_sds_isolate_and_softreset_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +	}
> +	return 0;
> +
> +retry:
> +	/*
> +	 * If the PHY down operation succeeds but the subsequent operation
> +	 * fails, revert to the default state.
> +	 */
> +	r = yt8824_utp_power_on(phydev);
> +	if (ret >= 0 && r < 0)
> +		ret = r;
> +	ret = yt8824_restore_working_status(phydev, ret);
> +	return ret;
> +}
> +
> +/**
> + * yt8824_suspend() - suspend the hardware
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_suspend(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret = 0;

No need to zero-initialize it

> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = yt8824_power_down(phydev);
> +	mutex_unlock(&priv->shared_lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * yt8824_config_aneg() - config negotiation
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_config_aneg(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int phy_ctrl = 0;
> +	int ret = 0;
> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		goto err;
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			      phydev->advertising))
> +		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN,
> +				     MDIO_AN_10GBT_CTRL,
> +				     MDIO_AN_10GBT_CTRL_ADV2_5G,
> +				     phy_ctrl);
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = __genphy_config_aneg(phydev, ret);
> +
> +err:
> +	mutex_unlock(&priv->shared_lock);
> +	return ret;
> +}
> +
> +/**
> + * yt8824_phy_package_probe_once()  - init phy packet for phy8824.
                                                   ^^ package ?


> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_phy_package_probe_once(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	struct device_node *np = phy_package_get_node(phydev);
> +	const char *interface_mode_name;
> +
> +	/* Initialise shared lock for YT8824 */
> +	mutex_init(&priv->shared_lock);
> +	priv->interface_mode = PHY_INTERFACE_MODE_INTERNAL;
> +	if (!of_property_read_string(np, "motorcomm,interface-mode",
> +				     &interface_mode_name)) {

I don't see that property documented anywhere in the bindings, and besides that
the typical way we deal with quad PHYs is to have each MAC node use
QSGMII / USXGMII as their phy-interface-mode. Any reason for needing that at
the package level ?


> +		if (!strcasecmp(interface_mode_name,
> +				phy_modes(PHY_INTERFACE_MODE_USXGMII))) {
> +			priv->interface_mode = PHY_INTERFACE_MODE_USXGMII;
> +		} else if (!strcasecmp
> +				(interface_mode_name,
> +				 phy_modes(PHY_INTERFACE_MODE_INTERNAL))) {
> +			priv->interface_mode = PHY_INTERFACE_MODE_INTERNAL;
> +		} else {
> +			return -EINVAL;
> +		}
> +	} else {
> +		phydev_warn(phydev, "%s, motorcomm,interface-mode missing in DTS.\n",
> +			    __func__);
> +	}
> +
> +	return 0;
> +}
> +

Maxime>  
> +/* PMA 10GBASE-T Template Test Mode Register*/
> +#define MDIO_PMA_10GBT_TESTMODE_MASK 0xE000	/* Template test mode */
> +#define MDIO_PMA_10GBT_TESTMODE_NORMAL 0x0000	/* Template Normal */
> +#define MDIO_PMA_10GBT_TESTMODE_1 0x2000 /* Template TestMode1 */
> +#define MDIO_PMA_10GBT_TESTMODE_2 0x4000 /* Template TestMode2 */
> +#define MDIO_PMA_10GBT_TESTMODE_3 0x6000 /* Template TestMode3 */
> +#define MDIO_PMA_10GBT_TESTMODE_4 0x8000 /* Template TestMode4 */
> +#define MDIO_PMA_10GBT_TESTMODE_5 0xa000 /* Template TestMode5 */
> +#define MDIO_PMA_10GBT_TESTMODE_6 0xc000 /* Template TestMode6 */
> +#define MDIO_PMA_10GBT_TESTMODE_7 0xe000 /* Template TestMode7 */
> +
>  /* PCS 10GBASE-R/-T status register 1. */
>  #define MDIO_PCS_10GBRT_STAT1_BLKLK	0x0001	/* Block lock attained */
>