Message-ID: <YM0iBYCL9FHlsue2@gerhold.net>
Date:   Sat, 19 Jun 2021 00:45:25 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
Cc:     bjorn.andersson@...aro.org, agross@...nel.org,
        daniel.lezcano@...aro.org, rjw@...ysocki.net,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, phone-devel@...r.kernel.org,
        konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
        martin.botka@...ainline.org, jeffrey.l.hugo@...il.com,
        jamipkettunen@...ainline.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [RESEND PATCH v4 1/3] cpuidle: qcom_spm: Detach state machine
 from main SPM handling

On Sat, Jun 19, 2021 at 12:32:50AM +0200, AngeloGioacchino Del Regno wrote:
> Il 18/06/21 23:37, Stephan Gerhold ha scritto:
> > > Though, it was ignored that the SPM driver is not used only
> > > on the ARM architecture.
> > 
> > This sentence is a bit misleading IMO. In mainline the SPM driver is
> > *currently* only used for CPUidle, and the old driver was as
> > CPUidle-specific as the new one. So saying that I "ignored" something
> > here is kind of wrong. :)
> > 
> > Can you re-phrase this a bit to say that the SPM hardware is also
> > used for power-collapse of the CPU caches/AVS/whatever and therefore we
> > need to refactor the driver to something more independent?
> > 
> 
> On SAWv4.1, the SPM is used to regulate AVS limits but *not* power
> collapse of the CPU caches "and whatever": platforms with this version
> are using different HW to accomplish that.
> 

OK, thanks for clarifying! My point stands though, I don't think
I "ignored" anything in my refactoring commit. :)

> 
> > > [...]
> > > @@ -213,132 +80,87 @@ static const struct of_device_id qcom_idle_state_match[] = {
> > > -static int spm_cpuidle_init(struct cpuidle_driver *drv, int cpu)
> > > +static int spm_cpuidle_register(int cpu)
> > >   {
> > > +	struct platform_device *pdev = NULL;
> > > +	struct spm_driver_data *spm = NULL;
> > > +	struct device_node *cpu_node, *saw_node;
> > >   	int ret;
> > > -	memcpy(drv, &qcom_spm_idle_driver, sizeof(*drv));
> > > -	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> > 
> > Somehow this line got lost, which means the first cpuidle_driver will
> > cover all CPUs and we will always fail to register the cpuidle_driver
> > for all other CPUs:
> > 
> > [    0.736591] failed to register cpuidle driver
> > [    0.744186] qcom-spm-cpuidle qcom-spm-cpuidle: Cannot register for CPU1: -16
> > [    0.748443] qcom-spm-cpuidle: probe of qcom-spm-cpuidle failed with error -16
> > 
> > (Then the device hangs forever.)
> > 
> 
> So you have discovered a bug for which your platform dies when SPM
> does not probe, probably due to something else being dependant on this.
> In this case, I would encourage you to produce a fix for your platform
> to not unexpectedly just hang forever if *some driver* doesn't probe:
> that's definitely not right.
> 

Fair enough, might investigate this further.

> > I added
> > spm->cpuidle_driver.cpumask = (struct cpumask *)cpumask_of(cpu);
> > below
> > 
> > > +	spm->cpuidle_driver = qcom_spm_idle_driver;
> > 
> > and this seems to make it boot again at least.
> > 
> 
> I really think that I've originally messed up the patch originally:
> this doesn't seem to be the right version, even though it was marked as
> v4. I trusted my folders organization too much. Apologies.
> 

It seems mostly equivalent with the previous v4 and v3, but didn't check
very carefully. No problem though, as long we can get it fixed. :)

> > However, it seems a bit pointless now to have a separate cpuidle_driver
> > per CPU, since they are all registered at the same time. With my
> > refactoring this was kind of convenient because the SPM platform devices
> > could happily probe independently and just register a cpuidle_driver for
> > the CPU they belong to.
> > 
> > With your patch, the cpuidle_drivers are registered at the same time for
> > all CPUs, so we might as well use a single cpuidle_driver that covers
> > all CPUs (like you already do without setting cpumask).
> > 
> > Note that if you have a single cpuidle_driver for all CPUs you need to
> > refactor spm_enter_idle_state() a bit. The container_of() will no longer
> > work to get the CPU-specific SPM. Before my changes there was a
> > DEFINE_PER_CPU for this. I guess we need to bring that back.
> > 
> > >   	[...]
> > > +	ret = dt_init_idle_driver(&spm->cpuidle_driver,
> > > +				  qcom_idle_state_match, 1);
> > > +	if (ret <= 0)
> > > +		return ret ? : -ENODEV;
> > > -	return drv;
> > > -}
> > > +	ret = qcom_scm_set_warm_boot_addr(cpu_resume_arm, cpumask_of(cpu));
> > > +	if (ret)
> > > +		return ret;
> > 
> > And the advantage here is that we should be able to do this with a
> > single firmware call (set the warm boot addr for all CPUs at once).
> > 
> 
> Probably staying with setting the cpumask is a better option; we don't
> really know if there's any platform requiring that kind of quirk: at
> least downstream, I recall that they made sure to send multiple calls.
> 

I don't think it really makes a difference but fair enough. I still
think it could be worth having a single cpuidle_driver (even if we call
qcom_scm_set_warm_boot_addr() separately for each CPU), but I suppose
this is more material for a potential follow-up patch.

> 
> > > [...]
> > > diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
> > > new file mode 100644
> > > index 000000000000..604eca2c4d4a
> > > --- /dev/null
> > > +++ b/include/soc/qcom/spm.h
> > > @@ -0,0 +1,45 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> > > + * Copyright (c) 2014,2015, Linaro Ltd.
> > > + * Copyright (C) 2020, AngeloGioacchino Del Regno <kholk11@...il.com>
> > > + */
> > > +
> > > +#ifndef __SPM_H__
> > > +#define __SPM_H__
> > > +
> > > +#include <linux/cpuidle.h>
> > > +
> > > +#define MAX_PMIC_DATA		2
> > > +#define MAX_SEQ_DATA		64
> > > +
> > > +enum pm_sleep_mode {
> > > +	PM_SLEEP_MODE_STBY,
> > > +	PM_SLEEP_MODE_RET,
> > > +	PM_SLEEP_MODE_SPC,
> > > +	PM_SLEEP_MODE_PC,
> > > +	PM_SLEEP_MODE_NR,
> > > +};
> > > +
> > > +struct spm_reg_data {
> > > +	const u16 *reg_offset;
> > > +	u32 spm_cfg;
> > > +	u32 spm_dly;
> > > +	u32 pmic_dly;
> > > +	u32 pmic_data[MAX_PMIC_DATA];
> > > +	u32 avs_ctl;
> > > +	u32 avs_limit;
> > 
> > Looks like you accidentally included changes from PATCH 2/3
> > ("soc: qcom: spm: Implement support for SAWv4.1, SDM630/660 L2 AVS")
> > here, reg_offset u8 -> u16 and adding avs_ctl and avs_limit should be in
> > a separate patch. It's really hard to see that you added those here
> > while moving the code. :/
> > 
> 
> This change belongs to 2/3.
> Will send a v5 with fixes.
> 

Thanks!
Stephan

Powered by blists - more mailing lists