* [PATCH v3 0/2] Add new PHY APIs to framework to get/set PHY attributes
@ 2020-07-13 9:38 Swapnil Jakhade
2020-07-13 9:38 ` [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs " Swapnil Jakhade
2020-07-13 9:38 ` [PATCH v3 2/2] phy: cadence-torrent: Use kernel PHY API to set " Swapnil Jakhade
0 siblings, 2 replies; 6+ messages in thread
From: Swapnil Jakhade @ 2020-07-13 9:38 UTC (permalink / raw)
To: vkoul, kishon, linux-kernel, maxime
Cc: mparab, sjakhade, yamonkar, nsekhar, tomi.valkeinen, jsarha, praneeth
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs to get/set PHY attributes
2020-07-13 9:38 [PATCH v3 0/2] Add new PHY APIs to framework to get/set PHY attributes Swapnil Jakhade
@ 2020-07-13 9:38 ` Swapnil Jakhade
2020-07-13 11:11 ` Vinod Koul
2020-07-13 9:38 ` [PATCH v3 2/2] phy: cadence-torrent: Use kernel PHY API to set " Swapnil Jakhade
1 sibling, 1 reply; 6+ messages in thread
From: Swapnil Jakhade @ 2020-07-13 9:38 UTC (permalink / raw)
To: vkoul, kishon, linux-kernel, maxime
Cc: mparab, sjakhade, yamonkar, nsekhar, tomi.valkeinen, jsarha, praneeth
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes
2020-07-13 9:38 [PATCH v3 0/2] Add new PHY APIs to framework to get/set PHY attributes Swapnil Jakhade
2020-07-13 9:38 ` [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs " Swapnil Jakhade
@ 2020-07-13 9:38 ` Swapnil Jakhade
1 sibling, 0 replies; 6+ messages in thread
From: Swapnil Jakhade @ 2020-07-13 9:38 UTC (permalink / raw)
To: vkoul, kishon, linux-kernel, maxime
Cc: mparab, sjakhade, yamonkar, nsekhar, tomi.valkeinen, jsarha, praneeth
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs to get/set PHY attributes
2020-07-13 9:38 ` [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs " Swapnil Jakhade
@ 2020-07-13 11:11 ` Vinod Koul
2020-07-13 11:58 ` Kishon Vijay Abraham I
0 siblings, 1 reply; 6+ messages in thread
From: Vinod Koul @ 2020-07-13 11:11 UTC (permalink / raw)
To: Swapnil Jakhade
Cc: kishon, linux-kernel, maxime, mparab, yamonkar, nsekhar,
tomi.valkeinen, jsarha, praneeth
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs to get/set PHY attributes
2020-07-13 11:11 ` Vinod Koul
@ 2020-07-13 11:58 ` Kishon Vijay Abraham I
2020-07-14 7:44 ` Swapnil Kashinath Jakhade
0 siblings, 1 reply; 6+ messages in thread
From: Kishon Vijay Abraham I @ 2020-07-13 11:58 UTC (permalink / raw)
To: Vinod Koul, Swapnil Jakhade
Cc: linux-kernel, maxime, mparab, yamonkar, nsekhar, tomi.valkeinen,
jsarha, praneeth
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs to get/set PHY attributes
2020-07-13 11:58 ` Kishon Vijay Abraham I
@ 2020-07-14 7:44 ` Swapnil Kashinath Jakhade
0 siblings, 0 replies; 6+ messages in thread
From: Swapnil Kashinath Jakhade @ 2020-07-14 7:44 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Vinod Koul
Cc: linux-kernel, maxime, Milind Parab, Yuti Suresh Amonkar, nsekhar,
tomi.valkeinen, jsarha, praneeth
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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-14 7:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 9:38 [PATCH v3 0/2] Add new PHY APIs to framework to get/set PHY attributes Swapnil Jakhade
2020-07-13 9:38 ` [PATCH v3 1/2] phy: Add new PHY attribute max_link_rate and APIs " Swapnil Jakhade
2020-07-13 11:11 ` Vinod Koul
2020-07-13 11:58 ` Kishon Vijay Abraham I
2020-07-14 7:44 ` Swapnil Kashinath Jakhade
2020-07-13 9:38 ` [PATCH v3 2/2] phy: cadence-torrent: Use kernel PHY API to set " Swapnil Jakhade
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).