Re: [PATCH net-next V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers

[Serge Semin]

Hi Russell

On Fri, Oct 27, 2023 at 12:54:36PM +0100, Russell King (Oracle) wrote:
> On Fri, Oct 27, 2023 at 02:04:15PM +0300, Serge Semin wrote:
> > Cc += Russell
> > 
> > * It's a good practice to add all the reviewers to Cc in the new patch
> > * revisions.
> > 
> > On Fri, Oct 27, 2023 at 10:13:06AM +0530, Raju Lakkaraju wrote:
> > > Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause
> > > 
> > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@xxxxxxxxxxxxx>
> > 
> > With a nitpick below clarified, feel free to add:
> > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > 
> > > ---
> > >  drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++++++
> > >  drivers/net/pcs/pcs-xpcs.h |  2 ++
> > >  2 files changed, 31 insertions(+)
> > > 
> > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > > index 4dbc21f604f2..31f0beba638a 100644
> > > --- a/drivers/net/pcs/pcs-xpcs.c
> > > +++ b/drivers/net/pcs/pcs-xpcs.c
> > > @@ -1090,6 +1090,28 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
> > >  	return 0;
> > >  }
> > >  
> > > +static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs,
> > > +				    struct phylink_link_state *state)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS);
> > > +	if (ret < 0) {
> > > +		state->link = 0;
> > > +		return ret;
> > > +	}
> > > +
> > > +	state->link = !!(ret & DW_VR_MII_MMD_STS_LINK_STS);
> > > +	if (!state->link)
> > > +		return 0;
> > > +
> > > +	state->speed = SPEED_2500;
> > 
> > > +	state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX;
> > 
> > Why is it '|=' instead of just '='? Is it possible to have the 'pause'
> > field having some additional flags set which would be required to
> > preserve?
> 
> The code is correct. There are other flags on state->pause other than
> these, and phylink initialises state->pause prior to calling the
> function. The only flags that should be modified here are these two
> bits that the code is setting.
> 
> Phylink will initialise it to MLO_PAUSE_NONE if expecting autoneg, or
> the configured values if autoneg on the link is disabled.

Thanks for clarification. Then no more comments from my side in this
patch regard.

Regarding the XPCS driver in general. Based on what you said the rest
of the XPCS state getters are wrong in fully re-writing the 'pause'
field. Right?

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!