From mboxrd@z Thu Jan 1 00:00:00 1970 From: lina.iyer@linaro.org (Lina Iyer) Date: Wed, 24 Sep 2014 12:47:50 -0600 Subject: [PATCH v6 1/5] qcom: spm: Add Subsystem Power Manager driver In-Reply-To: <1F7E5951-7704-4ADE-A1D1-2E380FF2540A@codeaurora.org> References: <1411516281-58328-1-git-send-email-lina.iyer@linaro.org> <1411516281-58328-2-git-send-email-lina.iyer@linaro.org> <1F7E5951-7704-4ADE-A1D1-2E380FF2540A@codeaurora.org> Message-ID: <20140924184750.GA1004@ilina-mac.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Sep 24 2014 at 12:07 -0600, Kumar Gala wrote: > >On Sep 23, 2014, at 6:51 PM, Lina Iyer wrote: > >> Based on work by many authors, available at codeaurora.org >> >> SPM is a hardware block that controls the peripheral logic surrounding >> the application cores (cpu/l$). When the core executes WFI instruction, >> the SPM takes over the putting the core in low power state as >> configured. The wake up for the SPM is an interrupt at the GIC, which >> then completes the rest of low power mode sequence and brings the core >> out of low power mode. >> >> The SPM has a set of control registers that configure the SPMs >> individually based on the type of the core and the runtime conditions. >> SPM is a finite state machine block to which a sequence is provided and >> it interprets the bytes and executes them in sequence. Each low power >> mode that the core can enter into is provided to the SPM as a sequence. >> >> Configure the SPM to set the core (cpu or L2) into its low power mode, >> the index of the first command in the sequence is set in the SPM_CTL >> register. When the core executes ARM wfi instruction, it triggers the >> SPM state machine to start executing from that index. The SPM state >> machine waits until the interrupt occurs and starts executing the rest >> of the sequence until it hits the end of the sequence. The end of the >> sequence jumps the core out of its low power mode. >> >> Signed-off-by: Lina Iyer >> [lina: 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#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 >