Re: [PATCH v9 4/4] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Jul 04, 2024 at 04:01:13PM +0200, Kamil Horák (2N) wrote:
> 
> On 6/22/24 21:12, Andrew Lunn wrote:
> > On Fri, Jun 21, 2024 at 01:26:33PM +0200, Kamil Horák (2N) wrote:
> > > Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
> > > Create set of functions alternative to IEEE 802.3 to handle
> > > configuration of these modes on compatible Broadcom PHYs.
> > What i've not seen anywhere is a link between BroadR-Reach and LRE.
> > Maybe you could explain the relationship here in the commit message?
> > And maybe also how LDS fits in.
> 
> Tried to extend it a bit... LRE should be for "Long Reach Ethernet" but
> Broadcom
> 
> only uses the acronym in the datasheets... LDS is "Long-Distance Signaling",
> really screwed
> 
> term for a link auto-negotiation...

You are allowed to ignore the data sheet. If using AN makes the code
more understandable, use AN. Just add a comment in the commit message.

> > > +static int bcm54811_read_abilities(struct phy_device *phydev)
> > > +{
> > > +	int i, val, err;
> > > +	u8 brr_mode;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(bcm54811_linkmodes); i++)
> > > +		linkmode_clear_bit(bcm54811_linkmodes[i], phydev->supported);
> > I think that needs a comment since it is not clear what is going on
> > here. What set these bits in supported?
> 
> This is an equivalent of genphy_read_abilities for an IEEE PHY, that is, it
> fills the phydev->supported bit array exactly
> 
> as genphy_read_abilities does. The genphy_read_abilities is even called if
> the PHY is not in BRR mode.

I lost the context. But genphy_read_abilities() is only called if
phydrv->get_features is not set. Don't you make use of this
bcm54811_read_abilities for get_features? So i'm wondering, what set
these bits in the first place?


> > > +
> > > +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (brr_mode) {
> > I would expect the DT property to be here somewhere. If the DT
> > property is present, set phydev->supported to only the BRR modes,
> > otherwise set it to the standard baseT modes. That should then allow
> > the core to do most of the validation. This is based on my
> > understanding the coupling hardware makes the two modes mutually
> > exclusive?
> 
> From my point of view relying on DT property only would imply to validate
> the setting with what is read from the PHY on
> 
> all code locations where it is currently read by bcm5481x_get_brrmode.

In general, the DT value is the source of truth. It does not matter
how the PHY is strapped etc, we should reconfigure it how the DT
property indicates. So i really would set phydev->supported based on
it.

> > > +	/* With BCM54811, BroadR-Reach implies no autoneg */
> > > +	if (brr)
> > > +		phydev->autoneg = 0;
> > So long as phydev->supported does not indicate autoneg, this should
> > not happen.
> 
> I also thought so but unfortunately our batch of bcm54811 indicates possible
> autoneg in its status register
> 
>  (LRESR_LDSABILITY) but refuses to negotiate. So this is rather a
> preparation for bcm54810 full support. Unlike bcm54811,

If the hardware is broke, feel free to ignore the bit. I would also
keep it KISS. If somebody does want bcm54810 to auto-neg, they can add
the feature, and ask you to test for regressions with the bcm54811.

	Andrew