Messages in this thread | | | Subject | Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature | From | Grygorii Strashko <> | Date | Fri, 14 Feb 2020 20:32:36 +0200 |
| |
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().
-- Best regards, grygorii
| |