Re: [PATCH v1 2/2] cpuidle: tegra: Check whether PMC is ready

From: Thierry Reding <thierry.reding@gmail.com>
To: Dmitry Osipenko <digetx@gmail.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] cpuidle: tegra: Check whether PMC is ready
Date: Mon, 16 Aug 2021 11:53:08 +0200	[thread overview]
Message-ID: <YRo1hK/KnS0oA1vj@orome.fritz.box> (raw)
In-Reply-To: <8f22c97d-63f7-6d2a-7b1e-92b0b80a5275@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]

On Sat, Aug 14, 2021 at 04:45:42PM +0300, Dmitry Osipenko wrote:
> 14.08.2021 13:37, Daniel Lezcano пишет:
> > On 11/08/2021 11:49, Thierry Reding wrote:
> >> On Mon, Jul 19, 2021 at 12:27:06AM +0300, Dmitry Osipenko wrote:
> >>> Check whether PMC is ready before proceeding with the cpuidle registration.
> >>> This fixes racing with the PMC driver probe order, which results in a
> >>> disabled deepest CC6 idling state if cpuidle driver is probed before the
> >>> PMC.
> >>>
> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>> ---
> >>>  drivers/cpuidle/cpuidle-tegra.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>
> >> Rafael, Daniel,
> >>
> >> would you mind if I took this into the Tegra tree? It's got a dependency
> >> on the PMC driver, which usually goes via the Tegra tree already, and
> >> there's nothing cpuidle-specific in here, it's all Tegra-specific
> >> integration quirks.
> > 
> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> I got another thought about how it could be solved. We could move the
> creation of the cpuidle platform device into the PMC driver. Thierry,
> what do you think?

Looking around a bit, it looks like we've got two "virtual" platform
devices related to CPU on Tegra20 and some of the later SoCs. A little
while ago when we introduced the CPU frequency driver for Tegra194 we
had a similar discussion. The problem at the time was that there was no
way to create a virtual platform device from platform code, and adding a
device tree node for this wasn't really an option either, since it does
not actually describe the hardware accurately.

What we ended up doing for Tegra194 was to add a compatible string to
the /cpus node ("nvidia,tegra194-ccplex") which was then used for
matching a CPU frequency driver against.

I imagine we could do something similar for these older chips and
perhaps even have a single driver for the CCPLEX that either registers
CPU idle and CPU frequency scaling functionality, or have that driver
register virtual devices. I slightly prefer the first variant because
then we associate the driver with the hardware that it's actually
driving. It's slightly unconventional because now CPU idle and CPU
frequency drivers would be implemented in the same driver, but it isn't
all that exotic these days anymore, either.

If the maintainers prefer we could always keep the code split into two
source files, one per subsystem, and call into that code from the CCPLEX
driver. I think even then it'd still be the cleanest solution because we
don't have to "invent" a new device just for the sake of fitting the
driver model that we happen to have.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]