[PATCH v1 0/3] net: phy: Add qca8081 ethernet phy driver

[PATCH v1 0/3] net: phy: Add qca8081 ethernet phy driver

[Luo Jie]

This patch series add the qca8081 ethernet phy driver support, which
improve the wol feature, leverage at803x phy driver and add the cdt
feature.

Changes in v1:
	* merge qca8081 phy driver into at803x.
	* add cdt feature.
	* leverage at803x phy driver helpers.

Luo Jie (3):
  net: phy: improve the wol feature of at803x
  net: phy: add qca8081 ethernet phy driver
  net: phy: add cdt feature of qca8081 phy

 drivers/net/phy/at803x.c | 632 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 557 insertions(+), 75 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 1/3] net: phy: improve the wol feature of at803x

[Luo Jie]

wol is controlled by bit 5 of reg 3.8012, which should be
configured by set_wol of phy_driver.

Signed-off-by: Luo Jie <luoj@codeaurora.org>
---
 drivers/net/phy/at803x.c | 50 +++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 5d62b85a4024..ecae26f11aa4 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -70,10 +70,14 @@
 #define AT803X_CDT_STATUS_DELTA_TIME_MASK	GENMASK(7, 0)
 #define AT803X_LED_CONTROL			0x18
 
-#define AT803X_DEVICE_ADDR			0x03
+/* WOL control */
+#define AT803X_PHY_MMD3_WOL_CTRL		0x8012
+#define AT803X_WOL_EN				BIT(5)
+
 #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
 #define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
 #define AT803X_LOC_MAC_ADDR_32_47_OFFSET	0x804A
+
 #define AT803X_REG_CHIP_CONFIG			0x1f
 #define AT803X_BT_BX_REG_SEL			0x8000
 
@@ -328,12 +332,6 @@ static int at803x_set_wol(struct phy_device *phydev,
 	struct net_device *ndev = phydev->attached_dev;
 	const u8 *mac;
 	int ret;
-	u32 value;
-	unsigned int i, offsets[] = {
-		AT803X_LOC_MAC_ADDR_32_47_OFFSET,
-		AT803X_LOC_MAC_ADDR_16_31_OFFSET,
-		AT803X_LOC_MAC_ADDR_0_15_OFFSET,
-	};
 
 	if (!ndev)
 		return -ENODEV;
@@ -344,23 +342,30 @@ static int at803x_set_wol(struct phy_device *phydev,
 		if (!is_valid_ether_addr(mac))
 			return -EINVAL;
 
-		for (i = 0; i < 3; i++)
-			phy_write_mmd(phydev, AT803X_DEVICE_ADDR, offsets[i],
-				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
+		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_32_47_OFFSET,
+				mac[1] | (mac[0] << 8));
+		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_16_31_OFFSET,
+				mac[3] | (mac[2] << 8));
+		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_0_15_OFFSET,
+				mac[5] | (mac[4] << 8));
 
-		value = phy_read(phydev, AT803X_INTR_ENABLE);
-		value |= AT803X_INTR_ENABLE_WOL;
-		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
+		/* clear the pending interrupt */
+		phy_read(phydev, AT803X_INTR_STATUS);
+
+		ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
 		if (ret)
 			return ret;
-		value = phy_read(phydev, AT803X_INTR_STATUS);
+
+		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
+				0, AT803X_WOL_EN);
+
 	} else {
-		value = phy_read(phydev, AT803X_INTR_ENABLE);
-		value &= (~AT803X_INTR_ENABLE_WOL);
-		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
+		ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
 		if (ret)
 			return ret;
-		value = phy_read(phydev, AT803X_INTR_STATUS);
+
+		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
+				AT803X_WOL_EN, 0);
 	}
 
 	return ret;
@@ -369,13 +374,16 @@ static int at803x_set_wol(struct phy_device *phydev,
 static void at803x_get_wol(struct phy_device *phydev,
 			   struct ethtool_wolinfo *wol)
 {
-	u32 value;
+	int ret;
 
 	wol->supported = WAKE_MAGIC;
 	wol->wolopts = 0;
 
-	value = phy_read(phydev, AT803X_INTR_ENABLE);
-	if (value & AT803X_INTR_ENABLE_WOL)
+	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL);
+	if (ret < 0)
+		return;
+
+	if (ret & AT803X_WOL_EN)
 		wol->wolopts |= WAKE_MAGIC;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 1/3] net: phy: improve the wol feature of at803x

[Andrew Lunn]

On Mon, Aug 30, 2021 at 07:07:31PM +0800, Luo Jie wrote:
> wol is controlled by bit 5 of reg 3.8012, which should be
> configured by set_wol of phy_driver.
> 
> Signed-off-by: Luo Jie <luoj@codeaurora.org>
> ---
>  drivers/net/phy/at803x.c | 50 +++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 5d62b85a4024..ecae26f11aa4 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -70,10 +70,14 @@
>  #define AT803X_CDT_STATUS_DELTA_TIME_MASK	GENMASK(7, 0)
>  #define AT803X_LED_CONTROL			0x18
>  
> -#define AT803X_DEVICE_ADDR			0x03
> +/* WOL control */
> +#define AT803X_PHY_MMD3_WOL_CTRL		0x8012
> +#define AT803X_WOL_EN				BIT(5)
> +
>  #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
>  #define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
>  #define AT803X_LOC_MAC_ADDR_32_47_OFFSET	0x804A
> +
>  #define AT803X_REG_CHIP_CONFIG			0x1f
>  #define AT803X_BT_BX_REG_SEL			0x8000
>  
> @@ -328,12 +332,6 @@ static int at803x_set_wol(struct phy_device *phydev,
>  	struct net_device *ndev = phydev->attached_dev;
>  	const u8 *mac;
>  	int ret;
> -	u32 value;
> -	unsigned int i, offsets[] = {
> -		AT803X_LOC_MAC_ADDR_32_47_OFFSET,
> -		AT803X_LOC_MAC_ADDR_16_31_OFFSET,
> -		AT803X_LOC_MAC_ADDR_0_15_OFFSET,
> -	};
>  
>  	if (!ndev)
>  		return -ENODEV;
> @@ -344,23 +342,30 @@ static int at803x_set_wol(struct phy_device *phydev,
>  		if (!is_valid_ether_addr(mac))
>  			return -EINVAL;
>  
> -		for (i = 0; i < 3; i++)
> -			phy_write_mmd(phydev, AT803X_DEVICE_ADDR, offsets[i],
> -				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
> +		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_32_47_OFFSET,
> +				mac[1] | (mac[0] << 8));
> +		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_16_31_OFFSET,
> +				mac[3] | (mac[2] << 8));
> +		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_0_15_OFFSET,
> +				mac[5] | (mac[4] << 8));

Please try to keep your changes minimal. It looks like all you really
need is to replace AT803X_DEVICE_ADDR with MDIO_MMD_PCS. Everything
else is O.K. Maybe make offset a const?

Making the change more complex than it needs to be makes it harder to
review.

>  
> -		value = phy_read(phydev, AT803X_INTR_ENABLE);
> -		value |= AT803X_INTR_ENABLE_WOL;
> -		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);

So that it be replaced with a phy_modify().


> +		/* clear the pending interrupt */
> +		phy_read(phydev, AT803X_INTR_STATUS);

But where did this come from? 

> +
> +		ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
>  		if (ret)
>  			return ret;
> -		value = phy_read(phydev, AT803X_INTR_STATUS);
> +
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
> +				0, AT803X_WOL_EN);
> +
>  	} else {
> -		value = phy_read(phydev, AT803X_INTR_ENABLE);
> -		value &= (~AT803X_INTR_ENABLE_WOL);
> -		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
> +		ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);

This makes sense

>  		if (ret)
>  			return ret;
> -		value = phy_read(phydev, AT803X_INTR_STATUS);
> +
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
> +				AT803X_WOL_EN, 0);

But where did this come from?

I could be wrong, but i get the feeling you just replaced the code
with what you have in your new driver, rather than step by step
improve this code.

Please break this patch up into a number of patches:

AT803X_DEVICE_ADDR with MDIO_MMD_PCS
read/write to modify.

Other patches for the remaining changes, if actually required, with a
good explanation of why they are needed.

    Andrew

Re: [PATCH v1 1/3] net: phy: improve the wol feature of at803x

[Jie Luo]

On 8/30/2021 9:32 PM, Andrew Lunn wrote:
> On Mon, Aug 30, 2021 at 07:07:31PM +0800, Luo Jie wrote:
>> wol is controlled by bit 5 of reg 3.8012, which should be
>> configured by set_wol of phy_driver.
>>
>> Signed-off-by: Luo Jie <luoj@codeaurora.org>
>> ---
>>   drivers/net/phy/at803x.c | 50 +++++++++++++++++++++++-----------------
>>   1 file changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>> index 5d62b85a4024..ecae26f11aa4 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>> @@ -70,10 +70,14 @@
>>   #define AT803X_CDT_STATUS_DELTA_TIME_MASK	GENMASK(7, 0)
>>   #define AT803X_LED_CONTROL			0x18
>>   
>> -#define AT803X_DEVICE_ADDR			0x03
>> +/* WOL control */
>> +#define AT803X_PHY_MMD3_WOL_CTRL		0x8012
>> +#define AT803X_WOL_EN				BIT(5)
>> +
>>   #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
>>   #define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
>>   #define AT803X_LOC_MAC_ADDR_32_47_OFFSET	0x804A
>> +
>>   #define AT803X_REG_CHIP_CONFIG			0x1f
>>   #define AT803X_BT_BX_REG_SEL			0x8000
>>   
>> @@ -328,12 +332,6 @@ static int at803x_set_wol(struct phy_device *phydev,
>>   	struct net_device *ndev = phydev->attached_dev;
>>   	const u8 *mac;
>>   	int ret;
>> -	u32 value;
>> -	unsigned int i, offsets[] = {
>> -		AT803X_LOC_MAC_ADDR_32_47_OFFSET,
>> -		AT803X_LOC_MAC_ADDR_16_31_OFFSET,
>> -		AT803X_LOC_MAC_ADDR_0_15_OFFSET,
>> -	};
>>   
>>   	if (!ndev)
>>   		return -ENODEV;
>> @@ -344,23 +342,30 @@ static int at803x_set_wol(struct phy_device *phydev,
>>   		if (!is_valid_ether_addr(mac))
>>   			return -EINVAL;
>>   
>> -		for (i = 0; i < 3; i++)
>> -			phy_write_mmd(phydev, AT803X_DEVICE_ADDR, offsets[i],
>> -				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
>> +		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_32_47_OFFSET,
>> +				mac[1] | (mac[0] << 8));
>> +		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_16_31_OFFSET,
>> +				mac[3] | (mac[2] << 8));
>> +		phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_0_15_OFFSET,
>> +				mac[5] | (mac[4] << 8));
> Please try to keep your changes minimal. It looks like all you really
> need is to replace AT803X_DEVICE_ADDR with MDIO_MMD_PCS. Everything
> else is O.K. Maybe make offset a const?
>
> Making the change more complex than it needs to be makes it harder to
> review.
thanks Andrew for the comments, will keep this changes minimal in the 
next patch series.
>
>>   
>> -		value = phy_read(phydev, AT803X_INTR_ENABLE);
>> -		value |= AT803X_INTR_ENABLE_WOL;
>> -		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
> So that it be replaced with a phy_modify().
yes, it's replaced with phy_modify().
>
>
>> +		/* clear the pending interrupt */
>> +		phy_read(phydev, AT803X_INTR_STATUS);
> But where did this come from?
this code is not new added, which is existed code for at803x phy driver.
>
>> +
>> +		ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
>>   		if (ret)
>>   			return ret;
>> -		value = phy_read(phydev, AT803X_INTR_STATUS);
>> +
>> +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
>> +				0, AT803X_WOL_EN);
>> +
>>   	} else {
>> -		value = phy_read(phydev, AT803X_INTR_ENABLE);
>> -		value &= (~AT803X_INTR_ENABLE_WOL);
>> -		ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
>> +		ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
> This makes sense
>
>>   		if (ret)
>>   			return ret;
>> -		value = phy_read(phydev, AT803X_INTR_STATUS);
>> +
>> +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
>> +				AT803X_WOL_EN, 0);
> But where did this come from?
For wol_set, i add the AT803X_WOL_EN configuration, which is necessary 
for the wol feature.
>
> I could be wrong, but i get the feeling you just replaced the code
> with what you have in your new driver, rather than step by step
> improve this code.
>
> Please break this patch up into a number of patches:
>
> AT803X_DEVICE_ADDR with MDIO_MMD_PCS
> read/write to modify.
>
> Other patches for the remaining changes, if actually required, with a
> good explanation of why they are needed.
>
>      Andrew

Hi Andrew, thanks for the suggestions. Will break the changes into 
minimal patches, and check the at803x phy driver in detail to

improve the driver in the next patch series.

[PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver

[Luo Jie]

qca8081 is a single port ethernet phy chip that supports
10/100/1000/2500 Mbps mode.

Signed-off-by: Luo Jie <luoj@codeaurora.org>
---
 drivers/net/phy/at803x.c | 389 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 338 insertions(+), 51 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index ecae26f11aa4..2b3563ae152f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -33,10 +33,10 @@
 #define AT803X_SFC_DISABLE_JABBER		BIT(0)
 
 #define AT803X_SPECIFIC_STATUS			0x11
-#define AT803X_SS_SPEED_MASK			(3 << 14)
-#define AT803X_SS_SPEED_1000			(2 << 14)
-#define AT803X_SS_SPEED_100			(1 << 14)
-#define AT803X_SS_SPEED_10			(0 << 14)
+#define AT803X_SS_SPEED_MASK			GENMASK(15, 14)
+#define AT803X_SS_SPEED_1000			2
+#define AT803X_SS_SPEED_100			1
+#define AT803X_SS_SPEED_10			0
 #define AT803X_SS_DUPLEX			BIT(13)
 #define AT803X_SS_SPEED_DUPLEX_RESOLVED		BIT(11)
 #define AT803X_SS_MDIX				BIT(6)
@@ -158,6 +158,8 @@
 #define QCA8337_PHY_ID				0x004dd036
 #define QCA8K_PHY_ID_MASK			0xffffffff
 
+#define QCA8081_PHY_ID				0x004dd101
+
 #define QCA8K_DEVFLAGS_REVISION_MASK		GENMASK(2, 0)
 
 #define AT803X_PAGE_FIBER			0
@@ -167,7 +169,73 @@
 #define AT803X_KEEP_PLL_ENABLED			BIT(0)
 #define AT803X_DISABLE_SMARTEEE			BIT(1)
 
-MODULE_DESCRIPTION("Qualcomm Atheros AR803x PHY driver");
+/* MII special status */
+#define QCA808X_SS_SPEED_MASK			GENMASK(9, 7)
+#define QCA808X_SS_SPEED_2500			4
+
+/* Conifg seed */
+#define QCA808X_PHY_DEBUG_LOCAL_SEED		9
+#define QCA808X_MASTER_SLAVE_SEED_ENABLE	BIT(1)
+#define QCA808X_MASTER_SLAVE_SEED_CFG		GENMASK(12, 2)
+#define QCA808X_MASTER_SLAVE_SEED_RANGE		0x32
+
+/* ADC threshold */
+#define QCA808X_PHY_DEBUG_ADC_THRESHOLD		0x2c80
+#define QCA808X_ADC_THRESHOLD_MASK		GENMASK(7, 0)
+#define QCA808X_ADC_THRESHOLD_80MV		0
+#define QCA808X_ADC_THRESHOLD_100MV		0xf0
+#define QCA808X_ADC_THRESHOLD_200MV		0x0f
+#define QCA808X_ADC_THRESHOLD_300MV		0xff
+
+/* CLD control */
+#define QCA808X_PHY_MMD3_ADDR_CLD_CTRL7		0x8007
+#define QCA808X_8023AZ_AFE_CTRL_MASK		GENMASK(8, 4)
+#define QCA808X_8023AZ_AFE_EN			0x90
+
+/* AZ control */
+#define QCA808X_PHY_MMD3_AZ_TRAINING_CTRL	0x8008
+#define QCA808X_MMD3_AZ_TRAINING_VAL		0x1c32
+
+/* AN 2.5G */
+#define QCA808X_FAST_RETRAIN_2500BT		BIT(5)
+#define QCA808X_ADV_LOOP_TIMING			BIT(0)
+
+/* Fast retrain related registers */
+#define QCA808X_PHY_MMD1_FAST_RETRAIN_CTL	0x93
+#define QCA808X_FAST_RETRAIN_CTRL_VALUE		0x1
+
+#define QCA808X_PHY_MMD1_MSE_THRESHOLD_20DB	0x8014
+#define QCA808X_MSE_THRESHOLD_20DB_VALUE	0x529
+
+#define QCA808X_PHY_MMD1_MSE_THRESHOLD_17DB	0x800E
+#define QCA808X_MSE_THRESHOLD_17DB_VALUE	0x341
+
+#define QCA808X_PHY_MMD1_MSE_THRESHOLD_27DB	0x801E
+#define QCA808X_MSE_THRESHOLD_27DB_VALUE	0x419
+
+#define QCA808X_PHY_MMD1_MSE_THRESHOLD_28DB	0x8020
+#define QCA808X_MSE_THRESHOLD_28DB_VALUE	0x341
+
+#define QCA808X_PHY_MMD7_TOP_OPTION1		0x901c
+#define QCA808X_TOP_OPTION1_DATA		0x0
+
+#define QCA808X_PHY_MMD7_EEE_LP_ADVERTISEMENT	0x40
+#define QCA808X_EEE_ADV_THP			0x8
+
+#define QCA808X_PHY_MMD3_DEBUG_1		0xa100
+#define QCA808X_MMD3_DEBUG_1_VALUE		0x9203
+#define QCA808X_PHY_MMD3_DEBUG_2		0xa101
+#define QCA808X_MMD3_DEBUG_2_VALUE		0x48ad
+#define QCA808X_PHY_MMD3_DEBUG_3		0xa103
+#define QCA808X_MMD3_DEBUG_3_VALUE		0x1698
+#define QCA808X_PHY_MMD3_DEBUG_4		0xa105
+#define QCA808X_MMD3_DEBUG_4_VALUE		0x8001
+#define QCA808X_PHY_MMD3_DEBUG_5		0xa106
+#define QCA808X_MMD3_DEBUG_5_VALUE		0x1111
+#define QCA808X_PHY_MMD3_DEBUG_6		0xa011
+#define QCA808X_MMD3_DEBUG_6_VALUE		0x5f85
+
+MODULE_DESCRIPTION("Qualcomm Atheros AR803x and QCA808X PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
 
@@ -711,11 +779,18 @@ static void at803x_remove(struct phy_device *phydev)
 
 static int at803x_get_features(struct phy_device *phydev)
 {
-	int err;
+	int val;
 
-	err = genphy_read_abilities(phydev);
-	if (err)
-		return err;
+	val = genphy_read_abilities(phydev);
+	if (val)
+		return val;
+
+	if (at803x_match_phy_id(phydev, QCA8081_PHY_ID)) {
+		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
+
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported,
+				val & MDIO_PMA_NG_EXTABLE_2_5GBT);
+	}
 
 	if (!at803x_match_phy_id(phydev, ATH8031_PHY_ID))
 		return 0;
@@ -935,44 +1010,44 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	}
 }
 
-static int at803x_read_status(struct phy_device *phydev)
+static int at803x_read_specific_status(struct phy_device *phydev)
 {
-	int ss, err, old_link = phydev->link;
-
-	/* Update the link, but return if there was an error */
-	err = genphy_update_link(phydev);
-	if (err)
-		return err;
-
-	/* why bother the PHY if nothing can have changed */
-	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
-		return 0;
+	int val;
 
-	phydev->speed = SPEED_UNKNOWN;
-	phydev->duplex = DUPLEX_UNKNOWN;
-	phydev->pause = 0;
-	phydev->asym_pause = 0;
+	val = phy_read(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL);
+	if (val < 0)
+		return val;
 
-	err = genphy_read_lpa(phydev);
-	if (err < 0)
-		return err;
+	switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, val)) {
+	case AT803X_SFC_MANUAL_MDI:
+		phydev->mdix_ctrl = ETH_TP_MDI;
+		break;
+	case AT803X_SFC_MANUAL_MDIX:
+		phydev->mdix_ctrl = ETH_TP_MDI_X;
+		break;
+	case AT803X_SFC_AUTOMATIC_CROSSOVER:
+		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+		break;
+	}
 
 	/* Read the AT8035 PHY-Specific Status register, which indicates the
 	 * speed and duplex that the PHY is actually using, irrespective of
 	 * whether we are in autoneg mode or not.
 	 */
-	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
-	if (ss < 0)
-		return ss;
+	val = phy_read(phydev, AT803X_SPECIFIC_STATUS);
+	if (val < 0)
+		return val;
 
-	if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
-		int sfc;
+	if (val & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
+		int speed;
 
-		sfc = phy_read(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL);
-		if (sfc < 0)
-			return sfc;
+		/* qca8081 phy takes the different bits for speed value from at803x phy */
+		if (at803x_match_phy_id(phydev, QCA8081_PHY_ID))
+			speed = FIELD_GET(QCA808X_SS_SPEED_MASK, val);
+		else
+			speed = FIELD_GET(AT803X_SS_SPEED_MASK, val);
 
-		switch (ss & AT803X_SS_SPEED_MASK) {
+		switch (speed) {
 		case AT803X_SS_SPEED_10:
 			phydev->speed = SPEED_10;
 			break;
@@ -982,30 +1057,51 @@ static int at803x_read_status(struct phy_device *phydev)
 		case AT803X_SS_SPEED_1000:
 			phydev->speed = SPEED_1000;
 			break;
+		case QCA808X_SS_SPEED_2500:
+			phydev->speed = SPEED_2500;
+			break;
 		}
-		if (ss & AT803X_SS_DUPLEX)
+
+		if (val & AT803X_SS_DUPLEX)
 			phydev->duplex = DUPLEX_FULL;
 		else
 			phydev->duplex = DUPLEX_HALF;
 
-		if (ss & AT803X_SS_MDIX)
+		if (val & AT803X_SS_MDIX)
 			phydev->mdix = ETH_TP_MDI_X;
 		else
 			phydev->mdix = ETH_TP_MDI;
-
-		switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, sfc)) {
-		case AT803X_SFC_MANUAL_MDI:
-			phydev->mdix_ctrl = ETH_TP_MDI;
-			break;
-		case AT803X_SFC_MANUAL_MDIX:
-			phydev->mdix_ctrl = ETH_TP_MDI_X;
-			break;
-		case AT803X_SFC_AUTOMATIC_CROSSOVER:
-			phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
-			break;
-		}
 	}
 
+	return 0;
+}
+
+static int at803x_read_status(struct phy_device *phydev)
+{
+	int err, old_link = phydev->link;
+
+	/* Update the link, but return if there was an error */
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	/* why bother the PHY if nothing can have changed */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	err = genphy_read_lpa(phydev);
+	if (err < 0)
+		return err;
+
+	err = at803x_read_specific_status(phydev);
+	if (err < 0)
+		return err;
+
 	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
 		phy_resolve_aneg_pause(phydev);
 
@@ -1053,7 +1149,24 @@ static int at803x_config_aneg(struct phy_device *phydev)
 			return ret;
 	}
 
-	return genphy_config_aneg(phydev);
+	ret = 0;
+	if (at803x_match_phy_id(phydev, QCA8081_PHY_ID)) {
+		int phy_ctrl = 0;
+
+		/* The reg MII_BMCR also needs to be configured for force mode. */
+		if (phydev->autoneg == AUTONEG_DISABLE)
+			genphy_c45_pma_setup_forced(phydev);
+
+		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)
+			return ret;
+	}
+
+	return __genphy_config_aneg(phydev, ret);
 }
 
 static int at803x_get_downshift(struct phy_device *phydev, u8 *d)
@@ -1325,6 +1438,161 @@ static int qca83xx_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca808x_phy_fast_retrain_cfg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
+			MDIO_AN_10GBT_CTRL_ADV2_5G |
+			QCA808X_FAST_RETRAIN_2500BT |
+			QCA808X_ADV_LOOP_TIMING);
+	if (ret)
+		return ret;
+
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_FAST_RETRAIN_CTL,
+			QCA808X_FAST_RETRAIN_CTRL_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_20DB,
+			QCA808X_MSE_THRESHOLD_20DB_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_17DB,
+			QCA808X_MSE_THRESHOLD_17DB_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_27DB,
+			QCA808X_MSE_THRESHOLD_27DB_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_28DB,
+			QCA808X_MSE_THRESHOLD_28DB_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_AN, QCA808X_PHY_MMD7_EEE_LP_ADVERTISEMENT,
+			QCA808X_EEE_ADV_THP);
+	phy_write_mmd(phydev, MDIO_MMD_AN, QCA808X_PHY_MMD7_TOP_OPTION1,
+			QCA808X_TOP_OPTION1_DATA);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_1,
+			QCA808X_MMD3_DEBUG_1_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_4,
+			QCA808X_MMD3_DEBUG_4_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_5,
+			QCA808X_MMD3_DEBUG_5_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_3,
+			QCA808X_MMD3_DEBUG_3_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_6,
+			QCA808X_MMD3_DEBUG_6_VALUE);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_2,
+			QCA808X_MMD3_DEBUG_2_VALUE);
+
+	return 0;
+}
+
+static int qca808x_phy_ms_random_seed_set(struct phy_device *phydev)
+{
+	u16 seed_value = (prandom_u32() % QCA808X_MASTER_SLAVE_SEED_RANGE) << 2;
+
+	return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
+			QCA808X_MASTER_SLAVE_SEED_CFG, seed_value);
+}
+
+static int qca808x_phy_ms_seed_enable(struct phy_device *phydev, bool enable)
+{
+	u16 seed_enable = 0;
+
+	if (enable)
+		seed_enable = QCA808X_MASTER_SLAVE_SEED_ENABLE;
+
+	return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
+			QCA808X_MASTER_SLAVE_SEED_ENABLE, seed_enable);
+}
+
+static int qca808x_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Active adc&vga on 802.3az for the link 1000M and 100M */
+	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_ADDR_CLD_CTRL7,
+			QCA808X_8023AZ_AFE_CTRL_MASK, QCA808X_8023AZ_AFE_EN);
+	if (ret)
+		return ret;
+
+	/* Adjust the threshold on 802.3az for the link 1000M */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
+			QCA808X_PHY_MMD3_AZ_TRAINING_CTRL, QCA808X_MMD3_AZ_TRAINING_VAL);
+	if (ret)
+		return ret;
+
+	/* Config the fast retrain for the link 2500M */
+	ret = qca808x_phy_fast_retrain_cfg(phydev);
+	if (ret)
+		return ret;
+
+	/* Configure ramdom seed to make phy linked as slave mode for link 2500M */
+	ret = qca808x_phy_ms_random_seed_set(phydev);
+	if (ret)
+		return ret;
+
+	/* Enable seed */
+	ret = qca808x_phy_ms_seed_enable(phydev, true);
+	if (ret)
+		return ret;
+
+	/* Configure adc threshold as 100mv for the link 10M */
+	return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_ADC_THRESHOLD,
+			QCA808X_ADC_THRESHOLD_MASK, QCA808X_ADC_THRESHOLD_100MV);
+}
+
+static int qca808x_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
+	if (ret < 0)
+		return ret;
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->lp_advertising,
+			ret & MDIO_AN_10GBT_STAT_LP2_5G);
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;
+
+	ret = at803x_read_specific_status(phydev);
+	if (ret < 0)
+		return ret;
+
+	if (phydev->link && phydev->speed == SPEED_2500)
+		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+	else
+		phydev->interface = PHY_INTERFACE_MODE_SMII;
+
+	return 0;
+}
+
+static int qca808x_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_soft_reset(phydev);
+	if (ret < 0)
+		return ret;
+
+	return qca808x_phy_ms_seed_enable(phydev, true);
+}
+
+static void qca808x_link_change_notify(struct phy_device *phydev)
+{
+	int ret;
+
+	/* generate random seed as a lower value to make PHY linked as SLAVE easily,
+	 * excpet for master/slave configuration fault detected.
+	 */
+	if (phydev->state == PHY_NOLINK) {
+		ret = phy_read(phydev, MII_STAT1000);
+		if (ret < 0)
+			return;
+
+		if (ret & LPA_1000MSFAIL) {
+			qca808x_phy_ms_seed_enable(phydev, false);
+		} else {
+			qca808x_phy_ms_random_seed_set(phydev);
+			qca808x_phy_ms_seed_enable(phydev, true);
+		}
+	}
+}
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* Qualcomm Atheros AR8035 */
@@ -1434,6 +1702,24 @@ static struct phy_driver at803x_driver[] = {
 	.get_sset_count = at803x_get_sset_count,
 	.get_strings = at803x_get_strings,
 	.get_stats = at803x_get_stats,
+}, {
+	/* Qualcomm QCA8081 */
+	PHY_ID_MATCH_EXACT(QCA8081_PHY_ID),
+	.name			= "QCA8081 PHY",
+	.get_features		= at803x_get_features,
+	.config_aneg		= at803x_config_aneg,
+	.config_intr		= at803x_config_intr,
+	.handle_interrupt	= at803x_handle_interrupt,
+	.get_tunable		= at803x_get_tunable,
+	.set_tunable		= at803x_set_tunable,
+	.set_wol		= at803x_set_wol,
+	.get_wol		= at803x_get_wol,
+	.config_init		= qca808x_config_init,
+	.read_status		= qca808x_read_status,
+	.soft_reset		= qca808x_soft_reset,
+	.link_change_notify	= qca808x_link_change_notify,
+	.suspend		= genphy_suspend,
+	.resume			= genphy_resume,
 }, };
 
 module_phy_driver(at803x_driver);
@@ -1444,6 +1730,7 @@ static struct mdio_device_id __maybe_unused atheros_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(ATH8032_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(ATH8035_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(ATH9331_PHY_ID) },
+	{ PHY_ID_MATCH_EXACT(QCA8081_PHY_ID) },
 	{ }
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver

[Russell King (Oracle)]

On Mon, Aug 30, 2021 at 07:07:32PM +0800, Luo Jie wrote:
> +/* AN 2.5G */
> +#define QCA808X_FAST_RETRAIN_2500BT		BIT(5)
> +#define QCA808X_ADV_LOOP_TIMING			BIT(0)
> 
> +/* Fast retrain related registers */
> +#define QCA808X_PHY_MMD1_FAST_RETRAIN_CTL	0x93
> +#define QCA808X_FAST_RETRAIN_CTRL_VALUE		0x1

These are standard 802.3 defined registers bits - please add
definitions for them to uapi/linux/mdio.h.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver

[Jie Luo]

On 8/30/2021 7:39 PM, Russell King (Oracle) wrote:
> On Mon, Aug 30, 2021 at 07:07:32PM +0800, Luo Jie wrote:
>> +/* AN 2.5G */
>> +#define QCA808X_FAST_RETRAIN_2500BT		BIT(5)
>> +#define QCA808X_ADV_LOOP_TIMING			BIT(0)
>>
>> +/* Fast retrain related registers */
>> +#define QCA808X_PHY_MMD1_FAST_RETRAIN_CTL	0x93
>> +#define QCA808X_FAST_RETRAIN_CTRL_VALUE		0x1
> These are standard 802.3 defined registers bits - please add
> definitions for them to uapi/linux/mdio.h.
>
> Thanks.
will add definitions for the standard registers in the next patch set, 
thanks Russell for the comments.

Re: [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver

[Andrew Lunn]

On Mon, Aug 30, 2021 at 07:07:32PM +0800, Luo Jie wrote:
> qca8081 is a single port ethernet phy chip that supports
> 10/100/1000/2500 Mbps mode.
> 
> Signed-off-by: Luo Jie <luoj@codeaurora.org>
> ---
>  drivers/net/phy/at803x.c | 389 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 338 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index ecae26f11aa4..2b3563ae152f 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -33,10 +33,10 @@
>  #define AT803X_SFC_DISABLE_JABBER		BIT(0)
>  
>  #define AT803X_SPECIFIC_STATUS			0x11
> -#define AT803X_SS_SPEED_MASK			(3 << 14)
> -#define AT803X_SS_SPEED_1000			(2 << 14)
> -#define AT803X_SS_SPEED_100			(1 << 14)
> -#define AT803X_SS_SPEED_10			(0 << 14)
> +#define AT803X_SS_SPEED_MASK			GENMASK(15, 14)
> +#define AT803X_SS_SPEED_1000			2
> +#define AT803X_SS_SPEED_100			1
> +#define AT803X_SS_SPEED_10			0

This looks like an improvement, and nothing to do with qca8081. Please
make it an separate patch.

>  #define AT803X_SS_DUPLEX			BIT(13)
>  #define AT803X_SS_SPEED_DUPLEX_RESOLVED		BIT(11)
>  #define AT803X_SS_MDIX				BIT(6)
> @@ -158,6 +158,8 @@
>  #define QCA8337_PHY_ID				0x004dd036
>  #define QCA8K_PHY_ID_MASK			0xffffffff
>  
> +#define QCA8081_PHY_ID				0x004dd101
> +

Maybe keep all the PHY_ID together?

>  #define QCA8K_DEVFLAGS_REVISION_MASK		GENMASK(2, 0)
>  
>  #define AT803X_PAGE_FIBER			0
> @@ -167,7 +169,73 @@
>  #define AT803X_KEEP_PLL_ENABLED			BIT(0)
>  #define AT803X_DISABLE_SMARTEEE			BIT(1)
>  
> @@ -711,11 +779,18 @@ static void at803x_remove(struct phy_device *phydev)
>  
>  static int at803x_get_features(struct phy_device *phydev)
>  {
> -	int err;
> +	int val;

Why? The driver pretty consistently uses err for return values which
are errors.

>  
> -	err = genphy_read_abilities(phydev);
> -	if (err)
> -		return err;
> +	val = genphy_read_abilities(phydev);
> +	if (val)
> +		return val;
> +
> +	if (at803x_match_phy_id(phydev, QCA8081_PHY_ID)) {
> +		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);

You don't check if val indicates if there was an error.

> +
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported,
> +				val & MDIO_PMA_NG_EXTABLE_2_5GBT);
> +	}
>  
>  	if (!at803x_match_phy_id(phydev, ATH8031_PHY_ID))
>  		return 0;
> @@ -935,44 +1010,44 @@ static void at803x_link_change_notify(struct phy_device *phydev)
>  	}
>  }
>  
> -static int at803x_read_status(struct phy_device *phydev)
> +static int at803x_read_specific_status(struct phy_device *phydev)
>  {
> -	int ss, err, old_link = phydev->link;
> -
> -	/* Update the link, but return if there was an error */
> -	err = genphy_update_link(phydev);
> -	if (err)
> -		return err;
> -
> -	/* why bother the PHY if nothing can have changed */
> -	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
> -		return 0;
> +	int val;
>  
> -	phydev->speed = SPEED_UNKNOWN;
> -	phydev->duplex = DUPLEX_UNKNOWN;
> -	phydev->pause = 0;
> -	phydev->asym_pause = 0;
> +	val = phy_read(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL);
> +	if (val < 0)
> +		return val;
>  
> -	err = genphy_read_lpa(phydev);
> -	if (err < 0)
> -		return err;
> +	switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, val)) {
> +	case AT803X_SFC_MANUAL_MDI:
> +		phydev->mdix_ctrl = ETH_TP_MDI;
> +		break;
> +	case AT803X_SFC_MANUAL_MDIX:
> +		phydev->mdix_ctrl = ETH_TP_MDI_X;
> +		break;
> +	case AT803X_SFC_AUTOMATIC_CROSSOVER:
> +		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> +		break;
> +	}
>  
>  	/* Read the AT8035 PHY-Specific Status register, which indicates the
>  	 * speed and duplex that the PHY is actually using, irrespective of
>  	 * whether we are in autoneg mode or not.
>  	 */
> -	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
> -	if (ss < 0)
> -		return ss;
> +	val = phy_read(phydev, AT803X_SPECIFIC_STATUS);
> +	if (val < 0)
> +		return val;

What was actually wrong with ss?

Is this another case of just copying code from your other driver,
rather than cleanly extending the existing driver?

There are two many changes here all at once. Please break this patch
up. You are aiming for lots of small patches which are obviously
correct. Part of being obviously correct is having a good commit
message, and that gets much easier when a patch is small.

	 Andrew

Re: [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver

[Jie Luo]

On 8/30/2021 9:48 PM, Andrew Lunn wrote:
> On Mon, Aug 30, 2021 at 07:07:32PM +0800, Luo Jie wrote:
>> qca8081 is a single port ethernet phy chip that supports
>> 10/100/1000/2500 Mbps mode.
>>
>> Signed-off-by: Luo Jie <luoj@codeaurora.org>
>> ---
>>   drivers/net/phy/at803x.c | 389 ++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 338 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>> index ecae26f11aa4..2b3563ae152f 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>> @@ -33,10 +33,10 @@
>>   #define AT803X_SFC_DISABLE_JABBER		BIT(0)
>>   
>>   #define AT803X_SPECIFIC_STATUS			0x11
>> -#define AT803X_SS_SPEED_MASK			(3 << 14)
>> -#define AT803X_SS_SPEED_1000			(2 << 14)
>> -#define AT803X_SS_SPEED_100			(1 << 14)
>> -#define AT803X_SS_SPEED_10			(0 << 14)
>> +#define AT803X_SS_SPEED_MASK			GENMASK(15, 14)
>> +#define AT803X_SS_SPEED_1000			2
>> +#define AT803X_SS_SPEED_100			1
>> +#define AT803X_SS_SPEED_10			0
> This looks like an improvement, and nothing to do with qca8081. Please
> make it an separate patch.
will make it an separate patch in the next patch series.
>>   #define AT803X_SS_DUPLEX			BIT(13)
>>   #define AT803X_SS_SPEED_DUPLEX_RESOLVED		BIT(11)
>>   #define AT803X_SS_MDIX				BIT(6)
>> @@ -158,6 +158,8 @@
>>   #define QCA8337_PHY_ID				0x004dd036
>>   #define QCA8K_PHY_ID_MASK			0xffffffff
>>   
>> +#define QCA8081_PHY_ID				0x004dd101
>> +
> Maybe keep all the PHY_ID together?
will move it to make PHY_ID together in the next patch series.
>
>>   #define QCA8K_DEVFLAGS_REVISION_MASK		GENMASK(2, 0)
>>   
>>   #define AT803X_PAGE_FIBER			0
>> @@ -167,7 +169,73 @@
>>   #define AT803X_KEEP_PLL_ENABLED			BIT(0)
>>   #define AT803X_DISABLE_SMARTEEE			BIT(1)
>>   
>> @@ -711,11 +779,18 @@ static void at803x_remove(struct phy_device *phydev)
>>   
>>   static int at803x_get_features(struct phy_device *phydev)
>>   {
>> -	int err;
>> +	int val;
> Why? The driver pretty consistently uses err for return values which
> are errors.
will keep err unchanged in the next patch set.
>
>>   
>> -	err = genphy_read_abilities(phydev);
>> -	if (err)
>> -		return err;
>> +	val = genphy_read_abilities(phydev);
>> +	if (val)
>> +		return val;
>> +
>> +	if (at803x_match_phy_id(phydev, QCA8081_PHY_ID)) {
>> +		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
> You don't check if val indicates if there was an error.
thanks Andrew for the comment, will add the check here.
>
>> +
>> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported,
>> +				val & MDIO_PMA_NG_EXTABLE_2_5GBT);
>> +	}
>>   
>>   	if (!at803x_match_phy_id(phydev, ATH8031_PHY_ID))
>>   		return 0;
>> @@ -935,44 +1010,44 @@ static void at803x_link_change_notify(struct phy_device *phydev)
>>   	}
>>   }
>>   
>> -static int at803x_read_status(struct phy_device *phydev)
>> +static int at803x_read_specific_status(struct phy_device *phydev)
>>   {
>> -	int ss, err, old_link = phydev->link;
>> -
>> -	/* Update the link, but return if there was an error */
>> -	err = genphy_update_link(phydev);
>> -	if (err)
>> -		return err;
>> -
>> -	/* why bother the PHY if nothing can have changed */
>> -	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
>> -		return 0;
>> +	int val;
>>   
>> -	phydev->speed = SPEED_UNKNOWN;
>> -	phydev->duplex = DUPLEX_UNKNOWN;
>> -	phydev->pause = 0;
>> -	phydev->asym_pause = 0;
>> +	val = phy_read(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL);
>> +	if (val < 0)
>> +		return val;
>>   
>> -	err = genphy_read_lpa(phydev);
>> -	if (err < 0)
>> -		return err;
>> +	switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, val)) {
>> +	case AT803X_SFC_MANUAL_MDI:
>> +		phydev->mdix_ctrl = ETH_TP_MDI;
>> +		break;
>> +	case AT803X_SFC_MANUAL_MDIX:
>> +		phydev->mdix_ctrl = ETH_TP_MDI_X;
>> +		break;
>> +	case AT803X_SFC_AUTOMATIC_CROSSOVER:
>> +		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>> +		break;
>> +	}
>>   
>>   	/* Read the AT8035 PHY-Specific Status register, which indicates the
>>   	 * speed and duplex that the PHY is actually using, irrespective of
>>   	 * whether we are in autoneg mode or not.
>>   	 */
>> -	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
>> -	if (ss < 0)
>> -		return ss;
>> +	val = phy_read(phydev, AT803X_SPECIFIC_STATUS);
>> +	if (val < 0)
>> +		return val;
> What was actually wrong with ss?
>
> Is this another case of just copying code from your other driver,
> rather than cleanly extending the existing driver?
>
> There are two many changes here all at once. Please break this patch
> up. You are aiming for lots of small patches which are obviously
> correct. Part of being obviously correct is having a good commit
> message, and that gets much easier when a patch is small.
>
> 	 Andrew

Hi Andrew,

i separate the phy specific status into a new function 
at803x_read_specific_status, since this function

need to be used for both at803x phy driver and qca8081 phy driver.

i will break the patch into the minimal changes and provide the commit 
message in detail in the next

patch series.

thanks for your helpful comments.

[PATCH v1 3/3] net: phy: add cdt feature of qca8081 phy

[Luo Jie]

To perform CDT of qca8081 phy:
1. disable hibernation.
2. force phy working in MDI mode.
3. force phy working in 1000BASE-T mode.
4. configure the related thresholds.

Signed-off-by: Luo Jie <luoj@codeaurora.org>
---
 drivers/net/phy/at803x.c | 193 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 190 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 2b3563ae152f..131dca926bbe 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -173,6 +173,32 @@
 #define QCA808X_SS_SPEED_MASK			GENMASK(9, 7)
 #define QCA808X_SS_SPEED_2500			4
 
+/* Hibernation yields lower power consumpiton in contrast with normal operation mode.
+ * when the copper cable is unplugged, the PHY enters into hibernation mode in about 10s.
+ */
+#define QCA808X_DBG_AN_TEST			0xb
+#define QCA808X_HIBERNATION_EN			BIT(15)
+
+#define QCA808X_CDT_ENABLE_TEST			BIT(15)
+#define QCA808X_CDT_INTER_CHECK_DIS		BIT(13)
+#define QCA808X_CDT_LENGTH_UNIT			BIT(10)
+
+#define QCA808X_MMD3_CDT_STATUS			0x8064
+#define QCA808X_MMD3_CDT_DIAG_PAIR_A		0x8065
+#define QCA808X_MMD3_CDT_DIAG_PAIR_B		0x8066
+#define QCA808X_MMD3_CDT_DIAG_PAIR_C		0x8067
+#define QCA808X_MMD3_CDT_DIAG_PAIR_D		0x8068
+#define QCA808X_CDT_DIAG_LENGTH			GENMASK(7, 0)
+
+#define QCA808X_CDT_CODE_PAIR_A			GENMASK(15, 12)
+#define QCA808X_CDT_CODE_PAIR_B			GENMASK(11, 8)
+#define QCA808X_CDT_CODE_PAIR_C			GENMASK(7, 4)
+#define QCA808X_CDT_CODE_PAIR_D			GENMASK(3, 0)
+#define QCA808X_CDT_STATUS_STAT_FAIL		0
+#define QCA808X_CDT_STATUS_STAT_NORMAL		1
+#define QCA808X_CDT_STATUS_STAT_OPEN		2
+#define QCA808X_CDT_STATUS_STAT_SHORT		3
+
 /* Conifg seed */
 #define QCA808X_PHY_DEBUG_LOCAL_SEED		9
 #define QCA808X_MASTER_SLAVE_SEED_ENABLE	BIT(1)
@@ -1302,8 +1328,14 @@ static int at803x_cdt_start(struct phy_device *phydev, int pair)
 {
 	u16 cdt;
 
-	cdt = FIELD_PREP(AT803X_CDT_MDI_PAIR_MASK, pair) |
-	      AT803X_CDT_ENABLE_TEST;
+	/* qca8081 takes the different bit 15 to enable CDT test */
+	if (at803x_match_phy_id(phydev, QCA8081_PHY_ID))
+		cdt = QCA808X_CDT_ENABLE_TEST |
+			QCA808X_CDT_LENGTH_UNIT |
+			QCA808X_CDT_INTER_CHECK_DIS;
+	else
+		cdt = FIELD_PREP(AT803X_CDT_MDI_PAIR_MASK, pair) |
+			AT803X_CDT_ENABLE_TEST;
 
 	return phy_write(phydev, AT803X_CDT, cdt);
 }
@@ -1311,10 +1343,16 @@ static int at803x_cdt_start(struct phy_device *phydev, int pair)
 static int at803x_cdt_wait_for_completion(struct phy_device *phydev)
 {
 	int val, ret;
+	u16 cdt_en;
+
+	if (at803x_match_phy_id(phydev, QCA8081_PHY_ID))
+		cdt_en = QCA808X_CDT_ENABLE_TEST;
+	else
+		cdt_en = AT803X_CDT_ENABLE_TEST;
 
 	/* One test run takes about 25ms */
 	ret = phy_read_poll_timeout(phydev, AT803X_CDT, val,
-				    !(val & AT803X_CDT_ENABLE_TEST),
+				    !(val & cdt_en),
 				    30000, 100000, true);
 
 	return ret < 0 ? ret : 0;
@@ -1438,6 +1476,153 @@ static int qca83xx_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static bool qca808x_cdt_fault_length_valid(int cdt_code)
+{
+	switch (cdt_code) {
+	case QCA808X_CDT_STATUS_STAT_SHORT:
+	case QCA808X_CDT_STATUS_STAT_OPEN:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static int qca808x_cable_test_result_trans(int cdt_code)
+{
+	switch (cdt_code) {
+	case QCA808X_CDT_STATUS_STAT_NORMAL:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case QCA808X_CDT_STATUS_STAT_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	case QCA808X_CDT_STATUS_STAT_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case QCA808X_CDT_STATUS_STAT_FAIL:
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static int qca808x_cdt_fault_length(struct phy_device *phydev, int pair)
+{
+	int val;
+	u32 cdt_length_reg = 0;
+
+	switch (pair) {
+	case ETHTOOL_A_CABLE_PAIR_A:
+		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_A;
+		break;
+	case ETHTOOL_A_CABLE_PAIR_B:
+		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_B;
+		break;
+	case ETHTOOL_A_CABLE_PAIR_C:
+		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_C;
+		break;
+	case ETHTOOL_A_CABLE_PAIR_D:
+		cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_D;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, cdt_length_reg);
+	if (val < 0)
+		return val;
+
+	return (FIELD_GET(QCA808X_CDT_DIAG_LENGTH, val) * 824) / 10;
+}
+
+static int qca808x_cable_test_start(struct phy_device *phydev)
+{
+	int ret;
+
+	/* perform CDT with the following configs:
+	 * 1. disable hibernation.
+	 * 2. force PHY working in MDI mode.
+	 * 3. for PHY working in 1000BaseT.
+	 * 4. configure the threshold.
+	 */
+
+	ret = at803x_debug_reg_mask(phydev, QCA808X_DBG_AN_TEST, QCA808X_HIBERNATION_EN, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = at803x_config_mdix(phydev, ETH_TP_MDI);
+	if (ret < 0)
+		return ret;
+
+	/* Force 1000base-T needs to configure PMA/PMD and MII_BMCR */
+	phydev->duplex = DUPLEX_FULL;
+	phydev->speed = SPEED_1000;
+	ret = genphy_c45_pma_setup_forced(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = genphy_setup_forced(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* configure the thresholds for open, short, pair ok test */
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8074, 0xc040);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8076, 0xc040);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8077, 0xa060);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8078, 0xc050);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807a, 0xc060);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, 0x807e, 0xb060);
+
+	return 0;
+}
+
+static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
+{
+	int ret, val;
+	int pair_a, pair_b, pair_c, pair_d;
+
+	*finished = false;
+
+	ret = at803x_cdt_start(phydev, 0);
+	if (ret)
+		return ret;
+
+	ret = at803x_cdt_wait_for_completion(phydev);
+	if (ret)
+		return ret;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, QCA808X_MMD3_CDT_STATUS);
+	if (val < 0)
+		return val;
+
+	pair_a = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, val);
+	pair_b = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, val);
+	pair_c = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, val);
+	pair_d = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, val);
+
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+				qca808x_cable_test_result_trans(pair_a));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
+				qca808x_cable_test_result_trans(pair_b));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
+				qca808x_cable_test_result_trans(pair_c));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
+				qca808x_cable_test_result_trans(pair_d));
+
+	if (qca808x_cdt_fault_length_valid(pair_a))
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A,
+				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A));
+	if (qca808x_cdt_fault_length_valid(pair_b))
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B,
+				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B));
+	if (qca808x_cdt_fault_length_valid(pair_c))
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C,
+				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C));
+	if (qca808x_cdt_fault_length_valid(pair_d))
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D,
+				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D));
+
+	*finished = true;
+
+	return 0;
+}
+
 static int qca808x_phy_fast_retrain_cfg(struct phy_device *phydev)
 {
 	int ret;
@@ -1714,6 +1899,8 @@ static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.set_wol		= at803x_set_wol,
 	.get_wol		= at803x_get_wol,
+	.cable_test_start	= qca808x_cable_test_start,
+	.cable_test_get_status	= qca808x_cable_test_get_status,
 	.config_init		= qca808x_config_init,
 	.read_status		= qca808x_read_status,
 	.soft_reset		= qca808x_soft_reset,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project