Re: [PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

[Stephen Boyd <sboyd@codeaurora.org>]

On 09/26/14 17:58, Lina Iyer wrote:
> Based on work by many authors, available at codeaurora.org
>
> SPM is a hardware block that controls the peripheral logic surrounding
> the application cores (cpu/l$). When the core executes WFI instruction,
> the SPM takes over the putting the core in low power state as
> configured. The wake up for the SPM is an interrupt at the GIC, which
> then completes the rest of low power mode sequence and brings the core
> out of low power mode.
>
> The SPM has a set of control registers that configure the SPMs
> individually based on the type of the core and the runtime conditions.
> SPM is a finite state machine block to which a sequence is provided and
> it interprets the bytes  and executes them in sequence. Each low power
> mode that the core can enter into is provided to the SPM as a sequence.
>
> Configure the SPM to set the core (cpu or L2) into its low power mode,
> the index of the first command in the sequence is set in the SPM_CTL
> register. When the core executes ARM wfi instruction, it triggers the
> SPM state machine to start executing from that index. The SPM state
> machine waits until the interrupt occurs and starts executing the rest
> of the sequence until it hits the end of the sequence. The end of the
> sequence jumps the core out of its low power mode.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> [lina: simplify the driver for initial submission, clean up and update
> commit text]
> ---
>  .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  10 +-
>  drivers/soc/qcom/Kconfig                           |   8 +
>  drivers/soc/qcom/Makefile                          |   1 +
>  drivers/soc/qcom/spm.c                             | 216 +++++++++++++++++++++
>  include/soc/qcom/spm.h                             |  35 ++++
>  5 files changed, 264 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/soc/qcom/spm.c
>  create mode 100644 include/soc/qcom/spm.h
>
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> index 1505fb8..9a9cc99 100644
> --- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> @@ -14,10 +14,9 @@ PROPERTIES
>  	Value type: <string>
>  	Definition: shall contain "qcom,saw2". A more specific value should be
>  		    one of:
> -			 "qcom,saw2-v1"
> -			 "qcom,saw2-v1.1"
> -			 "qcom,saw2-v2"
> -			 "qcom,saw2-v2.1"
> +			 "qcom,apq8064-saw2-v1.1-cpu"
> +			 "qcom,msm8974-saw2-v2.1-cpu"
> +			 "qcom,apq8084-saw2-v2.1-cpu"

It's probably not good to remove the old compatibles. Just add more to
the list. Please Cc dt reviewers on dt bindings.

>  
>  - reg:
>  	Usage: required
> @@ -26,10 +25,9 @@ PROPERTIES
>  		    the register region. An optional second element specifies
>  		    the base address and size of the alias register region.
>  
> -
>  Example:
>  
> -	regulator@2099000 {
> +	saw@2099000 {

saw is not a standard thing. Hence the usage of regulator here. I agree
when it doesn't directly control a regulator then it should be called
something else, power-controller perhaps? I don't really see a need to
change this example though.

>  		compatible = "qcom,saw2";
>  		reg = <0x02099000 0x1000>, <0x02009000 0x1000>;
>  	};
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 7dcd554..cd249c4 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -11,3 +11,11 @@ config QCOM_GSBI
>  
>  config QCOM_SCM
>  	bool
> +
> +config QCOM_PM
> +	bool "Qualcomm Power Management"
> +	depends on PM && ARCH_QCOM

Drop the PM dependency. There isn't any right? Honestly we don't want
this type of option at all. We want an option for each driver introduced.

> +	help
> +	  QCOM Platform specific power driver to manage cores and L2 low power
> +	  modes. It interface with various system drivers to put the cores in
> +	  low power modes.
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 70d52ed..20b329f 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
> +obj-$(CONFIG_QCOM_PM)	+=	spm.o
>  CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
>  obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o
> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> new file mode 100644
> index 0000000..0ba7949
> --- /dev/null
> +++ b/drivers/soc/qcom/spm.c
> @@ -0,0 +1,216 @@
> +/* Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +
> +#include <soc/qcom/spm.h>
> +
> +enum {
> +	SPM_REG_CFG,
> +	SPM_REG_SPM_CTL,
> +	SPM_REG_DLY,
> +	SPM_REG_PMIC_DATA_0,
> +	SPM_REG_PMIC_DATA_1,
> +	SPM_REG_PMIC_DATA_2,
> +	SPM_REG_PMIC_DATA_3,
> +	SPM_REG_VCTL,
> +	SPM_REG_SEQ_ENTRY_0,
> +	SPM_REG_SEQ_ENTRY_1,
> +	SPM_REG_SEQ_ENTRY_2,
> +	SPM_REG_SEQ_ENTRY_3,
> +	SPM_REG_SEQ_ENTRY_4,
> +	SPM_REG_SEQ_ENTRY_5,
> +	SPM_REG_SEQ_ENTRY_6,
> +	SPM_REG_SEQ_ENTRY_7,
> +	SPM_REG_SEQ_ENTRY_LAST = SPM_REG_SEQ_ENTRY_7,
> +	SPM_REG_SPM_STS,
> +	SPM_REG_PMIC_STS,
> +	SPM_REG_NR,
> +};
> +
> +struct spm_reg_data {
> +	/* Register position and initialization value */
> +	struct register_info {
> +		u8 offset;
> +		u32 value;
> +	} reg[SPM_REG_NR];
> +
> +	/* Start address offset for the supported idle states*/
> +	u8 start_addr[SPM_MODE_NR];
> +};
> +
> +struct spm_driver_data {
> +	void __iomem *reg_base_addr;
> +	const struct spm_reg_data *reg_data;
> +};
> +
> +/* SPM register data for 8974, 8084 */
> +static const struct spm_reg_data spm_reg_8974_8084_cpu  = {
> +	.reg[SPM_REG_CFG]		= {0x08, 0x1},
> +	.reg[SPM_REG_SPM_STS]		= {0x0C, 0x0},
> +	.reg[SPM_REG_PMIC_STS]		= {0x14, 0x0},
> +	.reg[SPM_REG_VCTL]		= {0x1C, 0x0},
> +	.reg[SPM_REG_SPM_CTL]		= {0x30, 0x1},
> +	.reg[SPM_REG_DLY]		= {0x34, 0x3C102800},
> +	.reg[SPM_REG_PMIC_DATA_0]	= {0x40, 0x0},
> +	.reg[SPM_REG_PMIC_DATA_1]	= {0x44, 0x0},
> +	.reg[SPM_REG_PMIC_DATA_2]	= {0x48, 0x0},
> +	.reg[SPM_REG_SEQ_ENTRY_0]	= {0x80, 0x000F0B03},
> +	.reg[SPM_REG_SEQ_ENTRY_1]	= {0x84, 0xE8108020},
> +	.reg[SPM_REG_SEQ_ENTRY_2]	= {0x88, 0xE83B035B},
> +	.reg[SPM_REG_SEQ_ENTRY_3]	= {0x8C, 0x300B1082},
> +	.reg[SPM_REG_SEQ_ENTRY_4]	= {0x90, 0x0F302606},

Is this endian agnostic? We don't need this initial value table. The
only thing that really is different is delay and seq entries. The seq
entries can be a byte array that gets written to the device in an endian
agnostic fashion and the delay can be a different struct member. The
register map can be per version of the spm (i.e. not per-soc) and that
can be pointed to by the SoC data.

I really don't like setting the SPM_CTL register's enable bit to 1 with
this table either. That should be done explicitly because it isn't
"configuration" like the delays or the sequences are. It's a bit that
will have some effect. It probably even needs to be cleared if we're
reprogramming the SPM sequence in a scenario like kexec where the bit
may already be set.

> +	.reg[SPM_REG_SEQ_ENTRY_5]	= {0x94, 0x0},
> +	.reg[SPM_REG_SEQ_ENTRY_6]	= {0x98, 0x0},
> +	.reg[SPM_REG_SEQ_ENTRY_7]	= {0x9C, 0x0},
> +
> +	.start_addr[SPM_MODE_CLOCK_GATING]	= 0,
> +	.start_addr[SPM_MODE_POWER_COLLAPSE]	= 3,
> +};
> +
> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct spm_driver_data, cpu_spm_drv);
> +
> +/**
> + * spm_set_low_power_mode() - Configure SPM start address for low power mode
> + * @mode: SPM LPM mode to enter
> + */
> +int qcom_spm_set_low_power_mode(enum spm_mode mode)
> +{
> +	struct spm_driver_data *drv = &__get_cpu_var(cpu_spm_drv);
> +	u32 start_addr = 0;

Unnecessary assignment.
 
> +	u32 ctl_val;
> +
> +	if (!drv || !drv->reg_data)
> +		return -ENXIO;

Does this ever happen? Please remove.

> +
> +	start_addr = drv->reg_data->start_addr[mode];
> +
> +	/* Update bits 10:4 in the SPM CTL register */

This comment provides nothing that isn't evident from the code. Remove.

> +	ctl_val = readl_relaxed(drv->reg_base_addr +
> +				drv->reg_data->reg[SPM_REG_SPM_CTL].offset);
> +	start_addr &= 0x7F;
> +	start_addr <<= 4;
> +	ctl_val &= 0xFFFFF80F;
> +	ctl_val |= start_addr;
> +	writel_relaxed(ctl_val, drv->reg_base_addr +
> +				drv->reg_data->reg[SPM_REG_SPM_CTL].offset);
> +	/* Ensure we have written the start address */
> +	wmb();
> +
> +	return 0;
> +}
> +
> +static struct spm_driver_data *spm_get_drv(struct platform_device *pdev)
> +{
> +	struct spm_driver_data *drv = NULL;
> +	struct device_node *cpu_node, *saw_node;
> +	u32 cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_node = of_get_cpu_node(cpu, NULL);
> +		if (!cpu_node)
> +			continue;
> +		saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);
> +		if (!saw_node)
> +			continue;
> +		if (saw_node == pdev->dev.of_node) {
> +			drv = &per_cpu(cpu_spm_drv, cpu);
> +			break;
> +		}

Missing a couple of_node_put()s.

> +	}
> +
> +	return drv;
> +}
> +
> +static const struct of_device_id spm_match_table[] __initconst = {
> +	{ .compatible = "qcom,msm8974-saw2-v2.1-cpu",
> +	  .data = &spm_reg_8974_8084_cpu },
> +	{ .compatible = "qcom,apq8084-saw2-v2.1-cpu",
> +	  .data = &spm_reg_8974_8084_cpu },
> +	{ },
> +};
> +
> +static int spm_dev_probe(struct platform_device *pdev)
> +{
> +	struct spm_driver_data *drv;
> +	struct resource *res;
> +	const struct of_device_id *match_id;
> +	int i;
> +
> +	 /* Get the right SPM device */
> +	drv = spm_get_drv(pdev);
> +	if (!drv)
> +		return -EINVAL;
> +
> +	/* Get the SPM start address */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	drv->reg_base_addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(drv->reg_base_addr))
> +		return PTR_ERR(drv->reg_base_addr);
> +
> +	match_id = of_match_node(spm_match_table, pdev->dev.of_node);
> +	if (!match_id)
> +		return -ENODEV;
> +
> +	/* Get the SPM register data for this instance */
> +	drv->reg_data = match_id->data;
> +	if (!drv->reg_data)
> +		return -EINVAL;
> +
> +	/* Write the SPM sequences first */
> +	for (i = SPM_REG_SEQ_ENTRY_0; i <= SPM_REG_SEQ_ENTRY_LAST; i++)
> +		writel_relaxed(drv->reg_data->reg[i].value,
> +			drv->reg_base_addr + drv->reg_data->reg[i].offset);
> +
> +	/* Write the SPM control registers */
> +	writel_relaxed(drv->reg_data->reg[SPM_REG_DLY].value,
> +		drv->reg_base_addr + drv->reg_data->reg[SPM_REG_DLY].offset);
> +
> +	writel_relaxed(drv->reg_data->reg[SPM_REG_CFG].value,
> +		drv->reg_base_addr + drv->reg_data->reg[SPM_REG_CFG].offset);
> +
> +	writel_relaxed(drv->reg_data->reg[SPM_REG_SPM_CTL].value,
> +			drv->reg_base_addr +
> +			drv->reg_data->reg[SPM_REG_SPM_CTL].offset);
> +
> +	/**
> +	 * Ensure all observers see the above register writes before the
> +	 * cpuidle driver is allowed to use the SPM.
> +	 */
> +	wmb();
> +
> +	return 0;
> +}
> +
> +static struct platform_driver spm_driver = {
> +	.probe = spm_dev_probe,
> +	.driver = {
> +		.name = "spm",

qcom-spm?

> +		.of_match_table = spm_match_table,
> +	},
> +};
> +
> +static int __init spm_driver_init(void)
> +{
> +	return platform_driver_register(&spm_driver);
> +}
> +device_initcall(spm_driver_init);

Why can't we support modules?

> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
> new file mode 100644
> index 0000000..997abfc
> --- /dev/null
> +++ b/include/soc/qcom/spm.h
> @@ -0,0 +1,35 @@
> +/* Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __QCOM_SPM_H
> +#define __QCOM_SPM_H
> +
> +enum spm_mode {
> +	SPM_MODE_CLOCK_GATING,
> +	SPM_MODE_RETENTION,
> +	SPM_MODE_GDHS,
> +	SPM_MODE_POWER_COLLAPSE,
> +	SPM_MODE_NR
> +};
> +
> +#if defined(CONFIG_QCOM_PM)
> +
> +int qcom_spm_set_low_power_mode(enum spm_mode mode);
> +
> +#else
> +
> +static inline int qcom_spm_set_low_power_mode(enum spm_mode mode)
> +{ return -ENOSYS; }
> +
> +#endif  /* CONFIG_QCOM_PM */
> +
> +#endif  /* __QCOM_SPM_H */

It would be nice to not have this file.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation