Re: [RFC PATCH 4/9] soc: samsung: Add generic power-management driver for Exynos

From: Chanwoo Choi
Date: Thu Jan 11 2018 - 00:44:18 EST

Next message: Ingo Molnar: "Re: [GIT PULL 00/27] perf/core improvements and fixes"
Previous message: Chanwoo Choi: "Re: [RFC PATCH 5/9] soc: samsung: pm: Add support for suspend-to-ram of Exynos5433"
In reply to: Krzysztof Kozlowski: "Re: [RFC PATCH 4/9] soc: samsung: Add generic power-management driver for Exynos"
Next in thread: Chanwoo Choi: "[PATCH 6/9] arm64: dts: exynos: Add iRAM device-tree node for Exynos5433"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Dear Krzysztof,

I'll try to consolidate the pm code for both arm and arm64.
So, drop this patch and then I'll start to move the code
from arch/arm/mach-exynos/* to drivers/soc/samsung/*.

But, I'm not sure it is possible to move all codes
to drivers/soc/samsung/*. I'll try it.

Best Regards,
Chanwoo Choi
Samsung Electronics

On 2018ë 01ì 09ì 21:37, Krzysztof Kozlowski wrote:
> On Tue, Jan 9, 2018 at 8:59 AM, Chanwoo Choi <cw00.choi@xxxxxxxxxxx> wrote:
>> To enter suspend, Exynos SoC requires the some machine dependent procedures.
>> This patch introduces the generic power-management driver to support
>> those requirements and generic interface for power state management.
>>
>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@xxxxxxxxxxx>
>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> ---
>> arch/arm/mach-exynos/common.h | 1 -
>> arch/arm/mach-exynos/exynos.c | 23 +----
>> drivers/soc/samsung/Makefile | 2 +-
>> drivers/soc/samsung/exynos-pm.c | 176 ++++++++++++++++++++++++++++++++++
>> include/linux/soc/samsung/exynos-pm.h | 21 ++++
>> 5 files changed, 199 insertions(+), 24 deletions(-)
>> create mode 100644 drivers/soc/samsung/exynos-pm.c
>> create mode 100644 include/linux/soc/samsung/exynos-pm.h
>>
>> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
>> index afbc143a3d5d..ad482c0fc131 100644
>> --- a/arch/arm/mach-exynos/common.h
>> +++ b/arch/arm/mach-exynos/common.h
>> @@ -119,7 +119,6 @@ enum {
>> * Magic values for bootloader indicating chosen low power mode.
>> * See also Documentation/arm/Samsung/Bootloader-interface.txt
>> */
>> -#define EXYNOS_SLEEP_MAGIC 0x00000bad
>> #define EXYNOS_AFTR_MAGIC 0xfcba0d10
>>
>> void exynos_set_boot_flag(unsigned int cpu, unsigned int mode);
>> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
>> index fbd108ce8745..0d5265d175c4 100644
>> --- a/arch/arm/mach-exynos/exynos.c
>> +++ b/arch/arm/mach-exynos/exynos.c
>> @@ -12,6 +12,7 @@
>> #include <linux/of_fdt.h>
>
> of_address.h might be not needed anymore.
>
>> #include <linux/platform_device.h>
>> #include <linux/irqchip.h>
>> +#include <linux/soc/samsung/exynos-pm.h>
>> #include <linux/soc/samsung/exynos-regs-pmu.h>
>>
>> #include <asm/cacheflush.h>
>> @@ -41,28 +42,6 @@
>> .id = -1,
>> };
>>
>> -void __iomem *sysram_base_addr __ro_after_init;
>> -void __iomem *sysram_ns_base_addr __ro_after_init;
>> -
>> -void __init exynos_sysram_init(void)
>> -{
>> - struct device_node *node;
>> -
>> - for_each_compatible_node(node, NULL, "samsung,exynos4210-sysram") {
>> - if (!of_device_is_available(node))
>> - continue;
>> - sysram_base_addr = of_iomap(node, 0);
>> - break;
>> - }
>> -
>> - for_each_compatible_node(node, NULL, "samsung,exynos4210-sysram-ns") {
>> - if (!of_device_is_available(node))
>> - continue;
>> - sysram_ns_base_addr = of_iomap(node, 0);
>> - break;
>> - }
>> -}
>> -
>> static void __init exynos_init_late(void)
>> {
>> if (of_machine_is_compatible("samsung,exynos5440"))
>> diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
>> index d2e637339a45..58ca5bdabf1f 100644
>> --- a/drivers/soc/samsung/Makefile
>> +++ b/drivers/soc/samsung/Makefile
>> @@ -1,5 +1,5 @@
>> # SPDX-License-Identifier: GPL-2.0
>> -obj-$(CONFIG_EXYNOS_PMU) += exynos-pmu.o
>> +obj-$(CONFIG_EXYNOS_PMU) += exynos-pmu.o exynos-pm.o
>>
>> obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS) += exynos3250-pmu.o exynos4-pmu.o \
>> exynos5250-pmu.o exynos5420-pmu.o \
>> diff --git a/drivers/soc/samsung/exynos-pm.c b/drivers/soc/samsung/exynos-pm.c
>> new file mode 100644
>> index 000000000000..45d84bbe5e61
>> --- /dev/null
>> +++ b/drivers/soc/samsung/exynos-pm.c
>> @@ -0,0 +1,176 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// based on arch/arm/mach-exynos/suspend.c
>> +// Copyright (c) 2018 Samsung Electronics Co., Ltd.
>> +//
>> +// Exynos Power Management support driver
>> +
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/regulator/machine.h>
>> +#include <linux/syscore_ops.h>
>> +#include <linux/suspend.h>
>> +
>> +#include <asm/cpuidle.h>
>> +#include <asm/io.h>
>> +#include <asm/suspend.h>
>> +
>> +#include <linux/soc/samsung/exynos-pm.h>
>> +#include <linux/soc/samsung/exynos-pmu.h>
>> +
>> +/*
>> + * The struct exynos_pm_data contains the callbacks of
>> + * both struct platform_suspend_ops and syscore_ops.
>> + * This structure is listed according to the call order,
>> + * because the callback call order for the two structures is mixed.
>> + */
>> +struct exynos_pm_data {
>> + int (*prepare)(void); /* for platform_suspend_ops */
>> + int (*suspend)(void); /* for syscore_ops */
>> + int (*enter)(suspend_state_t state); /* for platform_suspend_ops */
>> + void (*resume)(void); /* for syscore_ops */
>> + void (*finish)(void); /* for platform_suspend_ops */
>> +};
>> +
>> +static struct platform_suspend_ops exynos_pm_suspend_ops;
>> +static struct syscore_ops exynos_pm_syscore_ops;
>> +static const struct exynos_pm_data *pm_data __ro_after_init;
>
> It is already const, so __initconst?
>
>> +
>> +void __iomem *sysram_base_addr __ro_after_init;
>> +void __iomem *sysram_ns_base_addr __ro_after_init;
>> +
>> +static int exynos_pm_prepare(void)
>> +{
>> + int ret;
>> +
>> + /*
>> + * REVISIT: It would be better if struct platform_suspend_ops
>> + * .prepare handler get the suspend_state_t as a parameter to
>> + * avoid hard-coding the suspend to mem state. It's safe to do
>> + * it now only because the suspend_valid_only_mem function is
>> + * used as the .valid callback used to check if a given state
>> + * is supported by the platform anyways.
>> + */
>> + ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
>> + if (ret) {
>> + pr_err("Failed to prepare regulators for suspend (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + if (pm_data->prepare) {
>> + ret = pm_data->prepare();
>> + if (ret) {
>> + pr_err("Failed to prepare for suspend (%d)\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_pm_suspend(void)
>> +{
>> + if (pm_data->suspend)
>> + return pm_data->suspend();
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_pm_enter(suspend_state_t state)
>> +{
>> + int ret;
>> +
>> + exynos_sys_powerdown_conf(SYS_SLEEP);
>> +
>> + ret = pm_data->enter(state);
>> + if (ret) {
>> + pr_err("Failed to enter sleep\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void exynos_pm_resume(void)
>> +{
>> + exynos_sys_powerup_conf(SYS_SLEEP);
>> +
>> + if (pm_data->resume)
>> + pm_data->resume();
>> +}
>> +
>> +static void exynos_pm_finish(void)
>> +{
>> + int ret;
>> +
>> + ret = regulator_suspend_finish();
>> + if (ret)
>> + pr_warn("Failed to resume regulators from suspend (%d)\n", ret);
>> +
>> + if (pm_data->finish)
>> + pm_data->finish();
>> +}
>> +
>> +/*
>> + * Split the data between ARM architectures because it is relatively big
>> + * and useless on other arch.
>> + */
>> +#ifdef CONFIG_EXYNOS_PMU_ARM_DRIVERS
>> +#define exynos_pm_data_arm_ptr(data) (&data)
>> +#else
>> +#define exynos_pm_data_arm_ptr(data) NULL
>> +#endif
>> +
>> +static const struct of_device_id exynos_pm_of_device_ids[] = {
>> + { /*sentinel*/ },
>> +};
>> +
>> +void __init exynos_sysram_init(void)
>> +{
>> + struct device_node *np;
>> +
>> + for_each_compatible_node(np, NULL, "samsung,exynos4210-sysram") {
>> + if (!of_device_is_available(np))
>> + continue;
>> + sysram_base_addr = of_iomap(np, 0);
>> + break;
>> + }
>> +
>> + for_each_compatible_node(np, NULL, "samsung,exynos4210-sysram-ns") {
>> + if (!of_device_is_available(np))
>> + continue;
>> + sysram_ns_base_addr = of_iomap(np, 0);
>> + break;
>> + }
>> +}
>> +
>> +static int __init exynos_pm_init(void)
>> +{
>> + const struct of_device_id *match;
>> + struct device_node *np;
>> +
>> + np = of_find_matching_node_and_match(NULL,
>> + exynos_pm_of_device_ids, &match);
>> + if (!np) {
>> + pr_err("Failed to find PMU node for Exynos Power-Management\n");
>> + return -ENODEV;
>> + }
>> + pm_data = (const struct exynos_pm_data *) match->data;
>> +
>> + exynos_sysram_init();
>> +
>> + exynos_pm_suspend_ops.valid = suspend_valid_only_mem;
>> + exynos_pm_suspend_ops.prepare = exynos_pm_prepare;
>> + exynos_pm_syscore_ops.suspend = exynos_pm_suspend;
>> + exynos_pm_suspend_ops.enter = exynos_pm_enter;
>> + exynos_pm_syscore_ops.resume = exynos_pm_resume;
>> + exynos_pm_suspend_ops.finish = exynos_pm_finish;
>> +
>> + register_syscore_ops(&exynos_pm_syscore_ops);
>> + suspend_set_ops(&exynos_pm_suspend_ops);
>> +
>> + return 0;
>> +}
>> +postcore_initcall(exynos_pm_init);
>
> As I mentioned in cover letter, please move here first ARMv7 code. Now
> it looks like duplicating the existing code.
>
>> diff --git a/include/linux/soc/samsung/exynos-pm.h b/include/linux/soc/samsung/exynos-pm.h
>> new file mode 100644
>> index 000000000000..b1afe95ed10c
>> --- /dev/null
>> +++ b/include/linux/soc/samsung/exynos-pm.h
>> @@ -0,0 +1,21 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// Copyright (c) 2018 Samsung Electronics Co., Ltd.
>> +//
>> +// Header for Exynos Power-Management support driver
>
> Use header-style SPDX and comment.
>
> Best regards,
> Krzysztof
>
>> +
>> +#ifndef __LINUX_SOC_EXYNOS_PM_H
>> +#define __LINUX_SOC_EXYNOS_PM_H
>> +
>> +/*
>> + * Magic values for bootloader indicating chosen low power mode.
>> + * See also Documentation/arm/Samsung/Bootloader-interface.txt
>> + */
>> +#define EXYNOS_SLEEP_MAGIC 0x00000bad
>> +
>> +extern void __iomem *sysram_base_addr;
>> +extern void __iomem *sysram_ns_base_addr;
>
> Since these are now global symbols, they need nice exynos prefix.
> Also, probably they should not be globally modifiable. Only
> exynos_sysram_init() should write there. Instead export a global
> accessor (get()) and rest should use that one.
>
> Best regards,
> Krzysztof
>
>> +
>> +extern void exynos_sysram_init(void);
>> +
>> +#endif /* __LINUX_SOC_EXYNOS_PMU_H */
>> --
>> 1.9.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>

Next message: Ingo Molnar: "Re: [GIT PULL 00/27] perf/core improvements and fixes"
Previous message: Chanwoo Choi: "Re: [RFC PATCH 5/9] soc: samsung: pm: Add support for suspend-to-ram of Exynos5433"
In reply to: Krzysztof Kozlowski: "Re: [RFC PATCH 4/9] soc: samsung: Add generic power-management driver for Exynos"
Next in thread: Chanwoo Choi: "[PATCH 6/9] arm64: dts: exynos: Add iRAM device-tree node for Exynos5433"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]