On 11.07.2018 23:33, Florian Fainelli wrote: > > > On 07/11/2018 02:08 PM, Heiner Kallweit wrote: >> On 11.07.2018 22:55, Andrew Lunn wrote: >>>> +/** >>>> + * phy_speed_down - set speed to lowest speed supported by both link >>>> partners >>>> + * @phydev: the phy_device struct >>>> + * @sync: perform action synchronously >>>> + * >>>> + * Description: Typically used to save energy when waiting for a WoL >>>> packet >>>> + */ >>>> +int phy_speed_down(struct phy_device *phydev, bool sync) >>> >>> This sync parameter needs some more thought. I'm not sure it is safe. >>> >>> How does a PHY trigger a WoL wake up? I guess some use the interrupt >>> pin. How does a PHY indicate auto-neg has completed? It triggers an >>> interrupt. So it seems like there is a danger here we suspend, and >>> then wake up 2 seconds later when auto-neg has completed. >>> >>> I'm not sure we can safely suspend until auto-neg has completed. >>> >>>> +/** >>>> + * phy_speed_up - (re)set advertised speeds to all supported speeds >>>> + * @phydev: the phy_device struct >>>> + * @sync: perform action synchronously >>>> + * >>>> + * Description: Used to revert the effect of phy_speed_down >>>> + */ >>>> +int phy_speed_up(struct phy_device *phydev, bool sync) >>> >>> And here, i'm thinking the opposite. A MAC driver needs to be ready >>> for the PHY state to change at any time. So why do we need to wait? >>> Just let the normal mechanisms inform the MAC when the link is up. >>> >> I see your points, thanks for the feedback. In my case WoL triggers >> a PCI PME and the code works as expected, but I agree this may be >> different in other setups (external PHY). >> >> The sync parameter was inspired by following comment from Florian: >> "One thing that bothers me a bit is that this should ideally be >> offered as both blocking and non-blocking options" >> So let's see which comments he may have before preparing a v2. > > What I had in mind is that you would be able to register a callback that > would tell you when auto-negotiation completes, and not register one if > you did not want to have that information. > > As Andrew points out though, with PHY using interrupts, this might be a > bit challenging to do because you will get an interrupt about "something > has changed" and you would have to run the callback from the PHY state > machine to determine this was indeed a result of triggering > auto-negotiation. Maybe polling for auto-negotiation like you do here is > good enough. > OK, then I would poll for autoneg finished in phy_speed_down and remove the polling option from phy_speed_up. I will do some tests with this before submitting a v2.
> One nit, you might have to check for those functions that the PHY did > have auto-negotiation enabled and was not forced. > This I'm doing already, or do you mean something different?
