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

From: "Théo Lebrun" <theo.lebrun@bootlin.com>
To: "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: Fri, 17 Nov 2023 11:17:55 +0100	[thread overview]
Message-ID: <CX10D9YX1O1C.30PF317AG065N@tleb-bootlin-xps13-01> (raw)
In-Reply-To: <bdea68ad-7523-4738-8fa1-b670d81a6b93@kernel.org>

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.

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