| Message ID | 20240301193831.3346-2-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/4] usb: ehci-exynos: Use devm_clk_get_enabled() helpers | expand |
On Sat, Mar 02, 2024 at 01:08:08AM +0530, Anand Moon wrote: > The devm_clk_get_enabled() helpers: > - call devm_clk_get() > - call clk_prepare_enable() and register what is needed in order to > call clk_disable_unprepare() when needed, as a managed resource. > > This simplifies the code and avoids the calls to clk_disable_unprepare(). > > While at it, use dev_err_probe consistently, and use its return value > to return the error code. > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > drivers/usb/host/ehci-exynos.c | 30 +++++------------------------- > 1 file changed, 5 insertions(+), 25 deletions(-) > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > index f644b131cc0b..05aa3d9c2a3b 100644 > --- a/drivers/usb/host/ehci-exynos.c > +++ b/drivers/usb/host/ehci-exynos.c > @@ -159,19 +159,12 @@ static int exynos_ehci_probe(struct platform_device *pdev) > > err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); > if (err) > - goto fail_clk; > - > - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); > - > - if (IS_ERR(exynos_ehci->clk)) { > - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); > - err = PTR_ERR(exynos_ehci->clk); > - goto fail_clk; > - } > + goto fail_io; > > - err = clk_prepare_enable(exynos_ehci->clk); > - if (err) > - goto fail_clk; > + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); > + if (IS_ERR(exynos_ehci->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), > + "Failed to get usbhost clock\n"); What about the usb_put_hcd(hcd) call that used to happen here? Alan Stern > > hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(hcd->regs)) { > @@ -223,8 +216,6 @@ static int exynos_ehci_probe(struct platform_device *pdev) > exynos_ehci_phy_disable(&pdev->dev); > pdev->dev.of_node = exynos_ehci->of_node; > fail_io: > - clk_disable_unprepare(exynos_ehci->clk); > -fail_clk: > usb_put_hcd(hcd); > return err; > } > @@ -240,8 +231,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) > > exynos_ehci_phy_disable(&pdev->dev); > > - clk_disable_unprepare(exynos_ehci->clk); > - > usb_put_hcd(hcd); > } > > @@ -249,7 +238,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) > static int exynos_ehci_suspend(struct device *dev) > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); > > bool do_wakeup = device_may_wakeup(dev); > int rc; > @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev) > > exynos_ehci_phy_disable(dev); > > - clk_disable_unprepare(exynos_ehci->clk); > - > return rc; > } > > static int exynos_ehci_resume(struct device *dev) > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); > int ret; > > - ret = clk_prepare_enable(exynos_ehci->clk); > - if (ret) > - return ret; > - > ret = exynos_ehci_phy_enable(dev); > if (ret) { > dev_err(dev, "Failed to enable USB phy\n"); > - clk_disable_unprepare(exynos_ehci->clk); > return ret; > } > > -- > 2.43.0 >
Hi Alan On Sat, 2 Mar 2024 at 01:49, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Sat, Mar 02, 2024 at 01:08:08AM +0530, Anand Moon wrote: > > The devm_clk_get_enabled() helpers: > > - call devm_clk_get() > > - call clk_prepare_enable() and register what is needed in order to > > call clk_disable_unprepare() when needed, as a managed resource. > > > > This simplifies the code and avoids the calls to clk_disable_unprepare(). > > > > While at it, use dev_err_probe consistently, and use its return value > > to return the error code. > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > drivers/usb/host/ehci-exynos.c | 30 +++++------------------------- > > 1 file changed, 5 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > > index f644b131cc0b..05aa3d9c2a3b 100644 > > --- a/drivers/usb/host/ehci-exynos.c > > +++ b/drivers/usb/host/ehci-exynos.c > > @@ -159,19 +159,12 @@ static int exynos_ehci_probe(struct platform_device *pdev) > > > > err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); > > if (err) > > - goto fail_clk; > > - > > - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); > > - > > - if (IS_ERR(exynos_ehci->clk)) { > > - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); > > - err = PTR_ERR(exynos_ehci->clk); > > - goto fail_clk; > > - } > > + goto fail_io; > > > > - err = clk_prepare_enable(exynos_ehci->clk); > > - if (err) > > - goto fail_clk; > > + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); > > + if (IS_ERR(exynos_ehci->clk)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), > > + "Failed to get usbhost clock\n"); > > What about the usb_put_hcd(hcd) call that used to happen here? > Ok, I will update this in the next version. > Alan Stern Thanks -Anand
Le 01/03/2024 à 20:38, Anand Moon a écrit : > The devm_clk_get_enabled() helpers: > - call devm_clk_get() > - call clk_prepare_enable() and register what is needed in order to > call clk_disable_unprepare() when needed, as a managed resource. > > This simplifies the code and avoids the calls to clk_disable_unprepare(). > > While at it, use dev_err_probe consistently, and use its return value > to return the error code. > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > drivers/usb/host/ehci-exynos.c | 30 +++++------------------------- > 1 file changed, 5 insertions(+), 25 deletions(-) > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > index f644b131cc0b..05aa3d9c2a3b 100644 > --- a/drivers/usb/host/ehci-exynos.c > +++ b/drivers/usb/host/ehci-exynos.c > @@ -159,19 +159,12 @@ static int exynos_ehci_probe(struct platform_device *pdev) > > err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); > if (err) > - goto fail_clk; > - > - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); > - > - if (IS_ERR(exynos_ehci->clk)) { > - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); > - err = PTR_ERR(exynos_ehci->clk); > - goto fail_clk; > - } > + goto fail_io; > > - err = clk_prepare_enable(exynos_ehci->clk); > - if (err) > - goto fail_clk; > + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); > + if (IS_ERR(exynos_ehci->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), > + "Failed to get usbhost clock\n"); > > hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(hcd->regs)) { > @@ -223,8 +216,6 @@ static int exynos_ehci_probe(struct platform_device *pdev) > exynos_ehci_phy_disable(&pdev->dev); > pdev->dev.of_node = exynos_ehci->of_node; > fail_io: > - clk_disable_unprepare(exynos_ehci->clk); > -fail_clk: > usb_put_hcd(hcd); > return err; > } > @@ -240,8 +231,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) > > exynos_ehci_phy_disable(&pdev->dev); > > - clk_disable_unprepare(exynos_ehci->clk); > - > usb_put_hcd(hcd); > } > > @@ -249,7 +238,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) > static int exynos_ehci_suspend(struct device *dev) > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); > > bool do_wakeup = device_may_wakeup(dev); > int rc; > @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev) > > exynos_ehci_phy_disable(dev); > > - clk_disable_unprepare(exynos_ehci->clk); Hi, I don't think that removing clk_[en|dis]abble from the suspend and resume function is correct. The goal is to stop some hardware when the system is suspended, in order to save some power. Why did you removed it? CJ > - > return rc; > } > > static int exynos_ehci_resume(struct device *dev) > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); > int ret; > > - ret = clk_prepare_enable(exynos_ehci->clk); > - if (ret) > - return ret; > - > ret = exynos_ehci_phy_enable(dev); > if (ret) { > dev_err(dev, "Failed to enable USB phy\n"); > - clk_disable_unprepare(exynos_ehci->clk); > return ret; > } >
Hi Christophe, On Sat, 2 Mar 2024 at 21:19, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Le 01/03/2024 à 20:38, Anand Moon a écrit : > > The devm_clk_get_enabled() helpers: > > - call devm_clk_get() > > - call clk_prepare_enable() and register what is needed in order to > > call clk_disable_unprepare() when needed, as a managed resource. > > > > This simplifies the code and avoids the calls to clk_disable_unprepare(). > > > > While at it, use dev_err_probe consistently, and use its return value > > to return the error code. > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > drivers/usb/host/ehci-exynos.c | 30 +++++------------------------- > > 1 file changed, 5 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > > index f644b131cc0b..05aa3d9c2a3b 100644 > > --- a/drivers/usb/host/ehci-exynos.c > > +++ b/drivers/usb/host/ehci-exynos.c > > @@ -159,19 +159,12 @@ static int exynos_ehci_probe(struct platform_device *pdev) > > > > err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); > > if (err) > > - goto fail_clk; > > - > > - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); > > - > > - if (IS_ERR(exynos_ehci->clk)) { > > - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); > > - err = PTR_ERR(exynos_ehci->clk); > > - goto fail_clk; > > - } > > + goto fail_io; > > > > - err = clk_prepare_enable(exynos_ehci->clk); > > - if (err) > > - goto fail_clk; > > + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); > > + if (IS_ERR(exynos_ehci->clk)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), > > + "Failed to get usbhost clock\n"); > > > > hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > > if (IS_ERR(hcd->regs)) { > > @@ -223,8 +216,6 @@ static int exynos_ehci_probe(struct platform_device *pdev) > > exynos_ehci_phy_disable(&pdev->dev); > > pdev->dev.of_node = exynos_ehci->of_node; > > fail_io: > > - clk_disable_unprepare(exynos_ehci->clk); > > -fail_clk: > > usb_put_hcd(hcd); > > return err; > > } > > @@ -240,8 +231,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) > > > > exynos_ehci_phy_disable(&pdev->dev); > > > > - clk_disable_unprepare(exynos_ehci->clk); > > - > > usb_put_hcd(hcd); > > } > > > > @@ -249,7 +238,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) > > static int exynos_ehci_suspend(struct device *dev) > > { > > struct usb_hcd *hcd = dev_get_drvdata(dev); > > - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); > > > > bool do_wakeup = device_may_wakeup(dev); > > int rc; > > @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev) > > > > exynos_ehci_phy_disable(dev); > > > > - clk_disable_unprepare(exynos_ehci->clk); > > Hi, > > I don't think that removing clk_[en|dis]abble from the suspend and > resume function is correct. > > The goal is to stop some hardware when the system is suspended, in order > to save some power. Yes correct, > > Why did you removed it? > devm_clk_get_enabled function register callback for clk_prepare_enable and clk_disable_unprepare, so when the clock resource is not used it should get disabled. [0] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-devres.c#L75 I have also tested with rtc suspend & resume and did not find any issue. > CJ Thanks -Anand > > > - > > return rc; > > } > > > > static int exynos_ehci_resume(struct device *dev) > > { > > struct usb_hcd *hcd = dev_get_drvdata(dev); > > - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); > > int ret; > > > > - ret = clk_prepare_enable(exynos_ehci->clk); > > - if (ret) > > - return ret; > > - > > ret = exynos_ehci_phy_enable(dev); > > if (ret) { > > dev_err(dev, "Failed to enable USB phy\n"); > > - clk_disable_unprepare(exynos_ehci->clk); > > return ret; > > } > > >
Le 02/03/2024 à 17:35, Anand Moon a écrit : > Hi Christophe, > > On Sat, 2 Mar 2024 at 21:19, Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> >> Le 01/03/2024 à 20:38, Anand Moon a écrit : >>> The devm_clk_get_enabled() helpers: >>> - call devm_clk_get() >>> - call clk_prepare_enable() and register what is needed in order to >>> call clk_disable_unprepare() when needed, as a managed resource. >>> >>> This simplifies the code and avoids the calls to clk_disable_unprepare(). >>> >>> While at it, use dev_err_probe consistently, and use its return value >>> to return the error code. >>> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com> >>> --- >>> drivers/usb/host/ehci-exynos.c | 30 +++++------------------------- >>> 1 file changed, 5 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c >>> index f644b131cc0b..05aa3d9c2a3b 100644 >>> --- a/drivers/usb/host/ehci-exynos.c >>> +++ b/drivers/usb/host/ehci-exynos.c >>> @@ -159,19 +159,12 @@ static int exynos_ehci_probe(struct platform_device *pdev) >>> >>> err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); >>> if (err) >>> - goto fail_clk; >>> - >>> - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); >>> - >>> - if (IS_ERR(exynos_ehci->clk)) { >>> - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); >>> - err = PTR_ERR(exynos_ehci->clk); >>> - goto fail_clk; >>> - } >>> + goto fail_io; >>> >>> - err = clk_prepare_enable(exynos_ehci->clk); >>> - if (err) >>> - goto fail_clk; >>> + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); >>> + if (IS_ERR(exynos_ehci->clk)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), >>> + "Failed to get usbhost clock\n"); >>> >>> hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); >>> if (IS_ERR(hcd->regs)) { >>> @@ -223,8 +216,6 @@ static int exynos_ehci_probe(struct platform_device *pdev) >>> exynos_ehci_phy_disable(&pdev->dev); >>> pdev->dev.of_node = exynos_ehci->of_node; >>> fail_io: >>> - clk_disable_unprepare(exynos_ehci->clk); >>> -fail_clk: >>> usb_put_hcd(hcd); >>> return err; >>> } >>> @@ -240,8 +231,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) >>> >>> exynos_ehci_phy_disable(&pdev->dev); >>> >>> - clk_disable_unprepare(exynos_ehci->clk); >>> - >>> usb_put_hcd(hcd); >>> } >>> >>> @@ -249,7 +238,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) >>> static int exynos_ehci_suspend(struct device *dev) >>> { >>> struct usb_hcd *hcd = dev_get_drvdata(dev); >>> - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); >>> >>> bool do_wakeup = device_may_wakeup(dev); >>> int rc; >>> @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev) >>> >>> exynos_ehci_phy_disable(dev); >>> >>> - clk_disable_unprepare(exynos_ehci->clk); >> >> Hi, >> >> I don't think that removing clk_[en|dis]abble from the suspend and >> resume function is correct. >> >> The goal is to stop some hardware when the system is suspended, in order >> to save some power. > Yes correct, >> >> Why did you removed it? >> > > devm_clk_get_enabled function register callback for clk_prepare_enable > and clk_disable_unprepare, so when the clock resource is not used it should get > disabled. Same explanation as in the other patch. The registered function is called when the driver is *unloaded*, not when it magically knows that some things can be disabled or enabled. CJ > > [0] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-devres.c#L75 > > I have also tested with rtc suspend & resume and did not find any issue. > >> CJ > > Thanks > -Anand >> >>> - >>> return rc; >>> } >>> >>> static int exynos_ehci_resume(struct device *dev) >>> { >>> struct usb_hcd *hcd = dev_get_drvdata(dev); >>> - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); >>> int ret; >>> >>> - ret = clk_prepare_enable(exynos_ehci->clk); >>> - if (ret) >>> - return ret; >>> - >>> ret = exynos_ehci_phy_enable(dev); >>> if (ret) { >>> dev_err(dev, "Failed to enable USB phy\n"); >>> - clk_disable_unprepare(exynos_ehci->clk); >>> return ret; >>> } >>> >> > >
Le 02/03/2024 à 17:35, Anand Moon a écrit : > Hi Christophe, > > On Sat, 2 Mar 2024 at 21:19, Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> >> Le 01/03/2024 à 20:38, Anand Moon a écrit : >>> The devm_clk_get_enabled() helpers: >>> - call devm_clk_get() >>> - call clk_prepare_enable() and register what is needed in order to >>> call clk_disable_unprepare() when needed, as a managed resource. >>> >>> This simplifies the code and avoids the calls to clk_disable_unprepare(). >>> >>> While at it, use dev_err_probe consistently, and use its return value >>> to return the error code. >>> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com> >>> --- >>> drivers/usb/host/ehci-exynos.c | 30 +++++------------------------- >>> 1 file changed, 5 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c >>> index f644b131cc0b..05aa3d9c2a3b 100644 >>> --- a/drivers/usb/host/ehci-exynos.c >>> +++ b/drivers/usb/host/ehci-exynos.c >>> @@ -159,19 +159,12 @@ static int exynos_ehci_probe(struct platform_device *pdev) >>> >>> err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); >>> if (err) >>> - goto fail_clk; >>> - >>> - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); >>> - >>> - if (IS_ERR(exynos_ehci->clk)) { >>> - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); >>> - err = PTR_ERR(exynos_ehci->clk); >>> - goto fail_clk; >>> - } >>> + goto fail_io; >>> >>> - err = clk_prepare_enable(exynos_ehci->clk); >>> - if (err) >>> - goto fail_clk; >>> + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); >>> + if (IS_ERR(exynos_ehci->clk)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), >>> + "Failed to get usbhost clock\n"); >>> >>> hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); >>> if (IS_ERR(hcd->regs)) { >>> @@ -223,8 +216,6 @@ static int exynos_ehci_probe(struct platform_device *pdev) >>> exynos_ehci_phy_disable(&pdev->dev); >>> pdev->dev.of_node = exynos_ehci->of_node; >>> fail_io: >>> - clk_disable_unprepare(exynos_ehci->clk); >>> -fail_clk: >>> usb_put_hcd(hcd); >>> return err; >>> } >>> @@ -240,8 +231,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) >>> >>> exynos_ehci_phy_disable(&pdev->dev); >>> >>> - clk_disable_unprepare(exynos_ehci->clk); >>> - >>> usb_put_hcd(hcd); >>> } >>> >>> @@ -249,7 +238,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) >>> static int exynos_ehci_suspend(struct device *dev) >>> { >>> struct usb_hcd *hcd = dev_get_drvdata(dev); >>> - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); >>> >>> bool do_wakeup = device_may_wakeup(dev); >>> int rc; >>> @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev) >>> >>> exynos_ehci_phy_disable(dev); >>> >>> - clk_disable_unprepare(exynos_ehci->clk); >> >> Hi, >> >> I don't think that removing clk_[en|dis]abble from the suspend and >> resume function is correct. >> >> The goal is to stop some hardware when the system is suspended, in order >> to save some power. > Yes correct, >> >> Why did you removed it? >> > > devm_clk_get_enabled function register callback for clk_prepare_enable > and clk_disable_unprepare, so when the clock resource is not used it should get > disabled. Same explanation as in the other patch. The registered function is called when the driver is *unloaded*, not when it magically knows that some things can be disabled or enabled. CJ > > [0] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-devres.c#L75 > > I have also tested with rtc suspend & resume and did not find any issue. > >> CJ > > Thanks > -Anand >> >>> - >>> return rc; >>> } >>> >>> static int exynos_ehci_resume(struct device *dev) >>> { >>> struct usb_hcd *hcd = dev_get_drvdata(dev); >>> - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); >>> int ret; >>> >>> - ret = clk_prepare_enable(exynos_ehci->clk); >>> - if (ret) >>> - return ret; >>> - >>> ret = exynos_ehci_phy_enable(dev); >>> if (ret) { >>> dev_err(dev, "Failed to enable USB phy\n"); >>> - clk_disable_unprepare(exynos_ehci->clk); >>> return ret; >>> } >>> >> > >
On Sat, Mar 02, 2024 at 10:05:46PM +0530, Anand Moon wrote: > On Sat, 2 Mar 2024 at 21:19, Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: > > Le 01/03/2024 à 20:38, Anand Moon a écrit : > > > The devm_clk_get_enabled() helpers: > > > - call devm_clk_get() > > > - call clk_prepare_enable() and register what is needed in order to > > > call clk_disable_unprepare() when needed, as a managed resource. > > > > > > This simplifies the code and avoids the calls to clk_disable_unprepare(). > > > > > > While at it, use dev_err_probe consistently, and use its return value > > > to return the error code. > > > @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev) > > > > > > exynos_ehci_phy_disable(dev); > > > > > > - clk_disable_unprepare(exynos_ehci->clk); > > I don't think that removing clk_[en|dis]abble from the suspend and > > resume function is correct. > > > > The goal is to stop some hardware when the system is suspended, in order > > to save some power. > Yes correct, > > > > Why did you removed it? > devm_clk_get_enabled function register callback for clk_prepare_enable > and clk_disable_unprepare, so when the clock resource is not used it should get > disabled. > > [0] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-devres.c#L75 > > I have also tested with rtc suspend & resume and did not find any issue. You seem to be totally confused about how devres works, and arguing back after Christophe points this out to you instead of going back and doing the homework you should have done before posting these patches is really not OK (e.g. as you're wasting other people's time). And you clearly did not test these patches enough to confirm that you didn't break the driver. Johan
Hi Johon, Christophe, On Mon, 4 Mar 2024 at 14:48, Johan Hovold <johan@kernel.org> wrote: > > On Sat, Mar 02, 2024 at 10:05:46PM +0530, Anand Moon wrote: > > On Sat, 2 Mar 2024 at 21:19, Christophe JAILLET > > <christophe.jaillet@wanadoo.fr> wrote: > > > Le 01/03/2024 à 20:38, Anand Moon a écrit : > > > > > The devm_clk_get_enabled() helpers: > > > > - call devm_clk_get() > > > > - call clk_prepare_enable() and register what is needed in order to > > > > call clk_disable_unprepare() when needed, as a managed resource. > > > > > > > > This simplifies the code and avoids the calls to clk_disable_unprepare(). > > > > > > > > While at it, use dev_err_probe consistently, and use its return value > > > > to return the error code. > > > > > @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev) > > > > > > > > exynos_ehci_phy_disable(dev); > > > > > > > > - clk_disable_unprepare(exynos_ehci->clk); > > > > I don't think that removing clk_[en|dis]abble from the suspend and > > > resume function is correct. > > > > > > The goal is to stop some hardware when the system is suspended, in order > > > to save some power. > > Yes correct, > > > > > > Why did you removed it? > > > devm_clk_get_enabled function register callback for clk_prepare_enable > > and clk_disable_unprepare, so when the clock resource is not used it should get > > disabled. > > > > [0] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-devres.c#L75 > > > > I have also tested with rtc suspend & resume and did not find any issue. > > You seem to be totally confused about how devres works, and arguing back > after Christophe points this out to you instead of going back and doing > the homework you should have done before posting these patches is really > not OK (e.g. as you're wasting other people's time). > Ok, It seems to have fallen short in my understanding.. > And you clearly did not test these patches enough to confirm that you > didn't break the driver. Ok I missed the failure of the ehci driver. while testing. [root@archl-xu4 alarm]# echo no > /sys/module/printk/parameters/console_suspend [root@archl-xu4 alarm]# echo no > /sys/module/printk/parameters/console_suspend [root@archl-xu4 alarm]# echo no > /sys/module/printk/parameters/console_suspend [root@archl-xu4 alarm]# time rtcwake -s 30 -m mem rtcwake: assuming RTC uses UTC ... rtcwake: wakeup from "mem" using /dev/rtc0 at Mon Mar 4 09:44:25 2024 [11969.792928] PM: suspend entry (deep) [11969.798423] Filesystems sync: 0.003 seconds [11969.819722] Freezing user space processes [11969.825818] Freezing user space processes completed (elapsed 0.003 seconds) [11969.831585] OOM killer disabled. [11969.834586] Freezing remaining freezable tasks [11969.841553] Freezing remaining freezable tasks completed (elapsed 0.002 seconds) [11969.919178] sd 0:0:0:0: [sda] Synchronizing SCSI cache [11970.091681] wake enabled for irq 129 (gpx0-4) [11970.135766] wake enabled for irq 149 (gpx0-3) [11970.157943] samsung-pinctrl 13400000.pinctrl: Setting external wakeup interrupt mask: 0xffffffe7 [11970.179304] Disabling non-boot CPUs ... [11970.276394] s3c2410-wdt 101d0000.watchdog: watchdog disabled [11970.281961] wake disabled for irq 149 (gpx0-3) [11970.288997] phy phy-12130000.phy.6: phy_power_on was called before phy_init [11970.358899] exynos-ohci 12120000.usb: init err (00000000 0000) [11970.363298] exynos-ohci 12120000.usb: can't restart, -75 [11970.368581] usb usb2: root hub lost power or was reset [11970.373819] wake disabled for irq 129 (gpx0-4) [11970.382641] xhci-hcd xhci-hcd.8.auto: xHC error in resume, USBSTS 0x411, Reinit [11970.383237] s3c-rtc 101e0000.rtc: rtc disabled, re-enabling [11970.383355] xhci-hcd xhci-hcd.9.auto: xHC error in resume, USBSTS 0x401, Reinit [11970.383376] usb usb5: root hub lost power or was reset [11970.383396] usb usb6: root hub lost power or was reset [11970.388471] usb usb3: root hub lost power or was reset [11970.416740] usb usb4: root hub lost power or was reset [11970.770122] usb 3-1: reset high-speed USB device number 2 using xhci-hcd [11971.100601] usb 4-1: reset SuperSpeed USB device number 3 using xhci-hcd [11971.569524] usb 3-1.2: reset high-speed USB device number 3 using xhci-hcd [11974.575262] OOM killer enabled. [11974.576964] Restarting tasks ... done. [11974.580608] r8152-cfgselector 6-1: USB disconnect, device number 4 [11974.589302] random: crng reseeded on system resumption [11974.596363] PM: suspend exit real 0m34.951s user 0m0.012s sys 0m0.259s [root@archl-xu4 alarm]# [11974.640778] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63) [11975.180552] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0) [11975.192142] mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 200000000Hz, actual 200000000HZ div = 0) [11975.282474] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0) [11975.296174] mmc_host mmc0: Bus speed (slot 0) = 400000000Hz (slot req 200000000Hz, actual 200000000HZ div = 1) [11975.569457] usb 6-1: new SuperSpeed USB device number 5 using xhci-hcd [11975.614390] usb 6-1: New USB device found, idVendor=0bda, idProduct=8153, bcdDevice=30.00 [11975.622196] usb 6-1: New USB device strings: Mfr=1, Product=2, SerialNumber=6 [11975.629284] usb 6-1: Product: USB 10/100/1000 LAN [11975.633352] usb 6-1: Manufacturer: Realtek [11975.637458] usb 6-1: SerialNumber: 000001000000 [11975.871080] r8152-cfgselector 6-1: reset SuperSpeed USB device number 5 using xhci-hcd [11975.955112] r8152 6-1:1.0: load rtl8153a-3 v2 02/07/20 successfully [11976.032484] r8152 6-1:1.0 eth0: v1.12.13 [11976.134078] r8152 6-1:1.0 enu1: renamed from eth0 [root@archl-xu4 alarm]# [11981.522603] r8152 6-1:1.0 enu1: carrier on [root@archl-xu4 alarm]# > Ok, I will restore the clk changes in the suspend / resume functions in the next version and do thought testing. > Johan Thanks -Anand
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index f644b131cc0b..05aa3d9c2a3b 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -159,19 +159,12 @@ static int exynos_ehci_probe(struct platform_device *pdev) err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); if (err) - goto fail_clk; - - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); - - if (IS_ERR(exynos_ehci->clk)) { - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); - err = PTR_ERR(exynos_ehci->clk); - goto fail_clk; - } + goto fail_io; - err = clk_prepare_enable(exynos_ehci->clk); - if (err) - goto fail_clk; + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); + if (IS_ERR(exynos_ehci->clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), + "Failed to get usbhost clock\n"); hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); if (IS_ERR(hcd->regs)) { @@ -223,8 +216,6 @@ static int exynos_ehci_probe(struct platform_device *pdev) exynos_ehci_phy_disable(&pdev->dev); pdev->dev.of_node = exynos_ehci->of_node; fail_io: - clk_disable_unprepare(exynos_ehci->clk); -fail_clk: usb_put_hcd(hcd); return err; } @@ -240,8 +231,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) exynos_ehci_phy_disable(&pdev->dev); - clk_disable_unprepare(exynos_ehci->clk); - usb_put_hcd(hcd); } @@ -249,7 +238,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) static int exynos_ehci_suspend(struct device *dev) { struct usb_hcd *hcd = dev_get_drvdata(dev); - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); bool do_wakeup = device_may_wakeup(dev); int rc; @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev) exynos_ehci_phy_disable(dev); - clk_disable_unprepare(exynos_ehci->clk); - return rc; } static int exynos_ehci_resume(struct device *dev) { struct usb_hcd *hcd = dev_get_drvdata(dev); - struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd); int ret; - ret = clk_prepare_enable(exynos_ehci->clk); - if (ret) - return ret; - ret = exynos_ehci_phy_enable(dev); if (ret) { dev_err(dev, "Failed to enable USB phy\n"); - clk_disable_unprepare(exynos_ehci->clk); return ret; }
The devm_clk_get_enabled() helpers: - call devm_clk_get() - call clk_prepare_enable() and register what is needed in order to call clk_disable_unprepare() when needed, as a managed resource. This simplifies the code and avoids the calls to clk_disable_unprepare(). While at it, use dev_err_probe consistently, and use its return value to return the error code. Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- drivers/usb/host/ehci-exynos.c | 30 +++++------------------------- 1 file changed, 5 insertions(+), 25 deletions(-)