From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lorenzo Pieralisi Subject: Re: [PATCH v4 12/13] cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC Date: Fri, 14 Feb 2014 17:00:46 +0000 Message-ID: <20140214170046.GA32564@e102568-lin.cambridge.arm.com> References: <1392312816-17657-1-git-send-email-gregory.clement@free-electrons.com> <1392312816-17657-13-git-send-email-gregory.clement@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Return-path: Received: from service87.mimecast.com ([91.220.42.44]:39525 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752524AbaBNRAz convert rfc822-to-8bit (ORCPT ); Fri, 14 Feb 2014 12:00:55 -0500 In-Reply-To: <1392312816-17657-13-git-send-email-gregory.clement@free-electrons.com> Content-Disposition: inline Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Gregory CLEMENT Cc: Daniel Lezcano , "Rafael J. Wysocki" , "linux-pm@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , Ezequiel Garcia , "linux-arm-kernel@lists.infradead.org" , Lior Amsalem , Tawfik Bayouk , Nadav Haklai On Thu, Feb 13, 2014 at 05:33:35PM +0000, Gregory CLEMENT wrote: [...] > diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c > new file mode 100644 > index 000000000000..57c69812e79d > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c > @@ -0,0 +1,120 @@ > +/* > + * Marvell Armada 370 and Armada XP SoC cpuidle driver > + * > + * Copyright (C) 2013 Marvell > + * > + * Nadav Haklai > + * Gregory CLEMENT > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + * > + * Maintainer: Gregory CLEMENT > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include You should order them , then > + > +#define ARMADA_370_XP_MAX_STATES 3 Is this macro really needed ? > +#define ARMADA_370_XP_FLAG_DEEP_IDLE 0x10000 > +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle); > +extern void ll_clear_cpu_coherent(void); > +extern void ll_set_cpu_coherent(void); > + > +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle) > +{ > + armada_370_xp_pmsu_idle_prepare(deepidle); > + > + v7_exit_coherency_flush(all); The macro above clears the C bit in SCTLR and exit coherency (clears SMP bit in SCTLR), let's keep this in mind, see below. > + ll_clear_cpu_coherent(); And the macro above uses ldr/str exclusives, and this is done with MMU on and off (on cold-boot before jumping to secondary_startup and also before jumping to cpu_resume in armada_370_xp_cpu_resume). Can you explain to me how load/store exclusives work on this platform ? ARM ARM A3.4.5 "It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be performed to a memory region with the Device or Strongly-ordered memory attribute. Unless the implementation documentation explicitly states that LDREX and STREX operations to a memory region with the Device or Strongly-ordered attribute are permitted, the effect of such operations is UNPREDICTABLE." At least code must be commented and an explanation on why this works has to be given. > + > + dsb(); > + > + wfi(); > + > + ll_set_cpu_coherent(); > + > + asm volatile( > + "mrc p15, 0, %0, c1, c0, 0 \n\t" > + "tst %0, #(1 << 2) \n\t" > + "orreq r0, %0, #(1 << 2) \n\t" > + "mcreq p15, 0, %0, c1, c0, 0 \n\t" > + "isb " > + : : "r" (0)); First of all, complex code like this must be commented. Moreover, this sequence is wrong. If wfi completes the kernel would explode. 1) where is the SMP bit in SCTLR restored ? 2) where are tlbs flushed (ie processors run out of coherency for _some_ time, so tlbs might be stale) ? > + > + return 0; > +} > + > +static int armada_370_xp_enter_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, > + int index) > +{ > + bool deepidle = false; > + cpu_pm_enter(); > + > + if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE) > + deepidle = true; > + > + cpu_suspend(deepidle, armada_370_xp_cpu_suspend); > + > + cpu_pm_exit(); > + > + return index; You should check the cpu_suspend return value and demote the idle state accordingly, if it failed. Thanks, Lorenzo