Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <3e00b2ad-b58f-4b09-9230-683c58d3bb92@kernel.org>
Date:   Fri, 17 Nov 2023 13:51:49 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for
 J7200
Content-Language: en-US
To:     =?UTF-8?Q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Peter Chen <peter.chen@kernel.org>,
        Pawel Laszczak <pawell@cadence.com>,
        Nishanth Menon <nm@ti.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Tero Kristo <kristo@kernel.org>,
        "Vardhan, Vibhore" <vibhore@ti.com>
Cc:     linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        =?UTF-8?Q?Gr=C3=A9gory_Clement?= <gregory.clement@bootlin.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
References: <20231113-j7200-usb-suspend-v1-0-ad1ee714835c@bootlin.com>
 <20231113-j7200-usb-suspend-v1-3-ad1ee714835c@bootlin.com>
 <5080372b-1f48-4cbc-a6c4-8689c28983cb@kernel.org>
 <CWZH66HQZNYM.T623ZOEEE0BK@tleb-bootlin-xps13-01>
 <dad980f3-e032-41e4-a1e4-a16a7f45ff95@kernel.org>
 <CX0GOP07I40N.198G7LJ0HYDBG@tleb-bootlin-xps13-01>
 <bdea68ad-7523-4738-8fa1-b670d81a6b93@kernel.org>
 <CX10D9YX1O1C.30PF317AG065N@tleb-bootlin-xps13-01>
From:   Roger Quadros <rogerq@kernel.org>
In-Reply-To: <CX10D9YX1O1C.30PF317AG065N@tleb-bootlin-xps13-01>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 17/11/2023 12:17, Théo Lebrun wrote:
> Hello,
> 
> On Thu Nov 16, 2023 at 10:44 PM CET, Roger Quadros wrote:
>> On 16/11/2023 20:56, Théo Lebrun wrote:
>>> On Thu Nov 16, 2023 at 1:40 PM CET, Roger Quadros wrote:
>>>> On 15/11/2023 17:02, Théo Lebrun wrote:
>>>>> On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote:
>>>>>> You might want to check suspend/resume ops in cdns3-plat and
>>>>>> do something similar here.
>>>>>
>>>>> I'm unsure what you are referring to specifically in cdns3-plat?
>>>>
>>>> What I meant is, calling pm_runtime_get/put() from system suspend/resume
>>>> hooks doesn't seem right.
>>>>
>>>> How about using something like pm_runtime_forbid(dev) on devices which
>>>> loose USB context on runtime suspend e.g. J7200.
>>>> So at probe we can get rid of the pm_runtime_get_sync() call.
>>>
>>> What is the goal of enabling PM runtime to then block (ie forbid) it in
>>> its enabled state until system suspend?
>>
>> If USB controller retains context on runtime_suspend on some platforms
>> then we don't want to forbid PM runtime.
> 
> What's the point of runtime PM if nothing is done based on it? This is
> the current behavior of the driver.

Even if driver doesn't have runtime_suspend/resume hooks, wouldn't 
the USB Power domain turn off if we enable runtime PM and allow runtime
autosuspend and all children have runtime suspended?

> 
>>> Thinking some more about it and having read parts of the genpd source,
>>> it's unclear to me why there even is some PM runtime calls in this
>>> driver. No runtime_suspend/runtime_resume callbacks are registered.
>>> Also, power-domains work as expected without any PM runtime calls.
>>
>> Probably it was required when the driver was introduced.
> 
> I'm not seeing any behavior change in cdns3-ti since its addition in Oct
> 2019.
> 
>>> The power domain is turned on when attached to a device
>>> (see genpd_dev_pm_attach). It gets turned off automatically at
>>> suspend_noirq (taking into account the many things that make genpd
>>> complex: multiple devices per PD, subdomains, flags to customise the
>>> behavior, etc.). Removing calls to PM runtime at probe keeps the driver
>>> working.
>>>
>>> So my new proposal would be: remove all all PM runtime calls from this
>>> driver. Anything wrong with this approach?
>>
>> Nothing wrong if we don't expect runtime_pm to work with this driver.
>>
>>>
>>> Only possible reason I see for having PM runtime in this wrapper driver
>>> would be cut the full power-domain when USB isn't used, with some PM
>>> runtime interaction with the children node. But that cannot work
>>> currently as we don't register a runtime_resume to init the hardware,
>>> so this cannot be the current expected behavior.
>>>
>>>> e.g.
>>>>
>>>>         pm_runtime_set_active(dev);
>>>>         pm_runtime_enable(dev);
>>>>         if (cnds_ti->can_loose_context)
>>>>                 pm_runtime_forbid(dev);
>>>>
>>>>         pm_runtime_set_autosuspend_delay(dev, CNDS_TI_AUTOSUSPEND_DELAY);	/* could be 20ms? */
>>>
>>> Why mention autosuspend in this driver? This will turn the device off in
>>> CNDS_TI_AUTOSUSPEND_DELAY then nothing enables it back using
>>> pm_runtime_get. We have nothing to reconfigure the device, ie no
>>> runtime_resume, so we must not go into runtime suspend.
>>
>> It would be enabled/disabled based on when the child "cdns3,usb"
>> does runtime_resume/suspend.
> 
> Why care about being enabled or disabled if we don't do anything based
> on that? Children does do runtime PM stuff but I don't understand how
> that could influence us.
> 
> Regards,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
cheers,
-roger