Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions
On 15.08.2019 17:43, Florian Fainelli wrote: > > > On 8/15/2019 6:02 AM, Heiner Kallweit wrote: >> On 15.08.2019 14:35, Andrew Lunn wrote: >>> On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote: Now that the Realtek PHY driver maps the vendor-specific EEE registers to the standard MMD registers, we can remove all special handling and use the generic functions phy_ethtool_get/set_eee. >>> >>> Hi Heiner >>> >> Hi Andrew, >> >>> I think you should also add a call the phy_init_eee()? >>> >> I think it's not strictly needed. And few things regarding >> phy_init_eee are not fully clear to me: >> >> - When is it supposed to be called? Before each call to >> phy_ethtool_set_eee? Or once in the drivers init path? >> >> - The name is a little bit misleading as it's mainly a >> validity check. An actual "init" is done only if >> parameter clk_stop_enable is set. >> >> - It returns -EPROTONOSUPPORT if at least one link partner >> doesn't advertise EEE for current speed/duplex. To me this >> seems to be too restrictive. Example: >> We're at 1Gbps/full and link partner advertises EEE for >> 100Mbps only. Then phy_init_eee returns -EPROTONOSUPPORT. >> This keeps me from controlling 100Mbps EEE advertisement. > > That function needs a complete rework, it does not say what its name > implies, and there is an assumption that you have already locally > advertised EEE for it to work properly, that does not make any sense > since the whole purpose is to see whether EEE can/will be active with > the link partner (that's how I read it at least). > > Regarding whether the clock stop enable can be turned on or off is also > a bit suspicious, because a MAC driver does not know whether the PHY > supports doing that, I had started something in that area years ago: > > https://github.com/ffainelli/linux/commits/phy-eee-tx-clk > Not related to this patch, but to EEE support in general: There's something in the back of my mind to create linkmodes for all EEE modes. They could be used with the normal supported, advertising, and lp_advertising bitmaps. Means: - extend genphy_read_abilities to read supported EEE modes - extend genphy_read_status to read lp-advertised EEE modes - let genphy_config_aneg handle EEE advertising - .. This should help to make EEE mode handling consistent with link mode handling. Also still missing is support for the new C45 registers for 2.5Gbps and 5Gbps EEE (3.21, 7.62, 7.63). Open for me is how to deal best with the scenario that older PHY's use the C22 MMD registers for proprietary purposes. Writing to the MMD address register then may cause misbehavior of the PHY (that was the case for RTL8211B), and MMD reads may return random data. Maybe we need a flag to explicitly state whether MMD is supported or not. Heiner
Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions
On 8/15/2019 6:02 AM, Heiner Kallweit wrote: > On 15.08.2019 14:35, Andrew Lunn wrote: >> On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote: >>> Now that the Realtek PHY driver maps the vendor-specific EEE registers >>> to the standard MMD registers, we can remove all special handling and >>> use the generic functions phy_ethtool_get/set_eee. >> >> Hi Heiner >> > Hi Andrew, > >> I think you should also add a call the phy_init_eee()? >> > I think it's not strictly needed. And few things regarding > phy_init_eee are not fully clear to me: > > - When is it supposed to be called? Before each call to > phy_ethtool_set_eee? Or once in the drivers init path? > > - The name is a little bit misleading as it's mainly a > validity check. An actual "init" is done only if > parameter clk_stop_enable is set. > > - It returns -EPROTONOSUPPORT if at least one link partner > doesn't advertise EEE for current speed/duplex. To me this > seems to be too restrictive. Example: > We're at 1Gbps/full and link partner advertises EEE for > 100Mbps only. Then phy_init_eee returns -EPROTONOSUPPORT. > This keeps me from controlling 100Mbps EEE advertisement. That function needs a complete rework, it does not say what its name implies, and there is an assumption that you have already locally advertised EEE for it to work properly, that does not make any sense since the whole purpose is to see whether EEE can/will be active with the link partner (that's how I read it at least). Regarding whether the clock stop enable can be turned on or off is also a bit suspicious, because a MAC driver does not know whether the PHY supports doing that, I had started something in that area years ago: https://github.com/ffainelli/linux/commits/phy-eee-tx-clk -- Florian
Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions
On 15.08.2019 14:35, Andrew Lunn wrote: > On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote: >> Now that the Realtek PHY driver maps the vendor-specific EEE registers >> to the standard MMD registers, we can remove all special handling and >> use the generic functions phy_ethtool_get/set_eee. > > Hi Heiner > Hi Andrew, > I think you should also add a call the phy_init_eee()? > I think it's not strictly needed. And few things regarding phy_init_eee are not fully clear to me: - When is it supposed to be called? Before each call to phy_ethtool_set_eee? Or once in the drivers init path? - The name is a little bit misleading as it's mainly a validity check. An actual "init" is done only if parameter clk_stop_enable is set. - It returns -EPROTONOSUPPORT if at least one link partner doesn't advertise EEE for current speed/duplex. To me this seems to be too restrictive. Example: We're at 1Gbps/full and link partner advertises EEE for 100Mbps only. Then phy_init_eee returns -EPROTONOSUPPORT. This keeps me from controlling 100Mbps EEE advertisement. > Andrew > Heiner
Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions
On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote: > Now that the Realtek PHY driver maps the vendor-specific EEE registers > to the standard MMD registers, we can remove all special handling and > use the generic functions phy_ethtool_get/set_eee. Hi Heiner I think you should also add a call the phy_init_eee()? Andrew
