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.povl...@microchip.com>
>>> Signed-off-by: Steen Hegelund <steen.hegel...@microchip.com>
>>> Reviewed-by: Andrew Lunn <and...@lunn.ch>
>>> Reviewed-by: Alexandre Belloni <alexandre.bell...@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.
>
>
