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

[Kyle Switch <kyle.switch@motor-comm.com>]

On 9/9/26 20:03, Maxime Chevallier wrote:
> 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
Ans: will be fixed in next version.
>
>> +
>> +	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
Ans: will be fixed in next version.
>> +
>> +	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 ?
>
Ans: will be fixed in next version.
>> + * @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 ?

Ans: in the next version,we will add the corresponding dts.

The interface_mode here is used to indicate the current mode of the four 
ports,

which mainly includes two scenarios: 1. An independent external PHY 8824;

2. Four UTPs embedded in the switch. Different scenarios have somewhat

different processing logic for power down and power up, so an 
interface_mode

is defined separately here.

>
>
>> +		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 */
>>