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

From: Dmitry Osipenko <digetx@gmail.com>
To: Thierry Reding <thierry.reding@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 18:32:41 +0300	[thread overview]
Message-ID: <1fb32fd0-97c0-9215-6e38-c53c5155f27a@gmail.com> (raw)
In-Reply-To: <YRo1hK/KnS0oA1vj@orome.fritz.box>

16.08.2021 12:53, Thierry Reding пишет:
> 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.

It's doable, but it's a bit too much effort for a little problem we have
here. It also doesn't solve the root of the problem since PMC isn't a
part of CCPLEX. Should be better to stick with this patch for now then.