Messages in this thread | | | Subject | Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature | From | Dan Murphy <> | Date | Fri, 14 Feb 2020 12:31:52 -0600 |
| |
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
| |