Date:   Mon, 21 Jun 2021 13:54:00 +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: [PATCH v5 1/3] cpuidle: qcom_spm: Detach state machine from main
 SPM handling

On Sat, Jun 19, 2021 at 12:56:18AM +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;
> Though, it was ignored that the SPM driver is not used only
> on the ARM architecture.
> 

Can you please reword this sentence like I suggested in v4? The way you
write it at the moment it sounds like this fixes a regression, but
actually it extends the driver to cover more use cases.

> 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>
> ---
>  drivers/cpuidle/Kconfig.arm        |   1 +
>  drivers/cpuidle/cpuidle-qcom-spm.c | 295 ++++++-----------------------
>  drivers/soc/qcom/Kconfig           |   9 +
>  drivers/soc/qcom/Makefile          |   1 +
>  drivers/soc/qcom/spm.c             | 198 +++++++++++++++++++
>  include/soc/qcom/spm.h             |  43 +++++
>  6 files changed, 311 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
> [...]
> @@ -213,132 +80,88 @@ 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);
> +	cpu_node = of_cpu_device_node_get(cpu);
> +	if (!cpu_node)
> +		return -ENODEV;
>  
> -	/* Parse idle states from device tree */
> -	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 1);
> -	if (ret <= 0)
> -		return ret ? : -ENODEV;
> +	saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);
> +	if (!saw_node)
> +		return -ENODEV;
>  
> -	/* We have atleast one power down mode */
> -	return qcom_scm_set_warm_boot_addr(cpu_resume_arm, drv->cpumask);
> -}
> +	pdev = of_find_device_by_node(saw_node);
> +	of_node_put(saw_node);
> +	of_node_put(cpu_node);
> +	if (!pdev)
> +		return -ENODEV;
>  
> -static struct spm_driver_data *spm_get_drv(struct platform_device *pdev,
> -		int *spm_cpu)
> -{
> -	struct spm_driver_data *drv = NULL;
> -	struct device_node *cpu_node, *saw_node;
> -	int cpu;
> -	bool found = 0;
> +	spm = dev_get_drvdata(&pdev->dev);
> +	if (!spm)
> +		return -EINVAL;
>  
> -	for_each_possible_cpu(cpu) {
> -		cpu_node = of_cpu_device_node_get(cpu);
> -		if (!cpu_node)
> -			continue;
> -		saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);
> -		found = (saw_node == pdev->dev.of_node);
> -		of_node_put(saw_node);
> -		of_node_put(cpu_node);
> -		if (found)
> -			break;
> -	}
> +	spm->cpuidle_driver.cpumask = (struct cpumask *)cpumask_of(cpu);
> +	spm->cpuidle_driver = qcom_spm_idle_driver;

This should be in opposite order.

It's still freezing with this patch version. I used the chance to
investigate why it freezes, and it's actually not some bug of my
platform like you mentioned:

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

There is a simple reason why this happens: If "cpumask" is not set,
the cpuidle_driver applies to *all* CPUs. This means that as soon as
CPU1/2/3 go into standalone power collapse the SPM for CPU0 (rather than
the one for CPU1/2/3!) is re-configured. Eventually CPU0 will execute WFI
(without saving the CPU state) and ends up in power collapse (CPU state
destroyed) rather than standby (CPU state preserved).

> [...]
> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> [...]
> +static const u16 spm_reg_offset_v2_1[SPM_REG_NR] = {

This should be still u8 in this patch.

drivers/soc/qcom/spm.c:47:16: error: initialization of 'const u8 *' {aka 'const unsigned char *'} from incompatible pointer type 'const u16 *' {aka 'const short unsigned int *'} [-Werror=incompatible-pointer-types]
   47 |  .reg_offset = spm_reg_offset_v2_1,
      |                ^~~~~~~~~~~~~~~~~~~
drivers/soc/qcom/spm.c:47:16: note: (near initialization for 'spm_reg_8974_8084_cpu.reg_offset')

> [...]
> +static const u16 spm_reg_offset_v1_1[SPM_REG_NR] = {

drivers/soc/qcom/spm.c:68:16: error: initialization of 'const u8 *' {aka 'const unsigned char *'} from incompatible pointer type 'const u16 *' {aka 'const short unsigned int *'} [-Werror=incompatible-pointer-types]
   68 |  .reg_offset = spm_reg_offset_v1_1,
      |                ^~~~~~~~~~~~~~~~~~~
drivers/soc/qcom/spm.c:68:16: note: (near initialization for 'spm_reg_8064_cpu.reg_offset')

> [...]
> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
> new file mode 100644
> index 000000000000..719c604a8402
> --- /dev/null
> +++ b/include/soc/qcom/spm.h
> @@ -0,0 +1,43 @@
> +/* 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) 2021, AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
> + */
> +
> +#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 u8 *reg_offset;
> +	u32 spm_cfg;
> +	u32 spm_dly;
> +	u32 pmic_dly;
> +	u32 pmic_data[MAX_PMIC_DATA];
> +	u8 seq[MAX_SEQ_DATA];
> +	u8 start_index[PM_SLEEP_MODE_NR];
> +};
> +
> +struct spm_driver_data {
> +	struct cpuidle_driver cpuidle_driver;

Given that the SPM hardware seems to have several different uses,
not just CPUidle, wouldn't it be better to allocate the memory for the
cpuidle_driver from cpuidle-qcom-spm.c? Just devm_kzalloc() something
like:

struct spm_cpuidle_driver {
	struct cpuidle_driver cpuidle_driver;
	struct spm_driver_data *spm;
};

in cpuidle-qcom-spm.c.

And then there wouldn't be a need to have the implementation details of
the SPM driver in the spm.h header, all that cpuidle-qcom-spm needs is

struct spm_driver_data;

enum pm_sleep_mode {
	PM_SLEEP_MODE_STBY,
	PM_SLEEP_MODE_RET,
	PM_SLEEP_MODE_SPC,
	PM_SLEEP_MODE_PC,
	PM_SLEEP_MODE_NR,
};

void spm_set_low_power_mode(struct spm_driver_data *drv,
			    enum pm_sleep_mode mode);

Everything else can remain private to the spm.c driver,
like it was before.

Thanks,
Stephan

Powered by blists - more mailing lists