Re: [PATCH net-next v3 4/9] net: phy: add Lynx PCS module

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Jun 23, 2020 at 11:49:28AM +0000, Ioana Ciornei wrote:
> > This should be added to phylink_mii_c45_pcs_get_state().  There is nothing that
> > is Lynx PCS specific here.
> 
> The USXGMII standard only describes the auto-negotiation word, not the MMD
> where this can be found (MMD_VEND2 in this case).
> I would not add a generic phylink herper that reads the MMD and also
> decodes it.
> Maybe a helper that just decodes the USXGMII word read from the
> Lynx module - phylink_decode_usxgmii_word(state, lpa) ?

Yes, you're right - as they come from the vendor 2 MMD, there's no
standard.  So yes, just a helper to decode the USXGMII word please.

> > > +static void lynx_pcs_get_state_2500basex(struct mdio_device *pcs,
> > > +					 struct phylink_link_state *state) {
> > > +	struct mii_bus *bus = pcs->bus;
> > > +	int addr = pcs->addr;
> > > +	int bmsr, lpa;
> > > +
> > > +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
> > > +	lpa = mdiobus_read(bus, addr, MII_LPA);
> > > +	if (bmsr < 0 || lpa < 0) {
> > > +		state->link = false;
> > > +		return;
> > > +	}
> > > +
> > > +	state->link = !!(bmsr & BMSR_LSTATUS);
> > > +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> > > +	if (!state->link)
> > > +		return;
> > > +
> > > +	state->speed = SPEED_2500;
> > > +	state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX;
> > 
> > How do you know the other side is using pause frames, or is capable of dealing
> > with them?
> 
> Isn't this done by also looking into the PHY's pause frame bits and
> enabling pause frames in the MAC only when both the PCS and the PHY
> have flow enabled?

You are assuming that there is a PHY to read that information from.

There may not be a PHY - I have 2500base-X fibre links here, there is
no PHY to read that from, there is only the PCS - but this runs with
the configuration word present, so is not supported by this code (at
least at the moment.)

If there is a PHY, these bits will not be used anyway, so there's no
point setting them.

> Yep, will remove. I've gone through the documentation and the register
> should be initialized to 0x0001 when in SGMII mode
> (as done by phylink_mii_c22_pcs_config()).

Yep, that was actually written referring to the LX2160A documentation.

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