[v6,1/5] qcom: spm: Add Subsystem Power Manager driver

Commit Message

Lina Iyer Sept. 23, 2014, 11:51 p.m. UTC

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 <[e-mail]>
[lina: simplify the driver for initial submission, clean up and update
commit text]
---
 Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
 drivers/soc/qcom/Kconfig                          |   8 +
 drivers/soc/qcom/Makefile                         |   1 +
 drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
 include/soc/qcom/spm.h                            |  38 +++
 5 files changed, 478 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
 create mode 100644 drivers/soc/qcom/spm.c
 create mode 100644 include/soc/qcom/spm.h

Comments

Lina Iyer Sept. 24, 2014, 1:58 a.m. UTC | #1

On Tue, Sep 23 2014 at 17:51 -0600, 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 <[e-mail]>
>[lina: simplify the driver for initial submission, clean up and update
>commit text]
>---
> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
> drivers/soc/qcom/Kconfig                          |   8 +
> drivers/soc/qcom/Makefile                         |   1 +
> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
> include/soc/qcom/spm.h                            |  38 +++
> 5 files changed, 478 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
> create mode 100644 drivers/soc/qcom/spm.c
> create mode 100644 include/soc/qcom/spm.h
>
>diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
>new file mode 100644
>index 0000000..2ff2454
>--- /dev/null
>+++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>@@ -0,0 +1,43 @@
>+* Subsystem Power Manager (SPM)
>+
>+Qualcomm Snapdragons have SPM hardware blocks to control the Application
>+Processor Sub-System power. These SPM blocks run individual state machine
>+to determine what the core (L2 or Krait/Scorpion) would do when the WFI
>+instruction is executed by the core.
>+
>+The devicetree representation of the SPM block should be:
>+
>+Required properties
>+
>+- compatible: Must be -
>+		 "qcom,spm-v2.1"
>+- reg: The physical address and the size of the SPM's memory mapped registers
>+- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>+	This field is required on only for SPMs that control the CPU.
>+- qcom,saw2-clk-div: SAW2 configuration register to program the SPM runtime
>+	clocks. The register for this property is MSM_SPM_REG_SAW2_CFG.
>+- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
>+	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.
>+- qcom,saw2-enable: The SPM control register to enable/disable the sleep state
>+	machine. The register for this property is MSM_SPM_REG_SAW2_SPM_CTL.
>+
>+Optional properties
>+
>+- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>+- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>+
>+Example:
>+	spm@f9089000 {
>+		compatible = "qcom,spm-v2.1";
>+		#address-cells = <1>;
>+		#size-cells = <1>;
>+		reg = <0xf9089000 0x1000>;
>+		qcom,cpu = <&CPU0>;
>+		qcom,saw2-clk-div = <0x1>;
>+		qcom,saw2-delays = <0x20000400>;
>+		qcom,saw2-enable = <0x1>;
>+		qcom,saw2-spm-cmd-wfi = [03 0b 0f];
>+		qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
>+				a0 b0 03 68 70 3b 92 a0 b0
>+				82 2b 50 10 30 02 22 30 0f];
>+	};
>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
>+	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..1fa6a96
>--- /dev/null
>+++ b/drivers/soc/qcom/spm.c
>@@ -0,0 +1,388 @@
>+/* 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>
>+
>+#define NUM_SEQ_ENTRY 32
>+#define SPM_CTL_ENABLE BIT(0)
>+
>+enum {
>+	MSM_SPM_REG_SAW2_CFG,
>+	MSM_SPM_REG_SAW2_AVS_CTL,
>+	MSM_SPM_REG_SAW2_AVS_HYSTERESIS,
>+	MSM_SPM_REG_SAW2_SPM_CTL,
>+	MSM_SPM_REG_SAW2_PMIC_DLY,
>+	MSM_SPM_REG_SAW2_AVS_LIMIT,
>+	MSM_SPM_REG_SAW2_AVS_DLY,
>+	MSM_SPM_REG_SAW2_SPM_DLY,
>+	MSM_SPM_REG_SAW2_PMIC_DATA_0,
>+	MSM_SPM_REG_SAW2_PMIC_DATA_1,
>+	MSM_SPM_REG_SAW2_PMIC_DATA_2,
>+	MSM_SPM_REG_SAW2_PMIC_DATA_3,
>+	MSM_SPM_REG_SAW2_PMIC_DATA_4,
>+	MSM_SPM_REG_SAW2_PMIC_DATA_5,
>+	MSM_SPM_REG_SAW2_PMIC_DATA_6,
>+	MSM_SPM_REG_SAW2_PMIC_DATA_7,
>+	MSM_SPM_REG_SAW2_RST,
>+
>+	MSM_SPM_REG_NR_INITIALIZE = MSM_SPM_REG_SAW2_RST,
>+
>+	MSM_SPM_REG_SAW2_ID,
>+	MSM_SPM_REG_SAW2_SECURE,
>+	MSM_SPM_REG_SAW2_STS0,
>+	MSM_SPM_REG_SAW2_STS1,
>+	MSM_SPM_REG_SAW2_STS2,
>+	MSM_SPM_REG_SAW2_VCTL,
>+	MSM_SPM_REG_SAW2_SEQ_ENTRY,
>+	MSM_SPM_REG_SAW2_SPM_STS,
>+	MSM_SPM_REG_SAW2_AVS_STS,
>+	MSM_SPM_REG_SAW2_PMIC_STS,
>+	MSM_SPM_REG_SAW2_VERSION,
>+
>+	MSM_SPM_REG_NR,
>+};
>+
>+static u32 reg_offsets_saw2_v2_1[MSM_SPM_REG_NR] = {
>+	[MSM_SPM_REG_SAW2_SECURE]		= 0x00,
>+	[MSM_SPM_REG_SAW2_ID]			= 0x04,
>+	[MSM_SPM_REG_SAW2_CFG]			= 0x08,
>+	[MSM_SPM_REG_SAW2_SPM_STS]		= 0x0C,
>+	[MSM_SPM_REG_SAW2_AVS_STS]		= 0x10,
>+	[MSM_SPM_REG_SAW2_PMIC_STS]		= 0x14,
>+	[MSM_SPM_REG_SAW2_RST]			= 0x18,
>+	[MSM_SPM_REG_SAW2_VCTL]			= 0x1C,
>+	[MSM_SPM_REG_SAW2_AVS_CTL]		= 0x20,
>+	[MSM_SPM_REG_SAW2_AVS_LIMIT]		= 0x24,
>+	[MSM_SPM_REG_SAW2_AVS_DLY]		= 0x28,
>+	[MSM_SPM_REG_SAW2_AVS_HYSTERESIS]	= 0x2C,
>+	[MSM_SPM_REG_SAW2_SPM_CTL]		= 0x30,
>+	[MSM_SPM_REG_SAW2_SPM_DLY]		= 0x34,
>+	[MSM_SPM_REG_SAW2_PMIC_DATA_0]		= 0x40,
>+	[MSM_SPM_REG_SAW2_PMIC_DATA_1]		= 0x44,
>+	[MSM_SPM_REG_SAW2_PMIC_DATA_2]		= 0x48,
>+	[MSM_SPM_REG_SAW2_PMIC_DATA_3]		= 0x4C,
>+	[MSM_SPM_REG_SAW2_PMIC_DATA_4]		= 0x50,
>+	[MSM_SPM_REG_SAW2_PMIC_DATA_5]		= 0x54,
>+	[MSM_SPM_REG_SAW2_PMIC_DATA_6]		= 0x58,
>+	[MSM_SPM_REG_SAW2_PMIC_DATA_7]		= 0x5C,
>+	[MSM_SPM_REG_SAW2_SEQ_ENTRY]		= 0x80,
>+	[MSM_SPM_REG_SAW2_VERSION]		= 0xFD0,
>+};
I should probably remove the registers that we would not ever read/write
in this driver.

>+
>+struct spm_of {
>+	char *key;
>+	u32 id;
>+};
>+
>+struct msm_spm_mode {
>+	u32 mode;
>+	u32 start_addr;
>+};
>+
>+struct msm_spm_driver_data {
>+	void __iomem *reg_base_addr;
>+	u32 *reg_offsets;
>+	struct msm_spm_mode *modes;
>+	u32 num_modes;
>+};
>+
>+struct msm_spm_device {
>+	bool initialized;
>+	struct msm_spm_driver_data drv;
>+};
>+
>+static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);
>+
>+static const struct of_device_id msm_spm_match_table[] __initconst;
>+
>+static int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
>+						u32 mode)
>+{
>+	int i;
>+	u32 start_addr = 0;
>+	u32 ctl_val;
>+
>+	for (i = 0; i < drv->num_modes; i++) {
>+		if (drv->modes[i].mode == mode) {
>+			start_addr = drv->modes[i].start_addr;
>+			break;
>+		}
>+	}
>+
>+	if (i == drv->num_modes)
>+		return -EINVAL;
>+
>+	/* Update bits 10:4 in the SPM CTL register */
>+	ctl_val = readl_relaxed(drv->reg_base_addr +
>+			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>+	start_addr &= 0x7F;
>+	start_addr <<= 4;
>+	ctl_val &= 0xFFFFF80F;
>+	ctl_val |= start_addr;
>+	writel_relaxed(ctl_val, drv->reg_base_addr +
>+			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>+	/* Ensure we have written the start address */
>+	wmb();
>+
>+	return 0;
>+}
>+
>+static int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv,
>+					bool enable)
>+{
>+	u32 value = enable ? 0x01 : 0x00;
>+	u32 ctl_val;
>+
>+	ctl_val = readl_relaxed(drv->reg_base_addr +
>+			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>+
>+	/* Update SPM_CTL to enable/disable the SPM */
>+	if ((ctl_val & SPM_CTL_ENABLE) != value) {
>+		/* Clear the existing value and update */
>+		ctl_val &= ~0x1;
>+		ctl_val |= value;
>+		writel_relaxed(ctl_val, drv->reg_base_addr +
>+			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>+
>+		/* Ensure we have enabled/disabled before returning */
>+		wmb();
>+	}
>+
>+	return 0;
>+}
>+
>+/**
>+ * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
>+ * @mode: SPM LPM mode to enter
>+ */
>+int msm_spm_set_low_power_mode(u32 mode)
>+{
>+	struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
>+	int ret = -EINVAL;
>+
>+	if (!dev->initialized)
>+		return -ENXIO;
>+
>+	if (mode == MSM_SPM_MODE_DISABLED)
>+		ret = msm_spm_drv_set_spm_enable(&dev->drv, false);
>+	else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
>+		ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
>+
>+	return ret;
>+}
>+EXPORT_SYMBOL(msm_spm_set_low_power_mode);
>+
>+static void append_seq_data(u32 *reg_seq_entry, u8 *cmd, u32 *offset)
>+{
>+	u32 cmd_w;
>+	u32 offset_w = *offset / 4;
>+	u8 last_cmd;
>+
>+	while (1) {
>+		int i;
>+
>+		cmd_w = 0;
>+		last_cmd = 0;
>+		cmd_w = reg_seq_entry[offset_w];
>+
>+		for (i = (*offset % 4); i < 4; i++) {
>+			last_cmd = *(cmd++);
>+			cmd_w |=  last_cmd << (i * 8);
>+			(*offset)++;
>+			if (last_cmd == 0x0f)
>+				break;
>+		}
>+
>+		reg_seq_entry[offset_w++] = cmd_w;
>+		if (last_cmd == 0x0f)
>+			break;
>+	}
>+}
>+
>+static int msm_spm_seq_init(struct msm_spm_device *spm_dev,
>+			struct platform_device *pdev)
>+{
>+	int i;
>+	u8 *cmd;
>+	void *addr;
>+	u32 val;
>+	u32 count = 0;
>+	int offset = 0;
>+	struct msm_spm_mode modes[MSM_SPM_MODE_NR];
>+	u32 sequences[NUM_SEQ_ENTRY/4] = {0};
>+	struct msm_spm_driver_data *drv = &spm_dev->drv;
>+
>+	/* SPM sleep sequences */
>+	struct spm_of mode_of_data[] = {
>+		{"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
>+		{"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
>+	};
>+
>+	/**
>+	 * Compose the u32 array based on the individual bytes of the SPM
>+	 * sequence for each low power mode that we read from the DT.
>+	 * The sequences are appended if there is space available in the
>+	 * u32 after the end of the previous sequence.
>+	 */
>+
>+	for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
>+		cmd = (u8 *)of_get_property(pdev->dev.of_node,
>+						mode_of_data[i].key, &val);
>+		if (!cmd)
>+			continue;
>+		/* The last in the sequence should be 0x0F */
>+		if (cmd[val - 1] != 0x0F)
>+			continue;
>+		modes[count].mode = mode_of_data[i].id;
>+		modes[count].start_addr = offset;
>+		append_seq_data(&sequences[0], cmd, &offset);
>+		count++;
>+	}
>+
>+	/* Write the idle state sequences to SPM */
>+	drv->modes = devm_kcalloc(&pdev->dev, count,
>+					sizeof(modes[0]), GFP_KERNEL);
>+	if (!drv->modes)
>+		return -ENOMEM;
>+
>+	drv->num_modes = count;
>+	memcpy(drv->modes, modes, sizeof(modes[0]) * count);
>+
>+	/* Flush the integer array */
>+	addr = drv->reg_base_addr +
>+			drv->reg_offsets[MSM_SPM_REG_SAW2_SEQ_ENTRY];
>+	for (i = 0; i < ARRAY_SIZE(sequences); i++, addr += 4)
>+		writel_relaxed(sequences[i], addr);
>+
>+	/* Ensure we flush the writes */
>+	wmb();
>+
>+	return 0;
>+}
>+
>+static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
>+{
>+	struct msm_spm_device *dev = NULL;
>+	struct device_node *cpu_node;
>+	u32 cpu;
>+
>+	cpu_node = of_parse_phandle(pdev->dev.of_node, "qcom,cpu", 0);
>+	if (cpu_node) {
>+		for_each_possible_cpu(cpu) {
>+			if (of_get_cpu_node(cpu, NULL) == cpu_node)
>+				dev = &per_cpu(msm_cpu_spm_device, cpu);
>+		}
>+	}
>+
>+	return dev;
>+}
>+
>+static int msm_spm_dev_probe(struct platform_device *pdev)
>+{
>+	int ret;
>+	int i;
>+	u32 val;
>+	struct msm_spm_device *spm_dev;
>+	struct resource *res;
>+	const struct of_device_id *match_id;
>+
>+	/* SPM Configuration registers */
>+	struct spm_of spm_of_data[] = {
>+		{"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
>+		{"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
>+		{"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
>+	};
>+
>+	 /* Get the right SPM device */
>+	spm_dev = msm_spm_get_device(pdev);
>+	if (IS_ERR_OR_NULL(spm_dev))
>+		return -EINVAL;
>+
>+	/* Get the SPM start address */
>+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>+	if (!res) {
>+		ret = -EINVAL;
>+		goto fail;
>+	}
>+	spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
>+					resource_size(res));
>+	if (!spm_dev->drv.reg_base_addr) {
>+		ret = -ENOMEM;
>+		goto fail;
>+	}
>+
>+	match_id = of_match_node(msm_spm_match_table, pdev->dev.of_node);
>+	if (!match_id)
>+		return -ENODEV;
>+
>+	/* Use the register offsets for the SPM version in use */
>+	spm_dev->drv.reg_offsets = (u32 *)match_id->data;
>+	if (!spm_dev->drv.reg_offsets)
>+		return -EFAULT;
>+
>+	/* Read the SPM idle state sequences */
>+	ret = msm_spm_seq_init(spm_dev, pdev);
>+	if (ret)
>+		return ret;
>+
>+	/* Read the SPM register values */
>+	for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
>+		ret = of_property_read_u32(pdev->dev.of_node,
>+					spm_of_data[i].key, &val);
>+		if (ret)
>+			continue;
>+		writel_relaxed(val, spm_dev->drv.reg_base_addr +
>+				spm_dev->drv.reg_offsets[spm_of_data[i].id]);
>+	}
>+
>+	/* Flush all writes */
>+	wmb();
>+
>+	spm_dev->initialized = true;
>+	return ret;
>+fail:
>+	dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
>+	return ret;
>+}
>+
>+static const struct of_device_id msm_spm_match_table[] __initconst = {
>+	{.compatible = "qcom,spm-v2.1", .data = reg_offsets_saw2_v2_1},
>+	{ },
>+};
>+
>+
>+static struct platform_driver msm_spm_device_driver = {
>+	.probe = msm_spm_dev_probe,
>+	.driver = {
>+		.name = "spm",
>+		.owner = THIS_MODULE,
>+		.of_match_table = msm_spm_match_table,
>+	},
>+};
>+
>+static int __init msm_spm_device_init(void)
>+{
>+	return platform_driver_register(&msm_spm_device_driver);
>+}
>+device_initcall(msm_spm_device_init);
>diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
>new file mode 100644
>index 0000000..29686ef
>--- /dev/null
>+++ b/include/soc/qcom/spm.h
>@@ -0,0 +1,38 @@
>+/* 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 {
>+	MSM_SPM_MODE_DISABLED,
>+	MSM_SPM_MODE_CLOCK_GATING,
>+	MSM_SPM_MODE_RETENTION,
>+	MSM_SPM_MODE_GDHS,
>+	MSM_SPM_MODE_POWER_COLLAPSE,
>+	MSM_SPM_MODE_NR
>+};
>+
>+struct msm_spm_device;
>+
>+#if defined(CONFIG_QCOM_PM)
>+
>+int msm_spm_set_low_power_mode(u32 mode);
>+
>+#else
>+
>+static inline int msm_spm_set_low_power_mode(u32 mode)
>+{ return -ENOSYS; }
>+
>+#endif  /* CONFIG_QCOM_PM */
>+
>+#endif  /* __QCOM_SPM_H */
>-- 
>1.9.1
>

Kumar Gala Sept. 24, 2014, 4:33 p.m. UTC | #2

On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> 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 <[e-mail]>
> [lina: simplify the driver for initial submission, clean up and update
> commit text]
> ---
> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
> drivers/soc/qcom/Kconfig                          |   8 +
> drivers/soc/qcom/Makefile                         |   1 +
> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
> include/soc/qcom/spm.h                            |  38 +++
> 5 files changed, 478 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
> create mode 100644 drivers/soc/qcom/spm.c
> create mode 100644 include/soc/qcom/spm.h
> 
> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
> new file mode 100644
> index 0000000..2ff2454
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
> @@ -0,0 +1,43 @@
> +* Subsystem Power Manager (SPM)
> +
> +Qualcomm Snapdragons have SPM hardware blocks to control the Application
> +Processor Sub-System power. These SPM blocks run individual state machine
> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI
> +instruction is executed by the core.
> +
> +The devicetree representation of the SPM block should be:
> +
> +Required properties
> +
> +- compatible: Must be -
> +		 "qcom,spm-v2.1"
> +- reg: The physical address and the size of the SPM's memory mapped registers
> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
> +	This field is required on only for SPMs that control the CPU.

Let’s make this just cpu-handle instead of qcom,cpu.  The concept of a handle to a cpu is pretty generic.

> +- qcom,saw2-clk-div: SAW2 configuration register to program the SPM runtime
> +	clocks. The register for this property is MSM_SPM_REG_SAW2_CFG.

(add details on how this is used to compute timer tick.  Is it timer tick = saw_clk/saw2-clk-div?  What is valid range of values)

> +- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
> +	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.

Didn’t Stephen asked about splitting this up? Or at least treating it as an array of 3 values?

> +- qcom,saw2-enable: The SPM control register to enable/disable the sleep state
> +	machine. The register for this property is MSM_SPM_REG_SAW2_SPM_CTL.

Can this just be a boolean (exist or not), if so, probably change it to qcom,saw2-disable (so lack of property means enable)?

> +
> +Optional properties
> +
> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence

probably add something like: “array of bytes …” (want to convey the data type somehow, is there a max length?)

> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence

probably add something like: “array of bytes …” (want to convey the data type somehow, is there a max length?)

> +
> +Example:
> +	spm@f9089000 {
> +		compatible = "qcom,spm-v2.1";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0xf9089000 0x1000>;
> +		qcom,cpu = <&CPU0>;
> +		qcom,saw2-clk-div = <0x1>;
> +		qcom,saw2-delays = <0x20000400>;
> +		qcom,saw2-enable = <0x1>;
> +		qcom,saw2-spm-cmd-wfi = [03 0b 0f];
> +		qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
> +				a0 b0 03 68 70 3b 92 a0 b0
> +				82 2b 50 10 30 02 22 30 0f];
> +	};

- k

Lina Iyer Sept. 24, 2014, 5:21 p.m. UTC | #3

On Wed, Sep 24 2014 at 10:33 -0600, Kumar Gala wrote:
>
>On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> 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 <[e-mail]>
>> [lina: simplify the driver for initial submission, clean up and update
>> commit text]
>> ---
>> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
>> drivers/soc/qcom/Kconfig                          |   8 +
>> drivers/soc/qcom/Makefile                         |   1 +
>> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
>> include/soc/qcom/spm.h                            |  38 +++
>> 5 files changed, 478 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
>> create mode 100644 drivers/soc/qcom/spm.c
>> create mode 100644 include/soc/qcom/spm.h
>>
>> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
>> new file mode 100644
>> index 0000000..2ff2454
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>> @@ -0,0 +1,43 @@
>> +* Subsystem Power Manager (SPM)
>> +
>> +Qualcomm Snapdragons have SPM hardware blocks to control the Application
>> +Processor Sub-System power. These SPM blocks run individual state machine
>> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI
>> +instruction is executed by the core.
>> +
>> +The devicetree representation of the SPM block should be:
>> +
>> +Required properties
>> +
>> +- compatible: Must be -
>> +		 "qcom,spm-v2.1"
>> +- reg: The physical address and the size of the SPM's memory mapped registers
>> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>> +	This field is required on only for SPMs that control the CPU.
>
>Let’s make this just cpu-handle instead of qcom,cpu.  The concept of a handle to a cpu is pretty generic.
>
Okay. Will look into it.
You mean just the property name, right?

>> +- qcom,saw2-clk-div: SAW2 configuration register to program the SPM runtime
>> +	clocks. The register for this property is MSM_SPM_REG_SAW2_CFG.
>
>(add details on how this is used to compute timer tick.  Is it timer tick = saw_clk/saw2-clk-div?  What is valid range of values)
>
The SPM spec is not available for open use. The range of values is
irrelevant for the SPM clocks, usually, its a constant for an SoC, but
may vary between the SoC. Its how the SPM on the SoC interprets it. 

>> +- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
>> +	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.
>
>Didn’t Stephen asked about splitting this up? Or at least treating it as an array of 3 values?
>
Yes he did. My response was similar to the clk-div values, its not
something you can change without hardware spec documentation.
And I need to mix the three values up, anyways before I write to the
register. Splitting it up, doesnt help understanding/configuring the SPM
any better, so didnt change it.

>> +- qcom,saw2-enable: The SPM control register to enable/disable the sleep state
>> +	machine. The register for this property is MSM_SPM_REG_SAW2_SPM_CTL.
>
>Can this just be a boolean (exist or not), if so, probably change it to qcom,saw2-disable (so lack of property means enable)?
>
Okay, sure.

>> +
>> +Optional properties
>> +
>> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>
>probably add something like: “array of bytes …” (want to convey the data type somehow, is there a max length?)
>
Okay.

>> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>
>probably add something like: “array of bytes …” (want to convey the data type somehow, is there a max length?)
>
Okay.

>> +
>> +Example:
>> +	spm@f9089000 {
>> +		compatible = "qcom,spm-v2.1";
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		reg = <0xf9089000 0x1000>;
>> +		qcom,cpu = <&CPU0>;
>> +		qcom,saw2-clk-div = <0x1>;
>> +		qcom,saw2-delays = <0x20000400>;
>> +		qcom,saw2-enable = <0x1>;
>> +		qcom,saw2-spm-cmd-wfi = [03 0b 0f];
>> +		qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
>> +				a0 b0 03 68 70 3b 92 a0 b0
>> +				82 2b 50 10 30 02 22 30 0f];
>> +	};
>
>- k
>
>
>-- 
>Employee of Qualcomm Innovation Center, Inc.
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>

Josh Cartwright Sept. 24, 2014, 5:43 p.m. UTC | #4

Hey Lina-

A few comments inline:

On Tue, Sep 23, 2014 at 05:51:17PM -0600, Lina Iyer wrote:
> +++ b/drivers/soc/qcom/spm.c
[..]
> +
> +static u32 reg_offsets_saw2_v2_1[MSM_SPM_REG_NR] = {

const?

> +	[MSM_SPM_REG_SAW2_SECURE]		= 0x00,
> +	[MSM_SPM_REG_SAW2_ID]			= 0x04,
> +	[MSM_SPM_REG_SAW2_CFG]			= 0x08,
> +	[MSM_SPM_REG_SAW2_SPM_STS]		= 0x0C,
> +	[MSM_SPM_REG_SAW2_AVS_STS]		= 0x10,
> +	[MSM_SPM_REG_SAW2_PMIC_STS]		= 0x14,
> +	[MSM_SPM_REG_SAW2_RST]			= 0x18,
> +	[MSM_SPM_REG_SAW2_VCTL]			= 0x1C,
> +	[MSM_SPM_REG_SAW2_AVS_CTL]		= 0x20,
> +	[MSM_SPM_REG_SAW2_AVS_LIMIT]		= 0x24,
> +	[MSM_SPM_REG_SAW2_AVS_DLY]		= 0x28,
> +	[MSM_SPM_REG_SAW2_AVS_HYSTERESIS]	= 0x2C,
> +	[MSM_SPM_REG_SAW2_SPM_CTL]		= 0x30,
> +	[MSM_SPM_REG_SAW2_SPM_DLY]		= 0x34,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_0]		= 0x40,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_1]		= 0x44,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_2]		= 0x48,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_3]		= 0x4C,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_4]		= 0x50,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_5]		= 0x54,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_6]		= 0x58,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_7]		= 0x5C,
> +	[MSM_SPM_REG_SAW2_SEQ_ENTRY]		= 0x80,
> +	[MSM_SPM_REG_SAW2_VERSION]		= 0xFD0,
> +};
> +
> +struct spm_of {
> +	char *key;

const char *key?

> +	u32 id;
> +};
> +
> +struct msm_spm_mode {
> +	u32 mode;
> +	u32 start_addr;
> +};
> +
> +struct msm_spm_driver_data {
> +	void __iomem *reg_base_addr;
> +	u32 *reg_offsets;
> +	struct msm_spm_mode *modes;
> +	u32 num_modes;

Why u32?

Actually, the maximum modes is fixed, and really all you need to keep
around is the start_addr per-mode (which is only 5 bits), and an
additional bit indicating whether that mode is valid. I'd recommend
folding msm_spm_mode into msm_spm_driver_data completely.  Something
like this, maybe:

	struct msm_spm_driver_data {
		void __iomem *reg_base_addr;
		const u32 *reg_offsets;
		struct {
			u8 is_valid;
			u8 start_addr;
		} modes[MSM_SPM_MODE_NR];
	};

> +};
> +
> +struct msm_spm_device {
> +	bool initialized;
> +	struct msm_spm_driver_data drv;
> +};
> +
> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);

Why have both msm_spm_device and msm_spm_driver_data?

Would it be easier if you instead used 'struct msm_spm_device *', and
used NULL to indicate it has not been initialized?

> +static const struct of_device_id msm_spm_match_table[] __initconst;

Just move the table above probe.

> +
> +static int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
> +						u32 mode)
> +{
> +	int i;
> +	u32 start_addr = 0;
> +	u32 ctl_val;
> +
> +	for (i = 0; i < drv->num_modes; i++) {
> +		if (drv->modes[i].mode == mode) {
> +			start_addr = drv->modes[i].start_addr;
> +			break;
> +		}
> +	}
> +
> +	if (i == drv->num_modes)
> +		return -EINVAL;
> +
> +	/* Update bits 10:4 in the SPM CTL register */
> +	ctl_val = readl_relaxed(drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
> +	start_addr &= 0x7F;
> +	start_addr <<= 4;
> +	ctl_val &= 0xFFFFF80F;
> +	ctl_val |= start_addr;
> +	writel_relaxed(ctl_val, drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
> +	/* Ensure we have written the start address */
> +	wmb();
> +
> +	return 0;
> +}
> +
> +static int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv,
> +					bool enable)
> +{
> +	u32 value = enable ? 0x01 : 0x00;
> +	u32 ctl_val;
> +
> +	ctl_val = readl_relaxed(drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
> +
> +	/* Update SPM_CTL to enable/disable the SPM */
> +	if ((ctl_val & SPM_CTL_ENABLE) != value) {
> +		/* Clear the existing value and update */
> +		ctl_val &= ~0x1;
> +		ctl_val |= value;
> +		writel_relaxed(ctl_val, drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
> +
> +		/* Ensure we have enabled/disabled before returning */
> +		wmb();
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
> + * @mode: SPM LPM mode to enter
> + */
> +int msm_spm_set_low_power_mode(u32 mode)
> +{
> +	struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
> +	int ret = -EINVAL;
> +
> +	if (!dev->initialized)
> +		return -ENXIO;
> +
> +	if (mode == MSM_SPM_MODE_DISABLED)
> +		ret = msm_spm_drv_set_spm_enable(&dev->drv, false);

I would suggest not modeling "DISABLED" as a "mode", as it's not a state
like the others.  Instead, perhaps you could expect users to call
msm_spm_drv_set_spm_enable() directly.

> +	else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
> +		ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(msm_spm_set_low_power_mode);

Is this actually to be used by modules?

[..]
> +static int msm_spm_seq_init(struct msm_spm_device *spm_dev,
> +			struct platform_device *pdev)
> +{
> +	int i;
> +	u8 *cmd;

const u8 *cmd; will save you the cast below.

> +	void *addr;
> +	u32 val;
> +	u32 count = 0;
> +	int offset = 0;
> +	struct msm_spm_mode modes[MSM_SPM_MODE_NR];
> +	u32 sequences[NUM_SEQ_ENTRY/4] = {0};
> +	struct msm_spm_driver_data *drv = &spm_dev->drv;
> +
> +	/* SPM sleep sequences */
> +	struct spm_of mode_of_data[] = {

static const?

> +		{"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
> +		{"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
> +	};
> +
> +	/**
> +	 * Compose the u32 array based on the individual bytes of the SPM
> +	 * sequence for each low power mode that we read from the DT.
> +	 * The sequences are appended if there is space available in the
> +	 * u32 after the end of the previous sequence.
> +	 */
> +
> +	for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
> +		cmd = (u8 *)of_get_property(pdev->dev.of_node,
> +						mode_of_data[i].key, &val);
> +		if (!cmd)
> +			continue;
> +		/* The last in the sequence should be 0x0F */
> +		if (cmd[val - 1] != 0x0F)
> +			continue;
> +		modes[count].mode = mode_of_data[i].id;
> +		modes[count].start_addr = offset;
> +		append_seq_data(&sequences[0], cmd, &offset);
> +		count++;
> +	}
> +
> +	/* Write the idle state sequences to SPM */
> +	drv->modes = devm_kcalloc(&pdev->dev, count,
> +					sizeof(modes[0]), GFP_KERNEL);
> +	if (!drv->modes)
> +		return -ENOMEM;
> +
> +	drv->num_modes = count;
> +	memcpy(drv->modes, modes, sizeof(modes[0]) * count);
> +
> +	/* Flush the integer array */
> +	addr = drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SEQ_ENTRY];
> +	for (i = 0; i < ARRAY_SIZE(sequences); i++, addr += 4)
> +		writel_relaxed(sequences[i], addr);
> +
> +	/* Ensure we flush the writes */
> +	wmb();
> +
> +	return 0;
> +}
> +
> +static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
> +{
> +	struct msm_spm_device *dev = NULL;
> +	struct device_node *cpu_node;
> +	u32 cpu;
> +
> +	cpu_node = of_parse_phandle(pdev->dev.of_node, "qcom,cpu", 0);
> +	if (cpu_node) {
> +		for_each_possible_cpu(cpu) {
> +			if (of_get_cpu_node(cpu, NULL) == cpu_node)
> +				dev = &per_cpu(msm_cpu_spm_device, cpu);
> +		}
> +	}
> +
> +	return dev;
> +}
> +
> +static int msm_spm_dev_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	int i;
> +	u32 val;
> +	struct msm_spm_device *spm_dev;
> +	struct resource *res;
> +	const struct of_device_id *match_id;
> +
> +	/* SPM Configuration registers */
> +	struct spm_of spm_of_data[] = {

static const?

> +		{"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
> +		{"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
> +		{"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
> +	};
> +
> +	 /* Get the right SPM device */
> +	spm_dev = msm_spm_get_device(pdev);
> +	if (IS_ERR_OR_NULL(spm_dev))

Should this just be a simple NULL check?

> +		return -EINVAL;
> +
> +	/* Get the SPM start address */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +	spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
> +					resource_size(res));

devm_ioremap_resource()?

> +	if (!spm_dev->drv.reg_base_addr) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	match_id = of_match_node(msm_spm_match_table, pdev->dev.of_node);
> +	if (!match_id)
> +		return -ENODEV;
> +
> +	/* Use the register offsets for the SPM version in use */
> +	spm_dev->drv.reg_offsets = (u32 *)match_id->data;
> +	if (!spm_dev->drv.reg_offsets)
> +		return -EFAULT;
> +
> +	/* Read the SPM idle state sequences */
> +	ret = msm_spm_seq_init(spm_dev, pdev);
> +	if (ret)
> +		return ret;
> +
> +	/* Read the SPM register values */
> +	for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
> +		ret = of_property_read_u32(pdev->dev.of_node,
> +					spm_of_data[i].key, &val);
> +		if (ret)
> +			continue;
> +		writel_relaxed(val, spm_dev->drv.reg_base_addr +
> +				spm_dev->drv.reg_offsets[spm_of_data[i].id]);
> +	}
> +
> +	/* Flush all writes */

This isn't very descriptive.  Perhaps:

/*
 * Ensure all observers see the above register writes before the
 * updating of spm_dev->initialized
 */

> +	wmb();
> +
> +	spm_dev->initialized = true;
> +	return ret;
> +fail:
> +	dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
> +	return ret;
> +}
> +
> +static const struct of_device_id msm_spm_match_table[] __initconst = {
> +	{.compatible = "qcom,spm-v2.1", .data = reg_offsets_saw2_v2_1},
> +	{ },
> +};
> +
> +
> +static struct platform_driver msm_spm_device_driver = {
> +	.probe = msm_spm_dev_probe,
> +	.driver = {
> +		.name = "spm",
> +		.owner = THIS_MODULE,

This assignment is not necessary.

> +		.of_match_table = msm_spm_match_table,
> +	},
> +};
> +
> +static int __init msm_spm_device_init(void)
> +{
> +	return platform_driver_register(&msm_spm_device_driver);
> +}
> +device_initcall(msm_spm_device_init);
> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
> new file mode 100644
> index 0000000..29686ef
> --- /dev/null
> +++ b/include/soc/qcom/spm.h
> @@ -0,0 +1,38 @@
> +/* 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 {
> +	MSM_SPM_MODE_DISABLED,
> +	MSM_SPM_MODE_CLOCK_GATING,
> +	MSM_SPM_MODE_RETENTION,
> +	MSM_SPM_MODE_GDHS,
> +	MSM_SPM_MODE_POWER_COLLAPSE,
> +	MSM_SPM_MODE_NR
> +};

Is there a particular reason you aren't naming this enumeration, and
using it's type in msm_spm_set_low_power_mode()?

> +
> +struct msm_spm_device;

Why this forward declaration?

> +
> +#if defined(CONFIG_QCOM_PM)
> +
> +int msm_spm_set_low_power_mode(u32 mode);
> +
> +#else
> +
> +static inline int msm_spm_set_low_power_mode(u32 mode)
> +{ return -ENOSYS; }
> +
> +#endif  /* CONFIG_QCOM_PM */
> +
> +#endif  /* __QCOM_SPM_H */
> -- 
> 1.9.1
> 

Thanks,
  Josh

Kumar Gala Sept. 24, 2014, 5:53 p.m. UTC | #5

On Sep 24, 2014, at 12:21 PM, Lina Iyer <[e-mail]> wrote:

> On Wed, Sep 24 2014 at 10:33 -0600, Kumar Gala wrote:
>> 
>> On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> 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 <[e-mail]>
>>> [lina: simplify the driver for initial submission, clean up and update
>>> commit text]
>>> ---
>>> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
>>> drivers/soc/qcom/Kconfig                          |   8 +
>>> drivers/soc/qcom/Makefile                         |   1 +
>>> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
>>> include/soc/qcom/spm.h                            |  38 +++
>>> 5 files changed, 478 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
>>> create mode 100644 drivers/soc/qcom/spm.c
>>> create mode 100644 include/soc/qcom/spm.h
>>> 
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>> new file mode 100644
>>> index 0000000..2ff2454
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>> @@ -0,0 +1,43 @@
>>> +* Subsystem Power Manager (SPM)
>>> +
>>> +Qualcomm Snapdragons have SPM hardware blocks to control the Application
>>> +Processor Sub-System power. These SPM blocks run individual state machine
>>> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI
>>> +instruction is executed by the core.
>>> +
>>> +The devicetree representation of the SPM block should be:
>>> +
>>> +Required properties
>>> +
>>> +- compatible: Must be -
>>> +		 "qcom,spm-v2.1"
>>> +- reg: The physical address and the size of the SPM's memory mapped registers
>>> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>>> +	This field is required on only for SPMs that control the CPU.
>> 
>> Let’s make this just cpu-handle instead of qcom,cpu.  The concept of a handle to a cpu is pretty generic.
>> 
> Okay. Will look into it.
> You mean just the property name, right?

Correct.

> 
>>> +- qcom,saw2-clk-div: SAW2 configuration register to program the SPM runtime
>>> +	clocks. The register for this property is MSM_SPM_REG_SAW2_CFG.
>> 
>> (add details on how this is used to compute timer tick.  Is it timer tick = saw_clk/saw2-clk-div?  What is valid range of values)
>> 
> The SPM spec is not available for open use. The range of values is
> irrelevant for the SPM clocks, usually, its a constant for an SoC, but
> may vary between the SoC. Its how the SPM on the SoC interprets it.

Does the meaning of the divisor value change from SoC to SoC (not the value itself)?

Is it not always:

timer tick = sys_ref_clk / (qcom,saw2-clk-div + 1)
 
>>> +- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
>>> +	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.
>> 
>> Didn’t Stephen asked about splitting this up? Or at least treating it as an array of 3 values?
>> 
> Yes he did. My response was similar to the clk-div values, its not
> something you can change without hardware spec documentation.
> And I need to mix the three values up, anyways before I write to the
> register. Splitting it up, doesnt help understanding/configuring the SPM
> any better, so didnt change it.

Hmm, will this value change from SPM to SPM on the same SoC?  I’m not a fan of allowing random register values to get poked into the HW from DT.  While this one case might end up being acceptable, its a terrible practice and not something I want use in the habit of doing.

>>> +- qcom,saw2-enable: The SPM control register to enable/disable the sleep state
>>> +	machine. The register for this property is MSM_SPM_REG_SAW2_SPM_CTL.
>> 
>> Can this just be a boolean (exist or not), if so, probably change it to qcom,saw2-disable (so lack of property means enable)?
>> 
> Okay, sure.
> 
>>> +
>>> +Optional properties
>>> +
>>> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>> 
>> probably add something like: “array of bytes …” (want to convey the data type somehow, is there a max length?)
>> 
> Okay.
> 
>>> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>> 
>> probably add something like: “array of bytes …” (want to convey the data type somehow, is there a max length?)
>> 
> Okay.
> 
>>> +
>>> +Example:
>>> +	spm@f9089000 {
>>> +		compatible = "qcom,spm-v2.1";
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +		reg = <0xf9089000 0x1000>;
>>> +		qcom,cpu = <&CPU0>;
>>> +		qcom,saw2-clk-div = <0x1>;
>>> +		qcom,saw2-delays = <0x20000400>;
>>> +		qcom,saw2-enable = <0x1>;
>>> +		qcom,saw2-spm-cmd-wfi = [03 0b 0f];
>>> +		qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
>>> +				a0 b0 03 68 70 3b 92 a0 b0
>>> +				82 2b 50 10 30 02 22 30 0f];
>>> +	};
>> 
>> - k
>> 
>> 
>> -- 
>> Employee of Qualcomm Innovation Center, Inc.
>> 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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kumar Gala Sept. 24, 2014, 6:07 p.m. UTC | #6

On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> 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 <[e-mail]>
> [lina: simplify the driver for initial submission, clean up and update
> commit text]
> ---
> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
> drivers/soc/qcom/Kconfig                          |   8 +
> drivers/soc/qcom/Makefile                         |   1 +
> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
> include/soc/qcom/spm.h                            |  38 +++
> 5 files changed, 478 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
> create mode 100644 drivers/soc/qcom/spm.c
> create mode 100644 include/soc/qcom/spm.h

General comment, lets use qcom instead of msm for various things.

[snip]

> 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
> +	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..1fa6a96
> --- /dev/null
> +++ b/drivers/soc/qcom/spm.c
> @@ -0,0 +1,388 @@
> +/* 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>
> +
> +#define NUM_SEQ_ENTRY 32
> +#define SPM_CTL_ENABLE BIT(0)
> +
> +enum {
> +	MSM_SPM_REG_SAW2_CFG,
> +	MSM_SPM_REG_SAW2_AVS_CTL,
> +	MSM_SPM_REG_SAW2_AVS_HYSTERESIS,
> +	MSM_SPM_REG_SAW2_SPM_CTL,
> +	MSM_SPM_REG_SAW2_PMIC_DLY,
> +	MSM_SPM_REG_SAW2_AVS_LIMIT,
> +	MSM_SPM_REG_SAW2_AVS_DLY,
> +	MSM_SPM_REG_SAW2_SPM_DLY,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_0,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_1,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_2,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_3,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_4,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_5,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_6,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_7,
> +	MSM_SPM_REG_SAW2_RST,
> +
> +	MSM_SPM_REG_NR_INITIALIZE = MSM_SPM_REG_SAW2_RST,
> +
> +	MSM_SPM_REG_SAW2_ID,
> +	MSM_SPM_REG_SAW2_SECURE,
> +	MSM_SPM_REG_SAW2_STS0,
> +	MSM_SPM_REG_SAW2_STS1,
> +	MSM_SPM_REG_SAW2_STS2,
> +	MSM_SPM_REG_SAW2_VCTL,
> +	MSM_SPM_REG_SAW2_SEQ_ENTRY,
> +	MSM_SPM_REG_SAW2_SPM_STS,
> +	MSM_SPM_REG_SAW2_AVS_STS,
> +	MSM_SPM_REG_SAW2_PMIC_STS,
> +	MSM_SPM_REG_SAW2_VERSION,
> +
> +	MSM_SPM_REG_NR,
> +};
> +
> +static u32 reg_offsets_saw2_v2_1[MSM_SPM_REG_NR] = {

Why an array, can’t this just be an enum?

> +	[MSM_SPM_REG_SAW2_SECURE]		= 0x00,
> +	[MSM_SPM_REG_SAW2_ID]			= 0x04,
> +	[MSM_SPM_REG_SAW2_CFG]			= 0x08,
> +	[MSM_SPM_REG_SAW2_SPM_STS]		= 0x0C,
> +	[MSM_SPM_REG_SAW2_AVS_STS]		= 0x10,
> +	[MSM_SPM_REG_SAW2_PMIC_STS]		= 0x14,
> +	[MSM_SPM_REG_SAW2_RST]			= 0x18,
> +	[MSM_SPM_REG_SAW2_VCTL]			= 0x1C,
> +	[MSM_SPM_REG_SAW2_AVS_CTL]		= 0x20,
> +	[MSM_SPM_REG_SAW2_AVS_LIMIT]		= 0x24,
> +	[MSM_SPM_REG_SAW2_AVS_DLY]		= 0x28,
> +	[MSM_SPM_REG_SAW2_AVS_HYSTERESIS]	= 0x2C,
> +	[MSM_SPM_REG_SAW2_SPM_CTL]		= 0x30,
> +	[MSM_SPM_REG_SAW2_SPM_DLY]		= 0x34,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_0]		= 0x40,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_1]		= 0x44,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_2]		= 0x48,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_3]		= 0x4C,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_4]		= 0x50,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_5]		= 0x54,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_6]		= 0x58,
> +	[MSM_SPM_REG_SAW2_PMIC_DATA_7]		= 0x5C,
> +	[MSM_SPM_REG_SAW2_SEQ_ENTRY]		= 0x80,
> +	[MSM_SPM_REG_SAW2_VERSION]		= 0xFD0,
> +};
> +
> +struct spm_of {
> +	char *key;
> +	u32 id;
> +};
> +
> +struct msm_spm_mode {
> +	u32 mode;
> +	u32 start_addr;
> +};
> +
> +struct msm_spm_driver_data {
> +	void __iomem *reg_base_addr;
> +	u32 *reg_offsets;
> +	struct msm_spm_mode *modes;
> +	u32 num_modes;
> +};
> +
> +struct msm_spm_device {
> +	bool initialized;
> +	struct msm_spm_driver_data drv;
> +};
> +
> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);
> +
> +static const struct of_device_id msm_spm_match_table[] __initconst;
> +
> +static int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
> +						u32 mode)
> +{

Can we just fold this into msm_spm_set_low_power_mode
> 
> +	int i;
> +	u32 start_addr = 0;
> +	u32 ctl_val;
> +
> +	for (i = 0; i < drv->num_modes; i++) {
> +		if (drv->modes[i].mode == mode) {
> +			start_addr = drv->modes[i].start_addr;
> +			break;
> +		}
> +	}
> +
> +	if (i == drv->num_modes)
> +		return -EINVAL;
> +
> +	/* Update bits 10:4 in the SPM CTL register */
> +	ctl_val = readl_relaxed(drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
> +	start_addr &= 0x7F;
> +	start_addr <<= 4;
> +	ctl_val &= 0xFFFFF80F;
> +	ctl_val |= start_addr;
> +	writel_relaxed(ctl_val, drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
> +	/* Ensure we have written the start address */
> +	wmb();
> +
> +	return 0;
> +}
> +
> +static int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv,
> +					bool enable)
> +{
> +	u32 value = enable ? 0x01 : 0x00;
> +	u32 ctl_val;
> +
> +	ctl_val = readl_relaxed(drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
> +
> +	/* Update SPM_CTL to enable/disable the SPM */
> +	if ((ctl_val & SPM_CTL_ENABLE) != value) {
> +		/* Clear the existing value and update */
> +		ctl_val &= ~0x1;
> +		ctl_val |= value;
> +		writel_relaxed(ctl_val, drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
> +
> +		/* Ensure we have enabled/disabled before returning */
> +		wmb();
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
> + * @mode: SPM LPM mode to enter
> + */
> +int msm_spm_set_low_power_mode(u32 mode)
> +{
> +	struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
> +	int ret = -EINVAL;
> +
> +	if (!dev->initialized)
> +		return -ENXIO;
> +
> +	if (mode == MSM_SPM_MODE_DISABLED)
> +		ret = msm_spm_drv_set_spm_enable(&dev->drv, false);
> +	else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
> +		ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
> +

This could become:

	if (mode == MSM_SPM_MODE_DISABLED)
		return msm_spm_drv_set_spm_enable(&dev->drv, false);

	if (msm_spm_drv_set_spm_enable(&dev->drv, true))
		return ret;

	code from msm_spm_drv_set_low_power_mode

> +	return ret;
> +}
> +EXPORT_SYMBOL(msm_spm_set_low_power_mode);
> +
> +static void append_seq_data(u32 *reg_seq_entry, u8 *cmd, u32 *offset)
> +{
> +	u32 cmd_w;
> +	u32 offset_w = *offset / 4;
> +	u8 last_cmd;
> +
> +	while (1) {
> +		int i;
> +
> +		cmd_w = 0;
> +		last_cmd = 0;
> +		cmd_w = reg_seq_entry[offset_w];
> +
> +		for (i = (*offset % 4); i < 4; i++) {
> +			last_cmd = *(cmd++);
> +			cmd_w |=  last_cmd << (i * 8);
> +			(*offset)++;
> +			if (last_cmd == 0x0f)
> +				break;
> +		}
> +
> +		reg_seq_entry[offset_w++] = cmd_w;
> +		if (last_cmd == 0x0f)
> +			break;
> +	}
> +}
> +
> +static int msm_spm_seq_init(struct msm_spm_device *spm_dev,
> +			struct platform_device *pdev)
> +{
> +	int i;
> +	u8 *cmd;
> +	void *addr;
> +	u32 val;
> +	u32 count = 0;
> +	int offset = 0;
> +	struct msm_spm_mode modes[MSM_SPM_MODE_NR];
> +	u32 sequences[NUM_SEQ_ENTRY/4] = {0};
> +	struct msm_spm_driver_data *drv = &spm_dev->drv;
> +
> +	/* SPM sleep sequences */
> +	struct spm_of mode_of_data[] = {
> +		{"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
> +		{"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
> +	};
> +
> +	/**
> +	 * Compose the u32 array based on the individual bytes of the SPM
> +	 * sequence for each low power mode that we read from the DT.
> +	 * The sequences are appended if there is space available in the
> +	 * u32 after the end of the previous sequence.
> +	 */
> +
> +	for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
> +		cmd = (u8 *)of_get_property(pdev->dev.of_node,
> +						mode_of_data[i].key, &val);
> +		if (!cmd)
> +			continue;
> +		/* The last in the sequence should be 0x0F */
> +		if (cmd[val - 1] != 0x0F)
> +			continue;
> +		modes[count].mode = mode_of_data[i].id;
> +		modes[count].start_addr = offset;
> +		append_seq_data(&sequences[0], cmd, &offset);
> +		count++;
> +	}
> +
> +	/* Write the idle state sequences to SPM */
> +	drv->modes = devm_kcalloc(&pdev->dev, count,
> +					sizeof(modes[0]), GFP_KERNEL);
> +	if (!drv->modes)
> +		return -ENOMEM;
> +
> +	drv->num_modes = count;
> +	memcpy(drv->modes, modes, sizeof(modes[0]) * count);
> +
> +	/* Flush the integer array */
> +	addr = drv->reg_base_addr +
> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SEQ_ENTRY];
> +	for (i = 0; i < ARRAY_SIZE(sequences); i++, addr += 4)
> +		writel_relaxed(sequences[i], addr);
> +
> +	/* Ensure we flush the writes */
> +	wmb();
> +
> +	return 0;
> +}
> +
> +static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
> +{
> +	struct msm_spm_device *dev = NULL;
> +	struct device_node *cpu_node;
> +	u32 cpu;
> +
> +	cpu_node = of_parse_phandle(pdev->dev.of_node, "qcom,cpu", 0);
> +	if (cpu_node) {
> +		for_each_possible_cpu(cpu) {
> +			if (of_get_cpu_node(cpu, NULL) == cpu_node)
> +				dev = &per_cpu(msm_cpu_spm_device, cpu);
> +		}
> +	}
> +
> +	return dev;
> +}
> +
> +static int msm_spm_dev_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	int i;
> +	u32 val;
> +	struct msm_spm_device *spm_dev;
> +	struct resource *res;
> +	const struct of_device_id *match_id;
> +
> +	/* SPM Configuration registers */
> +	struct spm_of spm_of_data[] = {
> +		{"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
> +		{"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
> +		{"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
> +	};

Remove this array and do explicit of parsing and register setting, so only explicitly set what we should.

> +
> +	 /* Get the right SPM device */
> +	spm_dev = msm_spm_get_device(pdev);
> +	if (IS_ERR_OR_NULL(spm_dev))
> +		return -EINVAL;
> +
> +	/* Get the SPM start address */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +	spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
> +					resource_size(res));
> +	if (!spm_dev->drv.reg_base_addr) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}

Can we move to using devm_ioremap_resource() to reduce the platform_get_resource/devm_ioremap combo

> +
> +	match_id = of_match_node(msm_spm_match_table, pdev->dev.of_node);
> +	if (!match_id)
> +		return -ENODEV;
> +
> +	/* Use the register offsets for the SPM version in use */
> +	spm_dev->drv.reg_offsets = (u32 *)match_id->data;
> +	if (!spm_dev->drv.reg_offsets)
> +		return -EFAULT;
> +
> +	/* Read the SPM idle state sequences */
> +	ret = msm_spm_seq_init(spm_dev, pdev);
> +	if (ret)
> +		return ret;
> +
> +	/* Read the SPM register values */
> +	for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
> +		ret = of_property_read_u32(pdev->dev.of_node,
> +					spm_of_data[i].key, &val);
> +		if (ret)
> +			continue;
> +		writel_relaxed(val, spm_dev->drv.reg_base_addr +
> +				spm_dev->drv.reg_offsets[spm_of_data[i].id]);
> +	}

Change this to explicit parsing of each of prop and add proper read/modify/writes so we only change the things in the registers we should be touching

> +
> +	/* Flush all writes */
> +	wmb();
> +
> +	spm_dev->initialized = true;
> +	return ret;
> +fail:
> +	dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
> +	return ret;
> +}
> +
> +static const struct of_device_id msm_spm_match_table[] __initconst = {
> +	{.compatible = "qcom,spm-v2.1", .data = reg_offsets_saw2_v2_1},
> +	{ },
> +};
> +
> +
> +static struct platform_driver msm_spm_device_driver = {
> +	.probe = msm_spm_dev_probe,
> +	.driver = {
> +		.name = "spm",
> +		.owner = THIS_MODULE,
> +		.of_match_table = msm_spm_match_table,
> +	},
> +};
> +
> +static int __init msm_spm_device_init(void)
> +{
> +	return platform_driver_register(&msm_spm_device_driver);
> +}
> +device_initcall(msm_spm_device_init);
> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
> new file mode 100644
> index 0000000..29686ef
> --- /dev/null
> +++ b/include/soc/qcom/spm.h
> @@ -0,0 +1,38 @@
> +/* 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 {
> +	MSM_SPM_MODE_DISABLED,
> +	MSM_SPM_MODE_CLOCK_GATING,
> +	MSM_SPM_MODE_RETENTION,
> +	MSM_SPM_MODE_GDHS,
> +	MSM_SPM_MODE_POWER_COLLAPSE,
> +	MSM_SPM_MODE_NR
> +};

Why don’t we make this a named enum, than msm_spm_set_low_power_mode can take that enum
> 
> +
> +struct msm_spm_device;
> +
> +#if defined(CONFIG_QCOM_PM)
> +
> +int msm_spm_set_low_power_mode(u32 mode);

So this could become

int qcom_spm_set_low_power_mode(enum qcom_spm_mode mode)

> +
> +#else
> +
> +static inline int msm_spm_set_low_power_mode(u32 mode)
> +{ return -ENOSYS; }
> +
> +#endif  /* CONFIG_QCOM_PM */
> +
> +#endif  /* __QCOM_SPM_H */
> -- 
> 1.9.1
>

Lina Iyer Sept. 24, 2014, 6:47 p.m. UTC | #7

On Wed, Sep 24 2014 at 12:07 -0600, Kumar Gala wrote:
>
>On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> 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 <[e-mail]>
>> [lina: simplify the driver for initial submission, clean up and update
>> commit text]
>> ---
>> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
>> drivers/soc/qcom/Kconfig                          |   8 +
>> drivers/soc/qcom/Makefile                         |   1 +
>> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
>> include/soc/qcom/spm.h                            |  38 +++
>> 5 files changed, 478 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
>> create mode 100644 drivers/soc/qcom/spm.c
>> create mode 100644 include/soc/qcom/spm.h
>
>General comment, lets use qcom instead of msm for various things.
>
>[snip]
>
OK, Done. I renamed all msm_ functions to qcom_ functions as well.

>> 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
>> +	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..1fa6a96
>> --- /dev/null
>> +++ b/drivers/soc/qcom/spm.c
>> @@ -0,0 +1,388 @@
>> +/* 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>
>> +
>> +#define NUM_SEQ_ENTRY 32
>> +#define SPM_CTL_ENABLE BIT(0)
>> +
>> +enum {
>> +	MSM_SPM_REG_SAW2_CFG,
>> +	MSM_SPM_REG_SAW2_AVS_CTL,
>> +	MSM_SPM_REG_SAW2_AVS_HYSTERESIS,
>> +	MSM_SPM_REG_SAW2_SPM_CTL,
>> +	MSM_SPM_REG_SAW2_PMIC_DLY,
>> +	MSM_SPM_REG_SAW2_AVS_LIMIT,
>> +	MSM_SPM_REG_SAW2_AVS_DLY,
>> +	MSM_SPM_REG_SAW2_SPM_DLY,
>> +	MSM_SPM_REG_SAW2_PMIC_DATA_0,
>> +	MSM_SPM_REG_SAW2_PMIC_DATA_1,
>> +	MSM_SPM_REG_SAW2_PMIC_DATA_2,
>> +	MSM_SPM_REG_SAW2_PMIC_DATA_3,
>> +	MSM_SPM_REG_SAW2_PMIC_DATA_4,
>> +	MSM_SPM_REG_SAW2_PMIC_DATA_5,
>> +	MSM_SPM_REG_SAW2_PMIC_DATA_6,
>> +	MSM_SPM_REG_SAW2_PMIC_DATA_7,
>> +	MSM_SPM_REG_SAW2_RST,
>> +
>> +	MSM_SPM_REG_NR_INITIALIZE = MSM_SPM_REG_SAW2_RST,
>> +
>> +	MSM_SPM_REG_SAW2_ID,
>> +	MSM_SPM_REG_SAW2_SECURE,
>> +	MSM_SPM_REG_SAW2_STS0,
>> +	MSM_SPM_REG_SAW2_STS1,
>> +	MSM_SPM_REG_SAW2_STS2,
>> +	MSM_SPM_REG_SAW2_VCTL,
>> +	MSM_SPM_REG_SAW2_SEQ_ENTRY,
>> +	MSM_SPM_REG_SAW2_SPM_STS,
>> +	MSM_SPM_REG_SAW2_AVS_STS,
>> +	MSM_SPM_REG_SAW2_PMIC_STS,
>> +	MSM_SPM_REG_SAW2_VERSION,
>> +
>> +	MSM_SPM_REG_NR,
>> +};
>> +
>> +static u32 reg_offsets_saw2_v2_1[MSM_SPM_REG_NR] = {
>
>Why an array, can’t this just be an enum?
>
An array makes it easier to have multiple SPM versions. The value of the
enums are not very malleable as the driver scales to support multiple
SPM revisions.

>> +	[MSM_SPM_REG_SAW2_SECURE]		= 0x00,
>> +	[MSM_SPM_REG_SAW2_ID]			= 0x04,
>> +	[MSM_SPM_REG_SAW2_CFG]			= 0x08,
>> +	[MSM_SPM_REG_SAW2_SPM_STS]		= 0x0C,
>> +	[MSM_SPM_REG_SAW2_AVS_STS]		= 0x10,
>> +	[MSM_SPM_REG_SAW2_PMIC_STS]		= 0x14,
>> +	[MSM_SPM_REG_SAW2_RST]			= 0x18,
>> +	[MSM_SPM_REG_SAW2_VCTL]			= 0x1C,
>> +	[MSM_SPM_REG_SAW2_AVS_CTL]		= 0x20,
>> +	[MSM_SPM_REG_SAW2_AVS_LIMIT]		= 0x24,
>> +	[MSM_SPM_REG_SAW2_AVS_DLY]		= 0x28,
>> +	[MSM_SPM_REG_SAW2_AVS_HYSTERESIS]	= 0x2C,
>> +	[MSM_SPM_REG_SAW2_SPM_CTL]		= 0x30,
>> +	[MSM_SPM_REG_SAW2_SPM_DLY]		= 0x34,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_0]		= 0x40,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_1]		= 0x44,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_2]		= 0x48,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_3]		= 0x4C,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_4]		= 0x50,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_5]		= 0x54,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_6]		= 0x58,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_7]		= 0x5C,
>> +	[MSM_SPM_REG_SAW2_SEQ_ENTRY]		= 0x80,
>> +	[MSM_SPM_REG_SAW2_VERSION]		= 0xFD0,
>> +};
>> +
>> +struct spm_of {
>> +	char *key;
>> +	u32 id;
>> +};
>> +
>> +struct msm_spm_mode {
>> +	u32 mode;
>> +	u32 start_addr;
>> +};
>> +
>> +struct msm_spm_driver_data {
>> +	void __iomem *reg_base_addr;
>> +	u32 *reg_offsets;
>> +	struct msm_spm_mode *modes;
>> +	u32 num_modes;
>> +};
>> +
>> +struct msm_spm_device {
>> +	bool initialized;
>> +	struct msm_spm_driver_data drv;
>> +};
>> +
>> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);
>> +
>> +static const struct of_device_id msm_spm_match_table[] __initconst;
>> +
>> +static int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
>> +						u32 mode)
>> +{
>
>Can we just fold this into msm_spm_set_low_power_mode

OK.

>>
>> +	int i;
>> +	u32 start_addr = 0;
>> +	u32 ctl_val;
>> +
>> +	for (i = 0; i < drv->num_modes; i++) {
>> +		if (drv->modes[i].mode == mode) {
>> +			start_addr = drv->modes[i].start_addr;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (i == drv->num_modes)
>> +		return -EINVAL;
>> +
>> +	/* Update bits 10:4 in the SPM CTL register */
>> +	ctl_val = readl_relaxed(drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>> +	start_addr &= 0x7F;
>> +	start_addr <<= 4;
>> +	ctl_val &= 0xFFFFF80F;
>> +	ctl_val |= start_addr;
>> +	writel_relaxed(ctl_val, drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>> +	/* Ensure we have written the start address */
>> +	wmb();
>> +
>> +	return 0;
>> +}
>> +
>> +static int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv,
>> +					bool enable)
>> +{
>> +	u32 value = enable ? 0x01 : 0x00;
>> +	u32 ctl_val;
>> +
>> +	ctl_val = readl_relaxed(drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>> +
>> +	/* Update SPM_CTL to enable/disable the SPM */
>> +	if ((ctl_val & SPM_CTL_ENABLE) != value) {
>> +		/* Clear the existing value and update */
>> +		ctl_val &= ~0x1;
>> +		ctl_val |= value;
>> +		writel_relaxed(ctl_val, drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>> +
>> +		/* Ensure we have enabled/disabled before returning */
>> +		wmb();
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
>> + * @mode: SPM LPM mode to enter
>> + */
>> +int msm_spm_set_low_power_mode(u32 mode)
>> +{
>> +	struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
>> +	int ret = -EINVAL;
>> +
>> +	if (!dev->initialized)
>> +		return -ENXIO;
>> +
>> +	if (mode == MSM_SPM_MODE_DISABLED)
>> +		ret = msm_spm_drv_set_spm_enable(&dev->drv, false);
>> +	else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
>> +		ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
>> +
>
>This could become:
>
>	if (mode == MSM_SPM_MODE_DISABLED)
>		return msm_spm_drv_set_spm_enable(&dev->drv, false);
>
>	if (msm_spm_drv_set_spm_enable(&dev->drv, true))
>		return ret;
>
>	code from msm_spm_drv_set_low_power_mode
>
Sure

>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(msm_spm_set_low_power_mode);
>> +
>> +static void append_seq_data(u32 *reg_seq_entry, u8 *cmd, u32 *offset)
>> +{
>> +	u32 cmd_w;
>> +	u32 offset_w = *offset / 4;
>> +	u8 last_cmd;
>> +
>> +	while (1) {
>> +		int i;
>> +
>> +		cmd_w = 0;
>> +		last_cmd = 0;
>> +		cmd_w = reg_seq_entry[offset_w];
>> +
>> +		for (i = (*offset % 4); i < 4; i++) {
>> +			last_cmd = *(cmd++);
>> +			cmd_w |=  last_cmd << (i * 8);
>> +			(*offset)++;
>> +			if (last_cmd == 0x0f)
>> +				break;
>> +		}
>> +
>> +		reg_seq_entry[offset_w++] = cmd_w;
>> +		if (last_cmd == 0x0f)
>> +			break;
>> +	}
>> +}
>> +
>> +static int msm_spm_seq_init(struct msm_spm_device *spm_dev,
>> +			struct platform_device *pdev)
>> +{
>> +	int i;
>> +	u8 *cmd;
>> +	void *addr;
>> +	u32 val;
>> +	u32 count = 0;
>> +	int offset = 0;
>> +	struct msm_spm_mode modes[MSM_SPM_MODE_NR];
>> +	u32 sequences[NUM_SEQ_ENTRY/4] = {0};
>> +	struct msm_spm_driver_data *drv = &spm_dev->drv;
>> +
>> +	/* SPM sleep sequences */
>> +	struct spm_of mode_of_data[] = {
>> +		{"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
>> +		{"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
>> +	};
>> +
>> +	/**
>> +	 * Compose the u32 array based on the individual bytes of the SPM
>> +	 * sequence for each low power mode that we read from the DT.
>> +	 * The sequences are appended if there is space available in the
>> +	 * u32 after the end of the previous sequence.
>> +	 */
>> +
>> +	for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
>> +		cmd = (u8 *)of_get_property(pdev->dev.of_node,
>> +						mode_of_data[i].key, &val);
>> +		if (!cmd)
>> +			continue;
>> +		/* The last in the sequence should be 0x0F */
>> +		if (cmd[val - 1] != 0x0F)
>> +			continue;
>> +		modes[count].mode = mode_of_data[i].id;
>> +		modes[count].start_addr = offset;
>> +		append_seq_data(&sequences[0], cmd, &offset);
>> +		count++;
>> +	}
>> +
>> +	/* Write the idle state sequences to SPM */
>> +	drv->modes = devm_kcalloc(&pdev->dev, count,
>> +					sizeof(modes[0]), GFP_KERNEL);
>> +	if (!drv->modes)
>> +		return -ENOMEM;
>> +
>> +	drv->num_modes = count;
>> +	memcpy(drv->modes, modes, sizeof(modes[0]) * count);
>> +
>> +	/* Flush the integer array */
>> +	addr = drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SEQ_ENTRY];
>> +	for (i = 0; i < ARRAY_SIZE(sequences); i++, addr += 4)
>> +		writel_relaxed(sequences[i], addr);
>> +
>> +	/* Ensure we flush the writes */
>> +	wmb();
>> +
>> +	return 0;
>> +}
>> +
>> +static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
>> +{
>> +	struct msm_spm_device *dev = NULL;
>> +	struct device_node *cpu_node;
>> +	u32 cpu;
>> +
>> +	cpu_node = of_parse_phandle(pdev->dev.of_node, "qcom,cpu", 0);
>> +	if (cpu_node) {
>> +		for_each_possible_cpu(cpu) {
>> +			if (of_get_cpu_node(cpu, NULL) == cpu_node)
>> +				dev = &per_cpu(msm_cpu_spm_device, cpu);
>> +		}
>> +	}
>> +
>> +	return dev;
>> +}
>> +
>> +static int msm_spm_dev_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	int i;
>> +	u32 val;
>> +	struct msm_spm_device *spm_dev;
>> +	struct resource *res;
>> +	const struct of_device_id *match_id;
>> +
>> +	/* SPM Configuration registers */
>> +	struct spm_of spm_of_data[] = {
>> +		{"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
>> +		{"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
>> +		{"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
>> +	};
>
>Remove this array and do explicit of parsing and register setting, so only explicitly set what we should.
>
too many of_get_property() calls then Would it be okay if I use a
function to read the property and write to the SPM and call that
repeatedly based on key/enum passed?
I found this array method to be easy to scale.

Just for my curiosity, what is the drawback of this approach?

>> +
>> +	 /* Get the right SPM device */
>> +	spm_dev = msm_spm_get_device(pdev);
>> +	if (IS_ERR_OR_NULL(spm_dev))
>> +		return -EINVAL;
>> +
>> +	/* Get the SPM start address */
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		ret = -EINVAL;
>> +		goto fail;
>> +	}
>> +	spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
>> +					resource_size(res));
>> +	if (!spm_dev->drv.reg_base_addr) {
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>
>Can we move to using devm_ioremap_resource() to reduce the platform_get_resource/devm_ioremap combo
>
Okay, will fix.

>> +
>> +	match_id = of_match_node(msm_spm_match_table, pdev->dev.of_node);
>> +	if (!match_id)
>> +		return -ENODEV;
>> +
>> +	/* Use the register offsets for the SPM version in use */
>> +	spm_dev->drv.reg_offsets = (u32 *)match_id->data;
>> +	if (!spm_dev->drv.reg_offsets)
>> +		return -EFAULT;
>> +
>> +	/* Read the SPM idle state sequences */
>> +	ret = msm_spm_seq_init(spm_dev, pdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Read the SPM register values */
>> +	for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
>> +		ret = of_property_read_u32(pdev->dev.of_node,
>> +					spm_of_data[i].key, &val);
>> +		if (ret)
>> +			continue;
>> +		writel_relaxed(val, spm_dev->drv.reg_base_addr +
>> +				spm_dev->drv.reg_offsets[spm_of_data[i].id]);
>> +	}
>
>Change this to explicit parsing of each of prop and add proper read/modify/writes so we only change the things in the registers we should be touching
>
Well these registers are complete writes, so i am thinking, i could
create a function and call that function with different arguments, No?

>> +
>> +	/* Flush all writes */
>> +	wmb();
>> +
>> +	spm_dev->initialized = true;
>> +	return ret;
>> +fail:
>> +	dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id msm_spm_match_table[] __initconst = {
>> +	{.compatible = "qcom,spm-v2.1", .data = reg_offsets_saw2_v2_1},
>> +	{ },
>> +};
>> +
>> +
>> +static struct platform_driver msm_spm_device_driver = {
>> +	.probe = msm_spm_dev_probe,
>> +	.driver = {
>> +		.name = "spm",
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = msm_spm_match_table,
>> +	},
>> +};
>> +
>> +static int __init msm_spm_device_init(void)
>> +{
>> +	return platform_driver_register(&msm_spm_device_driver);
>> +}
>> +device_initcall(msm_spm_device_init);
>> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
>> new file mode 100644
>> index 0000000..29686ef
>> --- /dev/null
>> +++ b/include/soc/qcom/spm.h
>> @@ -0,0 +1,38 @@
>> +/* 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 {
>> +	MSM_SPM_MODE_DISABLED,
>> +	MSM_SPM_MODE_CLOCK_GATING,
>> +	MSM_SPM_MODE_RETENTION,
>> +	MSM_SPM_MODE_GDHS,
>> +	MSM_SPM_MODE_POWER_COLLAPSE,
>> +	MSM_SPM_MODE_NR
>> +};
>
>Why don’t we make this a named enum, than msm_spm_set_low_power_mode can take that enum
>>
Sure.
>> +
>> +struct msm_spm_device;
>> +
Need to remove this.

>> +#if defined(CONFIG_QCOM_PM)
>> +
>> +int msm_spm_set_low_power_mode(u32 mode);
>
>So this could become
>
>int qcom_spm_set_low_power_mode(enum qcom_spm_mode mode)
>
Agreed.

>> +
>> +#else
>> +
>> +static inline int msm_spm_set_low_power_mode(u32 mode)
>> +{ return -ENOSYS; }
>> +
>> +#endif  /* CONFIG_QCOM_PM */
>> +
>> +#endif  /* __QCOM_SPM_H */
>> --
>> 1.9.1
>>
>
>-- 
>Employee of Qualcomm Innovation Center, Inc.
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>

Lina Iyer Sept. 24, 2014, 7:01 p.m. UTC | #8

On Wed, Sep 24 2014 at 11:49 -0600, Josh Cartwright wrote:
>Hey Lina-
>
>A few comments inline:
>
>On Tue, Sep 23, 2014 at 05:51:17PM -0600, Lina Iyer wrote:
>> +++ b/drivers/soc/qcom/spm.c
>[..]
>> +
>> +static u32 reg_offsets_saw2_v2_1[MSM_SPM_REG_NR] = {
>
>const?
>
sure
>> +	[MSM_SPM_REG_SAW2_SECURE]		= 0x00,
>> +	[MSM_SPM_REG_SAW2_ID]			= 0x04,
>> +	[MSM_SPM_REG_SAW2_CFG]			= 0x08,
>> +	[MSM_SPM_REG_SAW2_SPM_STS]		= 0x0C,
>> +	[MSM_SPM_REG_SAW2_AVS_STS]		= 0x10,
>> +	[MSM_SPM_REG_SAW2_PMIC_STS]		= 0x14,
>> +	[MSM_SPM_REG_SAW2_RST]			= 0x18,
>> +	[MSM_SPM_REG_SAW2_VCTL]			= 0x1C,
>> +	[MSM_SPM_REG_SAW2_AVS_CTL]		= 0x20,
>> +	[MSM_SPM_REG_SAW2_AVS_LIMIT]		= 0x24,
>> +	[MSM_SPM_REG_SAW2_AVS_DLY]		= 0x28,
>> +	[MSM_SPM_REG_SAW2_AVS_HYSTERESIS]	= 0x2C,
>> +	[MSM_SPM_REG_SAW2_SPM_CTL]		= 0x30,
>> +	[MSM_SPM_REG_SAW2_SPM_DLY]		= 0x34,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_0]		= 0x40,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_1]		= 0x44,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_2]		= 0x48,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_3]		= 0x4C,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_4]		= 0x50,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_5]		= 0x54,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_6]		= 0x58,
>> +	[MSM_SPM_REG_SAW2_PMIC_DATA_7]		= 0x5C,
>> +	[MSM_SPM_REG_SAW2_SEQ_ENTRY]		= 0x80,
>> +	[MSM_SPM_REG_SAW2_VERSION]		= 0xFD0,
>> +};
>> +
>> +struct spm_of {
>> +	char *key;
>
>const char *key?
>
>> +	u32 id;
>> +};
>> +
>> +struct msm_spm_mode {
>> +	u32 mode;
>> +	u32 start_addr;
>> +};
>> +
>> +struct msm_spm_driver_data {
>> +	void __iomem *reg_base_addr;
>> +	u32 *reg_offsets;
>> +	struct msm_spm_mode *modes;
>> +	u32 num_modes;
>
>Why u32?
>
ssize_t, perhaps?

>Actually, the maximum modes is fixed, and really all you need to keep
>around is the start_addr per-mode (which is only 5 bits), and an
>additional bit indicating whether that mode is valid. I'd recommend
>folding msm_spm_mode into msm_spm_driver_data completely.  Something
>like this, maybe:
>
>	struct msm_spm_driver_data {
>		void __iomem *reg_base_addr;
>		const u32 *reg_offsets;
>		struct {
>			u8 is_valid;
>			u8 start_addr;
>		} modes[MSM_SPM_MODE_NR];
>	};
>
Sure, I thought about it, but between the MSM_SPM_MODE is a common
enumeration for cpus and L2. L2 can do additional low power mode - like
GDHS (Globally Distributed Head Switch off) which retains memory, which
do not exist for the cpu. Ends up consuming a lot of memory for each SPM
instance. May be with u8, that may be a lesser impact.

>> +};
>> +
>> +struct msm_spm_device {
>> +	bool initialized;
>> +	struct msm_spm_driver_data drv;
>> +};
>> +
>> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);
>
>Why have both msm_spm_device and msm_spm_driver_data?
>
Ah. the role of msm_spm_device is greatly simplified in this patch. The
structure also helps manage a collective of SPM, used in a list, when we
have L2s and CCIs and other device SPMs. Also voltage control representation would
be part of this structure.
But I could use pointers, without the need for initialized variables.

>Would it be easier if you instead used 'struct msm_spm_device *', and
>used NULL to indicate it has not been initialized?
>
>> +static const struct of_device_id msm_spm_match_table[] __initconst;
>
>Just move the table above probe.
>
It looked out of place above probe :(. Ah well, I wil remove the fwd declaration.

>> +
>> +static int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
>> +						u32 mode)
>> +{
>> +	int i;
>> +	u32 start_addr = 0;
>> +	u32 ctl_val;
>> +
>> +	for (i = 0; i < drv->num_modes; i++) {
>> +		if (drv->modes[i].mode == mode) {
>> +			start_addr = drv->modes[i].start_addr;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (i == drv->num_modes)
>> +		return -EINVAL;
>> +
>> +	/* Update bits 10:4 in the SPM CTL register */
>> +	ctl_val = readl_relaxed(drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>> +	start_addr &= 0x7F;
>> +	start_addr <<= 4;
>> +	ctl_val &= 0xFFFFF80F;
>> +	ctl_val |= start_addr;
>> +	writel_relaxed(ctl_val, drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>> +	/* Ensure we have written the start address */
>> +	wmb();
>> +
>> +	return 0;
>> +}
>> +
>> +static int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv,
>> +					bool enable)
>> +{
>> +	u32 value = enable ? 0x01 : 0x00;
>> +	u32 ctl_val;
>> +
>> +	ctl_val = readl_relaxed(drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>> +
>> +	/* Update SPM_CTL to enable/disable the SPM */
>> +	if ((ctl_val & SPM_CTL_ENABLE) != value) {
>> +		/* Clear the existing value and update */
>> +		ctl_val &= ~0x1;
>> +		ctl_val |= value;
>> +		writel_relaxed(ctl_val, drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
>> +
>> +		/* Ensure we have enabled/disabled before returning */
>> +		wmb();
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
>> + * @mode: SPM LPM mode to enter
>> + */
>> +int msm_spm_set_low_power_mode(u32 mode)
>> +{
>> +	struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
>> +	int ret = -EINVAL;
>> +
>> +	if (!dev->initialized)
>> +		return -ENXIO;
>> +
>> +	if (mode == MSM_SPM_MODE_DISABLED)
>> +		ret = msm_spm_drv_set_spm_enable(&dev->drv, false);
>
>I would suggest not modeling "DISABLED" as a "mode", as it's not a state
>like the others.  Instead, perhaps you could expect users to call
>msm_spm_drv_set_spm_enable() directly.
>
>> +	else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
>> +		ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(msm_spm_set_low_power_mode);
>
>Is this actually to be used by modules?
>
Nope.. Just msm-pm.c, which should be renamed to qcom-pm.c

>[..]
>> +static int msm_spm_seq_init(struct msm_spm_device *spm_dev,
>> +			struct platform_device *pdev)
>> +{
>> +	int i;
>> +	u8 *cmd;
>
>const u8 *cmd; will save you the cast below.
>
ok

>> +	void *addr;
>> +	u32 val;
>> +	u32 count = 0;
>> +	int offset = 0;
>> +	struct msm_spm_mode modes[MSM_SPM_MODE_NR];
>> +	u32 sequences[NUM_SEQ_ENTRY/4] = {0};
>> +	struct msm_spm_driver_data *drv = &spm_dev->drv;
>> +
>> +	/* SPM sleep sequences */
>> +	struct spm_of mode_of_data[] = {
>
>static const?
>
OK
>> +		{"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
>> +		{"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
>> +	};
>> +
>> +	/**
>> +	 * Compose the u32 array based on the individual bytes of the SPM
>> +	 * sequence for each low power mode that we read from the DT.
>> +	 * The sequences are appended if there is space available in the
>> +	 * u32 after the end of the previous sequence.
>> +	 */
>> +
>> +	for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
>> +		cmd = (u8 *)of_get_property(pdev->dev.of_node,
>> +						mode_of_data[i].key, &val);
>> +		if (!cmd)
>> +			continue;
>> +		/* The last in the sequence should be 0x0F */
>> +		if (cmd[val - 1] != 0x0F)
>> +			continue;
>> +		modes[count].mode = mode_of_data[i].id;
>> +		modes[count].start_addr = offset;
>> +		append_seq_data(&sequences[0], cmd, &offset);
>> +		count++;
>> +	}
>> +
>> +	/* Write the idle state sequences to SPM */
>> +	drv->modes = devm_kcalloc(&pdev->dev, count,
>> +					sizeof(modes[0]), GFP_KERNEL);
>> +	if (!drv->modes)
>> +		return -ENOMEM;
>> +
>> +	drv->num_modes = count;
>> +	memcpy(drv->modes, modes, sizeof(modes[0]) * count);
>> +
>> +	/* Flush the integer array */
>> +	addr = drv->reg_base_addr +
>> +			drv->reg_offsets[MSM_SPM_REG_SAW2_SEQ_ENTRY];
>> +	for (i = 0; i < ARRAY_SIZE(sequences); i++, addr += 4)
>> +		writel_relaxed(sequences[i], addr);
>> +
>> +	/* Ensure we flush the writes */
>> +	wmb();
>> +
>> +	return 0;
>> +}
>> +
>> +static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
>> +{
>> +	struct msm_spm_device *dev = NULL;
>> +	struct device_node *cpu_node;
>> +	u32 cpu;
>> +
>> +	cpu_node = of_parse_phandle(pdev->dev.of_node, "qcom,cpu", 0);
>> +	if (cpu_node) {
>> +		for_each_possible_cpu(cpu) {
>> +			if (of_get_cpu_node(cpu, NULL) == cpu_node)
>> +				dev = &per_cpu(msm_cpu_spm_device, cpu);
>> +		}
>> +	}
>> +
>> +	return dev;
>> +}
>> +
>> +static int msm_spm_dev_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	int i;
>> +	u32 val;
>> +	struct msm_spm_device *spm_dev;
>> +	struct resource *res;
>> +	const struct of_device_id *match_id;
>> +
>> +	/* SPM Configuration registers */
>> +	struct spm_of spm_of_data[] = {
>
>static const?
>
OK

>> +		{"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
>> +		{"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
>> +		{"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
>> +	};
>> +
>> +	 /* Get the right SPM device */
>> +	spm_dev = msm_spm_get_device(pdev);
>> +	if (IS_ERR_OR_NULL(spm_dev))
>
>Should this just be a simple NULL check?
>
Yeah, that should go, this is a reminiscent of the previous
implementation.

>> +		return -EINVAL;
>> +
>> +	/* Get the SPM start address */
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		ret = -EINVAL;
>> +		goto fail;
>> +	}
>> +	spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
>> +					resource_size(res));
>
>devm_ioremap_resource()?
>
Yes. sure.

>> +	if (!spm_dev->drv.reg_base_addr) {
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>> +
>> +	match_id = of_match_node(msm_spm_match_table, pdev->dev.of_node);
>> +	if (!match_id)
>> +		return -ENODEV;
>> +
>> +	/* Use the register offsets for the SPM version in use */
>> +	spm_dev->drv.reg_offsets = (u32 *)match_id->data;
>> +	if (!spm_dev->drv.reg_offsets)
>> +		return -EFAULT;
>> +
>> +	/* Read the SPM idle state sequences */
>> +	ret = msm_spm_seq_init(spm_dev, pdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Read the SPM register values */
>> +	for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
>> +		ret = of_property_read_u32(pdev->dev.of_node,
>> +					spm_of_data[i].key, &val);
>> +		if (ret)
>> +			continue;
>> +		writel_relaxed(val, spm_dev->drv.reg_base_addr +
>> +				spm_dev->drv.reg_offsets[spm_of_data[i].id]);
>> +	}
>> +
>> +	/* Flush all writes */
>
>This isn't very descriptive.  Perhaps:
>
>/*
> * Ensure all observers see the above register writes before the
> * updating of spm_dev->initialized
> */
>
ok

>> +	wmb();
>> +
>> +	spm_dev->initialized = true;
>> +	return ret;
>> +fail:
>> +	dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id msm_spm_match_table[] __initconst = {
>> +	{.compatible = "qcom,spm-v2.1", .data = reg_offsets_saw2_v2_1},
>> +	{ },
>> +};
>> +
>> +
>> +static struct platform_driver msm_spm_device_driver = {
>> +	.probe = msm_spm_dev_probe,
>> +	.driver = {
>> +		.name = "spm",
>> +		.owner = THIS_MODULE,
>
>This assignment is not necessary.
>
agreed.

>> +		.of_match_table = msm_spm_match_table,
>> +	},
>> +};
>> +
>> +static int __init msm_spm_device_init(void)
>> +{
>> +	return platform_driver_register(&msm_spm_device_driver);
>> +}
>> +device_initcall(msm_spm_device_init);
>> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
>> new file mode 100644
>> index 0000000..29686ef
>> --- /dev/null
>> +++ b/include/soc/qcom/spm.h
>> @@ -0,0 +1,38 @@
>> +/* 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 {
>> +	MSM_SPM_MODE_DISABLED,
>> +	MSM_SPM_MODE_CLOCK_GATING,
>> +	MSM_SPM_MODE_RETENTION,
>> +	MSM_SPM_MODE_GDHS,
>> +	MSM_SPM_MODE_POWER_COLLAPSE,
>> +	MSM_SPM_MODE_NR
>> +};
>
>Is there a particular reason you aren't naming this enumeration, and
>using it's type in msm_spm_set_low_power_mode()?
>
Will change.

>> +
>> +struct msm_spm_device;
>
>Why this forward declaration?
>
This should go.

>> +
>> +#if defined(CONFIG_QCOM_PM)
>> +
>> +int msm_spm_set_low_power_mode(u32 mode);
>> +
>> +#else
>> +
>> +static inline int msm_spm_set_low_power_mode(u32 mode)
>> +{ return -ENOSYS; }
>> +
>> +#endif  /* CONFIG_QCOM_PM */
>> +
>> +#endif  /* __QCOM_SPM_H */
>> --
>> 1.9.1
>>
>
>Thanks,
>  Josh
>
Thanks for your patience in reviewing the code.

Lina

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

Lina Iyer Sept. 24, 2014, 7:29 p.m. UTC | #9

On Wed, Sep 24 2014 at 11:53 -0600, Kumar Gala wrote:
>
>On Sep 24, 2014, at 12:21 PM, Lina Iyer <[e-mail]> wrote:
>
>> On Wed, Sep 24 2014 at 10:33 -0600, Kumar Gala wrote:
>>>
>>> On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> 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 <[e-mail]>
>>>> [lina: simplify the driver for initial submission, clean up and update
>>>> commit text]
>>>> ---
>>>> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
>>>> drivers/soc/qcom/Kconfig                          |   8 +
>>>> drivers/soc/qcom/Makefile                         |   1 +
>>>> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
>>>> include/soc/qcom/spm.h                            |  38 +++
>>>> 5 files changed, 478 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
>>>> create mode 100644 drivers/soc/qcom/spm.c
>>>> create mode 100644 include/soc/qcom/spm.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>>> new file mode 100644
>>>> index 0000000..2ff2454
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>>> @@ -0,0 +1,43 @@
>>>> +* Subsystem Power Manager (SPM)
>>>> +
>>>> +Qualcomm Snapdragons have SPM hardware blocks to control the Application
>>>> +Processor Sub-System power. These SPM blocks run individual state machine
>>>> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI
>>>> +instruction is executed by the core.
>>>> +
>>>> +The devicetree representation of the SPM block should be:
>>>> +
>>>> +Required properties
>>>> +
>>>> +- compatible: Must be -
>>>> +		 "qcom,spm-v2.1"
>>>> +- reg: The physical address and the size of the SPM's memory mapped registers
>>>> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>>>> +	This field is required on only for SPMs that control the CPU.
>>>
>>> Let’s make this just cpu-handle instead of qcom,cpu.  The concept of a handle to a cpu is pretty generic.
>>>
>> Okay. Will look into it.
>> You mean just the property name, right?
>
>Correct.
>
>>
>>>> +- qcom,saw2-clk-div: SAW2 configuration register to program the SPM runtime
>>>> +	clocks. The register for this property is MSM_SPM_REG_SAW2_CFG.
>>>
>>> (add details on how this is used to compute timer tick.  Is it timer tick = saw_clk/saw2-clk-div?  What is valid range of values)
>>>
>> The SPM spec is not available for open use. The range of values is
>> irrelevant for the SPM clocks, usually, its a constant for an SoC, but
>> may vary between the SoC. Its how the SPM on the SoC interprets it.
>
>Does the meaning of the divisor value change from SoC to SoC (not the value itself)?
>
>Is it not always:
>
>timer tick = sys_ref_clk / (qcom,saw2-clk-div + 1)
>
It is, in this case. But its not something you deviate from the spec.

>>>> +- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
>>>> +	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.
>>>
>>> Didn’t Stephen asked about splitting this up? Or at least treating it as an array of 3 values?
>>>
>> Yes he did. My response was similar to the clk-div values, its not
>> something you can change without hardware spec documentation.
>> And I need to mix the three values up, anyways before I write to the
>> register. Splitting it up, doesnt help understanding/configuring the SPM
>> any better, so didnt change it.
>
>Hmm, will this value change from SPM to SPM on the same SoC?  I’m not a fan of allowing random register values to get poked into the HW from DT.  While this one case might end up being acceptable, its a terrible practice and not something I want use in the habit of doing.
>
Ah. Tough proposition! The SPM sequence is a bunch of random register
values, which is not open to interpretation without the programming
guides. I am not sure how I can use DT for saving all the register data
then.

I agree its nice to have nice readable parameters in the DT, but, isnt
the purpose of the DT, the hardware configuration? In an alternate way
to do this, I could put all these register writes into the driver itself
for each SoC. Ugly as it may be, it would solve the problem. However,
the device node then just has the compatible string in it and may be
some configurable elements. I fail to see the big picture of the use of
DT in such a case.

FWIW, I do understand your stance with DT, and for the most part agree
with it.


>>>> +- qcom,saw2-enable: The SPM control register to enable/disable the sleep state
>>>> +	machine. The register for this property is MSM_SPM_REG_SAW2_SPM_CTL.
>>>
>>> Can this just be a boolean (exist or not), if so, probably change it to qcom,saw2-disable (so lack of property means enable)?
>>>
>> Okay, sure.
>>
>>>> +
>>>> +Optional properties
>>>> +
>>>> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>>>
>>> probably add something like: “array of bytes …” (want to convey the data type somehow, is there a max length?)
>>>
>> Okay.
>>
>>>> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>>>
>>> probably add something like: “array of bytes …” (want to convey the data type somehow, is there a max length?)
>>>
>> Okay.
>>
>>>> +
>>>> +Example:
>>>> +	spm@f9089000 {
>>>> +		compatible = "qcom,spm-v2.1";
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <1>;
>>>> +		reg = <0xf9089000 0x1000>;
>>>> +		qcom,cpu = <&CPU0>;
>>>> +		qcom,saw2-clk-div = <0x1>;
>>>> +		qcom,saw2-delays = <0x20000400>;
>>>> +		qcom,saw2-enable = <0x1>;
>>>> +		qcom,saw2-spm-cmd-wfi = [03 0b 0f];
>>>> +		qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
>>>> +				a0 b0 03 68 70 3b 92 a0 b0
>>>> +				82 2b 50 10 30 02 22 30 0f];
>>>> +	};
>>>
>>> - k
>>>
>>>
>>> --
>>> Employee of Qualcomm Innovation Center, Inc.
>>> 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 [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>-- 
>Employee of Qualcomm Innovation Center, Inc.
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>

Lina Iyer Sept. 26, 2014, 2:45 p.m. UTC | #10

On Wed, Sep 24 2014 at 13:30 -0600, Lina Iyer wrote:
>On Wed, Sep 24 2014 at 11:53 -0600, Kumar Gala wrote:
>>
>>On Sep 24, 2014, at 12:21 PM, Lina Iyer <[e-mail]> wrote:
>>
>>>On Wed, Sep 24 2014 at 10:33 -0600, Kumar Gala wrote:
>>>>
>>>>On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> wrote:
>>>>
>>>>+- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
>>>>>+	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.
>>>>
>>>>Didn’t Stephen asked about splitting this up? Or at least treating it as an array of 3 values?
>>>>
>>>Yes he did. My response was similar to the clk-div values, its not
>>>something you can change without hardware spec documentation.
>>>And I need to mix the three values up, anyways before I write to the
>>>register. Splitting it up, doesnt help understanding/configuring the SPM
>>>any better, so didnt change it.
>>
>>Hmm, will this value change from SPM to SPM on the same SoC?  I’m not a fan of allowing random register values to get poked into the HW from DT.  While this one case might end up being acceptable, its a terrible practice and not something I want use in the habit of doing.
>>
>Ah. Tough proposition! The SPM sequence is a bunch of random register
>values, which is not open to interpretation without the programming
>guides. I am not sure how I can use DT for saving all the register data
>then.
>
>I agree its nice to have nice readable parameters in the DT, but, isnt
>the purpose of the DT, the hardware configuration? In an alternate way
>to do this, I could put all these register writes into the driver itself
>for each SoC. Ugly as it may be, it would solve the problem. However,
>the device node then just has the compatible string in it and may be
>some configurable elements. I fail to see the big picture of the use of
>DT in such a case.
>
>FWIW, I do understand your stance with DT, and for the most part agree
>with it.
>
Based on our offline discussion, I will make the changes to move these
proprietary register values into the driver. I will submit a patch with
the changes soon.

Thanks,
Lina.

Kumar Gala Sept. 26, 2014, 2:53 p.m. UTC | #11

On Sep 26, 2014, at 9:45 AM, Lina Iyer <[e-mail]> wrote:

> On Wed, Sep 24 2014 at 13:30 -0600, Lina Iyer wrote:
>> On Wed, Sep 24 2014 at 11:53 -0600, Kumar Gala wrote:
>>> 
>>> On Sep 24, 2014, at 12:21 PM, Lina Iyer <[e-mail]> wrote:
>>> 
>>>> On Wed, Sep 24 2014 at 10:33 -0600, Kumar Gala wrote:
>>>>> 
>>>>> On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> wrote:
>>>>> 
>>>>> +- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
>>>>>> +	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.
>>>>> 
>>>>> Didn’t Stephen asked about splitting this up? Or at least treating it as an array of 3 values?
>>>>> 
>>>> Yes he did. My response was similar to the clk-div values, its not
>>>> something you can change without hardware spec documentation.
>>>> And I need to mix the three values up, anyways before I write to the
>>>> register. Splitting it up, doesnt help understanding/configuring the SPM
>>>> any better, so didnt change it.
>>> 
>>> Hmm, will this value change from SPM to SPM on the same SoC?  I’m not a fan of allowing random register values to get poked into the HW from DT.  While this one case might end up being acceptable, its a terrible practice and not something I want use in the habit of doing.
>>> 
>> Ah. Tough proposition! The SPM sequence is a bunch of random register
>> values, which is not open to interpretation without the programming
>> guides. I am not sure how I can use DT for saving all the register data
>> then.
>> 
>> I agree its nice to have nice readable parameters in the DT, but, isnt
>> the purpose of the DT, the hardware configuration? In an alternate way
>> to do this, I could put all these register writes into the driver itself
>> for each SoC. Ugly as it may be, it would solve the problem. However,
>> the device node then just has the compatible string in it and may be
>> some configurable elements. I fail to see the big picture of the use of
>> DT in such a case.
>> 
>> FWIW, I do understand your stance with DT, and for the most part agree
>> with it.
>> 
> Based on our offline discussion, I will make the changes to move these
> proprietary register values into the driver. I will submit a patch with
> the changes soon.

Curious, do the command sequences vary SPM to SPM on the same SoC?  I’m guessing the L2 SPM sequence is probably different from the CPU SPM.

Also, stephen pointed out that the SPMs should probably be part of the SAW nodes and binding.

thanks

Lina Iyer Sept. 26, 2014, 3:07 p.m. UTC | #12

On Fri, Sep 26 2014 at 08:53 -0600, Kumar Gala wrote:
>
>On Sep 26, 2014, at 9:45 AM, Lina Iyer <[e-mail]> wrote:
>
>> On Wed, Sep 24 2014 at 13:30 -0600, Lina Iyer wrote:
>>> On Wed, Sep 24 2014 at 11:53 -0600, Kumar Gala wrote:
>>>>
>>>> On Sep 24, 2014, at 12:21 PM, Lina Iyer <[e-mail]> wrote:
>>>>
>>>>> On Wed, Sep 24 2014 at 10:33 -0600, Kumar Gala wrote:
>>>>>>
>>>>>> On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> wrote:
>>>>>>
>>>>>> +- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
>>>>>>> +	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.
>>>>>>
>>>>>> Didn’t Stephen asked about splitting this up? Or at least treating it as an array of 3 values?
>>>>>>
>>>>> Yes he did. My response was similar to the clk-div values, its not
>>>>> something you can change without hardware spec documentation.
>>>>> And I need to mix the three values up, anyways before I write to the
>>>>> register. Splitting it up, doesnt help understanding/configuring the SPM
>>>>> any better, so didnt change it.
>>>>
>>>> Hmm, will this value change from SPM to SPM on the same SoC?  I’m not a fan of allowing random register values to get poked into the HW from DT.  While this one case might end up being acceptable, its a terrible practice and not something I want use in the habit of doing.
>>>>
>>> Ah. Tough proposition! The SPM sequence is a bunch of random register
>>> values, which is not open to interpretation without the programming
>>> guides. I am not sure how I can use DT for saving all the register data
>>> then.
>>>
>>> I agree its nice to have nice readable parameters in the DT, but, isnt
>>> the purpose of the DT, the hardware configuration? In an alternate way
>>> to do this, I could put all these register writes into the driver itself
>>> for each SoC. Ugly as it may be, it would solve the problem. However,
>>> the device node then just has the compatible string in it and may be
>>> some configurable elements. I fail to see the big picture of the use of
>>> DT in such a case.
>>>
>>> FWIW, I do understand your stance with DT, and for the most part agree
>>> with it.
>>>
>> Based on our offline discussion, I will make the changes to move these
>> proprietary register values into the driver. I will submit a patch with
>> the changes soon.
>
>Curious, do the command sequences vary SPM to SPM on the same SoC?  I’m guessing the L2 SPM sequence is probably different from the CPU SPM.
Not really. CPUs are the mostly the same in an SoC but yes, they may
differ from the L2.

>
>Also, stephen pointed out that the SPMs should probably be part of the SAW nodes and binding.
Well, the SAW nodes are essentially regulator bindings. They have no
bearing on idle part of the SAW functionality.

SPM driver will provide an API to change voltage. SPM used to need to
know the voltage and will need to for 8064, but 8074 onwards, SPM can be
skipped in setting the voltage. It might be a tad bit faster to use SPM
to communicate to the PMIC, though.

For 8064, the voltage is turned off when powering down the core and
would need to be restored back to the original value when powering back
on. The value is captured in the PMIC_DATA registers and used by the
SPM sequence.

The SAW regulator information is not related to the SPM functionality
that I am implementing here.
It seems unnecessary to have two varied functionality be bound together
just because we need to shadow the register value. We could find better
ways to do that.

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

Kevin Hilman Sept. 26, 2014, 4:59 p.m. UTC | #13

Lina Iyer <[e-mail]> writes:

> 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 <[e-mail]>

[...]

> +enum {
> +	MSM_SPM_REG_SAW2_CFG,
> +	MSM_SPM_REG_SAW2_AVS_CTL,
> +	MSM_SPM_REG_SAW2_AVS_HYSTERESIS,
> +	MSM_SPM_REG_SAW2_SPM_CTL,
> +	MSM_SPM_REG_SAW2_PMIC_DLY,
> +	MSM_SPM_REG_SAW2_AVS_LIMIT,
> +	MSM_SPM_REG_SAW2_AVS_DLY,
> +	MSM_SPM_REG_SAW2_SPM_DLY,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_0,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_1,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_2,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_3,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_4,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_5,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_6,
> +	MSM_SPM_REG_SAW2_PMIC_DATA_7,
> +	MSM_SPM_REG_SAW2_RST,
> +
> +	MSM_SPM_REG_NR_INITIALIZE = MSM_SPM_REG_SAW2_RST,
> +
> +	MSM_SPM_REG_SAW2_ID,
> +	MSM_SPM_REG_SAW2_SECURE,
> +	MSM_SPM_REG_SAW2_STS0,
> +	MSM_SPM_REG_SAW2_STS1,
> +	MSM_SPM_REG_SAW2_STS2,
> +	MSM_SPM_REG_SAW2_VCTL,
> +	MSM_SPM_REG_SAW2_SEQ_ENTRY,
> +	MSM_SPM_REG_SAW2_SPM_STS,
> +	MSM_SPM_REG_SAW2_AVS_STS,
> +	MSM_SPM_REG_SAW2_PMIC_STS,
> +	MSM_SPM_REG_SAW2_VERSION,
> +
> +	MSM_SPM_REG_NR,
> +};

Do you really need the MSM_SPM_REG_ prefix on all of these?  As these
names are all local to the driver, the extra prefix isn't needed IMO.

Also, consdiering that SPM seems to be sub-block of SAW2, the fact that
the SPM shows up twice in some of the names is a bit confusing.

Kevin

Lina Iyer Sept. 26, 2014, 5:19 p.m. UTC | #14

On Fri, Sep 26 2014 at 10:59 -0600, Kevin Hilman wrote:
>Lina Iyer <[e-mail]> writes:
>
>> 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 <[e-mail]>
>
>[...]
>
>> +enum {
>> +	MSM_SPM_REG_SAW2_CFG,
>> +	MSM_SPM_REG_SAW2_AVS_CTL,
>> +	MSM_SPM_REG_SAW2_AVS_HYSTERESIS,

>Do you really need the MSM_SPM_REG_ prefix on all of these?  As these
>names are all local to the driver, the extra prefix isn't needed IMO.
>
Nope, I dont.. I will remove them.

>Also, consdiering that SPM seems to be sub-block of SAW2, the fact that
>the SPM shows up twice in some of the names is a bit confusing.
>
I understand. I will fix that.

>

Kevin Hilman Sept. 26, 2014, 7:04 p.m. UTC | #15

Lina Iyer <[e-mail]> writes:

> On Wed, Sep 24 2014 at 12:07 -0600, Kumar Gala wrote:
>>
>>On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> 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 <[e-mail]>
>>> [lina: simplify the driver for initial submission, clean up and update
>>> commit text]
>>> ---
>>> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
>>> drivers/soc/qcom/Kconfig                          |   8 +
>>> drivers/soc/qcom/Makefile                         |   1 +
>>> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
>>> include/soc/qcom/spm.h                            |  38 +++
>>> 5 files changed, 478 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
>>> create mode 100644 drivers/soc/qcom/spm.c
>>> create mode 100644 include/soc/qcom/spm.h
>>
>>General comment, lets use qcom instead of msm for various things.
>>
>>[snip]
>>
> OK, Done. I renamed all msm_ functions to qcom_ functions as well.
>

Does that apply to the other parts of this series too?  like the msm-pm
and cpuidle layers?

Kevin

Lina Iyer Sept. 26, 2014, 7:12 p.m. UTC | #16

On Fri, Sep 26 2014 at 13:04 -0600, Kevin Hilman wrote:
>Lina Iyer <[e-mail]> writes:
>
>> On Wed, Sep 24 2014 at 12:07 -0600, Kumar Gala wrote:
>>>
>>>On Sep 23, 2014, at 6:51 PM, Lina Iyer <[e-mail]> 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 <[e-mail]>
>>>> [lina: simplify the driver for initial submission, clean up and update
>>>> commit text]
>>>> ---
>>>> Documentation/devicetree/bindings/arm/msm/spm.txt |  43 +++
>>>> drivers/soc/qcom/Kconfig                          |   8 +
>>>> drivers/soc/qcom/Makefile                         |   1 +
>>>> drivers/soc/qcom/spm.c                            | 388 ++++++++++++++++++++++
>>>> include/soc/qcom/spm.h                            |  38 +++
>>>> 5 files changed, 478 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
>>>> create mode 100644 drivers/soc/qcom/spm.c
>>>> create mode 100644 include/soc/qcom/spm.h
>>>
>>>General comment, lets use qcom instead of msm for various things.
>>>
>>>[snip]
>>>
>> OK, Done. I renamed all msm_ functions to qcom_ functions as well.
>>
>
>Does that apply to the other parts of this series too?  like the msm-pm
>and cpuidle layers?
>
Yes, there are no more msm's anymore :)

>Kevin
>
>

Kevin Hilman Sept. 26, 2014, 7:30 p.m. UTC | #17

Lina Iyer <[e-mail]> writes:

> On Fri, Sep 26 2014 at 13:04 -0600, Kevin Hilman wrote:
>>Lina Iyer <[e-mail]> writes:
>>
>>> On Wed, Sep 24 2014 at 12:07 -0600, Kumar Gala wrote:

[...]

>>>>
>>>>General comment, lets use qcom instead of msm for various things.
>>>>
>>>>[snip]
>>>>
>>> OK, Done. I renamed all msm_ functions to qcom_ functions as well.
>>>
>>
>>Does that apply to the other parts of this series too?  like the msm-pm
>>and cpuidle layers?
>>
> Yes, there are no more msm's anymore :)
>

<can of worms>

What about Documentation/devicetree/bindings/arm/msm?

</can of worms>

Patch

diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
new file mode 100644
index 0000000..2ff2454
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
@@ -0,0 +1,43 @@ 
+* Subsystem Power Manager (SPM)
+
+Qualcomm Snapdragons have SPM hardware blocks to control the Application
+Processor Sub-System power. These SPM blocks run individual state machine
+to determine what the core (L2 or Krait/Scorpion) would do when the WFI
+instruction is executed by the core.
+
+The devicetree representation of the SPM block should be:
+
+Required properties
+
+- compatible: Must be -
+		 "qcom,spm-v2.1"
+- reg: The physical address and the size of the SPM's memory mapped registers
+- qcom,cpu: phandle for the CPU that the SPM block is attached to.
+	This field is required on only for SPMs that control the CPU.
+- qcom,saw2-clk-div: SAW2 configuration register to program the SPM runtime
+	clocks. The register for this property is MSM_SPM_REG_SAW2_CFG.
+- qcom,saw2-delays: The SPM delay values that SPM sequences would refer to.
+	The register for this property is MSM_SPM_REG_SAW2_SPM_DLY.
+- qcom,saw2-enable: The SPM control register to enable/disable the sleep state
+	machine. The register for this property is MSM_SPM_REG_SAW2_SPM_CTL.
+
+Optional properties
+
+- qcom,saw2-spm-cmd-wfi: The WFI command sequence
+- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
+
+Example:
+	spm@f9089000 {
+		compatible = "qcom,spm-v2.1";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0xf9089000 0x1000>;
+		qcom,cpu = <&CPU0>;
+		qcom,saw2-clk-div = <0x1>;
+		qcom,saw2-delays = <0x20000400>;
+		qcom,saw2-enable = <0x1>;
+		qcom,saw2-spm-cmd-wfi = [03 0b 0f];
+		qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
+				a0 b0 03 68 70 3b 92 a0 b0
+				82 2b 50 10 30 02 22 30 0f];
+	};
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
+	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..1fa6a96
--- /dev/null
+++ b/drivers/soc/qcom/spm.c
@@ -0,0 +1,388 @@ 
+/* 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>
+
+#define NUM_SEQ_ENTRY 32
+#define SPM_CTL_ENABLE BIT(0)
+
+enum {
+	MSM_SPM_REG_SAW2_CFG,
+	MSM_SPM_REG_SAW2_AVS_CTL,
+	MSM_SPM_REG_SAW2_AVS_HYSTERESIS,
+	MSM_SPM_REG_SAW2_SPM_CTL,
+	MSM_SPM_REG_SAW2_PMIC_DLY,
+	MSM_SPM_REG_SAW2_AVS_LIMIT,
+	MSM_SPM_REG_SAW2_AVS_DLY,
+	MSM_SPM_REG_SAW2_SPM_DLY,
+	MSM_SPM_REG_SAW2_PMIC_DATA_0,
+	MSM_SPM_REG_SAW2_PMIC_DATA_1,
+	MSM_SPM_REG_SAW2_PMIC_DATA_2,
+	MSM_SPM_REG_SAW2_PMIC_DATA_3,
+	MSM_SPM_REG_SAW2_PMIC_DATA_4,
+	MSM_SPM_REG_SAW2_PMIC_DATA_5,
+	MSM_SPM_REG_SAW2_PMIC_DATA_6,
+	MSM_SPM_REG_SAW2_PMIC_DATA_7,
+	MSM_SPM_REG_SAW2_RST,
+
+	MSM_SPM_REG_NR_INITIALIZE = MSM_SPM_REG_SAW2_RST,
+
+	MSM_SPM_REG_SAW2_ID,
+	MSM_SPM_REG_SAW2_SECURE,
+	MSM_SPM_REG_SAW2_STS0,
+	MSM_SPM_REG_SAW2_STS1,
+	MSM_SPM_REG_SAW2_STS2,
+	MSM_SPM_REG_SAW2_VCTL,
+	MSM_SPM_REG_SAW2_SEQ_ENTRY,
+	MSM_SPM_REG_SAW2_SPM_STS,
+	MSM_SPM_REG_SAW2_AVS_STS,
+	MSM_SPM_REG_SAW2_PMIC_STS,
+	MSM_SPM_REG_SAW2_VERSION,
+
+	MSM_SPM_REG_NR,
+};
+
+static u32 reg_offsets_saw2_v2_1[MSM_SPM_REG_NR] = {
+	[MSM_SPM_REG_SAW2_SECURE]		= 0x00,
+	[MSM_SPM_REG_SAW2_ID]			= 0x04,
+	[MSM_SPM_REG_SAW2_CFG]			= 0x08,
+	[MSM_SPM_REG_SAW2_SPM_STS]		= 0x0C,
+	[MSM_SPM_REG_SAW2_AVS_STS]		= 0x10,
+	[MSM_SPM_REG_SAW2_PMIC_STS]		= 0x14,
+	[MSM_SPM_REG_SAW2_RST]			= 0x18,
+	[MSM_SPM_REG_SAW2_VCTL]			= 0x1C,
+	[MSM_SPM_REG_SAW2_AVS_CTL]		= 0x20,
+	[MSM_SPM_REG_SAW2_AVS_LIMIT]		= 0x24,
+	[MSM_SPM_REG_SAW2_AVS_DLY]		= 0x28,
+	[MSM_SPM_REG_SAW2_AVS_HYSTERESIS]	= 0x2C,
+	[MSM_SPM_REG_SAW2_SPM_CTL]		= 0x30,
+	[MSM_SPM_REG_SAW2_SPM_DLY]		= 0x34,
+	[MSM_SPM_REG_SAW2_PMIC_DATA_0]		= 0x40,
+	[MSM_SPM_REG_SAW2_PMIC_DATA_1]		= 0x44,
+	[MSM_SPM_REG_SAW2_PMIC_DATA_2]		= 0x48,
+	[MSM_SPM_REG_SAW2_PMIC_DATA_3]		= 0x4C,
+	[MSM_SPM_REG_SAW2_PMIC_DATA_4]		= 0x50,
+	[MSM_SPM_REG_SAW2_PMIC_DATA_5]		= 0x54,
+	[MSM_SPM_REG_SAW2_PMIC_DATA_6]		= 0x58,
+	[MSM_SPM_REG_SAW2_PMIC_DATA_7]		= 0x5C,
+	[MSM_SPM_REG_SAW2_SEQ_ENTRY]		= 0x80,
+	[MSM_SPM_REG_SAW2_VERSION]		= 0xFD0,
+};
+
+struct spm_of {
+	char *key;
+	u32 id;
+};
+
+struct msm_spm_mode {
+	u32 mode;
+	u32 start_addr;
+};
+
+struct msm_spm_driver_data {
+	void __iomem *reg_base_addr;
+	u32 *reg_offsets;
+	struct msm_spm_mode *modes;
+	u32 num_modes;
+};
+
+struct msm_spm_device {
+	bool initialized;
+	struct msm_spm_driver_data drv;
+};
+
+static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);
+
+static const struct of_device_id msm_spm_match_table[] __initconst;
+
+static int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
+						u32 mode)
+{
+	int i;
+	u32 start_addr = 0;
+	u32 ctl_val;
+
+	for (i = 0; i < drv->num_modes; i++) {
+		if (drv->modes[i].mode == mode) {
+			start_addr = drv->modes[i].start_addr;
+			break;
+		}
+	}
+
+	if (i == drv->num_modes)
+		return -EINVAL;
+
+	/* Update bits 10:4 in the SPM CTL register */
+	ctl_val = readl_relaxed(drv->reg_base_addr +
+			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
+	start_addr &= 0x7F;
+	start_addr <<= 4;
+	ctl_val &= 0xFFFFF80F;
+	ctl_val |= start_addr;
+	writel_relaxed(ctl_val, drv->reg_base_addr +
+			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
+	/* Ensure we have written the start address */
+	wmb();
+
+	return 0;
+}
+
+static int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv,
+					bool enable)
+{
+	u32 value = enable ? 0x01 : 0x00;
+	u32 ctl_val;
+
+	ctl_val = readl_relaxed(drv->reg_base_addr +
+			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
+
+	/* Update SPM_CTL to enable/disable the SPM */
+	if ((ctl_val & SPM_CTL_ENABLE) != value) {
+		/* Clear the existing value and update */
+		ctl_val &= ~0x1;
+		ctl_val |= value;
+		writel_relaxed(ctl_val, drv->reg_base_addr +
+			drv->reg_offsets[MSM_SPM_REG_SAW2_SPM_CTL]);
+
+		/* Ensure we have enabled/disabled before returning */
+		wmb();
+	}
+
+	return 0;
+}
+
+/**
+ * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
+ * @mode: SPM LPM mode to enter
+ */
+int msm_spm_set_low_power_mode(u32 mode)
+{
+	struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
+	int ret = -EINVAL;
+
+	if (!dev->initialized)
+		return -ENXIO;
+
+	if (mode == MSM_SPM_MODE_DISABLED)
+		ret = msm_spm_drv_set_spm_enable(&dev->drv, false);
+	else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
+		ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
+
+	return ret;
+}
+EXPORT_SYMBOL(msm_spm_set_low_power_mode);
+
+static void append_seq_data(u32 *reg_seq_entry, u8 *cmd, u32 *offset)
+{
+	u32 cmd_w;
+	u32 offset_w = *offset / 4;
+	u8 last_cmd;
+
+	while (1) {
+		int i;
+
+		cmd_w = 0;
+		last_cmd = 0;
+		cmd_w = reg_seq_entry[offset_w];
+
+		for (i = (*offset % 4); i < 4; i++) {
+			last_cmd = *(cmd++);
+			cmd_w |=  last_cmd << (i * 8);
+			(*offset)++;
+			if (last_cmd == 0x0f)
+				break;
+		}
+
+		reg_seq_entry[offset_w++] = cmd_w;
+		if (last_cmd == 0x0f)
+			break;
+	}
+}
+
+static int msm_spm_seq_init(struct msm_spm_device *spm_dev,
+			struct platform_device *pdev)
+{
+	int i;
+	u8 *cmd;
+	void *addr;
+	u32 val;
+	u32 count = 0;
+	int offset = 0;
+	struct msm_spm_mode modes[MSM_SPM_MODE_NR];
+	u32 sequences[NUM_SEQ_ENTRY/4] = {0};
+	struct msm_spm_driver_data *drv = &spm_dev->drv;
+
+	/* SPM sleep sequences */
+	struct spm_of mode_of_data[] = {
+		{"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
+		{"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
+	};
+
+	/**
+	 * Compose the u32 array based on the individual bytes of the SPM
+	 * sequence for each low power mode that we read from the DT.
+	 * The sequences are appended if there is space available in the
+	 * u32 after the end of the previous sequence.
+	 */
+
+	for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
+		cmd = (u8 *)of_get_property(pdev->dev.of_node,
+						mode_of_data[i].key, &val);
+		if (!cmd)
+			continue;
+		/* The last in the sequence should be 0x0F */
+		if (cmd[val - 1] != 0x0F)
+			continue;
+		modes[count].mode = mode_of_data[i].id;
+		modes[count].start_addr = offset;
+		append_seq_data(&sequences[0], cmd, &offset);
+		count++;
+	}
+
+	/* Write the idle state sequences to SPM */
+	drv->modes = devm_kcalloc(&pdev->dev, count,
+					sizeof(modes[0]), GFP_KERNEL);
+	if (!drv->modes)
+		return -ENOMEM;
+
+	drv->num_modes = count;
+	memcpy(drv->modes, modes, sizeof(modes[0]) * count);
+
+	/* Flush the integer array */
+	addr = drv->reg_base_addr +
+			drv->reg_offsets[MSM_SPM_REG_SAW2_SEQ_ENTRY];
+	for (i = 0; i < ARRAY_SIZE(sequences); i++, addr += 4)
+		writel_relaxed(sequences[i], addr);
+
+	/* Ensure we flush the writes */
+	wmb();
+
+	return 0;
+}
+
+static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
+{
+	struct msm_spm_device *dev = NULL;
+	struct device_node *cpu_node;
+	u32 cpu;
+
+	cpu_node = of_parse_phandle(pdev->dev.of_node, "qcom,cpu", 0);
+	if (cpu_node) {
+		for_each_possible_cpu(cpu) {
+			if (of_get_cpu_node(cpu, NULL) == cpu_node)
+				dev = &per_cpu(msm_cpu_spm_device, cpu);
+		}
+	}
+
+	return dev;
+}
+
+static int msm_spm_dev_probe(struct platform_device *pdev)
+{
+	int ret;
+	int i;
+	u32 val;
+	struct msm_spm_device *spm_dev;
+	struct resource *res;
+	const struct of_device_id *match_id;
+
+	/* SPM Configuration registers */
+	struct spm_of spm_of_data[] = {
+		{"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
+		{"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
+		{"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
+	};
+
+	 /* Get the right SPM device */
+	spm_dev = msm_spm_get_device(pdev);
+	if (IS_ERR_OR_NULL(spm_dev))
+		return -EINVAL;
+
+	/* Get the SPM start address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		ret = -EINVAL;
+		goto fail;
+	}
+	spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
+					resource_size(res));
+	if (!spm_dev->drv.reg_base_addr) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	match_id = of_match_node(msm_spm_match_table, pdev->dev.of_node);
+	if (!match_id)
+		return -ENODEV;
+
+	/* Use the register offsets for the SPM version in use */
+	spm_dev->drv.reg_offsets = (u32 *)match_id->data;
+	if (!spm_dev->drv.reg_offsets)
+		return -EFAULT;
+
+	/* Read the SPM idle state sequences */
+	ret = msm_spm_seq_init(spm_dev, pdev);
+	if (ret)
+		return ret;
+
+	/* Read the SPM register values */
+	for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
+		ret = of_property_read_u32(pdev->dev.of_node,
+					spm_of_data[i].key, &val);
+		if (ret)
+			continue;
+		writel_relaxed(val, spm_dev->drv.reg_base_addr +
+				spm_dev->drv.reg_offsets[spm_of_data[i].id]);
+	}
+
+	/* Flush all writes */
+	wmb();
+
+	spm_dev->initialized = true;
+	return ret;
+fail:
+	dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
+	return ret;
+}
+
+static const struct of_device_id msm_spm_match_table[] __initconst = {
+	{.compatible = "qcom,spm-v2.1", .data = reg_offsets_saw2_v2_1},
+	{ },
+};
+
+
+static struct platform_driver msm_spm_device_driver = {
+	.probe = msm_spm_dev_probe,
+	.driver = {
+		.name = "spm",
+		.owner = THIS_MODULE,
+		.of_match_table = msm_spm_match_table,
+	},
+};
+
+static int __init msm_spm_device_init(void)
+{
+	return platform_driver_register(&msm_spm_device_driver);
+}
+device_initcall(msm_spm_device_init);
diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
new file mode 100644
index 0000000..29686ef
--- /dev/null
+++ b/include/soc/qcom/spm.h
@@ -0,0 +1,38 @@ 
+/* 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 {
+	MSM_SPM_MODE_DISABLED,
+	MSM_SPM_MODE_CLOCK_GATING,
+	MSM_SPM_MODE_RETENTION,
+	MSM_SPM_MODE_GDHS,
+	MSM_SPM_MODE_POWER_COLLAPSE,
+	MSM_SPM_MODE_NR
+};
+
+struct msm_spm_device;
+
+#if defined(CONFIG_QCOM_PM)
+
+int msm_spm_set_low_power_mode(u32 mode);
+
+#else
+
+static inline int msm_spm_set_low_power_mode(u32 mode)
+{ return -ENOSYS; }
+
+#endif  /* CONFIG_QCOM_PM */
+
+#endif  /* __QCOM_SPM_H */