| 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: <5A56F9A9.10207@samsung.com>
Date: Thu, 11 Jan 2018 14:44:09 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
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
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@...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
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
Powered by blists - more mailing lists