diff mbox series

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

Message ID 20210718212706.21659-3-digetx@gmail.com
State New
Headers show
Series Fix Tegra PMC driver racing with cpuidle driver | expand

Commit Message

Dmitry Osipenko July 18, 2021, 9:27 p.m. UTC
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(+)

Comments

Thierry Reding Aug. 11, 2021, 9:49 a.m. UTC | #1
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.

Thierry

> 
> diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c
> index 508bd9f23792..9845629aeb6d 100644
> --- a/drivers/cpuidle/cpuidle-tegra.c
> +++ b/drivers/cpuidle/cpuidle-tegra.c
> @@ -337,6 +337,9 @@ static void tegra_cpuidle_setup_tegra114_c7_state(void)
>  
>  static int tegra_cpuidle_probe(struct platform_device *pdev)
>  {
> +	if (tegra_pmc_get_suspend_mode() == TEGRA_SUSPEND_NOT_READY)
> +		return -EPROBE_DEFER;
> +
>  	/* LP2 could be disabled in device-tree */
>  	if (tegra_pmc_get_suspend_mode() < TEGRA_SUSPEND_LP2)
>  		tegra_cpuidle_disable_state(TEGRA_CC6);
> -- 
> 2.32.0
>
Daniel Lezcano Aug. 14, 2021, 10:37 a.m. UTC | #2
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>
Dmitry Osipenko Aug. 14, 2021, 1:45 p.m. UTC | #3
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?
Thierry Reding Aug. 16, 2021, 9:53 a.m. UTC | #4
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
Dmitry Osipenko Aug. 16, 2021, 3:32 p.m. UTC | #5
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.
diff mbox series

Patch

diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c
index 508bd9f23792..9845629aeb6d 100644
--- a/drivers/cpuidle/cpuidle-tegra.c
+++ b/drivers/cpuidle/cpuidle-tegra.c
@@ -337,6 +337,9 @@  static void tegra_cpuidle_setup_tegra114_c7_state(void)
 
 static int tegra_cpuidle_probe(struct platform_device *pdev)
 {
+	if (tegra_pmc_get_suspend_mode() == TEGRA_SUSPEND_NOT_READY)
+		return -EPROBE_DEFER;
+
 	/* LP2 could be disabled in device-tree */
 	if (tegra_pmc_get_suspend_mode() < TEGRA_SUSPEND_LP2)
 		tegra_cpuidle_disable_state(TEGRA_CC6);