From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-ia64@vger.kernel.org, x86@kernel.org,
Al Stone <al.stone@linaro.org>,
Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
Mahesh Sivasubramanian <msivasub@codeaurora.org>,
Ashwin Chaugule <ashwin.chaugule@linaro.org>,
Prashanth Prakash <pprakash@codeaurora.org>
Subject: Re: [PATCH v3 2/5] ACPI / sleep: move acpi_processor_sleep to sleep.c
Date: Tue, 16 Feb 2016 21:13:09 +0100 [thread overview]
Message-ID: <2288656.HghHDFBFlP@vostro.rjw.lan> (raw)
In-Reply-To: <1449065446-26115-3-git-send-email-sudeep.holla@arm.com>
On Wednesday, December 02, 2015 02:10:43 PM Sudeep Holla wrote:
> acpi_processor_sleep is neither related nor used by CPUIdle framework.
> It's used in system suspend/resume path as a syscore operation. It makes
> more sense to move it to acpi/sleep.c where all the S-state transition
> (a.k.a. Linux system suspend/hiberate) related code are present.
>
> Also make it depend on CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT so that
> it's not compiled on architecture like ARM64 where S-states are not
> yet defined in ACPI.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
To me this goes in the right direction, but I'd take it a bit further.
> ---
> drivers/acpi/processor_driver.c | 2 --
> drivers/acpi/processor_idle.c | 37 -------------------------------------
> drivers/acpi/sleep.c | 35 +++++++++++++++++++++++++++++++++++
> include/acpi/processor.h | 8 --------
> 4 files changed, 35 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index f4e02ae93f58..29f787b2493f 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -313,7 +313,6 @@ static int __init acpi_processor_driver_init(void)
> if (result < 0)
> return result;
>
> - acpi_processor_syscore_init();
> register_hotcpu_notifier(&acpi_cpu_notifier);
> acpi_thermal_cpufreq_init();
> acpi_processor_ppc_init();
> @@ -329,7 +328,6 @@ static void __exit acpi_processor_driver_exit(void)
> acpi_processor_ppc_exit();
> acpi_thermal_cpufreq_exit();
> unregister_hotcpu_notifier(&acpi_cpu_notifier);
> - acpi_processor_syscore_exit();
> driver_unregister(&acpi_processor_driver);
> }
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 175c86bee3a9..a24067b850d2 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -30,7 +30,6 @@
> #include <linux/sched.h> /* need_resched() */
> #include <linux/tick.h>
> #include <linux/cpuidle.h>
> -#include <linux/syscore_ops.h>
> #include <acpi/processor.h>
>
> /*
> @@ -194,42 +193,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
>
> #endif
>
> -#ifdef CONFIG_PM_SLEEP
> -static u32 saved_bm_rld;
> -
> -static int acpi_processor_suspend(void)
> -{
> - acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &saved_bm_rld);
> - return 0;
> -}
> -
> -static void acpi_processor_resume(void)
> -{
> - u32 resumed_bm_rld = 0;
> -
> - acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &resumed_bm_rld);
> - if (resumed_bm_rld == saved_bm_rld)
> - return;
> -
> - acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
> -}
> -
> -static struct syscore_ops acpi_processor_syscore_ops = {
> - .suspend = acpi_processor_suspend,
> - .resume = acpi_processor_resume,
> -};
> -
> -void acpi_processor_syscore_init(void)
> -{
> - register_syscore_ops(&acpi_processor_syscore_ops);
> -}
> -
> -void acpi_processor_syscore_exit(void)
> -{
> - unregister_syscore_ops(&acpi_processor_syscore_ops);
> -}
> -#endif /* CONFIG_PM_SLEEP */
> -
> #if defined(CONFIG_X86)
> static void tsc_check_state(int state)
> {
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 0d94621dc856..73604d8ccfad 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -19,6 +19,7 @@
> #include <linux/reboot.h>
> #include <linux/acpi.h>
> #include <linux/module.h>
> +#include <linux/syscore_ops.h>
> #include <asm/io.h>
> #include <trace/events/power.h>
>
> @@ -677,6 +678,39 @@ static void acpi_sleep_suspend_setup(void)
> static inline void acpi_sleep_suspend_setup(void) {}
> #endif /* !CONFIG_SUSPEND */
>
> +#ifdef CONFIG_PM_SLEEP
> +static u32 saved_bm_rld;
> +
> +static int acpi_processor_suspend(void)
Why do we need mention processor in the function name here (and below)?
I'd call it something like acpi_save/restore_bm_rld() (maybe with a short comment
explaining what the BM RLD is).
> +{
> + acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &saved_bm_rld);
> + return 0;
> +}
> +
> +static void acpi_processor_resume(void)
> +{
> + u32 resumed_bm_rld = 0;
> +
> + acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &resumed_bm_rld);
> + if (resumed_bm_rld == saved_bm_rld)
> + return;
> +
> + acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
> +}
> +
> +static struct syscore_ops acpi_processor_syscore_ops = {
> + .suspend = acpi_processor_suspend,
> + .resume = acpi_processor_resume,
> +};
> +
> +void acpi_processor_syscore_init(void)
> +{
> + register_syscore_ops(&acpi_processor_syscore_ops);
> +}
> +#else
> +static inline void acpi_processor_syscore_init(void) {}
> +#endif /* CONFIG_PM_SLEEP */
> +
> #ifdef CONFIG_HIBERNATION
> static unsigned long s4_hardware_signature;
> static struct acpi_table_facs *facs;
> @@ -839,6 +873,7 @@ int __init acpi_sleep_init(void)
>
> sleep_states[ACPI_STATE_S0] = 1;
>
> + acpi_processor_syscore_init();
> acpi_sleep_suspend_setup();
> acpi_sleep_hibernate_setup();
>
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 54d7860cac11..6f1805dd5d3c 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -395,14 +395,6 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
> }
> #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
>
> -#if defined(CONFIG_PM_SLEEP) & defined(CONFIG_ACPI_PROCESSOR_IDLE)
> -void acpi_processor_syscore_init(void);
> -void acpi_processor_syscore_exit(void);
> -#else
> -static inline void acpi_processor_syscore_init(void) {}
> -static inline void acpi_processor_syscore_exit(void) {}
> -#endif
> -
> /* in processor_thermal.c */
> int acpi_processor_get_limit_info(struct acpi_processor *pr);
> extern const struct thermal_cooling_device_ops processor_cooling_ops;
>
Thanks,
Rafael
next prev parent reply other threads:[~2016-02-16 20:11 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-02 14:10 [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support Sudeep Holla
2015-12-02 14:10 ` [PATCH v3 1/5] ACPI / processor : add support for ACPI0010 processor container Sudeep Holla
2016-02-16 20:10 ` Rafael J. Wysocki
2016-02-17 11:54 ` Sudeep Holla
2016-02-17 11:54 ` [UPDATE] " Sudeep Holla
2016-02-23 23:41 ` Rafael J. Wysocki
2015-12-02 14:10 ` [PATCH v3 2/5] ACPI / sleep: move acpi_processor_sleep to sleep.c Sudeep Holla
2016-02-16 20:13 ` Rafael J. Wysocki [this message]
2016-02-17 12:03 ` Sudeep Holla
2016-02-17 12:03 ` [UPDATE] " Sudeep Holla
2016-02-23 23:42 ` Rafael J. Wysocki
2015-12-02 14:10 ` [PATCH v3 3/5] ACPI / processor_idle: replace PREFIX with pr_fmt Sudeep Holla
2016-02-16 20:15 ` Rafael J. Wysocki
2015-12-02 14:10 ` [PATCH v3 4/5] ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE Sudeep Holla
2016-02-16 20:18 ` Rafael J. Wysocki
2016-02-17 12:21 ` Sudeep Holla
2016-02-22 13:46 ` Sudeep Holla
2016-02-22 23:28 ` Rafael J. Wysocki
2016-02-23 9:32 ` Sudeep Holla
2015-12-02 14:10 ` [PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states Sudeep Holla
2016-02-16 20:46 ` Rafael J. Wysocki
2016-02-17 16:10 ` Sudeep Holla
2016-04-12 4:06 ` Vikas Sajjan
2016-04-12 14:29 ` Sudeep Holla
2015-12-09 22:52 ` [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support Prakash, Prashanth
2015-12-10 8:48 ` Sudeep Holla
2016-01-15 19:13 ` Prakash, Prashanth
2016-01-18 12:15 ` Sudeep Holla
2016-01-18 14:53 ` Rafael J. Wysocki
2016-01-27 18:26 ` Sudeep Holla
2016-02-16 20:08 ` Rafael J. Wysocki
2016-02-17 11:37 ` Sudeep Holla
2016-02-18 2:08 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2288656.HghHDFBFlP@vostro.rjw.lan \
--to=rjw@rjwysocki.net \
--cc=Lorenzo.Pieralisi@arm.com \
--cc=al.stone@linaro.org \
--cc=ashwin.chaugule@linaro.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-ia64@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=msivasub@codeaurora.org \
--cc=pprakash@codeaurora.org \
--cc=sudeep.holla@arm.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).