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

[Amelie DELAUNAY <amelie.delaunay@xxxxxxxxxxx>]

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@xxxxxxxxxxxxxx/ ]
>
> 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@xxxxxxxxx>
> > Cc: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> > Cc: Minas Harutyunyan <hminas@xxxxxxxxxxxx>
> > Cc: Kishon Vijay Abraham I <kishon@xxxxxx>
> >
> > ---
> >
> > 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;