Fix Tegra PMC driver racing with cpuidle driver
Subsystems (4)
cover Fix Tegra PMC driver racing with cpuidle driver
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(-)
Signature · 1 line
2.32.0
PATCH 1/2 soc/tegra: pmc: Prevent racing with cpuilde driver
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.
--- 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(-)
arch/arm/mach-tegra/pm.c +1 -1
@@ -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 ();
arch/arm/mach-tegra/pm.h +0 -6
@@ -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_ */
arch/arm/mach-tegra/tegra.c +0 -2
@@ -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 ();
drivers/soc/tegra/pmc.c +13 -1
@@ -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 ;
include/soc/tegra/pm.h +6 -0
@@ -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__ */
Signature · 1 line
2.32.0
Thierry Reding
AppliedRe: [PATCH v1 1/2] soc/tegra: pmc: Prevent racing with cpuilde driver
Quoted 5 lines · 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.
Quoted 8 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- 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
PATCH 2/2 cpuidle: tegra: Check whether PMC is ready
In reply to Dmitry OsipenkoCheck 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.
--- drivers/cpuidle/cpuidle-tegra.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/cpuidle/cpuidle-tegra.c +3 -0
@@ -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 );
Signature · 1 line
2.32.0
Re: [PATCH v1 2/2] cpuidle: tegra: Check whether PMC is ready
Quoted 9 lines · 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 <[email protected]>--- 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
Quoted 18 lines
drivers/cpuidle/cpuidle-tegra.c +3
-0@@ -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 );Signature · 1 line
2.32.0
Daniel Lezcano
Acked-byRe: [PATCH v1 2/2] cpuidle: tegra: Check whether PMC is ready
Quoted 11 lines · Thierry Reding
Quoted 9 lines · 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 <[email protected]>--- drivers/cpuidle/cpuidle-tegra.c | 3 +++ 1 file changed, 3 insertions(+)
Quoted 6 lines
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.
Signature · 5 lines
<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
14.08.2021 13:37, Daniel Lezcano пишет:
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Dmitry Osipenko
Quoted 9 lines
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 <[email protected]>--- drivers/cpuidle/cpuidle-tegra.c | 3 +++ 1 file changed, 3 insertions(+)
Quoted 8 lines
Quoted 6 lines
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 <[email protected]>
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
Quoted 20 lines · Dmitry Osipenko
14.08.2021 13:37, Daniel Lezcano пишет:
Quoted 19 lines · Thierry Reding
Quoted 17 lines · Dmitry Osipenko
Quoted 9 lines
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 <[email protected]>--- 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.
Quoted 5 lines
Quoted 1 line
Acked-by Daniel Lezcano <[email protected]>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
Re: [PATCH v1 2/2] cpuidle: tegra: Check whether PMC is ready
16.08.2021 12:53, Thierry Reding пишет:
Quoted 48 lines · Dmitry Osipenko
Quoted 20 lines
14.08.2021 13:37, Daniel Lezcano пишет:
Quoted 19 lines · Thierry Reding
Quoted 17 lines · Dmitry Osipenko
Quoted 9 lines
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 <[email protected]>--- 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.
Quoted 5 lines
Quoted 1 line
Acked-by Daniel Lezcano <[email protected]>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.
Quoted 5 lines
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.