Oops, I thought I had sent this, but it was sitting in the drafts
folder. Sending anyways because it looks like most of these issues
still exist in v10.
Lina Iyer <lina.iyer@xxxxxxxxxx> writes:
> 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.
>
> Add support for an idle driver to set up the SPM to place the core in
> Standby or Standalone power collapse mode when the core is idle.
>
> Based on work by: Mahesh Sivasubramanian <msivasub@xxxxxxxxxxxxxx>,
> Ai Li <ali@xxxxxxxxxxxxxx>, Praveen Chidambaram <pchidamb@xxxxxxxxxxxxxx>
> Original tree available at -
> git://codeaurora.org/quic/la/kernel/msm-3.10.git
>
> Signed-off-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
A few coding-style, readablity comments below...
> ---
> .../devicetree/bindings/arm/msm/qcom,saw2.txt | 31 +-
> drivers/soc/qcom/Kconfig | 8 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/spm.c | 334 +++++++++++++++++++++
> include/soc/qcom/pm.h | 31 ++
> 5 files changed, 399 insertions(+), 6 deletions(-)
> create mode 100644 drivers/soc/qcom/spm.c
> create mode 100644 include/soc/qcom/pm.h
>
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> index 1505fb8..690c3c0 100644
> --- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> @@ -2,11 +2,20 @@ SPM AVS Wrapper 2 (SAW2)
>
> The SAW2 is a wrapper around the Subsystem Power Manager (SPM) and the
> Adaptive Voltage Scaling (AVS) hardware. The SPM is a programmable
> -micro-controller that transitions a piece of hardware (like a processor or
> +power-controller that transitions a piece of hardware (like a processor or
> subsystem) into and out of low power modes via a direct connection to
> the PMIC. It can also be wired up to interact with other processors in the
> system, notifying them when a low power state is entered or exited.
>
> +Multiple revisions of the SAW hardware are supported using these Device Nodes.
> +SAW2 revisions differ in the register offset and configuration data. Also, the
> +same revision of the SAW in different SoCs may have different configuration
> +data due the the differences in hardware capabilities. Hence the SoC name, the
> +version of the SAW hardware in that SoC and the distinction between cpu (big
> +or Little) or cache, may be needed to uniquely identify the SAW register
> +configuration and initialization data. The compatible string is used to
> +indicate this parameter.
> +
> PROPERTIES
>
> - compatible:
> @@ -14,10 +23,13 @@ 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,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"
>
> - reg:
> Usage: required
> @@ -26,10 +38,17 @@ PROPERTIES
> the register region. An optional second element specifies
> the base address and size of the alias register region.
>
> +- regulator:
> + Usage: optional
> + Value type: boolean
> + Definition: Indicates that this SPM device acts as a regulator device
> + device for the core (CPU or Cache) the SPM is attached
> + to.
>
> Example:
>
> - regulator@2099000 {
> + power-controller@2099000 {
> compatible = "qcom,saw2";
> reg = <0x02099000 0x1000>, <0x02009000 0x1000>;
> + regulator;
> };
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 7dcd554..012fb37 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 ARCH_QCOM
> + 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..ee2e3ca
> --- /dev/null
> +++ b/drivers/soc/qcom/spm.c
> @@ -0,0 +1,334 @@
> +/* 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/of_device.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpuidle.h>
> +#include <linux/cpu_pm.h>
> +
> +#include <asm/proc-fns.h>
> +#include <asm/suspend.h>
> +
> +#include <soc/qcom/pm.h>
> +#include <soc/qcom/pm.h>
> +#include <soc/qcom/scm.h>
> +#include <soc/qcom/scm-boot.h>
> +
> +
> +#define SCM_CMD_TERMINATE_PC (0x2)
> +#define SCM_FLUSH_FLAG_MASK (0x3)
> +#define SCM_L2_ON (0x0)
> +#define SCM_L2_OFF (0x1)
Why the parens? Also, other than the _MASK, should the others be using
BIT() ?
> +#define MAX_PMIC_DATA 2
> +#define MAX_SEQ_DATA 64
> +
> +#define SPM_CTL_INDEX 0x7f
> +#define SPM_CTL_INDEX_SHIFT 4
> +#define SPM_CTL_EN BIT(0)
> +
> +enum spm_reg {
> + SPM_REG_CFG,
> + SPM_REG_SPM_CTL,
> + SPM_REG_DLY,
> + SPM_REG_PMIC_DLY,
> + SPM_REG_PMIC_DATA_0,
> + SPM_REG_PMIC_DATA_1,
> + SPM_REG_VCTL,
> + SPM_REG_SEQ_ENTRY,
> + SPM_REG_SPM_STS,
> + SPM_REG_PMIC_STS,
> + SPM_REG_NR,
> +};
> +
> +struct spm_reg_data {
> + /* Register position */
comment doesn't add value over field name.
> + const u8 *reg_offset;
> +
> + /* Register initialization values */
> + u32 spm_cfg;
> + u32 spm_dly;
> + u32 pmic_dly;
> + u32 pmic_data[MAX_PMIC_DATA];
> +
> + /* Sequences and start indices */
ditto
> + u8 seq[MAX_SEQ_DATA];
> + u8 start_index[PM_SLEEP_MODE_NR];
> +
> +};
> +
> +struct spm_driver_data {
> + void __iomem *reg_base;
> + const struct spm_reg_data *reg_data;
> + bool available;
> +};
> +
> +static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
> + [SPM_REG_CFG] = 0x08,
> + [SPM_REG_SPM_CTL] = 0x30,
> + [SPM_REG_DLY] = 0x34,
> + [SPM_REG_SEQ_ENTRY] = 0x80,
> +};
> +
> +/* SPM register data for 8974, 8084 */
> +static const struct spm_reg_data spm_reg_8974_8084_cpu = {
> + .reg_offset = spm_reg_offset_v2_1,
> + .spm_cfg = 0x1,
> + .spm_dly = 0x3C102800,
> + .seq = { 0x03, 0x0B, 0x0F, 0x00, 0x20, 0x80, 0x10, 0xE8, 0x5B, 0x03,
> + 0x3B, 0xE8, 0x5B, 0x82, 0x10, 0x0B, 0x30, 0x06, 0x26, 0x30,
> + 0x0F },
> + .start_index[PM_SLEEP_MODE_STBY] = 0,
> + .start_index[PM_SLEEP_MODE_SPC] = 3,
> +};
> +
> +static const u8 spm_reg_offset_v1_1[SPM_REG_NR] = {
> + [SPM_REG_CFG] = 0x08,
> + [SPM_REG_SPM_CTL] = 0x20,
> + [SPM_REG_PMIC_DLY] = 0x24,
> + [SPM_REG_PMIC_DATA_0] = 0x28,
> + [SPM_REG_PMIC_DATA_1] = 0x2C,
> + [SPM_REG_SEQ_ENTRY] = 0x80,
> +};
> +
> +/* SPM register data for 8064 */
> +static const struct spm_reg_data spm_reg_8064_cpu = {
> + .reg_offset = spm_reg_offset_v1_1,
> + .spm_cfg = 0x1F,
> + .pmic_dly = 0x02020004,
> + .pmic_data[0] = 0x0084009C,
> + .pmic_data[1] = 0x00A4001C,
> + .seq = { 0x03, 0x0F, 0x00, 0x24, 0x54, 0x10, 0x09, 0x03, 0x01,
> + 0x10, 0x54, 0x30, 0x0C, 0x24, 0x30, 0x0F },
> + .start_index[PM_SLEEP_MODE_STBY] = 0,
> + .start_index[PM_SLEEP_MODE_SPC] = 2,
> +};
> +
> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct spm_driver_data, cpu_spm_drv);
> +
> +static inline void spm_register_write(struct spm_driver_data *drv,
> + enum spm_reg reg, u32 val)
> +{
> + if (drv->reg_data->reg_offset[reg])
> + writel_relaxed(val, drv->reg_base +
> + drv->reg_data->reg_offset[reg]);
> +}
> +
> +static inline u32 spm_register_read(struct spm_driver_data *drv,
> + enum spm_reg reg)
> +{
> + return readl_relaxed(drv->reg_base + drv->reg_data->reg_offset[reg]);
> +}
> +
> +/**
> + * 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 pm_sleep_mode mode)
> +{
> + struct spm_driver_data *drv = this_cpu_ptr(&cpu_spm_drv);
> + u32 start_index;
> + u32 ctl_val;
> +
> + if (!drv->available)
> + return -ENXIO;
> +
> + start_index = drv->reg_data->start_index[mode];
> +
> + ctl_val = spm_register_read(drv, SPM_REG_SPM_CTL);
> + ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT);
> + ctl_val |= start_index << SPM_CTL_INDEX_SHIFT;
> + ctl_val |= SPM_CTL_EN;
> + spm_register_write(drv, SPM_REG_SPM_CTL, ctl_val);
> +
> + /* Ensure we have written the start address */
> + wmb();
> +
> + return 0;
> +}
> +
You used kernel doc for the function above, but none of the others.
> +static int qcom_pm_collapse(unsigned long int unused)
> +{
> + int ret;
> + u32 flag;
> + int cpu = smp_processor_id();
> +
> + ret = scm_set_warm_boot_addr(cpu_resume, cpu);
> + if (ret) {
> + pr_err("Failed to set warm boot address for cpu %d\n", cpu);
> + return ret;
> + }
> +
> + flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
> + scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
> +
> + /**
And speaking of kerneldoc, this is the wrong multi-line comment style.
The double '*' is for kernel doc. (c.f. Documentation/kernel-doc-nano-HOWTO.txt)
There's a few of these elsewhere.
> + * Returns here only if there was a pending interrupt and we did not
> + * power down as a result.
> + */
> + return 0;
> +}
> +
> +static int qcom_cpu_standby(void *unused)
> +{
> + int ret;
> +
> + ret = qcom_spm_set_low_power_mode(PM_SLEEP_MODE_STBY);
> + if (ret)
> + return ret;
> +
> + cpu_do_idle();
> +
> + return 0;
> +}
> +
> +static int qcom_cpu_spc(void *unused)
> +{ int ret;
> +
> + ret = qcom_spm_set_low_power_mode(PM_SLEEP_MODE_STBY);
> + if (ret)
> + return ret;
> +
> + cpu_pm_enter();
> + cpu_suspend(0, qcom_pm_collapse);
> + cpu_pm_exit();
> +
> + return 0;
> +}
> +
> +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;
> +
> + 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);
> + if (saw_node) {
> + if (saw_node == pdev->dev.of_node)
> + drv = &per_cpu(cpu_spm_drv, cpu);
> + of_node_put(saw_node);
> + }
> + of_node_put(cpu_node);
> + if (drv) {
> + *spm_cpu = cpu;
> + break;
> + }
> + }
> +
> + return drv;
> +}
> +
> +static const struct of_device_id spm_match_table[] = {
> + { .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 },
> + { .compatible = "qcom,apq8064-saw2-v1.1-cpu",
> + .data = &spm_reg_8064_cpu },
> + { },
> +};
> +
> +static struct qcom_cpu_pm_ops lpm_ops = {
> + .standby = qcom_cpu_standby,
> + .spc = qcom_cpu_spc,
> +};
> +
> +struct platform_device qcom_cpuidle_device = {
> + .name = "qcom_cpuidle",
> + .id = -1,
> + .dev.platform_data = &lpm_ops,
> +};
> +
> +static int spm_dev_probe(struct platform_device *pdev)
> +{
> + struct spm_driver_data *drv;
> + struct resource *res;
> + const struct of_device_id *match_id;
> + void __iomem *addr;
> + const u32 *seq_data;
> + int cpu = -EINVAL;
> + static bool cpuidle_drv_init;
> +
> + drv = spm_get_drv(pdev, &cpu);
> + if (!drv)
> + return -EINVAL;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(drv->reg_base))
> + return PTR_ERR(drv->reg_base);
> +
> + match_id = of_match_node(spm_match_table, pdev->dev.of_node);
> + if (!match_id)
> + return -ENODEV;
> +
> + drv->reg_data = match_id->data;
> + if (!drv->reg_data)
> + return -EINVAL;
> +
> + /* Write the SPM sequences, first.. */
drop the ','.
> + addr = drv->reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY];
> + seq_data = (const u32 *)drv->reg_data->seq;
> + __iowrite32_copy(addr, seq_data, ARRAY_SIZE(drv->reg_data->seq)/4);
> +
> + /* ..and then, the control registers.
Fix multi-line comment style.
> + * On some SoC's if the control registers are written first and if the
> + * CPU was held in reset, the reset signal could trigger the SPM state
> + * machine, before the sequences are completely written.
> + */
> + spm_register_write(drv, SPM_REG_CFG, drv->reg_data->spm_cfg);
> + spm_register_write(drv, SPM_REG_DLY, drv->reg_data->spm_dly);
> + spm_register_write(drv, SPM_REG_PMIC_DLY, drv->reg_data->pmic_dly);
> +
> + spm_register_write(drv, SPM_REG_PMIC_DATA_0,
> + drv->reg_data->pmic_data[0]);
> + spm_register_write(drv, SPM_REG_PMIC_DATA_1,
> + drv->reg_data->pmic_data[1]);
> +
> + /**
> + * Ensure all observers see the above register writes before the
> + * cpuidle driver is allowed to use the SPM.
> + */
> + wmb();
> + drv->available = true;
Others have already commented on this, but I'll add my $0.02 that this
suggest something is not right in the init sequence.
> + if ((cpu > -1) && !cpuidle_drv_init) {
> + platform_device_register(&qcom_cpuidle_device);
> + cpuidle_drv_init = true;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver spm_driver = {
> + .probe = spm_dev_probe,
> + .driver = {
> + .name = "qcom,spm",
> + .of_match_table = spm_match_table,
> + },
> +};
> +
> +module_platform_driver(spm_driver);
> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
> new file mode 100644
> index 0000000..d9a56d7
> --- /dev/null
> +++ b/include/soc/qcom/pm.h
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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_PM_H
> +#define __QCOM_PM_H
> +
> +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 qcom_cpu_pm_ops {
> + int (*standby)(void *data);
> + int (*spc)(void *data);
> +};
> +
> +#endif /* __QCOM_PM_H */
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
