linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/2] Add support to get/set PHY attributes using
@ 2020-04-28  7:10 Yuti Amonkar
  2020-04-28  7:10 ` [PATCH v1 1/2] phy: Add max_link_rate as a PHY attribute along with APIs to get/set its value Yuti Amonkar
  2020-04-28  7:10 ` [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes Yuti Amonkar
  0 siblings, 2 replies; 10+ messages in thread
From: Yuti Amonkar @ 2020-04-28  7:10 UTC (permalink / raw)
  To: linux-kernel, kishon, mark.rutland, maxime
  Cc: jsarha, tomi.valkeinen, praneeth, mparab, sjakhade, yamonkar

This patch series adds support to use kernel PHY subsystem APIs
to get/set PHY attributes like number of lanes and maximum link rate.
It includes following patches:

1. v1-0001-phy-Add-max_link_rate-as-a-PHY-attribute-along-wi.patch
This patch adds max_link_rate as a PHY attribute along with a pair of APIs
that allow the generic PHY subsystem to provide information on the maximum
value of link rate supported by the PHY. The PHY provider driver may use
phy_set_max_link_rate() to set the value that PHY supports. The controller
driver may then use phy_get_max_link_rate() to fetch the PHY link rate in
order to properly configure the controller.

2. v1-0002-phy-phy-cadence-torrent-Use-PHY-kernel-APIs-to-se.patch
This patch uses kernel PHY APIs phy_set_bus_width() and phy_set_max_link_rate()
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 get API's will be used in the Cadence MHDP DRM bridge driver [1] which is under
review for upstreaming.

[1]

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

Swapnil Jakhade (1):
  phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes

Yuti Amonkar (1):
  phy: Add max_link_rate as a PHY attribute along with APIs to get/set
    its value

 drivers/phy/cadence/phy-cadence-torrent.c |  3 +++
 include/linux/phy/phy.h                   | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

-- 
2.26.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v1 1/2] phy: Add max_link_rate as a PHY attribute along with APIs to get/set its value
  2020-04-28  7:10 [PATCH v1 0/2] Add support to get/set PHY attributes using Yuti Amonkar
@ 2020-04-28  7:10 ` Yuti Amonkar
  2020-04-28  7:10 ` [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes Yuti Amonkar
  1 sibling, 0 replies; 10+ messages in thread
From: Yuti Amonkar @ 2020-04-28  7:10 UTC (permalink / raw)
  To: linux-kernel, kishon, mark.rutland, maxime
  Cc: jsarha, tomi.valkeinen, praneeth, mparab, sjakhade, yamonkar

The patch adds a pair of APIs that allow the generic PHY subsystem to
provide information on the maximum value of link rate supported by the
PHY. The PHY provider driver may use phy_set_max_link_rate() to set the
maximum link rate that the PHY supports. The controller driver may then
use phy_get_max_link_rate() to fetch the PHY link rate in order to
properly configure the controller.

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

diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index bcee8eba62b3..a8b7b4a2b8de 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -119,6 +119,7 @@ struct phy_ops {
  */
 struct phy_attrs {
 	u32			bus_width;
+	u32			max_link_rate;
 	enum phy_mode		mode;
 };
 
@@ -231,6 +232,16 @@ static inline void phy_set_bus_width(struct phy *phy, int bus_width)
 {
 	phy->attrs.bus_width = bus_width;
 }
+
+static inline int phy_get_max_link_rate(struct phy *phy)
+{
+	return phy->attrs.max_link_rate;
+}
+
+static inline void phy_set_max_link_rate(struct phy *phy, int link_rate)
+{
+	phy->attrs.max_link_rate = link_rate;
+}
 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 +400,16 @@ static inline void phy_set_bus_width(struct phy *phy, int bus_width)
 	return;
 }
 
+static inline int phy_get_max_link_rate(struct phy *phy)
+{
+	return -ENOSYS;
+}
+
+static inline void phy_set_max_link_rate(struct phy *phy, int link_rate)
+{
+	return;
+}
+
 static inline struct phy *phy_get(struct device *dev, const char *string)
 {
 	return ERR_PTR(-ENOSYS);
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
  2020-04-28  7:10 [PATCH v1 0/2] Add support to get/set PHY attributes using Yuti Amonkar
  2020-04-28  7:10 ` [PATCH v1 1/2] phy: Add max_link_rate as a PHY attribute along with APIs to get/set its value Yuti Amonkar
@ 2020-04-28  7:10 ` Yuti Amonkar
  2020-04-29 12:27   ` Maxime Ripard
  1 sibling, 1 reply; 10+ messages in thread
From: Yuti Amonkar @ 2020-04-28  7:10 UTC (permalink / raw)
  To: linux-kernel, kishon, mark.rutland, maxime
  Cc: jsarha, tomi.valkeinen, praneeth, mparab, sjakhade, yamonkar

From: Swapnil Jakhade <sjakhade@cadence.com>

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

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

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index 7116127358ee..b914e5ddf93c 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -1852,6 +1852,9 @@ 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);
+
+			phy_set_bus_width(gphy, cdns_phy->phys[node].num_lanes);
+			phy_set_max_link_rate(gphy, cdns_phy->max_bit_rate);
 		} else {
 			dev_err(dev, "Driver supports only PHY_TYPE_DP\n");
 			ret = -ENOTSUPP;
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
  2020-04-28  7:10 ` [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes Yuti Amonkar
@ 2020-04-29 12:27   ` Maxime Ripard
  2020-04-30 14:06     ` Swapnil Kashinath Jakhade
  0 siblings, 1 reply; 10+ messages in thread
From: Maxime Ripard @ 2020-04-29 12:27 UTC (permalink / raw)
  To: Yuti Amonkar
  Cc: linux-kernel, kishon, mark.rutland, jsarha, tomi.valkeinen,
	praneeth, mparab, sjakhade

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

Hi,

On Tue, Apr 28, 2020 at 09:10:04AM +0200, Yuti Amonkar wrote:
> From: Swapnil Jakhade <sjakhade@cadence.com>
> 
> Use generic PHY framework function phy_set_bus_width() to set number
> of lanes and function phy_set_max_link_rate() to set maximum link rate
> supported by PHY.
> 
> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>

This should have your SoB.

> ---
>  drivers/phy/cadence/phy-cadence-torrent.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
> index 7116127358ee..b914e5ddf93c 100644
> --- a/drivers/phy/cadence/phy-cadence-torrent.c
> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> @@ -1852,6 +1852,9 @@ 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);
> +
> +			phy_set_bus_width(gphy, cdns_phy->phys[node].num_lanes);
> +			phy_set_max_link_rate(gphy, cdns_phy->max_bit_rate);

I think what Kishon meant in his previous mail is that it's not really clear (to
me at least) how that function would be useful.

In this particular case, what would the consumer make of that information? Does
the phy needs to be reconfigured based on the max rate being changed?

Some phy_configure_opts structures also have a somewhat similar field that can
be negociated between the provider and the consumer using phy_validate, wouldn't
that be redundant?

Most of that discussion can only happen when you've provided a use-case for that
series, so a consumer using it would help greatly there.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
  2020-04-29 12:27   ` Maxime Ripard
@ 2020-04-30 14:06     ` Swapnil Kashinath Jakhade
  2020-05-07 17:17       ` Maxime Ripard
  0 siblings, 1 reply; 10+ messages in thread
From: Swapnil Kashinath Jakhade @ 2020-04-30 14:06 UTC (permalink / raw)
  To: Maxime Ripard, Yuti Suresh Amonkar
  Cc: linux-kernel, kishon, mark.rutland, jsarha, tomi.valkeinen,
	praneeth, Milind Parab

Hi Maxime,

Thank you so much for reviewing the patch. Please see inline reply below.

> -----Original Message-----
> From: Maxime Ripard <maxime@cerno.tech>
> Sent: Wednesday, April 29, 2020 5:58 PM
> To: Yuti Suresh Amonkar <yamonkar@cadence.com>
> Cc: linux-kernel@vger.kernel.org; kishon@ti.com; mark.rutland@arm.com;
> jsarha@ti.com; tomi.valkeinen@ti.com; praneeth@ti.com; Milind Parab
> <mparab@cadence.com>; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>
> Subject: Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to
> set PHY attributes
> 
> EXTERNAL MAIL
> 
> 
> Hi,
> 
> On Tue, Apr 28, 2020 at 09:10:04AM +0200, Yuti Amonkar wrote:
> > From: Swapnil Jakhade <sjakhade@cadence.com>
> >
> > Use generic PHY framework function phy_set_bus_width() to set number
> > of lanes and function phy_set_max_link_rate() to set maximum link rate
> > supported by PHY.
> >
> > Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> 
> This should have your SoB.
> 
> > ---
> >  drivers/phy/cadence/phy-cadence-torrent.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> b/drivers/phy/cadence/phy-cadence-torrent.c
> > index 7116127358ee..b914e5ddf93c 100644
> > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > @@ -1852,6 +1852,9 @@ 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);
> > +
> > +			phy_set_bus_width(gphy, cdns_phy-
> >phys[node].num_lanes);
> > +			phy_set_max_link_rate(gphy, cdns_phy-
> >max_bit_rate);
> 
> I think what Kishon meant in his previous mail is that it's not really clear (to
> me at least) how that function would be useful.
> 
> In this particular case, what would the consumer make of that information?
> Does
> the phy needs to be reconfigured based on the max rate being changed?
> 
> Some phy_configure_opts structures also have a somewhat similar field that
> can
> be negociated between the provider and the consumer using phy_validate,
> wouldn't
> that be redundant?
> 
> Most of that discussion can only happen when you've provided a use-case
> for that
> series, so a consumer using it would help greatly there.

Actually, for this particular case, consumer driver will be the Cadence MHDP bridge driver
for DisplayPort which is also under review process for upstreaming [1]. So this DRM bridge
driver will make use of the PHY APIs phy_get_bus_width() and phy_get_max_link_rate()
during execution of probe function to get the number of lanes and maximum link rate
supported by Cadence Torrent PHY. This information is required to set the host capabilities
in the DRM bridge driver, based on which initial values for DisplayPort link training will be
determined.
The changes in this PHY patch series are based on suggestions in the review comments in [1]
which asks to use kernel PHY APIs to read these properties instead of directly accessing PHY
device node. The complete driver and actual use of these APIs can be found in [2]. This is how
we are planning to use these APIs.

[1]
https://patchwork.freedesktop.org/patch/355362/?series=73996&rev=1

[2]
https://github.com/t-c-collab/linux/blob/cdns-mhdp-upstream/drivers/gpu/drm/bridge/cdns-mhdp-core.c#L1594

Thanks & regards,
Swapnil

> 
> Maxime

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
  2020-04-30 14:06     ` Swapnil Kashinath Jakhade
@ 2020-05-07 17:17       ` Maxime Ripard
  2020-05-08  7:50         ` Tomi Valkeinen
  0 siblings, 1 reply; 10+ messages in thread
From: Maxime Ripard @ 2020-05-07 17:17 UTC (permalink / raw)
  To: Swapnil Kashinath Jakhade
  Cc: Yuti Suresh Amonkar, linux-kernel, kishon, mark.rutland, jsarha,
	tomi.valkeinen, praneeth, Milind Parab

[-- Attachment #1: Type: text/plain, Size: 3859 bytes --]

Hi!

On Thu, Apr 30, 2020 at 02:06:20PM +0000, Swapnil Kashinath Jakhade wrote:
> Thank you so much for reviewing the patch. Please see inline reply below.
> 
> > -----Original Message-----
> > From: Maxime Ripard <maxime@cerno.tech>
> > Sent: Wednesday, April 29, 2020 5:58 PM
> > To: Yuti Suresh Amonkar <yamonkar@cadence.com>
> > Cc: linux-kernel@vger.kernel.org; kishon@ti.com; mark.rutland@arm.com;
> > jsarha@ti.com; tomi.valkeinen@ti.com; praneeth@ti.com; Milind Parab
> > <mparab@cadence.com>; Swapnil Kashinath Jakhade
> > <sjakhade@cadence.com>
> > Subject: Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to
> > set PHY attributes
> > 
> > EXTERNAL MAIL
> > 
> > 
> > Hi,
> > 
> > On Tue, Apr 28, 2020 at 09:10:04AM +0200, Yuti Amonkar wrote:
> > > From: Swapnil Jakhade <sjakhade@cadence.com>
> > >
> > > Use generic PHY framework function phy_set_bus_width() to set number
> > > of lanes and function phy_set_max_link_rate() to set maximum link rate
> > > supported by PHY.
> > >
> > > Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> > 
> > This should have your SoB.
> > 
> > > ---
> > >  drivers/phy/cadence/phy-cadence-torrent.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> > b/drivers/phy/cadence/phy-cadence-torrent.c
> > > index 7116127358ee..b914e5ddf93c 100644
> > > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > > @@ -1852,6 +1852,9 @@ 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);
> > > +
> > > +			phy_set_bus_width(gphy, cdns_phy-
> > >phys[node].num_lanes);
> > > +			phy_set_max_link_rate(gphy, cdns_phy-
> > >max_bit_rate);
> > 
> > I think what Kishon meant in his previous mail is that it's not really clear (to
> > me at least) how that function would be useful.
> > 
> > In this particular case, what would the consumer make of that information?
> > Does
> > the phy needs to be reconfigured based on the max rate being changed?
> > 
> > Some phy_configure_opts structures also have a somewhat similar field that
> > can
> > be negociated between the provider and the consumer using phy_validate,
> > wouldn't
> > that be redundant?
> > 
> > Most of that discussion can only happen when you've provided a use-case
> > for that
> > series, so a consumer using it would help greatly there.
> 
> Actually, for this particular case, consumer driver will be the Cadence MHDP
> bridge driver for DisplayPort which is also under review process for
> upstreaming [1]. So this DRM bridge driver will make use of the PHY APIs
> phy_get_bus_width() and phy_get_max_link_rate() during execution of probe
> function to get the number of lanes and maximum link rate supported by Cadence
> Torrent PHY. This information is required to set the host capabilities in the
> DRM bridge driver, based on which initial values for DisplayPort link training
> will be determined.
>
> The changes in this PHY patch series are based on suggestions in the review
> comments in [1] which asks to use kernel PHY APIs to read these properties
> instead of directly accessing PHY device node. The complete driver and actual
> use of these APIs can be found in [2]. This is how we are planning to use
> these APIs.

I haven't really looked into the displayport spec, but I'd assume that there's a
lot more parameters that would need to be negociated between the phy and the DP
block? If so, then it would make more sense to follow the path we did for
MIPI-DSI where the parameters can be negociated through the phy_configure /
phy_validate interface.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
  2020-05-07 17:17       ` Maxime Ripard
@ 2020-05-08  7:50         ` Tomi Valkeinen
  2020-05-13  2:37           ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 10+ messages in thread
From: Tomi Valkeinen @ 2020-05-08  7:50 UTC (permalink / raw)
  To: Maxime Ripard, Swapnil Kashinath Jakhade
  Cc: Yuti Suresh Amonkar, linux-kernel, kishon, mark.rutland, jsarha,
	praneeth, Milind Parab

On 07/05/2020 20:17, Maxime Ripard wrote:

>> Actually, for this particular case, consumer driver will be the Cadence MHDP
>> bridge driver for DisplayPort which is also under review process for
>> upstreaming [1]. So this DRM bridge driver will make use of the PHY APIs
>> phy_get_bus_width() and phy_get_max_link_rate() during execution of probe
>> function to get the number of lanes and maximum link rate supported by Cadence
>> Torrent PHY. This information is required to set the host capabilities in the
>> DRM bridge driver, based on which initial values for DisplayPort link training
>> will be determined.
>>
>> The changes in this PHY patch series are based on suggestions in the review
>> comments in [1] which asks to use kernel PHY APIs to read these properties
>> instead of directly accessing PHY device node. The complete driver and actual
>> use of these APIs can be found in [2]. This is how we are planning to use
>> these APIs.
> 
> I haven't really looked into the displayport spec, but I'd assume that there's a
> lot more parameters that would need to be negociated between the phy and the DP
> block? If so, then it would make more sense to follow the path we did for
> MIPI-DSI where the parameters can be negociated through the phy_configure /
> phy_validate interface.

I don't think this is negotiation, but just exposing the (max) capabilities of PHY, inside which the 
configure can work. Maybe all the capabilities could handled with a struct (struct phy_attrs), 
instead of adding separate functions for each, though.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
  2020-05-08  7:50         ` Tomi Valkeinen
@ 2020-05-13  2:37           ` Kishon Vijay Abraham I
  2020-05-18  6:54             ` Swapnil Kashinath Jakhade
  0 siblings, 1 reply; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2020-05-13  2:37 UTC (permalink / raw)
  To: Tomi Valkeinen, Maxime Ripard, Swapnil Kashinath Jakhade
  Cc: Yuti Suresh Amonkar, linux-kernel, mark.rutland, jsarha,
	praneeth, Milind Parab, Vinod Koul

Hi,

On 5/8/2020 1:20 PM, Tomi Valkeinen wrote:
> On 07/05/2020 20:17, Maxime Ripard wrote:
> 
>>> Actually, for this particular case, consumer driver will be the Cadence MHDP
>>> bridge driver for DisplayPort which is also under review process for
>>> upstreaming [1]. So this DRM bridge driver will make use of the PHY APIs
>>> phy_get_bus_width() and phy_get_max_link_rate() during execution of probe
>>> function to get the number of lanes and maximum link rate supported by Cadence
>>> Torrent PHY. This information is required to set the host capabilities in the
>>> DRM bridge driver, based on which initial values for DisplayPort link training
>>> will be determined.
>>>
>>> The changes in this PHY patch series are based on suggestions in the review
>>> comments in [1] which asks to use kernel PHY APIs to read these properties
>>> instead of directly accessing PHY device node. The complete driver and actual
>>> use of these APIs can be found in [2]. This is how we are planning to use
>>> these APIs.
>>
>> I haven't really looked into the displayport spec, but I'd assume that there's a
>> lot more parameters that would need to be negociated between the phy and the DP
>> block? If so, then it would make more sense to follow the path we did for
>> MIPI-DSI where the parameters can be negociated through the phy_configure /
>> phy_validate interface.
> 
> I don't think this is negotiation, but just exposing the (max) capabilities of
> PHY, inside which the configure can work. Maybe all the capabilities could
> handled with a struct (struct phy_attrs), instead of adding separate functions
> for each, though.

yeah, that makes sense. Just that users should take care not to over-write all
the phy attributes with partial information.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
  2020-05-13  2:37           ` Kishon Vijay Abraham I
@ 2020-05-18  6:54             ` Swapnil Kashinath Jakhade
  2020-05-18 12:11               ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 10+ messages in thread
From: Swapnil Kashinath Jakhade @ 2020-05-18  6:54 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Tomi Valkeinen, Maxime Ripard
  Cc: Yuti Suresh Amonkar, linux-kernel, mark.rutland, jsarha,
	praneeth, Milind Parab, Vinod Koul

Hi Kishon,

> -----Original Message-----
> From: Kishon Vijay Abraham I <kishon@ti.com>
> Sent: Wednesday, May 13, 2020 8:08 AM
> To: Tomi Valkeinen <tomi.valkeinen@ti.com>; Maxime Ripard
> <maxime@cerno.tech>; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>
> Cc: Yuti Suresh Amonkar <yamonkar@cadence.com>; linux-
> kernel@vger.kernel.org; mark.rutland@arm.com; jsarha@ti.com;
> praneeth@ti.com; Milind Parab <mparab@cadence.com>; Vinod Koul
> <vkoul@kernel.org>
> Subject: Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to
> set PHY attributes
> 
> EXTERNAL MAIL
> 
> 
> Hi,
> 
> On 5/8/2020 1:20 PM, Tomi Valkeinen wrote:
> > On 07/05/2020 20:17, Maxime Ripard wrote:
> >
> >>> Actually, for this particular case, consumer driver will be the
> >>> Cadence MHDP bridge driver for DisplayPort which is also under
> >>> review process for upstreaming [1]. So this DRM bridge driver will
> >>> make use of the PHY APIs
> >>> phy_get_bus_width() and phy_get_max_link_rate() during execution of
> >>> probe function to get the number of lanes and maximum link rate
> >>> supported by Cadence Torrent PHY. This information is required to
> >>> set the host capabilities in the DRM bridge driver, based on which
> >>> initial values for DisplayPort link training will be determined.
> >>>
> >>> The changes in this PHY patch series are based on suggestions in the
> >>> review comments in [1] which asks to use kernel PHY APIs to read
> >>> these properties instead of directly accessing PHY device node. The
> >>> complete driver and actual use of these APIs can be found in [2].
> >>> This is how we are planning to use these APIs.
> >>
> >> I haven't really looked into the displayport spec, but I'd assume
> >> that there's a lot more parameters that would need to be negociated
> >> between the phy and the DP block? If so, then it would make more
> >> sense to follow the path we did for MIPI-DSI where the parameters can
> >> be negociated through the phy_configure / phy_validate interface.
> >
> > I don't think this is negotiation, but just exposing the (max)
> > capabilities of PHY, inside which the configure can work. Maybe all
> > the capabilities could handled with a struct (struct phy_attrs),
> > instead of adding separate functions for each, though.
> 
> yeah, that makes sense. Just that users should take care not to over-write all
> the phy attributes with partial information.

It would be really helpful if you could clarify a bit regarding how to handle this
exactly. What I could understand from Tomi' suggestion is that all PHY attributes
in struct phy_attrs should have single pair of functions to get and set all the PHY
attributes (e.g. phy_get_attrs / phy_set_attrs), instead of separate get/set pair of
functions for individual attribute (bus_width, mode, max_link_rate etc). Is this
understanding correct? If so, how should the existing functions for bus_width and
mode be used?

Thanks & regards,
Swapnil

> 
> Thanks
> Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes
  2020-05-18  6:54             ` Swapnil Kashinath Jakhade
@ 2020-05-18 12:11               ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2020-05-18 12:11 UTC (permalink / raw)
  To: Swapnil Kashinath Jakhade, Tomi Valkeinen, Maxime Ripard
  Cc: Yuti Suresh Amonkar, linux-kernel, mark.rutland, jsarha,
	praneeth, Milind Parab, Vinod Koul

Hi,

On 5/18/2020 12:24 PM, Swapnil Kashinath Jakhade wrote:
> Hi Kishon,
> 
>> -----Original Message-----
>> From: Kishon Vijay Abraham I <kishon@ti.com>
>> Sent: Wednesday, May 13, 2020 8:08 AM
>> To: Tomi Valkeinen <tomi.valkeinen@ti.com>; Maxime Ripard
>> <maxime@cerno.tech>; Swapnil Kashinath Jakhade
>> <sjakhade@cadence.com>
>> Cc: Yuti Suresh Amonkar <yamonkar@cadence.com>; linux-
>> kernel@vger.kernel.org; mark.rutland@arm.com; jsarha@ti.com;
>> praneeth@ti.com; Milind Parab <mparab@cadence.com>; Vinod Koul
>> <vkoul@kernel.org>
>> Subject: Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to
>> set PHY attributes
>>
>> EXTERNAL MAIL
>>
>>
>> Hi,
>>
>> On 5/8/2020 1:20 PM, Tomi Valkeinen wrote:
>>> On 07/05/2020 20:17, Maxime Ripard wrote:
>>>
>>>>> Actually, for this particular case, consumer driver will be the
>>>>> Cadence MHDP bridge driver for DisplayPort which is also under
>>>>> review process for upstreaming [1]. So this DRM bridge driver will
>>>>> make use of the PHY APIs
>>>>> phy_get_bus_width() and phy_get_max_link_rate() during execution of
>>>>> probe function to get the number of lanes and maximum link rate
>>>>> supported by Cadence Torrent PHY. This information is required to
>>>>> set the host capabilities in the DRM bridge driver, based on which
>>>>> initial values for DisplayPort link training will be determined.
>>>>>
>>>>> The changes in this PHY patch series are based on suggestions in the
>>>>> review comments in [1] which asks to use kernel PHY APIs to read
>>>>> these properties instead of directly accessing PHY device node. The
>>>>> complete driver and actual use of these APIs can be found in [2].
>>>>> This is how we are planning to use these APIs.
>>>>
>>>> I haven't really looked into the displayport spec, but I'd assume
>>>> that there's a lot more parameters that would need to be negociated
>>>> between the phy and the DP block? If so, then it would make more
>>>> sense to follow the path we did for MIPI-DSI where the parameters can
>>>> be negociated through the phy_configure / phy_validate interface.
>>>
>>> I don't think this is negotiation, but just exposing the (max)
>>> capabilities of PHY, inside which the configure can work. Maybe all
>>> the capabilities could handled with a struct (struct phy_attrs),
>>> instead of adding separate functions for each, though.
>>
>> yeah, that makes sense. Just that users should take care not to over-write all
>> the phy attributes with partial information.
> 
> It would be really helpful if you could clarify a bit regarding how to handle this
> exactly. What I could understand from Tomi' suggestion is that all PHY attributes
> in struct phy_attrs should have single pair of functions to get and set all the PHY
> attributes (e.g. phy_get_attrs / phy_set_attrs), instead of separate get/set pair of
> functions for individual attribute (bus_width, mode, max_link_rate etc). Is this
> understanding correct? If so, how should the existing functions for bus_width and
> mode be used?

Yes, your understanding is correct. There are already existing users of
bus_width, mode, so let's not disturb that. That could maybe deprecated later.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-05-18 12:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28  7:10 [PATCH v1 0/2] Add support to get/set PHY attributes using Yuti Amonkar
2020-04-28  7:10 ` [PATCH v1 1/2] phy: Add max_link_rate as a PHY attribute along with APIs to get/set its value Yuti Amonkar
2020-04-28  7:10 ` [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes Yuti Amonkar
2020-04-29 12:27   ` Maxime Ripard
2020-04-30 14:06     ` Swapnil Kashinath Jakhade
2020-05-07 17:17       ` Maxime Ripard
2020-05-08  7:50         ` Tomi Valkeinen
2020-05-13  2:37           ` Kishon Vijay Abraham I
2020-05-18  6:54             ` Swapnil Kashinath Jakhade
2020-05-18 12:11               ` Kishon Vijay Abraham I

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).