[PATCH v2 0/3] Cleanup the call ordering of phy_init and phy_power_on

[PATCH v2 0/3] Cleanup the call ordering of phy_init and phy_power_on

[Jules Maselbas]

Hi,

Last year Ahmad asked what is the correct order when calling phy_init
and phy_power_on. Since then, I didn't see the situation improve much
and I am once again toying around with usb phy driver.

The following two patches were in my tree for a year... Last year i
previously tried to change the call order in the dwc2 driver but this
requires the relevent phy to be also compatible with the "new" ordering.
The stm32-usbphyc driver wasn't compatible, I am not sure if that is
still is the case.

For now simply add documentation, hopefully correct, but I am not an
expert on actual phy sementics or usage in the kernel. And add warning
when the order is not what's expected.

Best,

---
changes in v2:
  - Update the documentation syntax for returned value

Jules Maselbas (3):
  phy: core: Add documentation of phy operation order
  phy: core: Update documentation syntax
  phy: core: Warn when phy_power_on is called before phy_init

 drivers/phy/phy-core.c | 44 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] phy: core: Add documentation of phy operation order

[Jules Maselbas]

Add documentation on phy function usage: init function must be
called before power_on; power_off must be called before exit.

Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
Cc: Minas Harutyunyan <hminas@synopsys.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
---
change in v2:
  - Update the documentation syntax for returned value

 drivers/phy/phy-core.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 91e28d6ce450..2f32f999445f 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -229,6 +229,17 @@ void phy_pm_runtime_forbid(struct phy *phy)
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);
 
+/**
+ * phy_init - phy internal initialization before phy operation
+ * @phy: the phy returned by phy_get()
+ *
+ * Used to allow phy's driver to perform phy internal initialization,
+ * such as PLL block powering, clock initialization or anything that's
+ * is required by the phy to perform the start of operation.
+ * Must be called before phy_power_on().
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
 int phy_init(struct phy *phy)
 {
 	int ret;
@@ -258,6 +269,14 @@ int phy_init(struct phy *phy)
 }
 EXPORT_SYMBOL_GPL(phy_init);
 
+/**
+ * phy_exit - Phy internal un-initialization
+ * @phy: the phy returned by phy_get()
+ *
+ * Must be called after phy_power_off().
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
 int phy_exit(struct phy *phy)
 {
 	int ret;
@@ -287,6 +306,14 @@ int phy_exit(struct phy *phy)
 }
 EXPORT_SYMBOL_GPL(phy_exit);
 
+/**
+ * phy_power_on - Enable the phy and enter proper operation
+ * @phy: the phy returned by phy_get()
+ *
+ * Must be called after phy_init().
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
 int phy_power_on(struct phy *phy)
 {
 	int ret = 0;
@@ -329,6 +356,14 @@ int phy_power_on(struct phy *phy)
 }
 EXPORT_SYMBOL_GPL(phy_power_on);
 
+/**
+ * phy_power_off - Disable the phy.
+ * @phy: the phy returned by phy_get()
+ *
+ * Must be called before phy_exit().
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
 int phy_power_off(struct phy *phy)
 {
 	int ret;
-- 
2.17.1

[PATCH v2 2/3] phy: core: Update documentation syntax

[Jules Maselbas]

Update the syntax used by the documentation of phy operation functions.
This is to unify the syntax with the newly added documentation.

Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
---
change in v2:
  - New patch

 drivers/phy/phy-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 2f32f999445f..cbdad65d2caa 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -467,7 +467,7 @@ EXPORT_SYMBOL_GPL(phy_reset);
  * runtime, which are otherwise lost after host controller reset and cannot
  * be applied in phy_init() or phy_power_on().
  *
- * Returns: 0 if successful, an negative error code otherwise
+ * Return: %0 if successful, a negative error code otherwise
  */
 int phy_calibrate(struct phy *phy)
 {
@@ -493,7 +493,7 @@ EXPORT_SYMBOL_GPL(phy_calibrate);
  * on the phy. The configuration will be applied on the current phy
  * mode, that can be changed using phy_set_mode().
  *
- * Returns: 0 if successful, an negative error code otherwise
+ * Return: %0 if successful, a negative error code otherwise
  */
 int phy_configure(struct phy *phy, union phy_configure_opts *opts)
 {
@@ -527,7 +527,7 @@ EXPORT_SYMBOL_GPL(phy_configure);
  * PHY, so calling it as many times as deemed fit will have no side
  * effect.
  *
- * Returns: 0 if successful, an negative error code otherwise
+ * Return: %0 if successful, a negative error code otherwise
  */
 int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
 		 union phy_configure_opts *opts)
-- 
2.17.1

[PATCH v2 3/3] phy: core: Warn when phy_power_on is called before phy_init

[Jules Maselbas]

A warning when the order of phy operation is mixed up by drivers,
this is an atempt to make the phy usage more uniform across (usb)
drivers.

Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
Cc: Minas Harutyunyan <hminas@synopsys.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
---
change in v2:
  - no changes

 drivers/phy/phy-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index cbdad65d2caa..0cb4da62577e 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -252,6 +252,9 @@ int phy_init(struct phy *phy)
 		return ret;
 	ret = 0; /* Override possible ret == -ENOTSUPP */
 
+	if (phy->power_count > phy->init_count)
+		dev_warn(&phy->dev, "phy_power_on was called before phy_init\n");
+
 	mutex_lock(&phy->mutex);
 	if (phy->init_count == 0 && phy->ops->init) {
 		ret = phy->ops->init(phy);
-- 
2.17.1

Re: [PATCH v2 3/3] phy: core: Warn when phy_power_on is called before phy_init

[Ahmad Fatoum]

Hello Jules,

On 23.03.22 12:07, Jules Maselbas wrote:
> A warning when the order of phy operation is mixed up by drivers,
> this is an atempt to make the phy usage more uniform across (usb)
> drivers.

Thanks for picking up this suggestion.

> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
> Cc: Minas Harutyunyan <hminas@synopsys.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> change in v2:
>   - no changes
> 
>  drivers/phy/phy-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index cbdad65d2caa..0cb4da62577e 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -252,6 +252,9 @@ int phy_init(struct phy *phy)
>  		return ret;
>  	ret = 0; /* Override possible ret == -ENOTSUPP */
>  
> +	if (phy->power_count > phy->init_count)

This needs to be moved into the critical section below.

> +		dev_warn(&phy->dev, "phy_power_on was called before phy_init\n");

I am wondering how often would this be triggered for e.g. a PHY that's being
runtime suspended. But the warning being obnoxious is the point of the patch,
so perhaps it's ok to not make it into a dev_warn_once. 

> +
>  	mutex_lock(&phy->mutex);
>  	if (phy->init_count == 0 && phy->ops->init) {
>  		ret = phy->ops->init(phy);


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 3/3] phy: core: Warn when phy_power_on is called before phy_init

[Jules Maselbas]

Hi Ahmad,

On Wed, Mar 23, 2022 at 12:13:42PM +0100, Ahmad Fatoum wrote:
> Hello Jules,
> 
> On 23.03.22 12:07, Jules Maselbas wrote:
> > A warning when the order of phy operation is mixed up by drivers,
> > this is an atempt to make the phy usage more uniform across (usb)
> > drivers.
> 
> Thanks for picking up this suggestion.
> 
> > Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> > Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
> > Cc: Minas Harutyunyan <hminas@synopsys.com>
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> > change in v2:
> >   - no changes
> > 
> >  drivers/phy/phy-core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > index cbdad65d2caa..0cb4da62577e 100644
> > --- a/drivers/phy/phy-core.c
> > +++ b/drivers/phy/phy-core.c
> > @@ -252,6 +252,9 @@ int phy_init(struct phy *phy)
> >  		return ret;
> >  	ret = 0; /* Override possible ret == -ENOTSUPP */
> >  
> > +	if (phy->power_count > phy->init_count)
> 
> This needs to be moved into the critical section below.
yes, you're right, I'll send a v3 later, giving some time for other
people to comment.

> 
> > +		dev_warn(&phy->dev, "phy_power_on was called before phy_init\n");
> 
> I am wondering how often would this be triggered for e.g. a PHY that's being
> runtime suspended. But the warning being obnoxious is the point of the patch,
> so perhaps it's ok to not make it into a dev_warn_once. 
I don't really know how often this will be printed, this is an open point.

> 
> > +
> >  	mutex_lock(&phy->mutex);
> >  	if (phy->init_count == 0 && phy->ops->init) {
> >  		ret = phy->ops->init(phy);
> 
>