Messages in this thread |  | | Subject | Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature | From | Dan Murphy <> | Date | Wed, 5 Feb 2020 15:51:43 -0600 |
| |
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
|  |