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
