Since we already have 5.13-rc3, I assume http://vger.kernel.org/~davem/net-next.html
is out of date.
changes v3:
- remove RFC tag
changes v2:
- use generic MII_* defines where possible
- rework phylink validate
- remove phylink get state function
- reorder cabletest patches to make PHY flag patch in the right order
- fix MDI-X detection
This patches provide support for cable testing on the ksz886x switches.
Since it has one special port, we needed to add phylink with validation
and extra quirk for the PHY to signal, that one port will not provide
valid cable testing reports.
Michael Grzeschik (2):
net: phy: micrel: move phy reg offsets to common header
net: dsa: microchip: ksz8795: add phylink support
Oleksij Rempel (7):
net: phy: micrel: use consistent indention after define
net: phy: micrel: apply resume errata workaround for ksz8873 and
ksz8863
net: phy/dsa micrel/ksz886x add MDI-X support
net: phy: micrel: ksz8081 add MDI-X support
net: dsa: microchip: ksz8795: add LINK_MD register support
net: dsa: dsa_slave_phy_connect(): extend phy's flags with port
specific phy flags
net: phy: micrel: ksz886x/ksz8081: add cabletest support
drivers/net/dsa/microchip/ksz8795.c | 218 +++++++++----
drivers/net/dsa/microchip/ksz8795_reg.h | 67 +---
drivers/net/ethernet/micrel/ksz884x.c | 105 +-----
drivers/net/phy/micrel.c | 403 +++++++++++++++++++++++-
drivers/net/phy/phylink.c | 2 +-
include/linux/micrel_phy.h | 16 +
net/dsa/slave.c | 4 +
7 files changed, 588 insertions(+), 227 deletions(-)
--
2.29.2
This patch extends the flags of the phy that's being connected with the
port specific flags of the switch port.
This is needed to handle a port specific erratum of the KSZ8873 switch,
which is added in a later patch.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/phy/phylink.c | 2 +-
net/dsa/slave.c | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
This patch changes the indention to one space between "#define" and the
macro.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/phy/micrel.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
The ksz8873 and ksz8863 switches are affected by following errata:
| "Receiver error in 100BASE-TX mode following Soft Power Down"
|
| Some KSZ8873 devices may exhibit receiver errors after transitioning
| from Soft Power Down mode to Normal mode, as controlled by register 195
| (0xC3) bits [1:0]. When exiting Soft Power Down mode, the receiver
| blocks may not start up properly, causing the PHY to miss data and
| exhibit erratic behavior. The problem may appear on either port 1 or
| port 2, or both ports. The problem occurs only for 100BASE-TX, not
| 10BASE-T.
|
| END USER IMPLICATIONS
| When the failure occurs, the following symptoms are seen on the affected
| port(s):
| - The port is able to link
| - LED0 blinks, even when there is no traffic
| - The MIB counters indicate receive errors (Rx Fragments, Rx Symbol
| Errors, Rx CRC Errors, Rx Alignment Errors)
| - Only a small fraction of packets is correctly received and forwarded
| through the switch. Most packets are dropped due to receive errors.
|
| The failing condition cannot be corrected by the following:
| - Removing and reconnecting the cable
| - Hardware reset
| - Software Reset and PCS Reset bits in register 67 (0x43)
|
| Work around:
| The problem can be corrected by setting and then clearing the Port Power
| Down bits (registers 29 (0x1D) and 45 (0x2D), bit 3). This must be done
| separately for each affected port after returning from Soft Power Down
| Mode to Normal Mode. The following procedure will ensure no further
| issues due to this erratum. To enter Soft Power Down Mode, set register
| 195 (0xC3), bits [1:0] = 10.
|
| To exit Soft Power Down Mode, follow these steps:
| 1. Set register 195 (0xC3), bits [1:0] = 00 // Exit soft power down mode
| 2. Wait 1ms minimum
| 3. Set register 29 (0x1D), bit [3] = 1 // Enter PHY port 1 power down mode
| 4. Set register 29 (0x1D), bit [3] = 0 // Exit PHY port 1 power down mode
| 5. Set register 45 (0x2D), bit [3] = 1 // Enter PHY port 2 power down mode
| 6. Set register 45 (0x2D), bit [3] = 0 // Exit PHY port 2 power down mode
This patch implements steps 2...6 of the suggested workaround. The first
step needs to be implemented in the switch driver.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/phy/micrel.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
From: Michael Grzeschik <redacted>
This patch adds the phylink support to the ksz8795 driver to provide
configuration exceptions on quirky KSZ8863 and KSZ8873 ports.
Signed-off-by: Michael Grzeschik <redacted>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/dsa/microchip/ksz8795.c | 59 +++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
@@ -1420,11 +1421,69 @@ static int ksz8_setup(struct dsa_switch *ds)return0;}+staticvoidksz8_validate(structdsa_switch*ds,intport,+unsignedlong*supported,+structphylink_link_state*state)+{+__ETHTOOL_DECLARE_LINK_MODE_MASK(mask)={0,};+structksz_device*dev=ds->priv;++if(port==dev->cpu_port){+if(state->interface!=PHY_INTERFACE_MODE_RMII&&+state->interface!=PHY_INTERFACE_MODE_MII&&+state->interface!=PHY_INTERFACE_MODE_NA)+gotounsupported;+}elseif(port>dev->port_cnt){+bitmap_zero(supported,__ETHTOOL_LINK_MODE_MASK_NBITS);+dev_err(ds->dev,"Unsupported port: %i\n",port);+return;+}else{+if(state->interface!=PHY_INTERFACE_MODE_INTERNAL&&+state->interface!=PHY_INTERFACE_MODE_NA)+gotounsupported;+}++/* Allow all the expected bits */+phylink_set_port_modes(mask);+phylink_set(mask,Autoneg);++/* Silicon Errata Sheet (DS80000830A):+*"Port 1 does not respond to received flow control PAUSE frames"+*So,disablePausesupporton"Port 1"(port==0)forallksz88x3+*switches.+*/+if(!ksz_is_ksz88x3(dev)||port)+phylink_set(mask,Pause);++/* Asym pause is not supported on KSZ8863 and KSZ8873 */+if(!ksz_is_ksz88x3(dev))+phylink_set(mask,Asym_Pause);++/* 10M and 100M are only supported */+phylink_set(mask,10baseT_Half);+phylink_set(mask,10baseT_Full);+phylink_set(mask,100baseT_Half);+phylink_set(mask,100baseT_Full);++bitmap_and(supported,supported,mask,+__ETHTOOL_LINK_MODE_MASK_NBITS);+bitmap_and(state->advertising,state->advertising,mask,+__ETHTOOL_LINK_MODE_MASK_NBITS);++return;++unsupported:+bitmap_zero(supported,__ETHTOOL_LINK_MODE_MASK_NBITS);+dev_err(ds->dev,"Unsupported interface: %s, port: %d\n",+phy_modes(state->interface),port);+}+staticconststructdsa_switch_opsksz8_switch_ops={.get_tag_protocol=ksz8_get_tag_protocol,.setup=ksz8_setup,.phy_read=ksz_phy_read16,.phy_write=ksz_phy_write16,+.phylink_validate=ksz8_validate,.phylink_mac_link_down=ksz_mac_link_down,.port_enable=ksz_enable_port,.get_strings=ksz8_get_strings,
This patch support for cable test for the ksz886x switches and the
ksz8081 PHY.
The patch was tested on a KSZ8873RLL switch with following results:
- port 1:
- provides invalid values, thus return -ENOTSUPP
(Errata: DS80000830A: "LinkMD does not work on Port 1",
http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf)
- port 2:
- can detect distance
- can detect open on each wire of pair A (wire 1 and 2)
- can detect open only on one wire of pair B (only wire 3)
- can detect short between wires of a pair (wires 1 + 2 or 3 + 6)
- short between pairs is detected as open.
For example short between wires 2 + 3 is detected as open.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
- added PHY_POLL_CABLE_TEST to make it work in interrupt mode
---
drivers/net/dsa/microchip/ksz8795.c | 13 ++
drivers/net/phy/micrel.c | 180 ++++++++++++++++++++++++++++
include/linux/micrel_phy.h | 1 +
3 files changed, 194 insertions(+)
@@ -53,6 +54,18 @@#define KSZPHY_INTCS_STATUS (KSZPHY_INTCS_LINK_DOWN_STATUS |\KSZPHY_INTCS_LINK_UP_STATUS)+/* LinkMD Control/Status */+#define KSZ8081_LMD 0x1d+#define KSZ8081_LMD_ENABLE_TEST BIT(15)+#define KSZ8081_LMD_STAT_NORMAL 0+#define KSZ8081_LMD_STAT_OPEN 1+#define KSZ8081_LMD_STAT_SHORT 2+#define KSZ8081_LMD_STAT_FAIL 3+#define KSZ8081_LMD_STAT_MASK GENMASK(14, 13)+/* Short cable (<10 meter) has been detected by LinkMD */+#define KSZ8081_LMD_SHORT_INDICATOR BIT(12)+#define KSZ8081_LMD_DELTA_TIME_MASK GENMASK(8, 0)+/* PHY Control 1 */#define MII_KSZPHY_CTRL_1 0x1e#define KSZ8081_CTRL1_MDIX_STAT BIT(4)
@@ -1386,6 +1399,167 @@ static int kszphy_probe(struct phy_device *phydev)return0;}+staticintksz886x_cable_test_start(structphy_device*phydev)+{+if(phydev->dev_flags&MICREL_KSZ8_P1_ERRATA)+return-ENOTSUPP;++/* If autoneg is enabled, we won't be able to test cross pair+*short.Inthiscase,thePHYwill"detect"alinkand+*confusetheinternalstatemachine-disableautoneghere.+*Ifautonegisdisabled,weshouldsetthespeedto10mbit.+*/+returnphy_clear_bits(phydev,MII_BMCR,BMCR_ANENABLE|BMCR_SPEED100);+}++staticintksz886x_cable_test_result_trans(u16status)+{+switch(FIELD_GET(KSZ8081_LMD_STAT_MASK,status)){+caseKSZ8081_LMD_STAT_NORMAL:+returnETHTOOL_A_CABLE_RESULT_CODE_OK;+caseKSZ8081_LMD_STAT_SHORT:+returnETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;+caseKSZ8081_LMD_STAT_OPEN:+returnETHTOOL_A_CABLE_RESULT_CODE_OPEN;+caseKSZ8081_LMD_STAT_FAIL:+/* fall through */+default:+returnETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;+}+}++staticboolksz886x_cable_test_failed(u16status)+{+returnFIELD_GET(KSZ8081_LMD_STAT_MASK,status)==+KSZ8081_LMD_STAT_FAIL;+}++staticboolksz886x_cable_test_fault_length_valid(u16status)+{+switch(FIELD_GET(KSZ8081_LMD_STAT_MASK,status)){+caseKSZ8081_LMD_STAT_OPEN:+/* fall through */+caseKSZ8081_LMD_STAT_SHORT:+returntrue;+}+returnfalse;+}++staticintksz886x_cable_test_fault_length(u16status)+{+intdt;++/* According to the data sheet the distance to the fault is+*DELTA_TIME*0.4meters.+*/+dt=FIELD_GET(KSZ8081_LMD_DELTA_TIME_MASK,status);++return(dt*400)/10;+}++staticintksz886x_cable_test_wait_for_completion(structphy_device*phydev)+{+intval,ret;++ret=phy_read_poll_timeout(phydev,KSZ8081_LMD,val,+!(val&KSZ8081_LMD_ENABLE_TEST),+30000,100000,true);++returnret<0?ret:0;+}++staticintksz886x_cable_test_one_pair(structphy_device*phydev,intpair)+{+staticconstintethtool_pair[]={+ETHTOOL_A_CABLE_PAIR_A,+ETHTOOL_A_CABLE_PAIR_B,+};+intret,val,mdix;++/* There is no way to choice the pair, like we do one ksz9031.+*WecanworkaroundthislimitationbyusingtheMDI-Xfunctionality.+*/+if(pair==0)+mdix=ETH_TP_MDI;+else+mdix=ETH_TP_MDI_X;++switch(phydev->phy_id&MICREL_PHY_ID_MASK){+casePHY_ID_KSZ8081:+ret=ksz8081_config_mdix(phydev,mdix);+break;+casePHY_ID_KSZ886X:+ret=ksz886x_config_mdix(phydev,mdix);+break;+default:+ret=-ENODEV;+}++if(ret)+returnret;++/* Now we are ready to fire. This command will send a 100ns pulse+*tothepair.+*/+ret=phy_write(phydev,KSZ8081_LMD,KSZ8081_LMD_ENABLE_TEST);+if(ret)+returnret;++ret=ksz886x_cable_test_wait_for_completion(phydev);+if(ret)+returnret;++val=phy_read(phydev,KSZ8081_LMD);+if(val<0)+returnval;++if(ksz886x_cable_test_failed(val))+return-EAGAIN;++ret=ethnl_cable_test_result(phydev,ethtool_pair[pair],+ksz886x_cable_test_result_trans(val));+if(ret)+returnret;++if(!ksz886x_cable_test_fault_length_valid(val))+return0;++returnethnl_cable_test_fault_length(phydev,ethtool_pair[pair],+ksz886x_cable_test_fault_length(val));+}++staticintksz886x_cable_test_get_status(structphy_device*phydev,+bool*finished)+{+unsignedlongpair_mask=0x3;+intretries=20;+intpair,ret;++*finished=false;++/* Try harder if link partner is active */+while(pair_mask&&retries--){+for_each_set_bit(pair,&pair_mask,4){+ret=ksz886x_cable_test_one_pair(phydev,pair);+if(ret==-EAGAIN)+continue;+if(ret<0)+returnret;+clear_bit(pair,&pair_mask);+}+/* If link partner is in autonegotiation mode it will send 2ms+*ofFLPswithatleast6msofsilence.+*Add2mssleeptohavebetterchancestohitthissilence.+*/+if(pair_mask)+msleep(2);+}++*finished=true;++return0;+}+staticstructphy_driverksphy_driver[]={{.phy_id=PHY_ID_KS8737,
@@ -1048,6 +1048,92 @@ static int ksz8873mll_config_aneg(struct phy_device *phydev)return0;}+staticintksz886x_config_mdix(structphy_device*phydev,u8ctrl)+{+u16val;++switch(ctrl){+caseETH_TP_MDI:+val=KSZ886X_BMCR_DISABLE_AUTO_MDIX;+break;+caseETH_TP_MDI_X:+/* Note: The naming of the bit KSZ886X_BMCR_FORCE_MDI is bit+*counterintuitive,the"-X"in"1 = Force MDI"inthedata+*sheetseemstobemissing:+*1=ForceMDI(sic!)(transmitonRX+/RX-pins)+*0=Normaloperation(transmitonTX+/TX-pins)+*/+val=KSZ886X_BMCR_DISABLE_AUTO_MDIX|KSZ886X_BMCR_FORCE_MDI;+break;+caseETH_TP_MDI_AUTO:+val=0;+break;+default:+return0;+}++returnphy_modify(phydev,MII_BMCR,+KSZ886X_BMCR_HP_MDIX|KSZ886X_BMCR_FORCE_MDI|+KSZ886X_BMCR_DISABLE_AUTO_MDIX,+KSZ886X_BMCR_HP_MDIX|val);+}++staticintksz886x_config_aneg(structphy_device*phydev)+{+intret;++ret=genphy_config_aneg(phydev);+if(ret)+returnret;++/* The MDI-X configuration is automatically changed by the PHY after+*switchingfromautonegofftoon.So,takeMDI-Xconfigurationunder+*owncontrolandsetitafterautonegconfigurationwasdone.+*/+returnksz886x_config_mdix(phydev,phydev->mdix_ctrl);+}++staticintksz886x_mdix_update(structphy_device*phydev)+{+intret;++ret=phy_read(phydev,MII_BMCR);+if(ret<0)+returnret;++if(ret&KSZ886X_BMCR_DISABLE_AUTO_MDIX){+if(ret&KSZ886X_BMCR_FORCE_MDI)+phydev->mdix_ctrl=ETH_TP_MDI_X;+else+phydev->mdix_ctrl=ETH_TP_MDI;+}else{+phydev->mdix_ctrl=ETH_TP_MDI_AUTO;+}++ret=phy_read(phydev,MII_KSZPHY_CTRL);+if(ret<0)+returnret;++/* Same reverse logic as KSZ886X_BMCR_FORCE_MDI */+if(ret&KSZ886X_CTRL_MDIX_STAT)+phydev->mdix=ETH_TP_MDI_X;+else+phydev->mdix=ETH_TP_MDI;++return0;+}++staticintksz886x_read_status(structphy_device*phydev)+{+intret;++ret=ksz886x_mdix_update(phydev);+if(ret<0)+returnret;++returngenphy_read_status(phydev);+}+staticintksz886x_resume(structphy_device*phydev){intret;
@@ -1420,6 +1506,8 @@ static struct phy_driver ksphy_driver[] = {.name="Micrel KSZ8851 Ethernet MAC or KSZ886X Switch",/* PHY_BASIC_FEATURES */.config_init=kszphy_config_init,+.config_aneg=ksz886x_config_aneg,+.read_status=ksz886x_read_status,.suspend=genphy_suspend,.resume=ksz886x_resume,},{
From: Michael Grzeschik <redacted>
Some micrel devices share the same PHY register defines. This patch
moves them to one common header so other drivers can reuse them.
And reuse generic MII_* defines where possible.
Signed-off-by: Michael Grzeschik <redacted>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/dsa/microchip/ksz8795.c | 119 ++++++++++++------------
drivers/net/dsa/microchip/ksz8795_reg.h | 62 ------------
drivers/net/ethernet/micrel/ksz884x.c | 105 +++------------------
include/linux/micrel_phy.h | 13 +++
4 files changed, 88 insertions(+), 211 deletions(-)
@@ -55,11 +55,17 @@/* PHY Control 1 */#define MII_KSZPHY_CTRL_1 0x1e+#define KSZ8081_CTRL1_MDIX_STAT BIT(4)/* PHY Control 2 / PHY Control (if no PHY Control 1) */#define MII_KSZPHY_CTRL_2 0x1f#define MII_KSZPHY_CTRL MII_KSZPHY_CTRL_2/* bitmap of PHY register to set interrupt mode */+#define KSZ8081_CTRL2_HP_MDIX BIT(15)+#define KSZ8081_CTRL2_MDI_MDI_X_SELECT BIT(14)+#define KSZ8081_CTRL2_DISABLE_AUTO_MDIX BIT(13)+#define KSZ8081_CTRL2_FORCE_LINK BIT(11)+#define KSZ8081_CTRL2_POWER_SAVING BIT(10)#define KSZPHY_CTRL_INT_ACTIVE_HIGH BIT(9)#define KSZPHY_RMII_REF_CLK_SEL BIT(7)
@@ -422,6 +428,87 @@ static int ksz8081_config_init(struct phy_device *phydev)returnkszphy_config_init(phydev);}+staticintksz8081_config_mdix(structphy_device*phydev,u8ctrl)+{+u16val;++switch(ctrl){+caseETH_TP_MDI:+val=KSZ8081_CTRL2_DISABLE_AUTO_MDIX;+break;+caseETH_TP_MDI_X:+val=KSZ8081_CTRL2_DISABLE_AUTO_MDIX|+KSZ8081_CTRL2_MDI_MDI_X_SELECT;+break;+caseETH_TP_MDI_AUTO:+val=0;+break;+default:+return0;+}++returnphy_modify(phydev,MII_KSZPHY_CTRL_2,+KSZ8081_CTRL2_HP_MDIX|+KSZ8081_CTRL2_MDI_MDI_X_SELECT|+KSZ8081_CTRL2_DISABLE_AUTO_MDIX,+KSZ8081_CTRL2_HP_MDIX|val);+}++staticintksz8081_config_aneg(structphy_device*phydev)+{+intret;++ret=genphy_config_aneg(phydev);+if(ret)+returnret;++/* The MDI-X configuration is automatically changed by the PHY after+*switchingfromautonegofftoon.So,takeMDI-Xconfigurationunder+*owncontrolandsetitafterautonegconfigurationwasdone.+*/+returnksz8081_config_mdix(phydev,phydev->mdix_ctrl);+}++staticintksz8081_mdix_update(structphy_device*phydev)+{+intret;++ret=phy_read(phydev,MII_KSZPHY_CTRL_2);+if(ret<0)+returnret;++if(ret&KSZ8081_CTRL2_DISABLE_AUTO_MDIX){+if(ret&KSZ8081_CTRL2_MDI_MDI_X_SELECT)+phydev->mdix_ctrl=ETH_TP_MDI_X;+else+phydev->mdix_ctrl=ETH_TP_MDI;+}else{+phydev->mdix_ctrl=ETH_TP_MDI_AUTO;+}++ret=phy_read(phydev,MII_KSZPHY_CTRL_1);+if(ret<0)+returnret;++if(ret&KSZ8081_CTRL1_MDIX_STAT)+phydev->mdix=ETH_TP_MDI;+else+phydev->mdix=ETH_TP_MDI_X;++return0;+}++staticintksz8081_read_status(structphy_device*phydev)+{+intret;++ret=ksz8081_mdix_update(phydev);+if(ret<0)+returnret;++returngenphy_read_status(phydev);+}+staticintksz8061_config_init(structphy_device*phydev){intret;
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-05-26 15:08:26
On Wed, May 26, 2021 at 06:30:36AM +0200, Oleksij Rempel wrote:
quoted hunk
This patch extends the flags of the phy that's being connected with the
port specific flags of the switch port.
This is needed to handle a port specific erratum of the KSZ8873 switch,
which is added in a later patch.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/phy/phylink.c | 2 +-
net/dsa/slave.c | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
I don't think this has any benefit. phy_attach_direct() does this
internally:
phydev->dev_flags |= flags;
which means the above change is effectively doing:
phydev->dev_flags |= phydev->dev_flags;
So, are you sure you need this?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-05-26 22:01:40
On Wed, May 26, 2021 at 06:30:29AM +0200, Oleksij Rempel wrote:
quoted hunk
From: Michael Grzeschik <redacted>
Some micrel devices share the same PHY register defines. This patch
moves them to one common header so other drivers can reuse them.
And reuse generic MII_* defines where possible.
Signed-off-by: Michael Grzeschik <redacted>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/dsa/microchip/ksz8795.c | 119 ++++++++++++------------
drivers/net/dsa/microchip/ksz8795_reg.h | 62 ------------
drivers/net/ethernet/micrel/ksz884x.c | 105 +++------------------
include/linux/micrel_phy.h | 13 +++
4 files changed, 88 insertions(+), 211 deletions(-)
I am deeply confused as to what this function is doing. It is reading
the 8-bit port registers P_NEG_RESTART_CTRL, P_SPEED_STATUS and
P_FORCE_CTRL and stitching them into a 16-bit "MII_BMCR"?
What layout does this control register even have? Seeing as this is the
implementation of ksz_phy_read16(), I expect that MII_BMCR has the
layout specified in clause 22.2.4.1?
But clause 22 says register 0.5 is "Unidirectional enable", not
"PHY_HP_MDIX" (whatever that might be), and bits 0.4:0 are reserved and
must be written as zero and ignored on read.
- case PHY_REG_STATUS:
+ case MII_BMSR:
ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
- data = PHY_100BTX_FD_CAPABLE |
- PHY_100BTX_CAPABLE |
- PHY_10BT_FD_CAPABLE |
- PHY_10BT_CAPABLE |
- PHY_AUTO_NEG_CAPABLE;
+ data = BMSR_100FULL |
+ BMSR_100HALF |
+ BMSR_10FULL |
+ BMSR_10HALF |
+ BMSR_ANEGCAPABLE;
if (link & PORT_AUTO_NEG_COMPLETE)
- data |= PHY_AUTO_NEG_ACKNOWLEDGE;
+ data |= BMSR_ANEGCOMPLETE;
if (link & PORT_STAT_LINK_GOOD)
- data |= PHY_LINK_STATUS;
+ data |= BMSR_LSTATUS;
break;
- case PHY_REG_ID_1:
+ case MII_PHYSID1:
data = KSZ8795_ID_HI;
break;
- case PHY_REG_ID_2:
+ case MII_PHYSID2:
if (ksz_is_ksz88x3(dev))
data = KSZ8863_ID_LO;
else
data = KSZ8795_ID_LO;
break;
- case PHY_REG_AUTO_NEGOTIATION:
+ case MII_ADVERTISE:
ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
- data = PHY_AUTO_NEG_802_3;
+ data = ADVERTISE_CSMA;
if (ctrl & PORT_AUTO_NEG_SYM_PAUSE)
- data |= PHY_AUTO_NEG_SYM_PAUSE;
+ data |= ADVERTISE_PAUSE_CAP;
if (ctrl & PORT_AUTO_NEG_100BTX_FD)
- data |= PHY_AUTO_NEG_100BTX_FD;
+ data |= ADVERTISE_100FULL;
if (ctrl & PORT_AUTO_NEG_100BTX)
- data |= PHY_AUTO_NEG_100BTX;
+ data |= ADVERTISE_100HALF;
if (ctrl & PORT_AUTO_NEG_10BT_FD)
- data |= PHY_AUTO_NEG_10BT_FD;
+ data |= ADVERTISE_10FULL;
if (ctrl & PORT_AUTO_NEG_10BT)
- data |= PHY_AUTO_NEG_10BT;
+ data |= ADVERTISE_10HALF;
break;
- case PHY_REG_REMOTE_CAPABILITY:
+ case MII_LPA:
ksz_pread8(dev, p, regs[P_REMOTE_STATUS], &link);
- data = PHY_AUTO_NEG_802_3;
+ data = LPA_SLCT;
if (link & PORT_REMOTE_SYM_PAUSE)
- data |= PHY_AUTO_NEG_SYM_PAUSE;
+ data |= LPA_PAUSE_CAP;
if (link & PORT_REMOTE_100BTX_FD)
- data |= PHY_AUTO_NEG_100BTX_FD;
+ data |= LPA_100FULL;
if (link & PORT_REMOTE_100BTX)
- data |= PHY_AUTO_NEG_100BTX;
+ data |= LPA_100HALF;
if (link & PORT_REMOTE_10BT_FD)
- data |= PHY_AUTO_NEG_10BT_FD;
+ data |= LPA_10FULL;
if (link & PORT_REMOTE_10BT)
- data |= PHY_AUTO_NEG_10BT;
- if (data & ~PHY_AUTO_NEG_802_3)
- data |= PHY_REMOTE_ACKNOWLEDGE_NOT;
+ data |= LPA_10HALF;
+ if (data & ~LPA_SLCT)
+ data |= LPA_LPACK;
break;
default:
processed = false;
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-05-26 22:13:16
On Wed, May 26, 2021 at 06:30:30AM +0200, Oleksij Rempel wrote:
quoted hunk
From: Michael Grzeschik <redacted>
This patch adds the phylink support to the ksz8795 driver to provide
configuration exceptions on quirky KSZ8863 and KSZ8873 ports.
Signed-off-by: Michael Grzeschik <redacted>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/dsa/microchip/ksz8795.c | 59 +++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
Is this possible or do we just like to invent things to check?
Unless I'm missing something, ksz8_switch_init() does:
dev->ds->num_ports = dev->port_cnt;
and dsa_port_phylink_validate() does:
ds->ops->phylink_validate(ds, dp->index, supported, state);
where dp->index is set to @port by dsa_port_touch() in this loop:
for (port = 0; port < ds->num_ports; port++) {
dp = dsa_port_touch(ds, port);
if (!dp)
return -ENOMEM;
}
So, if 0 <= dp->index < ds->num_ports == dev->port_cnt, what is the point?
+ } else {
+ if (state->interface != PHY_INTERFACE_MODE_INTERNAL &&
+ state->interface != PHY_INTERFACE_MODE_NA)
+ goto unsupported;
+ }
+
+ /* Allow all the expected bits */
+ phylink_set_port_modes(mask);
+ phylink_set(mask, Autoneg);
+
+ /* Silicon Errata Sheet (DS80000830A):
+ * "Port 1 does not respond to received flow control PAUSE frames"
+ * So, disable Pause support on "Port 1" (port == 0) for all ksz88x3
+ * switches.
+ */
+ if (!ksz_is_ksz88x3(dev) || port)
+ phylink_set(mask, Pause);
+
+ /* Asym pause is not supported on KSZ8863 and KSZ8873 */
+ if (!ksz_is_ksz88x3(dev))
+ phylink_set(mask, Asym_Pause);
+
+ /* 10M and 100M are only supported */
+ phylink_set(mask, 10baseT_Half);
+ phylink_set(mask, 10baseT_Full);
+ phylink_set(mask, 100baseT_Half);
+ phylink_set(mask, 100baseT_Full);
+
+ bitmap_and(supported, supported, mask,
+ __ETHTOOL_LINK_MODE_MASK_NBITS);
+ bitmap_and(state->advertising, state->advertising, mask,
+ __ETHTOOL_LINK_MODE_MASK_NBITS);
+
+ return;
+
+unsupported:
+ bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ dev_err(ds->dev, "Unsupported interface: %s, port: %d\n",
+ phy_modes(state->interface), port);
+}
+
static const struct dsa_switch_ops ksz8_switch_ops = {
.get_tag_protocol = ksz8_get_tag_protocol,
.setup = ksz8_setup,
.phy_read = ksz_phy_read16,
.phy_write = ksz_phy_write16,
+ .phylink_validate = ksz8_validate,
.phylink_mac_link_down = ksz_mac_link_down,
.port_enable = ksz_enable_port,
.get_strings = ksz8_get_strings,
--
2.29.2
@@ -54,11 +54,11 @@ KSZPHY_INTCS_LINK_UP_STATUS) /* PHY Control 1 */-#define MII_KSZPHY_CTRL_1 0x1e+#define MII_KSZPHY_CTRL_1 0x1e /* PHY Control 2 / PHY Control (if no PHY Control 1) */-#define MII_KSZPHY_CTRL_2 0x1f-#define MII_KSZPHY_CTRL MII_KSZPHY_CTRL_2+#define MII_KSZPHY_CTRL_2 0x1f+#define MII_KSZPHY_CTRL MII_KSZPHY_CTRL_2 /* bitmap of PHY register to set interrupt mode */ #define KSZPHY_CTRL_INT_ACTIVE_HIGH BIT(9) #define KSZPHY_RMII_REF_CLK_SEL BIT(7)
--
2.29.2
And the last column of these macros at the end is aligned with spaces
unlike everything else:
/* Write/read to/from extended registers */
#define MII_KSZPHY_EXTREG 0x0b
#define KSZPHY_EXTREG_WRITE 0x8000
#define MII_KSZPHY_EXTREG_WRITE 0x0c
#define MII_KSZPHY_EXTREG_READ 0x0d
/* Extended registers */
#define MII_KSZPHY_CLK_CONTROL_PAD_SKEW 0x104
#define MII_KSZPHY_RX_DATA_PAD_SKEW 0x105
#define MII_KSZPHY_TX_DATA_PAD_SKEW 0x106
I guess if you're going to send this patch you might as well refactor it all.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-05-26 22:43:46
On Wed, May 26, 2021 at 06:30:32AM +0200, Oleksij Rempel wrote:
The ksz8873 and ksz8863 switches are affected by following errata:
| "Receiver error in 100BASE-TX mode following Soft Power Down"
|
| Some KSZ8873 devices may exhibit receiver errors after transitioning
| from Soft Power Down mode to Normal mode, as controlled by register 195
| (0xC3) bits [1:0]. When exiting Soft Power Down mode, the receiver
| blocks may not start up properly, causing the PHY to miss data and
| exhibit erratic behavior. The problem may appear on either port 1 or
| port 2, or both ports. The problem occurs only for 100BASE-TX, not
| 10BASE-T.
|
| END USER IMPLICATIONS
| When the failure occurs, the following symptoms are seen on the affected
| port(s):
| - The port is able to link
| - LED0 blinks, even when there is no traffic
| - The MIB counters indicate receive errors (Rx Fragments, Rx Symbol
| Errors, Rx CRC Errors, Rx Alignment Errors)
| - Only a small fraction of packets is correctly received and forwarded
| through the switch. Most packets are dropped due to receive errors.
|
| The failing condition cannot be corrected by the following:
| - Removing and reconnecting the cable
| - Hardware reset
| - Software Reset and PCS Reset bits in register 67 (0x43)
|
| Work around:
| The problem can be corrected by setting and then clearing the Port Power
| Down bits (registers 29 (0x1D) and 45 (0x2D), bit 3). This must be done
| separately for each affected port after returning from Soft Power Down
| Mode to Normal Mode. The following procedure will ensure no further
| issues due to this erratum. To enter Soft Power Down Mode, set register
| 195 (0xC3), bits [1:0] = 10.
|
| To exit Soft Power Down Mode, follow these steps:
| 1. Set register 195 (0xC3), bits [1:0] = 00 // Exit soft power down mode
| 2. Wait 1ms minimum
| 3. Set register 29 (0x1D), bit [3] = 1 // Enter PHY port 1 power down mode
| 4. Set register 29 (0x1D), bit [3] = 0 // Exit PHY port 1 power down mode
| 5. Set register 45 (0x2D), bit [3] = 1 // Enter PHY port 2 power down mode
| 6. Set register 45 (0x2D), bit [3] = 0 // Exit PHY port 2 power down mode
This patch implements steps 2...6 of the suggested workaround. The first
step needs to be implemented in the switch driver.
Am I right in understanding that register 195 (0xc3) is not a port register?
To hit the erratum, you have to enter Soft Power Down in the first place,
presumably by writing register 0xc3 from somewhere, right?
Where does Linux write this register from?
Once we find that place that enters/exits Soft Power Down mode, can't we
just toggle the Port Power Down bits for each port, exactly like the ERR
workaround says, instead of fooling around with a PHY driver?
Are you able to explain the relation between the call paths of
phy_resume() and the lifetime of the Soft Power Down setting of the
switch? How do we know that the PHYs are resumed after the switch has
exited Soft Power Down mode?
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-05-27 15:17:04
quoted
switch (reg) {
- case PHY_REG_CTRL:
+ case MII_BMCR:
ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
if (restart & PORT_PHY_LOOPBACK)
- data |= PHY_LOOPBACK;
+ data |= BMCR_LOOPBACK;
if (ctrl & PORT_FORCE_100_MBIT)
- data |= PHY_SPEED_100MBIT;
+ data |= BMCR_SPEED100;
if (ksz_is_ksz88x3(dev)) {
if ((ctrl & PORT_AUTO_NEG_ENABLE))
- data |= PHY_AUTO_NEG_ENABLE;
+ data |= BMCR_ANENABLE;
} else {
if (!(ctrl & PORT_AUTO_NEG_DISABLE))
- data |= PHY_AUTO_NEG_ENABLE;
+ data |= BMCR_ANENABLE;
}
if (restart & PORT_POWER_DOWN)
- data |= PHY_POWER_DOWN;
+ data |= BMCR_PDOWN;
if (restart & PORT_AUTO_NEG_RESTART)
- data |= PHY_AUTO_NEG_RESTART;
+ data |= BMCR_ANRESTART;
if (ctrl & PORT_FORCE_FULL_DUPLEX)
- data |= PHY_FULL_DUPLEX;
+ data |= BMCR_FULLDPLX;
if (speed & PORT_HP_MDIX)
- data |= PHY_HP_MDIX;
+ data |= KSZ886X_BMCR_HP_MDIX;
if (restart & PORT_FORCE_MDIX)
- data |= PHY_FORCE_MDIX;
+ data |= KSZ886X_BMCR_FORCE_MDI;
if (restart & PORT_AUTO_MDIX_DISABLE)
- data |= PHY_AUTO_MDIX_DISABLE;
+ data |= KSZ886X_BMCR_DISABLE_AUTO_MDIX;
if (restart & PORT_TX_DISABLE)
- data |= PHY_TRANSMIT_DISABLE;
+ data |= KSZ886X_BMCR_DISABLE_TRANSMIT;
if (restart & PORT_LED_OFF)
- data |= PHY_LED_DISABLE;
+ data |= KSZ886X_BMCR_DISABLE_LED;
break;
I am deeply confused as to what this function is doing. It is reading
the 8-bit port registers P_NEG_RESTART_CTRL, P_SPEED_STATUS and
P_FORCE_CTRL and stitching them into a 16-bit "MII_BMCR"?
Sort of. Take a look at the datasheet for the ksz8841. It has clause
22 like registers which it exports to a PHY driver. It puts MDIX
control into the bottom of the BMCR. So this DSA driver is emulating
the ksz8841 so it can share the PHY driver.
Andrew
On Wed, May 26, 2021 at 04:08:11PM +0100, Russell King (Oracle) wrote:
On Wed, May 26, 2021 at 06:30:36AM +0200, Oleksij Rempel wrote:
quoted
This patch extends the flags of the phy that's being connected with the
port specific flags of the switch port.
This is needed to handle a port specific erratum of the KSZ8873 switch,
which is added in a later patch.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/phy/phylink.c | 2 +-
net/dsa/slave.c | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
I don't think this has any benefit. phy_attach_direct() does this
internally:
phydev->dev_flags |= flags;
which means the above change is effectively doing:
phydev->dev_flags |= phydev->dev_flags;
So, are you sure you need this?
Ah, good point. Back for two years, phy_attach_direct() was doing
phydev->dev_flags = flags;
I didn't noticed this change.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, May 27, 2021 at 01:13:04AM +0300, Vladimir Oltean wrote:
On Wed, May 26, 2021 at 06:30:30AM +0200, Oleksij Rempel wrote:
quoted
From: Michael Grzeschik <redacted>
This patch adds the phylink support to the ksz8795 driver to provide
configuration exceptions on quirky KSZ8863 and KSZ8873 ports.
Signed-off-by: Michael Grzeschik <redacted>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/dsa/microchip/ksz8795.c | 59 +++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
Is this possible or do we just like to invent things to check?
Unless I'm missing something, ksz8_switch_init() does:
dev->ds->num_ports = dev->port_cnt;
and dsa_port_phylink_validate() does:
ds->ops->phylink_validate(ds, dp->index, supported, state);
where dp->index is set to @port by dsa_port_touch() in this loop:
for (port = 0; port < ds->num_ports; port++) {
dp = dsa_port_touch(ds, port);
if (!dp)
return -ENOMEM;
}
So, if 0 <= dp->index < ds->num_ports == dev->port_cnt, what is the point?
good point
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
@@ -54,11 +54,11 @@ KSZPHY_INTCS_LINK_UP_STATUS) /* PHY Control 1 */-#define MII_KSZPHY_CTRL_1 0x1e+#define MII_KSZPHY_CTRL_1 0x1e /* PHY Control 2 / PHY Control (if no PHY Control 1) */-#define MII_KSZPHY_CTRL_2 0x1f-#define MII_KSZPHY_CTRL MII_KSZPHY_CTRL_2+#define MII_KSZPHY_CTRL_2 0x1f+#define MII_KSZPHY_CTRL MII_KSZPHY_CTRL_2 /* bitmap of PHY register to set interrupt mode */ #define KSZPHY_CTRL_INT_ACTIVE_HIGH BIT(9) #define KSZPHY_RMII_REF_CLK_SEL BIT(7)
--
2.29.2
And the last column of these macros at the end is aligned with spaces
unlike everything else:
/* Write/read to/from extended registers */
#define MII_KSZPHY_EXTREG 0x0b
#define KSZPHY_EXTREG_WRITE 0x8000
#define MII_KSZPHY_EXTREG_WRITE 0x0c
#define MII_KSZPHY_EXTREG_READ 0x0d
/* Extended registers */
#define MII_KSZPHY_CLK_CONTROL_PAD_SKEW 0x104
#define MII_KSZPHY_RX_DATA_PAD_SKEW 0x105
#define MII_KSZPHY_TX_DATA_PAD_SKEW 0x106
I guess if you're going to send this patch you might as well refactor it all.
On Thu, May 27, 2021 at 01:43:29AM +0300, Vladimir Oltean wrote:
On Wed, May 26, 2021 at 06:30:32AM +0200, Oleksij Rempel wrote:
quoted
The ksz8873 and ksz8863 switches are affected by following errata:
| "Receiver error in 100BASE-TX mode following Soft Power Down"
|
| Some KSZ8873 devices may exhibit receiver errors after transitioning
| from Soft Power Down mode to Normal mode, as controlled by register 195
| (0xC3) bits [1:0]. When exiting Soft Power Down mode, the receiver
| blocks may not start up properly, causing the PHY to miss data and
| exhibit erratic behavior. The problem may appear on either port 1 or
| port 2, or both ports. The problem occurs only for 100BASE-TX, not
| 10BASE-T.
|
| END USER IMPLICATIONS
| When the failure occurs, the following symptoms are seen on the affected
| port(s):
| - The port is able to link
| - LED0 blinks, even when there is no traffic
| - The MIB counters indicate receive errors (Rx Fragments, Rx Symbol
| Errors, Rx CRC Errors, Rx Alignment Errors)
| - Only a small fraction of packets is correctly received and forwarded
| through the switch. Most packets are dropped due to receive errors.
|
| The failing condition cannot be corrected by the following:
| - Removing and reconnecting the cable
| - Hardware reset
| - Software Reset and PCS Reset bits in register 67 (0x43)
|
| Work around:
| The problem can be corrected by setting and then clearing the Port Power
| Down bits (registers 29 (0x1D) and 45 (0x2D), bit 3). This must be done
| separately for each affected port after returning from Soft Power Down
| Mode to Normal Mode. The following procedure will ensure no further
| issues due to this erratum. To enter Soft Power Down Mode, set register
| 195 (0xC3), bits [1:0] = 10.
|
| To exit Soft Power Down Mode, follow these steps:
| 1. Set register 195 (0xC3), bits [1:0] = 00 // Exit soft power down mode
| 2. Wait 1ms minimum
| 3. Set register 29 (0x1D), bit [3] = 1 // Enter PHY port 1 power down mode
| 4. Set register 29 (0x1D), bit [3] = 0 // Exit PHY port 1 power down mode
| 5. Set register 45 (0x2D), bit [3] = 1 // Enter PHY port 2 power down mode
| 6. Set register 45 (0x2D), bit [3] = 0 // Exit PHY port 2 power down mode
This patch implements steps 2...6 of the suggested workaround. The first
step needs to be implemented in the switch driver.
Am I right in understanding that register 195 (0xc3) is not a port register?
To hit the erratum, you have to enter Soft Power Down in the first place,
presumably by writing register 0xc3 from somewhere, right?
Where does Linux write this register from?
Once we find that place that enters/exits Soft Power Down mode, can't we
just toggle the Port Power Down bits for each port, exactly like the ERR
workaround says, instead of fooling around with a PHY driver?
The KSZ8873 switch is using multiple register mappings.
https://ww1.microchip.com/downloads/en/DeviceDoc/00002348A.pdf
Page 38:
"The MIIM interface is used to access the MII PHY registers defined in
this section. The SPI, I2C, and SMI interfaces can also be used to access
some of these registers. The latter three interfaces use a different
mapping mechanism than the MIIM interface."
This PHY driver is able to work directly over MIIM (MDIO). Or work with DSA over
integrated register translation mapping.
Are you able to explain the relation between the call paths of
phy_resume() and the lifetime of the Soft Power Down setting of the
switch? How do we know that the PHYs are resumed after the switch has
exited Soft Power Down mode?
The MII_BMCRs BMCR_PDOWN bit is mapped to the "register 29 (0x1D), bit
[3]" for the PHY0 and to "register 45 (0x2D), bit [3]" for the PHY1.
I assume, I'll need to add this comments to the commit message. Or do
you have other suggestions on how this should be implemented?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-10 13:06:02
On Thu, Jun 10, 2021 at 01:49:20PM +0200, Oleksij Rempel wrote:
On Thu, May 27, 2021 at 01:43:29AM +0300, Vladimir Oltean wrote:
quoted
On Wed, May 26, 2021 at 06:30:32AM +0200, Oleksij Rempel wrote:
quoted
The ksz8873 and ksz8863 switches are affected by following errata:
| "Receiver error in 100BASE-TX mode following Soft Power Down"
|
| Some KSZ8873 devices may exhibit receiver errors after transitioning
| from Soft Power Down mode to Normal mode, as controlled by register 195
| (0xC3) bits [1:0]. When exiting Soft Power Down mode, the receiver
| blocks may not start up properly, causing the PHY to miss data and
| exhibit erratic behavior. The problem may appear on either port 1 or
| port 2, or both ports. The problem occurs only for 100BASE-TX, not
| 10BASE-T.
|
| END USER IMPLICATIONS
| When the failure occurs, the following symptoms are seen on the affected
| port(s):
| - The port is able to link
| - LED0 blinks, even when there is no traffic
| - The MIB counters indicate receive errors (Rx Fragments, Rx Symbol
| Errors, Rx CRC Errors, Rx Alignment Errors)
| - Only a small fraction of packets is correctly received and forwarded
| through the switch. Most packets are dropped due to receive errors.
|
| The failing condition cannot be corrected by the following:
| - Removing and reconnecting the cable
| - Hardware reset
| - Software Reset and PCS Reset bits in register 67 (0x43)
|
| Work around:
| The problem can be corrected by setting and then clearing the Port Power
| Down bits (registers 29 (0x1D) and 45 (0x2D), bit 3). This must be done
| separately for each affected port after returning from Soft Power Down
| Mode to Normal Mode. The following procedure will ensure no further
| issues due to this erratum. To enter Soft Power Down Mode, set register
| 195 (0xC3), bits [1:0] = 10.
|
| To exit Soft Power Down Mode, follow these steps:
| 1. Set register 195 (0xC3), bits [1:0] = 00 // Exit soft power down mode
| 2. Wait 1ms minimum
| 3. Set register 29 (0x1D), bit [3] = 1 // Enter PHY port 1 power down mode
| 4. Set register 29 (0x1D), bit [3] = 0 // Exit PHY port 1 power down mode
| 5. Set register 45 (0x2D), bit [3] = 1 // Enter PHY port 2 power down mode
| 6. Set register 45 (0x2D), bit [3] = 0 // Exit PHY port 2 power down mode
This patch implements steps 2...6 of the suggested workaround. The first
step needs to be implemented in the switch driver.
Am I right in understanding that register 195 (0xc3) is not a port register?
To hit the erratum, you have to enter Soft Power Down in the first place,
presumably by writing register 0xc3 from somewhere, right?
Where does Linux write this register from?
Once we find that place that enters/exits Soft Power Down mode, can't we
just toggle the Port Power Down bits for each port, exactly like the ERR
workaround says, instead of fooling around with a PHY driver?
The KSZ8873 switch is using multiple register mappings.
https://ww1.microchip.com/downloads/en/DeviceDoc/00002348A.pdf
Page 38:
"The MIIM interface is used to access the MII PHY registers defined in
this section. The SPI, I2C, and SMI interfaces can also be used to access
some of these registers. The latter three interfaces use a different
mapping mechanism than the MIIM interface."
This PHY driver is able to work directly over MIIM (MDIO). Or work with DSA over
integrated register translation mapping.
This doesn't answer my question of where is the Soft Power Down mode enabled.
Are you able to explain the relation between the call paths of
phy_resume() and the lifetime of the Soft Power Down setting of the
switch? How do we know that the PHYs are resumed after the switch has
exited Soft Power Down mode?
The MII_BMCRs BMCR_PDOWN bit is mapped to the "register 29 (0x1D), bit
[3]" for the PHY0 and to "register 45 (0x2D), bit [3]" for the PHY1.
I assume, I'll need to add this comments to the commit message. Or do
you have other suggestions on how this should be implemented?
According to "3.2 Power Management" in the datasheet you shared:
There are 5 (five) operation modes under the power management function,
which is controlled by two bits in Register 195 (0xC3) and one bit in
Register 29 (0x1D), 45 (0x2D) as shown below:
Register 195 bit[1:0] = 00 Normal Operation Mode
Register 195 bit[1:0] = 01 Energy Detect Mode
Register 195 bit[1:0] = 10 Soft Power Down Mode
Register 195 bit[1:0] = 11 Power Saving Mode
Register 29, 45 bit 3 = 1 Port Based Power Down Mode
3.2.4 SOFT POWER DOWN MODE
The soft power down mode is entered by setting bit[1:0]=10 in register
195. When KSZ8873MLL/FLL/RLL is in this mode, all PLL clocks are
disabled, the PHY and the MAC are off, all internal registers values
will not change. When the host set bit[1:0]=00 in register 195, this
device will be back from current soft power down mode to normal
operation mode.
3.2.5 PORT-BASED POWER DOWN MODE
In addition, the KSZ8873MLL/FLL/RLL features a per-port power down mode.
To save power, a PHY port that is not in use can be powered down via
port control register 29 or 45 bit 3, or MIIM PHY register. It saves
about 15 mA per port.
From the above I understand that the first 4 power management modes are
global, and the 5th isn't.
You've explained how the PHY driver enters port-based power down mode.
But the ERR describes an issue being triggered by a global power down
mode. What you are describing is not what the ERR text is describing.
Excuse my perhaps stupid question, but have you triggered the issue
described by the erratum? Does this patch fix that? Where is the disconnect?
On Thu, Jun 10, 2021 at 04:04:45PM +0300, Vladimir Oltean wrote:
On Thu, Jun 10, 2021 at 01:49:20PM +0200, Oleksij Rempel wrote:
quoted
On Thu, May 27, 2021 at 01:43:29AM +0300, Vladimir Oltean wrote:
quoted
On Wed, May 26, 2021 at 06:30:32AM +0200, Oleksij Rempel wrote:
quoted
The ksz8873 and ksz8863 switches are affected by following errata:
| "Receiver error in 100BASE-TX mode following Soft Power Down"
|
| Some KSZ8873 devices may exhibit receiver errors after transitioning
| from Soft Power Down mode to Normal mode, as controlled by register 195
| (0xC3) bits [1:0]. When exiting Soft Power Down mode, the receiver
| blocks may not start up properly, causing the PHY to miss data and
| exhibit erratic behavior. The problem may appear on either port 1 or
| port 2, or both ports. The problem occurs only for 100BASE-TX, not
| 10BASE-T.
|
| END USER IMPLICATIONS
| When the failure occurs, the following symptoms are seen on the affected
| port(s):
| - The port is able to link
| - LED0 blinks, even when there is no traffic
| - The MIB counters indicate receive errors (Rx Fragments, Rx Symbol
| Errors, Rx CRC Errors, Rx Alignment Errors)
| - Only a small fraction of packets is correctly received and forwarded
| through the switch. Most packets are dropped due to receive errors.
|
| The failing condition cannot be corrected by the following:
| - Removing and reconnecting the cable
| - Hardware reset
| - Software Reset and PCS Reset bits in register 67 (0x43)
|
| Work around:
| The problem can be corrected by setting and then clearing the Port Power
| Down bits (registers 29 (0x1D) and 45 (0x2D), bit 3). This must be done
| separately for each affected port after returning from Soft Power Down
| Mode to Normal Mode. The following procedure will ensure no further
| issues due to this erratum. To enter Soft Power Down Mode, set register
| 195 (0xC3), bits [1:0] = 10.
|
| To exit Soft Power Down Mode, follow these steps:
| 1. Set register 195 (0xC3), bits [1:0] = 00 // Exit soft power down mode
| 2. Wait 1ms minimum
| 3. Set register 29 (0x1D), bit [3] = 1 // Enter PHY port 1 power down mode
| 4. Set register 29 (0x1D), bit [3] = 0 // Exit PHY port 1 power down mode
| 5. Set register 45 (0x2D), bit [3] = 1 // Enter PHY port 2 power down mode
| 6. Set register 45 (0x2D), bit [3] = 0 // Exit PHY port 2 power down mode
This patch implements steps 2...6 of the suggested workaround. The first
step needs to be implemented in the switch driver.
Am I right in understanding that register 195 (0xc3) is not a port register?
To hit the erratum, you have to enter Soft Power Down in the first place,
presumably by writing register 0xc3 from somewhere, right?
Where does Linux write this register from?
Once we find that place that enters/exits Soft Power Down mode, can't we
just toggle the Port Power Down bits for each port, exactly like the ERR
workaround says, instead of fooling around with a PHY driver?
The KSZ8873 switch is using multiple register mappings.
https://ww1.microchip.com/downloads/en/DeviceDoc/00002348A.pdf
Page 38:
"The MIIM interface is used to access the MII PHY registers defined in
this section. The SPI, I2C, and SMI interfaces can also be used to access
some of these registers. The latter three interfaces use a different
mapping mechanism than the MIIM interface."
This PHY driver is able to work directly over MIIM (MDIO). Or work with DSA over
integrated register translation mapping.
This doesn't answer my question of where is the Soft Power Down mode enabled.
Are you able to explain the relation between the call paths of
phy_resume() and the lifetime of the Soft Power Down setting of the
switch? How do we know that the PHYs are resumed after the switch has
exited Soft Power Down mode?
The MII_BMCRs BMCR_PDOWN bit is mapped to the "register 29 (0x1D), bit
[3]" for the PHY0 and to "register 45 (0x2D), bit [3]" for the PHY1.
I assume, I'll need to add this comments to the commit message. Or do
you have other suggestions on how this should be implemented?
According to "3.2 Power Management" in the datasheet you shared:
There are 5 (five) operation modes under the power management function,
which is controlled by two bits in Register 195 (0xC3) and one bit in
Register 29 (0x1D), 45 (0x2D) as shown below:
Register 195 bit[1:0] = 00 Normal Operation Mode
Register 195 bit[1:0] = 01 Energy Detect Mode
Register 195 bit[1:0] = 10 Soft Power Down Mode
Register 195 bit[1:0] = 11 Power Saving Mode
Register 29, 45 bit 3 = 1 Port Based Power Down Mode
3.2.4 SOFT POWER DOWN MODE
The soft power down mode is entered by setting bit[1:0]=10 in register
195. When KSZ8873MLL/FLL/RLL is in this mode, all PLL clocks are
disabled, the PHY and the MAC are off, all internal registers values
will not change. When the host set bit[1:0]=00 in register 195, this
device will be back from current soft power down mode to normal
operation mode.
3.2.5 PORT-BASED POWER DOWN MODE
In addition, the KSZ8873MLL/FLL/RLL features a per-port power down mode.
To save power, a PHY port that is not in use can be powered down via
port control register 29 or 45 bit 3, or MIIM PHY register. It saves
about 15 mA per port.
From the above I understand that the first 4 power management modes are
global, and the 5th isn't.
You've explained how the PHY driver enters port-based power down mode.
But the ERR describes an issue being triggered by a global power down
mode. What you are describing is not what the ERR text is describing.
Excuse my perhaps stupid question, but have you triggered the issue
described by the erratum? Does this patch fix that? Where is the disconnect?
Yes, this issue was seen at some early point of development (back in 2019)
reproducible on system start. Where switch was in some default state or
on a state configured by the bootloader. I didn't tried to reproduce it
now. With other words, there is no need to provide global power
management by the DSA driver to trigger it.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-10 18:18:25
On Thu, Jun 10, 2021 at 03:25:05PM +0200, Oleksij Rempel wrote:
Yes, this issue was seen at some early point of development (back in 2019)
reproducible on system start. Where switch was in some default state or
on a state configured by the bootloader. I didn't tried to reproduce it
now. With other words, there is no need to provide global power
management by the DSA driver to trigger it.
If you're sure about that then add it to the commit message or comments,
since this is not what the ERR description says.