diff mbox series

[v1,1/4] usb: ehci-exynos: Use devm_clk_get_enabled() helpers

Message ID 20240301193831.3346-2-linux.amoon@gmail.com
State New
Headers
Series [v1,1/4] usb: ehci-exynos: Use devm_clk_get_enabled() helpers |

Commit Message

Anand Moon March 1, 2024, 7:38 p.m. UTC 
  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(-)

Comments

Alan Stern March 1, 2024, 8:19 p.m. UTC | #1 
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
>
Anand Moon March 2, 2024, 3:41 p.m. UTC | #2 
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
Christophe JAILLET March 2, 2024, 3:49 p.m. UTC | #3 
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;
>   	}
>
Anand Moon March 2, 2024, 4:35 p.m. UTC | #4 
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;
> >       }
> >
>
Christophe JAILLET March 2, 2024, 6:41 p.m. UTC | #5 
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;
>>>        }
>>>
>>
> 
>
Johan Hovold March 4, 2024, 9:18 a.m. UTC | #6 
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
Anand Moon March 4, 2024, 10:05 a.m. UTC | #7 
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 mbox series

Patch

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;
 	}