| Message ID | 1417541958-56907-4-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Dec 02 2014 at 10:40 -0700, Lina Iyer wrote: + [...] >+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; >+ struct cpuidle_device *dev; >+ >+ drv = spm_get_drv(pdev, &cpu); >+ if (!drv) >+ return -EINVAL; >+ >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >+ drv->reg_base = devm_ioremap_resource(&pdev->dev, res); >+ if (IS_ERR(drv->reg_base)) >+ return PTR_ERR(drv->reg_base); >+ >+ match_id = of_match_node(spm_match_table, pdev->dev.of_node); >+ if (!match_id) >+ return -ENODEV; >+ >+ drv->reg_data = match_id->data; >+ >+ /* 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 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]); >+ >+ per_cpu(cpu_spm_drv, cpu) = drv; >+ >+ /* Register the cpuidle device for the cpu, we are ready for cpuidle */ >+ dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); >+ if (!dev) >+ return -ENOMEM; >+ >+ dev->cpu = cpu; >+ return cpuidle_register_device(dev); >+} >+ >+static struct qcom_cpu_pm_ops lpm_ops = { >+ .standby = qcom_cpu_standby, >+ .spc = qcom_cpu_spc, >+}; >+ >+static struct platform_device qcom_cpuidle_drv = { >+ .name = "qcom_cpuidle", >+ .id = -1, >+ .dev.platform_data = &lpm_ops, >+}; >+ >+static struct platform_driver spm_driver = { >+ .probe = spm_dev_probe, >+ .driver = { >+ .name = "saw", >+ .of_match_table = spm_match_table, >+ }, >+}; >+ >+static int __init qcom_spm_init(void) >+{ >+ int ret; >+ >+ /* >+ * cpuidle driver need to registered before the cpuidle device >+ * for any cpu. Register the device for the the cpuidle driver. >+ */ >+ ret = platform_device_register(&qcom_cpuidle_drv); >+ if (ret) >+ return ret; Stephen pointed out that we would have the platform device lying around on a non-QCOM device when using multi_v7_defconfig. So instead of doing this here, we could do this in the probe.. if (!cpuidle_get_driver()) { int ret = platform_device_register(&qcom_cpuidle_drv); if (ret) return ret; } Would that be okay? The successful probe indicates that we are on a QCOM SoC, and we have not registered a cpuidle_driver before this. Thanks, Lina >+ >+ return platform_driver_register(&spm_driver); >+} >+module_init(qcom_spm_init); >+ >+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 > -- 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/03/2014 12:05 AM, Lina Iyer wrote: > On Tue, Dec 02 2014 at 10:40 -0700, Lina Iyer wrote: > + > [...] [ ... ] >> +static int __init qcom_spm_init(void) >> +{ >> + int ret; >> + >> + /* >> + * cpuidle driver need to registered before the cpuidle device >> + * for any cpu. Register the device for the the cpuidle driver. >> + */ >> + ret = platform_device_register(&qcom_cpuidle_drv); >> + if (ret) >> + return ret; > Stephen pointed out that we would have the platform device lying around > on a non-QCOM device when using multi_v7_defconfig. Perhaps I am missing the point, but this is not supposed to happen, no ? > So instead of doing this here, we could do this in the probe.. > > if (!cpuidle_get_driver()) { > int ret = platform_device_register(&qcom_cpuidle_drv); > if (ret) > return ret; > } > > Would that be okay? > > The successful probe indicates that we are on a QCOM SoC, and we have not > registered a cpuidle_driver before this. > > Thanks, > Lina > >> + >> + return platform_driver_register(&spm_driver); >> +} >> +module_init(qcom_spm_init); >> + >> +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 >>
On Wed, Dec 03 2014 at 02:11 -0700, Daniel Lezcano wrote: >On 12/03/2014 12:05 AM, Lina Iyer wrote: >>On Tue, Dec 02 2014 at 10:40 -0700, Lina Iyer wrote: >>+ >>[...] > >[ ... ] > >>>+static int __init qcom_spm_init(void) >>>+{ >>>+ int ret; >>>+ >>>+ /* >>>+ * cpuidle driver need to registered before the cpuidle device >>>+ * for any cpu. Register the device for the the cpuidle driver. >>>+ */ >>>+ ret = platform_device_register(&qcom_cpuidle_drv); >>>+ if (ret) >>>+ return ret; >>Stephen pointed out that we would have the platform device lying around >>on a non-QCOM device when using multi_v7_defconfig. > >Perhaps I am missing the point, but this is not supposed to happen, no ? > This would happen, since the file would compile on multi_v7 and we would initialize and register this device regardless. The cpuidle-qcom.c driver probe would bail out looking for a matching compatible property. So we would not register a cpuidle driver but the device would lay around. >>So instead of doing this here, we could do this in the probe.. >> >>if (!cpuidle_get_driver()) { >> int ret = platform_device_register(&qcom_cpuidle_drv); >> if (ret) >> return ret; >>} >> >>Would that be okay? >> >>The successful probe indicates that we are on a QCOM SoC, and we have not >>registered a cpuidle_driver before this. >> >>Thanks, >>Lina >> >>>+ >>>+ return platform_driver_register(&spm_driver); >>>+} >>>+module_init(qcom_spm_init); >>>+ >>>+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 >>> > > >-- > <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 > -- 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 Wed, Dec 03 2014 at 07:31 -0700, Lina Iyer wrote: >On Wed, Dec 03 2014 at 02:11 -0700, Daniel Lezcano wrote: >>On 12/03/2014 12:05 AM, Lina Iyer wrote: >>>On Tue, Dec 02 2014 at 10:40 -0700, Lina Iyer wrote: >>>+ >>>[...] >> >>[ ... ] >> >>>>+static int __init qcom_spm_init(void) >>>>+{ >>>>+ int ret; >>>>+ >>>>+ /* >>>>+ * cpuidle driver need to registered before the cpuidle device >>>>+ * for any cpu. Register the device for the the cpuidle driver. >>>>+ */ >>>>+ ret = platform_device_register(&qcom_cpuidle_drv); >>>>+ if (ret) >>>>+ return ret; >>>Stephen pointed out that we would have the platform device lying around >>>on a non-QCOM device when using multi_v7_defconfig. >> >>Perhaps I am missing the point, but this is not supposed to happen, no ? >> >This would happen, since the file would compile on multi_v7 and we would >initialize and register this device regardless. The cpuidle-qcom.c >driver probe would bail out looking for a matching compatible property. >So we would not register a cpuidle driver but the device would lay >around. > Not the best thing to do, but I noticed that ACPI driver does reference count to register cpuidle driver for the first device and remove them as the reference count decreases. I am not sure, if we need to reference count here, since the SPM devices themselves are not removable. >>>So instead of doing this here, we could do this in the probe.. >>> >>>if (!cpuidle_get_driver()) { >>> int ret = platform_device_register(&qcom_cpuidle_drv); >>> if (ret) >>> return ret; >>>} >>> >>>Would that be okay? >>> >>>The successful probe indicates that we are on a QCOM SoC, and we have not >>>registered a cpuidle_driver before this. >>> >>>Thanks, >>>Lina >>> >>>>+ >>>>+ return platform_driver_register(&spm_driver); >>>>+} >>>>+module_init(qcom_spm_init); >>>>+ >>>>+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 >>>> >> >> >>-- >><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 >> -- 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 Wednesday 03 December 2014 07:31:22 Lina Iyer wrote: > >>>+static int __init qcom_spm_init(void) > >>>+{ > >>>+ int ret; > >>>+ > >>>+ /* > >>>+ * cpuidle driver need to registered before the cpuidle device > >>>+ * for any cpu. Register the device for the the cpuidle driver. > >>>+ */ > >>>+ ret = platform_device_register(&qcom_cpuidle_drv); > >>>+ if (ret) > >>>+ return ret; > >>Stephen pointed out that we would have the platform device lying around > >>on a non-QCOM device when using multi_v7_defconfig. > > > >Perhaps I am missing the point, but this is not supposed to happen, no ? > > > This would happen, since the file would compile on multi_v7 and we would > initialize and register this device regardless. The cpuidle-qcom.c > driver probe would bail out looking for a matching compatible property. > So we would not register a cpuidle driver but the device would lay > around. I think the problem is registering a platform_device. I've complained about this before, but it still seems to get copied all over the place. Please don't do this but have a driver that looks at DT to figure out whether to access hardware or not. Arnd -- 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/03/2014 09:35 PM, Arnd Bergmann wrote: > On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote: >>>>> +static int __init qcom_spm_init(void) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + /* >>>>> + * cpuidle driver need to registered before the cpuidle device >>>>> + * for any cpu. Register the device for the the cpuidle driver. >>>>> + */ >>>>> + ret = platform_device_register(&qcom_cpuidle_drv); >>>>> + if (ret) >>>>> + return ret; >>>> Stephen pointed out that we would have the platform device lying around >>>> on a non-QCOM device when using multi_v7_defconfig. >>> >>> Perhaps I am missing the point, but this is not supposed to happen, no ? >>> >> This would happen, since the file would compile on multi_v7 and we would >> initialize and register this device regardless. The cpuidle-qcom.c >> driver probe would bail out looking for a matching compatible property. >> So we would not register a cpuidle driver but the device would lay >> around. > > I think the problem is registering a platform_device. I've complained > about this before, but it still seems to get copied all over the > place. Please don't do this but have a driver that looks at DT to > figure out whether to access hardware or not. We did this approach but, I can remember why, someone was complaining about it also :) The platform device/driver paradigm allowed us to split the arch specific parts by passing the pm ops through the platform data. Would make sense to have a single common place for the ARM arch where we initialize the platform device for cpuidle ?
On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote: > On 12/03/2014 09:35 PM, Arnd Bergmann wrote: > > On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote: > >>>>> +static int __init qcom_spm_init(void) > >>>>> +{ > >>>>> + int ret; > >>>>> + > >>>>> + /* > >>>>> + * cpuidle driver need to registered before the cpuidle device > >>>>> + * for any cpu. Register the device for the the cpuidle driver. > >>>>> + */ > >>>>> + ret = platform_device_register(&qcom_cpuidle_drv); > >>>>> + if (ret) > >>>>> + return ret; > >>>> Stephen pointed out that we would have the platform device lying around > >>>> on a non-QCOM device when using multi_v7_defconfig. > >>> > >>> Perhaps I am missing the point, but this is not supposed to happen, no ? > >>> > >> This would happen, since the file would compile on multi_v7 and we would > >> initialize and register this device regardless. The cpuidle-qcom.c > >> driver probe would bail out looking for a matching compatible property. > >> So we would not register a cpuidle driver but the device would lay > >> around. > > > > I think the problem is registering a platform_device. I've complained > > about this before, but it still seems to get copied all over the > > place. Please don't do this but have a driver that looks at DT to > > figure out whether to access hardware or not. > > We did this approach but, I can remember why, someone was complaining > about it also > > The platform device/driver paradigm allowed us to split the arch > specific parts by passing the pm ops through the platform data. > > Would make sense to have a single common place for the ARM arch where we > initialize the platform device for cpuidle ? No. It's really not a device, and if you pretend that it is, you get into problems like this. Arnd -- 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 Thu, Dec 04 2014 at 02:02 -0700, Arnd Bergmann wrote: >On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote: >> On 12/03/2014 09:35 PM, Arnd Bergmann wrote: >> > On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote: >> >>>>> +static int __init qcom_spm_init(void) >> >>>>> +{ >> >>>>> + int ret; >> >>>>> + >> >>>>> + /* >> >>>>> + * cpuidle driver need to registered before the cpuidle device >> >>>>> + * for any cpu. Register the device for the the cpuidle driver. >> >>>>> + */ >> >>>>> + ret = platform_device_register(&qcom_cpuidle_drv); >> >>>>> + if (ret) >> >>>>> + return ret; >> >>>> Stephen pointed out that we would have the platform device lying around >> >>>> on a non-QCOM device when using multi_v7_defconfig. >> >>> >> >>> Perhaps I am missing the point, but this is not supposed to happen, no ? >> >>> >> >> This would happen, since the file would compile on multi_v7 and we would >> >> initialize and register this device regardless. The cpuidle-qcom.c >> >> driver probe would bail out looking for a matching compatible property. >> >> So we would not register a cpuidle driver but the device would lay >> >> around. >> > >> > I think the problem is registering a platform_device. I've complained >> > about this before, but it still seems to get copied all over the >> > place. Please don't do this but have a driver that looks at DT to >> > figure out whether to access hardware or not. >> >> We did this approach but, I can remember why, someone was complaining >> about it also >> >> The platform device/driver paradigm allowed us to split the arch >> specific parts by passing the pm ops through the platform data. >> >> Would make sense to have a single common place for the ARM arch where we >> initialize the platform device for cpuidle ? > >No. It's really not a device, and if you pretend that it is, you get >into problems like this. Arnd, the problem is that the provides function pointers to the SoC code that the cpuilde driver uses to call based on the idle state. After much discussions, we came down to using function pointers from translating from DT strings, other than using that again, I dont see a good way of achieving the ability of cpuidle driver staying a separate driver from the SPM driver. Daniel, thoughts? -- 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 Thursday 04 December 2014 09:28:34 Lina Iyer wrote: > On Thu, Dec 04 2014 at 02:02 -0700, Arnd Bergmann wrote: > >On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote: > >> On 12/03/2014 09:35 PM, Arnd Bergmann wrote: > >> > On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote: > >> >>>>> +static int __init qcom_spm_init(void) > >> >>>>> +{ > >> >>>>> + int ret; > >> >>>>> + > >> >>>>> + /* > >> >>>>> + * cpuidle driver need to registered before the cpuidle device > >> >>>>> + * for any cpu. Register the device for the the cpuidle driver. > >> >>>>> + */ > >> >>>>> + ret = platform_device_register(&qcom_cpuidle_drv); > >> >>>>> + if (ret) > >> >>>>> + return ret; > >> >>>> Stephen pointed out that we would have the platform device lying around > >> >>>> on a non-QCOM device when using multi_v7_defconfig. > >> >>> > >> >>> Perhaps I am missing the point, but this is not supposed to happen, no ? > >> >>> > >> >> This would happen, since the file would compile on multi_v7 and we would > >> >> initialize and register this device regardless. The cpuidle-qcom.c > >> >> driver probe would bail out looking for a matching compatible property. > >> >> So we would not register a cpuidle driver but the device would lay > >> >> around. > >> > > >> > I think the problem is registering a platform_device. I've complained > >> > about this before, but it still seems to get copied all over the > >> > place. Please don't do this but have a driver that looks at DT to > >> > figure out whether to access hardware or not. > >> > >> We did this approach but, I can remember why, someone was complaining > >> about it also > >> > >> The platform device/driver paradigm allowed us to split the arch > >> specific parts by passing the pm ops through the platform data. > >> > >> Would make sense to have a single common place for the ARM arch where we > >> initialize the platform device for cpuidle ? > > > >No. It's really not a device, and if you pretend that it is, you get > >into problems like this. > > Arnd, the problem is that the provides function pointers to the SoC code > that the cpuilde driver uses to call based on the idle state. > > After much discussions, we came down to using function pointers from > translating from DT strings, other than using that again, I dont see a > good way of achieving the ability of cpuidle driver staying a separate > driver from the SPM driver. > > Daniel, thoughts? Maybe the problem is trying too hard to separate two things that really belong together then. In general, the strategy of coming up with subsystems for a class of devices and them turning platform code into drivers for that subsystem has worked out really well, but I think for cpufreq, cpuidle and smp, it really hasn't, and in the third case we haven't even tried coming up with a subsystem for that reason. Having all cpuidle code generally live in drivers/cpuidle is still a good idea IMO, but using a platform device just for the purpose of passing data between some platform specific code and another platform specific driver hasn't worked out that well here. Arnd -- 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 Thu, Dec 04 2014 at 11:20 -0700, Arnd Bergmann wrote: >On Thursday 04 December 2014 09:28:34 Lina Iyer wrote: >> On Thu, Dec 04 2014 at 02:02 -0700, Arnd Bergmann wrote: >> >On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote: >> >> On 12/03/2014 09:35 PM, Arnd Bergmann wrote: >> >> > On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote: >> >> >>>>> +static int __init qcom_spm_init(void) >> >> >>>>> +{ >> >> >>>>> + int ret; >> >> >>>>> + >> >> >>>>> + /* >> >> >>>>> + * cpuidle driver need to registered before the cpuidle device >> >> >>>>> + * for any cpu. Register the device for the the cpuidle driver. >> >> >>>>> + */ >> >> >>>>> + ret = platform_device_register(&qcom_cpuidle_drv); >> >> >>>>> + if (ret) >> >> >>>>> + return ret; >> >> >>>> Stephen pointed out that we would have the platform device lying around >> >> >>>> on a non-QCOM device when using multi_v7_defconfig. >> >> >>> >> >> >>> Perhaps I am missing the point, but this is not supposed to happen, no ? >> >> >>> >> >> >> This would happen, since the file would compile on multi_v7 and we would >> >> >> initialize and register this device regardless. The cpuidle-qcom.c >> >> >> driver probe would bail out looking for a matching compatible property. >> >> >> So we would not register a cpuidle driver but the device would lay >> >> >> around. >> >> > >> >> > I think the problem is registering a platform_device. I've complained >> >> > about this before, but it still seems to get copied all over the >> >> > place. Please don't do this but have a driver that looks at DT to >> >> > figure out whether to access hardware or not. >> >> >> >> We did this approach but, I can remember why, someone was complaining >> >> about it also >> >> >> >> The platform device/driver paradigm allowed us to split the arch >> >> specific parts by passing the pm ops through the platform data. >> >> >> >> Would make sense to have a single common place for the ARM arch where we >> >> initialize the platform device for cpuidle ? >> > >> >No. It's really not a device, and if you pretend that it is, you get >> >into problems like this. >> >> Arnd, the problem is that the provides function pointers to the SoC code >> that the cpuilde driver uses to call based on the idle state. >> >> After much discussions, we came down to using function pointers from >> translating from DT strings, other than using that again, I dont see a >> good way of achieving the ability of cpuidle driver staying a separate >> driver from the SPM driver. >> >> Daniel, thoughts? > >Maybe the problem is trying too hard to separate two things that really >belong together then. In general, the strategy of coming up with subsystems >for a class of devices and them turning platform code into drivers for >that subsystem has worked out really well, but I think for cpufreq, cpuidle >and smp, it really hasn't, and in the third case we haven't even tried >coming up with a subsystem for that reason. > >Having all cpuidle code generally live in drivers/cpuidle is still a good >idea IMO, but using a platform device just for the purpose of passing >data between some platform specific code and another platform specific >driver hasn't worked out that well here. > To achieve both, I cant think of a better way to initialize the cpuidle driver without the use of reference count (0 ==> platform_driver_register). I tried creating dummy platform device in the DT but something or another gives in to an ugly implementation. Any other ideas? Lina. -- 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/04/2014 07:20 PM, Arnd Bergmann wrote: > On Thursday 04 December 2014 09:28:34 Lina Iyer wrote: >> On Thu, Dec 04 2014 at 02:02 -0700, Arnd Bergmann wrote: >>> On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote: >>>> On 12/03/2014 09:35 PM, Arnd Bergmann wrote: >>>>> On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote: >>>>>>>>> +static int __init qcom_spm_init(void) >>>>>>>>> +{ >>>>>>>>> + int ret; >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * cpuidle driver need to registered before the cpuidle device >>>>>>>>> + * for any cpu. Register the device for the the cpuidle driver. >>>>>>>>> + */ >>>>>>>>> + ret = platform_device_register(&qcom_cpuidle_drv); >>>>>>>>> + if (ret) >>>>>>>>> + return ret; >>>>>>>> Stephen pointed out that we would have the platform device lying around >>>>>>>> on a non-QCOM device when using multi_v7_defconfig. >>>>>>> >>>>>>> Perhaps I am missing the point, but this is not supposed to happen, no ? >>>>>>> >>>>>> This would happen, since the file would compile on multi_v7 and we would >>>>>> initialize and register this device regardless. The cpuidle-qcom.c >>>>>> driver probe would bail out looking for a matching compatible property. >>>>>> So we would not register a cpuidle driver but the device would lay >>>>>> around. >>>>> >>>>> I think the problem is registering a platform_device. I've complained >>>>> about this before, but it still seems to get copied all over the >>>>> place. Please don't do this but have a driver that looks at DT to >>>>> figure out whether to access hardware or not. >>>> >>>> We did this approach but, I can remember why, someone was complaining >>>> about it also >>>> >>>> The platform device/driver paradigm allowed us to split the arch >>>> specific parts by passing the pm ops through the platform data. >>>> >>>> Would make sense to have a single common place for the ARM arch where we >>>> initialize the platform device for cpuidle ? >>> >>> No. It's really not a device, and if you pretend that it is, you get >>> into problems like this. >> >> Arnd, the problem is that the provides function pointers to the SoC code >> that the cpuilde driver uses to call based on the idle state. >> >> After much discussions, we came down to using function pointers from >> translating from DT strings, other than using that again, I dont see a >> good way of achieving the ability of cpuidle driver staying a separate >> driver from the SPM driver. >> >> Daniel, thoughts? > > Maybe the problem is trying too hard to separate two things that really > belong together then. In general, the strategy of coming up with subsystems > for a class of devices and them turning platform code into drivers for > that subsystem has worked out really well, but I think for cpufreq, cpuidle > and smp, it really hasn't, and in the third case we haven't even tried > coming up with a subsystem for that reason. > > Having all cpuidle code generally live in drivers/cpuidle is still a good > idea IMO, but using a platform device just for the purpose of passing > data between some platform specific code and another platform specific > driver hasn't worked out that well here. At the beginning, all that become from not including mach files from the drivers directory which make sense. Perhaps it is time to write a similar mechanism for the cpuidle drivers where we can still separate the low level PM code from the generic cpuidle code. Something like (very roughly): Index: cpuidle-next/drivers/cpuidle/driver.c =================================================================== --- cpuidle-next.orig/drivers/cpuidle/driver.c 2014-12-16 14:04:51.750861310 +0100 +++ cpuidle-next/drivers/cpuidle/driver.c 2014-12-16 15:09:52.202706756 +0100 @@ -178,6 +178,45 @@ static void __cpuidle_driver_init(struct } } +struct cpuidle_ops_reg { + const char *name; + struct cpuidle_ops *ops; + struct list_head *list; +}; + +static LIST_HEAD(ops_list); + +static struct cpuidle_ops *cpuidle_find_ops(const char *name) +{ + struct cpuidle_ops_reg *ops_reg; + + list_for_each_entry(ops_reg, &ops_list, list) { + if (!strcmp(ops_reg->name, name)) + return ops_reg->ops; + } + + return NULL; +} + +int cpuidle_register_ops(const char *name, struct cpuidle_ops *ops) +{ + struct cpuidle_ops_reg *reg; + + reg = kmalloc(sizeof(*reg), GFP_KERNEL); + if (!reg) + return -ENOMEM; + + reg->name = name; + reg->ops = ops; + INIT_LIST_HEAD(®->list); + + spin_lock(&cpuidle_driver_lock); + list_add(&ops_list, ®->list); + spin_unlock(&cpuidle_driver_lock); + + return 0; +} + /** * __cpuidle_register_driver: register the driver * @drv: a valid pointer to a struct cpuidle_driver @@ -194,6 +233,7 @@ static void __cpuidle_driver_init(struct static int __cpuidle_register_driver(struct cpuidle_driver *drv) { int ret; + struct cpuidle_ops *ops; if (!drv || !drv->state_count) return -EINVAL; @@ -201,6 +241,10 @@ static int __cpuidle_register_driver(str if (cpuidle_disabled()) return -ENODEV; + ops = cpuidle_find_ops(drv->name); + if (ops) + drv->ops = ops; + __cpuidle_driver_init(drv); ret = __cpuidle_set_driver(drv); Index: cpuidle-next/include/linux/cpuidle.h =================================================================== --- cpuidle-next.orig/include/linux/cpuidle.h 2014-12-16 14:06:09.442858231 +0100 +++ cpuidle-next/include/linux/cpuidle.h 2014-12-16 14:59:46.594730753 +0100 @@ -109,11 +109,21 @@ static inline int cpuidle_get_last_resid * CPUIDLE DRIVER INTERFACE * ****************************/ +struct cpuidle_ops { + int (*standby)(struct cpuidle_driver *drv, + struct cpuidle_device *dev); + int (*retention)(struct cpuidle_driver *drv, + struct cpuidle_device *dev); + int (*poweroff)(struct cpuidle_driver *drv, + struct cpuidle_device *dev); +}; + struct cpuidle_driver { const char *name; struct module *owner; int refcnt; + struct cpuidle_ops *ops; /* used by the cpuidle framework to setup the broadcast timer */ unsigned int bctimer:1; /* states array must be ordered in decreasing power consumption */
On Tuesday 16 December 2014 15:12:22 Daniel Lezcano wrote: > At the beginning, all that become from not including mach files from the > drivers directory which make sense. > > Perhaps it is time to write a similar mechanism for the cpuidle drivers > where we can still separate the low level PM code from the generic > cpuidle code. That way you basically duplicate the same thing we already have, which isn't much better. In the example of drivers/soc/qcom/spm.c, just call cpuidle_register from the spm_dev_probe() function and be done with it. You already have a device that is responsible for handling this, don't try to construct more than you already need. I would assume that the same can be done for most other platforms. There are probably cases where the same piece of hardware is responsible for both cpuidle and cpufreq, but what that means is really that you should have a single driver for it that does both things. Same for SMP support: if you have one register block that does both the SMP bringup and the cpuidle stuff, then have *one* driver for this block that does it all. There are currently a few dependencies that require doing SMP bringup early during boot, but we decided years ago that those are all artificial dependencies and we should be able to boot secondary CPUs much later than we currently do. Arnd -- 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 Friday 05 December 2014 08:45:26 Lina Iyer wrote: > On Thu, Dec 04 2014 at 11:20 -0700, Arnd Bergmann wrote: > >On Thursday 04 December 2014 09:28:34 Lina Iyer wrote: > >Having all cpuidle code generally live in drivers/cpuidle is still a good > >idea IMO, but using a platform device just for the purpose of passing > >data between some platform specific code and another platform specific > >driver hasn't worked out that well here. > > > To achieve both, I cant think of a better way to initialize the cpuidle > driver without the use of reference count (0 ==> > platform_driver_register). > > I tried creating dummy platform device in the DT but something or > another gives in to an ugly implementation. > > Any other ideas? Hi Lina, sorry for missing your mail earlier, I just replied to Daniel on this topic, when the thread popped up again. Arnd -- 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/16/2014 06:38 AM, Arnd Bergmann wrote: > On Tuesday 16 December 2014 15:12:22 Daniel Lezcano wrote: >> At the beginning, all that become from not including mach files from the >> drivers directory which make sense. >> >> Perhaps it is time to write a similar mechanism for the cpuidle drivers >> where we can still separate the low level PM code from the generic >> cpuidle code. > That way you basically duplicate the same thing we already have, which > isn't much better. > > In the example of drivers/soc/qcom/spm.c, just call cpuidle_register > from the spm_dev_probe() function and be done with it. You already > have a device that is responsible for handling this, don't try to > construct more than you already need. > > I would assume that the same can be done for most other platforms. > > There are probably cases where the same piece of hardware is responsible > for both cpuidle and cpufreq, but what that means is really that you > should have a single driver for it that does both things. Same for > SMP support: if you have one register block that does both the SMP > bringup and the cpuidle stuff, then have *one* driver for this block > that does it all. There are currently a few dependencies that require > doing SMP bringup early during boot, but we decided years ago that those > are all artificial dependencies and we should be able to boot secondary > CPUs much later than we currently do. > +1. The SPM harware is used for hotplug, suspend, cpuidle, as well as provides a regulator for a CPU, so all these things should be done in a single driver. Booting secondary CPUs early is not hard here either if we move the smp ops into the same driver. The only downside then is that it can't be a module, but I would guess that we can work on making that possible by allowing smp ops to be inserted and removed at any time.
On Tuesday 16 December 2014 11:18:18 Stephen Boyd wrote: > On 12/16/2014 06:38 AM, Arnd Bergmann wrote: > > On Tuesday 16 December 2014 15:12:22 Daniel Lezcano wrote: > >> At the beginning, all that become from not including mach files from the > >> drivers directory which make sense. > >> > >> Perhaps it is time to write a similar mechanism for the cpuidle drivers > >> where we can still separate the low level PM code from the generic > >> cpuidle code. > > That way you basically duplicate the same thing we already have, which > > isn't much better. > > > > In the example of drivers/soc/qcom/spm.c, just call cpuidle_register > > from the spm_dev_probe() function and be done with it. You already > > have a device that is responsible for handling this, don't try to > > construct more than you already need. > > > > I would assume that the same can be done for most other platforms. > > > > There are probably cases where the same piece of hardware is responsible > > for both cpuidle and cpufreq, but what that means is really that you > > should have a single driver for it that does both things. Same for > > SMP support: if you have one register block that does both the SMP > > bringup and the cpuidle stuff, then have *one* driver for this block > > that does it all. There are currently a few dependencies that require > > doing SMP bringup early during boot, but we decided years ago that those > > are all artificial dependencies and we should be able to boot secondary > > CPUs much later than we currently do. > > > > +1. The SPM harware is used for hotplug, suspend, cpuidle, as well as > provides a regulator for a CPU, so all these things should be done in a > single driver. Booting secondary CPUs early is not hard here either if > we move the smp ops into the same driver. The only downside then is that > it can't be a module, but I would guess that we can work on making that > possible by allowing smp ops to be inserted and removed at any time. Right, I don't see modular SMP operations happening any time soon, but it also doesn't seem like a fundamental restriction. Arnd -- 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/16/2014 03:38 PM, Arnd Bergmann wrote: > On Tuesday 16 December 2014 15:12:22 Daniel Lezcano wrote: >> At the beginning, all that become from not including mach files from the >> drivers directory which make sense. >> >> Perhaps it is time to write a similar mechanism for the cpuidle drivers >> where we can still separate the low level PM code from the generic >> cpuidle code. > > That way you basically duplicate the same thing we already have, which > isn't much better. > > In the example of drivers/soc/qcom/spm.c, just call cpuidle_register > from the spm_dev_probe() function and be done with it. You already > have a device that is responsible for handling this, don't try to > construct more than you already need. If you have to call cpuidle_register, then the cpuidle_driver should be provided with all the idle states definition and so on. That is exactly the opposite of what we have been doing since a couple of years where each SoC had their own driver, in their own directory and duplicating the code again and again from one platform to another platform. The changes we have been through cleaned up most of the situation but still we have more to do and I would like to prevent stepping back or give the opportunity to step back. I don't think we separated code which is not supposed to be. That has a positive side effect of cleaning up the drivers. Also, I understand your point and I am willing to solve this issue but still by keeping the pm low level code separated from the cpuidle driver. > I would assume that the same can be done for most other platforms. > > There are probably cases where the same piece of hardware is responsible > for both cpuidle and cpufreq, but what that means is really that you > should have a single driver for it that does both things. Same for > SMP support: if you have one register block that does both the SMP > bringup and the cpuidle stuff, then have *one* driver for this block > that does it all. There are currently a few dependencies that require > doing SMP bringup early during boot, but we decided years ago that those > are all artificial dependencies and we should be able to boot secondary > CPUs much later than we currently do. I agree with this point and given the number of drivers, the easiest way to do that is to create cpu pm ops as I gave in the previous email and reuse them for cpu hotplug/bringup. And I believe that is possible if we continue our approach by splitting the low level pm code from the cpuidle driver. What about doing something simple ? Like creating a struct arm_cpu_pm_ops variable visible from everywhere and filled by the different platform ?
On Wed, Dec 17, 2014 at 01:15:28PM +0000, Daniel Lezcano wrote: [...] > > I would assume that the same can be done for most other platforms. > > > > There are probably cases where the same piece of hardware is responsible > > for both cpuidle and cpufreq, but what that means is really that you > > should have a single driver for it that does both things. Same for > > SMP support: if you have one register block that does both the SMP > > bringup and the cpuidle stuff, then have *one* driver for this block > > that does it all. There are currently a few dependencies that require > > doing SMP bringup early during boot, but we decided years ago that those > > are all artificial dependencies and we should be able to boot secondary > > CPUs much later than we currently do. > > I agree with this point and given the number of drivers, the easiest way > to do that is to create cpu pm ops as I gave in the previous email and > reuse them for cpu hotplug/bringup. And I believe that is possible if we > continue our approach by splitting the low level pm code from the > cpuidle driver. > > What about doing something simple ? Like creating a struct > arm_cpu_pm_ops variable visible from everywhere and filled by the > different platform ? I agree with this approach, which by the way consists in defining cpu operations as ARM64 does and use those from cpuidle, suspend and hotplug code. The MCPM interface does that already, probably we should enhance/rename it since people think it must be used for multicluster power management whereas it is a pretty generic [drivers -> mach] code interface (I am talking about the API, not the synchronization scheme), and can certainly be extended/refactored according to new platforms need. Thanks, Lorenzo -- 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 Tue, Dec 16 2014 at 12:18 -0700, Stephen Boyd wrote: >On 12/16/2014 06:38 AM, Arnd Bergmann wrote: >> On Tuesday 16 December 2014 15:12:22 Daniel Lezcano wrote: >>> At the beginning, all that become from not including mach files from the >>> drivers directory which make sense. >>> >>> Perhaps it is time to write a similar mechanism for the cpuidle drivers >>> where we can still separate the low level PM code from the generic >>> cpuidle code. >> That way you basically duplicate the same thing we already have, which >> isn't much better. >> >> In the example of drivers/soc/qcom/spm.c, just call cpuidle_register >> from the spm_dev_probe() function and be done with it. You already >> have a device that is responsible for handling this, don't try to >> construct more than you already need. >> >> I would assume that the same can be done for most other platforms. >> >> There are probably cases where the same piece of hardware is responsible >> for both cpuidle and cpufreq, but what that means is really that you >> should have a single driver for it that does both things. Same for >> SMP support: if you have one register block that does both the SMP >> bringup and the cpuidle stuff, then have *one* driver for this block >> that does it all. There are currently a few dependencies that require >> doing SMP bringup early during boot, but we decided years ago that those >> are all artificial dependencies and we should be able to boot secondary >> CPUs much later than we currently do. >> > >+1. The SPM harware is used for hotplug, suspend, cpuidle, as well as >provides a regulator for a CPU, so all these things should be done in a >single driver. Booting secondary CPUs early is not hard here either if >we move the smp ops into the same driver. The only downside then is that >it can't be a module, but I would guess that we can work on making that >possible by allowing smp ops to be inserted and removed at any time. > I am not sure, just because the hardware supports multiple functionality, everything should go into the same file. If you think of a SPM driver as something that provides a service, you would see that all the functionality of cpuidle, hotplug, suspend, use the service and can have their own place to share their commonality. SPM h/w is not just for cpus. It could be a generic power controller (to an extent) for the cpu, cache, coherenency interface and pretty much any bus/clock exports the services of that device. There needs to be a distinction between an entity that provides the functionality and a one that uses it. You cannot build on additional functionality neatly if you have them all together. As such, I am quite appalled by the addition of the idle functions in the SPM driver, but seems like its the direction many want, hence went with it. But, please dont suggest using SPM driver as a dumping ground for all functionality that SPM may indirectly influence. Thanks, Lina -- 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
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..90812c5 --- /dev/null +++ b/drivers/soc/qcom/spm.c @@ -0,0 +1,338 @@ +/* + * 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]); +} + +/* Ensure a guaranteed write, before return */ +static inline void spm_register_write_sync(struct spm_driver_data *drv, + enum spm_reg reg, u32 val) +{ + u32 ret; + + if (!drv->reg_data->reg_offset[reg]) + return; + + do { + writel_relaxed(val, drv->reg_base + + drv->reg_data->reg_offset[reg]); + ret = readl_relaxed(drv->reg_base + + drv->reg_data->reg_offset[reg]); + if (ret == val) + break; + cpu_relax(); + } while (1); +} + +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 void 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; + + 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_sync(drv, SPM_REG_SPM_CTL, ctl_val); +} + +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; + ret = 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 ret; +} + +static int qcom_cpu_standby(void *unused) +{ + spm_set_low_power_mode(PM_SLEEP_MODE_STBY); + cpu_do_idle(); + + return 0; +} + +static int qcom_cpu_spc(void *unused) +{ + int ret; + + spm_set_low_power_mode(PM_SLEEP_MODE_STBY); + cpu_pm_enter(); + ret = cpu_suspend(0, qcom_pm_collapse); + cpu_pm_exit(); + + return ret; +} + +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; + + 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); + found = (saw_node == pdev->dev.of_node); + 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 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; + struct cpuidle_device *dev; + + drv = spm_get_drv(pdev, &cpu); + if (!drv) + return -EINVAL; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + drv->reg_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(drv->reg_base)) + return PTR_ERR(drv->reg_base); + + match_id = of_match_node(spm_match_table, pdev->dev.of_node); + if (!match_id) + return -ENODEV; + + drv->reg_data = match_id->data; + + /* 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 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]); + + per_cpu(cpu_spm_drv, cpu) = drv; + + /* Register the cpuidle device for the cpu, we are ready for cpuidle */ + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); + if (!dev) + return -ENOMEM; + + dev->cpu = cpu; + return cpuidle_register_device(dev); +} + +static struct qcom_cpu_pm_ops lpm_ops = { + .standby = qcom_cpu_standby, + .spc = qcom_cpu_spc, +}; + +static struct platform_device qcom_cpuidle_drv = { + .name = "qcom_cpuidle", + .id = -1, + .dev.platform_data = &lpm_ops, +}; + +static struct platform_driver spm_driver = { + .probe = spm_dev_probe, + .driver = { + .name = "saw", + .of_match_table = spm_match_table, + }, +}; + +static int __init qcom_spm_init(void) +{ + int ret; + + /* + * cpuidle driver need to registered before the cpuidle device + * for any cpu. Register the device for the the cpuidle driver. + */ + ret = platform_device_register(&qcom_cpuidle_drv); + if (ret) + return ret; + + return platform_driver_register(&spm_driver); +} +module_init(qcom_spm_init); + +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 */
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> --- .../devicetree/bindings/arm/msm/qcom,saw2.txt | 31 +- drivers/soc/qcom/Kconfig | 8 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/spm.c | 338 +++++++++++++++++++++ include/soc/qcom/pm.h | 31 ++ 5 files changed, 403 insertions(+), 6 deletions(-) create mode 100644 drivers/soc/qcom/spm.c create mode 100644 include/soc/qcom/pm.h