Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

From: Kevin Hilman <khilman@kernel.org>
To: "Théo Lebrun" <theo.lebrun@bootlin.com>,
	"Roger Quadros" <rogerq@kernel.org>,
	"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,
	"Grégory Clement" <gregory.clement@bootlin.com>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200
Date: Wed, 22 Nov 2023 14:23:04 -0800	[thread overview]
Message-ID: <7h34wxfmwn.fsf@baylibre.com> (raw)
In-Reply-To: <CX15J7B8F8HH.1WZ10OOW31X1H@tleb-bootlin-xps13-01>

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> Hi Roger,
>
> On Fri Nov 17, 2023 at 12:51 PM CET, Roger Quadros wrote:
>> On 17/11/2023 12:17, Théo Lebrun wrote:
>> > 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.

The point is to signal to the power domain the device is in that it can
power on/off.  These IP blocks are (re)used on many different SoCs, so
the driver should not make any assumptions about what power domain it is
in (if any.)

>> 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?
>
> That cannot be the currently desired behavior as it would require a
> runtime_resume implementation that restores this wrapper to its desired
> state.

Or, for this USB IP block to be in a power domain that has memory
retention, in which case the power domain can still go off to save
power, but not lose context.

> It could however be something that could be implemented. It would be a
> matter of enabling PM runtime and that is it in the probe. No need to
> even init the hardware in the probe. Then the runtime_resume
> implementation would call the new cdns_ti_init_hw.

This is the way.

> This is what the cdns3-imx wrapper is doing in a way, though what they
> need is clocks rather than some registers to be written.
>
> That all feels like outside the scope of the current patch series
> though.
>
> My suggestion for V2 would still therefore be to remove all PM runtime
> as it has no impact. It could be added later down the road if cutting
> the power-domain is a goal of yours.

It may have no impact on the platform you are on, but it's very likely
to have an impact on other platforms with this same IP. 

Kevin