RE: [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs to get/set PHY attributes

[Swapnil Kashinath Jakhade <sjakhade@cadence.com>]

Hi,

Thanks for your comments.

> -----Original Message-----
> From: Kishon Vijay Abraham I <kishon@ti.com>
> Sent: Monday, July 13, 2020 5:28 PM
> To: Vinod Koul <vkoul@kernel.org>; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>
> Cc: linux-kernel@vger.kernel.org; maxime@cerno.tech; Milind Parab
> <mparab@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com>;
> nsekhar@ti.com; tomi.valkeinen@ti.com; jsarha@ti.com; praneeth@ti.com
> Subject: Re: [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and
> APIs to get/set PHY attributes
> 
> EXTERNAL MAIL
> 
> 
> 
> 
> On 7/13/2020 4:41 PM, Vinod Koul wrote:
> > On 13-07-20, 11:38, Swapnil Jakhade wrote:
> >> Add new PHY attribute max_link_rate to struct phy_attrs.
> >> Add a pair of PHY APIs to get/set all the PHY attributes.
> >> Use phy_set_attrs() to set attribute values in the PHY provider driver.
> >> Use phy_get_attrs() to get attribute values in the controller driver.
> >>
> >> Signed-off-by: Yuti Amonkar <yamonkar@cadence.com>
> >> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> >> ---
> >>  include/linux/phy/phy.h | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index
> >> bcee8eba62b3..7fb59359ab7b 100644
> >> --- a/include/linux/phy/phy.h
> >> +++ b/include/linux/phy/phy.h
> >> @@ -115,10 +115,12 @@ struct phy_ops {
> >>  /**
> >>   * struct phy_attrs - represents phy attributes
> >>   * @bus_width: Data path width implemented by PHY
> >> + * @max_link_rate: Maximum link rate supported by PHY (in Mbps)
> >>   * @mode: PHY mode
> >>   */
> >>  struct phy_attrs {
> >>  	u32			bus_width;
> >> +	u32			max_link_rate;
> >>  	enum phy_mode		mode;
> >>  };
> >>
> >> @@ -231,6 +233,16 @@ static inline void phy_set_bus_width(struct phy
> >> *phy, int bus_width)  {
> >>  	phy->attrs.bus_width = bus_width;
> >>  }
> >> +
> >> +static inline void phy_get_attrs(struct phy *phy, struct phy_attrs
> >> +*attrs) {
> >> +	memcpy(attrs, &phy->attrs, sizeof(struct phy_attrs)); }
> >> +
> >> +static inline void phy_set_attrs(struct phy *phy, struct phy_attrs
> >> +attrs) {
> >> +	memcpy(&phy->attrs, &attrs, sizeof(struct phy_attrs)); }
> >
> > we already have APIs for mode and bus_width so why not add one for
> > link_rate and call them?

First version of this patch series [1] was based on this approach. But as per the further discussion in the same thread [2], phy_get_attrs/phy_set_attrs are implemented.
[1] https://lkml.org/lkml/2020/4/28/139

[2] https://lkml.org/lkml/2020/5/18/472

> >
> > Also I see you are using phy_set_attrs() in second patch, why add
> > phy_get_attrs() when we have no user?

As mentioned in cover letter, phy_get_attrs() is planned to be used in Cadence MHDP DRM bridge driver for DisplayPort. Next version of this driver patch series [3] will use this API.

[3] https://lkml.org/lkml/2020/2/26/263

> 
> One of the factors to consider is who will set the attributes; it could either be
> phy provider (like 2/2 of this series) or phy consumer (factors like PCIe speed,
> lane are usually negotiated with the phy consumer).
> 
> Now if phy provider is setting/getting the attributes, then
> phy_set_attrs/phy_get_attrs should be protected by mutex. We don't want
> to be updating attributes when phy consumer is doing some phy ops.
> 
> If phy_consumer is updating attributes, it could directly access the phy
> attributes if it's updating within one of those phy ops. Don't really see a need
> for using an API to update the attributes then.
> 
> However if it's updating outside the phy_ops, then it would still make sense
> to use the APIs to update attributes with all those mutex protection.

Ok. Please check if following implementation is correct in phy_get_attrs/phy_set_attrs  APIs for the above case.

mutex_lock(&phy->mutex);
memcpy(....);
mutex_unlock(&phy->mutex);


Thanks & regards,
Swapnil
> 
> Regards
> Kishon
> >
> >>  struct phy *phy_get(struct device *dev, const char *string);  struct
> >> phy *phy_optional_get(struct device *dev, const char *string);
> >> struct phy *devm_phy_get(struct device *dev, const char *string); @@
> >> -389,6 +401,16 @@ static inline void phy_set_bus_width(struct phy *phy,
> int bus_width)
> >>  	return;
> >>  }
> >>
> >> +static inline void phy_get_attrs(struct phy *phy, struct phy_attrs
> >> +*attrs) {
> >> +	return;
> >> +}
> >> +
> >> +static inline void phy_set_attrs(struct phy *phy, struct phy_attrs
> >> +attrs) {
> >> +	return;
> >> +}
> >> +
> >>  static inline struct phy *phy_get(struct device *dev, const char
> >> *string)  {
> >>  	return ERR_PTR(-ENOSYS);
> >> --
> >> 2.26.1
> >