Re: [Linux-stm32] [PATCH] usb: dwc2: Change ordering of phy_init and phy_power_on

[Amelie DELAUNAY <amelie.delaunay@foss.st.com>]

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