Message-ID: <YM0SCaAdYMxZQya1@gerhold.net>
Date:   Fri, 18 Jun 2021 23:37:13 +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

Hi,

Thanks for the patch!

On Fri, Jun 18, 2021 at 08:09:05PM +0200, AngeloGioacchino Del Regno wrote:
> In commit a871be6b8eee ("cpuidle: Convert Qualcomm SPM driver to a generic
> CPUidle driver") the SPM driver has been converted to a
> generic CPUidle driver: that was mainly made to simplify the
> driver and that was a great accomplishment;
>

Hmm, you mention my commit but did not Cc me.
Only saw this patch while randomly looking through linux-arm-msm. :)

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

> In preparation for the enablement of SPM features on AArch64/ARM64,
> split the cpuidle-qcom-spm driver in two: the CPUIdle related
> state machine (currently used only on ARM SoCs) stays there, while
> the SPM communication handling lands back in soc/qcom/spm.c and
> also making sure to not discard the simplifications that were
> introduced in the aforementioned commit.
> 
> Since now the "two drivers" are split, the SCM dependency in the
> main SPM handling is gone and for this reason it was also possible
> to move the SPM initialization early: this will also make sure that
> whenever the SAW CPUIdle driver is getting initialized, the SPM
> driver will be ready to do the job.
> 
> Please note that the anticipation of the SPM initialization was
> also done to optimize the boot times on platforms that have their
> CPU/L2 idle states managed by other means (such as PSCI), while
> needing SAW initialization for other purposes, like AVS control.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>

The diff is quite hard to read for me (just the nature of this change,
not your fault). Just mentioning this in case I miss something obvious.

> ---
>  drivers/cpuidle/Kconfig.arm        |   1 +
>  drivers/cpuidle/cpuidle-qcom-spm.c | 294 ++++++-----------------------
>  drivers/soc/qcom/Kconfig           |   9 +
>  drivers/soc/qcom/Makefile          |   1 +
>  drivers/soc/qcom/spm.c             | 198 +++++++++++++++++++
>  include/soc/qcom/spm.h             |  45 +++++
>  6 files changed, 312 insertions(+), 236 deletions(-)
>  create mode 100644 drivers/soc/qcom/spm.c
>  create mode 100644 include/soc/qcom/spm.h
> 
> [...]
> diff --git a/drivers/cpuidle/cpuidle-qcom-spm.c b/drivers/cpuidle/cpuidle-qcom-spm.c
> index adf91a6e4d7d..3dd7bb10b82d 100644
> --- a/drivers/cpuidle/cpuidle-qcom-spm.c
> +++ b/drivers/cpuidle/cpuidle-qcom-spm.c
> [...]
> -static int spm_dev_probe(struct platform_device *pdev)
> +static int spm_cpuidle_drv_probe(struct platform_device *pdev)
>  {
>  	[...]
> +	for_each_possible_cpu(cpu) {
> +		ret = spm_cpuidle_register(cpu);
> +		if (ret != -ENODEV) {
> +			dev_err(&pdev->dev,
> +				"Cannot register for CPU%d: %d\n", cpu, ret);
> +			break;

Huh, why does this error for ret = 0? :S

[    0.736332] qcom-spm-cpuidle qcom-spm-cpuidle: Cannot register for CPU0: 0

A couple more lines in the UART log, then the device hangs forever.
For testing I changed this to: if (ret && ret != -ENODEV).

> [...]
> @@ -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.)

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.

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).

> [...]
> 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. :/

Thanks,
Stephan

Powered by blists - more mailing lists