Message ID | 20200204181319.27381-1-dmurphy@ti.com |
---|---|
State | Deferred |
Delegated to: | David Miller |
Headers | show |
Series | [net-next,v2] net: phy: dp83867: Add speed optimization feature | expand |
On 04.02.2020 19:13, Dan Murphy wrote: > Set the speed optimization bit on the DP83867 PHY. > This feature can also be strapped on the 64 pin PHY devices > but the 48 pin devices do not have the strap pin available to enable > this feature in the hardware. PHY team suggests to have this bit set. > > With this bit set the PHY will auto negotiate and report the link > parameters in the PHYSTS register. This register provides a single > location within the register set for quick access to commonly accessed > information. > > In this case when auto negotiation is on the PHY core reads the bits > that have been configured or if auto negotiation is off the PHY core > reads the BMCR register and sets the phydev parameters accordingly. > > This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a > 4-wire cable. If this should occur the PHYSTS register contains the > current negotiated speed and duplex mode. > > In overriding the genphy_read_status the dp83867_read_status will do a > genphy_read_status to setup the LP and pause bits. And then the PHYSTS > register is read and the phydev speed and duplex mode settings are > updated. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> net-next is closed currently. See here for details: https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt Reopening will be announced on the netdev mailing list, you can also check net-next status here: http://vger.kernel.org/~davem/net-next.html > --- > v2 - Updated read status to call genphy_read_status first, added link_change > callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ > > drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c > index 967f57ed0b65..6f86ca1ebb51 100644 > --- a/drivers/net/phy/dp83867.c > +++ b/drivers/net/phy/dp83867.c > @@ -21,6 +21,7 @@ > #define DP83867_DEVADDR 0x1f > > #define MII_DP83867_PHYCTRL 0x10 > +#define MII_DP83867_PHYSTS 0x11 > #define MII_DP83867_MICR 0x12 > #define MII_DP83867_ISR 0x13 > #define DP83867_CFG2 0x14 > @@ -118,6 +119,15 @@ > #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) > #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 > > +/* PHY STS bits */ > +#define DP83867_PHYSTS_1000 BIT(15) > +#define DP83867_PHYSTS_100 BIT(14) > +#define DP83867_PHYSTS_DUPLEX BIT(13) > +#define DP83867_PHYSTS_LINK BIT(10) > + > +/* CFG2 bits */ > +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) > + > /* CFG3 bits */ > #define DP83867_CFG3_INT_OE BIT(7) > #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) > @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device *phydev) > return phy_write(phydev, MII_DP83867_MICR, micr_status); > } > > +static void dp83867_link_change_notify(struct phy_device *phydev) > +{ > + if (phydev->state != PHY_RUNNING) > + return; > + > + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) > + phydev_warn(phydev, "Downshift detected connection is %iMbps\n", > + phydev->speed); > +} > + > +static int dp83867_read_status(struct phy_device *phydev) > +{ > + int status = phy_read(phydev, MII_DP83867_PHYSTS); > + int ret; > + > + ret = genphy_read_status(phydev); > + if (ret) > + return ret; > + > + if (status < 0) > + return status; > + > + if (status & DP83867_PHYSTS_DUPLEX) > + phydev->duplex = DUPLEX_FULL; > + else > + phydev->duplex = DUPLEX_HALF; > + > + if (status & DP83867_PHYSTS_1000) > + phydev->speed = SPEED_1000; > + else if (status & DP83867_PHYSTS_100) > + phydev->speed = SPEED_100; > + else > + phydev->speed = SPEED_10; > + > + return 0; > +} > + > static int dp83867_config_port_mirroring(struct phy_device *phydev) > { > struct dp83867_private *dp83867 = > @@ -467,6 +514,11 @@ static int dp83867_config_init(struct phy_device *phydev) > int ret, val, bs; > u16 delay; > > + /* Force speed optimization for the PHY even if it strapped */ > + ret = phy_set_bits(phydev, DP83867_CFG2, DP83867_SPEED_OPTIMIZED_EN); > + if (ret) > + return ret; > + > ret = dp83867_verify_rgmii_cfg(phydev); > if (ret) > return ret; > @@ -655,6 +707,9 @@ static struct phy_driver dp83867_driver[] = { > .config_init = dp83867_config_init, > .soft_reset = dp83867_phy_reset, > > + .read_status = dp83867_read_status, > + .link_change_notify = dp83867_link_change_notify, > + > .get_wol = dp83867_get_wol, > .set_wol = dp83867_set_wol, > >
Heiner On 2/4/20 2:08 PM, Heiner Kallweit wrote: > On 04.02.2020 19:13, Dan Murphy wrote: >> Set the speed optimization bit on the DP83867 PHY. >> This feature can also be strapped on the 64 pin PHY devices >> but the 48 pin devices do not have the strap pin available to enable >> this feature in the hardware. PHY team suggests to have this bit set. >> >> With this bit set the PHY will auto negotiate and report the link >> parameters in the PHYSTS register. This register provides a single >> location within the register set for quick access to commonly accessed >> information. >> >> In this case when auto negotiation is on the PHY core reads the bits >> that have been configured or if auto negotiation is off the PHY core >> reads the BMCR register and sets the phydev parameters accordingly. >> >> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >> 4-wire cable. If this should occur the PHYSTS register contains the >> current negotiated speed and duplex mode. >> >> In overriding the genphy_read_status the dp83867_read_status will do a >> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >> register is read and the phydev speed and duplex mode settings are >> updated. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > net-next is closed currently. See here for details: > https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt > Reopening will be announced on the netdev mailing list, you can also > check net-next status here: http://vger.kernel.org/~davem/net-next.html > Thanks Heiner for the link I RTM. I will wait for the opening. Dan
On 04.02.2020 19:13, Dan Murphy wrote: > Set the speed optimization bit on the DP83867 PHY. > This feature can also be strapped on the 64 pin PHY devices > but the 48 pin devices do not have the strap pin available to enable > this feature in the hardware. PHY team suggests to have this bit set. > > With this bit set the PHY will auto negotiate and report the link > parameters in the PHYSTS register. This register provides a single > location within the register set for quick access to commonly accessed > information. > > In this case when auto negotiation is on the PHY core reads the bits > that have been configured or if auto negotiation is off the PHY core > reads the BMCR register and sets the phydev parameters accordingly. > > This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a > 4-wire cable. If this should occur the PHYSTS register contains the > current negotiated speed and duplex mode. > > In overriding the genphy_read_status the dp83867_read_status will do a > genphy_read_status to setup the LP and pause bits. And then the PHYSTS > register is read and the phydev speed and duplex mode settings are > updated. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > v2 - Updated read status to call genphy_read_status first, added link_change > callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ > As stated in the first review, it would be appreciated if you implement also the downshift tunable. This could be a separate patch in this series. Most of the implementation would be boilerplate code. And I have to admit that I'm not too happy with the term "speed optimization". This sounds like the PHY has some magic to establish a 1.2Gbps link. Even though the vendor may call it this way in the datasheet, the standard term is "downshift". I'm fine with using "speed optimization" in constants to be in line with the datasheet. Just a comment in the code would be helpful that speed optimization is the vendor's term for downshift. > drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c > index 967f57ed0b65..6f86ca1ebb51 100644 > --- a/drivers/net/phy/dp83867.c > +++ b/drivers/net/phy/dp83867.c > @@ -21,6 +21,7 @@ > #define DP83867_DEVADDR 0x1f > > #define MII_DP83867_PHYCTRL 0x10 > +#define MII_DP83867_PHYSTS 0x11 > #define MII_DP83867_MICR 0x12 > #define MII_DP83867_ISR 0x13 > #define DP83867_CFG2 0x14 > @@ -118,6 +119,15 @@ > #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) > #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 > > +/* PHY STS bits */ > +#define DP83867_PHYSTS_1000 BIT(15) > +#define DP83867_PHYSTS_100 BIT(14) > +#define DP83867_PHYSTS_DUPLEX BIT(13) > +#define DP83867_PHYSTS_LINK BIT(10) > + > +/* CFG2 bits */ > +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) > + > /* CFG3 bits */ > #define DP83867_CFG3_INT_OE BIT(7) > #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) > @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device *phydev) > return phy_write(phydev, MII_DP83867_MICR, micr_status); > } > > +static void dp83867_link_change_notify(struct phy_device *phydev) > +{ > + if (phydev->state != PHY_RUNNING) > + return; > + > + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) > + phydev_warn(phydev, "Downshift detected connection is %iMbps\n", > + phydev->speed); The link partner may simply not advertise 1Gbps. How do you know that a link speed of e.g. 100Mbps is caused by a downshift? Some PHY's I've seen with this feature have a flag somewhere indicating that downshift occurred. How about the PHY here? > +} > + > +static int dp83867_read_status(struct phy_device *phydev) > +{ > + int status = phy_read(phydev, MII_DP83867_PHYSTS); > + int ret; > + > + ret = genphy_read_status(phydev); > + if (ret) > + return ret; > + > + if (status < 0) > + return status; > + > + if (status & DP83867_PHYSTS_DUPLEX) > + phydev->duplex = DUPLEX_FULL; > + else > + phydev->duplex = DUPLEX_HALF; > + > + if (status & DP83867_PHYSTS_1000) > + phydev->speed = SPEED_1000; > + else if (status & DP83867_PHYSTS_100) > + phydev->speed = SPEED_100; > + else > + phydev->speed = SPEED_10; > + > + return 0; > +} > + > static int dp83867_config_port_mirroring(struct phy_device *phydev) > { > struct dp83867_private *dp83867 = > @@ -467,6 +514,11 @@ static int dp83867_config_init(struct phy_device *phydev) > int ret, val, bs; > u16 delay; > > + /* Force speed optimization for the PHY even if it strapped */ > + ret = phy_set_bits(phydev, DP83867_CFG2, DP83867_SPEED_OPTIMIZED_EN); > + if (ret) > + return ret; > + > ret = dp83867_verify_rgmii_cfg(phydev); > if (ret) > return ret; > @@ -655,6 +707,9 @@ static struct phy_driver dp83867_driver[] = { > .config_init = dp83867_config_init, > .soft_reset = dp83867_phy_reset, > > + .read_status = dp83867_read_status, > + .link_change_notify = dp83867_link_change_notify, > + > .get_wol = dp83867_get_wol, > .set_wol = dp83867_set_wol, > >
Heiner On 2/5/20 3:16 PM, Heiner Kallweit wrote: > On 04.02.2020 19:13, Dan Murphy wrote: >> Set the speed optimization bit on the DP83867 PHY. >> This feature can also be strapped on the 64 pin PHY devices >> but the 48 pin devices do not have the strap pin available to enable >> this feature in the hardware. PHY team suggests to have this bit set. >> >> With this bit set the PHY will auto negotiate and report the link >> parameters in the PHYSTS register. This register provides a single >> location within the register set for quick access to commonly accessed >> information. >> >> In this case when auto negotiation is on the PHY core reads the bits >> that have been configured or if auto negotiation is off the PHY core >> reads the BMCR register and sets the phydev parameters accordingly. >> >> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >> 4-wire cable. If this should occur the PHYSTS register contains the >> current negotiated speed and duplex mode. >> >> In overriding the genphy_read_status the dp83867_read_status will do a >> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >> register is read and the phydev speed and duplex mode settings are >> updated. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> v2 - Updated read status to call genphy_read_status first, added link_change >> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ >> > As stated in the first review, it would be appreciated if you implement > also the downshift tunable. This could be a separate patch in this series. > Most of the implementation would be boilerplate code. I just don't have a requirement from our customer to make it adjustable so I did not want to add something extra. I can add in for v3. > > And I have to admit that I'm not too happy with the term "speed optimization". > This sounds like the PHY has some magic to establish a 1.2Gbps link. > Even though the vendor may call it this way in the datasheet, the standard > term is "downshift". I'm fine with using "speed optimization" in constants > to be in line with the datasheet. Just a comment in the code would be helpful > that speed optimization is the vendor's term for downshift. Ack. The data sheet actually says "Speed optimization, also known as link downshift" So I probably will just rename everything down shift. > >> drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> >> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >> index 967f57ed0b65..6f86ca1ebb51 100644 >> --- a/drivers/net/phy/dp83867.c >> +++ b/drivers/net/phy/dp83867.c >> @@ -21,6 +21,7 @@ >> #define DP83867_DEVADDR 0x1f >> >> #define MII_DP83867_PHYCTRL 0x10 >> +#define MII_DP83867_PHYSTS 0x11 >> #define MII_DP83867_MICR 0x12 >> #define MII_DP83867_ISR 0x13 >> #define DP83867_CFG2 0x14 >> @@ -118,6 +119,15 @@ >> #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) >> #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 >> >> +/* PHY STS bits */ >> +#define DP83867_PHYSTS_1000 BIT(15) >> +#define DP83867_PHYSTS_100 BIT(14) >> +#define DP83867_PHYSTS_DUPLEX BIT(13) >> +#define DP83867_PHYSTS_LINK BIT(10) >> + >> +/* CFG2 bits */ >> +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) >> + >> /* CFG3 bits */ >> #define DP83867_CFG3_INT_OE BIT(7) >> #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) >> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device *phydev) >> return phy_write(phydev, MII_DP83867_MICR, micr_status); >> } >> >> +static void dp83867_link_change_notify(struct phy_device *phydev) >> +{ >> + if (phydev->state != PHY_RUNNING) >> + return; >> + >> + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) >> + phydev_warn(phydev, "Downshift detected connection is %iMbps\n", >> + phydev->speed); > The link partner may simply not advertise 1Gbps. How do you know that > a link speed of e.g. 100Mbps is caused by a downshift? > Some PHY's I've seen with this feature have a flag somewhere indicating > that downshift occurred. How about the PHY here? I don't see a register that gives us that status I will ask the hardware team if there is one. This is a 1Gbps PHY by default so if a slower connection is established due to faulty cabling or LP advertisement then this would be a down shift IMO. Dan
On 2/5/20 1:51 PM, Dan Murphy wrote: > Heiner > > On 2/5/20 3:16 PM, Heiner Kallweit wrote: >> On 04.02.2020 19:13, Dan Murphy wrote: >>> Set the speed optimization bit on the DP83867 PHY. >>> This feature can also be strapped on the 64 pin PHY devices >>> but the 48 pin devices do not have the strap pin available to enable >>> this feature in the hardware. PHY team suggests to have this bit set. >>> >>> With this bit set the PHY will auto negotiate and report the link >>> parameters in the PHYSTS register. This register provides a single >>> location within the register set for quick access to commonly accessed >>> information. >>> >>> In this case when auto negotiation is on the PHY core reads the bits >>> that have been configured or if auto negotiation is off the PHY core >>> reads the BMCR register and sets the phydev parameters accordingly. >>> >>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to >>> accomodate a >>> 4-wire cable. If this should occur the PHYSTS register contains the >>> current negotiated speed and duplex mode. >>> >>> In overriding the genphy_read_status the dp83867_read_status will do a >>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>> register is read and the phydev speed and duplex mode settings are >>> updated. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> --- >>> v2 - Updated read status to call genphy_read_status first, added >>> link_change >>> callback to notify of speed change and use phy_set_bits - >>> https://lore.kernel.org/patchwork/patch/1188348/ >>> >> As stated in the first review, it would be appreciated if you implement >> also the downshift tunable. This could be a separate patch in this >> series. >> Most of the implementation would be boilerplate code. > > I just don't have a requirement from our customer to make it adjustable > so I did not want to add something extra. > > I can add in for v3. > >> >> And I have to admit that I'm not too happy with the term "speed >> optimization". >> This sounds like the PHY has some magic to establish a 1.2Gbps link. >> Even though the vendor may call it this way in the datasheet, the >> standard >> term is "downshift". I'm fine with using "speed optimization" in >> constants >> to be in line with the datasheet. Just a comment in the code would be >> helpful >> that speed optimization is the vendor's term for downshift. > > Ack. The data sheet actually says "Speed optimization, also known as > link downshift" > > So I probably will just rename everything down shift. > >> >>> drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 55 insertions(+) >>> >>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >>> index 967f57ed0b65..6f86ca1ebb51 100644 >>> --- a/drivers/net/phy/dp83867.c >>> +++ b/drivers/net/phy/dp83867.c >>> @@ -21,6 +21,7 @@ >>> #define DP83867_DEVADDR 0x1f >>> #define MII_DP83867_PHYCTRL 0x10 >>> +#define MII_DP83867_PHYSTS 0x11 >>> #define MII_DP83867_MICR 0x12 >>> #define MII_DP83867_ISR 0x13 >>> #define DP83867_CFG2 0x14 >>> @@ -118,6 +119,15 @@ >>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) >>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 >>> +/* PHY STS bits */ >>> +#define DP83867_PHYSTS_1000 BIT(15) >>> +#define DP83867_PHYSTS_100 BIT(14) >>> +#define DP83867_PHYSTS_DUPLEX BIT(13) >>> +#define DP83867_PHYSTS_LINK BIT(10) >>> + >>> +/* CFG2 bits */ >>> +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) >>> + >>> /* CFG3 bits */ >>> #define DP83867_CFG3_INT_OE BIT(7) >>> #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) >>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device >>> *phydev) >>> return phy_write(phydev, MII_DP83867_MICR, micr_status); >>> } >>> +static void dp83867_link_change_notify(struct phy_device *phydev) >>> +{ >>> + if (phydev->state != PHY_RUNNING) >>> + return; >>> + >>> + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) >>> + phydev_warn(phydev, "Downshift detected connection is >>> %iMbps\n", >>> + phydev->speed); >> The link partner may simply not advertise 1Gbps. How do you know that >> a link speed of e.g. 100Mbps is caused by a downshift? >> Some PHY's I've seen with this feature have a flag somewhere indicating >> that downshift occurred. How about the PHY here? > > I don't see a register that gives us that status > > I will ask the hardware team if there is one. > > This is a 1Gbps PHY by default so if a slower connection is established > due to faulty cabling or LP advertisement then this would be a down > shift IMO. With your current link_change_notify function it would not be possible to know whether the PHY was connected to a link partner that advertised only 10/100 and so 100 ended up being the link speed, or the link partner was capable of 10/100/1000 and downshift reduced the link speed. If you cannot tell the difference from a register, it might be better to simply omit that function then.
Florian On 2/5/20 4:00 PM, Florian Fainelli wrote: > On 2/5/20 1:51 PM, Dan Murphy wrote: >> Heiner >> >> On 2/5/20 3:16 PM, Heiner Kallweit wrote: >>> On 04.02.2020 19:13, Dan Murphy wrote: >>>> Set the speed optimization bit on the DP83867 PHY. >>>> This feature can also be strapped on the 64 pin PHY devices >>>> but the 48 pin devices do not have the strap pin available to enable >>>> this feature in the hardware. PHY team suggests to have this bit set. >>>> >>>> With this bit set the PHY will auto negotiate and report the link >>>> parameters in the PHYSTS register. This register provides a single >>>> location within the register set for quick access to commonly accessed >>>> information. >>>> >>>> In this case when auto negotiation is on the PHY core reads the bits >>>> that have been configured or if auto negotiation is off the PHY core >>>> reads the BMCR register and sets the phydev parameters accordingly. >>>> >>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to >>>> accomodate a >>>> 4-wire cable. If this should occur the PHYSTS register contains the >>>> current negotiated speed and duplex mode. >>>> >>>> In overriding the genphy_read_status the dp83867_read_status will do a >>>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>>> register is read and the phydev speed and duplex mode settings are >>>> updated. >>>> >>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>> --- >>>> v2 - Updated read status to call genphy_read_status first, added >>>> link_change >>>> callback to notify of speed change and use phy_set_bits - >>>> https://lore.kernel.org/patchwork/patch/1188348/ >>>> >>> As stated in the first review, it would be appreciated if you implement >>> also the downshift tunable. This could be a separate patch in this >>> series. >>> Most of the implementation would be boilerplate code. >> I just don't have a requirement from our customer to make it adjustable >> so I did not want to add something extra. >> >> I can add in for v3. >> >>> And I have to admit that I'm not too happy with the term "speed >>> optimization". >>> This sounds like the PHY has some magic to establish a 1.2Gbps link. >>> Even though the vendor may call it this way in the datasheet, the >>> standard >>> term is "downshift". I'm fine with using "speed optimization" in >>> constants >>> to be in line with the datasheet. Just a comment in the code would be >>> helpful >>> that speed optimization is the vendor's term for downshift. >> Ack. The data sheet actually says "Speed optimization, also known as >> link downshift" >> >> So I probably will just rename everything down shift. >> >>>> drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 55 insertions(+) >>>> >>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >>>> index 967f57ed0b65..6f86ca1ebb51 100644 >>>> --- a/drivers/net/phy/dp83867.c >>>> +++ b/drivers/net/phy/dp83867.c >>>> @@ -21,6 +21,7 @@ >>>> #define DP83867_DEVADDR 0x1f >>>> #define MII_DP83867_PHYCTRL 0x10 >>>> +#define MII_DP83867_PHYSTS 0x11 >>>> #define MII_DP83867_MICR 0x12 >>>> #define MII_DP83867_ISR 0x13 >>>> #define DP83867_CFG2 0x14 >>>> @@ -118,6 +119,15 @@ >>>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) >>>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 >>>> +/* PHY STS bits */ >>>> +#define DP83867_PHYSTS_1000 BIT(15) >>>> +#define DP83867_PHYSTS_100 BIT(14) >>>> +#define DP83867_PHYSTS_DUPLEX BIT(13) >>>> +#define DP83867_PHYSTS_LINK BIT(10) >>>> + >>>> +/* CFG2 bits */ >>>> +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) >>>> + >>>> /* CFG3 bits */ >>>> #define DP83867_CFG3_INT_OE BIT(7) >>>> #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) >>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device >>>> *phydev) >>>> return phy_write(phydev, MII_DP83867_MICR, micr_status); >>>> } >>>> +static void dp83867_link_change_notify(struct phy_device *phydev) >>>> +{ >>>> + if (phydev->state != PHY_RUNNING) >>>> + return; >>>> + >>>> + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) >>>> + phydev_warn(phydev, "Downshift detected connection is >>>> %iMbps\n", >>>> + phydev->speed); >>> The link partner may simply not advertise 1Gbps. How do you know that >>> a link speed of e.g. 100Mbps is caused by a downshift? >>> Some PHY's I've seen with this feature have a flag somewhere indicating >>> that downshift occurred. How about the PHY here? >> I don't see a register that gives us that status >> >> I will ask the hardware team if there is one. >> >> This is a 1Gbps PHY by default so if a slower connection is established >> due to faulty cabling or LP advertisement then this would be a down >> shift IMO. > With your current link_change_notify function it would not be possible > to know whether the PHY was connected to a link partner that advertised > only 10/100 and so 100 ended up being the link speed, or the link > partner was capable of 10/100/1000 and downshift reduced the link speed. > > If you cannot tell the difference from a register, it might be better to > simply omit that function then. Yeah I thought it was a bit redundant and wonky to see in the log that the link established to xG/Mbps and then see another message saying the downshift occurred. Dan
Heiner On 2/5/20 3:16 PM, Heiner Kallweit wrote: > On 04.02.2020 19:13, Dan Murphy wrote: >> Set the speed optimization bit on the DP83867 PHY. >> This feature can also be strapped on the 64 pin PHY devices >> but the 48 pin devices do not have the strap pin available to enable >> this feature in the hardware. PHY team suggests to have this bit set. >> >> With this bit set the PHY will auto negotiate and report the link >> parameters in the PHYSTS register. This register provides a single >> location within the register set for quick access to commonly accessed >> information. >> >> In this case when auto negotiation is on the PHY core reads the bits >> that have been configured or if auto negotiation is off the PHY core >> reads the BMCR register and sets the phydev parameters accordingly. >> >> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >> 4-wire cable. If this should occur the PHYSTS register contains the >> current negotiated speed and duplex mode. >> >> In overriding the genphy_read_status the dp83867_read_status will do a >> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >> register is read and the phydev speed and duplex mode settings are >> updated. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> v2 - Updated read status to call genphy_read_status first, added link_change >> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ >> > As stated in the first review, it would be appreciated if you implement > also the downshift tunable. This could be a separate patch in this series. > Most of the implementation would be boilerplate code. I looked at this today and there are no registers that allow tuning the downshift attempts. There is only a RO register that tells you how many attempts it took to achieve a link. So at the very least we could put in the get_tunable but there will be no set. So we should probably skip this for this PHY. Dan
On 06.02.2020 23:13, Dan Murphy wrote: > Heiner > > On 2/5/20 3:16 PM, Heiner Kallweit wrote: >> On 04.02.2020 19:13, Dan Murphy wrote: >>> Set the speed optimization bit on the DP83867 PHY. >>> This feature can also be strapped on the 64 pin PHY devices >>> but the 48 pin devices do not have the strap pin available to enable >>> this feature in the hardware. PHY team suggests to have this bit set. >>> >>> With this bit set the PHY will auto negotiate and report the link >>> parameters in the PHYSTS register. This register provides a single >>> location within the register set for quick access to commonly accessed >>> information. >>> >>> In this case when auto negotiation is on the PHY core reads the bits >>> that have been configured or if auto negotiation is off the PHY core >>> reads the BMCR register and sets the phydev parameters accordingly. >>> >>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >>> 4-wire cable. If this should occur the PHYSTS register contains the >>> current negotiated speed and duplex mode. >>> >>> In overriding the genphy_read_status the dp83867_read_status will do a >>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>> register is read and the phydev speed and duplex mode settings are >>> updated. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> --- >>> v2 - Updated read status to call genphy_read_status first, added link_change >>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ >>> >> As stated in the first review, it would be appreciated if you implement >> also the downshift tunable. This could be a separate patch in this series. >> Most of the implementation would be boilerplate code. > > > I looked at this today and there are no registers that allow tuning the downshift attempts. There is only a RO register that tells you how many attempts it took to achieve a link. So at the very least we could put in the get_tunable but there will be no set. > The get operation for the downshift tunable should return after how many failed attempts the PHY starts a downshift. This doesn't match with your description of this register, so yes: Implementing the tunable for this PHY doesn't make sense. However this register may be useful in the link_change_notify() callback to figure out whether a downshift happened, to trigger the info message you had in your first version. > So we should probably skip this for this PHY. > > Dan > Heiner
Heiner On 2/6/20 4:28 PM, Heiner Kallweit wrote: > On 06.02.2020 23:13, Dan Murphy wrote: >> Heiner >> >> On 2/5/20 3:16 PM, Heiner Kallweit wrote: >>> On 04.02.2020 19:13, Dan Murphy wrote: >>>> Set the speed optimization bit on the DP83867 PHY. >>>> This feature can also be strapped on the 64 pin PHY devices >>>> but the 48 pin devices do not have the strap pin available to enable >>>> this feature in the hardware. PHY team suggests to have this bit set. >>>> >>>> With this bit set the PHY will auto negotiate and report the link >>>> parameters in the PHYSTS register. This register provides a single >>>> location within the register set for quick access to commonly accessed >>>> information. >>>> >>>> In this case when auto negotiation is on the PHY core reads the bits >>>> that have been configured or if auto negotiation is off the PHY core >>>> reads the BMCR register and sets the phydev parameters accordingly. >>>> >>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >>>> 4-wire cable. If this should occur the PHYSTS register contains the >>>> current negotiated speed and duplex mode. >>>> >>>> In overriding the genphy_read_status the dp83867_read_status will do a >>>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>>> register is read and the phydev speed and duplex mode settings are >>>> updated. >>>> >>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>> --- >>>> v2 - Updated read status to call genphy_read_status first, added link_change >>>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ >>>> >>> As stated in the first review, it would be appreciated if you implement >>> also the downshift tunable. This could be a separate patch in this series. >>> Most of the implementation would be boilerplate code. >> >> I looked at this today and there are no registers that allow tuning the downshift attempts. There is only a RO register that tells you how many attempts it took to achieve a link. So at the very least we could put in the get_tunable but there will be no set. >> > The get operation for the downshift tunable should return after how many failed > attempts the PHY starts a downshift. This doesn't match with your description of > this register, so yes: Implementing the tunable for this PHY doesn't make sense. True. This register is only going to return 1,2,4 and 8. And it is defaulted to 4 attempts. > > However this register may be useful in the link_change_notify() callback to > figure out whether a downshift happened, to trigger the info message you had in > your first version. Thats a good idea but.. The register is defaulted to always report 4 attempts were made. It never reports 0 attempts so we would never know the truth behind the reporting. Kinda like matching the speeds. Dan
On 06.02.2020 23:36, Dan Murphy wrote: > Heiner > > On 2/6/20 4:28 PM, Heiner Kallweit wrote: >> On 06.02.2020 23:13, Dan Murphy wrote: >>> Heiner >>> >>> On 2/5/20 3:16 PM, Heiner Kallweit wrote: >>>> On 04.02.2020 19:13, Dan Murphy wrote: >>>>> Set the speed optimization bit on the DP83867 PHY. >>>>> This feature can also be strapped on the 64 pin PHY devices >>>>> but the 48 pin devices do not have the strap pin available to enable >>>>> this feature in the hardware. PHY team suggests to have this bit set. >>>>> >>>>> With this bit set the PHY will auto negotiate and report the link >>>>> parameters in the PHYSTS register. This register provides a single >>>>> location within the register set for quick access to commonly accessed >>>>> information. >>>>> >>>>> In this case when auto negotiation is on the PHY core reads the bits >>>>> that have been configured or if auto negotiation is off the PHY core >>>>> reads the BMCR register and sets the phydev parameters accordingly. >>>>> >>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >>>>> 4-wire cable. If this should occur the PHYSTS register contains the >>>>> current negotiated speed and duplex mode. >>>>> >>>>> In overriding the genphy_read_status the dp83867_read_status will do a >>>>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>>>> register is read and the phydev speed and duplex mode settings are >>>>> updated. >>>>> >>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>> --- >>>>> v2 - Updated read status to call genphy_read_status first, added link_change >>>>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ >>>>> >>>> As stated in the first review, it would be appreciated if you implement >>>> also the downshift tunable. This could be a separate patch in this series. >>>> Most of the implementation would be boilerplate code. >>> >>> I looked at this today and there are no registers that allow tuning the downshift attempts. There is only a RO register that tells you how many attempts it took to achieve a link. So at the very least we could put in the get_tunable but there will be no set. >>> >> The get operation for the downshift tunable should return after how many failed >> attempts the PHY starts a downshift. This doesn't match with your description of >> this register, so yes: Implementing the tunable for this PHY doesn't make sense. > True. This register is only going to return 1,2,4 and 8. And it is defaulted to 4 attempts. >> >> However this register may be useful in the link_change_notify() callback to >> figure out whether a downshift happened, to trigger the info message you had in >> your first version. > > Thats a good idea but.. The register is defaulted to always report 4 attempts were made. It never reports 0 attempts so we would never know the truth behind the reporting. Kinda like matching the speeds. > I just had a brief look at the datasheet here: http://www.ti.com/lit/ds/symlink/dp83867ir.pdf It says: The number of failed link attempts before falling back to 100-M operation is configurable. (p.45) Description of SPEED_OPT_ATTEMPT_CNT in CFG2 says "select attempt count", so it sounds like it's an RW register. It's marked as RO however, maybe it's a typo in the datasheet. Did you test whether register is writable? Last but not least this register is exactly what's needed for the downshift tunable. Checking whether a downshift occurred should be possible by reading SPEED_OPT_EVENT_INT in ISR. In interrupt mode however this may require a custom interrupt handler (implementation of handle_interrupt callback). Alternatively you could check SPEED_OPT_STATUS in PHYSTS. It says "valid only during aneg" but that sounds a little bit weird. Would need to be tested whether bit remains set after downshifted aneg is finished. > Dan > Heiner
Heiner On 2/6/20 5:04 PM, Heiner Kallweit wrote: > On 06.02.2020 23:36, Dan Murphy wrote: >> Heiner >> >> On 2/6/20 4:28 PM, Heiner Kallweit wrote: >>> On 06.02.2020 23:13, Dan Murphy wrote: >>>> Heiner >>>> >>>> On 2/5/20 3:16 PM, Heiner Kallweit wrote: >>>>> On 04.02.2020 19:13, Dan Murphy wrote: >>>>>> Set the speed optimization bit on the DP83867 PHY. >>>>>> This feature can also be strapped on the 64 pin PHY devices >>>>>> but the 48 pin devices do not have the strap pin available to enable >>>>>> this feature in the hardware. PHY team suggests to have this bit set. >>>>>> >>>>>> With this bit set the PHY will auto negotiate and report the link >>>>>> parameters in the PHYSTS register. This register provides a single >>>>>> location within the register set for quick access to commonly accessed >>>>>> information. >>>>>> >>>>>> In this case when auto negotiation is on the PHY core reads the bits >>>>>> that have been configured or if auto negotiation is off the PHY core >>>>>> reads the BMCR register and sets the phydev parameters accordingly. >>>>>> >>>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >>>>>> 4-wire cable. If this should occur the PHYSTS register contains the >>>>>> current negotiated speed and duplex mode. >>>>>> >>>>>> In overriding the genphy_read_status the dp83867_read_status will do a >>>>>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>>>>> register is read and the phydev speed and duplex mode settings are >>>>>> updated. >>>>>> >>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>>> --- >>>>>> v2 - Updated read status to call genphy_read_status first, added link_change >>>>>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ >>>>>> >>>>> As stated in the first review, it would be appreciated if you implement >>>>> also the downshift tunable. This could be a separate patch in this series. >>>>> Most of the implementation would be boilerplate code. >>>> I looked at this today and there are no registers that allow tuning the downshift attempts. There is only a RO register that tells you how many attempts it took to achieve a link. So at the very least we could put in the get_tunable but there will be no set. >>>> >>> The get operation for the downshift tunable should return after how many failed >>> attempts the PHY starts a downshift. This doesn't match with your description of >>> this register, so yes: Implementing the tunable for this PHY doesn't make sense. >> True. This register is only going to return 1,2,4 and 8. And it is defaulted to 4 attempts. >>> However this register may be useful in the link_change_notify() callback to >>> figure out whether a downshift happened, to trigger the info message you had in >>> your first version. >> Thats a good idea but.. The register is defaulted to always report 4 attempts were made. It never reports 0 attempts so we would never know the truth behind the reporting. Kinda like matching the speeds. >> > I just had a brief look at the datasheet here: http://www.ti.com/lit/ds/symlink/dp83867ir.pdf > It says: The number of failed link attempts before falling back to 100-M operation is configurable. (p.45) > Description of SPEED_OPT_ATTEMPT_CNT in CFG2 says "select attempt count", so it sounds like it's > an RW register. It's marked as RO however, maybe it's a typo in the datasheet. > Did you test whether register is writable? Yes I did and it was a no go. It is definitely a RO. I will complain to the HW team and get it straightened out. We have time before net-next opens > Last but not least this register is exactly what's needed for the downshift tunable. > > Checking whether a downshift occurred should be possible by reading SPEED_OPT_EVENT_INT in ISR. > In interrupt mode however this may require a custom interrupt handler (implementation of > handle_interrupt callback). Yes the HW team did say R13b5 could be checked but after thinking about it the issue with that is that is a clear on read register so other status would be lost. There could be a race condition between the interrupt handler and the link notification change to be able to indicate whether the downshift happened or not. Same with polling mode can we be guaranteed that the status would be updated before the link change was called? Dan
Grygorii On 2/14/20 12:32 PM, Grygorii Strashko wrote: > > > On 06/02/2020 00:01, Dan Murphy wrote: >> Florian >> >> On 2/5/20 4:00 PM, Florian Fainelli wrote: >>> On 2/5/20 1:51 PM, Dan Murphy wrote: >>>> Heiner >>>> >>>> On 2/5/20 3:16 PM, Heiner Kallweit wrote: >>>>> On 04.02.2020 19:13, Dan Murphy wrote: >>>>>> Set the speed optimization bit on the DP83867 PHY. >>>>>> This feature can also be strapped on the 64 pin PHY devices >>>>>> but the 48 pin devices do not have the strap pin available to enable >>>>>> this feature in the hardware. PHY team suggests to have this bit >>>>>> set. >>>>>> >>>>>> With this bit set the PHY will auto negotiate and report the link >>>>>> parameters in the PHYSTS register. This register provides a single >>>>>> location within the register set for quick access to commonly >>>>>> accessed >>>>>> information. >>>>>> >>>>>> In this case when auto negotiation is on the PHY core reads the bits >>>>>> that have been configured or if auto negotiation is off the PHY core >>>>>> reads the BMCR register and sets the phydev parameters accordingly. >>>>>> >>>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to >>>>>> accomodate a >>>>>> 4-wire cable. If this should occur the PHYSTS register contains the >>>>>> current negotiated speed and duplex mode. >>>>>> >>>>>> In overriding the genphy_read_status the dp83867_read_status will >>>>>> do a >>>>>> genphy_read_status to setup the LP and pause bits. And then the >>>>>> PHYSTS >>>>>> register is read and the phydev speed and duplex mode settings are >>>>>> updated. >>>>>> >>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>>> --- >>>>>> v2 - Updated read status to call genphy_read_status first, added >>>>>> link_change >>>>>> callback to notify of speed change and use phy_set_bits - >>>>>> https://lore.kernel.org/patchwork/patch/1188348/ >>>>>> >>>>> As stated in the first review, it would be appreciated if you >>>>> implement >>>>> also the downshift tunable. This could be a separate patch in this >>>>> series. >>>>> Most of the implementation would be boilerplate code. >>>> I just don't have a requirement from our customer to make it >>>> adjustable >>>> so I did not want to add something extra. >>>> >>>> I can add in for v3. >>>> >>>>> And I have to admit that I'm not too happy with the term "speed >>>>> optimization". >>>>> This sounds like the PHY has some magic to establish a 1.2Gbps link. >>>>> Even though the vendor may call it this way in the datasheet, the >>>>> standard >>>>> term is "downshift". I'm fine with using "speed optimization" in >>>>> constants >>>>> to be in line with the datasheet. Just a comment in the code would be >>>>> helpful >>>>> that speed optimization is the vendor's term for downshift. >>>> Ack. The data sheet actually says "Speed optimization, also known as >>>> link downshift" >>>> >>>> So I probably will just rename everything down shift. >>>> >>>>>> drivers/net/phy/dp83867.c | 55 >>>>>> +++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 55 insertions(+) >>>>>> >>>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >>>>>> index 967f57ed0b65..6f86ca1ebb51 100644 >>>>>> --- a/drivers/net/phy/dp83867.c >>>>>> +++ b/drivers/net/phy/dp83867.c >>>>>> @@ -21,6 +21,7 @@ >>>>>> #define DP83867_DEVADDR 0x1f >>>>>> #define MII_DP83867_PHYCTRL 0x10 >>>>>> +#define MII_DP83867_PHYSTS 0x11 >>>>>> #define MII_DP83867_MICR 0x12 >>>>>> #define MII_DP83867_ISR 0x13 >>>>>> #define DP83867_CFG2 0x14 >>>>>> @@ -118,6 +119,15 @@ >>>>>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) >>>>>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 >>>>>> +/* PHY STS bits */ >>>>>> +#define DP83867_PHYSTS_1000 BIT(15) >>>>>> +#define DP83867_PHYSTS_100 BIT(14) >>>>>> +#define DP83867_PHYSTS_DUPLEX BIT(13) >>>>>> +#define DP83867_PHYSTS_LINK BIT(10) >>>>>> + >>>>>> +/* CFG2 bits */ >>>>>> +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) >>>>>> + >>>>>> /* CFG3 bits */ >>>>>> #define DP83867_CFG3_INT_OE BIT(7) >>>>>> #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) >>>>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct >>>>>> phy_device >>>>>> *phydev) >>>>>> return phy_write(phydev, MII_DP83867_MICR, micr_status); >>>>>> } >>>>>> +static void dp83867_link_change_notify(struct phy_device >>>>>> *phydev) >>>>>> +{ >>>>>> + if (phydev->state != PHY_RUNNING) >>>>>> + return; >>>>>> + >>>>>> + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) >>>>>> + phydev_warn(phydev, "Downshift detected connection is >>>>>> %iMbps\n", >>>>>> + phydev->speed); >>>>> The link partner may simply not advertise 1Gbps. How do you know that >>>>> a link speed of e.g. 100Mbps is caused by a downshift? >>>>> Some PHY's I've seen with this feature have a flag somewhere >>>>> indicating >>>>> that downshift occurred. How about the PHY here? >>>> I don't see a register that gives us that status >>>> >>>> I will ask the hardware team if there is one. >>>> >>>> This is a 1Gbps PHY by default so if a slower connection is >>>> established >>>> due to faulty cabling or LP advertisement then this would be a down >>>> shift IMO. >>> With your current link_change_notify function it would not be possible >>> to know whether the PHY was connected to a link partner that advertised >>> only 10/100 and so 100 ended up being the link speed, or the link >>> partner was capable of 10/100/1000 and downshift reduced the link >>> speed. >>> >>> If you cannot tell the difference from a register, it might be >>> better to >>> simply omit that function then. >> >> Yeah I thought it was a bit redundant and wonky to see in the log >> that the link established to xG/Mbps and then see another message >> saying the downshift occurred. > > I think it's good idea to have this message as just wrong cable might > be used. > > But this notifier make no sense in it current form - it will produce > noise in case of forced 100m/10M. > > FYI. PHY sequence to update link: > phy_state_machine() > |-phy_check_link_status() > |-phy_link_down/up() > |- .phy_link_change()->phy_link_change() > |-adjust_link() ----> netdev callback > |-phydev->drv->link_change_notify(phydev); > > So, log output has to be done or in .read_status() or > some info has to be saved in .read_status() and then re-used in > .link_change_notify(). > OK I will try to find a way to give some sort of message. Also we did get confirmation from HW guys and you also confirmed that the number of attempts for downshift is configurable. So I will be adding back the tunable code once net-next opens. Dan
On 06/02/2020 00:01, Dan Murphy wrote: > Florian > > On 2/5/20 4:00 PM, Florian Fainelli wrote: >> On 2/5/20 1:51 PM, Dan Murphy wrote: >>> Heiner >>> >>> On 2/5/20 3:16 PM, Heiner Kallweit wrote: >>>> On 04.02.2020 19:13, Dan Murphy wrote: >>>>> Set the speed optimization bit on the DP83867 PHY. >>>>> This feature can also be strapped on the 64 pin PHY devices >>>>> but the 48 pin devices do not have the strap pin available to enable >>>>> this feature in the hardware. PHY team suggests to have this bit set. >>>>> >>>>> With this bit set the PHY will auto negotiate and report the link >>>>> parameters in the PHYSTS register. This register provides a single >>>>> location within the register set for quick access to commonly accessed >>>>> information. >>>>> >>>>> In this case when auto negotiation is on the PHY core reads the bits >>>>> that have been configured or if auto negotiation is off the PHY core >>>>> reads the BMCR register and sets the phydev parameters accordingly. >>>>> >>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to >>>>> accomodate a >>>>> 4-wire cable. If this should occur the PHYSTS register contains the >>>>> current negotiated speed and duplex mode. >>>>> >>>>> In overriding the genphy_read_status the dp83867_read_status will do a >>>>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>>>> register is read and the phydev speed and duplex mode settings are >>>>> updated. >>>>> >>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>> --- >>>>> v2 - Updated read status to call genphy_read_status first, added >>>>> link_change >>>>> callback to notify of speed change and use phy_set_bits - >>>>> https://lore.kernel.org/patchwork/patch/1188348/ >>>>> >>>> As stated in the first review, it would be appreciated if you implement >>>> also the downshift tunable. This could be a separate patch in this >>>> series. >>>> Most of the implementation would be boilerplate code. >>> I just don't have a requirement from our customer to make it adjustable >>> so I did not want to add something extra. >>> >>> I can add in for v3. >>> >>>> And I have to admit that I'm not too happy with the term "speed >>>> optimization". >>>> This sounds like the PHY has some magic to establish a 1.2Gbps link. >>>> Even though the vendor may call it this way in the datasheet, the >>>> standard >>>> term is "downshift". I'm fine with using "speed optimization" in >>>> constants >>>> to be in line with the datasheet. Just a comment in the code would be >>>> helpful >>>> that speed optimization is the vendor's term for downshift. >>> Ack. The data sheet actually says "Speed optimization, also known as >>> link downshift" >>> >>> So I probably will just rename everything down shift. >>> >>>>> drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 55 insertions(+) >>>>> >>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >>>>> index 967f57ed0b65..6f86ca1ebb51 100644 >>>>> --- a/drivers/net/phy/dp83867.c >>>>> +++ b/drivers/net/phy/dp83867.c >>>>> @@ -21,6 +21,7 @@ >>>>> #define DP83867_DEVADDR 0x1f >>>>> #define MII_DP83867_PHYCTRL 0x10 >>>>> +#define MII_DP83867_PHYSTS 0x11 >>>>> #define MII_DP83867_MICR 0x12 >>>>> #define MII_DP83867_ISR 0x13 >>>>> #define DP83867_CFG2 0x14 >>>>> @@ -118,6 +119,15 @@ >>>>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) >>>>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 >>>>> +/* PHY STS bits */ >>>>> +#define DP83867_PHYSTS_1000 BIT(15) >>>>> +#define DP83867_PHYSTS_100 BIT(14) >>>>> +#define DP83867_PHYSTS_DUPLEX BIT(13) >>>>> +#define DP83867_PHYSTS_LINK BIT(10) >>>>> + >>>>> +/* CFG2 bits */ >>>>> +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) >>>>> + >>>>> /* CFG3 bits */ >>>>> #define DP83867_CFG3_INT_OE BIT(7) >>>>> #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) >>>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device >>>>> *phydev) >>>>> return phy_write(phydev, MII_DP83867_MICR, micr_status); >>>>> } >>>>> +static void dp83867_link_change_notify(struct phy_device *phydev) >>>>> +{ >>>>> + if (phydev->state != PHY_RUNNING) >>>>> + return; >>>>> + >>>>> + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) >>>>> + phydev_warn(phydev, "Downshift detected connection is >>>>> %iMbps\n", >>>>> + phydev->speed); >>>> The link partner may simply not advertise 1Gbps. How do you know that >>>> a link speed of e.g. 100Mbps is caused by a downshift? >>>> Some PHY's I've seen with this feature have a flag somewhere indicating >>>> that downshift occurred. How about the PHY here? >>> I don't see a register that gives us that status >>> >>> I will ask the hardware team if there is one. >>> >>> This is a 1Gbps PHY by default so if a slower connection is established >>> due to faulty cabling or LP advertisement then this would be a down >>> shift IMO. >> With your current link_change_notify function it would not be possible >> to know whether the PHY was connected to a link partner that advertised >> only 10/100 and so 100 ended up being the link speed, or the link >> partner was capable of 10/100/1000 and downshift reduced the link speed. >> >> If you cannot tell the difference from a register, it might be better to >> simply omit that function then. > > Yeah I thought it was a bit redundant and wonky to see in the log that the link established to xG/Mbps and then see another message saying the downshift occurred. I think it's good idea to have this message as just wrong cable might be used. But this notifier make no sense in it current form - it will produce noise in case of forced 100m/10M. FYI. PHY sequence to update link: phy_state_machine() |-phy_check_link_status() |-phy_link_down/up() |- .phy_link_change()->phy_link_change() |-adjust_link() ----> netdev callback |-phydev->drv->link_change_notify(phydev); So, log output has to be done or in .read_status() or some info has to be saved in .read_status() and then re-used in .link_change_notify().
Grygorii On 2/14/20 12:31 PM, Dan Murphy wrote: > Grygorii > > On 2/14/20 12:32 PM, Grygorii Strashko wrote: >> >> >> On 06/02/2020 00:01, Dan Murphy wrote: >>> Florian >>> >>> On 2/5/20 4:00 PM, Florian Fainelli wrote: >>>> On 2/5/20 1:51 PM, Dan Murphy wrote: >>>>> Heiner >>>>> >>>>> On 2/5/20 3:16 PM, Heiner Kallweit wrote: >>>>>> On 04.02.2020 19:13, Dan Murphy wrote: >>>>>>> Set the speed optimization bit on the DP83867 PHY. >>>>>>> This feature can also be strapped on the 64 pin PHY devices >>>>>>> but the 48 pin devices do not have the strap pin available to >>>>>>> enable >>>>>>> this feature in the hardware. PHY team suggests to have this >>>>>>> bit set. >>>>>>> >>>>>>> With this bit set the PHY will auto negotiate and report the link >>>>>>> parameters in the PHYSTS register. This register provides a single >>>>>>> location within the register set for quick access to commonly >>>>>>> accessed >>>>>>> information. >>>>>>> >>>>>>> In this case when auto negotiation is on the PHY core reads the >>>>>>> bits >>>>>>> that have been configured or if auto negotiation is off the PHY >>>>>>> core >>>>>>> reads the BMCR register and sets the phydev parameters accordingly. >>>>>>> >>>>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to >>>>>>> accomodate a >>>>>>> 4-wire cable. If this should occur the PHYSTS register contains >>>>>>> the >>>>>>> current negotiated speed and duplex mode. >>>>>>> >>>>>>> In overriding the genphy_read_status the dp83867_read_status >>>>>>> will do a >>>>>>> genphy_read_status to setup the LP and pause bits. And then the >>>>>>> PHYSTS >>>>>>> register is read and the phydev speed and duplex mode settings are >>>>>>> updated. >>>>>>> >>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>>>> --- >>>>>>> v2 - Updated read status to call genphy_read_status first, added >>>>>>> link_change >>>>>>> callback to notify of speed change and use phy_set_bits - >>>>>>> https://lore.kernel.org/patchwork/patch/1188348/ >>>>>>> >>>>>> As stated in the first review, it would be appreciated if you >>>>>> implement >>>>>> also the downshift tunable. This could be a separate patch in this >>>>>> series. >>>>>> Most of the implementation would be boilerplate code. >>>>> I just don't have a requirement from our customer to make it >>>>> adjustable >>>>> so I did not want to add something extra. >>>>> >>>>> I can add in for v3. >>>>> >>>>>> And I have to admit that I'm not too happy with the term "speed >>>>>> optimization". >>>>>> This sounds like the PHY has some magic to establish a 1.2Gbps link. >>>>>> Even though the vendor may call it this way in the datasheet, the >>>>>> standard >>>>>> term is "downshift". I'm fine with using "speed optimization" in >>>>>> constants >>>>>> to be in line with the datasheet. Just a comment in the code >>>>>> would be >>>>>> helpful >>>>>> that speed optimization is the vendor's term for downshift. >>>>> Ack. The data sheet actually says "Speed optimization, also known as >>>>> link downshift" >>>>> >>>>> So I probably will just rename everything down shift. >>>>> >>>>>>> drivers/net/phy/dp83867.c | 55 >>>>>>> +++++++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 55 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >>>>>>> index 967f57ed0b65..6f86ca1ebb51 100644 >>>>>>> --- a/drivers/net/phy/dp83867.c >>>>>>> +++ b/drivers/net/phy/dp83867.c >>>>>>> @@ -21,6 +21,7 @@ >>>>>>> #define DP83867_DEVADDR 0x1f >>>>>>> #define MII_DP83867_PHYCTRL 0x10 >>>>>>> +#define MII_DP83867_PHYSTS 0x11 >>>>>>> #define MII_DP83867_MICR 0x12 >>>>>>> #define MII_DP83867_ISR 0x13 >>>>>>> #define DP83867_CFG2 0x14 >>>>>>> @@ -118,6 +119,15 @@ >>>>>>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) >>>>>>> #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 >>>>>>> +/* PHY STS bits */ >>>>>>> +#define DP83867_PHYSTS_1000 BIT(15) >>>>>>> +#define DP83867_PHYSTS_100 BIT(14) >>>>>>> +#define DP83867_PHYSTS_DUPLEX BIT(13) >>>>>>> +#define DP83867_PHYSTS_LINK BIT(10) >>>>>>> + >>>>>>> +/* CFG2 bits */ >>>>>>> +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) >>>>>>> + >>>>>>> /* CFG3 bits */ >>>>>>> #define DP83867_CFG3_INT_OE BIT(7) >>>>>>> #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) >>>>>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct >>>>>>> phy_device >>>>>>> *phydev) >>>>>>> return phy_write(phydev, MII_DP83867_MICR, micr_status); >>>>>>> } >>>>>>> +static void dp83867_link_change_notify(struct phy_device >>>>>>> *phydev) >>>>>>> +{ >>>>>>> + if (phydev->state != PHY_RUNNING) >>>>>>> + return; >>>>>>> + >>>>>>> + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) >>>>>>> + phydev_warn(phydev, "Downshift detected connection is >>>>>>> %iMbps\n", >>>>>>> + phydev->speed); >>>>>> The link partner may simply not advertise 1Gbps. How do you know >>>>>> that >>>>>> a link speed of e.g. 100Mbps is caused by a downshift? >>>>>> Some PHY's I've seen with this feature have a flag somewhere >>>>>> indicating >>>>>> that downshift occurred. How about the PHY here? >>>>> I don't see a register that gives us that status >>>>> >>>>> I will ask the hardware team if there is one. >>>>> >>>>> This is a 1Gbps PHY by default so if a slower connection is >>>>> established >>>>> due to faulty cabling or LP advertisement then this would be a down >>>>> shift IMO. >>>> With your current link_change_notify function it would not be possible >>>> to know whether the PHY was connected to a link partner that >>>> advertised >>>> only 10/100 and so 100 ended up being the link speed, or the link >>>> partner was capable of 10/100/1000 and downshift reduced the link >>>> speed. >>>> >>>> If you cannot tell the difference from a register, it might be >>>> better to >>>> simply omit that function then. >>> >>> Yeah I thought it was a bit redundant and wonky to see in the log >>> that the link established to xG/Mbps and then see another message >>> saying the downshift occurred. >> >> I think it's good idea to have this message as just wrong cable might >> be used. >> >> But this notifier make no sense in it current form - it will produce >> noise in case of forced 100m/10M. >> >> FYI. PHY sequence to update link: >> phy_state_machine() >> |-phy_check_link_status() >> |-phy_link_down/up() >> |- .phy_link_change()->phy_link_change() >> |-adjust_link() ----> netdev callback >> |-phydev->drv->link_change_notify(phydev); >> >> So, log output has to be done or in .read_status() or >> some info has to be saved in .read_status() and then re-used in >> .link_change_notify(). >> > OK I will try to find a way to give some sort of message. > > Also we did get confirmation from HW guys and you also confirmed that > the number of attempts for downshift is configurable. So I will be > adding back the tunable code once net-next opens. > I worked on this a bit and I think the notification is a bit complicated to get into the code just to print a message. First the notification comes from the interrupt register which is COR. So if I read the interrupt register in read_status then the ack_interrupt call back won't do anything and status will be lost so if we need to implement other features that depend on the interrupt status that status is cleared. In addition the downshift interrupt will be read and cleared so the state of any downshift is lost after the message. The link_change_notifier is called first then the ack_interrupt function is called so as I stated the downshift status will be reset to no downshift as the bit is cleared. So I don't think adding this notifier is worth the complex code to print a message. Dan
On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote: > Grygorii > > On 2/14/20 12:32 PM, Grygorii Strashko wrote: > > I think it's good idea to have this message as just wrong cable might be > > used. > > > > But this notifier make no sense in it current form - it will produce > > noise in case of forced 100m/10M. > > > > FYI. PHY sequence to update link: > > phy_state_machine() > > |-phy_check_link_status() > > |-phy_link_down/up() > > |- .phy_link_change()->phy_link_change() > > |-adjust_link() ----> netdev callback > > |-phydev->drv->link_change_notify(phydev); > > > > So, log output has to be done or in .read_status() or > > some info has to be saved in .read_status() and then re-used in > > .link_change_notify(). > > > OK I will try to find a way to give some sort of message. How do you know the speed that the PHY downshifted to? If the speed and duplex are available in some PHY specific status register, then one way you can detect downshift is to decode the negotiated speed/duplex from the advertisements (specifically the LPA read from the registers and the advertisement that we should be advertising - some PHYs modify their registers when downshifting) and check whether it matches the negotiated parameters in the PHY specific status register. Alternatively, if the PHY modifies the advertisement register on downshift, comparing the advertisement register with what it should be will tell you if downshift has occurred. Note, however, that if both ends of the link are capable of downshift, and they downshift at the same time, it can be difficult to reliably tell whether the downshift was performed by the local PHY or the remote PHY - even if the local PHY gives you status bits for downshift, you won't know if the remote end downshifted instead. It's a bit like auto MDI/MDIX - if pairswap is needed, either end may do it, and which end does it may change each time the link comes up. So, reporting downshift in the kernel log may not be all that useful, it may be more suited to being reported through a future ethtool interface just like MDI/MDIX.
Russell On 2/18/20 10:25 AM, Russell King - ARM Linux admin wrote: > On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote: >> Grygorii >> >> On 2/14/20 12:32 PM, Grygorii Strashko wrote: >>> I think it's good idea to have this message as just wrong cable might be >>> used. >>> >>> But this notifier make no sense in it current form - it will produce >>> noise in case of forced 100m/10M. >>> >>> FYI. PHY sequence to update link: >>> phy_state_machine() >>> |-phy_check_link_status() >>> |-phy_link_down/up() >>> |- .phy_link_change()->phy_link_change() >>> |-adjust_link() ----> netdev callback >>> |-phydev->drv->link_change_notify(phydev); >>> >>> So, log output has to be done or in .read_status() or >>> some info has to be saved in .read_status() and then re-used in >>> .link_change_notify(). >>> >> OK I will try to find a way to give some sort of message. > How do you know the speed that the PHY downshifted to? The DP83867 has a register PHYSTS where BIT 15:14 indicate the speed that the PHY negotiated. In the same register BIT 13 indicates the duplex mode. > If the speed and duplex are available in some PHY specific status > register, then one way you can detect downshift is to decode the > negotiated speed/duplex from the advertisements (specifically the LPA > read from the registers and the advertisement that we should be > advertising - some PHYs modify their registers when downshifting) and > check whether it matches the negotiated parameters in the PHY > specific status register. > > Alternatively, if the PHY modifies the advertisement register on > downshift, comparing the advertisement register with what it should > be will tell you if downshift has occurred. The ISR register BIT 5 indicates if a downshift occurred or not. So we can indicate that the PHY downshifted but there is no cause in the registers bit field. My concern for this bit though is the register is clear on read so all other interrupts are lost if we only read to check downshift. And the link_change_notifier is called before the interrupt ACK call back. We could call the interrupt function and get the downshift status but again it will clear the interrupt register and any other statuses may be lost. Dan
On Tue, Feb 18, 2020 at 10:36:47AM -0600, Dan Murphy wrote: > Russell > > On 2/18/20 10:25 AM, Russell King - ARM Linux admin wrote: > > On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote: > > > Grygorii > > > > > > On 2/14/20 12:32 PM, Grygorii Strashko wrote: > > > > I think it's good idea to have this message as just wrong cable might be > > > > used. > > > > > > > > But this notifier make no sense in it current form - it will produce > > > > noise in case of forced 100m/10M. > > > > > > > > FYI. PHY sequence to update link: > > > > phy_state_machine() > > > > |-phy_check_link_status() > > > > |-phy_link_down/up() > > > > |- .phy_link_change()->phy_link_change() > > > > |-adjust_link() ----> netdev callback > > > > |-phydev->drv->link_change_notify(phydev); > > > > > > > > So, log output has to be done or in .read_status() or > > > > some info has to be saved in .read_status() and then re-used in > > > > .link_change_notify(). > > > > > > > OK I will try to find a way to give some sort of message. > > How do you know the speed that the PHY downshifted to? > > The DP83867 has a register PHYSTS where BIT 15:14 indicate the speed that > the PHY negotiated. > > In the same register BIT 13 indicates the duplex mode. > > > If the speed and duplex are available in some PHY specific status > > register, then one way you can detect downshift is to decode the > > negotiated speed/duplex from the advertisements (specifically the LPA > > read from the registers and the advertisement that we should be > > advertising - some PHYs modify their registers when downshifting) and > > check whether it matches the negotiated parameters in the PHY > > specific status register. > > > > Alternatively, if the PHY modifies the advertisement register on > > downshift, comparing the advertisement register with what it should > > be will tell you if downshift has occurred. > > The ISR register BIT 5 indicates if a downshift occurred or not. So we can > indicate that the PHY downshifted but there is no cause in the registers bit > field. My concern for this bit though is the register is clear on read so > all other interrupts are lost if we only read to check downshift. And the > link_change_notifier is called before the interrupt ACK call back. We could > call the interrupt function and get the downshift status but again it will > clear the interrupt register and any other statuses may be lost. What's wrong with having an ack_interrupt() method that reads the PHY ISR register, and records in a driver private flag that bit 5 has been set? The read_status() method can clear the flag if link goes down, or check the flag if link is up and report that a downshift event occurred. If IRQs are not in use, then read_status() would have to read the ISR itself. It may be better to move ack_interrupt() to did_interrupt(), which will ensure that it gets executed before the PHY state machine is triggered by phy_interrupt().
Russell On 2/18/20 10:49 AM, Russell King - ARM Linux admin wrote: > On Tue, Feb 18, 2020 at 10:36:47AM -0600, Dan Murphy wrote: >> Russell >> >> On 2/18/20 10:25 AM, Russell King - ARM Linux admin wrote: >>> On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote: >>>> Grygorii >>>> >>>> On 2/14/20 12:32 PM, Grygorii Strashko wrote: >>>>> I think it's good idea to have this message as just wrong cable might be >>>>> used. >>>>> >>>>> But this notifier make no sense in it current form - it will produce >>>>> noise in case of forced 100m/10M. >>>>> >>>>> FYI. PHY sequence to update link: >>>>> phy_state_machine() >>>>> |-phy_check_link_status() >>>>> |-phy_link_down/up() >>>>> |- .phy_link_change()->phy_link_change() >>>>> |-adjust_link() ----> netdev callback >>>>> |-phydev->drv->link_change_notify(phydev); >>>>> >>>>> So, log output has to be done or in .read_status() or >>>>> some info has to be saved in .read_status() and then re-used in >>>>> .link_change_notify(). >>>>> >>>> OK I will try to find a way to give some sort of message. >>> How do you know the speed that the PHY downshifted to? >> The DP83867 has a register PHYSTS where BIT 15:14 indicate the speed that >> the PHY negotiated. >> >> In the same register BIT 13 indicates the duplex mode. >> >>> If the speed and duplex are available in some PHY specific status >>> register, then one way you can detect downshift is to decode the >>> negotiated speed/duplex from the advertisements (specifically the LPA >>> read from the registers and the advertisement that we should be >>> advertising - some PHYs modify their registers when downshifting) and >>> check whether it matches the negotiated parameters in the PHY >>> specific status register. >>> >>> Alternatively, if the PHY modifies the advertisement register on >>> downshift, comparing the advertisement register with what it should >>> be will tell you if downshift has occurred. >> The ISR register BIT 5 indicates if a downshift occurred or not. So we can >> indicate that the PHY downshifted but there is no cause in the registers bit >> field. My concern for this bit though is the register is clear on read so >> all other interrupts are lost if we only read to check downshift. And the >> link_change_notifier is called before the interrupt ACK call back. We could >> call the interrupt function and get the downshift status but again it will >> clear the interrupt register and any other statuses may be lost. > What's wrong with having an ack_interrupt() method that reads the > PHY ISR register, and records in a driver private flag that bit 5 > has been set? The read_status() method can clear the flag if link > goes down, or check the flag if link is up and report that a > downshift event occurred. > > If IRQs are not in use, then read_status() would have to read the > ISR itself. > > It may be better to move ack_interrupt() to did_interrupt(), which > will ensure that it gets executed before the PHY state machine is > triggered by phy_interrupt(). > Well now the read_status is becoming a lot more complex. It would be better to remove the ack_interrupt call back and just have read_status call the interrupt function regardless of interrupts or not. Because all the interrupt function would be doing is clearing all the interrupts in the ISR register on a link up/down event. And as you pointed out we can check and set a flag that indicates if a downshift has happened on link up status and clear it on link down. We would need to set the downshift interrupt mask to always report that bit. As opposed to not setting any interrupts if interrupts are not enabled. If the user wants to track WoL interrupt or any other feature interrupt we would have to add that flag to the read_status as well seems like it could get a bit out of control. Again this is a lot of error prone complex changes and tracking just to populate a message in the kernel log. There is no guarantee that the LP did not force the downshift or advertise that it supports 1Gbps. So what condition is really being reported here? There seems like there are so many different scenarios why the PHY could not negotiate to its advertised 1Gbps. Dan
On Tue, Feb 18, 2020 at 11:12:37AM -0600, Dan Murphy wrote: > Well now the read_status is becoming a lot more complex. It would be better > to remove the ack_interrupt call back and just have read_status call the > interrupt function regardless of interrupts or not. Because all the > interrupt function would be doing is clearing all the interrupts in the ISR > register on a link up/down event. And as you pointed out we can check and > set a flag that indicates if a downshift has happened on link up status and > clear it on link down. We would need to set the downshift interrupt mask to > always report that bit. As opposed to not setting any interrupts if > interrupts are not enabled. If the user wants to track WoL interrupt or any > other feature interrupt we would have to add that flag to the read_status as > well seems like it could get a bit out of control. To be honest, I don't like phylib's interrupt implementation; it imposes a fixed way of dealing with interrupts on phylib drivers, and it sounds like its ideas don't match what modern PHYs want. For example, the Marvell 88x3310 can produce interrupts for GPIOs that are on-board, or temperature alarms, or other stuff... but phylib's idea is that a PHY interrupt is for signalling that the link changed somehow, and imposes a did_interrupt(), handle interrupt, clear_interrupt() structure whether or not it's suitable for the PHY. If you don't provide the functions phylib wants, then phydev->irq gets killed. Plus, phylib claims the interrupt for you at certain points depending on whether the PHY is bound to a network interface or not. So, my suggestion to move ack_interrupt to did_interrupt will result in phylib forcing phydev->irq to PHY_POLL, killing any support for interrupts what so ever... > Again this is a lot of error prone complex changes and tracking just to > populate a message in the kernel log. There is no guarantee that the LP did > not force the downshift or advertise that it supports 1Gbps. So what > condition is really being reported here? There seems like there are so many > different scenarios why the PHY could not negotiate to its advertised 1Gbps. Note that when a PHY wants to downshift, it does that by changing its advertisement that is sent to the other PHY. So, if both ends support 1G, 100M and 10M, and the remote end decides to downshift to 100M, the remote end basically stops advertising 1G and restarts negotiation afresh with the new advertisement. In that case, if you look at ethtool's output, it will indicate that the link partner is not advertising 1G, and the link is operating at 100M. If 100M doesn't work (and there are cases where connections become quite flakey) then 100M may also be omitted as well, negotiation restarted, causing a downshift to 10M. That's basically how downshift works - a PHY will attempt to establish link a number of times before deciding to restart negotiation with some of the advertisement omitted, and the reduced negotiation advertisement appears in the remote PHY's link partner registers. As I mentioned, the PHY on either end of the link can be the one which decides to downshift, and the partner PHY has no idea that a downshift has happened.
Russell On 2/18/20 11:33 AM, Russell King - ARM Linux admin wrote: > On Tue, Feb 18, 2020 at 11:12:37AM -0600, Dan Murphy wrote: >> Well now the read_status is becoming a lot more complex. It would be better >> to remove the ack_interrupt call back and just have read_status call the >> interrupt function regardless of interrupts or not. Because all the >> interrupt function would be doing is clearing all the interrupts in the ISR >> register on a link up/down event. And as you pointed out we can check and >> set a flag that indicates if a downshift has happened on link up status and >> clear it on link down. We would need to set the downshift interrupt mask to >> always report that bit. As opposed to not setting any interrupts if >> interrupts are not enabled. If the user wants to track WoL interrupt or any >> other feature interrupt we would have to add that flag to the read_status as >> well seems like it could get a bit out of control. > To be honest, I don't like phylib's interrupt implementation; it > imposes a fixed way of dealing with interrupts on phylib drivers, > and it sounds like its ideas don't match what modern PHYs want. > > For example, the Marvell 88x3310 can produce interrupts for GPIOs > that are on-board, or temperature alarms, or other stuff... but > phylib's idea is that a PHY interrupt is for signalling that the > link changed somehow, and imposes a did_interrupt(), handle > interrupt, clear_interrupt() structure whether or not it's > suitable for the PHY. If you don't provide the functions phylib > wants, then phydev->irq gets killed. Plus, phylib claims the > interrupt for you at certain points depending on whether the > PHY is bound to a network interface or not. > > So, my suggestion to move ack_interrupt to did_interrupt will > result in phylib forcing phydev->irq to PHY_POLL, killing any > support for interrupts what so ever... Does it really make sense to do all of this for a downshift message in the kernel log? >> Again this is a lot of error prone complex changes and tracking just to >> populate a message in the kernel log. There is no guarantee that the LP did >> not force the downshift or advertise that it supports 1Gbps. So what >> condition is really being reported here? There seems like there are so many >> different scenarios why the PHY could not negotiate to its advertised 1Gbps. > Note that when a PHY wants to downshift, it does that by changing its > advertisement that is sent to the other PHY. > > So, if both ends support 1G, 100M and 10M, and the remote end decides > to downshift to 100M, the remote end basically stops advertising 1G > and restarts negotiation afresh with the new advertisement. > > In that case, if you look at ethtool's output, it will indicate that > the link partner is not advertising 1G, and the link is operating at > 100M. > > If 100M doesn't work (and there are cases where connections become > quite flakey) then 100M may also be omitted as well, negotiation > restarted, causing a downshift to 10M. > > That's basically how downshift works - a PHY will attempt to > establish link a number of times before deciding to restart > negotiation with some of the advertisement omitted, and the > reduced negotiation advertisement appears in the remote PHY's > link partner registers. This is the way I understand it too. > As I mentioned, the PHY on either end of the link can be the one > which decides to downshift, and the partner PHY has no idea that > a downshift has happened. > Exactly so we can only report that if the PHY on our end caused the downshift. If this PHY does not cause the downshift then the message will not be presented even though a downshift occurred. So what is the value in presenting this message?
On Tue, Feb 18, 2020 at 11:38:33AM -0600, Dan Murphy wrote: > Russell > > On 2/18/20 11:33 AM, Russell King - ARM Linux admin wrote: > > As I mentioned, the PHY on either end of the link can be the one > > which decides to downshift, and the partner PHY has no idea that > > a downshift has happened. > > > Exactly so we can only report that if the PHY on our end caused the > downshift. If this PHY does not cause the downshift then the message will > not be presented even though a downshift occurred. So what is the value in > presenting this message? I think we are in agreement over questioning the value of reporting the downshift!
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index 967f57ed0b65..6f86ca1ebb51 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -21,6 +21,7 @@ #define DP83867_DEVADDR 0x1f #define MII_DP83867_PHYCTRL 0x10 +#define MII_DP83867_PHYSTS 0x11 #define MII_DP83867_MICR 0x12 #define MII_DP83867_ISR 0x13 #define DP83867_CFG2 0x14 @@ -118,6 +119,15 @@ #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK (0x1f << 8) #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT 8 +/* PHY STS bits */ +#define DP83867_PHYSTS_1000 BIT(15) +#define DP83867_PHYSTS_100 BIT(14) +#define DP83867_PHYSTS_DUPLEX BIT(13) +#define DP83867_PHYSTS_LINK BIT(10) + +/* CFG2 bits */ +#define DP83867_SPEED_OPTIMIZED_EN (BIT(8) | BIT(9)) + /* CFG3 bits */ #define DP83867_CFG3_INT_OE BIT(7) #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device *phydev) return phy_write(phydev, MII_DP83867_MICR, micr_status); } +static void dp83867_link_change_notify(struct phy_device *phydev) +{ + if (phydev->state != PHY_RUNNING) + return; + + if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10) + phydev_warn(phydev, "Downshift detected connection is %iMbps\n", + phydev->speed); +} + +static int dp83867_read_status(struct phy_device *phydev) +{ + int status = phy_read(phydev, MII_DP83867_PHYSTS); + int ret; + + ret = genphy_read_status(phydev); + if (ret) + return ret; + + if (status < 0) + return status; + + if (status & DP83867_PHYSTS_DUPLEX) + phydev->duplex = DUPLEX_FULL; + else + phydev->duplex = DUPLEX_HALF; + + if (status & DP83867_PHYSTS_1000) + phydev->speed = SPEED_1000; + else if (status & DP83867_PHYSTS_100) + phydev->speed = SPEED_100; + else + phydev->speed = SPEED_10; + + return 0; +} + static int dp83867_config_port_mirroring(struct phy_device *phydev) { struct dp83867_private *dp83867 = @@ -467,6 +514,11 @@ static int dp83867_config_init(struct phy_device *phydev) int ret, val, bs; u16 delay; + /* Force speed optimization for the PHY even if it strapped */ + ret = phy_set_bits(phydev, DP83867_CFG2, DP83867_SPEED_OPTIMIZED_EN); + if (ret) + return ret; + ret = dp83867_verify_rgmii_cfg(phydev); if (ret) return ret; @@ -655,6 +707,9 @@ static struct phy_driver dp83867_driver[] = { .config_init = dp83867_config_init, .soft_reset = dp83867_phy_reset, + .read_status = dp83867_read_status, + .link_change_notify = dp83867_link_change_notify, + .get_wol = dp83867_get_wol, .set_wol = dp83867_set_wol,
Set the speed optimization bit on the DP83867 PHY. This feature can also be strapped on the 64 pin PHY devices but the 48 pin devices do not have the strap pin available to enable this feature in the hardware. PHY team suggests to have this bit set. With this bit set the PHY will auto negotiate and report the link parameters in the PHYSTS register. This register provides a single location within the register set for quick access to commonly accessed information. In this case when auto negotiation is on the PHY core reads the bits that have been configured or if auto negotiation is off the PHY core reads the BMCR register and sets the phydev parameters accordingly. This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a 4-wire cable. If this should occur the PHYSTS register contains the current negotiated speed and duplex mode. In overriding the genphy_read_status the dp83867_read_status will do a genphy_read_status to setup the LP and pause bits. And then the PHYSTS register is read and the phydev speed and duplex mode settings are updated. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v2 - Updated read status to call genphy_read_status first, added link_change callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)