| Message ID | 1403875377-940-10-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Gregory, On Fri, 27 Jun 2014 15:22:50 +0200, Gregory CLEMENT wrote: > /* No locking is needed because we only access per-CPU registers */ > -static void mvebu_v7_pmsu_idle_prepare(bool deepidle) > +static void mvebu_v7_pmsu_idle_prepare(bool deepidle, bool snoopdis) I continue to find it odd to have more and more boolean arguments to a function to indicate what the function should do. It often indicates that the function should rather be split in smaller, fine-grained functions. Another sad consequence of using booleans is that the call sites of the functions can no longer be understood properly: mvebu_v7_pmsu_idle_prepare(false, true); mvebu_v7_pmsu_idle_prepare(true, true); mvebu_v7_pmsu_idle_prepare(false, false); But oh well, it looks like it's the easiest solution here, and I admit it's convenient to have one function to the entire business of preparing for idle. However, this change also badly conflicts with the CPU hotplug support that I have sent, and which is already part of linux-next, and for which Jason has sent a pull request to the arm-soc folks. See http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-mvebu/platsmp.c for the usage of the armada_370_xp_pmsu_idle_enter() function. Maybe your patch series should be based on mvebu/soc instead? Thomas
Hi Thomas, >> /* No locking is needed because we only access per-CPU registers */ >> -static void mvebu_v7_pmsu_idle_prepare(bool deepidle) >> +static void mvebu_v7_pmsu_idle_prepare(bool deepidle, bool snoopdis) > > I continue to find it odd to have more and more boolean arguments to a > function to indicate what the function should do. It often indicates > that the function should rather be split in smaller, fine-grained > functions. Another sad consequence of using booleans is that the call > sites of the functions can no longer be understood properly: > > mvebu_v7_pmsu_idle_prepare(false, true); > mvebu_v7_pmsu_idle_prepare(true, true); > mvebu_v7_pmsu_idle_prepare(false, false); > > But oh well, it looks like it's the easiest solution here, and I admit > it's convenient to have one function to the entire business of > preparing for idle. Maybe we can use flags, hence we use only one arguments, it became extensible, and more readable too. Something like #define PMSU_IDLE_NO_FLAG 0 #define PMSU_IDLE_DEEPIDLE BIT(0) #define PMSU_IDLE_SNOOPDIS BIT(1) then we can have: mvebu_v7_pmsu_idle_prepare(PMSU_IDLE_SNOOPDIS); mvebu_v7_pmsu_idle_prepare(PMSU_IDLE_DEEPIDLE | PMSU_IDLE_SNOOPDIS); mvebu_v7_pmsu_idle_prepare(PMSU_IDLE_NO_FLAG); static void mvebu_v7_pmsu_idle_prepare(u32 flags) Thanks, Gregory
Dear Gregory CLEMENT, On Thu, 03 Jul 2014 14:50:22 +0200, Gregory CLEMENT wrote: > Maybe we can use flags, hence we use only one arguments, it became extensible, > and more readable too. Something like > > #define PMSU_IDLE_NO_FLAG 0 > #define PMSU_IDLE_DEEPIDLE BIT(0) > #define PMSU_IDLE_SNOOPDIS BIT(1) > > then we can have: > mvebu_v7_pmsu_idle_prepare(PMSU_IDLE_SNOOPDIS); > mvebu_v7_pmsu_idle_prepare(PMSU_IDLE_DEEPIDLE | PMSU_IDLE_SNOOPDIS); > mvebu_v7_pmsu_idle_prepare(PMSU_IDLE_NO_FLAG); > > static void mvebu_v7_pmsu_idle_prepare(u32 flags) Yes, seems a good idea to me. Used "unsigned int" as the type for the flags argument, and maybe use PMSU_IDLE_NORMAL for 0 instead of NO_FLAG. Thomas
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index 8bf737fb3aac..a2869c5daeb0 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -180,7 +180,7 @@ static void mvebu_v7_pmsu_enable_l2_powerdown_onidle(void) } /* No locking is needed because we only access per-CPU registers */ -static void mvebu_v7_pmsu_idle_prepare(bool deepidle) +static void mvebu_v7_pmsu_idle_prepare(bool deepidle, bool snoopdis) { unsigned int hw_cpu = cpu_logical_map(smp_processor_id()); u32 reg; @@ -211,15 +211,17 @@ static void mvebu_v7_pmsu_idle_prepare(bool deepidle) reg |= PMSU_CONTROL_AND_CONFIG_PWDDN_REQ; writel(reg, pmsu_mp_base + PMSU_CONTROL_AND_CONFIG(hw_cpu)); - /* Disable snoop disable by HW - SW is taking care of it */ - reg = readl(pmsu_mp_base + PMSU_CPU_POWER_DOWN_CONTROL(hw_cpu)); - reg |= PMSU_CPU_POWER_DOWN_DIS_SNP_Q_SKIP; - writel(reg, pmsu_mp_base + PMSU_CPU_POWER_DOWN_CONTROL(hw_cpu)); + if (snoopdis) { + /* Disable snoop disable by HW - SW is taking care of it */ + reg = readl(pmsu_mp_base + PMSU_CPU_POWER_DOWN_CONTROL(hw_cpu)); + reg |= PMSU_CPU_POWER_DOWN_DIS_SNP_Q_SKIP; + writel(reg, pmsu_mp_base + PMSU_CPU_POWER_DOWN_CONTROL(hw_cpu)); + } } static noinline int do_armada_xp_370_cpu_suspend(unsigned long deepidle) { - mvebu_v7_pmsu_idle_prepare(deepidle); + mvebu_v7_pmsu_idle_prepare(deepidle, true); v7_exit_coherency_flush(all);
On some mvebu v7 SoC (the ones which don't use PJ4B core) there is no such feature. So add a parameter in mvebu_v7_pmsu_idle_prepare function to make its use optional. Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- arch/arm/mach-mvebu/pmsu.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)