Hi Caesar,
thanks for investigating this.
Am Freitag, 5. Juni 2015, 12:47:55 schrieb Caesar Wang:
> In general, the correct flow is:
>
> cpu off:
> reset_control_assert
> regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), BIT(pd))
>
> cpu on:
> regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), 0)
> reset_control_deassert
>
> You can repro it with bringing CPU up and down.
> Says:(test scripts)
>
> 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" ]]; do 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 [34473.736646] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 3.14.0 #1
> [34473.736687] [<c010dfb0>] (unwind_backtrace) from [<c010a460>]
> (show_stack+0x20/0x24) [34473.736711] [<c010a460>] (show_stack) from
> [<c0630670>] (dump_stack+0x70/0x8c) [34473.736731] [<c0630670>]
> (dump_stack) from [<c062fa0c>] (panic+0xa8/0x1fc) [34473.736754]
> [<c062fa0c>] (panic) from [<c0191efc>] (watchdog_timer_fn+0x234/0x26c)
> [34473.736777] [<c0191efc>] (watchdog_timer_fn) from [<c014103c>]
> (__run_hrtimer+0x118/0x1e0) [34473.736797] [<c014103c>] (__run_hrtimer)
> from [<c0141c04>] (hrtimer_interrupt+0x148/0x2a0) [34473.736820]
> [<c0141c04>] (hrtimer_interrupt) from [<c04d9994>]
> (arch_timer_handler_phys+0x38/0x48) [34473.736844] [<c04d9994>]
> (arch_timer_handler_phys) from [<c016acc4>]
> (handle_percpu_devid_irq+0xb8/0x124) [34473.736867] [<c016acc4>]
> (handle_percpu_devid_irq) from [<c0166a30>] (generic_handle_irq+0x30/0x40)
> [34473.736887] [<c0166a30>] (generic_handle_irq) from [<c0166d28>]
> (__handle_domain_irq+0x8c/0xb0) [34473.736905] [<c0166d28>]
> (__handle_domain_irq) from [<c01003a0>] (gic_handle_irq+0x48/0x6c)
> [34473.736922] [<c01003a0>] (gic_handle_irq) from [<c010b040>]
> (__irq_svc+0x40/0x50) [34473.736936] Exception stack(0xee127f70 to
> 0xee127fb8)
> [34473.736948] 7f60: ffffffed 00000000
> 2dd6d000 00000000 [34473.736964] 7f80: ee126000 00000015 c0b46bac c0b46bac
> 0000406a 410fc0d1 00000000 ee127fc4 [34473.736979] 7fa0: ee127fb8 ee127fb8
> c0107038 c010703c 600f0013 ffffffff [34473.736995] [<c010b040>] (__irq_svc)
> from [<c010703c>] (arch_cpu_idle+0x40/0x48) [34473.737013] [<c010703c>]
> (arch_cpu_idle) from [<c0166978>] (cpu_startup_entry+0x170/0x1d0)
> [34473.737031] [<c0166978>] (cpu_startup_entry) from [<c010c254>]
> (secondary_start_kernel+0x138/0x160) [34473.737059] [<c010c254>]
> (secondary_start_kernel) from [<00100464>] (0x100464) [34474.903740] SMP:
> failed to stop secondary CPUs
> [34476.099964] SMP: failed to stop secondary CPUs
> ...
>
> Signed-off-by: Caesar Wang <wxt at rock-chips.com>
> ---
>
> arch/arm/mach-rockchip/platsmp.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-rockchip/platsmp.c
> b/arch/arm/mach-rockchip/platsmp.c index 5b4ca3c..1230d3d 100644
> --- a/arch/arm/mach-rockchip/platsmp.c
> +++ b/arch/arm/mach-rockchip/platsmp.c
> @@ -88,20 +88,17 @@ static int pmu_set_power_domain(int pd, bool on)
> return PTR_ERR(rstc);
> }
>
> - if (on)
> + if (on) {
> + regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), val);
> reset_control_deassert(rstc);
> - else
> + } else {
> reset_control_assert(rstc);
> + regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), val);
> + }
you're loosing the return value of regmap_update_bits here, I guess it should
look like below?
if (on) {
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;
}
reset_control_deassert(rstc);
} else {
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;
}
}
>
> 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;
> - }
> -
> ret = -1;
> while (ret != on) {
> ret = pmu_power_domain_is_on(pd);
second question - with this patch, what happens actually is
cpu off:
reset_control_assert
regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), BIT(pd))
wait_for_power_domain_to_turn_off
cpu on:
regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), 0)
reset_control_deassert
wait_for_power_domain_to_turn_on
So shouldn't the deassertion of the reset happen after the powerdomain
sucessfull turned on? Like
cpu on:
regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), 0)
wait_for_power_domain_to_turn_on
reset_control_deassert
