All of lore.kernel.org
 help / color / mirror / Atom feed
* Correct ordering of phy_init and phy_power_on
@ 2020-12-20 23:06 Ahmad Fatoum
  2020-12-21  3:15 ` Kishon Vijay Abraham I
  2021-01-12 17:01 ` [PATCH] usb: dwc2: Change " Jules Maselbas
  0 siblings, 2 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2020-12-20 23:06 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Vinod Koul, Minas Harutyunyan, linux-usb
  Cc: linux-kernel, Pengutronix Kernel Team, linux-usb, Jules Maselbas

Hello,

I just noticed that USB controller drivers differ in the order in which they
do phy_init and phy_power_on. For example:

__dwc2_lowlevel_hw_enable():

	ret = phy_power_on(hsotg->phy);    
	if (ret == 0)                      
        	ret = phy_init(hsotg->phy);

dwc3_core_init():

	ret = dwc3_core_soft_reset(dwc); // internally does phy_init(dwc->usb2_generic_phy);
	/* [snip] */
	ret = phy_power_on(dwc->usb2_generic_phy);


My initial assumption has been init -> power_on, but at least the phy-stm32-usbphyc
(used with dwc2) is written with the assumption that exit -> power_off (and therefore
power_on -> init). If they are swapped, disabling fails.

So how was it meant to be?

Cheers,
Ahmad

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Correct ordering of phy_init and phy_power_on
  2020-12-20 23:06 Correct ordering of phy_init and phy_power_on Ahmad Fatoum
@ 2020-12-21  3:15 ` Kishon Vijay Abraham I
  2020-12-22  9:08   ` Ahmad Fatoum
  2021-01-12 17:01 ` [PATCH] usb: dwc2: Change " Jules Maselbas
  1 sibling, 1 reply; 6+ messages in thread
From: Kishon Vijay Abraham I @ 2020-12-21  3:15 UTC (permalink / raw)
  To: Ahmad Fatoum, Vinod Koul, Minas Harutyunyan, linux-usb
  Cc: linux-kernel, Pengutronix Kernel Team, Jules Maselbas

Hi,

On 21/12/20 4:36 am, Ahmad Fatoum wrote:
> Hello,
> 
> I just noticed that USB controller drivers differ in the order in which they
> do phy_init and phy_power_on. For example:
> 
> __dwc2_lowlevel_hw_enable():
> 
> 	ret = phy_power_on(hsotg->phy);    
> 	if (ret == 0)                      
>         	ret = phy_init(hsotg->phy);
> 
> dwc3_core_init():
> 
> 	ret = dwc3_core_soft_reset(dwc); // internally does phy_init(dwc->usb2_generic_phy);
> 	/* [snip] */
> 	ret = phy_power_on(dwc->usb2_generic_phy);
> 
> 
> My initial assumption has been init -> power_on, but at least the phy-stm32-usbphyc
> (used with dwc2) is written with the assumption that exit -> power_off (and therefore
> power_on -> init). If they are swapped, disabling fails.
> 
> So how was it meant to be?

It is intended to be ->init() and then ->power_on(). So ideally it
should be the way dwc3 is.

Thanks,
Kishon

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Correct ordering of phy_init and phy_power_on
  2020-12-21  3:15 ` Kishon Vijay Abraham I
@ 2020-12-22  9:08   ` Ahmad Fatoum
  0 siblings, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2020-12-22  9:08 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Vinod Koul, Minas Harutyunyan, linux-usb
  Cc: linux-kernel, Pengutronix Kernel Team, Jules Maselbas

Hello Kishon,

On 21.12.20 04:15, Kishon Vijay Abraham I wrote:
>> So how was it meant to be?
> 
> It is intended to be ->init() and then ->power_on(). So ideally it
> should be the way dwc3 is.

Thanks. Should we do something about the inconsistency?

Amend documentation and maybe print a warning when order is wrong,
so users are encouraged to fix their drivers?

The way it is, you can't properly mix some of the PHY and USB controller drivers.

Cheers,
Ahmad

> 
> Thanks,
> Kishon
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] usb: dwc2: Change ordering of phy_init and phy_power_on
  2020-12-20 23:06 Correct ordering of phy_init and phy_power_on Ahmad Fatoum
  2020-12-21  3:15 ` Kishon Vijay Abraham I
@ 2021-01-12 17:01 ` Jules Maselbas
  2021-01-12 21:30   ` Ahmad Fatoum
  1 sibling, 1 reply; 6+ messages in thread
From: Jules Maselbas @ 2021-01-12 17:01 UTC (permalink / raw)
  To: linux-usb
  Cc: Greg Kroah-Hartman, Jules Maselbas, Ahmad Fatoum,
	Minas Harutyunyan, Kishon Vijay Abraham I

Call phy_init before phy_power_on as this is the intended way of using
the generic phy api.

Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: Minas Harutyunyan <hminas@synopsys.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>

---

I have quickly looked at usb-phy if this change could break something or
not. The following cmd list the compatible strings for usb-phy used by dwc2:

git grep 'snps,dwc2' -- arch/ | sed 's/:.*$//' | { while read file; do \
        phyname=$(git grep -A10 'snps,dwc2' -- "$file" | \
                sed -n '/phys/{s/.*<&\([^ >]*\).*/\1/p}'); \
        [ "$phyname" ] && { \
	        git grep -A10 "${phyname}: " -- "$file" | \
                grep -m1 'compatible'; \
        }; done };

From this output I took a look at:
 - brcm,kona-usb2-phy
 - samsung,exynos3250-usb2-phy
 - rockchip,rk3288-usb
 - amlogic,meson-gxbb-usb2-phy
 - amlogic,meson-gxl-usb2-phy
 - img,pistachio-usb-phy

Most of these phys only defines .power_on and .power_off;
brcm,kona-usb2-phy also defines .init; and amlogic,meson-gxl-usb2-phy defines
.init .exit and .reset

From what I've seen it seems to be OK for these two phy to call
init/exit first and then power_on/power_off.
---
 drivers/usb/dwc2/platform.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index b58ce996add7..a07dff088a26 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -142,9 +142,9 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
 	} else if (hsotg->plat && hsotg->plat->phy_init) {
 		ret = hsotg->plat->phy_init(pdev, hsotg->plat->phy_type);
 	} else {
-		ret = phy_power_on(hsotg->phy);
+		ret = phy_init(hsotg->phy);
 		if (ret == 0)
-			ret = phy_init(hsotg->phy);
+			ret = phy_power_on(hsotg->phy);
 	}
 
 	return ret;
@@ -176,9 +176,9 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
 	} else if (hsotg->plat && hsotg->plat->phy_exit) {
 		ret = hsotg->plat->phy_exit(pdev, hsotg->plat->phy_type);
 	} else {
-		ret = phy_exit(hsotg->phy);
+		ret = phy_power_off(hsotg->phy);
 		if (ret == 0)
-			ret = phy_power_off(hsotg->phy);
+			ret = phy_exit(hsotg->phy);
 	}
 	if (ret)
 		return ret;
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc2: Change ordering of phy_init and phy_power_on
  2021-01-12 17:01 ` [PATCH] usb: dwc2: Change " Jules Maselbas
@ 2021-01-12 21:30   ` Ahmad Fatoum
  2021-01-19 14:22     ` [Linux-stm32] " Amelie DELAUNAY
  0 siblings, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2021-01-12 21:30 UTC (permalink / raw)
  To: Jules Maselbas, linux-usb
  Cc: Greg Kroah-Hartman, Minas Harutyunyan, Kishon Vijay Abraham I,
	linux-stm32, Amelie Delaunay

Hello Jules,

+ linux-stm32 and Amelie, who upstreamed dwc2 glue for the stm32mp1.

[ some context: https://lore.kernel.org/lkml/6cd01e79-fdc0-3bd4-32b5-a85142533f8a@pengutronix.de/ ]

On 12.01.21 18:01, Jules Maselbas wrote:
> Call phy_init before phy_power_on as this is the intended way of using
> the generic phy api.

Even if the PHY driver code itself doesn't show an apparent dependency
between the power on and init operation, the hardware may expect things to
happen in a defined order. This is at least the case for the stm32-usbphyc
and would be violated if we just swap the order in the controller.

Instead, why not take it slow:

 - Document that phy_init -> phy_power_on is the correct order
 - Throw a warning when the order is violated
 - Ask for this patch to marinate a while in linux-next, so people
   have a chance to sort out incompatibilities with their PHY drivers

Cheers,
Ahmad

> 
> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Cc: Minas Harutyunyan <hminas@synopsys.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> 
> ---
> 
> I have quickly looked at usb-phy if this change could break something or
> not. The following cmd list the compatible strings for usb-phy used by dwc2:
> 
> git grep 'snps,dwc2' -- arch/ | sed 's/:.*$//' | { while read file; do \
>         phyname=$(git grep -A10 'snps,dwc2' -- "$file" | \
>                 sed -n '/phys/{s/.*<&\([^ >]*\).*/\1/p}'); \
>         [ "$phyname" ] && { \
> 	        git grep -A10 "${phyname}: " -- "$file" | \
>                 grep -m1 'compatible'; \
>         }; done };
> 
> From this output I took a look at:
>  - brcm,kona-usb2-phy
>  - samsung,exynos3250-usb2-phy
>  - rockchip,rk3288-usb
>  - amlogic,meson-gxbb-usb2-phy
>  - amlogic,meson-gxl-usb2-phy
>  - img,pistachio-usb-phy
> 
> Most of these phys only defines .power_on and .power_off;
> brcm,kona-usb2-phy also defines .init; and amlogic,meson-gxl-usb2-phy defines
> .init .exit and .reset
> 
> From what I've seen it seems to be OK for these two phy to call
> init/exit first and then power_on/power_off.
> ---
>  drivers/usb/dwc2/platform.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index b58ce996add7..a07dff088a26 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -142,9 +142,9 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
>  	} else if (hsotg->plat && hsotg->plat->phy_init) {
>  		ret = hsotg->plat->phy_init(pdev, hsotg->plat->phy_type);
>  	} else {
> -		ret = phy_power_on(hsotg->phy);
> +		ret = phy_init(hsotg->phy);
>  		if (ret == 0)
> -			ret = phy_init(hsotg->phy);
> +			ret = phy_power_on(hsotg->phy);
>  	}
>  
>  	return ret;
> @@ -176,9 +176,9 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>  	} else if (hsotg->plat && hsotg->plat->phy_exit) {
>  		ret = hsotg->plat->phy_exit(pdev, hsotg->plat->phy_type);
>  	} else {
> -		ret = phy_exit(hsotg->phy);
> +		ret = phy_power_off(hsotg->phy);
>  		if (ret == 0)
> -			ret = phy_power_off(hsotg->phy);
> +			ret = phy_exit(hsotg->phy);
>  	}
>  	if (ret)
>  		return ret;
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Linux-stm32] [PATCH] usb: dwc2: Change ordering of phy_init and phy_power_on
  2021-01-12 21:30   ` Ahmad Fatoum
@ 2021-01-19 14:22     ` Amelie DELAUNAY
  0 siblings, 0 replies; 6+ messages in thread
From: Amelie DELAUNAY @ 2021-01-19 14:22 UTC (permalink / raw)
  To: Ahmad Fatoum, Jules Maselbas, linux-usb
  Cc: Minas Harutyunyan, Greg Kroah-Hartman, linux-stm32,
	Kishon Vijay Abraham I

Hi Ahmad, Jules,

On 1/12/21 10:30 PM, Ahmad Fatoum wrote:
> Hello Jules,
> 
> + linux-stm32 and Amelie, who upstreamed dwc2 glue for the stm32mp1.
> 
> [ some context: https://lore.kernel.org/lkml/6cd01e79-fdc0-3bd4-32b5-a85142533f8a@pengutronix.de/ ]
> 
> On 12.01.21 18:01, Jules Maselbas wrote:
>> Call phy_init before phy_power_on as this is the intended way of using
>> the generic phy api.
> 
> Even if the PHY driver code itself doesn't show an apparent dependency
> between the power on and init operation, the hardware may expect things to
> happen in a defined order. This is at least the case for the stm32-usbphyc
> and would be violated if we just swap the order in the controller.
> 
> Instead, why not take it slow:
> 
>   - Document that phy_init -> phy_power_on is the correct order
>   - Throw a warning when the order is violated
>   - Ask for this patch to marinate a while in linux-next, so people
>     have a chance to sort out incompatibilities with their PHY drivers
> 

I agree with Ahmad, this should be documented somewhere.

Even if, with latest stm32-usbphyc updates 
(https://lore.kernel.org/patchwork/project/lkml/list/?series=478783), 
the order phy_init() then phy_power_on() would ensure a recommendation 
of STM32MP15 AN5031 [1].

Regards,
Amelie

[1] 
https://www.st.com/resource/en/application_note/dm00389996-getting-started-with-stm32mp151-stm32mp153-and-stm32mp157-line-hardware-development-stmicroelectronics.pdf

> Cheers,
> Ahmad
> 
>>
>> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
>> Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> Cc: Minas Harutyunyan <hminas@synopsys.com>
>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> ---
>>
>> I have quickly looked at usb-phy if this change could break something or
>> not. The following cmd list the compatible strings for usb-phy used by dwc2:
>>
>> git grep 'snps,dwc2' -- arch/ | sed 's/:.*$//' | { while read file; do \
>>          phyname=$(git grep -A10 'snps,dwc2' -- "$file" | \
>>                  sed -n '/phys/{s/.*<&\([^ >]*\).*/\1/p}'); \
>>          [ "$phyname" ] && { \
>> 	        git grep -A10 "${phyname}: " -- "$file" | \
>>                  grep -m1 'compatible'; \
>>          }; done };
>>
>>  From this output I took a look at:
>>   - brcm,kona-usb2-phy
>>   - samsung,exynos3250-usb2-phy
>>   - rockchip,rk3288-usb
>>   - amlogic,meson-gxbb-usb2-phy
>>   - amlogic,meson-gxl-usb2-phy
>>   - img,pistachio-usb-phy
>>
>> Most of these phys only defines .power_on and .power_off;
>> brcm,kona-usb2-phy also defines .init; and amlogic,meson-gxl-usb2-phy defines
>> .init .exit and .reset
>>
>>  From what I've seen it seems to be OK for these two phy to call
>> init/exit first and then power_on/power_off.
>> ---
>>   drivers/usb/dwc2/platform.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index b58ce996add7..a07dff088a26 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -142,9 +142,9 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
>>   	} else if (hsotg->plat && hsotg->plat->phy_init) {
>>   		ret = hsotg->plat->phy_init(pdev, hsotg->plat->phy_type);
>>   	} else {
>> -		ret = phy_power_on(hsotg->phy);
>> +		ret = phy_init(hsotg->phy);
>>   		if (ret == 0)
>> -			ret = phy_init(hsotg->phy);
>> +			ret = phy_power_on(hsotg->phy);
>>   	}
>>   
>>   	return ret;
>> @@ -176,9 +176,9 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>>   	} else if (hsotg->plat && hsotg->plat->phy_exit) {
>>   		ret = hsotg->plat->phy_exit(pdev, hsotg->plat->phy_type);
>>   	} else {
>> -		ret = phy_exit(hsotg->phy);
>> +		ret = phy_power_off(hsotg->phy);
>>   		if (ret == 0)
>> -			ret = phy_power_off(hsotg->phy);
>> +			ret = phy_exit(hsotg->phy);
>>   	}
>>   	if (ret)
>>   		return ret;
>>
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-19 18:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-20 23:06 Correct ordering of phy_init and phy_power_on Ahmad Fatoum
2020-12-21  3:15 ` Kishon Vijay Abraham I
2020-12-22  9:08   ` Ahmad Fatoum
2021-01-12 17:01 ` [PATCH] usb: dwc2: Change " Jules Maselbas
2021-01-12 21:30   ` Ahmad Fatoum
2021-01-19 14:22     ` [Linux-stm32] " Amelie DELAUNAY

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.