Messages in this thread |  | | Subject | Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature | From | Florian Fainelli <> | Date | Wed, 5 Feb 2020 14:00:30 -0800 |
| |
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
|  |