Received-SPF: pass (google.com: domain of linux-kernel+bounces-89553-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <aee270dc-80b2-48c1-a7cb-b1692b5a7677@wanadoo.fr>
Date: Sat, 2 Mar 2024 19:41:15 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 1/4] usb: ehci-exynos: Use devm_clk_get_enabled()
 helpers
Content-Language: en-MW
To: Anand Moon <linux.amoon@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
 Alim Akhtar <alim.akhtar@samsung.com>, linux-usb@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240301193831.3346-1-linux.amoon@gmail.com>
 <20240301193831.3346-2-linux.amoon@gmail.com>
 <3d1c7682-d163-4bcf-bd41-d7db0c8f61d1@wanadoo.fr>
 <CANAwSgR0aQ7nt1y5xknvVjHSnfvTaC8JZMLWurb8z2D0Oxg6Rw@mail.gmail.com>
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
In-Reply-To: <CANAwSgR0aQ7nt1y5xknvVjHSnfvTaC8JZMLWurb8z2D0Oxg6Rw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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