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

[PATCH v4 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. v4-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. v4-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:

v4:
    - Protect phy_get_attrs/phy_set_attrs APIs with mutex

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                   | 26 +++++++++++++++++++++++
 2 files changed, 33 insertions(+)

-- 
2.26.1

[PATCH v4 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_get_attrs() to get attribute values and phy_set_attrs()
to set attribute values.

Signed-off-by: Yuti Amonkar <yamonkar@cadence.com>
Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
---
 include/linux/phy/phy.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index bcee8eba62b3..5d8ebb056c1d 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,20 @@ 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)
+{
+	mutex_lock(&phy->mutex);
+	memcpy(attrs, &phy->attrs, sizeof(struct phy_attrs));
+	mutex_unlock(&phy->mutex);
+}
+
+static inline void phy_set_attrs(struct phy *phy, struct phy_attrs attrs)
+{
+	mutex_lock(&phy->mutex);
+	memcpy(&phy->attrs, &attrs, sizeof(struct phy_attrs));
+	mutex_unlock(&phy->mutex);
+}
 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 +405,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 v4 1/2] phy: Add new PHY attribute max_link_rate and APIs to get/set PHY attributes

[Kishon Vijay Abraham I]

On 7/17/2020 12:20 PM, 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_get_attrs() to get attribute values and phy_set_attrs()
> to set attribute values.
> 
> Signed-off-by: Yuti Amonkar <yamonkar@cadence.com>
> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>

> ---
>  include/linux/phy/phy.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index bcee8eba62b3..5d8ebb056c1d 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,20 @@ 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)
> +{
> +	mutex_lock(&phy->mutex);
> +	memcpy(attrs, &phy->attrs, sizeof(struct phy_attrs));
> +	mutex_unlock(&phy->mutex);
> +}
> +
> +static inline void phy_set_attrs(struct phy *phy, struct phy_attrs attrs)
> +{
> +	mutex_lock(&phy->mutex);
> +	memcpy(&phy->attrs, &attrs, sizeof(struct phy_attrs));
> +	mutex_unlock(&phy->mutex);
> +}
>  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 +405,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);
> 

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

[Laurent Pinchart]

Hi Swapnil,

Thank you for the patch.

On Fri, Jul 17, 2020 at 08:50:32AM +0200, 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_get_attrs() to get attribute values and phy_set_attrs()
> to set attribute values.
> 
> Signed-off-by: Yuti Amonkar <yamonkar@cadence.com>
> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> ---
>  include/linux/phy/phy.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index bcee8eba62b3..5d8ebb056c1d 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,20 @@ 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)
> +{
> +	mutex_lock(&phy->mutex);
> +	memcpy(attrs, &phy->attrs, sizeof(struct phy_attrs));
> +	mutex_unlock(&phy->mutex);
> +}
> +
> +static inline void phy_set_attrs(struct phy *phy, struct phy_attrs attrs)

Passing the second argument by (const) pointer would be more efficient.

> +{
> +	mutex_lock(&phy->mutex);
> +	memcpy(&phy->attrs, &attrs, sizeof(struct phy_attrs));
> +	mutex_unlock(&phy->mutex);
> +}

These two functions should be documented. I'm a but puzzled by the need
to protect the data with phy->mutex. Isn't phy->attrs static,
initialized at driver probe time, and then never changed ? If so I think
we can just access it directly, both in the PHY provider and consumer.

If the data can change at runtime, then the documentation of these
functions need to explain what can change, and when.

>  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 +405,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);

-- 
Regards,

Laurent Pinchart

[PATCH v4 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 v4 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Kishon Vijay Abraham I]

On 7/17/2020 12:20 PM, Swapnil Jakhade wrote:
> 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>

Acked-by: Kishon Vijay Abraham I <kishon@ti.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;
> 

Re: [PATCH v4 0/2] Add new PHY APIs to framework to get/set PHY attributes

[Sekhar Nori]

Hi Vinod,

On 7/17/20 12:20 PM, Swapnil Jakhade wrote:
> 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. v4-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. v4-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.

Is it possible to queue these for v5.9 also? I did notice that phy
updates for v5.9-rc1 are posted already. But these APIs are needed for
the DisplayPort driver thats getting ready for merge too. Having these
queued now will make managing dependencies much easier.

Thanks,
Sekhar

Re: [PATCH v4 0/2] Add new PHY APIs to framework to get/set PHY attributes

[Vinod Koul]

Hi Sekhar,

On 26-07-20, 01:24, Sekhar Nori wrote:
> Hi Vinod,
> 
> On 7/17/20 12:20 PM, Swapnil Jakhade wrote:
> > 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. v4-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. v4-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.
> 
> Is it possible to queue these for v5.9 also? I did notice that phy
> updates for v5.9-rc1 are posted already. But these APIs are needed for
> the DisplayPort driver thats getting ready for merge too. Having these
> queued now will make managing dependencies much easier.

I would prefer if we defer core change to post rc1. For your display, we
can provide a signed tag for you/drm folks to fetch.

Would that be okay?

Thanks
-- 
~Vinod

Re: [PATCH v4 0/2] Add new PHY APIs to framework to get/set PHY attributes

[Sekhar Nori]

On 7/27/20 3:25 PM, Vinod Koul wrote:
> Hi Sekhar,
> 
> On 26-07-20, 01:24, Sekhar Nori wrote:
>> Hi Vinod,
>>
>> On 7/17/20 12:20 PM, Swapnil Jakhade wrote:
>>> 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. v4-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. v4-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.
>>
>> Is it possible to queue these for v5.9 also? I did notice that phy
>> updates for v5.9-rc1 are posted already. But these APIs are needed for
>> the DisplayPort driver thats getting ready for merge too. Having these
>> queued now will make managing dependencies much easier.
> 
> I would prefer if we defer core change to post rc1. For your display, we
> can provide a signed tag for you/drm folks to fetch.
> 
> Would that be okay?

Okay, that would work too.

Thanks,
Sekhar