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 16:01:11 -0600 |
| |
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
|  |