Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Wong Vee Khee <vee.khee.wong@linux.intel.com>,
        Russell King <linux@armlinux.org.uk>,
        Andrew Lunn <andrew@lunn.ch>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Voon Weifeng <weifeng.voon@intel.com>,
        Ong Boong Leong <boon.leong.ong@intel.com>
References: <20210323084853.25432-1-vee.khee.wong@linux.intel.com>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH net-next 1/1] net: phy: marvell10g: Add PHY loopback
 support for 88E2110 PHY
Message-ID: <1745f2b8-b5f9-dc2a-20e4-6a8fd0e7b2d4@gmail.com>
Date:   Tue, 23 Mar 2021 10:37:53 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20210323084853.25432-1-vee.khee.wong@linux.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 23.03.2021 09:48, Wong Vee Khee wrote:
> From: Tan Tee Min <tee.min.tan@intel.com>
> 
> Add support for PHY loopback for the Marvell 88E2110 PHY.
> 
> This allow user to perform selftest using ethtool.
> 
> Signed-off-by: Tan Tee Min <tee.min.tan@intel.com>
> Signed-off-by: Wong Vee Khee <vee.khee.wong@linux.intel.com>
> ---
>  drivers/net/phy/marvell10g.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index b1bb9b8e1e4e..c45a8f11bdcf 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -89,6 +89,8 @@ enum {
>  	MV_V2_TEMP_CTRL_DISABLE	= 0xc000,
>  	MV_V2_TEMP		= 0xf08c,
>  	MV_V2_TEMP_UNKNOWN	= 0x9600, /* unknown function */
> +
> +	MV_LOOPBACK		= BIT(14), /* Loopback (88E2110 only) */

Why do you state 88E2110 only?
This is the standard PCS loopback bit as described in clause 45.2.3.1.2
It's defined already as MDIO_PCS_CTRL1_LOOPBACK.
E.g. the 88x3310 spec also describes this bit.

>  };
>  
>  struct mv3310_priv {
> @@ -765,6 +767,15 @@ static int mv3310_set_tunable(struct phy_device *phydev,
>  	}
>  }
>  
> +static int mv3310_loopback(struct phy_device *phydev, bool enable)
> +{
> +	if (phydev->drv->phy_id != MARVELL_PHY_ID_88E2110)
> +		return -EOPNOTSUPP;

If you use the function in the 2110 PHY driver only, then why this check?
And why name it 3310 if it can be used with 2110 only?

This function uses c45 standard functionality only, therefore it should
go to generic code (similar to genphy_loopback for c22).


> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T,
> +			      MV_LOOPBACK, enable ? MV_LOOPBACK : 0);
> +}
> +
>  static struct phy_driver mv3310_drivers[] = {
>  	{
>  		.phy_id		= MARVELL_PHY_ID_88X3310,
> @@ -796,6 +807,7 @@ static struct phy_driver mv3310_drivers[] = {
>  		.get_tunable	= mv3310_get_tunable,
>  		.set_tunable	= mv3310_set_tunable,
>  		.remove		= mv3310_remove,
> +		.set_loopback	= mv3310_loopback,
>  	},
>  };
>  
>