[PATCH v4 0/3] ARM: rockchip: fix the SMP

[PATCH v4 0/3] ARM: rockchip: fix the SMP

[Caesar Wang]

    Verified on url =
    https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14


Caesar Wang (3):
  ARM: rockchip: fix the CPU soft reset
  ARM: rockchip: ensure CPU to enter WFI/WFE state
  ARM: rockchip: fix the SMP code style

 arch/arm/mach-rockchip/platsmp.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH v4 1/3] ARM: rockchip: fix the CPU soft reset

[Caesar Wang]

We need different orderings when turning a core on and turning a core
off.  In one case we need to assert reset before turning power off.
In ther other case we need to turn power on and the deassert reset.

In general, the correct flow is:

CPU off:
    reset_control_assert
    regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), BIT(pd))
    ensure power domain is on
CPU on:
    regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), 0)
    reset_control_deassert
    ensure power domain is on

This is needed for stressing CPU up/down, as per:
    cd /sys/devices/system/cpu/
    for i in $(seq 1000); do
        echo "================= $i ============"
        for j in $(seq 100); do
            while [[ "$(cat cpu1/online)$(cat cpu2/online)$(cat cpu3/online)" != "000"" ]]
                echo 0 > cpu1/online
                echo 0 > cpu2/online
                echo 0 > cpu3/online
            done
            while [[ "$(cat cpu1/online)$(cat cpu2/online)$(cat cpu3/online)" != "111" ]]; do
                echo 1 > cpu1/online
                echo 1 > cpu2/online
                echo 1 > cpu3/online
            done
        done
    done

The following is reproducile log:
    [34466.186812] PM: noirq suspend of devices complete after 0.669 msecs
    [34466.186824] Disabling non-boot CPUs ...
    [34466.187509] CPU1: shutdown
    [34466.188672] CPU2: shutdown
    [34473.736627] Kernel panic - not syncing:Watchdog detected hard LOCKUP on cpu 0
    .......

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>
---

 arch/arm/mach-rockchip/platsmp.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
index 5b4ca3c..a297b86 100644
--- a/arch/arm/mach-rockchip/platsmp.c
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -88,18 +88,26 @@ static int pmu_set_power_domain(int pd, bool on)
 			return PTR_ERR(rstc);
 		}
 
+		if (!on)
+			reset_control_assert(rstc);
+
+		ret = regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), val);
+		if (ret < 0) {
+			pr_err("%s: could not update power domain\n", __func__);
+			reset_control_put(rstc);
+			return ret;
+		}
+
 		if (on)
 			reset_control_deassert(rstc);
-		else
-			reset_control_assert(rstc);
 
 		reset_control_put(rstc);
-	}
-
-	ret = regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), val);
-	if (ret < 0) {
-		pr_err("%s: could not update power domain\n", __func__);
-		return ret;
+	} else {
+		ret = regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), val);
+		if (ret < 0) {
+			pr_err("%s: could not update power domain\n", __func__);
+			return ret;
+		}
 	}
 
 	ret = -1;
-- 
1.9.1

[PATCH v4 1/3] ARM: rockchip: fix the CPU soft reset

[Doug Anderson]

Caesar,

On Fri, Jun 5, 2015 at 10:05 AM, Caesar Wang <wxt@rock-chips.com> wrote:
> +               if (!on)
> +                       reset_control_assert(rstc);
> +
> +               ret = regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), val);
> +               if (ret < 0) {
> +                       pr_err("%s: could not update power domain\n", __func__);
> +                       reset_control_put(rstc);
> +                       return ret;
> +               }
> +
>                 if (on)
>                         reset_control_deassert(rstc);
> -               else
> -                       reset_control_assert(rstc);

As Heiko indicated in the previous patchset, he thought it would be
nice to move the 'pmu_power_domain_is_on(pd)' to before you deasserted
reset.  ...but then I pointed out that you tested that in patch set #2
and it didn't work.

Talking to Heiko offline he thought that perhaps you could make
'pmu_power_domain_is_on(pd)' work if you increased your 'udelay(10);'
in rockchip_boot_secondary().  Perhaps the old
'pmu_power_domain_is_on' was acting like an extra bit of delay and
that's why moving it broke things.


I actually went back and tested patch set #2 and it worked for me, so
I couldn't test Heiko's theory.  Could you go and reproduce the
problem with patch set #2 again and then try increasing the udelay()
and see if your problems go away?  If that works, it might be a
slightly better solution.  Note that I think Heiko had a slightly
cleaner version of your patch set #2 that he posted in response to
your patch set #3.


-Doug

[PATCH v4 1/3] ARM: rockchip: fix the CPU soft reset

[Caesar Wang]

? 2015?06?06? 04:55, Doug Anderson ??:
> Caesar,
>
> On Fri, Jun 5, 2015 at 10:05 AM, Caesar Wang <wxt@rock-chips.com> wrote:
>> +               if (!on)
>> +                       reset_control_assert(rstc);
>> +
>> +               ret = regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), val);
>> +               if (ret < 0) {
>> +                       pr_err("%s: could not update power domain\n", __func__);
>> +                       reset_control_put(rstc);
>> +                       return ret;
>> +               }
>> +
>>                  if (on)
>>                          reset_control_deassert(rstc);
>> -               else
>> -                       reset_control_assert(rstc);
> As Heiko indicated in the previous patchset, he thought it would be
> nice to move the 'pmu_power_domain_is_on(pd)' to before you deasserted
> reset.  ...but then I pointed out that you tested that in patch set #2
> and it didn't work.
>
> Talking to Heiko offline he thought that perhaps you could make
> 'pmu_power_domain_is_on(pd)' work if you increased your 'udelay(10);'
> in rockchip_boot_secondary().  Perhaps the old
> 'pmu_power_domain_is_on' was acting like an extra bit of delay and
> that's why moving it broke things.
>
>
> I actually went back and tested patch set #2 and it worked for me, so
> I couldn't test Heiko's theory.  Could you go and reproduce the
> problem with patch set #2 again and then try increasing the udelay()
> and see if your problems go away?  If that works, it might be a
> slightly better solution.  Note that I think Heiko had a slightly
> cleaner version of your patch set #2 that he posted in response to
> your patch set #3.

@@ -150,13 +159,15 @@ static int __cpuinit 
rockchip_boot_secondary(unsigned int cpu,
                  * sram_base_addr + 4: 0xdeadbeaf
                  * sram_base_addr + 8: start address for pc
                  * */
-               udelay(10);
+               udelay(20);

I increased the 'udelay(20)' or 'udelay(50)' in rockchip_boot_secondary().
Set#2 also can repro this issue over 22600 cycles with testing scripts. 
(about 1 hours)

log:
================= 226 ============
[ 4069.134419] CPU1: shutdown
[ 4069.164431] CPU2: shutdown
[ 4069.204475] CPU3: shutdown
......
[ 4072.454453] CPU1: shutdown
[ 4072.504436] CPU2: shutdown
[ 4072.554426] CPU3: shutdown
[ 4072.577827] CPU1: Booted secondary processor
[ 4072.582611] CPU2: Booted secondary processor
[ 4072.587426] CPU3: Booted secondary processor
<hang>

The set #4 will be better work.

>
> -Doug
>
>
>

-- 
Thanks,
- Caesar

[PATCH v4 1/3] ARM: rockchip: fix the CPU soft reset

[Doug Anderson]

Caesar,

On Sat, Jun 6, 2015 at 7:51 PM, Caesar Wang <wxt@rock-chips.com> wrote:
> @@ -150,13 +159,15 @@ static int __cpuinit rockchip_boot_secondary(unsigned
> int cpu,
>                  * sram_base_addr + 4: 0xdeadbeaf
>                  * sram_base_addr + 8: start address for pc
>                  * */
> -               udelay(10);
> +               udelay(20);
>
> I increased the 'udelay(20)' or 'udelay(50)' in rockchip_boot_secondary().
> Set#2 also can repro this issue over 22600 cycles with testing scripts.
> (about 1 hours)
>
> log:
> ================= 226 ============
> [ 4069.134419] CPU1: shutdown
> [ 4069.164431] CPU2: shutdown
> [ 4069.204475] CPU3: shutdown
> ......
> [ 4072.454453] CPU1: shutdown
> [ 4072.504436] CPU2: shutdown
> [ 4072.554426] CPU3: shutdown
> [ 4072.577827] CPU1: Booted secondary processor
> [ 4072.582611] CPU2: Booted secondary processor
> [ 4072.587426] CPU3: Booted secondary processor
> <hang>
>
> The set #4 will be better work.

OK, I'm OK with this, but I'd like to get Heiko's opinion.

Also:
* Just for kicks, does mdelay(1) work?  I know that's 100x more than
udelay(10), but previously we were actually looping waiting for the
power domain, right?  ...so maybe the old code used to introduce a
pretty big delay.

* Does anyone from the chip design team have any idea why patch set #4
works but patch set #2 doesn't?  I know it's Sunday morning in China
right now, but maybe you could ask Monday?


Thanks!

-Doug

[PATCH v4 1/3] ARM: rockchip: fix the CPU soft reset

[Caesar Wang]

? 2015?06?07? 11:43, Doug Anderson ??:
> Caesar,
>
> On Sat, Jun 6, 2015 at 7:51 PM, Caesar Wang <wxt@rock-chips.com> wrote:
>> @@ -150,13 +159,15 @@ static int __cpuinit rockchip_boot_secondary(unsigned
>> int cpu,
>>                   * sram_base_addr + 4: 0xdeadbeaf
>>                   * sram_base_addr + 8: start address for pc
>>                   * */
>> -               udelay(10);
>> +               udelay(20);
>>
>> I increased the 'udelay(20)' or 'udelay(50)' in rockchip_boot_secondary().
>> Set#2 also can repro this issue over 22600 cycles with testing scripts.
>> (about 1 hours)
>>
>> log:
>> ================= 226 ============
>> [ 4069.134419] CPU1: shutdown
>> [ 4069.164431] CPU2: shutdown
>> [ 4069.204475] CPU3: shutdown
>> ......
>> [ 4072.454453] CPU1: shutdown
>> [ 4072.504436] CPU2: shutdown
>> [ 4072.554426] CPU3: shutdown
>> [ 4072.577827] CPU1: Booted secondary processor
>> [ 4072.582611] CPU2: Booted secondary processor
>> [ 4072.587426] CPU3: Booted secondary processor
>> <hang>
>>
>> The set #4 will be better work.
> OK, I'm OK with this, but I'd like to get Heiko's opinion.
>
> Also:
> * Just for kicks, does mdelay(1) work?  I know that's 100x more than
OK, it should delay more time.

the mdelay(1) can be work over 50000 cycles, so that should be work.


Perhaps, can we use 'usleep_range(500, 1000)' to work.
Heiko, do you agree with it?

> udelay(10), but previously we were actually looping waiting for the
> power domain, right?  ...so maybe the old code used to introduce a
> pretty big delay.
>
> * Does anyone from the chip design team have any idea why patch set #4
> works but patch set #2 doesn't?  I know it's Sunday morning in China
> right now, but maybe you could ask Monday?
>
>
> Thanks!
>
> -Doug
>
>
>

-- 
Thanks,
- Caesar

[PATCH v4 1/3] ARM: rockchip: fix the CPU soft reset

[Heiko Stübner]

Hi Caesar, Doug,

Am Sonntag, 7. Juni 2015, 13:51:24 schrieb Caesar Wang:
> ? 2015?06?07? 11:43, Doug Anderson ??:
> > Caesar,
> > 
> > On Sat, Jun 6, 2015 at 7:51 PM, Caesar Wang <wxt@rock-chips.com> wrote:
> >> @@ -150,13 +159,15 @@ static int __cpuinit
> >> rockchip_boot_secondary(unsigned
> >> int cpu,
> >> 
> >>                   * sram_base_addr + 4: 0xdeadbeaf
> >>                   * sram_base_addr + 8: start address for pc
> >>                   * */
> >> 
> >> -               udelay(10);
> >> +               udelay(20);
> >> 
> >> I increased the 'udelay(20)' or 'udelay(50)' in
> >> rockchip_boot_secondary().
> >> Set#2 also can repro this issue over 22600 cycles with testing scripts.
> >> (about 1 hours)
> >> 
> >> log:
> >> ================= 226 ============
> >> [ 4069.134419] CPU1: shutdown
> >> [ 4069.164431] CPU2: shutdown
> >> [ 4069.204475] CPU3: shutdown
> >> ......
> >> [ 4072.454453] CPU1: shutdown
> >> [ 4072.504436] CPU2: shutdown
> >> [ 4072.554426] CPU3: shutdown
> >> [ 4072.577827] CPU1: Booted secondary processor
> >> [ 4072.582611] CPU2: Booted secondary processor
> >> [ 4072.587426] CPU3: Booted secondary processor
> >> <hang>
> >> 
> >> The set #4 will be better work.
> > 
> > OK, I'm OK with this, but I'd like to get Heiko's opinion.
> > 
> > Also:
> > * Just for kicks, does mdelay(1) work?  I know that's 100x more than
> 
> OK, it should delay more time.
> 
> the mdelay(1) can be work over 50000 cycles, so that should be work.
> 
> 
> Perhaps, can we use 'usleep_range(500, 1000)' to work.
> Heiko, do you agree with it?

yep :-)

As I said before, doing

	powerup, deassert_reset, wait_for_powerdomain

feels like it is only moving the problem a bit but is actually only working by 
chance, as my [little bit of :-) ] common sense tells me, that we really only 
should deassert the reset when we're sure that the core has power, i.e.

	powerup, wait_for_powerdomain, deassert_reset

Also, when going down this path, please take a look at the slightly different 
variant I posted as response to v3, as it makes the diff a bit smaller :-)


As for {u/m}delay vs. your usleep_ranges, I don't know if you're allowed to 
sleep in this area. Other architectures only seem to use udelay in __cpu_up 
which calls the smp_secondary_startup callback, like:

- arch/sh/kernel/smp.c
- arch/m32r/kernel/smpboot.c [is even a udelay(1000)
and more


Heiko

> > udelay(10), but previously we were actually looping waiting for the
> > power domain, right?  ...so maybe the old code used to introduce a
> > pretty big delay.
> > 
> > * Does anyone from the chip design team have any idea why patch set #4
> > works but patch set #2 doesn't?  I know it's Sunday morning in China
> > right now, but maybe you could ask Monday?
> > 
> > 
> > Thanks!
> > 
> > -Doug

[PATCH v4 2/3] ARM: rockchip: ensure CPU to enter WFI/WFE state

[Caesar Wang]

In idle mode, core1/2/3 of Cortex-A17 should be either power off or in
WFI/WFE state.
we can delay 1ms to ensure the CPU enter WFI/WFE state.

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 arch/arm/mach-rockchip/platsmp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
index a297b86..d809fbc 100644
--- a/arch/arm/mach-rockchip/platsmp.c
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -327,6 +327,9 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
 #ifdef CONFIG_HOTPLUG_CPU
 static int rockchip_cpu_kill(unsigned int cpu)
 {
+	/* ensure CPU can enter the WFI/WFE state */
+	mdelay(1);
+
 	pmu_set_power_domain(0 + cpu, false);
 	return 1;
 }
-- 
1.9.1

[PATCH v4 3/3] ARM: rockchip: fix the SMP code style

[Caesar Wang]

Use the below scripts to check:
scripts/checkpatch.pl -f --subject arch/arm/mach-rockchip/platsmp.c
Although there is a check, it's no matter.

CHECK: usleep_range is preferred over udelay; see
Documentation/timers/timers-howto.txt
+167udelay(10);
total: 0 errors, 0 warnings, 1 checks, 362 lines checked

 Changes in v4:
    - Add reset_control_put(rstc) for the non-error case.
    - Fix commit information in PATCH [1/3]

 Changes in v3:
    - FIx the PATCH v2, it doesn't work on chromium 3.14.

 Changes in v2:
    - As Kever points out, Fix the subject typo WIF/WFI in PATCH [2/3].
    - As Heiko suggestion, re-adjust the cpu on/off flow in PATCH [1/3].
    - Use the checkpatch.pl -f --subjective to check in PATCH [3/3].

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 arch/arm/mach-rockchip/platsmp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
index d809fbc..63e9c7c 100644
--- a/arch/arm/mach-rockchip/platsmp.c
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -115,7 +115,7 @@ static int pmu_set_power_domain(int pd, bool on)
 		ret = pmu_power_domain_is_on(pd);
 		if (ret < 0) {
 			pr_err("%s: could not read power domain state\n",
-				 __func__);
+			       __func__);
 			return ret;
 		}
 	}
@@ -139,7 +139,7 @@ static int __cpuinit rockchip_boot_secondary(unsigned int cpu,
 
 	if (cpu >= ncores) {
 		pr_err("%s: cpu %d outside maximum number of cpus %d\n",
-							__func__, cpu, ncores);
+		       __func__, cpu, ncores);
 		return -ENXIO;
 	}
 
@@ -158,7 +158,7 @@ static int __cpuinit rockchip_boot_secondary(unsigned int cpu,
 		 * */
 		udelay(10);
 		writel(virt_to_phys(rockchip_secondary_startup),
-			sram_base_addr + 8);
+		       sram_base_addr + 8);
 		writel(0xDEADBEAF, sram_base_addr + 4);
 		dsb_sev();
 	}
@@ -337,7 +337,7 @@ static int rockchip_cpu_kill(unsigned int cpu)
 static void rockchip_cpu_die(unsigned int cpu)
 {
 	v7_exit_coherency_flush(louis);
-	while(1)
+	while (1)
 		cpu_do_idle();
 }
 #endif
@@ -350,4 +350,5 @@ static struct smp_operations rockchip_smp_ops __initdata = {
 	.cpu_die		= rockchip_cpu_die,
 #endif
 };
+
 CPU_METHOD_OF_DECLARE(rk3066_smp, "rockchip,rk3066-smp", &rockchip_smp_ops);
-- 
1.9.1

[PATCH v4 3/3] ARM: rockchip: fix the SMP code style

[Doug Anderson]

Caesar,

On Fri, Jun 5, 2015 at 10:05 AM, Caesar Wang <wxt@rock-chips.com> wrote:
> Use the below scripts to check:
> scripts/checkpatch.pl -f --subject arch/arm/mach-rockchip/platsmp.c
> Although there is a check, it's no matter.
>
> CHECK: usleep_range is preferred over udelay; see
> Documentation/timers/timers-howto.txt
> +167udelay(10);
> total: 0 errors, 0 warnings, 1 checks, 362 lines checked
>
>  Changes in v4:
>     - Add reset_control_put(rstc) for the non-error case.
>     - Fix commit information in PATCH [1/3]
>
>  Changes in v3:
>     - FIx the PATCH v2, it doesn't work on chromium 3.14.
>
>  Changes in v2:
>     - As Kever points out, Fix the subject typo WIF/WFI in PATCH [2/3].
>     - As Heiko suggestion, re-adjust the cpu on/off flow in PATCH [1/3].
>     - Use the checkpatch.pl -f --subjective to check in PATCH [3/3].

The change log should (ideally) be with each patch.  If that's too
hard and you want to put it in one place, please put it in the cover
letter (the 0/3 email).

Also: the change log should be "below the cut".  That is, it should be
below the "---" and above the diffstat.

Other than those problems:

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


-Doug

[PATCH v4 0/3] ARM: rockchip: fix the SMP

[Caesar Wang]

    Verified on url =
    https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14


Caesar Wang (3):
  ARM: rockchip: fix the CPU soft reset
  ARM: rockchip: ensure CPU to enter WFI/WFE state
  ARM: rockchip: fix the SMP code style

 arch/arm/mach-rockchip/platsmp.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

-- 
1.9.1