| Message ID | 20210718212706.21659-3-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Fix Tegra PMC driver racing with cpuidle driver | expand |
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 >
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>
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?
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
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 --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);
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(+)