On 08.01.2019 20:29, Sean Wang wrote:
> On Tue, Jan 8, 2019 at 11:07 AM Frank Wunderlich
> <fran...@public-files.de> wrote:
>>
>>
>>> In mtk_phy_connect() I would say this complete block can be removed.
>>> All this is handled internally by phylib.
>>>
>>> dev->phydev->autoneg = AUTONEG_ENABLE;
>>> dev->phydev->speed = 0;
>>> dev->phydev->duplex = 0;
>>>
>>> phy_set_max_speed(dev->phydev, SPEED_1000);
>>> phy_support_asym_pause(dev->phydev);
>>> linkmode_copy(dev->phydev->advertising, dev->phydev->supported);
>>> linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>>> dev->phydev->advertising);
>>> phy_start_aneg(dev->phydev);
>>>
>>> Could you please test this?
>>
>> seems to work well, no warning in dmesg
>>
>> traffic 940Mbit sending, 900 receive
>>
>
> The change to phy initialized valve determined by phylib looks good to
> me and simplify the mac driver a lot. You could have it in v2.
>
>>> And a further proposal:
>>> Instead of the open-coded dev_info() in mtk_phy_connect_node()
>>> you could use phy_attached_info().
>>
>> have not tried this yet...because it's only an info-message and needs
>> different parameters...have to look how i can convert it
>
> I think it can be done in a separate patch
>
>>
>>> Calling phy_start() in the ndo_open callback is fine, you can leave
>>> it there.
>>
>> have it re-added
>>
>> pushed to my repo: https://github.com/frank-w/BPI-R2-4.14/commits/5.0-rc
>>
>> regards Frank
>
When talking about improvements: Also this
if (dev->phydev->link)
netif_carrier_on(dev);
else
netif_carrier_off(dev);
in mtk_phy_link_adjust() isn't needed. It's done by phy_link_change() in phylib.
