| Message ID | 1423487543-10593-2-git-send-email-zyw@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Chris, Am Montag, 9. Februar 2015, 21:12:23 schrieb Chris Zhong: > The watchdog clock should be disable in dw_wdt_suspend, but we set a > dummy clock to watchdog for rk3288. So the watchdog will continue to > work during suspend. And we switch the system clock to 32khz from 24Mhz, > during suspend, so the watchdog timer over count will increase to > 755 times, about 12.5 hours, the original value is 60 seconds. So > watchdog will reset the system over a night, but voltage are all > incorrect, then it hang on reset. > > Signed-off-by: Chris Zhong <zyw@rock-chips.com> > Signed-off-by: Daniel Kurtz <djkurtz@google.com> The SGRF is not writeable in all bootmodes (I've talked with Doug about this to verify I remembered this correctly), so handling the sgrf gate for the watchdog is not safe for all possible boards. Why not simply turn off the watchdog in the driver during suspend? Heiko > > --- > > arch/arm/mach-rockchip/pm.c | 11 ++++++++--- > arch/arm/mach-rockchip/pm.h | 2 ++ > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c > index a3ab397..b07d886 100644 > --- a/arch/arm/mach-rockchip/pm.c > +++ b/arch/arm/mach-rockchip/pm.c > @@ -75,9 +75,13 @@ static void rk3288_slp_mode_set(int level) > regmap_read(pmu_regmap, RK3288_PMU_PWRMODE_CON, > &rk3288_pmu_pwr_mode_con); > > - /* set bit 8 so that system will resume to FAST_BOOT_ADDR */ > + /* > + * SGRF_FAST_BOOT_EN - system to boot from FAST_BOOT_ADDR > + * PCLK_WDT_GATE - disable WDT during suspend. > + */ > regmap_write(sgrf_regmap, RK3288_SGRF_SOC_CON0, > - SGRF_FAST_BOOT_EN | SGRF_FAST_BOOT_EN_WRITE); > + SGRF_PCLK_WDT_GATE | SGRF_FAST_BOOT_EN > + | SGRF_PCLK_WDT_GATE_WRITE | SGRF_FAST_BOOT_EN_WRITE); > > /* booting address of resuming system is from this register value */ > regmap_write(sgrf_regmap, RK3288_SGRF_FAST_BOOT_ADDR, > @@ -122,7 +126,8 @@ static void rk3288_slp_mode_set_resume(void) > rk3288_pmu_pwr_mode_con); > > regmap_write(sgrf_regmap, RK3288_SGRF_SOC_CON0, > - rk3288_sgrf_soc_con0 | SGRF_FAST_BOOT_EN_WRITE); > + rk3288_sgrf_soc_con0 | SGRF_PCLK_WDT_GATE_WRITE > + | SGRF_FAST_BOOT_EN_WRITE); > } > > static int rockchip_lpmode_enter(unsigned long arg) > diff --git a/arch/arm/mach-rockchip/pm.h b/arch/arm/mach-rockchip/pm.h > index 96beaa0..d463978 100644 > --- a/arch/arm/mach-rockchip/pm.h > +++ b/arch/arm/mach-rockchip/pm.h > @@ -44,6 +44,8 @@ void __init rockchip_suspend_init(void); > > #define RK3288_SGRF_SOC_CON0 (0x0000) > #define RK3288_SGRF_FAST_BOOT_ADDR (0x0120) > +#define SGRF_PCLK_WDT_GATE BIT(6) > +#define SGRF_PCLK_WDT_GATE_WRITE BIT(22) > #define SGRF_FAST_BOOT_EN BIT(8) > #define SGRF_FAST_BOOT_EN_WRITE BIT(24)
On 03/03/2015 04:50 AM, Heiko Stuebner wrote: > Hi Chris, > > Am Montag, 9. Februar 2015, 21:12:23 schrieb Chris Zhong: >> The watchdog clock should be disable in dw_wdt_suspend, but we set a >> dummy clock to watchdog for rk3288. So the watchdog will continue to >> work during suspend. And we switch the system clock to 32khz from 24Mhz, >> during suspend, so the watchdog timer over count will increase to >> 755 times, about 12.5 hours, the original value is 60 seconds. So >> watchdog will reset the system over a night, but voltage are all >> incorrect, then it hang on reset. >> >> Signed-off-by: Chris Zhong <zyw@rock-chips.com> >> Signed-off-by: Daniel Kurtz <djkurtz@google.com> > The SGRF is not writeable in all bootmodes (I've talked with Doug about this > to verify I remembered this correctly), so handling the sgrf gate for the > watchdog is not safe for all possible boards. > > Why not simply turn off the watchdog in the driver during suspend? I think SGRF is writeable, since we would set this RK3288_SGRF_SOC_CON0 register when suspend. and this SGRF_PCLK_WDT_GATE is one bit of RK3288_SGRF_SOC_CON0. I tried to set wdt_en(WDT_CR[bit 0]) to 0 in watchdog driver, but that would cause system reboot. > > Heiko > >> --- >> >> arch/arm/mach-rockchip/pm.c | 11 ++++++++--- >> arch/arm/mach-rockchip/pm.h | 2 ++ >> 2 files changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c >> index a3ab397..b07d886 100644 >> --- a/arch/arm/mach-rockchip/pm.c >> +++ b/arch/arm/mach-rockchip/pm.c >> @@ -75,9 +75,13 @@ static void rk3288_slp_mode_set(int level) >> regmap_read(pmu_regmap, RK3288_PMU_PWRMODE_CON, >> &rk3288_pmu_pwr_mode_con); >> >> - /* set bit 8 so that system will resume to FAST_BOOT_ADDR */ >> + /* >> + * SGRF_FAST_BOOT_EN - system to boot from FAST_BOOT_ADDR >> + * PCLK_WDT_GATE - disable WDT during suspend. >> + */ >> regmap_write(sgrf_regmap, RK3288_SGRF_SOC_CON0, >> - SGRF_FAST_BOOT_EN | SGRF_FAST_BOOT_EN_WRITE); >> + SGRF_PCLK_WDT_GATE | SGRF_FAST_BOOT_EN >> + | SGRF_PCLK_WDT_GATE_WRITE | SGRF_FAST_BOOT_EN_WRITE); >> >> /* booting address of resuming system is from this register value */ >> regmap_write(sgrf_regmap, RK3288_SGRF_FAST_BOOT_ADDR, >> @@ -122,7 +126,8 @@ static void rk3288_slp_mode_set_resume(void) >> rk3288_pmu_pwr_mode_con); >> >> regmap_write(sgrf_regmap, RK3288_SGRF_SOC_CON0, >> - rk3288_sgrf_soc_con0 | SGRF_FAST_BOOT_EN_WRITE); >> + rk3288_sgrf_soc_con0 | SGRF_PCLK_WDT_GATE_WRITE >> + | SGRF_FAST_BOOT_EN_WRITE); >> } >> >> static int rockchip_lpmode_enter(unsigned long arg) >> diff --git a/arch/arm/mach-rockchip/pm.h b/arch/arm/mach-rockchip/pm.h >> index 96beaa0..d463978 100644 >> --- a/arch/arm/mach-rockchip/pm.h >> +++ b/arch/arm/mach-rockchip/pm.h >> @@ -44,6 +44,8 @@ void __init rockchip_suspend_init(void); >> >> #define RK3288_SGRF_SOC_CON0 (0x0000) >> #define RK3288_SGRF_FAST_BOOT_ADDR (0x0120) >> +#define SGRF_PCLK_WDT_GATE BIT(6) >> +#define SGRF_PCLK_WDT_GATE_WRITE BIT(22) >> #define SGRF_FAST_BOOT_EN BIT(8) >> #define SGRF_FAST_BOOT_EN_WRITE BIT(24) > > >
Hi, On Mon, Mar 2, 2015 at 9:57 PM, Chris Zhong <zyw@rock-chips.com> wrote: > > On 03/03/2015 04:50 AM, Heiko Stuebner wrote: >> >> Hi Chris, >> >> Am Montag, 9. Februar 2015, 21:12:23 schrieb Chris Zhong: >>> >>> The watchdog clock should be disable in dw_wdt_suspend, but we set a >>> dummy clock to watchdog for rk3288. So the watchdog will continue to >>> work during suspend. And we switch the system clock to 32khz from 24Mhz, >>> during suspend, so the watchdog timer over count will increase to >>> 755 times, about 12.5 hours, the original value is 60 seconds. So >>> watchdog will reset the system over a night, but voltage are all >>> incorrect, then it hang on reset. >>> >>> Signed-off-by: Chris Zhong <zyw@rock-chips.com> >>> Signed-off-by: Daniel Kurtz <djkurtz@google.com> >> >> The SGRF is not writeable in all bootmodes (I've talked with Doug about >> this >> to verify I remembered this correctly), so handling the sgrf gate for the >> watchdog is not safe for all possible boards. >> >> Why not simply turn off the watchdog in the driver during suspend? > > I think SGRF is writeable, since we would set this RK3288_SGRF_SOC_CON0 > register when suspend. > and this SGRF_PCLK_WDT_GATE is one bit of RK3288_SGRF_SOC_CON0. OK, hmmm. ...so I guess you're right that our current suspend/resume code assumes that this register is writable... I was relatively certain that SGRF was supposed to be non-accessible in boot modes when we're running the kernel at a lower privilege level. I think I remember thinking that whenever someone finally manages to implement that they would be in for quite a task dealing with suspend/resume. This is one thing that they'd have to deal with, but they'd also have to deal with the fact that we program the CPU to jump straight back to the kernel at resume time and the CPU will (as I understand it) be in "secure SVC" mode. ...so my thought is that it's OK to add this to the suspend/resume code and it'll just be another thing to figure out if anyone ever runs this in a different mode. I guess that brings up the question about whether we should revisit (e142a4e clk: rockchip: add a dummy clock for the watchdog pclk on rk3288) and actually try to implement it correctly. I'm still leaning towards leave it the way it is with the dummy clock, but if someone wants to try implementing it for real then I suppose you could have a nicer way to implement dw_wdt (no need for a kernel thread to keep patting). > I tried to set wdt_en(WDT_CR[bit 0]) to 0 in watchdog driver, but that would > cause system reboot. Wow, that stinks. That explains some of the hackiness of the current dw_wdt driver that was wondering about. That stinks that it's totally not documented in the user manual (unless I missed it). With all that: Reviewed-by: Doug Anderson <dianders@chromium.org> Tested-by: Doug Anderson <dianders@chromium.org>
Hi Chris & Doug, Am Mittwoch, 11. März 2015, 14:23:38 schrieb Doug Anderson: > On Mon, Mar 2, 2015 at 9:57 PM, Chris Zhong <zyw@rock-chips.com> wrote: > > On 03/03/2015 04:50 AM, Heiko Stuebner wrote: > >> Hi Chris, > >> > >> Am Montag, 9. Februar 2015, 21:12:23 schrieb Chris Zhong: > >>> The watchdog clock should be disable in dw_wdt_suspend, but we set a > >>> dummy clock to watchdog for rk3288. So the watchdog will continue to > >>> work during suspend. And we switch the system clock to 32khz from 24Mhz, > >>> during suspend, so the watchdog timer over count will increase to > >>> 755 times, about 12.5 hours, the original value is 60 seconds. So > >>> watchdog will reset the system over a night, but voltage are all > >>> incorrect, then it hang on reset. > >>> > >>> Signed-off-by: Chris Zhong <zyw@rock-chips.com> > >>> Signed-off-by: Daniel Kurtz <djkurtz@google.com> > >> > >> The SGRF is not writeable in all bootmodes (I've talked with Doug about > >> this > >> to verify I remembered this correctly), so handling the sgrf gate for the > >> watchdog is not safe for all possible boards. > >> > >> Why not simply turn off the watchdog in the driver during suspend? > > > > I think SGRF is writeable, since we would set this RK3288_SGRF_SOC_CON0 > > register when suspend. > > and this SGRF_PCLK_WDT_GATE is one bit of RK3288_SGRF_SOC_CON0. > > OK, hmmm. ...so I guess you're right that our current suspend/resume > code assumes that this register is writable... > > I was relatively certain that SGRF was supposed to be non-accessible > in boot modes when we're running the kernel at a lower privilege > level. I think I remember thinking that whenever someone finally > manages to implement that they would be in for quite a task dealing > with suspend/resume. This is one thing that they'd have to deal with, > but they'd also have to deal with the fact that we program the CPU to > jump straight back to the kernel at resume time and the CPU will (as I > understand it) be in "secure SVC" mode. > > ...so my thought is that it's OK to add this to the suspend/resume > code and it'll just be another thing to figure out if anyone ever runs > this in a different mode. with Chris recent comments I was leaning towards this as well, so I'm very happy to have someone share these thoughts :-) > I guess that brings up the question about whether we should revisit > (e142a4e clk: rockchip: add a dummy clock for the watchdog pclk on > rk3288) and actually try to implement it correctly. I'm still leaning > towards leave it the way it is with the dummy clock, but if someone > wants to try implementing it for real then I suppose you could have a > nicer way to implement dw_wdt (no need for a kernel thread to keep > patting). There are some more clocks living in the GRF and SGRF (muxes relating to vcodec and some more). At some point I want to tackle this, but for now I think that it can stay how it is. > > I tried to set wdt_en(WDT_CR[bit 0]) to 0 in watchdog driver, but that > > would cause system reboot. > > Wow, that stinks. That explains some of the hackiness of the current > dw_wdt driver that was wondering about. That stinks that it's totally > not documented in the user manual (unless I missed it). > > With all that: > > Reviewed-by: Doug Anderson <dianders@chromium.org> > Tested-by: Doug Anderson <dianders@chromium.org> I've now applied both patches with Doug's tags to my v4.1-armsoc/soc branch, but adapted the commit message for patch1 a tiny bit. Heiko
diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c index a3ab397..b07d886 100644 --- a/arch/arm/mach-rockchip/pm.c +++ b/arch/arm/mach-rockchip/pm.c @@ -75,9 +75,13 @@ static void rk3288_slp_mode_set(int level) regmap_read(pmu_regmap, RK3288_PMU_PWRMODE_CON, &rk3288_pmu_pwr_mode_con); - /* set bit 8 so that system will resume to FAST_BOOT_ADDR */ + /* + * SGRF_FAST_BOOT_EN - system to boot from FAST_BOOT_ADDR + * PCLK_WDT_GATE - disable WDT during suspend. + */ regmap_write(sgrf_regmap, RK3288_SGRF_SOC_CON0, - SGRF_FAST_BOOT_EN | SGRF_FAST_BOOT_EN_WRITE); + SGRF_PCLK_WDT_GATE | SGRF_FAST_BOOT_EN + | SGRF_PCLK_WDT_GATE_WRITE | SGRF_FAST_BOOT_EN_WRITE); /* booting address of resuming system is from this register value */ regmap_write(sgrf_regmap, RK3288_SGRF_FAST_BOOT_ADDR, @@ -122,7 +126,8 @@ static void rk3288_slp_mode_set_resume(void) rk3288_pmu_pwr_mode_con); regmap_write(sgrf_regmap, RK3288_SGRF_SOC_CON0, - rk3288_sgrf_soc_con0 | SGRF_FAST_BOOT_EN_WRITE); + rk3288_sgrf_soc_con0 | SGRF_PCLK_WDT_GATE_WRITE + | SGRF_FAST_BOOT_EN_WRITE); } static int rockchip_lpmode_enter(unsigned long arg) diff --git a/arch/arm/mach-rockchip/pm.h b/arch/arm/mach-rockchip/pm.h index 96beaa0..d463978 100644 --- a/arch/arm/mach-rockchip/pm.h +++ b/arch/arm/mach-rockchip/pm.h @@ -44,6 +44,8 @@ void __init rockchip_suspend_init(void); #define RK3288_SGRF_SOC_CON0 (0x0000) #define RK3288_SGRF_FAST_BOOT_ADDR (0x0120) +#define SGRF_PCLK_WDT_GATE BIT(6) +#define SGRF_PCLK_WDT_GATE_WRITE BIT(22) #define SGRF_FAST_BOOT_EN BIT(8) #define SGRF_FAST_BOOT_EN_WRITE BIT(24)