From mboxrd@z Thu Jan 1 00:00:00 1970 From: b20788@freescale.com (Anson Huang) Date: Wed, 20 Mar 2013 18:06:53 -0400 Subject: [PATCH 3/3] ARM: imx: enable RBC to support anatop LPM mode In-Reply-To: <20130320090117.GA27071@S2101-09.ap.freescale.net> References: <1363801180-8284-1-git-send-email-b20788@freescale.com> <1363801180-8284-3-git-send-email-b20788@freescale.com> <20130320090117.GA27071@S2101-09.ap.freescale.net> Message-ID: <20130320220653.GC13929@ubuntu> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Mar 20, 2013 at 05:01:19PM +0800, Shawn Guo wrote: > On Wed, Mar 20, 2013 at 01:39:40PM -0400, Anson Huang wrote: > > RBC is to control whether some ANATOP sub modules > > can enter lpm mode when SOC is into STOP mode, if > > RBC is enabled and PMIC_VSTBY_REQ is set, ANATOP > > will have below behaviors: > > > > 1. Digital LDOs(CORE, SOC and PU) are bypassed; > > 2. Analog LDOs(1P1, 2P5, 3P0) are disabled; > > > > As the 2P5 is necessary for DRAM IO pre-drive in > > STOP mode, so we need to enable weak 2P5 in STOP > > mode when 2P5 LDO is disabled. > > > > For RBC settings, there are some rules as below > > due to hardware designe: > > s/designe/design Accepted. > > > > > 1. All interrupts must be masked during operating > > RBC registers; > > 2. At least 2 CKIL(32K) cycles is needed after the > > RBC setting is changed. > > > > Signed-off-by: Anson Huang > > --- > > arch/arm/mach-imx/anatop.c | 19 +++++++++++++++++++ > > arch/arm/mach-imx/clk-imx6q.c | 35 +++++++++++++++++++++++++++++++++++ > > arch/arm/mach-imx/common.h | 3 +++ > > arch/arm/mach-imx/gpc.c | 21 ++++++++++++++++++++- > > arch/arm/mach-imx/pm-imx6q.c | 2 ++ > > 5 files changed, 79 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mach-imx/anatop.c b/arch/arm/mach-imx/anatop.c > > index 8f6ab27..38b4f44 100644 > > --- a/arch/arm/mach-imx/anatop.c > > +++ b/arch/arm/mach-imx/anatop.c > > @@ -18,12 +18,29 @@ > > > > #define REG_SET 0x4 > > #define REG_CLR 0x8 > > +#define ANA_MISC0 0x150 > > #define ANA_REG_CORE 0x140 > > +#define ANA_REG_2P5 0x130 > > Please put these registers in acceding offset. Accepted. > > > > > +#define BM_ANADIG_REG_2P5_ENABLE_WEAK_LINREG 0x40000 > > +#define BM_ANADIG_ANA_MISC0_STOP_MODE_CONFIG 0x1000 > > #define BM_ANADIG_REG_CORE_FET_ODRIVE 0x20000000 > > > > static struct regmap *anatop; > > > > +void imx_anatop_enable_weak2p5(bool enable) > > static Accepted. > > > +{ > > + u32 val; > > + > > + regmap_read(anatop, ANA_MISC0, &val); > > + > > + /* can only be enabled when stop_mode_config is clear. */ > > + regmap_write(anatop, ANA_REG_2P5 + ((enable && > > + ((val & BM_ANADIG_ANA_MISC0_STOP_MODE_CONFIG) > > + == 0)) ? REG_SET : REG_CLR), > > + BM_ANADIG_REG_2P5_ENABLE_WEAK_LINREG); > > It's a little difficult to parse the expression even it's on the same > line, not mentioning with indentation. There are so many levels of > parentheses. > > ((enable && ((val & BM_ANADIG_ANA_MISC0_STOP_MODE_CONFIG) == 0)) ? REG_SET : REG_CLR) > > What about rewriting it as below to make it easy for people to read? > > reg = ANA_REG_2P5; > reg += (enable && (val & BM_ANADIG_ANA_MISC0_STOP_MODE_CONFIG) == 0) ? > REG_SET : REG_CLR; > regmap_write(anatop, reg, BM_ANADIG_REG_2P5_ENABLE_WEAK_LINREG); Accepted. > > > +} > > + > > void imx_anatop_enable_fet_odrive(bool enable) > > { > > regmap_write(anatop, ANA_REG_CORE + (enable ? > > @@ -32,12 +49,14 @@ void imx_anatop_enable_fet_odrive(bool enable) > > > > void imx_anatop_pre_suspend(void) > > { > > + imx_anatop_enable_weak2p5(true); > > imx_anatop_enable_fet_odrive(true); > > } > > > > void imx_anatop_post_resume(void) > > { > > imx_anatop_enable_fet_odrive(false); > > + imx_anatop_enable_weak2p5(false); > > } > > > > void __init imx_anatop_init(void) > > diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c > > index b365efc..646ce12 100644 > > --- a/arch/arm/mach-imx/clk-imx6q.c > > +++ b/arch/arm/mach-imx/clk-imx6q.c > > @@ -14,6 +14,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -25,6 +26,8 @@ > > > > #define CCR 0x0 > > #define BM_CCR_WB_COUNT (0x7 << 16) > > +#define BM_CCR_RBC_BYPASS_COUNT (0x3f << 21) > > +#define BM_CCR_RBC_EN (0x1 << 27) > > > > #define CCGR0 0x68 > > #define CCGR1 0x6c > > @@ -70,6 +73,38 @@ void imx6q_set_chicken_bit(void) > > writel_relaxed(val, ccm_base + CGPR); > > } > > > > +void imx6q_set_rbc(bool enable) > > Same question as imx6q_set_wb, can we manage to call it in > imx6q_set_lpm()? The intension behind this is we should try to > a centralized place to configure CCM registers/bits for suspend > instead of exporting so many functions to suspend routine to call > individually. Will do it in the way as well bias did. > > > +{ > > + u32 val; > > + > > + /* > > + * need to mask all interrupts in GPC before > > + * operating RBC configurations > > + */ > > + imx_gpc_mask_all(); > > + > > + /* configurate RBC enable bit */ > > s/configurate/configure Accepted. > > > + val = readl_relaxed(ccm_base + CCR); > > + val &= ~BM_CCR_RBC_EN; > > + val |= enable ? BM_CCR_RBC_EN : 0; > > + writel_relaxed(val, ccm_base + CCR); > > + > > + /* configurate RBC count */ > > Ditto Accepted. > > > + val = readl_relaxed(ccm_base + CCR); > > + val &= ~BM_CCR_RBC_BYPASS_COUNT; > > + val |= enable ? BM_CCR_RBC_BYPASS_COUNT : 0; > > + writel(val, ccm_base + CCR); > > + > > + /* > > + * need to delay at least 2 cycles of CKIL(32K) > > + * due to hardware design requirement, which is > > + * ~61us, here we use 65us for safe > > + */ > > + udelay(65); > > Nit, have a new line here. Accepted. > > > + /* restore GPC interrupt mask settings */ > > + imx_gpc_restore_all(); > > +} > > + > > void imx6q_set_wb(bool enable) > > { > > u32 val; > > diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h > > index b9125cf..66fe41c 100644 > > --- a/arch/arm/mach-imx/common.h > > +++ b/arch/arm/mach-imx/common.h > > @@ -129,11 +129,14 @@ extern void imx_src_prepare_restart(void); > > extern void imx_gpc_init(void); > > extern void imx_gpc_pre_suspend(void); > > extern void imx_gpc_post_resume(void); > > +extern void imx_gpc_mask_all(void); > > +extern void imx_gpc_restore_all(void); > > extern void imx_anatop_init(void); > > extern void imx_anatop_pre_suspend(void); > > extern void imx_anatop_post_resume(void); > > extern int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode); > > extern void imx6q_set_chicken_bit(void); > > +extern void imx6q_set_rbc(bool enable); > > extern void imx6q_set_wb(bool enable); > > Since they are taking "enable" parameter, can we rename them as below? > > void imx6q_enable_rbc(bool enable); > void imx6q_enable_wb(bool enable); OK. > > > extern void imx_cpu_die(unsigned int cpu); > > extern int imx_cpu_kill(unsigned int cpu); > > diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c > > index a96ccc7..504049e 100644 > > --- a/arch/arm/mach-imx/gpc.c > > +++ b/arch/arm/mach-imx/gpc.c > > @@ -1,5 +1,5 @@ > > /* > > - * Copyright 2011 Freescale Semiconductor, Inc. > > + * Copyright 2011-2013 Freescale Semiconductor, Inc. > > * Copyright 2011 Linaro Ltd. > > * > > * The code contained herein is licensed under the GNU General Public > > @@ -68,6 +68,25 @@ static int imx_gpc_irq_set_wake(struct irq_data *d, unsigned int on) > > return 0; > > } > > > > +void imx_gpc_mask_all(void) > > +{ > > + void __iomem *reg_imr1 = gpc_base + GPC_IMR1; > > + int i; > > + > > + for (i = 0; i < IMR_NUM; i++) > > + writel_relaxed(0xffffffff, reg_imr1 + i * 4); > > + > > +} > > + > > +void imx_gpc_restore_all(void) > > +{ > > + void __iomem *reg_imr1 = gpc_base + GPC_IMR1; > > + int i; > > + > > + for (i = 0; i < IMR_NUM; i++) > > + writel_relaxed(gpc_saved_imrs[i], reg_imr1 + i * 4); > > +} > > + > > static void imx_gpc_irq_unmask(struct irq_data *d) > > { > > void __iomem *reg; > > diff --git a/arch/arm/mach-imx/pm-imx6q.c b/arch/arm/mach-imx/pm-imx6q.c > > index 57ca274..24ac7bc 100644 > > --- a/arch/arm/mach-imx/pm-imx6q.c > > +++ b/arch/arm/mach-imx/pm-imx6q.c > > @@ -36,10 +36,12 @@ static int imx6q_pm_enter(suspend_state_t state) > > imx_gpc_pre_suspend(); > > This call saves mask in gpc_saved_imrs and mask all interrupts except > those wakeup sources ... > > > imx_anatop_pre_suspend(); > > imx_set_cpu_jump(0, v7_cpu_resume); > > + imx6q_set_rbc(true); > > At the end of imx6q_set_rbc(), imx_gpc_restore_all() will be called to > restore mask bits with gpc_saved_imrs, so wakeup source handling gets > lost, and any in-use interrupt will wake up system, which is > undesirable. Oh, my mistake, will fix it in V2. > > Shawn > > > imx6q_set_wb(true); > > /* Zzz ... */ > > cpu_suspend(0, imx6q_suspend_finish); > > imx6q_set_wb(false); > > + imx6q_set_rbc(false); > > imx_smp_prepare(); > > imx_anatop_post_resume(); > > imx_gpc_post_resume(); > > -- > > 1.7.9.5 > > > >