Re: [PATCH 2/3] usb: xhci: tegra: Add runtime PM support

[Jon Hunter]

On 09/03/18 09:13, Mathias Nyman wrote:
> On 09.03.2018 10:36, Thierry Reding wrote:
>> On Thu, Mar 08, 2018 at 09:31:07PM +0000, Jon Hunter wrote:
>>>
>>> On 01/03/18 14:18, Mathias Nyman wrote:
>>>> On 14.02.2018 18:34, Jon Hunter wrote:
>>>>> Add runtime PM support to the Tegra XHCI driver and move the function
>>>>> calls to enable/disable the clocks, regulators and PHY into the
>>>>> runtime
>>>>> PM callbacks.
>>>>>
>>>>> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>>>>> ---
>>>>> ÂÂ drivers/usb/host/xhci-tegra.c | 80
>>>>> ++++++++++++++++++++++++++++++-------------
>>>>> ÂÂ 1 file changed, 56 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/host/xhci-tegra.c
>>>>> b/drivers/usb/host/xhci-tegra.c
>>>>> index 02b0b24faa58..42aa67858b53 100644
>>>>> --- a/drivers/usb/host/xhci-tegra.c
>>>>> +++ b/drivers/usb/host/xhci-tegra.c
>>>>> @@ -18,6 +18,7 @@
>>>>> ÂÂ #include <linux/phy/tegra/xusb.h>
>>>>> ÂÂ #include <linux/platform_device.h>
>>>>> ÂÂ #include <linux/pm.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>> ÂÂ #include <linux/regulator/consumer.h>
>>>>> ÂÂ #include <linux/reset.h>
>>>>> ÂÂ #include <linux/slab.h>
>>>>> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
>>>>> platform_device *pdev)
>>>>> ÂÂÂÂÂÂÂ */
>>>>> ÂÂÂÂÂÂ platform_set_drvdata(pdev, tegra);
>>>>> ÂÂ -ÂÂÂ err = tegra_xusb_clk_enable(tegra);
>>>>> -ÂÂÂ if (err) {
>>>>> -ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
>>>>> -ÂÂÂÂÂÂÂ goto put_usb2;
>>>>> -ÂÂÂ }
>>>>> -
>>>>> -ÂÂÂ err = regulator_bulk_enable(tegra->soc->num_supplies,
>>>>> tegra->supplies);
>>>>> -ÂÂÂ if (err) {
>>>>> -ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to enable regulators: %d\n",
>>>>> err);
>>>>> -ÂÂÂÂÂÂÂ goto disable_clk;
>>>>> -ÂÂÂ }
>>>>> +ÂÂÂ pm_runtime_enable(&pdev->dev);
>>>>> ÂÂ -ÂÂÂ err = tegra_xusb_phy_enable(tegra);
>>>>> +ÂÂÂ err = pm_runtime_get_sync(&pdev->dev);
>>>>> ÂÂÂÂÂÂ if (err < 0) {
>>>>
>>>> Does this mean that if runtime PM is disabled then clocks and regulator
>>>> will never be enabled
>>>> for Tegra xhci?
>>>>
>>>> How about keeping the clock and regualtor enabling in probe, and
>>>> instead
>>>> add something like:
>>>>
>>>> pm_runtime_set_active(&pdev->dev);
>>>> pm_runtime_enable(&pdev->dev);
>>>> pm_runtime_get_noresume(&pdev->dev);
>>>
>>> For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
>>> AFAIK there is not and so yes we should handle the case when PM_RUNTIME
>>> is disabled.
>>>
>>> Typically we do something like ...
>>>
>>> ÂÂÂÂ pm_runtime_enable(&pdev->dev);
>>> ÂÂÂÂ if (!pm_runtime_enabled(&pdev->dev))
>>> ÂÂÂÂret = tegra_xusb_runtime_resume(&pdev->dev);
>>> ÂÂÂÂ else
>>> ÂÂÂÂÂÂÂÂ ret = pm_runtime_get_sync(&pdev->dev);
>>>
>>> That way we can keep the regulator and clock stuff in the handler. I
>>> will update this series.
>>
>> Is there any good reason why we don't depend on PM for 32-bit as well?
>> I'm not aware of any differences in drivers that are 32-bit specific for
>> Tegra, and I'm not even sure the !PM case gets any testing at all. And
>> even if, do we really still want to support that?
>>
>> I don't see any advantage these days for having it disabled.
> 
> I don't know much about Tegra, but I'd still like to turn this question
> around:
> 
> Is there any reason why clks and regulators can't initially be turned on
> in probe,
> and then let runtime PM handle them later if PM is supported?

I personally prefer having the regulator, clock, etc handling for
enabling and disabling for a device in their own handler. Duplicating
the calls to the regulator and clock frameworks in probe and the rpm
handlers seems more prone to mistakes. Hence, that's why I would prefer
the option I suggested above, which IMO is the best of both worlds.

> Shouldn't this work in all cases, and it avoids creating new dependencies?

Yes but when you want to use frameworks such as genpd it becomes more
complex to support !PM and that is why for 64-bit Tegra we have a
dependency today.

Cheers
Jon

-- 
nvpublic