Re: [RFC Patch net-next] r8169: add phylink support

[Andrew Lunn]

> And one more question, when the driver initializes RTL8116af,
> r8169_mdio_register() will still be called. So tp->phydev =
> mdiobus_get_phy(new_bus, 0) will still be executed. This feels redundant
> since we are now using phylink. However, if I bypass this assignment,
> it breaks several existing functions which are strongly rely on
> tp->phydev. Is it acceptable to keep this tp->phydev for now, or is
> there a preferred way to handle this problem?

You probably want to do some refactoring, before swapping to phylink.

rtl_link_chg_patch() needs to know the link speed, so looks at
phydev->speed. The rtl_mac_link_up() call gets passed the speed.  So i
would refactor rtl_link_chg_patch() to be passed the speed. And then
when you swap to phylink, and r8169_phylink_handler() disappears you
can call rtl_link_chg_patch() from mac_link_up.

rtl_coalesce_info() i would refactor to put the current speed in tp,
set is when rtl_mac_link_up() is called, and back to -1 when
rtl_mac_link_down is called. This can be another refactor patch, have
r8169_phylink_handler() set the speed.

Same change for r8169_get_tx_lpi_timer_us().

EEE is quite different for phylink, so rtl8169_get_eee() and
rtl8169_set_eee() will change and should not need to reference phydev.
The same is true for rtl8169_get_pauseparam() and
rtl8169_set_pauseparam().

rtl8169_set_link_ksettings() is broken! It should not be touching
phydev members like this. That also mostly disappears with the swap to
phylink.

r8169_apply_firmware() is interesting. Is this putting firmware in the
MAC or the PHY? Or both? If it was only PHY, i would move this into
the PHY driver.

I would try to move the code in r8169_phy_config.c into the PHY
driver. The tricky part is knowing what MAC version is being used.

/* Chip doesn't support pause in jumbo mode */ should be done
differently. We have helpers phy_support_sym_pause() and
phy_support_asym_pause(). A new helper should be added
phy_support_no_pause(). Both advertising and supported should be
cleared. And these should be an else clause for when jumbo is
disabled, and pause can be used again. phylink however does this in a
different way. It might be you need to call phylink_stop() &
phylink_start() and ensure the mac_get_caps() returns the correct
capabilities.

Try to remove as many references to phydev as you can.

I would also suggest lots of small patches when doing this
refactoring. When you break something, it will make it easier to
figure out what broke it.

    Andrew