RE: [PATCH v3] net: macb: add support for high speed interface

[Parshuram Raju Thombare <pthombar@cadence.com>]

Hi,

I was trying to find out any ethernet driver where this issue of selecting appropriate
pcs_ops due to phylink changing interface mode dynamically is handled. 
But, apparently, so far only mvpp2 has adapted pcs_ops. And even in mvpp2, it is
not obvious how this case is handled. 

Also, apart from interface mode changed due to SFPs with different types of PHY
being used, it is not clear when phylink selects interface mode different than it
initially requested to the ethernet driver.

>pcs_config and pcs_link_up passes "interface" as an argument, and in
>pcs_get_state call "state->interface" appeared to be populated just before
>calling it and hence should be valid.

It seems state->interface in pcs_get_state is not always valid when SFPs with
different types of PHY are used.  
There is a chance of SFP with different type of PHY is inserted, eventually invoking
phylink_resolve for interface mode different than phylink initially requested,
and causing major reconfiguration.

However, pcs_get_state is called before major reconfiguration, where selecting
which pcs_ops and PCS to be used is difficult without correct interface mode.  

As struct phylink and hence phy_state is private to phylink layer, IMO this need to be
handled at phylink level by passing appropriate interface mode to all necessary
methods registered by drivers.

Something like

523 static void phylink_mac_pcs_get_state(struct phylink *pl,
 524                                       struct phylink_link_state *state)
 525 {
 526         linkmode_copy(state->advertising, pl->link_config.advertising);
 527         linkmode_zero(state->lp_advertising);
 528         if (pl->phydev)
 529                 state->interface = pl->phy_state.interface;
 530         else
 531                 state->interface = pl->link_config.interface;
 532         state->an_enabled = pl->link_config.an_enabled;
 533         state->speed = SPEED_UNKNOWN;
 534         state->duplex = DUPLEX_UNKNOWN;
 535         state->pause = MLO_PAUSE_NONE;
 536         state->an_complete = 0;
 537         state->link = 1;
 538
 539         if (pl->pcs_ops)
 540                 pl->pcs_ops->pcs_get_state(pl->pcs, state);
 541         else
 542                 pl->mac_ops->mac_pcs_get_state(pl->config, state);


Regards,
Parshuram Thombare