| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJKOXPd5nZApbx-v2dZhuUy5ociUwNGHa_wAoz1b-XKbJy1nmg@mail.gmail.com>
Date: Tue, 9 Jan 2018 13:37:40 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Chanwoo Choi <cw00.choi@...sung.com>
Cc: Marek Szyprowski <m.szyprowski@...sung.com>,
Sylwester Nawrocki <s.nawrocki@...sung.com>, kgene@...nel.org,
Tomasz Figa <tomasz.figa@...il.com>, chanwoo@...nel.org,
Jaehoon Chung <jh80.chung@...sung.com>,
Inki Dae <inki.dae@...sung.com>,
linux-arm-kernel@...ts.infradead.org,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Jonghwa Lee <jonghwa3.lee@...sung.com>
Subject: Re: [RFC PATCH 4/9] soc: samsung: Add generic power-management driver
for Exynos
On Tue, Jan 9, 2018 at 8:59 AM, Chanwoo Choi <cw00.choi@...sung.com> 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@...sung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
> ---
> 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
>
Powered by blists - more mailing lists