| Message ID | 1432154048-19126-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Doug Thank you for pointing out. Reviewed-by: Chris Zhong <zyw@rock-chips.com> On 05/21/2015 04:34 AM, Doug Anderson wrote: > In the commit (0ea001d ARM: rockchip: disable dapswjdp during suspend) > we made the assumption that we didn't need to restore dapswjdp after > suspend because "the MASKROM will enable it back". > > It turns out that's not a safe assumption. In some cases (pending > interrupts) it's possible that the WFI might act as a no-op and the > MaskROM will never run. Since we're changing the bit, we should > restore it ourselves. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > arch/arm/mach-rockchip/pm.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c > index b0dcbe2..a7be465 100644 > --- a/arch/arm/mach-rockchip/pm.c > +++ b/arch/arm/mach-rockchip/pm.c > @@ -48,6 +48,7 @@ static struct regmap *sgrf_regmap; > > static u32 rk3288_pmu_pwr_mode_con; > static u32 rk3288_sgrf_soc_con0; > +static u32 rk3288_sgrf_cpu_con0; > > static inline u32 rk3288_l2_config(void) > { > @@ -70,6 +71,7 @@ static void rk3288_slp_mode_set(int level) > { > u32 mode_set, mode_set1; > > + regmap_read(sgrf_regmap, RK3288_SGRF_CPU_CON0, &rk3288_sgrf_cpu_con0); > regmap_read(sgrf_regmap, RK3288_SGRF_SOC_CON0, &rk3288_sgrf_soc_con0); > > regmap_read(pmu_regmap, RK3288_PMU_PWRMODE_CON, > @@ -129,6 +131,9 @@ static void rk3288_slp_mode_set(int level) > > static void rk3288_slp_mode_set_resume(void) > { > + regmap_write(sgrf_regmap, RK3288_SGRF_CPU_CON0, > + rk3288_sgrf_cpu_con0 | SGRF_DAPDEVICEEN_WRITE); > + > regmap_write(pmu_regmap, RK3288_PMU_PWRMODE_CON, > rk3288_pmu_pwr_mode_con); >
Hi, Doug: ? 2015?05?21? 04:34, Doug Anderson ??: > In the commit (0ea001d ARM: rockchip: disable dapswjdp during suspend) > we made the assumption that we didn't need to restore dapswjdp after > suspend because "the MASKROM will enable it back". > > It turns out that's not a safe assumption. In some cases (pending > interrupts) it's possible that the WFI might act as a no-op and the > MaskROM will never run. I don't think this can happen. It seems we set PMU_GLOBAL_INT_DISABLE bit, which means in power off flow, the IRQ will not accepted until the ARM is power off. Do you see the SGRF_CPU_CON0[0] is 0 after resume? Anyway, restore the value is okay, which make the code more symmetrically Since we're changing the bit, we should > restore it ourselves. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > arch/arm/mach-rockchip/pm.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c > index b0dcbe2..a7be465 100644 > --- a/arch/arm/mach-rockchip/pm.c > +++ b/arch/arm/mach-rockchip/pm.c > @@ -48,6 +48,7 @@ static struct regmap *sgrf_regmap; > > static u32 rk3288_pmu_pwr_mode_con; > static u32 rk3288_sgrf_soc_con0; > +static u32 rk3288_sgrf_cpu_con0; > > static inline u32 rk3288_l2_config(void) > { > @@ -70,6 +71,7 @@ static void rk3288_slp_mode_set(int level) > { > u32 mode_set, mode_set1; > > + regmap_read(sgrf_regmap, RK3288_SGRF_CPU_CON0, &rk3288_sgrf_cpu_con0); > regmap_read(sgrf_regmap, RK3288_SGRF_SOC_CON0, &rk3288_sgrf_soc_con0); > > regmap_read(pmu_regmap, RK3288_PMU_PWRMODE_CON, > @@ -129,6 +131,9 @@ static void rk3288_slp_mode_set(int level) > > static void rk3288_slp_mode_set_resume(void) > { > + regmap_write(sgrf_regmap, RK3288_SGRF_CPU_CON0, > + rk3288_sgrf_cpu_con0 | SGRF_DAPDEVICEEN_WRITE); > + > regmap_write(pmu_regmap, RK3288_PMU_PWRMODE_CON, > rk3288_pmu_pwr_mode_con); > >
Huang Tao, On Wed, May 20, 2015 at 7:02 PM, Huang, Tao <huangtao@rock-chips.com> wrote: >> It turns out that's not a safe assumption. In some cases (pending >> interrupts) it's possible that the WFI might act as a no-op and the >> MaskROM will never run. > > > I don't think this can happen. It seems we set PMU_GLOBAL_INT_DISABLE > bit, which means in power off flow, the IRQ will not accepted until the > ARM is power off. Do you see the SGRF_CPU_CON0[0] is 0 after resume? From testing, it appears that it can happen that the WFI might act as a no-op. ...but you're right in pointing out that unless we can solve the PMU_GLOBAL_INT_DISABLE() problem then this patch here to fix DAPDEVICEEN isn't too useful. I was hoping to fix the PMU_GLOBAL_INT_DISABLE() in <https://chromium-review.googlesource.com/#/c/272442/> or find a similar solution. ...if we can's solve that though, then I'm not sure what to suggest. > Anyway, restore the value is okay, which make the code more symmetrically OK. Heiko: applying this won't hurt, but it will only be useful if we find a way to return from the WFI successfully... -Doug
Hi Doug, Am Donnerstag, 21. Mai 2015, 08:40:55 schrieb Doug Anderson: > On Wed, May 20, 2015 at 7:02 PM, Huang, Tao <huangtao@rock-chips.com> wrote: > >> It turns out that's not a safe assumption. In some cases (pending > >> interrupts) it's possible that the WFI might act as a no-op and the > >> MaskROM will never run. > > Anyway, restore the value is okay, which make the code more symmetrically > > OK. Heiko: applying this won't hurt, but it will only be useful if we > find a way to return from the WFI successfully... As it looks sane but doesn't fix an immediate problem I have applied this to my soc-branch for 4.2 . Thanks Heiko
diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c index b0dcbe2..a7be465 100644 --- a/arch/arm/mach-rockchip/pm.c +++ b/arch/arm/mach-rockchip/pm.c @@ -48,6 +48,7 @@ static struct regmap *sgrf_regmap; static u32 rk3288_pmu_pwr_mode_con; static u32 rk3288_sgrf_soc_con0; +static u32 rk3288_sgrf_cpu_con0; static inline u32 rk3288_l2_config(void) { @@ -70,6 +71,7 @@ static void rk3288_slp_mode_set(int level) { u32 mode_set, mode_set1; + regmap_read(sgrf_regmap, RK3288_SGRF_CPU_CON0, &rk3288_sgrf_cpu_con0); regmap_read(sgrf_regmap, RK3288_SGRF_SOC_CON0, &rk3288_sgrf_soc_con0); regmap_read(pmu_regmap, RK3288_PMU_PWRMODE_CON, @@ -129,6 +131,9 @@ static void rk3288_slp_mode_set(int level) static void rk3288_slp_mode_set_resume(void) { + regmap_write(sgrf_regmap, RK3288_SGRF_CPU_CON0, + rk3288_sgrf_cpu_con0 | SGRF_DAPDEVICEEN_WRITE); + regmap_write(pmu_regmap, RK3288_PMU_PWRMODE_CON, rk3288_pmu_pwr_mode_con);
In the commit (0ea001d ARM: rockchip: disable dapswjdp during suspend) we made the assumption that we didn't need to restore dapswjdp after suspend because "the MASKROM will enable it back". It turns out that's not a safe assumption. In some cases (pending interrupts) it's possible that the WFI might act as a no-op and the MaskROM will never run. Since we're changing the bit, we should restore it ourselves. Signed-off-by: Doug Anderson <dianders@chromium.org> --- arch/arm/mach-rockchip/pm.c | 5 +++++ 1 file changed, 5 insertions(+)