2020-12-20 23:25:40

by Ahmad Fatoum

Subject: Correct ordering of phy_init and phy_power_on

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 |

2020-12-21 03:17:36

by Kishon Vijay Abraham I

Subject: Re: Correct ordering of phy_init and phy_power_on

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

2020-12-22 09:09:56

by Ahmad Fatoum

Subject: Re: Correct ordering of phy_init and phy_power_on

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 |