Re: [PATCH v14 2/4] phy: Add media type and speed serdes configuration interfaces

Hi Steen,

On 12/02/21 6:35 pm, Steen Hegelund wrote:

Hi Kishon,

On Fri, 2021-02-12 at 17:02 +0530, Kishon Vijay Abraham I wrote:

quoted

EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe

Hi Steen,

On 10/02/21 2:22 pm, Steen Hegelund wrote:

quoted

Provide new phy configuration interfaces for media type and speed
that
allows allows e.g. PHYs used for ethernet to be configured with
this
information.

Signed-off-by: Lars Povlsen <redacted>
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/phy/phy-core.c  | 30 ++++++++++++++++++++++++++++++
 include/linux/phy/phy.h | 26 ++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 71cb10826326..ccb575b13777 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c

@@ -373,6 +373,36 @@ int phy_set_mode_ext(struct phy *phy, enum

phy_mode mode, int submode)
 }
 EXPORT_SYMBOL_GPL(phy_set_mode_ext);

+int phy_set_media(struct phy *phy, enum phy_media media)
+{
+     int ret;
+
+     if (!phy || !phy->ops->set_media)
+             return 0;
+
+     mutex_lock(&phy->mutex);
+     ret = phy->ops->set_media(phy, media);
+     mutex_unlock(&phy->mutex);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(phy_set_media);
+
+int phy_set_speed(struct phy *phy, int speed)
+{
+     int ret;
+
+     if (!phy || !phy->ops->set_speed)
+             return 0;
+
+     mutex_lock(&phy->mutex);
+     ret = phy->ops->set_speed(phy, speed);
+     mutex_unlock(&phy->mutex);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(phy_set_speed);

Can't speed derived from mode? Do we need a separate set_speed
function?

Thanks
Kishon

Yes the client will need to be able to choose the speed as needed: 
e.g. lower than the serdes mode supports, in case the the media or the
other end is not capable of running that speed.  

An example is a 10G and 25G serdes connected via DAC and as there is no
inband autoneg, the 25G client would have to manually select 10G speed
to communicate with its partner.

Okay. Is it going to be some sort of manual negotiation where the
Ethernet controller invokes set_speed with different speeds? Or the
Ethernet controller will get the speed using some out of band mechanism
and invokes set_speed once with the actual speed?

Thanks
Kishon

quoted

quoted

+
 int phy_reset(struct phy *phy)
 {
      int ret;

diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index e435bdb0bab3..e4fd69a1faa7 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h

@@ -44,6 +44,12 @@ enum phy_mode {

      PHY_MODE_DP
 };

+enum phy_media {
+     PHY_MEDIA_DEFAULT,
+     PHY_MEDIA_SR,
+     PHY_MEDIA_DAC,
+};
+
 /**
  * union phy_configure_opts - Opaque generic phy configuration
  *

@@ -64,6 +70,8 @@ union phy_configure_opts {

  * @power_on: powering on the phy
  * @power_off: powering off the phy
  * @set_mode: set the mode of the phy
+ * @set_media: set the media type of the phy (optional)
+ * @set_speed: set the speed of the phy (optional)
  * @reset: resetting the phy
  * @calibrate: calibrate the phy
  * @release: ops to be performed while the consumer relinquishes
the PHY

@@ -75,6 +83,8 @@ struct phy_ops {

      int     (*power_on)(struct phy *phy);
      int     (*power_off)(struct phy *phy);
      int     (*set_mode)(struct phy *phy, enum phy_mode mode, int
submode);
+     int     (*set_media)(struct phy *phy, enum phy_media media);
+     int     (*set_speed)(struct phy *phy, int speed);

      /**
       * @configure:

@@ -215,6 +225,8 @@ int phy_power_off(struct phy *phy);

 int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int
submode);
 #define phy_set_mode(phy, mode) \
      phy_set_mode_ext(phy, mode, 0)
+int phy_set_media(struct phy *phy, enum phy_media media);
+int phy_set_speed(struct phy *phy, int speed);
 int phy_configure(struct phy *phy, union phy_configure_opts
*opts);
 int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
               union phy_configure_opts *opts);

@@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy

*phy, enum phy_mode mode,
 #define phy_set_mode(phy, mode) \
      phy_set_mode_ext(phy, mode, 0)

+static inline int phy_set_media(struct phy *phy, enum phy_media
media)
+{
+     if (!phy)
+             return 0;
+     return -ENOSYS;
+}
+
+static inline int phy_set_speed(struct phy *phy, int speed)
+{
+     if (!phy)
+             return 0;
+     return -ENOSYS;
+}
+
 static inline enum phy_mode phy_get_mode(struct phy *phy)
 {
      return PHY_MODE_INVALID;

Thanks for your comments.