Re: [PATCH net-next 5/7] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Oleksij,

On Thu, 12 Sep 2024 06:46:29 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Maxime,
> 
> On Wed, Sep 11, 2024 at 11:27:09PM +0200, Maxime Chevallier wrote:
> ...
> >  
> > +/**
> > + * struct phy_device_config - General PHY device configuration parameters for
> > + * status reporting and bulk configuration
> > + *
> > + * A structure containing generic PHY device information, allowing to expose
> > + * internal status to userspace, and perform PHY configuration in a controlled
> > + * manner.
> > + *
> > + * @isolate: The MII-side isolation status of the PHY
> > + * @loopback: The loopback state of the PHY
> > + */
> > +struct phy_device_config {
> > +	bool isolate;
> > +	bool loopback;
> > +};  
>  
> I would recommend to have loopback enum. There are different levels of
> loopback:
> https://www.ti.com/document-viewer/DP83TD510E/datasheet#GUID-50834313-DEF1-42FB-BA00-9B0902B2D7E4/TITLE-SNLS656SNLS5055224
> 
> I imagine something like this:
> 
> /*
>  * enum phy_loopback_mode - PHY loopback modes
>  * These modes represent different loopback configurations to
>  * facilitate in-circuit testing of the PHY's digital and analog paths.
>  */
> enum phy_loopback_mode {
> 	PHY_LOOPBACK_NONE = 0,		/* No loopback mode enabled */
> 	PHY_LOOPBACK_MII,		/* MII Loopback: MAC to PHY internal loopback */
> 	PHY_LOOPBACK_PCS,		/* PCS Loopback: PCS layer loopback, no signal processing */
> 	PHY_LOOPBACK_DIGITAL,		/* Digital Loopback: Loops back entire digital TX/RX path */
> 	PHY_LOOPBACK_ANALOG,		/* Analog Loopback: Loops back after analog front-end */
> 	PHY_LOOPBACK_FAR_END		/* Far-End (Reverse) Loopback: Receiver to MAC interface loopback */
> };

I agree with you on that, having the ability to fine-tune where the
loopback happens is really useful for debug. The main problem I would
see is to come-up with a set of modes that are somewhat generic, as
vendors implement a wide variety of loopback modes.

For example, on BaseT4 PHYs the Analog loopback doesn't exist and is
more akin to using a loopback stub, whereas the Digital loopback would
be a loopback at the PMD level (I don't know if that even exists).

That being said, the list of possible places to setup the loopback
within a PHY is finite and it's conceivable to come-up with a set of
loopback modes.

Thanks a lot for the feedback,

Maxime