* 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@vger.kernel.org, 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@vger.kernel.org, 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@vger.kernel.org, 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@st-md-mailman.stormreply.com, 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@st-md-mailman.stormreply.com, 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox