| Message ID | 1417065854-37745-4-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, 2014-11-26 at 22:24 -0700, Lina Iyer wrote: > > - compatible: > @@ -14,10 +23,13 @@ PROPERTIES > Value type: <string> > Definition: shall contain "qcom,saw2". A more specific value should be > one of: Which driver is supposed to handle this property? > - "qcom,saw2-v1" > - "qcom,saw2-v1.1" > - "qcom,saw2-v2" > - "qcom,saw2-v2.1" > + "qcom,saw2-v1" > + "qcom,saw2-v1.1" > + "qcom,saw2-v2" > + "qcom,saw2-v2.1" Doesn't seems to be used or supported? > + "qcom,apq8064-saw2-v1.1-cpu" > + "qcom,msm8974-saw2-v2.1-cpu" > + "qcom,apq8084-saw2-v2.1-cpu" > <snip> > - regulator@2099000 { > + power-controller@2099000 { > compatible = "qcom,saw2"; > reg = <0x02099000 0x1000>, <0x02009000 0x1000>; > + regulator; > <snip> > +static const struct of_device_id spm_match_table[] = { > + { .compatible = "qcom,msm8974-saw2-v2.1-cpu", > + .data = &spm_reg_8974_8084_cpu }, > + { .compatible = "qcom,apq8084-saw2-v2.1-cpu", > + .data = &spm_reg_8974_8084_cpu }, > + { .compatible = "qcom,apq8064-saw2-v1.1-cpu", > + .data = &spm_reg_8064_cpu }, > + { }, > +}; > + Regards, Ivan
On 11/27/2014 06:24 AM, Lina Iyer wrote: > SPM is a hardware block that controls the peripheral logic surrounding > the application cores (cpu/l$). When the core executes WFI instruction, > the SPM takes over the putting the core in low power state as > configured. The wake up for the SPM is an interrupt at the GIC, which > then completes the rest of low power mode sequence and brings the core > out of low power mode. > > The SPM has a set of control registers that configure the SPMs > individually based on the type of the core and the runtime conditions. > SPM is a finite state machine block to which a sequence is provided and > it interprets the bytes and executes them in sequence. Each low power > mode that the core can enter into is provided to the SPM as a sequence. > > Configure the SPM to set the core (cpu or L2) into its low power mode, > the index of the first command in the sequence is set in the SPM_CTL > register. When the core executes ARM wfi instruction, it triggers the > SPM state machine to start executing from that index. The SPM state > machine waits until the interrupt occurs and starts executing the rest > of the sequence until it hits the end of the sequence. The end of the > sequence jumps the core out of its low power mode. > > Add support for an idle driver to set up the SPM to place the core in > Standby or Standalone power collapse mode when the core is idle. > > Based on work by: Mahesh Sivasubramanian <msivasub@codeaurora.org>, > Ai Li <ali@codeaurora.org>, Praveen Chidambaram <pchidamb@codeaurora.org> > Original tree available at - > git://codeaurora.org/quic/la/kernel/msm-3.10.git > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > Reviewed-by: Kevin Hilman <khilman@linaro.org> [ ... ] > +static struct spm_driver_data *spm_get_drv(struct platform_device *pdev, > + int *spm_cpu) > +{ > + struct spm_driver_data *drv = NULL; > + struct device_node *cpu_node, *saw_node; > + int cpu; > + bool found = false; > + > + for_each_possible_cpu(cpu) { > + cpu_node = of_cpu_device_node_get(cpu); > + if (!cpu_node) > + continue; > + saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0); > + if (saw_node) { > + if (saw_node == pdev->dev.of_node) > + found = true; > + of_node_put(saw_node); > + } > + of_node_put(cpu_node); > + if (found) > + break; > + } > + > + if (found) { > + drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); > + if (drv) > + *spm_cpu = cpu; > + } > + > + return drv; > +} > + > +static const struct of_device_id spm_match_table[] = { > + { .compatible = "qcom,msm8974-saw2-v2.1-cpu", > + .data = &spm_reg_8974_8084_cpu }, > + { .compatible = "qcom,apq8084-saw2-v2.1-cpu", > + .data = &spm_reg_8974_8084_cpu }, > + { .compatible = "qcom,apq8064-saw2-v1.1-cpu", > + .data = &spm_reg_8064_cpu }, > + { }, > +}; > + > +static const struct qcom_cpu_pm_ops lpm_ops = { > + .standby = qcom_cpu_standby, > + .spc = qcom_cpu_spc, > +}; > + > +static int spm_dev_probe(struct platform_device *pdev) > +{ > + struct spm_driver_data *drv; > + struct resource *res; > + const struct of_device_id *match_id; > + void __iomem *addr; > + int cpu = -EINVAL; > + static bool cpuidle_drv_init; ^^^^^^^^^ As already said in a previous comment, please find a way to remove that. > + const struct platform_device_info qcom_cpuidle_info = { > + .name = "qcom_cpuidle", > + .id = -1, > + .data = &lpm_ops, > + .size_data = sizeof(lpm_ops), > + }; > + > + drv = spm_get_drv(pdev, &cpu); > + if (!drv || cpu < 0) > + return -EINVAL; As already said in a previous comment, it is not possible to have "cpu < 0" with "drv != NULL", so except I am missing something the test should be: if (!drv) return -EINVAL; There is something wrong with the init sequence. Don't you find weird you have to backward search for the cpu belonging to the pdev each time the probe function is called ? > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + drv->reg_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(drv->reg_base)) > + return PTR_ERR(drv->reg_base); > + > + match_id = of_match_node(spm_match_table, pdev->dev.of_node); > + if (!match_id) > + return -ENODEV; > + > + drv->reg_data = match_id->data; > + > + /* Write the SPM sequences first.. */ > + addr = drv->reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY]; > + __iowrite32_copy(addr, drv->reg_data->seq, > + ARRAY_SIZE(drv->reg_data->seq) / 4); > + > + /* > + * ..and then the control registers. > + * On some SoC's if the control registers are written first and if the > + * CPU was held in reset, the reset signal could trigger the SPM state > + * machine, before the sequences are completely written. > + */ > + spm_register_write(drv, SPM_REG_CFG, drv->reg_data->spm_cfg); > + spm_register_write(drv, SPM_REG_DLY, drv->reg_data->spm_dly); > + spm_register_write(drv, SPM_REG_PMIC_DLY, drv->reg_data->pmic_dly); > + > + spm_register_write(drv, SPM_REG_PMIC_DATA_0, > + drv->reg_data->pmic_data[0]); > + spm_register_write(drv, SPM_REG_PMIC_DATA_1, > + drv->reg_data->pmic_data[1]); > + > + /* > + * Ensure all observers see the above register writes before the > + * cpuidle driver is allowed to use the SPM. > + */ > + wmb(); > + per_cpu(cpu_spm_drv, cpu) = drv; > + > + if (!cpuidle_drv_init) { > + platform_device_register_full(&qcom_cpuidle_info); > + cpuidle_drv_init = true; > + } > + > + return 0; > +} > + > +static struct platform_driver spm_driver = { > + .probe = spm_dev_probe, > + .driver = { > + .name = "saw", > + .of_match_table = spm_match_table, > + }, > +}; > + > +module_platform_driver(spm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("SAW power controller driver"); > +MODULE_ALIAS("platform:saw"); > diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h > new file mode 100644 > index 0000000..d9a56d7 > --- /dev/null > +++ b/include/soc/qcom/pm.h > @@ -0,0 +1,31 @@ > +/* > + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved. > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#ifndef __QCOM_PM_H > +#define __QCOM_PM_H > + > +enum pm_sleep_mode { > + PM_SLEEP_MODE_STBY, > + PM_SLEEP_MODE_RET, > + PM_SLEEP_MODE_SPC, > + PM_SLEEP_MODE_PC, > + PM_SLEEP_MODE_NR, > +}; > + > +struct qcom_cpu_pm_ops { > + int (*standby)(void *data); > + int (*spc)(void *data); > +}; > + > +#endif /* __QCOM_PM_H */ >
On Thu, Nov 27, 2014 at 05:24:07AM +0000, Lina Iyer wrote: [...] > +static int spm_set_low_power_mode(enum pm_sleep_mode mode) > +{ > + struct spm_driver_data *drv = per_cpu(cpu_spm_drv, > + smp_processor_id()); > + u32 start_index; > + u32 ctl_val; > + > + if (!drv) > + return -ENXIO; > + > + start_index = drv->reg_data->start_index[mode]; > + > + ctl_val = spm_register_read(drv, SPM_REG_SPM_CTL); > + ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT); > + ctl_val |= start_index << SPM_CTL_INDEX_SHIFT; > + ctl_val |= SPM_CTL_EN; > + spm_register_write(drv, SPM_REG_SPM_CTL, ctl_val); > + > + /* Ensure we have written the start address */ > + wmb(); Can you explain please what this wmb is meant to achieve ? If it is there to make sure the write made it to the SPM that's not a proper way to achieve it, the barrier ensures ordering, not write completion. > + > + return 0; > +} > + > +static int qcom_pm_collapse(unsigned long int unused) > +{ > + int ret; > + u32 flag; > + int cpu = smp_processor_id(); > + > + ret = scm_set_warm_boot_addr(cpu_resume, cpu); > + if (ret) > + return ret; > + > + flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK; > + scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag); > + > + /* > + * Returns here only if there was a pending interrupt and we did not > + * power down as a result. > + */ > + return 0; I know the return value is changed by the cpu_suspend implementation to signal error when not returning from cpu_resume, but you should not return 0 anyway. > +} > + > +static int qcom_cpu_standby(void *unused) > +{ > + int ret; > + > + ret = spm_set_low_power_mode(PM_SLEEP_MODE_STBY); > + if (ret) > + return ret; > + > + cpu_do_idle(); > + > + return 0; > +} > + > +static int qcom_cpu_spc(void *unused) > +{ int ret; > + > + ret = spm_set_low_power_mode(PM_SLEEP_MODE_STBY); > + if (ret) > + return ret; > + > + cpu_pm_enter(); > + cpu_suspend(0, qcom_pm_collapse); You must stash the return value and propagate it. > + cpu_pm_exit(); > + > + return 0; > +} > + > +static struct spm_driver_data *spm_get_drv(struct platform_device *pdev, > + int *spm_cpu) > +{ > + struct spm_driver_data *drv = NULL; > + struct device_node *cpu_node, *saw_node; > + int cpu; > + bool found = false; > + > + for_each_possible_cpu(cpu) { > + cpu_node = of_cpu_device_node_get(cpu); > + if (!cpu_node) > + continue; > + saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0); + if (saw_node) { + if (saw_node == pdev->dev.of_node) + found = true; + of_node_put(saw_node); + } can be: found = (saw_node == pdev->dev.of_node); of_node_put(saw_node); Thanks, Lorenzo > + of_node_put(cpu_node); > + if (found) > + break; > + } > + > + if (found) { > + drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); > + if (drv) > + *spm_cpu = cpu; > + } > + > + return drv; > +} > + > +static const struct of_device_id spm_match_table[] = { > + { .compatible = "qcom,msm8974-saw2-v2.1-cpu", > + .data = &spm_reg_8974_8084_cpu }, > + { .compatible = "qcom,apq8084-saw2-v2.1-cpu", > + .data = &spm_reg_8974_8084_cpu }, > + { .compatible = "qcom,apq8064-saw2-v1.1-cpu", > + .data = &spm_reg_8064_cpu }, > + { }, > +}; > + > +static const struct qcom_cpu_pm_ops lpm_ops = { > + .standby = qcom_cpu_standby, > + .spc = qcom_cpu_spc, > +}; > + > +static int spm_dev_probe(struct platform_device *pdev) > +{ > + struct spm_driver_data *drv; > + struct resource *res; > + const struct of_device_id *match_id; > + void __iomem *addr; > + int cpu = -EINVAL; > + static bool cpuidle_drv_init; > + const struct platform_device_info qcom_cpuidle_info = { > + .name = "qcom_cpuidle", > + .id = -1, > + .data = &lpm_ops, > + .size_data = sizeof(lpm_ops), > + }; > + > + drv = spm_get_drv(pdev, &cpu); > + if (!drv || cpu < 0) > + return -EINVAL; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + drv->reg_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(drv->reg_base)) > + return PTR_ERR(drv->reg_base); > + > + match_id = of_match_node(spm_match_table, pdev->dev.of_node); > + if (!match_id) > + return -ENODEV; > + > + drv->reg_data = match_id->data; > + > + /* Write the SPM sequences first.. */ > + addr = drv->reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY]; > + __iowrite32_copy(addr, drv->reg_data->seq, > + ARRAY_SIZE(drv->reg_data->seq) / 4); > + > + /* > + * ..and then the control registers. > + * On some SoC's if the control registers are written first and if the > + * CPU was held in reset, the reset signal could trigger the SPM state > + * machine, before the sequences are completely written. > + */ > + spm_register_write(drv, SPM_REG_CFG, drv->reg_data->spm_cfg); > + spm_register_write(drv, SPM_REG_DLY, drv->reg_data->spm_dly); > + spm_register_write(drv, SPM_REG_PMIC_DLY, drv->reg_data->pmic_dly); > + > + spm_register_write(drv, SPM_REG_PMIC_DATA_0, > + drv->reg_data->pmic_data[0]); > + spm_register_write(drv, SPM_REG_PMIC_DATA_1, > + drv->reg_data->pmic_data[1]); > + > + /* > + * Ensure all observers see the above register writes before the > + * cpuidle driver is allowed to use the SPM. > + */ > + wmb(); > + per_cpu(cpu_spm_drv, cpu) = drv; > + > + if (!cpuidle_drv_init) { > + platform_device_register_full(&qcom_cpuidle_info); > + cpuidle_drv_init = true; > + } > + > + return 0; > +} > + > +static struct platform_driver spm_driver = { > + .probe = spm_dev_probe, > + .driver = { > + .name = "saw", > + .of_match_table = spm_match_table, > + }, > +}; > + > +module_platform_driver(spm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("SAW power controller driver"); > +MODULE_ALIAS("platform:saw"); > diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h > new file mode 100644 > index 0000000..d9a56d7 > --- /dev/null > +++ b/include/soc/qcom/pm.h > @@ -0,0 +1,31 @@ > +/* > + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved. > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#ifndef __QCOM_PM_H > +#define __QCOM_PM_H > + > +enum pm_sleep_mode { > + PM_SLEEP_MODE_STBY, > + PM_SLEEP_MODE_RET, > + PM_SLEEP_MODE_SPC, > + PM_SLEEP_MODE_PC, > + PM_SLEEP_MODE_NR, > +}; > + > +struct qcom_cpu_pm_ops { > + int (*standby)(void *data); > + int (*spc)(void *data); > +}; > + > +#endif /* __QCOM_PM_H */ > -- > 2.1.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Ivan, On Thu, Nov 27 2014 at 01:43 -0700, Ivan T. Ivanov wrote: > >Hi, > >On Wed, 2014-11-26 at 22:24 -0700, Lina Iyer wrote: >> > >> - compatible: >> @@ -14,10 +23,13 @@ PROPERTIES >> Value type: <string> >> Definition: shall contain "qcom,saw2". A more specific value should be >> one of: > >Which driver is supposed to handle this property? > drivers/soc/qcom/spm.c - SAW power controller driver. >> - "qcom,saw2-v1" >> - "qcom,saw2-v1.1" >> - "qcom,saw2-v2" >> - "qcom,saw2-v2.1" >> + "qcom,saw2-v1" >> + "qcom,saw2-v1.1" >> + "qcom,saw2-v2" >> + "qcom,saw2-v2.1" > >Doesn't seems to be used or supported? > Used for identify the version of the SAW that does voltage changes. Any SAW that can change the voltage would use one of the above compatible strings. We only need a generic version number for regulator functionality as opposed to the ones used for idle control (like below), which needs to correctly identify the version number (saw2-v1.1), cpu/L2 (-cpu_variant of the SAW on a particlar SoC (qcom,apq/msmXXXX) to set the correct register values for that SoC. L2 SAW is currently not upstream or part of this series. Will follow suit, when we have the power domains to control cluster low power modes. The regulator functionality is currently not available upstream, as well for 8084 and 8074. Thanks, Lina >> + "qcom,apq8064-saw2-v1.1-cpu" >> + "qcom,msm8974-saw2-v2.1-cpu" >> + "qcom,apq8084-saw2-v2.1-cpu" >> > ><snip> > >> - regulator@2099000 { >> + power-controller@2099000 { >> compatible = "qcom,saw2"; >> reg = <0x02099000 0x1000>, <0x02009000 0x1000>; >> + regulator; >> > ><snip> > >> +static const struct of_device_id spm_match_table[] = { >> + { .compatible = "qcom,msm8974-saw2-v2.1-cpu", >> + .data = &spm_reg_8974_8084_cpu }, >> + { .compatible = "qcom,apq8084-saw2-v2.1-cpu", >> + .data = &spm_reg_8974_8084_cpu }, >> + { .compatible = "qcom,apq8064-saw2-v1.1-cpu", >> + .data = &spm_reg_8064_cpu }, >> + { }, >> +}; >> + > >Regards, >Ivan
On Thu, Nov 27 2014 at 01:52 -0700, Daniel Lezcano wrote: >On 11/27/2014 06:24 AM, Lina Iyer wrote: >+ static bool cpuidle_drv_init; > > ^^^^^^^^^ > >As already said in a previous comment, please find a way to remove that. > I will look into it. Stephen and I wanted the cpuidle driver to be probed only after any of the SPMs are ready. And possibly, only for that cpu, for which the SPM has been probed. To achieve the SPM -> CPUIDLE Device relation, I havent found a good way to do that. Without using CPUIDLE_MULTIPLE_DRIVERS, initializing each cpuidle device, separate from the cpuidle driver, requires that I update the cpuidle_driver->cpumask after probing each SPM device, to allow for only one driver and cpuidle devices only for the probed cpus. Using the cpuidle_register_driver(), resets the drv->refcnt in __cpuidle_driver_init. I may need something like this in the else clause of CPUIDLE_MULTIPLE_DRIVERS - static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) { struct cpuidle_driver *drv = cpuidle_curr_driver; if (drv && !cpumask_test_cpu(cpu, drv->cpumask)) drv = NULL; return drv; } void cpuidle_update_cpumask(struct cpumask *mask) { struct cpuidle_driver *drv; spin_lock(&cpuidle_driver_lock); drv = cpuidle_get_driver(); if (drv) drv->cpumask = mask ?: cpu_possible_mask; spin_unlock(&cpuidle_driver_lock); } With that, I could register cpuidle driver the first time when the mask changes from empty and consequent updates would just update the cpumask. (I am not sure, if I missed anything in this change). It just seemed far too invasive at this time, in lieu of the static bool. >>+ const struct platform_device_info qcom_cpuidle_info = { >>+ .name = "qcom_cpuidle", >>+ .id = -1, >>+ .data = &lpm_ops, >>+ .size_data = sizeof(lpm_ops), >>+ }; >>+ >>+ drv = spm_get_drv(pdev, &cpu); >>+ if (!drv || cpu < 0) >>+ return -EINVAL; > >As already said in a previous comment, it is not possible to have "cpu >< 0" with "drv != NULL", so except I am missing something the test >should be: > > if (!drv) > return -EINVAL; > Sorry, done. >There is something wrong with the init sequence. Don't you find weird >you have to backward search for the cpu belonging to the pdev each >time the probe function is called ? > > Well, it was that or the SPM device node pointing to the CPU that it references. It seems more common to have an iterator than the doubly linked device nodes. I dont have a strong preference either way, just chose the way that made device nodes easier. >>+ >>+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>+ drv->reg_base = devm_ioremap_resource(&pdev->dev, res); >>+ if (IS_ERR(drv->reg_base)) >>+ return PTR_ERR(drv->reg_base); >>+ >>+ match_id = of_match_node(spm_match_table, pdev->dev.of_node); >>+ if (!match_id) >>+ return -ENODEV; >>+ >>+ drv->reg_data = match_id->data; >>+ >>+ /* Write the SPM sequences first.. */ >>+ addr = drv->reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY]; >>+ __iowrite32_copy(addr, drv->reg_data->seq, >>+ ARRAY_SIZE(drv->reg_data->seq) / 4); >>+ >>+ /* >>+ * ..and then the control registers. >>+ * On some SoC's if the control registers are written first and if the >>+ * CPU was held in reset, the reset signal could trigger the SPM state >>+ * machine, before the sequences are completely written. >>+ */ >>+ spm_register_write(drv, SPM_REG_CFG, drv->reg_data->spm_cfg); >>+ spm_register_write(drv, SPM_REG_DLY, drv->reg_data->spm_dly); >>+ spm_register_write(drv, SPM_REG_PMIC_DLY, drv->reg_data->pmic_dly); >>+ >>+ spm_register_write(drv, SPM_REG_PMIC_DATA_0, >>+ drv->reg_data->pmic_data[0]); >>+ spm_register_write(drv, SPM_REG_PMIC_DATA_1, >>+ drv->reg_data->pmic_data[1]); >>+ >>+ /* >>+ * Ensure all observers see the above register writes before the >>+ * cpuidle driver is allowed to use the SPM. >>+ */ >>+ wmb(); >>+ per_cpu(cpu_spm_drv, cpu) = drv; >>+ >>+ if (!cpuidle_drv_init) { >>+ platform_device_register_full(&qcom_cpuidle_info); >>+ cpuidle_drv_init = true; >>+ } >>+ >>+ return 0; >>+} >>+ >>+static struct platform_driver spm_driver = { >>+ .probe = spm_dev_probe, >>+ .driver = { >>+ .name = "saw", >>+ .of_match_table = spm_match_table, >>+ }, >>+}; >>+ >>+module_platform_driver(spm_driver); >>+ >>+MODULE_LICENSE("GPL v2"); >>+MODULE_DESCRIPTION("SAW power controller driver"); >>+MODULE_ALIAS("platform:saw"); >>diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h >>new file mode 100644 >>index 0000000..d9a56d7 >>--- /dev/null >>+++ b/include/soc/qcom/pm.h >>@@ -0,0 +1,31 @@ >>+/* >>+ * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved. >>+ * >>+ * This software is licensed under the terms of the GNU General Public >>+ * License version 2, as published by the Free Software Foundation, and >>+ * may be copied, distributed, and modified under those terms. >>+ * >>+ * This program is distributed in the hope that it will be useful, >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>+ * GNU General Public License for more details. >>+ * >>+ */ >>+ >>+#ifndef __QCOM_PM_H >>+#define __QCOM_PM_H >>+ >>+enum pm_sleep_mode { >>+ PM_SLEEP_MODE_STBY, >>+ PM_SLEEP_MODE_RET, >>+ PM_SLEEP_MODE_SPC, >>+ PM_SLEEP_MODE_PC, >>+ PM_SLEEP_MODE_NR, >>+}; >>+ >>+struct qcom_cpu_pm_ops { >>+ int (*standby)(void *data); >>+ int (*spc)(void *data); >>+}; >>+ >>+#endif /* __QCOM_PM_H */ >> > > >-- > <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs > >Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | ><http://twitter.com/#!/linaroorg> Twitter | ><http://www.linaro.org/linaro-blog/> Blog >
On Thu, Nov 27 2014 at 08:01 -0700, Lorenzo Pieralisi wrote: >On Thu, Nov 27, 2014 at 05:24:07AM +0000, Lina Iyer wrote: > >[...] > >> +static int spm_set_low_power_mode(enum pm_sleep_mode mode) >> +{ >> + struct spm_driver_data *drv = per_cpu(cpu_spm_drv, >> + smp_processor_id()); >> + u32 start_index; >> + u32 ctl_val; >> + >> + if (!drv) >> + return -ENXIO; >> + >> + start_index = drv->reg_data->start_index[mode]; >> + >> + ctl_val = spm_register_read(drv, SPM_REG_SPM_CTL); >> + ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT); >> + ctl_val |= start_index << SPM_CTL_INDEX_SHIFT; >> + ctl_val |= SPM_CTL_EN; >> + spm_register_write(drv, SPM_REG_SPM_CTL, ctl_val); >> + >> + /* Ensure we have written the start address */ >> + wmb(); > >Can you explain please what this wmb is meant to achieve ? If it is there >to make sure the write made it to the SPM that's not a proper way >to achieve it, the barrier ensures ordering, not write completion. > A write completion would be great, but assuming that the context switch using SCM or WFI could ensure the flush, then ordering is probably good enough for this. Is my understanding incorrect? >> + >> + return 0; >> +} >> + >> +static int qcom_pm_collapse(unsigned long int unused) >> +{ >> + int ret; >> + u32 flag; >> + int cpu = smp_processor_id(); >> + >> + ret = scm_set_warm_boot_addr(cpu_resume, cpu); >> + if (ret) >> + return ret; >> + >> + flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK; >> + scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag); >> + >> + /* >> + * Returns here only if there was a pending interrupt and we did not >> + * power down as a result. >> + */ >> + return 0; > >I know the return value is changed by the cpu_suspend implementation to >signal error when not returning from cpu_resume, but you should not >return 0 anyway. > Sure. Will do. >> +} >> + >> +static int qcom_cpu_standby(void *unused) >> +{ >> + int ret; >> + >> + ret = spm_set_low_power_mode(PM_SLEEP_MODE_STBY); >> + if (ret) >> + return ret; >> + >> + cpu_do_idle(); >> + >> + return 0; >> +} >> + >> +static int qcom_cpu_spc(void *unused) >> +{ int ret; >> + >> + ret = spm_set_low_power_mode(PM_SLEEP_MODE_STBY); >> + if (ret) >> + return ret; >> + >> + cpu_pm_enter(); >> + cpu_suspend(0, qcom_pm_collapse); > >You must stash the return value and propagate it. > Sure. Will do. >> + cpu_pm_exit(); >> + >> + return 0; >> +} >> + >> +static struct spm_driver_data *spm_get_drv(struct platform_device *pdev, >> + int *spm_cpu) >> +{ >> + struct spm_driver_data *drv = NULL; >> + struct device_node *cpu_node, *saw_node; >> + int cpu; >> + bool found = false; >> + >> + for_each_possible_cpu(cpu) { >> + cpu_node = of_cpu_device_node_get(cpu); >> + if (!cpu_node) >> + continue; >> + saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0); > >+ if (saw_node) { >+ if (saw_node == pdev->dev.of_node) >+ found = true; >+ of_node_put(saw_node); >+ } > >can be: > found = (saw_node == pdev->dev.of_node); > of_node_put(saw_node); > Sure. >Thanks, >Lorenzo > >> + of_node_put(cpu_node); >> + if (found) >> + break; >> + } >> + >> + if (found) { >> + drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); >> + if (drv) >> + *spm_cpu = cpu; >> + } >> + >> + return drv; >> +} >> + >> +static const struct of_device_id spm_match_table[] = { >> + { .compatible = "qcom,msm8974-saw2-v2.1-cpu", >> + .data = &spm_reg_8974_8084_cpu }, >> + { .compatible = "qcom,apq8084-saw2-v2.1-cpu", >> + .data = &spm_reg_8974_8084_cpu }, >> + { .compatible = "qcom,apq8064-saw2-v1.1-cpu", >> + .data = &spm_reg_8064_cpu }, >> + { }, >> +}; >> + >> +static const struct qcom_cpu_pm_ops lpm_ops = { >> + .standby = qcom_cpu_standby, >> + .spc = qcom_cpu_spc, >> +}; >> + >> +static int spm_dev_probe(struct platform_device *pdev) >> +{ >> + struct spm_driver_data *drv; >> + struct resource *res; >> + const struct of_device_id *match_id; >> + void __iomem *addr; >> + int cpu = -EINVAL; >> + static bool cpuidle_drv_init; >> + const struct platform_device_info qcom_cpuidle_info = { >> + .name = "qcom_cpuidle", >> + .id = -1, >> + .data = &lpm_ops, >> + .size_data = sizeof(lpm_ops), >> + }; >> + >> + drv = spm_get_drv(pdev, &cpu); >> + if (!drv || cpu < 0) >> + return -EINVAL; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + drv->reg_base = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(drv->reg_base)) >> + return PTR_ERR(drv->reg_base); >> + >> + match_id = of_match_node(spm_match_table, pdev->dev.of_node); >> + if (!match_id) >> + return -ENODEV; >> + >> + drv->reg_data = match_id->data; >> + >> + /* Write the SPM sequences first.. */ >> + addr = drv->reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY]; >> + __iowrite32_copy(addr, drv->reg_data->seq, >> + ARRAY_SIZE(drv->reg_data->seq) / 4); >> + >> + /* >> + * ..and then the control registers. >> + * On some SoC's if the control registers are written first and if the >> + * CPU was held in reset, the reset signal could trigger the SPM state >> + * machine, before the sequences are completely written. >> + */ >> + spm_register_write(drv, SPM_REG_CFG, drv->reg_data->spm_cfg); >> + spm_register_write(drv, SPM_REG_DLY, drv->reg_data->spm_dly); >> + spm_register_write(drv, SPM_REG_PMIC_DLY, drv->reg_data->pmic_dly); >> + >> + spm_register_write(drv, SPM_REG_PMIC_DATA_0, >> + drv->reg_data->pmic_data[0]); >> + spm_register_write(drv, SPM_REG_PMIC_DATA_1, >> + drv->reg_data->pmic_data[1]); >> + >> + /* >> + * Ensure all observers see the above register writes before the >> + * cpuidle driver is allowed to use the SPM. >> + */ >> + wmb(); >> + per_cpu(cpu_spm_drv, cpu) = drv; >> + >> + if (!cpuidle_drv_init) { >> + platform_device_register_full(&qcom_cpuidle_info); >> + cpuidle_drv_init = true; >> + } >> + >> + return 0; >> +} >> + >> +static struct platform_driver spm_driver = { >> + .probe = spm_dev_probe, >> + .driver = { >> + .name = "saw", >> + .of_match_table = spm_match_table, >> + }, >> +}; >> + >> +module_platform_driver(spm_driver); >> + >> +MODULE_LICENSE("GPL v2"); >> +MODULE_DESCRIPTION("SAW power controller driver"); >> +MODULE_ALIAS("platform:saw"); >> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h >> new file mode 100644 >> index 0000000..d9a56d7 >> --- /dev/null >> +++ b/include/soc/qcom/pm.h >> @@ -0,0 +1,31 @@ >> +/* >> + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved. >> + * >> + * This software is licensed under the terms of the GNU General Public >> + * License version 2, as published by the Free Software Foundation, and >> + * may be copied, distributed, and modified under those terms. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> + >> +#ifndef __QCOM_PM_H >> +#define __QCOM_PM_H >> + >> +enum pm_sleep_mode { >> + PM_SLEEP_MODE_STBY, >> + PM_SLEEP_MODE_RET, >> + PM_SLEEP_MODE_SPC, >> + PM_SLEEP_MODE_PC, >> + PM_SLEEP_MODE_NR, >> +}; >> + >> +struct qcom_cpu_pm_ops { >> + int (*standby)(void *data); >> + int (*spc)(void *data); >> +}; >> + >> +#endif /* __QCOM_PM_H */ >> -- >> 2.1.0 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >-- >To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2014 07:50 PM, Lina Iyer wrote: > On Thu, Nov 27 2014 at 01:52 -0700, Daniel Lezcano wrote: >> On 11/27/2014 06:24 AM, Lina Iyer wrote: > >> + static bool cpuidle_drv_init; >> >> ^^^^^^^^^ >> >> As already said in a previous comment, please find a way to remove that. >> > I will look into it. Stephen and I wanted the cpuidle driver to be > probed only after any of the SPMs are ready. And possibly, only for that > cpu, for which the SPM has been probed. > > To achieve the SPM -> CPUIDLE Device relation, I havent found a good way > to do that. Without using CPUIDLE_MULTIPLE_DRIVERS, initializing each > cpuidle device, separate from the cpuidle driver, requires that I update > the cpuidle_driver->cpumask after probing each SPM device, to allow for > only one driver and cpuidle devices only for the probed cpus. Using the > cpuidle_register_driver(), resets the drv->refcnt in > __cpuidle_driver_init. > > I may need something like this in the else clause of > CPUIDLE_MULTIPLE_DRIVERS - > > static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) > { > struct cpuidle_driver *drv = cpuidle_curr_driver; > > if (drv && !cpumask_test_cpu(cpu, drv->cpumask)) > drv = NULL; > > return drv; > } You don't have to deal with the drv->cpumask and CPUIDLE_MULTIPLE_DRIVERS. This field gives the list of the cpus the driver will have to potentially handle. Just register the driver at the first place with the platform device by using cpuidle_register_driver(drv), I suggest somewhere else than the spm code. The underlying code will do: if (!drv->cpumask) drv->cpumask = (struct cpumask *)cpu_possible_mask; Until the cpuidle device is not initialized for a specific cpu, the cpuidle code will have no effect for this cpu. If you register the driver but without registering the cpuidle devices that will result in nothing more than a structure initialized but inoperative. For each SPM being probed: struct cpuidle_device *dev = &per_cpu(cpuidle_dev, cpu); dev->cpu = cpu; cpuidle_register_device(dev); Note cpuidle_dev is exported via cpuidle.h, so you don't have to redeclare a per cpu cpuidle device by your own. So rephrasing all that: (1) in cpuidle-qcom register the driver (2) in the spm code register the device for each spm probe (1) being called before (2). If after that in the code you still have a "static bool cpuidle_drv_init", then I guess we have a problem in the init sequence somewhere else. > void cpuidle_update_cpumask(struct cpumask *mask) { > struct cpuidle_driver *drv; > > spin_lock(&cpuidle_driver_lock); > drv = cpuidle_get_driver(); > if (drv) > drv->cpumask = mask ?: cpu_possible_mask; > spin_unlock(&cpuidle_driver_lock); > } > > With that, I could register cpuidle driver the first time when the mask > changes > from empty and consequent updates would just update the cpumask. (I am not > sure, if I missed anything in this change). It just seemed far too > invasive at > this time, in lieu of the static bool. > >>> + const struct platform_device_info qcom_cpuidle_info = { >>> + .name = "qcom_cpuidle", >>> + .id = -1, >>> + .data = &lpm_ops, >>> + .size_data = sizeof(lpm_ops), >>> + }; >>> + >>> + drv = spm_get_drv(pdev, &cpu); >>> + if (!drv || cpu < 0) >>> + return -EINVAL; >> >> As already said in a previous comment, it is not possible to have "cpu >> < 0" with "drv != NULL", so except I am missing something the test >> should be: >> >> if (!drv) >> return -EINVAL; >> > Sorry, done. > >> There is something wrong with the init sequence. Don't you find weird >> you have to backward search for the cpu belonging to the pdev each >> time the probe function is called ? >> >> > Well, it was that or the SPM device node pointing to the CPU that it > references. It seems more common to have an iterator than the doubly > linked device nodes. I dont have a strong preference either way, just > chose the way that made device nodes easier. > >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + drv->reg_base = devm_ioremap_resource(&pdev->dev, res); >>> + if (IS_ERR(drv->reg_base)) >>> + return PTR_ERR(drv->reg_base); >>> + >>> + match_id = of_match_node(spm_match_table, pdev->dev.of_node); >>> + if (!match_id) >>> + return -ENODEV; >>> + >>> + drv->reg_data = match_id->data; >>> + >>> + /* Write the SPM sequences first.. */ >>> + addr = drv->reg_base + >>> drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY]; >>> + __iowrite32_copy(addr, drv->reg_data->seq, >>> + ARRAY_SIZE(drv->reg_data->seq) / 4); >>> + >>> + /* >>> + * ..and then the control registers. >>> + * On some SoC's if the control registers are written first and >>> if the >>> + * CPU was held in reset, the reset signal could trigger the SPM >>> state >>> + * machine, before the sequences are completely written. >>> + */ >>> + spm_register_write(drv, SPM_REG_CFG, drv->reg_data->spm_cfg); >>> + spm_register_write(drv, SPM_REG_DLY, drv->reg_data->spm_dly); >>> + spm_register_write(drv, SPM_REG_PMIC_DLY, drv->reg_data->pmic_dly); >>> + >>> + spm_register_write(drv, SPM_REG_PMIC_DATA_0, >>> + drv->reg_data->pmic_data[0]); >>> + spm_register_write(drv, SPM_REG_PMIC_DATA_1, >>> + drv->reg_data->pmic_data[1]); >>> + >>> + /* >>> + * Ensure all observers see the above register writes before the >>> + * cpuidle driver is allowed to use the SPM. >>> + */ >>> + wmb(); >>> + per_cpu(cpu_spm_drv, cpu) = drv; >>> + >>> + if (!cpuidle_drv_init) { >>> + platform_device_register_full(&qcom_cpuidle_info); >>> + cpuidle_drv_init = true; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static struct platform_driver spm_driver = { >>> + .probe = spm_dev_probe, >>> + .driver = { >>> + .name = "saw", >>> + .of_match_table = spm_match_table, >>> + }, >>> +}; >>> + >>> +module_platform_driver(spm_driver); >>> + >>> +MODULE_LICENSE("GPL v2"); >>> +MODULE_DESCRIPTION("SAW power controller driver"); >>> +MODULE_ALIAS("platform:saw"); >>> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h >>> new file mode 100644 >>> index 0000000..d9a56d7 >>> --- /dev/null >>> +++ b/include/soc/qcom/pm.h >>> @@ -0,0 +1,31 @@ >>> +/* >>> + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved. >>> + * >>> + * This software is licensed under the terms of the GNU General Public >>> + * License version 2, as published by the Free Software Foundation, and >>> + * may be copied, distributed, and modified under those terms. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + * >>> + */ >>> + >>> +#ifndef __QCOM_PM_H >>> +#define __QCOM_PM_H >>> + >>> +enum pm_sleep_mode { >>> + PM_SLEEP_MODE_STBY, >>> + PM_SLEEP_MODE_RET, >>> + PM_SLEEP_MODE_SPC, >>> + PM_SLEEP_MODE_PC, >>> + PM_SLEEP_MODE_NR, >>> +}; >>> + >>> +struct qcom_cpu_pm_ops { >>> + int (*standby)(void *data); >>> + int (*spc)(void *data); >>> +}; >>> + >>> +#endif /* __QCOM_PM_H */ >>> >> >> >> -- >> <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs >> >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | >> <http://twitter.com/#!/linaroorg> Twitter | >> <http://www.linaro.org/linaro-blog/> Blog >>
On Mon, Dec 01, 2014 at 06:57:05PM +0000, Lina Iyer wrote: > On Thu, Nov 27 2014 at 08:01 -0700, Lorenzo Pieralisi wrote: > >On Thu, Nov 27, 2014 at 05:24:07AM +0000, Lina Iyer wrote: > > > >[...] > > > >> +static int spm_set_low_power_mode(enum pm_sleep_mode mode) > >> +{ > >> + struct spm_driver_data *drv = per_cpu(cpu_spm_drv, > >> + smp_processor_id()); > >> + u32 start_index; > >> + u32 ctl_val; > >> + > >> + if (!drv) > >> + return -ENXIO; > >> + > >> + start_index = drv->reg_data->start_index[mode]; > >> + > >> + ctl_val = spm_register_read(drv, SPM_REG_SPM_CTL); > >> + ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT); > >> + ctl_val |= start_index << SPM_CTL_INDEX_SHIFT; > >> + ctl_val |= SPM_CTL_EN; > >> + spm_register_write(drv, SPM_REG_SPM_CTL, ctl_val); > >> + > >> + /* Ensure we have written the start address */ > >> + wmb(); > > > >Can you explain please what this wmb is meant to achieve ? If it is there > >to make sure the write made it to the SPM that's not a proper way > >to achieve it, the barrier ensures ordering, not write completion. > > A write completion would be great, but assuming that the context switch > using SCM or WFI could ensure the flush, then ordering is probably good > enough for this. I assume you want the write to the device to complete (side-effects are observed) before the WFI (which would raise a StandbyWFI signal). The safer way is to read back from the device (or check/poll for the device state change) rather than a barrier.
On Tue, Dec 02 2014 at 02:53 -0700, Daniel Lezcano wrote: >On 12/01/2014 07:50 PM, Lina Iyer wrote: >>On Thu, Nov 27 2014 at 01:52 -0700, Daniel Lezcano wrote: >>>On 11/27/2014 06:24 AM, Lina Iyer wrote: >> >>>+ static bool cpuidle_drv_init; >>> >>> ^^^^^^^^^ >>> >>>As already said in a previous comment, please find a way to remove that. >>> >>I will look into it. Stephen and I wanted the cpuidle driver to be >>probed only after any of the SPMs are ready. And possibly, only for that >>cpu, for which the SPM has been probed. >> >>To achieve the SPM -> CPUIDLE Device relation, I havent found a good way >>to do that. Without using CPUIDLE_MULTIPLE_DRIVERS, initializing each >>cpuidle device, separate from the cpuidle driver, requires that I update >>the cpuidle_driver->cpumask after probing each SPM device, to allow for >>only one driver and cpuidle devices only for the probed cpus. Using the >>cpuidle_register_driver(), resets the drv->refcnt in >>__cpuidle_driver_init. >> >>I may need something like this in the else clause of >>CPUIDLE_MULTIPLE_DRIVERS - >> >>static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) >>{ >> struct cpuidle_driver *drv = cpuidle_curr_driver; >> >> if (drv && !cpumask_test_cpu(cpu, drv->cpumask)) >> drv = NULL; >> >> return drv; >>} > >You don't have to deal with the drv->cpumask and >CPUIDLE_MULTIPLE_DRIVERS. This field gives the list of the cpus the >driver will have to potentially handle. > >Just register the driver at the first place with the platform device >by using cpuidle_register_driver(drv), I suggest somewhere else than >the spm code. > >The underlying code will do: > > if (!drv->cpumask) > drv->cpumask = (struct cpumask *)cpu_possible_mask; > >Until the cpuidle device is not initialized for a specific cpu, the >cpuidle code will have no effect for this cpu. If you register the >driver but without registering the cpuidle devices that will result in >nothing more than a structure initialized but inoperative. > >For each SPM being probed: > >struct cpuidle_device *dev = &per_cpu(cpuidle_dev, cpu); >dev->cpu = cpu; >cpuidle_register_device(dev); > >Note cpuidle_dev is exported via cpuidle.h, so you don't have to >redeclare a per cpu cpuidle device by your own. > >So rephrasing all that: > >(1) in cpuidle-qcom register the driver >(2) in the spm code register the device for each spm probe > >(1) being called before (2). > >If after that in the code you still have a "static bool >cpuidle_drv_init", then I guess we have a problem in the init sequence >somewhere else. > Okay, Thanks. Last night I was able to get a patch working with one cpuidle_register_driver and multiple cpuidle_register_device. The issue using an module_init() was - 1. Register a device to probe the cpuidle driver. 2. Register cpuidle devices for every SPM probed. What used to happen is that the cpuidle driver probe always happens after the cpuidle devices were created. I have a solution where I register for the cpuidle device to probe a driver, at postcore_initcall() at which point, the cpuidle framework would also be available. I am not sure how elegant it looks, but thats the latest initcall I could get a consistent driver initialization before the SPM probe (therefore the device registration). I will send the patch this morning. >>void cpuidle_update_cpumask(struct cpumask *mask) { >> struct cpuidle_driver *drv; >> >> spin_lock(&cpuidle_driver_lock); >> drv = cpuidle_get_driver(); >> if (drv) >> drv->cpumask = mask ?: cpu_possible_mask; >> spin_unlock(&cpuidle_driver_lock); >>} >> >>With that, I could register cpuidle driver the first time when the mask >>changes >>from empty and consequent updates would just update the cpumask. (I am not >>sure, if I missed anything in this change). It just seemed far too >>invasive at >>this time, in lieu of the static bool. >> >>>>+ const struct platform_device_info qcom_cpuidle_info = { >>>>+ .name = "qcom_cpuidle", >>>>+ .id = -1, >>>>+ .data = &lpm_ops, >>>>+ .size_data = sizeof(lpm_ops), >>>>+ }; >>>>+ >>>>+ drv = spm_get_drv(pdev, &cpu); >>>>+ if (!drv || cpu < 0) >>>>+ return -EINVAL; >>> >>>As already said in a previous comment, it is not possible to have "cpu >>>< 0" with "drv != NULL", so except I am missing something the test >>>should be: >>> >>> if (!drv) >>> return -EINVAL; >>> >>Sorry, done. >> >>>There is something wrong with the init sequence. Don't you find weird >>>you have to backward search for the cpu belonging to the pdev each >>>time the probe function is called ? >>> >>> >>Well, it was that or the SPM device node pointing to the CPU that it >>references. It seems more common to have an iterator than the doubly >>linked device nodes. I dont have a strong preference either way, just >>chose the way that made device nodes easier. >> This one, I havent addressed. How strongly do you feel about this? I still have to iterate through all the cpus to find the cpu node that matches the handle specified in the SAW node. Thanks, Lina
On 12/02/2014 04:35 PM, Lina Iyer wrote: > On Tue, Dec 02 2014 at 02:53 -0700, Daniel Lezcano wrote: >> On 12/01/2014 07:50 PM, Lina Iyer wrote: >>> On Thu, Nov 27 2014 at 01:52 -0700, Daniel Lezcano wrote: >>>> On 11/27/2014 06:24 AM, Lina Iyer wrote: >>> >>>> + static bool cpuidle_drv_init; >>>> >>>> ^^^^^^^^^ >>>> >>>> As already said in a previous comment, please find a way to remove >>>> that. >>>> >>> I will look into it. Stephen and I wanted the cpuidle driver to be >>> probed only after any of the SPMs are ready. And possibly, only for that >>> cpu, for which the SPM has been probed. >>> >>> To achieve the SPM -> CPUIDLE Device relation, I havent found a good way >>> to do that. Without using CPUIDLE_MULTIPLE_DRIVERS, initializing each >>> cpuidle device, separate from the cpuidle driver, requires that I update >>> the cpuidle_driver->cpumask after probing each SPM device, to allow for >>> only one driver and cpuidle devices only for the probed cpus. Using the >>> cpuidle_register_driver(), resets the drv->refcnt in >>> __cpuidle_driver_init. >>> >>> I may need something like this in the else clause of >>> CPUIDLE_MULTIPLE_DRIVERS - >>> >>> static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) >>> { >>> struct cpuidle_driver *drv = cpuidle_curr_driver; >>> >>> if (drv && !cpumask_test_cpu(cpu, drv->cpumask)) >>> drv = NULL; >>> >>> return drv; >>> } >> >> You don't have to deal with the drv->cpumask and >> CPUIDLE_MULTIPLE_DRIVERS. This field gives the list of the cpus the >> driver will have to potentially handle. >> >> Just register the driver at the first place with the platform device >> by using cpuidle_register_driver(drv), I suggest somewhere else than >> the spm code. >> >> The underlying code will do: >> >> if (!drv->cpumask) >> drv->cpumask = (struct cpumask *)cpu_possible_mask; >> >> Until the cpuidle device is not initialized for a specific cpu, the >> cpuidle code will have no effect for this cpu. If you register the >> driver but without registering the cpuidle devices that will result in >> nothing more than a structure initialized but inoperative. >> >> For each SPM being probed: >> >> struct cpuidle_device *dev = &per_cpu(cpuidle_dev, cpu); >> dev->cpu = cpu; >> cpuidle_register_device(dev); >> >> Note cpuidle_dev is exported via cpuidle.h, so you don't have to >> redeclare a per cpu cpuidle device by your own. >> >> So rephrasing all that: >> >> (1) in cpuidle-qcom register the driver >> (2) in the spm code register the device for each spm probe >> >> (1) being called before (2). >> >> If after that in the code you still have a "static bool >> cpuidle_drv_init", then I guess we have a problem in the init sequence >> somewhere else. >> > Okay, Thanks. Last night I was able to get a patch working with one > cpuidle_register_driver and multiple cpuidle_register_device. The issue > using an module_init() was - > > 1. Register a device to probe the cpuidle driver. > 2. Register cpuidle devices for every SPM probed. That's it. > What used to happen is that the cpuidle driver probe always happens > after the cpuidle devices were created. > > I have a solution where I register for the cpuidle device to probe a > driver, at postcore_initcall() at which point, the cpuidle framework > would also be available. > > I am not sure how elegant it looks, but thats the latest initcall I > could get a consistent driver initialization before the SPM probe > (therefore the device registration). I don't have the code in front of me but we can probably find a nice place to probe the cpuidle driver. Is there any pm.c file with an init function that can be useful ? In any case, regarding the unusual init sequence for the cpuidle driver, I think the probe would deserve a comment. > I will send the patch this morning. Ok, cool. [ ... ] >>>> There is something wrong with the init sequence. Don't you find weird >>>> you have to backward search for the cpu belonging to the pdev each >>>> time the probe function is called ? >>>> >>>> >>> Well, it was that or the SPM device node pointing to the CPU that it >>> references. It seems more common to have an iterator than the doubly >>> linked device nodes. I dont have a strong preference either way, just >>> chose the way that made device nodes easier. >>> > This one, I havent addressed. How strongly do you feel about this? I > still have to iterate through all the cpus to find the cpu node that > matches the handle specified in the SAW node. IMO, we can live with that if there is no longer any static variable around telling us we initialized something. Thanks ! -- Daniel
On Tue, Dec 02 2014 at 04:10 -0700, Catalin Marinas wrote: >On Mon, Dec 01, 2014 at 06:57:05PM +0000, Lina Iyer wrote: >> On Thu, Nov 27 2014 at 08:01 -0700, Lorenzo Pieralisi wrote: >> >On Thu, Nov 27, 2014 at 05:24:07AM +0000, Lina Iyer wrote: >> > >> >[...] >> > >> >> +static int spm_set_low_power_mode(enum pm_sleep_mode mode) >> >> +{ >> >> + struct spm_driver_data *drv = per_cpu(cpu_spm_drv, >> >> + smp_processor_id()); >> >> + u32 start_index; >> >> + u32 ctl_val; >> >> + >> >> + if (!drv) >> >> + return -ENXIO; >> >> + >> >> + start_index = drv->reg_data->start_index[mode]; >> >> + >> >> + ctl_val = spm_register_read(drv, SPM_REG_SPM_CTL); >> >> + ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT); >> >> + ctl_val |= start_index << SPM_CTL_INDEX_SHIFT; >> >> + ctl_val |= SPM_CTL_EN; >> >> + spm_register_write(drv, SPM_REG_SPM_CTL, ctl_val); >> >> + >> >> + /* Ensure we have written the start address */ >> >> + wmb(); >> > >> >Can you explain please what this wmb is meant to achieve ? If it is there >> >to make sure the write made it to the SPM that's not a proper way >> >to achieve it, the barrier ensures ordering, not write completion. >> >> A write completion would be great, but assuming that the context switch >> using SCM or WFI could ensure the flush, then ordering is probably good >> enough for this. > >I assume you want the write to the device to complete (side-effects are >observed) before the WFI (which would raise a StandbyWFI signal). The >safer way is to read back from the device (or check/poll for the device >state change) rather than a barrier. Polling back to see if the register write was indeed successful, seems a bit more than what I would like on a low latency mode like Standby. Let me check with the hardware engineers, if a write ordering is required. It seems to me like I am not getting much out of the wmb() anyways. I will find out what kind of guarantees I would need to ensure the SPM write is successful. Thanks for your time, Catalin. Thanks, Lina.
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt index 1505fb8..690c3c0 100644 --- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt @@ -2,11 +2,20 @@ SPM AVS Wrapper 2 (SAW2) The SAW2 is a wrapper around the Subsystem Power Manager (SPM) and the Adaptive Voltage Scaling (AVS) hardware. The SPM is a programmable -micro-controller that transitions a piece of hardware (like a processor or +power-controller that transitions a piece of hardware (like a processor or subsystem) into and out of low power modes via a direct connection to the PMIC. It can also be wired up to interact with other processors in the system, notifying them when a low power state is entered or exited. +Multiple revisions of the SAW hardware are supported using these Device Nodes. +SAW2 revisions differ in the register offset and configuration data. Also, the +same revision of the SAW in different SoCs may have different configuration +data due the the differences in hardware capabilities. Hence the SoC name, the +version of the SAW hardware in that SoC and the distinction between cpu (big +or Little) or cache, may be needed to uniquely identify the SAW register +configuration and initialization data. The compatible string is used to +indicate this parameter. + PROPERTIES - compatible: @@ -14,10 +23,13 @@ PROPERTIES Value type: <string> Definition: shall contain "qcom,saw2". A more specific value should be one of: - "qcom,saw2-v1" - "qcom,saw2-v1.1" - "qcom,saw2-v2" - "qcom,saw2-v2.1" + "qcom,saw2-v1" + "qcom,saw2-v1.1" + "qcom,saw2-v2" + "qcom,saw2-v2.1" + "qcom,apq8064-saw2-v1.1-cpu" + "qcom,msm8974-saw2-v2.1-cpu" + "qcom,apq8084-saw2-v2.1-cpu" - reg: Usage: required @@ -26,10 +38,17 @@ PROPERTIES the register region. An optional second element specifies the base address and size of the alias register region. +- regulator: + Usage: optional + Value type: boolean + Definition: Indicates that this SPM device acts as a regulator device + device for the core (CPU or Cache) the SPM is attached + to. Example: - regulator@2099000 { + power-controller@2099000 { compatible = "qcom,saw2"; reg = <0x02099000 0x1000>, <0x02009000 0x1000>; + regulator; }; diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 7dcd554..012fb37 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -11,3 +11,11 @@ config QCOM_GSBI config QCOM_SCM bool + +config QCOM_PM + bool "Qualcomm Power Management" + depends on ARCH_QCOM + help + QCOM Platform specific power driver to manage cores and L2 low power + modes. It interface with various system drivers to put the cores in + low power modes. diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 70d52ed..20b329f 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o +obj-$(CONFIG_QCOM_PM) += spm.o CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1) obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c new file mode 100644 index 0000000..0b4616a --- /dev/null +++ b/drivers/soc/qcom/spm.c @@ -0,0 +1,327 @@ +/* + * 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/init.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/cpuidle.h> +#include <linux/cpu_pm.h> + +#include <asm/proc-fns.h> +#include <asm/suspend.h> + +#include <soc/qcom/pm.h> +#include <soc/qcom/pm.h> +#include <soc/qcom/scm.h> +#include <soc/qcom/scm-boot.h> + + +#define SCM_CMD_TERMINATE_PC 0x2 +#define SCM_FLUSH_FLAG_MASK 0x3 +#define SCM_L2_ON 0x0 +#define SCM_L2_OFF 0x1 +#define MAX_PMIC_DATA 2 +#define MAX_SEQ_DATA 64 +#define SPM_CTL_INDEX 0x7f +#define SPM_CTL_INDEX_SHIFT 4 +#define SPM_CTL_EN BIT(0) + +enum spm_reg { + SPM_REG_CFG, + SPM_REG_SPM_CTL, + SPM_REG_DLY, + SPM_REG_PMIC_DLY, + SPM_REG_PMIC_DATA_0, + SPM_REG_PMIC_DATA_1, + SPM_REG_VCTL, + SPM_REG_SEQ_ENTRY, + SPM_REG_SPM_STS, + SPM_REG_PMIC_STS, + SPM_REG_NR, +}; + +struct spm_reg_data { + const u8 *reg_offset; + u32 spm_cfg; + u32 spm_dly; + u32 pmic_dly; + u32 pmic_data[MAX_PMIC_DATA]; + u8 seq[MAX_SEQ_DATA]; + u8 start_index[PM_SLEEP_MODE_NR]; +}; + +struct spm_driver_data { + void __iomem *reg_base; + const struct spm_reg_data *reg_data; +}; + +static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = { + [SPM_REG_CFG] = 0x08, + [SPM_REG_SPM_CTL] = 0x30, + [SPM_REG_DLY] = 0x34, + [SPM_REG_SEQ_ENTRY] = 0x80, +}; + +/* SPM register data for 8974, 8084 */ +static const struct spm_reg_data spm_reg_8974_8084_cpu = { + .reg_offset = spm_reg_offset_v2_1, + .spm_cfg = 0x1, + .spm_dly = 0x3C102800, + .seq = { 0x03, 0x0B, 0x0F, 0x00, 0x20, 0x80, 0x10, 0xE8, 0x5B, 0x03, + 0x3B, 0xE8, 0x5B, 0x82, 0x10, 0x0B, 0x30, 0x06, 0x26, 0x30, + 0x0F }, + .start_index[PM_SLEEP_MODE_STBY] = 0, + .start_index[PM_SLEEP_MODE_SPC] = 3, +}; + +static const u8 spm_reg_offset_v1_1[SPM_REG_NR] = { + [SPM_REG_CFG] = 0x08, + [SPM_REG_SPM_CTL] = 0x20, + [SPM_REG_PMIC_DLY] = 0x24, + [SPM_REG_PMIC_DATA_0] = 0x28, + [SPM_REG_PMIC_DATA_1] = 0x2C, + [SPM_REG_SEQ_ENTRY] = 0x80, +}; + +/* SPM register data for 8064 */ +static const struct spm_reg_data spm_reg_8064_cpu = { + .reg_offset = spm_reg_offset_v1_1, + .spm_cfg = 0x1F, + .pmic_dly = 0x02020004, + .pmic_data[0] = 0x0084009C, + .pmic_data[1] = 0x00A4001C, + .seq = { 0x03, 0x0F, 0x00, 0x24, 0x54, 0x10, 0x09, 0x03, 0x01, + 0x10, 0x54, 0x30, 0x0C, 0x24, 0x30, 0x0F }, + .start_index[PM_SLEEP_MODE_STBY] = 0, + .start_index[PM_SLEEP_MODE_SPC] = 2, +}; + +static DEFINE_PER_CPU_SHARED_ALIGNED(struct spm_driver_data *, cpu_spm_drv); + +static inline void spm_register_write(struct spm_driver_data *drv, + enum spm_reg reg, u32 val) +{ + if (drv->reg_data->reg_offset[reg]) + writel_relaxed(val, drv->reg_base + + drv->reg_data->reg_offset[reg]); +} + +static inline u32 spm_register_read(struct spm_driver_data *drv, + enum spm_reg reg) +{ + return readl_relaxed(drv->reg_base + drv->reg_data->reg_offset[reg]); +} + +static int spm_set_low_power_mode(enum pm_sleep_mode mode) +{ + struct spm_driver_data *drv = per_cpu(cpu_spm_drv, + smp_processor_id()); + u32 start_index; + u32 ctl_val; + + if (!drv) + return -ENXIO; + + start_index = drv->reg_data->start_index[mode]; + + ctl_val = spm_register_read(drv, SPM_REG_SPM_CTL); + ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT); + ctl_val |= start_index << SPM_CTL_INDEX_SHIFT; + ctl_val |= SPM_CTL_EN; + spm_register_write(drv, SPM_REG_SPM_CTL, ctl_val); + + /* Ensure we have written the start address */ + wmb(); + + return 0; +} + +static int qcom_pm_collapse(unsigned long int unused) +{ + int ret; + u32 flag; + int cpu = smp_processor_id(); + + ret = scm_set_warm_boot_addr(cpu_resume, cpu); + if (ret) + return ret; + + flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK; + scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag); + + /* + * Returns here only if there was a pending interrupt and we did not + * power down as a result. + */ + return 0; +} + +static int qcom_cpu_standby(void *unused) +{ + int ret; + + ret = spm_set_low_power_mode(PM_SLEEP_MODE_STBY); + if (ret) + return ret; + + cpu_do_idle(); + + return 0; +} + +static int qcom_cpu_spc(void *unused) +{ int ret; + + ret = spm_set_low_power_mode(PM_SLEEP_MODE_STBY); + if (ret) + return ret; + + cpu_pm_enter(); + cpu_suspend(0, qcom_pm_collapse); + cpu_pm_exit(); + + return 0; +} + +static struct spm_driver_data *spm_get_drv(struct platform_device *pdev, + int *spm_cpu) +{ + struct spm_driver_data *drv = NULL; + struct device_node *cpu_node, *saw_node; + int cpu; + bool found = false; + + for_each_possible_cpu(cpu) { + cpu_node = of_cpu_device_node_get(cpu); + if (!cpu_node) + continue; + saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0); + if (saw_node) { + if (saw_node == pdev->dev.of_node) + found = true; + of_node_put(saw_node); + } + of_node_put(cpu_node); + if (found) + break; + } + + if (found) { + drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); + if (drv) + *spm_cpu = cpu; + } + + return drv; +} + +static const struct of_device_id spm_match_table[] = { + { .compatible = "qcom,msm8974-saw2-v2.1-cpu", + .data = &spm_reg_8974_8084_cpu }, + { .compatible = "qcom,apq8084-saw2-v2.1-cpu", + .data = &spm_reg_8974_8084_cpu }, + { .compatible = "qcom,apq8064-saw2-v1.1-cpu", + .data = &spm_reg_8064_cpu }, + { }, +}; + +static const struct qcom_cpu_pm_ops lpm_ops = { + .standby = qcom_cpu_standby, + .spc = qcom_cpu_spc, +}; + +static int spm_dev_probe(struct platform_device *pdev) +{ + struct spm_driver_data *drv; + struct resource *res; + const struct of_device_id *match_id; + void __iomem *addr; + int cpu = -EINVAL; + static bool cpuidle_drv_init; + const struct platform_device_info qcom_cpuidle_info = { + .name = "qcom_cpuidle", + .id = -1, + .data = &lpm_ops, + .size_data = sizeof(lpm_ops), + }; + + drv = spm_get_drv(pdev, &cpu); + if (!drv || cpu < 0) + return -EINVAL; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + drv->reg_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(drv->reg_base)) + return PTR_ERR(drv->reg_base); + + match_id = of_match_node(spm_match_table, pdev->dev.of_node); + if (!match_id) + return -ENODEV; + + drv->reg_data = match_id->data; + + /* Write the SPM sequences first.. */ + addr = drv->reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY]; + __iowrite32_copy(addr, drv->reg_data->seq, + ARRAY_SIZE(drv->reg_data->seq) / 4); + + /* + * ..and then the control registers. + * On some SoC's if the control registers are written first and if the + * CPU was held in reset, the reset signal could trigger the SPM state + * machine, before the sequences are completely written. + */ + spm_register_write(drv, SPM_REG_CFG, drv->reg_data->spm_cfg); + spm_register_write(drv, SPM_REG_DLY, drv->reg_data->spm_dly); + spm_register_write(drv, SPM_REG_PMIC_DLY, drv->reg_data->pmic_dly); + + spm_register_write(drv, SPM_REG_PMIC_DATA_0, + drv->reg_data->pmic_data[0]); + spm_register_write(drv, SPM_REG_PMIC_DATA_1, + drv->reg_data->pmic_data[1]); + + /* + * Ensure all observers see the above register writes before the + * cpuidle driver is allowed to use the SPM. + */ + wmb(); + per_cpu(cpu_spm_drv, cpu) = drv; + + if (!cpuidle_drv_init) { + platform_device_register_full(&qcom_cpuidle_info); + cpuidle_drv_init = true; + } + + return 0; +} + +static struct platform_driver spm_driver = { + .probe = spm_dev_probe, + .driver = { + .name = "saw", + .of_match_table = spm_match_table, + }, +}; + +module_platform_driver(spm_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("SAW power controller driver"); +MODULE_ALIAS("platform:saw"); diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h new file mode 100644 index 0000000..d9a56d7 --- /dev/null +++ b/include/soc/qcom/pm.h @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#ifndef __QCOM_PM_H +#define __QCOM_PM_H + +enum pm_sleep_mode { + PM_SLEEP_MODE_STBY, + PM_SLEEP_MODE_RET, + PM_SLEEP_MODE_SPC, + PM_SLEEP_MODE_PC, + PM_SLEEP_MODE_NR, +}; + +struct qcom_cpu_pm_ops { + int (*standby)(void *data); + int (*spc)(void *data); +}; + +#endif /* __QCOM_PM_H */