| Message ID | 20190111133133.24803-4-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | A3700 USB S2RAM support | expand |
Hello! On 01/11/2019 04:31 PM, Miquel Raynal wrote: > No need to initialize the PHY from the driver's probe. It is done by > the core automatically and doing it twice would increment the > phy->powercount counter to 2 instead of 1. During later suspend > operation, the counter will be decremented to one, no phy->power_off() > will occur and worst than that, the following phy->power_on() at Worse. > resume time will be also skipped, failing the whole S2RAM operation. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/usb/host/ehci-orion.c | 26 +++----------------------- > 1 file changed, 3 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c > index 1ad72647a069..3109f082949e 100644 > --- a/drivers/usb/host/ehci-orion.c > +++ b/drivers/usb/host/ehci-orion.c > @@ -257,15 +257,7 @@ static int ehci_orion_drv_probe(struct platform_device *pdev) > if (IS_ERR(priv->phy)) { > err = PTR_ERR(priv->phy); > if (err != -ENOSYS) > - goto err_phy_get; > - } else { > - err = phy_init(priv->phy); > - if (err) > - goto err_phy_init; > - > - err = phy_power_on(priv->phy); > - if (err) > - goto err_phy_power_on; > + goto err_dis_clk; Familiar code in unfamiliar place. Somebody must have blindly copied it... :-) [...] MBR, Sergei
Hi Miquel, On ven., janv. 11 2019, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > No need to initialize the PHY from the driver's probe. It is done by > the core automatically and doing it twice would increment the Do you know exactly which core take care of it? When the phy support was added to this driver there was not such feature. I made some research and found that recently (less than one year ago) a series was added to initialize PHYs at HCD level[1]. I think that our platform was forgotten in the conversion. Could you check that we are now aligned with the requirement of this series? Thanks, Gregory [1]:https://www.spinics.net/lists/linux-usb/msg166281.html > phy->powercount counter to 2 instead of 1. During later suspend > operation, the counter will be decremented to one, no phy->power_off() > will occur and worst than that, the following phy->power_on() at > resume time will be also skipped, failing the whole S2RAM operation. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/usb/host/ehci-orion.c | 26 +++----------------------- > 1 file changed, 3 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c > index 1ad72647a069..3109f082949e 100644 > --- a/drivers/usb/host/ehci-orion.c > +++ b/drivers/usb/host/ehci-orion.c > @@ -257,15 +257,7 @@ static int ehci_orion_drv_probe(struct platform_device *pdev) > if (IS_ERR(priv->phy)) { > err = PTR_ERR(priv->phy); > if (err != -ENOSYS) > - goto err_phy_get; > - } else { > - err = phy_init(priv->phy); > - if (err) > - goto err_phy_init; > - > - err = phy_power_on(priv->phy); > - if (err) > - goto err_phy_power_on; > + goto err_dis_clk; > } > > /* > @@ -297,19 +289,12 @@ static int ehci_orion_drv_probe(struct platform_device *pdev) > > err = usb_add_hcd(hcd, irq, IRQF_SHARED); > if (err) > - goto err_add_hcd; > + goto err_dis_clk; > > device_wakeup_enable(hcd->self.controller); > return 0; > > -err_add_hcd: > - if (!IS_ERR(priv->phy)) > - phy_power_off(priv->phy); > -err_phy_power_on: > - if (!IS_ERR(priv->phy)) > - phy_exit(priv->phy); > -err_phy_init: > -err_phy_get: > +err_dis_clk: > if (!IS_ERR(priv->clk)) > clk_disable_unprepare(priv->clk); > usb_put_hcd(hcd); > @@ -327,11 +312,6 @@ static int ehci_orion_drv_remove(struct platform_device *pdev) > > usb_remove_hcd(hcd); > > - if (!IS_ERR(priv->phy)) { > - phy_power_off(priv->phy); > - phy_exit(priv->phy); > - } > - > if (!IS_ERR(priv->clk)) > clk_disable_unprepare(priv->clk); > > -- > 2.19.1 >
Hi Gregory, Gregory CLEMENT <gregory.clement@bootlin.com> wrote on Fri, 18 Jan 2019 17:25:30 +0100: > Hi Miquel, > > On ven., janv. 11 2019, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > No need to initialize the PHY from the driver's probe. It is done by > > the core automatically and doing it twice would increment the > > Do you know exactly which core take care of it? The Orion's probe function calls usb_add_hcd() which handles PHY initialization (init, set mode, power on). > > When the phy support was added to this driver there was not such > feature. I made some research and found that recently (less than one > year ago) a series was added to initialize PHYs at HCD level[1]. I think > that our platform was forgotten in the conversion. AFAICS the conversion was applied to ehci-platform.c but not to other drivers so yes, this one was left aside but there are maybe others in the same situation. > > Could you check that we are now aligned with the requirement of this > series? There has been quite a few evolutions since this series but for what I understand, yes we are. Thanks, Miquèl
Hi Sergei, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote on Fri, 11 Jan 2019 21:03:01 +0300: > Hello! > > On 01/11/2019 04:31 PM, Miquel Raynal wrote: > > > No need to initialize the PHY from the driver's probe. It is done by > > the core automatically and doing it twice would increment the > > phy->powercount counter to 2 instead of 1. During later suspend > > operation, the counter will be decremented to one, no phy->power_off() > > will occur and worst than that, the following phy->power_on() at > > Worse. Noted > > > resume time will be also skipped, failing the whole S2RAM operation. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/usb/host/ehci-orion.c | 26 +++----------------------- > > 1 file changed, 3 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c > > index 1ad72647a069..3109f082949e 100644 > > --- a/drivers/usb/host/ehci-orion.c > > +++ b/drivers/usb/host/ehci-orion.c > > @@ -257,15 +257,7 @@ static int ehci_orion_drv_probe(struct platform_device *pdev) > > if (IS_ERR(priv->phy)) { > > err = PTR_ERR(priv->phy); > > if (err != -ENOSYS) > > - goto err_phy_get; > > - } else { > > - err = phy_init(priv->phy); > > - if (err) > > - goto err_phy_init; > > - > > - err = phy_power_on(priv->phy); > > - if (err) > > - goto err_phy_power_on; > > + goto err_dis_clk; > > Familiar code in unfamiliar place. Somebody must have blindly copied it... :-) Actually a git blame shows that this code is there since 2014, 4 years before the HCD core supported PHYs. This driver was probably forgotten during the process. Thanks, Miquèl
diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c index 1ad72647a069..3109f082949e 100644 --- a/drivers/usb/host/ehci-orion.c +++ b/drivers/usb/host/ehci-orion.c @@ -257,15 +257,7 @@ static int ehci_orion_drv_probe(struct platform_device *pdev) if (IS_ERR(priv->phy)) { err = PTR_ERR(priv->phy); if (err != -ENOSYS) - goto err_phy_get; - } else { - err = phy_init(priv->phy); - if (err) - goto err_phy_init; - - err = phy_power_on(priv->phy); - if (err) - goto err_phy_power_on; + goto err_dis_clk; } /* @@ -297,19 +289,12 @@ static int ehci_orion_drv_probe(struct platform_device *pdev) err = usb_add_hcd(hcd, irq, IRQF_SHARED); if (err) - goto err_add_hcd; + goto err_dis_clk; device_wakeup_enable(hcd->self.controller); return 0; -err_add_hcd: - if (!IS_ERR(priv->phy)) - phy_power_off(priv->phy); -err_phy_power_on: - if (!IS_ERR(priv->phy)) - phy_exit(priv->phy); -err_phy_init: -err_phy_get: +err_dis_clk: if (!IS_ERR(priv->clk)) clk_disable_unprepare(priv->clk); usb_put_hcd(hcd); @@ -327,11 +312,6 @@ static int ehci_orion_drv_remove(struct platform_device *pdev) usb_remove_hcd(hcd); - if (!IS_ERR(priv->phy)) { - phy_power_off(priv->phy); - phy_exit(priv->phy); - } - if (!IS_ERR(priv->clk)) clk_disable_unprepare(priv->clk);
No need to initialize the PHY from the driver's probe. It is done by the core automatically and doing it twice would increment the phy->powercount counter to 2 instead of 1. During later suspend operation, the counter will be decremented to one, no phy->power_off() will occur and worst than that, the following phy->power_on() at resume time will be also skipped, failing the whole S2RAM operation. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/usb/host/ehci-orion.c | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-)