netdev - Re: [PATCH V2 net-next] net: phy: realtek: add support for RTL8221F(D)(I)-VD-CG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ca814367-5147-67d1-8cdb-35f57c2cbb99@gmail.com>
Date:   Tue, 16 Aug 2022 15:22:52 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     wei.fang@....com, andrew@...n.ch, linux@...linux.org.uk,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, netdev@...r.kernel.org
Cc:     xiaoning.wang@....com
Subject: Re: [PATCH V2 net-next] net: phy: realtek: add support for
 RTL8221F(D)(I)-VD-CG

On 16.08.2022 21:48, wei.fang@....com wrote:
> From: Clark Wang <xiaoning.wang@....com>
> 
> RTL8221F(D)(I)-VD-CG is the pin-to-pin upgrade chip from
> RTL8221F(D)(I)-CG.

Here you talk about RTL8221, in the driver struct definition you say RTL8211.
You changed the naming already for v2. It would be time for you to clarify
which chip you actually mean.
RTL8221 is a 2.5Gbps PHY, however you don't handle this mode in your code.

> 
> Add new PHY ID for this chip.
> It does not support RTL8211F_PHYCR2 anymore, so remove the w/r operation
> of this register.
> 
> Signed-off-by: Clark Wang <xiaoning.wang@....com>
> Signed-off-by: Wei Fang <wei.fang@....com>
> ---
> V2 change:
> 1. Commit message changed, RTL8221 instead of RTL8821.
> 2. Add has_phycr2 to struct rtl821x_priv.
> ---
>  drivers/net/phy/realtek.c | 44 ++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index a5671ab896b3..3d99fd6664d7 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -70,6 +70,7 @@
>  #define RTLGEN_SPEED_MASK			0x0630
>  
>  #define RTL_GENERIC_PHYID			0x001cc800
> +#define RTL_8211FVD_PHYID			0x001cc878
>  
>  MODULE_DESCRIPTION("Realtek PHY driver");
>  MODULE_AUTHOR("Johnson Leung");
> @@ -78,6 +79,7 @@ MODULE_LICENSE("GPL");
>  struct rtl821x_priv {
>  	u16 phycr1;
>  	u16 phycr2;
> +	bool has_phycr2;
>  };
>  
>  static int rtl821x_read_page(struct phy_device *phydev)
> @@ -94,6 +96,7 @@ static int rtl821x_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
>  	struct rtl821x_priv *priv;
> +	u32 phy_id = phydev->drv->phy_id;
>  	int ret;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -108,13 +111,16 @@ static int rtl821x_probe(struct phy_device *phydev)
>  	if (of_property_read_bool(dev->of_node, "realtek,aldps-enable"))
>  		priv->phycr1 |= RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF;
>  
> -	ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR2);
> -	if (ret < 0)
> -		return ret;
> +	priv->has_phycr2 = !(phy_id == RTL_8211FVD_PHYID);
> +	if (priv->has_phycr2) {
> +		ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR2);
> +		if (ret < 0)
> +			return ret;
>  
> -	priv->phycr2 = ret & RTL8211F_CLKOUT_EN;
> -	if (of_property_read_bool(dev->of_node, "realtek,clkout-disable"))
> -		priv->phycr2 &= ~RTL8211F_CLKOUT_EN;
> +		priv->phycr2 = ret & RTL8211F_CLKOUT_EN;
> +		if (of_property_read_bool(dev->of_node, "realtek,clkout-disable"))
> +			priv->phycr2 &= ~RTL8211F_CLKOUT_EN;
> +	}
>  
>  	phydev->priv = priv;
>  
> @@ -400,12 +406,14 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>  			val_rxdly ? "enabled" : "disabled");
>  	}
>  
> -	ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> -			       RTL8211F_CLKOUT_EN, priv->phycr2);
> -	if (ret < 0) {
> -		dev_err(dev, "clkout configuration failed: %pe\n",
> -			ERR_PTR(ret));
> -		return ret;
> +	if (priv->has_phycr2) {
> +		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> +				       RTL8211F_CLKOUT_EN, priv->phycr2);
> +		if (ret < 0) {
> +			dev_err(dev, "clkout configuration failed: %pe\n",
> +				ERR_PTR(ret));
> +			return ret;
> +		}
>  	}
>  
>  	return genphy_soft_reset(phydev);
> @@ -923,6 +931,18 @@ static struct phy_driver realtek_drvs[] = {
>  		.resume		= rtl821x_resume,
>  		.read_page	= rtl821x_read_page,
>  		.write_page	= rtl821x_write_page,
> +	}, {
> +		PHY_ID_MATCH_EXACT(RTL_8211FVD_PHYID),
> +		.name		= "RTL8211F-VD Gigabit Ethernet",

This conflicts with RTL8221 in the commit message.

> +		.probe		= rtl821x_probe,
> +		.config_init	= &rtl8211f_config_init,
> +		.read_status	= rtlgen_read_status,
> +		.config_intr	= &rtl8211f_config_intr,
> +		.handle_interrupt = rtl8211f_handle_interrupt,
> +		.suspend	= genphy_suspend,
> +		.resume		= rtl821x_resume,
> +		.read_page	= rtl821x_read_page,
> +		.write_page	= rtl821x_write_page,
>  	}, {
>  		.name		= "Generic FE-GE Realtek PHY",
>  		.match_phy_device = rtlgen_match_phy_device,