Re: [RFC PATCH net-next] net: dsa: sja1105: let phylink help with the replay of link callbacks

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Thu, Oct 03, 2024 at 05:07:53PM +0300, Vladimir Oltean wrote:
> sja1105_static_config_reload() changes major settings in the switch and
> it requires a reset. A use case is to change things like Qdiscs (but see
> sja1105_reset_reasons[] for full list) while PTP synchronization is
> running, and the servo loop must not exit the locked state (s2).
> Therefore, stopping and restarting the phylink instances of all ports is
> not desirable, because that also stops the phylib state machine, and
> retriggers a seconds-long auto-negotiation process that breaks PTP.
> Thus, saving and restoring the link management settings is handled
> privately by the driver.
> 
> The method got progressively more complex as SGMII support got added,
> because this is handled through the xpcs phylink_pcs component, to which
> we don't have unfettered access. Nonetheless, the switch reset line is
> hardwired to also reset the XPCS, creating a situation where it loses
> state and needs to be reprogrammed at a moment in time outside phylink's
> control.
> 
> Although commits 907476c66d73 ("net: dsa: sja1105: call PCS
> config/link_up via pcs_ops structure") and 41bf58314b17 ("net: dsa:
> sja1105: use phylink_pcs internally") made the sja1105 <-> xpcs
> interaction slightly prettier, we still depend heavily on the PCS being
> "XPCS-like", because to back up its settings, we read the MII_BMCR
> register, through a mdiobus_c45_read() operation, breaking all layering
> separation.
> 
> But the phylink instance already has all that state, and more. It's just
> that it's private. In this proposal, phylink offers 2 helpers for
> walking the MAC and PCS drivers again through the callbacks required
> during a destructive reset operation: mac_link_down() -> pcs_link_down()
> -> mac_config() -> pcs_config() -> mac_link_up() -> pcs_link_up().
> 
> This creates the unique opportunity to simplify away even more code than
> just the xpcs handling from sja1105_static_config_reload().
> The sja1105_set_port_config() method is also invoked from
> sja1105_mac_link_up(). And since that is now called directly by
> phylink.. we can just remove it from sja1105_static_config_reload().
> This makes it possible to re-merge sja1105_set_port_speed() and
> sja1105_set_port_config() in a later change.
> 
> Note that my only setups with sja1105 where the xpcs is used is with the
> xpcs on the CPU-facing port (fixed-link). Thus, I cannot test xpcs + PHY.
> But the replay procedure walks through all ports, and I did test a
> regular RGMII user port + a PHY.
> 
> ptp4l[54.552]: master offset          5 s2 freq    -931 path delay       764
> ptp4l[55.551]: master offset         22 s2 freq    -913 path delay       764
> ptp4l[56.551]: master offset         13 s2 freq    -915 path delay       765
> ptp4l[57.552]: master offset          5 s2 freq    -919 path delay       765
> ptp4l[58.553]: master offset         13 s2 freq    -910 path delay       765
> ptp4l[59.553]: master offset         13 s2 freq    -906 path delay       765
> ptp4l[60.553]: master offset          6 s2 freq    -909 path delay       765
> ptp4l[61.553]: master offset          6 s2 freq    -907 path delay       765
> ptp4l[62.553]: master offset          6 s2 freq    -906 path delay       765
> ptp4l[63.553]: master offset         14 s2 freq    -896 path delay       765
> $ ip link set br0 type bridge vlan_filtering 1
> [   63.983283] sja1105 spi2.0 sw0p0: Link is Down
> [   63.991913] sja1105 spi2.0: Link is Down
> [   64.009784] sja1105 spi2.0: Reset switch and programmed static config. Reason: VLAN filtering
> [   64.020217] sja1105 spi2.0 sw0p0: Link is Up - 1Gbps/Full - flow control off
> [   64.030683] sja1105 spi2.0: Link is Up - 1Gbps/Full - flow control off
> ptp4l[64.554]: master offset       7397 s2 freq   +6491 path delay       765
> ptp4l[65.554]: master offset         38 s2 freq   +1352 path delay       765
> ptp4l[66.554]: master offset      -2225 s2 freq    -900 path delay       764
> ptp4l[67.555]: master offset      -2226 s2 freq   -1569 path delay       765
> ptp4l[68.555]: master offset      -1553 s2 freq   -1563 path delay       765
> ptp4l[69.555]: master offset       -865 s2 freq   -1341 path delay       765
> ptp4l[70.555]: master offset       -401 s2 freq   -1137 path delay       765
> ptp4l[71.556]: master offset       -145 s2 freq   -1001 path delay       765
> ptp4l[72.558]: master offset        -26 s2 freq    -926 path delay       765
> ptp4l[73.557]: master offset         30 s2 freq    -877 path delay       765
> ptp4l[74.557]: master offset         47 s2 freq    -851 path delay       765
> ptp4l[75.557]: master offset         29 s2 freq    -855 path delay       765
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> This was not what was discussed in
> https://lore.kernel.org/netdev/E1ssjcz-005Ns9-D5@rmk-PC.armlinux.org.uk/,
> but I will approach that perhaps differently, depending on the feedback here.

I don't want to repost this because I have no updates to it, but could I
get some feedback please?