Re: [PATCH v12 2/4] phy: Add ethernet serdes configuration option

[Steen Hegelund]

Hi Kishon,


On Fri, 2021-01-15 at 14:14 +0530, Kishon Vijay Abraham I wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi,
> 
> On 07/01/21 2:49 pm, Steen Hegelund wrote:
> > Provide a new ethernet phy configuration structure, that
> > allow PHYs used for ethernet to be configured with
> > speed, media type and clock information.
> > 
> > Signed-off-by: Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>
> > Signed-off-by: Steen Hegelund <steen.hegelund@xxxxxxxxxxxxx>
> > Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
> > ---
> >  include/linux/phy/phy-ethernet-serdes.h | 30
> > +++++++++++++++++++++++++
> >  include/linux/phy/phy.h                 |  4 ++++
> >  2 files changed, 34 insertions(+)
> >  create mode 100644 include/linux/phy/phy-ethernet-serdes.h
> > 
> > diff --git a/include/linux/phy/phy-ethernet-serdes.h
> > b/include/linux/phy/phy-ethernet-serdes.h
> > new file mode 100644
> > index 000000000000..d2462fadf179
> > --- /dev/null
> > +++ b/include/linux/phy/phy-ethernet-serdes.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> > +/*
> > + * Microchip Sparx5 Ethernet SerDes driver
> > + *
> > + * Copyright (c) 2020 Microschip Inc
> > + */
> > +#ifndef __PHY_ETHERNET_SERDES_H_
> > +#define __PHY_ETHERNET_SERDES_H_
> > +
> > +#include <linux/types.h>
> > +
> > +enum ethernet_media_type {
> > +     ETH_MEDIA_DEFAULT,
> > +     ETH_MEDIA_SR,
> > +     ETH_MEDIA_DAC,
> > +};
> > +
> > +/**
> > + * struct phy_configure_opts_eth_serdes - Ethernet SerDes This
> > structure is used
> > + * to represent the configuration state of a Ethernet Serdes PHY.
> > + * @speed: Speed of the serdes interface in Mbps
> > + * @media_type: Specifies which media the serdes will be using
> > + */
> > +struct phy_configure_opts_eth_serdes {
> > +     u32                        speed;
> > +     enum ethernet_media_type   media_type;
> > +};
> 
> Is media type going to be determined dynamically by the Ethernet
> controller. If it's not determined dynamically, it shouldn't be in
> PHY
> ops but rather as a DT parameter.

Yes the media type is dynamic, as it will be determined by the feedback
from the attached SFP or DAC attached, which can be changed at any
time, so it is not static in a way that allows it to be part of the DT.

> 
> phy_configure_opts is mostly used with things like DP where the
> controller probes the configurations supported by SERDES using the
> configure and validate ops. I don't think for Ethernet it is
> required.

>From what you explained I think the situation is very similar with the
Ethernet SerDes in that the actual media (and speed) is not known in
advance, but will be obtained "out-of-band" by the client and may
change at any point in time when the user changes the physical setup
(e.g cables).

> 
> Thanks
> Kishon
> 
> > +
> > +#endif
> > +
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > index e435bdb0bab3..78ecb375cede 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -18,6 +18,7 @@
> > 
> >  #include <linux/phy/phy-dp.h>
> >  #include <linux/phy/phy-mipi-dphy.h>
> > +#include <linux/phy/phy-ethernet-serdes.h>
> > 
> >  struct phy;
> > 
> > @@ -49,11 +50,14 @@ enum phy_mode {
> >   *
> >   * @mipi_dphy:       Configuration set applicable for phys
> > supporting
> >   *           the MIPI_DPHY phy mode.
> > + * @eth_serdes: Configuration set applicable for phys supporting
> > + *           the ethernet serdes.
> >   * @dp:              Configuration set applicable for phys
> > supporting
> >   *           the DisplayPort protocol.
> >   */
> >  union phy_configure_opts {
> >       struct phy_configure_opts_mipi_dphy     mipi_dphy;
> > +     struct phy_configure_opts_eth_serdes    eth_serdes;
> >       struct phy_configure_opts_dp            dp;
> >  };
> > 
> > 

BR
Steen