[PATCH v3 0/2] Add new PHY APIs to framework to get/set PHY attributes

[PATCH v3 0/2] Add new PHY APIs to framework to get/set PHY attributes

[Swapnil Jakhade]

This patch series adds a new pair of PHY APIs that can be used to get/set
all the PHY attributes. It also adds a new PHY attribute max_link_rate.

It includes following patches:

1. v3-0001-phy-Add-new-PHY-attribute-max_link_rate-and-APIs-.patch
This patch adds max_link_rate as a new PHY attribute along with a pair of
APIs that allow using the generic PHY subsystem to get/set PHY attributes
supported by the PHY.

2. v3-0002-phy-cadence-torrent-Use-kernel-PHY-API-to-set-PHY.patch
This patch uses PHY API phy_set_attrs() to set corresponding PHY properties
in Cadence Torrent PHY driver. This will enable drivers using this PHY to
read these properties using PHY framework.

The phy_get_attrs() API will be used in the DRM bridge driver [1] which is
in the process of upstreaming.

[1]

https://lkml.org/lkml/2020/2/26/263

Version History:

v3:
    - Add comment describing new PHY attribute max_link_rate
    - Use of memcpy to copy structure members
    - Change commit log a bit

v2:
    - Implemented single pair of functions to get/set all PHY attributes

Swapnil Jakhade (2):
  phy: Add new PHY attribute max_link_rate and APIs to get/set PHY
    attributes
  phy: cadence-torrent: Use kernel PHY API to set PHY attributes

 drivers/phy/cadence/phy-cadence-torrent.c |  7 +++++++
 include/linux/phy/phy.h                   | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+)

-- 
2.26.1

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

[Swapnil Jakhade]

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));
+}
 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

[PATCH v3 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Swapnil Jakhade]

Use generic PHY framework function phy_set_attrs() to set number
of lanes and maximum link rate supported by PHY.

Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index 7116127358ee..af81707ff0c6 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	struct cdns_torrent_phy *cdns_phy;
 	struct device *dev = &pdev->dev;
 	struct phy_provider *phy_provider;
+	struct phy_attrs torrent_attr;
 	const struct of_device_id *match;
 	struct cdns_torrent_data *data;
 	struct device_node *child;
@@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 				 cdns_phy->phys[node].num_lanes,
 				 cdns_phy->max_bit_rate / 1000,
 				 cdns_phy->max_bit_rate % 1000);
+
+			torrent_attr.bus_width = cdns_phy->phys[node].num_lanes;
+			torrent_attr.max_link_rate = cdns_phy->max_bit_rate;
+			torrent_attr.mode = PHY_MODE_DP;
+
+			phy_set_attrs(gphy, torrent_attr);
 		} else {
 			dev_err(dev, "Driver supports only PHY_TYPE_DP\n");
 			ret = -ENOTSUPP;
-- 
2.26.1

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

[Vinod Koul]

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?

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

>  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

-- 
~Vinod

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

[Kishon Vijay Abraham I]

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?
> 
> Also I see you are using phy_set_attrs() in second patch, why add
> phy_get_attrs() when we have no user?

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.

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
> 

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

[Swapnil Kashinath Jakhade]

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
> >