| Message ID | 20210112170124.14148-1-jmaselbas@kalray.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc2: Change ordering of phy_init and phy_power_on | expand |
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; >
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; >> >
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;
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(-)