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

[Kishon Vijay Abraham I <kishon@ti.com>]

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:
>> 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:
>>> 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 <lars.povlsen@microchip.com>
>>> 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
> 
>>
>>> +
>>>  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.
> 
>