On 04.07.2018 16:46, Andrew Lunn wrote: > On Mon, Jul 02, 2018 at 11:54:54PM +0200, Heiner Kallweit wrote: >> On 02.07.2018 23:21, Andrew Lunn wrote: >>>> - auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM; >>> >>> This bit you probably want to keep. The PHY never says it support >>> Pause. The MAC needs to enable pause if the MAC supports pause. >>> >> Actually I assumed that phylib would do this for me. But: >> In phy_probe() first phydev->supported is copied to >> phydev->advertising, and only after this both pause flags are added >> to phydev->supported. Therefore I think they are not advertised. >> Is this intentional? It sounds a little weird to me to add the >> pause flags to the supported features per default, but not >> advertise them. > > phylib has no idea if the MAC supports Pause. So it should not enable > it by default. The MAC needs to enable it. And a lot of MAC drivers > get this wrong... > >> Except e.g. we call by chance phy_set_max_speed(), which copies >> phydev->supported to phydev->advertising after having adjusted >> the supported speeds. > > As you correctly pointed out, phy_set_max_speed() is masking out too > much. > >> If this is not a bug, then where would be the right place to add >> the pause flags to phydev->advertising? > > Before you call phy_start(). > Thanks for the clarification. I think beginning of next week I can provide a v2 of the patch series.
Heiner > Andrew >
