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@xxxxxxxxxx>
> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
