[PATCH v1 0/2] Fix Tegra PMC driver racing with cpuidle driver

[PATCH v1 0/2] Fix Tegra PMC driver racing with cpuidle driver

[Dmitry Osipenko]

I stumbled upon a problem where tegra-cpuidle driver is probed before
Power Management controller driver in a specific kernel configuration,
implicitly disabling the deepest CPU idling state. This series fixes
that trouble.

Dmitry Osipenko (2):
  soc/tegra: pmc: Prevent racing with cpuilde driver
  cpuidle: tegra: Check whether PMC is ready

 arch/arm/mach-tegra/pm.c        |  2 +-
 arch/arm/mach-tegra/pm.h        |  6 ------
 arch/arm/mach-tegra/tegra.c     |  2 --
 drivers/cpuidle/cpuidle-tegra.c |  3 +++
 drivers/soc/tegra/pmc.c         | 14 +++++++++++++-
 include/soc/tegra/pm.h          |  6 ++++++
 6 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.32.0

[PATCH v1 1/2] soc/tegra: pmc: Prevent racing with cpuilde driver

[Dmitry Osipenko]

Both PMC and cpuidle drivers are probed at the same init level and
cpuidle depends on the PMC suspend mode. Add new default suspend mode
that indicates whether PMC driver has been probed and reset the mode in
a case of deferred probe of the PMC driver.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 arch/arm/mach-tegra/pm.c    |  2 +-
 arch/arm/mach-tegra/pm.h    |  6 ------
 arch/arm/mach-tegra/tegra.c |  2 --
 drivers/soc/tegra/pmc.c     | 14 +++++++++++++-
 include/soc/tegra/pm.h      |  6 ++++++
 5 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 6452ebf68d40..b21f51b8e19e 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -403,7 +403,7 @@ static const struct platform_suspend_ops tegra_suspend_ops = {
 	.enter		= tegra_suspend_enter,
 };
 
-void __init tegra_init_suspend(void)
+void tegra_pm_init_suspend(void)
 {
 	enum tegra_suspend_mode mode = tegra_pmc_get_suspend_mode();
 
diff --git a/arch/arm/mach-tegra/pm.h b/arch/arm/mach-tegra/pm.h
index 81525f5f4a44..e63f96de2825 100644
--- a/arch/arm/mach-tegra/pm.h
+++ b/arch/arm/mach-tegra/pm.h
@@ -25,10 +25,4 @@ void tegra30_sleep_core_init(void);
 
 extern void (*tegra_tear_down_cpu)(void);
 
-#ifdef CONFIG_PM_SLEEP
-void tegra_init_suspend(void);
-#else
-static inline void tegra_init_suspend(void) {}
-#endif
-
 #endif /* _MACH_TEGRA_PM_H_ */
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index c011359bcdb4..ab5008f35803 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -84,8 +84,6 @@ static void __init tegra_dt_init(void)
 
 static void __init tegra_dt_init_late(void)
 {
-	tegra_init_suspend();
-
 	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
 	    of_machine_is_compatible("compal,paz00"))
 		tegra_paz00_wifikill_init();
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index ea62f84d1c8b..50091c4ec948 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -436,7 +436,7 @@ struct tegra_pmc {
 
 static struct tegra_pmc *pmc = &(struct tegra_pmc) {
 	.base = NULL,
-	.suspend_mode = TEGRA_SUSPEND_NONE,
+	.suspend_mode = TEGRA_SUSPEND_NOT_READY,
 };
 
 static inline struct tegra_powergate *
@@ -1812,6 +1812,7 @@ static int tegra_pmc_parse_dt(struct tegra_pmc *pmc, struct device_node *np)
 	u32 value, values[2];
 
 	if (of_property_read_u32(np, "nvidia,suspend-mode", &value)) {
+		pmc->suspend_mode = TEGRA_SUSPEND_NONE;
 	} else {
 		switch (value) {
 		case 0:
@@ -2785,6 +2786,11 @@ static int tegra_pmc_regmap_init(struct tegra_pmc *pmc)
 	return 0;
 }
 
+static void tegra_pmc_reset_suspend_mode(void *data)
+{
+	pmc->suspend_mode = TEGRA_SUSPEND_NOT_READY;
+}
+
 static int tegra_pmc_probe(struct platform_device *pdev)
 {
 	void __iomem *base;
@@ -2803,6 +2809,11 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
+	err = devm_add_action_or_reset(&pdev->dev, tegra_pmc_reset_suspend_mode,
+				       NULL);
+	if (err)
+		return err;
+
 	/* take over the memory region from the early initialization */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
@@ -2909,6 +2920,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	tegra_pmc_clock_register(pmc, pdev->dev.of_node);
 	platform_set_drvdata(pdev, pmc);
+	tegra_pm_init_suspend();
 
 	return 0;
 
diff --git a/include/soc/tegra/pm.h b/include/soc/tegra/pm.h
index 08477d7bfab9..433878927026 100644
--- a/include/soc/tegra/pm.h
+++ b/include/soc/tegra/pm.h
@@ -14,6 +14,7 @@ enum tegra_suspend_mode {
 	TEGRA_SUSPEND_LP1, /* CPU voltage off, DRAM self-refresh */
 	TEGRA_SUSPEND_LP0, /* CPU + core voltage off, DRAM self-refresh */
 	TEGRA_MAX_SUSPEND_MODE,
+	TEGRA_SUSPEND_NOT_READY,
 };
 
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM)
@@ -28,6 +29,7 @@ void tegra_pm_clear_cpu_in_lp2(void);
 void tegra_pm_set_cpu_in_lp2(void);
 int tegra_pm_enter_lp2(void);
 int tegra_pm_park_secondary_cpu(unsigned long cpu);
+void tegra_pm_init_suspend(void);
 #else
 static inline enum tegra_suspend_mode
 tegra_pm_validate_suspend_mode(enum tegra_suspend_mode mode)
@@ -61,6 +63,10 @@ static inline int tegra_pm_park_secondary_cpu(unsigned long cpu)
 {
 	return -ENOTSUPP;
 }
+
+static inline void tegra_pm_init_suspend(void)
+{
+}
 #endif /* CONFIG_PM_SLEEP */
 
 #endif /* __SOC_TEGRA_PM_H__ */
-- 
2.32.0

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

[Dmitry Osipenko]

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(+)

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

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

[Thierry Reding]

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

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
> 

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

Re: [PATCH v1 1/2] soc/tegra: pmc: Prevent racing with cpuilde driver

[Thierry Reding]

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

On Mon, Jul 19, 2021 at 12:27:05AM +0300, Dmitry Osipenko wrote:
> Both PMC and cpuidle drivers are probed at the same init level and
> cpuidle depends on the PMC suspend mode. Add new default suspend mode
> that indicates whether PMC driver has been probed and reset the mode in
> a case of deferred probe of the PMC driver.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  arch/arm/mach-tegra/pm.c    |  2 +-
>  arch/arm/mach-tegra/pm.h    |  6 ------
>  arch/arm/mach-tegra/tegra.c |  2 --
>  drivers/soc/tegra/pmc.c     | 14 +++++++++++++-
>  include/soc/tegra/pm.h      |  6 ++++++
>  5 files changed, 20 insertions(+), 10 deletions(-)

Applied, thanks.

Thierry

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

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

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


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Dmitry Osipenko]

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?

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

[Thierry Reding]

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

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

[Dmitry Osipenko]

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.