This patch series add the qca8081 ethernet phy driver support, which
improve the wol feature, leverage at803x phy driver and add the fast
retrain, master/slave seed and CDT feature.
Changes in v3:
* correct a typo "excpet".
* remove the suffix "PHY" from phy name.
Changes in v2:
* add definitions of fast retrain related registers in mdio.h.
* break up the patch into small patches.
* improve the at803x legacy code.
Changes in v1:
* merge qca8081 phy driver into at803x.
* add cdt feature.
* leverage at803x phy driver helpers.
Luo Jie (13):
net: phy: at803x: replace AT803X_DEVICE_ADDR with MDIO_MMD_PCS
net: phy: at803x: use phy_modify()
net: phy: at803x: improve the WOL feature
net: phy: at803x: use GENMASK() for speed status
net: phy: add qca8081 ethernet phy driver
net: phy: add qca8081 read_status
net: phy: add qca8081 get_features
net: phy: add qca8081 config_aneg
net: phy: add constants for fast retrain related register
net: phy: add qca8081 config_init
net: phy: add qca8081 soft_reset and enable master/slave seed
net: phy: adjust qca8081 master/slave seed value if link down
net: phy: add qca8081 cdt feature
drivers/net/phy/at803x.c | 572 +++++++++++++++++++++++++++++++++++---
include/uapi/linux/mdio.h | 10 +
2 files changed, 536 insertions(+), 46 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Replace AT803X_DEVICE_ADDR with MDIO_MMD_PCS defined in mdio.h.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
1. Separate the function at803x_read_specific_status from
the at803x_read_status, since it can be reused by the
read_status of qca8081 phy driver excepting adding the
2500M speed.
2. Add the qca8081 read_status function qca808x_read_status.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 95 ++++++++++++++++++++++++++++++----------
1 file changed, 73 insertions(+), 22 deletions(-)
@@ -934,27 +937,9 @@ static void at803x_link_change_notify(struct phy_device *phydev)}}-staticintat803x_read_status(structphy_device*phydev)+staticintat803x_read_specific_status(structphy_device*phydev){-intss,err,old_link=phydev->link;--/* Update the link, but return if there was an error */-err=genphy_update_link(phydev);-if(err)-returnerr;--/* why bother the PHY if nothing can have changed */-if(phydev->autoneg==AUTONEG_ENABLE&&old_link&&phydev->link)-return0;--phydev->speed=SPEED_UNKNOWN;-phydev->duplex=DUPLEX_UNKNOWN;-phydev->pause=0;-phydev->asym_pause=0;--err=genphy_read_lpa(phydev);-if(err<0)-returnerr;+intss;/* Read the AT8035 PHY-Specific Status register, which indicates the*speedandduplexthatthePHYisactuallyusing,irrespectiveof
@@ -965,13 +950,19 @@ static int at803x_read_status(struct phy_device *phydev)returnss;if(ss&AT803X_SS_SPEED_DUPLEX_RESOLVED){-intsfc;+intsfc,speed;sfc=phy_read(phydev,AT803X_SPECIFIC_FUNCTION_CONTROL);if(sfc<0)returnsfc;-switch(FIELD_GET(AT803X_SS_SPEED_MASK,ss)){+/* qca8081 takes the different bits for speed value from at803x */+if(phydev->drv->phy_id==QCA8081_PHY_ID)+speed=FIELD_GET(QCA808X_SS_SPEED_MASK,ss);+else+speed=FIELD_GET(AT803X_SS_SPEED_MASK,ss);++switch(speed){caseAT803X_SS_SPEED_10:phydev->speed=SPEED_10;break;
@@ -981,6 +972,9 @@ static int at803x_read_status(struct phy_device *phydev)caseAT803X_SS_SPEED_1000:phydev->speed=SPEED_1000;break;+caseQCA808X_SS_SPEED_2500:+phydev->speed=SPEED_2500;+break;}if(ss&AT803X_SS_DUPLEX)phydev->duplex=DUPLEX_FULL;
@@ -1005,6 +999,35 @@ static int at803x_read_status(struct phy_device *phydev)}}+return0;+}++staticintat803x_read_status(structphy_device*phydev)+{+interr,old_link=phydev->link;++/* Update the link, but return if there was an error */+err=genphy_update_link(phydev);+if(err)+returnerr;++/* why bother the PHY if nothing can have changed */+if(phydev->autoneg==AUTONEG_ENABLE&&old_link&&phydev->link)+return0;++phydev->speed=SPEED_UNKNOWN;+phydev->duplex=DUPLEX_UNKNOWN;+phydev->pause=0;+phydev->asym_pause=0;++err=genphy_read_lpa(phydev);+if(err<0)+returnerr;++err=at803x_read_specific_status(phydev);+if(err<0)+returnerr;+if(phydev->autoneg==AUTONEG_ENABLE&&phydev->autoneg_complete)phy_resolve_aneg_pause(phydev);
Use GENMASK() for the current speed value.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
qca8081 is a single port ethernet phy chip that supports
10/100/1000/2500 Mbps mode.
Add the basic phy driver features, and reuse the at803x
phy driver functions.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
The wol feature is controlled by the MMD3.8012 bit5,
need to set this bit when the wol function is enabled.
The reg18 bit0 is for enabling WOL interrupt, when wol
occurs, the wol interrupt status reg19 bit0 is set to 1.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
@@ -327,7 +329,6 @@ static int at803x_set_wol(struct phy_device *phydev,structnet_device*ndev=phydev->attached_dev;constu8*mac;intret;-u32value;unsignedinti;constunsignedintoffsets[]={AT803X_LOC_MAC_ADDR_32_47_OFFSET,
@@ -348,18 +349,29 @@ static int at803x_set_wol(struct phy_device *phydev,phy_write_mmd(phydev,MDIO_MMD_PCS,offsets[i],mac[(i*2)+1]|(mac[(i*2)]<<8));+/* Enable WOL function */+ret=phy_modify_mmd(phydev,MDIO_MMD_PCS,AT803X_PHY_MMD3_WOL_CTRL,+0,AT803X_WOL_EN);+if(ret)+returnret;+/* Enable WOL interrupt */ret=phy_modify(phydev,AT803X_INTR_ENABLE,0,AT803X_INTR_ENABLE_WOL);if(ret)returnret;-value=phy_read(phydev,AT803X_INTR_STATUS);}else{+/* Disable WoL function */+ret=phy_modify_mmd(phydev,MDIO_MMD_PCS,AT803X_PHY_MMD3_WOL_CTRL,+AT803X_WOL_EN,0);+if(ret)+returnret;+/* Disable WOL interrupt */ret=phy_modify(phydev,AT803X_INTR_ENABLE,AT803X_INTR_ENABLE_WOL,0);if(ret)returnret;-value=phy_read(phydev,AT803X_INTR_STATUS);}-returnret;+/* Clear WOL status */+returnphy_read(phydev,AT803X_INTR_STATUS);}staticvoidat803x_get_wol(structphy_device*phydev,
@@ -1084,7 +1084,30 @@ static int at803x_config_aneg(struct phy_device *phydev)returnret;}-returngenphy_config_aneg(phydev);+/* Do not restart auto-negotiation by setting ret to 0 defautly,+*whencalling__genphy_config_aneglater.+*/+ret=0;++if(phydev->drv->phy_id==QCA8081_PHY_ID){+intphy_ctrl=0;++/* The reg MII_BMCR also needs to be configured for force mode, the+*genphy_config_anegisalsoneeded.+*/+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)+returnret;+}++return__genphy_config_aneg(phydev,ret);}staticintat803x_get_downshift(structphy_device*phydev,u8*d)
Add constants for 2.5G and 5G fast retrain capability
in 10G AN control register, fast retrain status and
control register and THP bypass register into mdio.h.
Signed-off-by: Luo Jie <redacted>
---
include/uapi/linux/mdio.h | 10 ++++++++++
1 file changed, 10 insertions(+)
1. The master/slave seed needs to be updated when the link can't
be created.
2. The case where two qca8081 PHYs are connected each other and
master/slave seed is generated as the same value also needs
to be considered, so adding this code change into read_status
instead of link_change_notify.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
@@ -1556,6 +1556,22 @@ static int qca808x_read_status(struct phy_device *phydev)elsephydev->interface=PHY_INTERFACE_MODE_SMII;+/* generate seed as a lower random value to make PHY linked as SLAVE easily,+*exceptformaster/slaveconfigurationfaultdetected.+*thereasonfornotputtingthiscodeintothefunctionlink_change_notifyis+*thecornercasewherethelinkpartnerisalsotheqca8081PHYandtheseed+*valueisconfiguredasthesamevalue,thelinkcan'tbeupandnolinkchange+*occurs.+*/+if(!phydev->link){+if(phydev->master_slave_state==MASTER_SLAVE_STATE_ERR){+qca808x_phy_ms_seed_enable(phydev,false);+}else{+qca808x_phy_ms_random_seed_set(phydev);+qca808x_phy_ms_seed_enable(phydev,true);+}+}+return0;}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <redacted>
---
drivers/net/phy/at803x.c | 193 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 190 insertions(+), 3 deletions(-)
@@ -1294,8 +1320,14 @@ static int at803x_cdt_start(struct phy_device *phydev, int pair){u16cdt;-cdt=FIELD_PREP(AT803X_CDT_MDI_PAIR_MASK,pair)|-AT803X_CDT_ENABLE_TEST;+/* qca8081 takes the different bit 15 to enable CDT test */+if(phydev->drv->phy_id==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;returnphy_write(phydev,AT803X_CDT,cdt);}
@@ -1303,10 +1335,16 @@ static int at803x_cdt_start(struct phy_device *phydev, int pair)staticintat803x_cdt_wait_for_completion(structphy_device*phydev){intval,ret;+u16cdt_en;++if(phydev->drv->phy_id==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);returnret<0?ret:0;
@@ -1586,6 +1624,153 @@ static int qca808x_soft_reset(struct phy_device *phydev)returnqca808x_phy_ms_seed_enable(phydev,true);}+staticboolqca808x_cdt_fault_length_valid(intcdt_code)+{+switch(cdt_code){+caseQCA808X_CDT_STATUS_STAT_SHORT:+caseQCA808X_CDT_STATUS_STAT_OPEN:+returntrue;+default:+returnfalse;+}+}++staticintqca808x_cable_test_result_trans(intcdt_code)+{+switch(cdt_code){+caseQCA808X_CDT_STATUS_STAT_NORMAL:+returnETHTOOL_A_CABLE_RESULT_CODE_OK;+caseQCA808X_CDT_STATUS_STAT_SHORT:+returnETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;+caseQCA808X_CDT_STATUS_STAT_OPEN:+returnETHTOOL_A_CABLE_RESULT_CODE_OPEN;+caseQCA808X_CDT_STATUS_STAT_FAIL:+default:+returnETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;+}+}++staticintqca808x_cdt_fault_length(structphy_device*phydev,intpair)+{+intval;+u32cdt_length_reg=0;++switch(pair){+caseETHTOOL_A_CABLE_PAIR_A:+cdt_length_reg=QCA808X_MMD3_CDT_DIAG_PAIR_A;+break;+caseETHTOOL_A_CABLE_PAIR_B:+cdt_length_reg=QCA808X_MMD3_CDT_DIAG_PAIR_B;+break;+caseETHTOOL_A_CABLE_PAIR_C:+cdt_length_reg=QCA808X_MMD3_CDT_DIAG_PAIR_C;+break;+caseETHTOOL_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)+returnval;++return(FIELD_GET(QCA808X_CDT_DIAG_LENGTH,val)*824)/10;+}++staticintqca808x_cable_test_start(structphy_device*phydev)+{+intret;++/* perform CDT with the following configs:+*1.disablehibernation.+*2.forcePHYworkinginMDImode.+*3.forPHYworkingin1000BaseT.+*4.configurethethreshold.+*/++ret=at803x_debug_reg_mask(phydev,QCA808X_DBG_AN_TEST,QCA808X_HIBERNATION_EN,0);+if(ret<0)+returnret;++ret=at803x_config_mdix(phydev,ETH_TP_MDI);+if(ret<0)+returnret;++/* 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)+returnret;++ret=genphy_setup_forced(phydev);+if(ret<0)+returnret;++/* 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);++return0;+}++staticintqca808x_cable_test_get_status(structphy_device*phydev,bool*finished)+{+intret,val;+intpair_a,pair_b,pair_c,pair_d;++*finished=false;++ret=at803x_cdt_start(phydev,0);+if(ret)+returnret;++ret=at803x_cdt_wait_for_completion(phydev);+if(ret)+returnret;++val=phy_read_mmd(phydev,MDIO_MMD_PCS,QCA808X_MMD3_CDT_STATUS);+if(val<0)+returnval;++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;++return0;+}+staticstructphy_driverat803x_driver[]={{/* Qualcomm Atheros AR8035 */
@@ -1379,6 +1424,79 @@ static int qca83xx_config_init(struct phy_device *phydev)return0;}+staticintqca808x_phy_fast_retrain_config(structphy_device*phydev)+{+intret;++ret=phy_write_mmd(phydev,MDIO_MMD_AN,MDIO_AN_10GBT_CTRL,+MDIO_AN_10GBT_CTRL_ADV2_5G|+MDIO_AN_10GBT_CTRL_ADVFSRT2_5G|+MDIO_AN_10GBT_CTRL_ADVLPTIMING);+if(ret)+returnret;++ret=phy_write_mmd(phydev,MDIO_MMD_PMAPMD,MDIO_PMA_10GBR_FSRT_CSR,+MDIO_PMA_10GBR_FSRT_ENABLE);+if(ret)+returnret;++ret=phy_write_mmd(phydev,MDIO_MMD_AN,MDIO_AN_CTRL2,MDIO_AN_THP_BP2_5GT);+if(ret)+returnret;++phy_write_mmd(phydev,MDIO_MMD_AN,QCA808X_PHY_MMD7_TOP_OPTION1,+QCA808X_TOP_OPTION1_DATA);++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_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);++return0;+}++staticintqca808x_config_init(structphy_device*phydev)+{+intret;++/* 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)+returnret;++/* 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)+returnret;++/* Config the fast retrain for the link 2500M */+ret=qca808x_phy_fast_retrain_config(phydev);+if(ret)+returnret;++/* Configure adc threshold as 100mv for the link 10M */+returnat803x_debug_reg_mask(phydev,QCA808X_PHY_DEBUG_ADC_THRESHOLD,+QCA808X_ADC_THRESHOLD_MASK,QCA808X_ADC_THRESHOLD_100MV);+}+staticintqca808x_read_status(structphy_device*phydev){intret;
qca8081 phy is a single port phy, configure
phy the lower seed value to make it linked as slave
mode easier.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 47 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
@@ -1471,6 +1477,25 @@ static int qca808x_phy_fast_retrain_config(struct phy_device *phydev)return0;}+staticintqca808x_phy_ms_random_seed_set(structphy_device*phydev)+{+u16seed_value=(prandom_u32()%QCA808X_MASTER_SLAVE_SEED_RANGE)<<2;++returnat803x_debug_reg_mask(phydev,QCA808X_PHY_DEBUG_LOCAL_SEED,+QCA808X_MASTER_SLAVE_SEED_CFG,seed_value);+}++staticintqca808x_phy_ms_seed_enable(structphy_device*phydev,boolenable)+{+u16seed_enable=0;++if(enable)+seed_enable=QCA808X_MASTER_SLAVE_SEED_ENABLE;++returnat803x_debug_reg_mask(phydev,QCA808X_PHY_DEBUG_LOCAL_SEED,+QCA808X_MASTER_SLAVE_SEED_ENABLE,seed_enable);+}+staticintqca808x_config_init(structphy_device*phydev){intret;
@@ -1492,6 +1517,16 @@ static int qca808x_config_init(struct phy_device *phydev)if(ret)returnret;+/* Configure lower ramdom seed to make phy linked as slave mode */+ret=qca808x_phy_ms_random_seed_set(phydev);+if(ret)+returnret;++/* Enable seed */+ret=qca808x_phy_ms_seed_enable(phydev,true);+if(ret)+returnret;+/* Configure adc threshold as 100mv for the link 10M */returnat803x_debug_reg_mask(phydev,QCA808X_PHY_DEBUG_ADC_THRESHOLD,QCA808X_ADC_THRESHOLD_MASK,QCA808X_ADC_THRESHOLD_100MV);
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-10-18 18:41:12
quoted hunk
@@ -348,18 +349,29 @@ static int at803x_set_wol(struct phy_device *phydev, phy_write_mmd(phydev, MDIO_MMD_PCS, offsets[i], mac[(i * 2) + 1] | (mac[(i * 2)] << 8));+ /* Enable WOL function */+ ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,+ 0, AT803X_WOL_EN);+ if (ret)+ return ret;+ /* Enable WOL interrupt */ ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL); if (ret) return ret;- value = phy_read(phydev, AT803X_INTR_STATUS); } else {+ /* Disable WoL function */+ ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,+ AT803X_WOL_EN, 0);+ if (ret)+ return ret;+ /* Disable WOL interrupt */ ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0); if (ret) return ret;- value = phy_read(phydev, AT803X_INTR_STATUS); }- return ret;+ /* Clear WOL status */+ return phy_read(phydev, AT803X_INTR_STATUS);
It looks like you could be clearing other interrupt bits which have
not been serviced yet. Is it possible to clear just WoL?
Also, you are returning the contents of the interrupt status register?
You should probably be returning 0 if the read was successful.
Andrew
@@ -1084,7 +1084,30 @@ static int at803x_config_aneg(struct phy_device *phydev)returnret;}-returngenphy_config_aneg(phydev);+/* Do not restart auto-negotiation by setting ret to 0 defautly,+*whencalling__genphy_config_aneglater.+*/+ret=0;++if(phydev->drv->phy_id==QCA8081_PHY_ID){+intphy_ctrl=0;++/* The reg MII_BMCR also needs to be configured for force mode, the+*genphy_config_anegisalsoneeded.+*/+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);
Does the PHY also have MDIO_MMD_AN, MDIO_AN_ADVERTISE ? I'm wondering
if you can use genphy_c45_an_config_aneg()
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-10-18 21:42:14
quoted hunk
+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);+
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-10-18 22:04:16
On Mon, Oct 18, 2021 at 11:33:32AM +0800, Luo Jie wrote:
quoted hunk
1. The master/slave seed needs to be updated when the link can't
be created.
2. The case where two qca8081 PHYs are connected each other and
master/slave seed is generated as the same value also needs
to be considered, so adding this code change into read_status
instead of link_change_notify.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
@@ -1556,6 +1556,22 @@ static int qca808x_read_status(struct phy_device *phydev)elsephydev->interface=PHY_INTERFACE_MODE_SMII;+/* generate seed as a lower random value to make PHY linked as SLAVE easily,+*exceptformaster/slaveconfigurationfaultdetected.+*thereasonfornotputtingthiscodeintothefunctionlink_change_notifyis+*thecornercasewherethelinkpartnerisalsotheqca8081PHYandtheseed+*valueisconfiguredasthesamevalue,thelinkcan'tbeupandnolinkchange+*occurs.+*/+if(!phydev->link){+if(phydev->master_slave_state==MASTER_SLAVE_STATE_ERR){+qca808x_phy_ms_seed_enable(phydev,false);+}else{+qca808x_phy_ms_random_seed_set(phydev);+qca808x_phy_ms_seed_enable(phydev,true);+}+}
Are you assuming here that the status is polled once a second, and
each poll you choose a new seed and see if it succeeds? What happens
when interrupts are used, not polling?
Andrew
+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);+
Could genphy_c45_read_lpa() be used here?
Andrew
Hi Andrew,
Thanks for the comments, the MDIO_STAT1 of PHY does not follow the standard, bit0~bit6 of MDIO_STAT1 are
always 0, genphy_c45_read_lpa can't be used.
@@ -1084,7 +1084,30 @@ static int at803x_config_aneg(struct phy_device *phydev)returnret;}-returngenphy_config_aneg(phydev);+/* Do not restart auto-negotiation by setting ret to 0 defautly,+*whencalling__genphy_config_aneglater.+*/+ret=0;++if(phydev->drv->phy_id==QCA8081_PHY_ID){+intphy_ctrl=0;++/* The reg MII_BMCR also needs to be configured for force mode, the+*genphy_config_anegisalsoneeded.+*/+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);
Does the PHY also have MDIO_MMD_AN, MDIO_AN_ADVERTISE ? I'm wondering
if you can use genphy_c45_an_config_aneg()
Andrew
Thanks Andrew for this comments, since the PHY does not have the regiser MDIO_AN_ADVERTISE,
genphy_c45_an_config_aneg can't be used here.
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-10-19 12:29:15
Hi Andrew,
when this register AT803X_INTR_STATUS bits are cleared after read, we can't
clear only WOL interrupt here.
O.K. But you do have the value of the interrupt status register. So
you could call phy_trigger_machine(phydev) if there are any other
interrupt pending. They won't get lost that way.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-10-19 12:32:09
On Tue, Oct 19, 2021 at 08:10:15PM +0800, Jie Luo wrote:
On 10/19/2021 5:42 AM, Andrew Lunn wrote:
quoted
quoted
+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);+
Could genphy_c45_read_lpa() be used here?
Andrew
Hi Andrew,
Thanks for the comments, the MDIO_STAT1 of PHY does not follow the
standard, bit0~bit6 of MDIO_STAT1 are
always 0, genphy_c45_read_lpa can't be used.
O.K. It is a shame the hardware partially follow the standard, but
breaks it as well. Why go to the effort of partially following it,
when you don't gain anything from it because you need custom code
anyway?
Andrew
Hi Andrew,
when this register AT803X_INTR_STATUS bits are cleared after read, we can't
clear only WOL interrupt here.
O.K. But you do have the value of the interrupt status register. So
you could call phy_trigger_machine(phydev) if there are any other
interrupt pending. They won't get lost that way.
Andrew
This make sense, thanks for this comment, will add it in the next patch set.
On Tue, Oct 19, 2021 at 08:10:15PM +0800, Jie Luo wrote:
quoted
On 10/19/2021 5:42 AM, Andrew Lunn wrote:
quoted
quoted
+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);+
Could genphy_c45_read_lpa() be used here?
Andrew
Hi Andrew,
Thanks for the comments, the MDIO_STAT1 of PHY does not follow the
standard, bit0~bit6 of MDIO_STAT1 are
always 0, genphy_c45_read_lpa can't be used.
O.K. It is a shame the hardware partially follow the standard, but
breaks it as well. Why go to the effort of partially following it,
when you don't gain anything from it because you need custom code
anyway?
Andrew
Hi Andrew,
Thanks for the suggestion. qca8081 PHY indeed add 2.5G capability based on the
general 1G PHY, i will feedback this to the HW design team, thanks for this comments.
@@ -719,6 +719,15 @@ static int at803x_get_features(struct phy_device *phydev)if(err)returnerr;+if(phydev->drv->phy_id==QCA8081_PHY_ID){+err=phy_read_mmd(phydev,MDIO_MMD_PMAPMD,MDIO_PMA_NG_EXTABLE);+if(err<0)+returnerr;++linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,phydev->supported,+err&MDIO_PMA_NG_EXTABLE_2_5GBT);+}
genphy_c45_pma_read_abilities()?
Andrew
Hi Andrew,
Thanks for this comment, if we use genphy_c45_pma_read_abilities here, the ETHTOOL_LINK_MODE_Autoneg_BIT
will be lost, since MDIO_MMD_AN.MDIO_STAT1 does not have bit MDIO_AN_STAT1_ABLE.
@@ -719,6 +719,15 @@ static int at803x_get_features(struct phy_device *phydev)if(err)returnerr;+if(phydev->drv->phy_id==QCA8081_PHY_ID){+err=phy_read_mmd(phydev,MDIO_MMD_PMAPMD,MDIO_PMA_NG_EXTABLE);+if(err<0)+returnerr;++linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,phydev->supported,+err&MDIO_PMA_NG_EXTABLE_2_5GBT);+}
genphy_c45_pma_read_abilities()?
Andrew
Hi Andrew,
Thanks for this comment, if we use genphy_c45_pma_read_abilities here, the
ETHTOOL_LINK_MODE_Autoneg_BIT
will be lost, since MDIO_MMD_AN.MDIO_STAT1 does not have bit
MDIO_AN_STAT1_ABLE.
Yes, if your PHY breaks the standard, the helpers are not much use,
that assume standard compliment PHYs.
Andrew
On Mon, Oct 18, 2021 at 11:33:32AM +0800, Luo Jie wrote:
quoted
1. The master/slave seed needs to be updated when the link can't
be created.
2. The case where two qca8081 PHYs are connected each other and
master/slave seed is generated as the same value also needs
to be considered, so adding this code change into read_status
instead of link_change_notify.
Signed-off-by: Luo Jie <redacted>
---
drivers/net/phy/at803x.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
@@ -1556,6 +1556,22 @@ static int qca808x_read_status(struct phy_device *phydev)elsephydev->interface=PHY_INTERFACE_MODE_SMII;+/* generate seed as a lower random value to make PHY linked as SLAVE easily,+*exceptformaster/slaveconfigurationfaultdetected.+*thereasonfornotputtingthiscodeintothefunctionlink_change_notifyis+*thecornercasewherethelinkpartnerisalsotheqca8081PHYandtheseed+*valueisconfiguredasthesamevalue,thelinkcan'tbeupandnolinkchange+*occurs.+*/+if(!phydev->link){+if(phydev->master_slave_state==MASTER_SLAVE_STATE_ERR){+qca808x_phy_ms_seed_enable(phydev,false);+}else{+qca808x_phy_ms_random_seed_set(phydev);+qca808x_phy_ms_seed_enable(phydev,true);+}+}
Are you assuming here that the status is polled once a second, and
each poll you choose a new seed and see if it succeeds? What happens
when interrupts are used, not polling?
Andrew
Hi Andrew,
yes, this code assumes that the PHY POLL is used, and choose a new random seed value
on each poll if no link is created.
when the interrupts is used, this corner case seems can't be covered since there is
no related interrupt occurs when the seed is configured as same value.