Re: [RFC PATCH v2 net-next 04/15] phy: allow querying the address of protocol converters through phy_get_status()

[Vladimir Oltean]

Hi Vinod,

On Fri, Sep 29, 2023 at 09:53:22PM +0530, Vinod Koul wrote:
> On 23-09-23, 16:48, Vladimir Oltean wrote:
> > The bit stream handled by a SerDes lane needs protocol converters to be
> > usable for Ethernet. On Freescale/NXP SoCs, those protocol converters
> > are located on the internal MDIO buses of the Ethernet MACs that need
> > them.
> > 
> > The location on that MDIO bus, on these SoCs, is not fixed, but given by
> > some control registers of the SerDes block itself.
> > 
> > Because no one modifies those addresses from the power-on default, so
> > far we've relied on hardcoding the default values in the device trees,
> > resulting in something like this:
> > 
> > 		pcs_mdio1: mdio@8c07000 {
> > 			compatible = "fsl,fman-memac-mdio";
> > 
> > 			pcs1: ethernet-phy@0 {
> > 				reg = <0>;
> > 			};
> > 		};
> > 
> > where the "reg" of "pcs1" can actually be retrieved from "serdes_1".
> > 
> > That was for the PCS. For AN/LT blocks, that can also be done, but the
> > MAC to PCS to AN/LT block mapping is non-trivial and extremely easy to
> > get wrong, which will confuse and frustrate any device tree writers.
> > 
> > The proposal is to take advantage of the fact that these protocol
> > converters *are* discoverable, and to side-step that entire device tree
> > mapping issue by not putting them in the device tree at all. So, one of
> > the consumers of the SerDes PHY uses the phy_get_status() API to figure
> > out the address on the MDIO bus, it also has a reference to the MDIO bus
> > => it can create the mdio_device in a non OF-based manner.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> > ---
> > v1->v2: patch is new
> > 
> >  include/linux/phy/phy.h | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > index f1f03fa66943..ee721067517b 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -56,6 +56,33 @@ enum phy_media {
> >  enum phy_status_type {
> >  	/* Valid for PHY_MODE_ETHERNET and PHY_MODE_ETHTOOL */
> >  	PHY_STATUS_CDR_LOCK,
> > +	PHY_STATUS_PCVT_ADDR,
> > +};
> > +
> > +/* enum phy_pcvt_type - PHY protocol converter type
> 
> It is not a generic protocol converter but an ethernet phy protocol
> converter, so i guess we should add that here (we are generic phy and
> not ethernet phy here!

Can you please show, using a diff, what modification you would like to see?
In my mind, enum phy_pcvt_type could also contain non-Ethernet protocol
converter types, and so, I didn't want to add "ethernet_" in it.
The "ETHERNET_" prefix will be part of the individual enum values that
are applicable to Ethernet.

> > + *
> > + * @PHY_PCVT_ETHERNET_PCS: Ethernet Physical Coding Sublayer, top-most layer of
> > + *			   an Ethernet PHY. Connects through MII to the MAC,
> > + *			   and handles link status detection and the conversion
> > + *			   of MII signals to link-specific code words (8b/10b,
> > + *			   64b/66b etc).
> > + * @PHY_PCVT_ETHERNET_ANLT: Ethernet Auto-Negotiation and Link Training,
> > + *			    bottom-most layer of an Ethernet PHY, beneath the
> > + *			    PMA and PMD. Its activity is only visible on the
> > + *			    physical medium, and it is responsible for
> > + *			    selecting the most adequate PCS/PMA/PMD set that
> > + *			    can operate on that medium.
> > + */
> > +enum phy_pcvt_type {
> > +	PHY_PCVT_ETHERNET_PCS,
> > +	PHY_PCVT_ETHERNET_ANLT,
> > +};
> > +
> > +struct phy_status_opts_pcvt {
> > +	enum phy_pcvt_type type;
> > +	union {
> > +		unsigned int mdio;
> > +	} addr;
> >  };
> >  
> >  /* If the CDR (Clock and Data Recovery) block is able to lock onto the RX bit
> > @@ -71,9 +98,11 @@ struct phy_status_opts_cdr {
> >   * union phy_status_opts - Opaque generic phy status
> >   *
> >   * @cdr:	Configuration set applicable for PHY_STATUS_CDR_LOCK.
> > + * @pcvt:	Configuration set applicable for PHY_STATUS_PCVT_ADDR.
> >   */
> >  union phy_status_opts {
> >  	struct phy_status_opts_cdr		cdr;
> > +	struct phy_status_opts_pcvt		pcvt;
> >  };
> >  
> >  /**
> > -- 
> > 2.34.1
> 
> -- 
> ~Vinod