diff mbox

[RFC,1/3] ARM64: meson: Add Amlogic Meson GX PM Suspend

Message ID 1478183365-23708-2-git-send-email-narmstrong@baylibre.com (mailing list archive)
State Superseded
Headers show

Commit Message

Neil Armstrong Nov. 3, 2016, 2:29 p.m. UTC 
The Amlogic Meson GX SoCs uses a non-standard argument to the
PSCI CPU_SUSPEND call to enter system suspend.

Implement such call within platform_suspend_ops.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/meson/Kconfig       |  6 +++
 drivers/firmware/meson/Makefile      |  1 +
 drivers/firmware/meson/meson_gx_pm.c | 86 ++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 drivers/firmware/meson/meson_gx_pm.c

Comments

Mark Rutland Nov. 3, 2016, 3:30 p.m. UTC | #1 
On Thu, Nov 03, 2016 at 03:29:23PM +0100, Neil Armstrong wrote:
> The Amlogic Meson GX SoCs uses a non-standard argument to the
> PSCI CPU_SUSPEND call to enter system suspend.
> 
> Implement such call within platform_suspend_ops.

While I am very much not happy with platform-specific extensions to PSCI, if
this needs to go anywhere, it should live in the existing PSCI driver.

That is to say, NAK to other driver invoking PSCI functions.

Thanks,
Mark.

> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/meson/Kconfig       |  6 +++
>  drivers/firmware/meson/Makefile      |  1 +
>  drivers/firmware/meson/meson_gx_pm.c | 86 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 drivers/firmware/meson/meson_gx_pm.c
> 
> diff --git a/drivers/firmware/meson/Kconfig b/drivers/firmware/meson/Kconfig
> index 170d7e8..5b3fea3 100644
> --- a/drivers/firmware/meson/Kconfig
> +++ b/drivers/firmware/meson/Kconfig
> @@ -7,3 +7,9 @@ config MESON_SM
>  	depends on ARM64_4K_PAGES
>  	help
>  	  Say y here to enable the Amlogic secure monitor driver
> +
> +config MESON_GX_PM
> +	bool
> +	default ARCH_MESON if ARM64
> +	help
> +	  Say y here to enable the Amlogic GX SoC Power Management
> diff --git a/drivers/firmware/meson/Makefile b/drivers/firmware/meson/Makefile
> index 9ab3884..b6e285d 100644
> --- a/drivers/firmware/meson/Makefile
> +++ b/drivers/firmware/meson/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_MESON_SM) +=	meson_sm.o
> +obj-$(CONFIG_MESON_GX_PM) +=	meson_gx_pm.o
> diff --git a/drivers/firmware/meson/meson_gx_pm.c b/drivers/firmware/meson/meson_gx_pm.c
> new file mode 100644
> index 0000000..c104c2e
> --- /dev/null
> +++ b/drivers/firmware/meson/meson_gx_pm.c
> @@ -0,0 +1,86 @@
> +/*
> + * Amlogic Meson GX Power Management
> + *
> + * Copyright (c) 2016 Baylibre, SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License 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/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/suspend.h>
> +#include <linux/arm-smccc.h>
> +
> +#include <uapi/linux/psci.h>
> +
> +#include <asm/suspend.h>
> +
> +/*
> + * The Amlogic GX SoCs uses a special argument value to the
> + * PSCI CPU_SUSPEND method to enter SUSPEND_MEM.
> + */
> +
> +#define MESON_SUSPEND_PARAM	0x0010000
> +#define PSCI_FN_NATIVE(version, name)	PSCI_##version##_FN64_##name
> +
> +static int meson_gx_suspend_finish(unsigned long arg)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(PSCI_FN_NATIVE(0_2, CPU_SUSPEND), arg,
> +		      virt_to_phys(cpu_resume), 0, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
> +static int meson_gx_suspend_enter(suspend_state_t state)
> +{
> +	switch (state) {
> +	case PM_SUSPEND_MEM:
> +		return cpu_suspend(MESON_SUSPEND_PARAM,
> +				   meson_gx_suspend_finish);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct platform_suspend_ops meson_gx_pm_ops = {
> +		.enter = meson_gx_suspend_enter,
> +		.valid = suspend_valid_only_mem,
> +};
> +
> +static const struct of_device_id meson_gx_pm_match[] = {
> +	{ .compatible = "amlogic,meson-gx-pm", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, meson_gx_pm_match);
> +
> +static int meson_gx_pm_probe(struct platform_device *pdev)
> +{
> +	suspend_set_ops(&meson_gx_pm_ops);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver meson_gx_pm_driver = {
> +	.probe = meson_gx_pm_probe,
> +	.driver = {
> +		.name = "meson-gx-pm",
> +		.of_match_table = meson_gx_pm_match,
> +	},
> +};
> +
> +module_platform_driver(meson_gx_pm_driver);
> +
> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
> +MODULE_DESCRIPTION("Amlogic Meson GX PM driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Sudeep Holla Nov. 3, 2016, 9:53 p.m. UTC | #2 
On Thu, Nov 03, 2016 at 03:29:23PM +0100, Neil Armstrong wrote:
> The Amlogic Meson GX SoCs uses a non-standard argument to the
> PSCI CPU_SUSPEND call to enter system suspend.
> 
> Implement such call within platform_suspend_ops.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/meson/Kconfig       |  6 +++
>  drivers/firmware/meson/Makefile      |  1 +
>  drivers/firmware/meson/meson_gx_pm.c | 86 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 drivers/firmware/meson/meson_gx_pm.c
> 
> diff --git a/drivers/firmware/meson/Kconfig b/drivers/firmware/meson/Kconfig
> index 170d7e8..5b3fea3 100644
> --- a/drivers/firmware/meson/Kconfig
> +++ b/drivers/firmware/meson/Kconfig
> @@ -7,3 +7,9 @@ config MESON_SM
>  	depends on ARM64_4K_PAGES
>  	help
>  	  Say y here to enable the Amlogic secure monitor driver
> +
> +config MESON_GX_PM
> +	bool
> +	default ARCH_MESON if ARM64
> +	help
> +	  Say y here to enable the Amlogic GX SoC Power Management
> diff --git a/drivers/firmware/meson/Makefile b/drivers/firmware/meson/Makefile
> index 9ab3884..b6e285d 100644
> --- a/drivers/firmware/meson/Makefile
> +++ b/drivers/firmware/meson/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_MESON_SM) +=	meson_sm.o
> +obj-$(CONFIG_MESON_GX_PM) +=	meson_gx_pm.o
> diff --git a/drivers/firmware/meson/meson_gx_pm.c b/drivers/firmware/meson/meson_gx_pm.c
> new file mode 100644
> index 0000000..c104c2e
> --- /dev/null
> +++ b/drivers/firmware/meson/meson_gx_pm.c
> @@ -0,0 +1,86 @@
> +/*
> + * Amlogic Meson GX Power Management
> + *
> + * Copyright (c) 2016 Baylibre, SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License 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/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/suspend.h>
> +#include <linux/arm-smccc.h>
> +
> +#include <uapi/linux/psci.h>
> +
> +#include <asm/suspend.h>
> +
> +/*
> + * The Amlogic GX SoCs uses a special argument value to the
> + * PSCI CPU_SUSPEND method to enter SUSPEND_MEM.
> + */
> +
> +#define MESON_SUSPEND_PARAM	0x0010000
> +#define PSCI_FN_NATIVE(version, name)	PSCI_##version##_FN64_##name
> +
> +static int meson_gx_suspend_finish(unsigned long arg)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(PSCI_FN_NATIVE(0_2, CPU_SUSPEND), arg,
> +		      virt_to_phys(cpu_resume), 0, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
> +static int meson_gx_suspend_enter(suspend_state_t state)
> +{
> +	switch (state) {
> +	case PM_SUSPEND_MEM:
> +		return cpu_suspend(MESON_SUSPEND_PARAM,
> +				   meson_gx_suspend_finish);

Short response to this patch: NAK

To be constructive, since this system lacks PSCI system suspend, it just
can't support. Alternatively, this can be normal cpuidle state and you
can use suspend to idle to achieve the same and you need not hack/work
around using platform specific driver.

--
Regards,
Sudeep
Neil Armstrong Nov. 4, 2016, 9:13 a.m. UTC | #3 
On 11/03/2016 10:53 PM, Sudeep Holla wrote:
> 
> Short response to this patch: NAK
> 
> To be constructive, since this system lacks PSCI system suspend, it just
> can't support. Alternatively, this can be normal cpuidle state and you
> can use suspend to idle to achieve the same and you need not hack/work
> around using platform specific driver.
> 
> --
> Regards,
> Sudeep
> 

Hi Mark, Sudeep,

Thanks for your replies.
First of all, I never intended to have this patchset Acked, I certainly know this is a bad hack !

But, in the current Linux PSCI implementation, there is no way to handle System Suspend when the
SoC FW implements PSCI 0.2, except idle-to-suspend which is quite different since the FW does not handle the wakeup.

Don't worry, I'll prefer Amlogic to conform to PSCI 1.0, but like the SCPI, the GXBB was implemented
using an early version of these FW protocols... and the SoC is here and should be supported somehow.

I have a simple question : Could it be possible to declare an idle-state to be used exclusively by suspend-to-mem ?
For example while parsing the idle-states, if we encounter a particular property, the we save the state, don't
add it to the cpuidle states and register a platform_suspend_ops using this state.

Would it be accepted to be able to select a declared DT idle-state and reserve it to suspend-to-mem state ?

In the Amlogic case, their CPU_SUSPEND is partially conform, but since the power_state parameter was left as
implementation defined, they added a deeper cluster sleep state.
But potentially, we could need to handle system suspend on PSCI0.2 systems using a particular idle-state ?

Yes, Sudeep mentioned suspend-to-idle, but in our tests the kernel enters each idle-state at boot time and when
we declare this deep sleep idle state, it makes the whole system enter this system suspend state.

Neil
diff mbox

Patch

diff --git a/drivers/firmware/meson/Kconfig b/drivers/firmware/meson/Kconfig
index 170d7e8..5b3fea3 100644
--- a/drivers/firmware/meson/Kconfig
+++ b/drivers/firmware/meson/Kconfig
@@ -7,3 +7,9 @@  config MESON_SM
 	depends on ARM64_4K_PAGES
 	help
 	  Say y here to enable the Amlogic secure monitor driver
+
+config MESON_GX_PM
+	bool
+	default ARCH_MESON if ARM64
+	help
+	  Say y here to enable the Amlogic GX SoC Power Management
diff --git a/drivers/firmware/meson/Makefile b/drivers/firmware/meson/Makefile
index 9ab3884..b6e285d 100644
--- a/drivers/firmware/meson/Makefile
+++ b/drivers/firmware/meson/Makefile
@@ -1 +1,2 @@ 
 obj-$(CONFIG_MESON_SM) +=	meson_sm.o
+obj-$(CONFIG_MESON_GX_PM) +=	meson_gx_pm.o
diff --git a/drivers/firmware/meson/meson_gx_pm.c b/drivers/firmware/meson/meson_gx_pm.c
new file mode 100644
index 0000000..c104c2e
--- /dev/null
+++ b/drivers/firmware/meson/meson_gx_pm.c
@@ -0,0 +1,86 @@ 
+/*
+ * Amlogic Meson GX Power Management
+ *
+ * Copyright (c) 2016 Baylibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License 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/of.h>
+#include <linux/platform_device.h>
+#include <linux/suspend.h>
+#include <linux/arm-smccc.h>
+
+#include <uapi/linux/psci.h>
+
+#include <asm/suspend.h>
+
+/*
+ * The Amlogic GX SoCs uses a special argument value to the
+ * PSCI CPU_SUSPEND method to enter SUSPEND_MEM.
+ */
+
+#define MESON_SUSPEND_PARAM	0x0010000
+#define PSCI_FN_NATIVE(version, name)	PSCI_##version##_FN64_##name
+
+static int meson_gx_suspend_finish(unsigned long arg)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(PSCI_FN_NATIVE(0_2, CPU_SUSPEND), arg,
+		      virt_to_phys(cpu_resume), 0, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
+static int meson_gx_suspend_enter(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_MEM:
+		return cpu_suspend(MESON_SUSPEND_PARAM,
+				   meson_gx_suspend_finish);
+	}
+
+	return -EINVAL;
+}
+
+static const struct platform_suspend_ops meson_gx_pm_ops = {
+		.enter = meson_gx_suspend_enter,
+		.valid = suspend_valid_only_mem,
+};
+
+static const struct of_device_id meson_gx_pm_match[] = {
+	{ .compatible = "amlogic,meson-gx-pm", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, meson_gx_pm_match);
+
+static int meson_gx_pm_probe(struct platform_device *pdev)
+{
+	suspend_set_ops(&meson_gx_pm_ops);
+
+	return 0;
+}
+
+static struct platform_driver meson_gx_pm_driver = {
+	.probe = meson_gx_pm_probe,
+	.driver = {
+		.name = "meson-gx-pm",
+		.of_match_table = meson_gx_pm_match,
+	},
+};
+
+module_platform_driver(meson_gx_pm_driver);
+
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_DESCRIPTION("Amlogic Meson GX PM driver");
+MODULE_LICENSE("GPL v2");