[PATCH 1/2] ARM: rockchip: decrease the wait time for resume

[PATCH 1/2] ARM: rockchip: decrease the wait time for resume

[Chris Zhong]

The delay time for wait the 24MHz OSC stabilization is 750ms, and the
delay time for wait the external PMU stabilization is 750ms too, let's
decrease them to 30ms.

Signed-off-by: Chris Zhong <zyw@rock-chips.com>
---

 arch/arm/mach-rockchip/pm.c | 3 +++
 arch/arm/mach-rockchip/pm.h | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c
index 50cb781..a3ab397 100644
--- a/arch/arm/mach-rockchip/pm.c
+++ b/arch/arm/mach-rockchip/pm.c
@@ -209,6 +209,9 @@ static int rk3288_suspend_init(struct device_node *np)
 	memcpy(rk3288_bootram_base, rockchip_slp_cpu_resume,
 	       rk3288_bootram_sz);
 
+	regmap_write(pmu_regmap, RK3288_PMU_OSC_CNT, OSC_STABL_CNT_THRESH);
+	regmap_write(pmu_regmap, RK3288_PMU_STABL_CNT, PMU_STABL_CNT_THRESH);
+
 	return 0;
 }
 
diff --git a/arch/arm/mach-rockchip/pm.h b/arch/arm/mach-rockchip/pm.h
index 7d752ff..96beaa0 100644
--- a/arch/arm/mach-rockchip/pm.h
+++ b/arch/arm/mach-rockchip/pm.h
@@ -57,6 +57,10 @@ void __init rockchip_suspend_init(void);
 /* PMU_WAKEUP_CFG1 bits */
 #define PMU_ARMINT_WAKEUP_EN		BIT(0)
 
+/* wait 30ms for OSC stable and 30ms for pmic stable */
+#define OSC_STABL_CNT_THRESH	(32 * 30)
+#define PMU_STABL_CNT_THRESH	(32 * 30)
+
 enum rk3288_pwr_mode_con {
 	PMU_PWR_MODE_EN = 0,
 	PMU_CLK_CORE_SRC_GATE_EN,
-- 
1.9.1

[PATCH 2/2] ARM: rockchip: disable watchdog during suspend

[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>

---

 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)
 
-- 
1.9.1

[PATCH 1/2] ARM: rockchip: decrease the wait time for resume

[Heiko Stuebner]

Am Montag, 9. Februar 2015, 21:12:22 schrieb Chris Zhong:
> The delay time for wait the 24MHz OSC stabilization is 750ms, and the
> delay time for wait the external PMU stabilization is 750ms too, let's
> decrease them to 30ms.

just to understand whats happening here:

The default delay time for wait the 24MHz OSC and PMU stabilization is 750ms, 
= reset value in the register and your patch is decreasing this to 30ms.

Are the new 30ms for each of the two long enough in all cases?


Heiko


> 
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> ---
> 
>  arch/arm/mach-rockchip/pm.c | 3 +++
>  arch/arm/mach-rockchip/pm.h | 4 ++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c
> index 50cb781..a3ab397 100644
> --- a/arch/arm/mach-rockchip/pm.c
> +++ b/arch/arm/mach-rockchip/pm.c
> @@ -209,6 +209,9 @@ static int rk3288_suspend_init(struct device_node *np)
>  	memcpy(rk3288_bootram_base, rockchip_slp_cpu_resume,
>  	       rk3288_bootram_sz);
> 
> +	regmap_write(pmu_regmap, RK3288_PMU_OSC_CNT, OSC_STABL_CNT_THRESH);
> +	regmap_write(pmu_regmap, RK3288_PMU_STABL_CNT, PMU_STABL_CNT_THRESH);
> +
>  	return 0;
>  }
> 
> diff --git a/arch/arm/mach-rockchip/pm.h b/arch/arm/mach-rockchip/pm.h
> index 7d752ff..96beaa0 100644
> --- a/arch/arm/mach-rockchip/pm.h
> +++ b/arch/arm/mach-rockchip/pm.h
> @@ -57,6 +57,10 @@ void __init rockchip_suspend_init(void);
>  /* PMU_WAKEUP_CFG1 bits */
>  #define PMU_ARMINT_WAKEUP_EN		BIT(0)
> 
> +/* wait 30ms for OSC stable and 30ms for pmic stable */
> +#define OSC_STABL_CNT_THRESH	(32 * 30)
> +#define PMU_STABL_CNT_THRESH	(32 * 30)
> +
>  enum rk3288_pwr_mode_con {
>  	PMU_PWR_MODE_EN = 0,
>  	PMU_CLK_CORE_SRC_GATE_EN,

[PATCH 2/2] ARM: rockchip: disable watchdog during suspend

[Heiko Stuebner]

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)

[PATCH 1/2] ARM: rockchip: decrease the wait time for resume

[Chris Zhong]

On 03/03/2015 04:47 AM, Heiko Stuebner wrote:
> Am Montag, 9. Februar 2015, 21:12:22 schrieb Chris Zhong:
>> The delay time for wait the 24MHz OSC stabilization is 750ms, and the
>> delay time for wait the external PMU stabilization is 750ms too, let's
>> decrease them to 30ms.
> just to understand whats happening here:
>
> The default delay time for wait the 24MHz OSC and PMU stabilization is 750ms,
> = reset value in the register and your patch is decreasing this to 30ms.
>
> Are the new 30ms for each of the two long enough in all cases?
Yes, the 30ms are safe for wait them to stabilization.

>
> Heiko
>
>
>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>> ---
>>
>>   arch/arm/mach-rockchip/pm.c | 3 +++
>>   arch/arm/mach-rockchip/pm.h | 4 ++++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c
>> index 50cb781..a3ab397 100644
>> --- a/arch/arm/mach-rockchip/pm.c
>> +++ b/arch/arm/mach-rockchip/pm.c
>> @@ -209,6 +209,9 @@ static int rk3288_suspend_init(struct device_node *np)
>>   	memcpy(rk3288_bootram_base, rockchip_slp_cpu_resume,
>>   	       rk3288_bootram_sz);
>>
>> +	regmap_write(pmu_regmap, RK3288_PMU_OSC_CNT, OSC_STABL_CNT_THRESH);
>> +	regmap_write(pmu_regmap, RK3288_PMU_STABL_CNT, PMU_STABL_CNT_THRESH);
>> +
>>   	return 0;
>>   }
>>
>> diff --git a/arch/arm/mach-rockchip/pm.h b/arch/arm/mach-rockchip/pm.h
>> index 7d752ff..96beaa0 100644
>> --- a/arch/arm/mach-rockchip/pm.h
>> +++ b/arch/arm/mach-rockchip/pm.h
>> @@ -57,6 +57,10 @@ void __init rockchip_suspend_init(void);
>>   /* PMU_WAKEUP_CFG1 bits */
>>   #define PMU_ARMINT_WAKEUP_EN		BIT(0)
>>
>> +/* wait 30ms for OSC stable and 30ms for pmic stable */
>> +#define OSC_STABL_CNT_THRESH	(32 * 30)
>> +#define PMU_STABL_CNT_THRESH	(32 * 30)
>> +
>>   enum rk3288_pwr_mode_con {
>>   	PMU_PWR_MODE_EN = 0,
>>   	PMU_CLK_CORE_SRC_GATE_EN,
>
>
>

[PATCH 2/2] ARM: rockchip: disable watchdog during suspend

[Chris Zhong]

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)
>
>
>

[PATCH 2/2] ARM: rockchip: disable watchdog during suspend

[Doug Anderson]

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>

[PATCH 1/2] ARM: rockchip: decrease the wait time for resume

[Doug Anderson]

Hi,

On Mon, Feb 9, 2015 at 5:12 AM, Chris Zhong <zyw@rock-chips.com> wrote:
> The delay time for wait the 24MHz OSC stabilization is 750ms, and the
> delay time for wait the external PMU stabilization is 750ms too, let's
> decrease them to 30ms.
>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> ---
>
>  arch/arm/mach-rockchip/pm.c | 3 +++
>  arch/arm/mach-rockchip/pm.h | 4 ++++
>  2 files changed, 7 insertions(+)

I have done a lot of testing and as far as I'm aware this patch is
safe and right.

Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>

[PATCH 2/2] ARM: rockchip: disable watchdog during suspend

[Heiko Stuebner]

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