cpuidle driver for QCOM SoCs: 8064, 8074, 8084

drivers/soc/qcom/spm.c new file +74 -0

+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)

drivers/soc/qcom/spm.c new file +8 -0

+
+	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");

include/soc/qcom/pm.h new file +31 -0

new file mode 100644
index 0000000..d9a56d7
@@ -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 */

Signature · 1 line

2.1.0

On Tue, Dec 02 2014 at 10:40 -0700, Lina Iyer wrote:
+
[...]

Quoted 5 lines

drivers/soc/qcom/spm.c new file +5 -0

+static int __init qcom_spm_init(void)
+{
+    int ret;
+
+    /*

drivers/soc/qcom/spm.c line 330

Quoted 6 lines

drivers/soc/qcom/spm.c new file +6 -0

+     * 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

Quoted 48 lines

drivers/soc/qcom/spm.c new file +8 -0

+
+    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");

include/soc/qcom/pm.h new file +31 -1

new file mode 100644
index 0000000..d9a56d7
@@ -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

Quoted 3 lines · Lina Iyer

On Tue, Dec 02 2014 at 10:40 -0700, Lina Iyer wrote:
+
[...]

[ ... ]

Quoted 5 lines

Quoted 5 lines

drivers/soc/qcom/spm.c new file +5 -0

+static int __init qcom_spm_init(void)
+{
+    int ret;
+
+    /*

Quoted 8 lines

drivers/soc/qcom/spm.c line 330

Quoted 6 lines

drivers/soc/qcom/spm.c new file +6 -0

+     * 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 ?

Quoted 64 lines

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

Quoted 48 lines

drivers/soc/qcom/spm.c new file +8 -0

+
+    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");

include/soc/qcom/pm.h new file +31 -1

new file mode 100644
index 0000000..d9a56d7
@@ -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

Signature · 5 lines

<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

Quoted 21 lines · Daniel Lezcano

Quoted 3 lines · Lina Iyer

On Tue, Dec 02 2014 at 10:40 -0700, Lina Iyer wrote:
+
[...]

[ ... ]

Quoted 13 lines

drivers/soc/qcom/spm.c line 330

Quoted 11 lines

drivers/soc/qcom/spm.c new file +11 -0

+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.

Quoted 2 lines

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.

Quoted 73 lines

Quoted 64 lines

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

Quoted 48 lines

drivers/soc/qcom/spm.c new file +8 -0

+
+    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");

include/soc/qcom/pm.h new file +31 -1

new file mode 100644
index 0000000..d9a56d7
@@ -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

Signature · 5 lines

<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

Quoted 14 lines

Quoted 13 lines

drivers/soc/qcom/spm.c line 330

Quoted 11 lines

drivers/soc/qcom/spm.c new file +11 -0

+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.

Quoted 2 lines

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.

Quoted 14 lines · On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote:

Quoted 14 lines

Quoted 13 lines

drivers/soc/qcom/spm.c line 330

Quoted 11 lines

drivers/soc/qcom/spm.c new file +11 -0

+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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 23 lines · Arnd Bergmann

Quoted 14 lines · On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote:

Quoted 14 lines

Quoted 13 lines

drivers/soc/qcom/spm.c line 330

Quoted 11 lines

drivers/soc/qcom/spm.c new file +11 -0

+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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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 ?

Quoted 32 lines · On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote:

Quoted 23 lines · Arnd Bergmann

Quoted 14 lines · On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote:

Quoted 14 lines

Quoted 13 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 33 lines · Arnd Bergmann

Quoted 32 lines · On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote:

Quoted 23 lines · Arnd Bergmann

Quoted 14 lines · On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote:

Quoted 14 lines

Quoted 13 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 8 lines

Quoted 5 lines

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?

Quoted 46 lines · On Thursday 04 December 2014 09:28:34 Lina Iyer wrote:

Quoted 33 lines · Arnd Bergmann

Quoted 32 lines · On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote:

Quoted 23 lines · Arnd Bergmann

Quoted 14 lines · On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote:

Quoted 14 lines

Quoted 13 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 7 lines · Arnd Bergmann

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?

Quoted 46 lines · On Thursday 04 December 2014 09:28:34 Lina Iyer wrote:

Quoted 33 lines · Arnd Bergmann

Quoted 32 lines · On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote:

Quoted 23 lines · Arnd Bergmann

Quoted 14 lines · On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote:

Quoted 14 lines

Quoted 13 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 6 lines · 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.

Quoted 17 lines · Arnd Bergmann

Quoted 6 lines · 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.

Quoted 10 lines

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.

Quoted 17 lines · Arnd Bergmann

Quoted 6 lines · 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.

Quoted 10 lines

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.

Quoted 6 lines · 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.

Quoted 11 lines

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 ?

Dependent patchsets -

        https://lkml.org/lkml/2014/8/4/767
        http://www.spinics.net/lists/linux-arm-msm/msg10799.html
        http://www.spinics.net/lists/linux-arm-msm/msg10795.html

Changes since v13:
- Return values for idle states propagated back to the cpuidle driver.
- Remove static bool cpuidle_drv_init.
- Register cpuidle driver and cpuidle device separately.
- cpuidle device registered only when the SPM for the cpu is probed.
- Initialization changes to ensure dynamic cpuidle devices are registered only after the cpuidle driver is registered.
- Removed wmb, replaced with a poll loop to ensure that the SPM registers are written before executing wfi.
- Added spm_register_write_sync() for write guarantees.
- Removed irrelevant return value for spm_set_low_power_mode().
- Added comments, updated module description.
- Removed Reviewed-by and Acked-by on the spm and cpuidle-qcom patches.

Quoted 5 lines · Lina Iyer

Dependent patchsets -

        https://lkml.org/lkml/2014/8/4/767
        http://www.spinics.net/lists/linux-arm-msm/msg10799.html
        http://www.spinics.net/lists/linux-arm-msm/msg10795.html

Quoted 13 lines

Changes since v13:
- Return values for idle states propagated back to the cpuidle driver.
- Remove static bool cpuidle_drv_init.
- Register cpuidle driver and cpuidle device separately.
- cpuidle device registered only when the SPM for the cpu is probed.
- Initialization changes to ensure dynamic cpuidle devices are registered only after the cpuidle driver is registered.
- Removed wmb, replaced with a poll loop to ensure that the SPM registers are written before executing wfi.
- Added spm_register_write_sync() for write guarantees.
- Removed irrelevant return value for spm_set_low_power_mode().
- Added comments, updated module description.
- Removed Reviewed-by and Acked-by on the spm and cpuidle-qcom patches.

I think this series should also include updates to qcom_defconfig that enable QCOM_PM and the cpuidle driver.

Then it will be enabled by default and get more broad testing.

Kevin