Re: [PATCH v6 4/7] ARM: rockchip: add suspend and resume for RK3288
From: Chris Zhong
Date: Tue Nov 11 2014 - 02:33:20 EST
On 10/30/2014 03:01 AM, Kevin Hilman wrote:
Chris Zhong <zyw@xxxxxxxxxxxxxx> writes:
It's a basic version of suspend and resume for rockchip,
it only support RK3288 now.
Signed-off-by: Tony Xie <xxx@xxxxxxxxxxxxxx>
Signed-off-by: Chris Zhong <zyw@xxxxxxxxxxxxxx>
Tested-by: Doug Anderson <dianders@xxxxxxxxxxxx>
Reviewed-by: Doug Anderson <dianders@xxxxxxxxxxxx>
---
+static void rk3288_fill_in_bootram(u32 level)
This function name no longer reflects what it does...
have removed in v7.
+{
+ rkpm_bootdata_cpusp = rk3288_bootram_phy + (SZ_4K - 8);
+ rkpm_bootdata_cpu_code = virt_to_phys(cpu_resume);
+
+ rkpm_bootdata_l2ctlr_f = 1;
+ rkpm_bootdata_l2ctlr = rk3288_l2_config();
+
+ rkpm_bootdata_ddr_code = 0;
Already initialized to 0 in the resume code.
have removed in v7
+}
In addition, you're no longer copying the resume code to SRAM, but this
function writes values into the version of resume code that's in memory.
IOW, this function is being called for every suspend cycle, but has no
effect since the resume code has already been copied to SRAM.
Seems to me like this whole 'fill_in_bootram()' needs to go away, and be
replaced by a single setup at init time.
Done in v7
+static u32 rk3288_pmu_pwr_mode_con;
+static u32 rk3288_sgrf_soc_con0;
Minor nit, and it's up to Heiko's preference here, but it's generally
preferred to have global variables like this at the top of the file,
not interspersed with the code.
Also, I'm not sure if you expect the number of registers that need
saving to grow here, but regmap provides a regcache facility for keeping
copies of things that may be useful. Probably not worth the over head
for a single register in each of these regmaps, but worth exploring if
this is going to grow.
have moved them to the top of file in v7
+static void rk3288_slp_mode_set(int level)
+{
+ u32 mode_set, mode_set1;
+
+ regmap_read(sgrf_regmap, RK3288_SGRF_SOC_CON0, &rk3288_sgrf_soc_con0);
+
+ 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 */
+ regmap_write(sgrf_regmap, RK3288_SGRF_SOC_CON0,
+ BIT(SGRF_FAST_BOOT_EN) | BIT(SGRF_FAST_BOOT_EN + 16));
Comment says "bit 8", but code says bit 8 and bit 24, and if bit 24 is
needed, it should probably get its own #define in the header.
Done in v7
+
+ /* booting address of resuming system is from this register value */
+ regmap_write(sgrf_regmap, RK3288_SGRF_FAST_BOOT_ADDR,
+ rk3288_bootram_phy);
+
+ regmap_write(pmu_regmap, RK3288_PMU_WAKEUP_CFG1,
+ PMU_ARMINT_WAKEUP_EN);
+
+ mode_set = BIT(PMU_GLOBAL_INT_DISABLE) | BIT(PMU_L2FLUSH_EN) |
+ BIT(PMU_SREF0_ENTER_EN) | BIT(PMU_SREF1_ENTER_EN) |
+ BIT(PMU_DDR0_GATING_EN) | BIT(PMU_DDR1_GATING_EN) |
+ BIT(PMU_PWR_MODE_EN) | BIT(PMU_CHIP_PD_EN) |
+ BIT(PMU_SCU_EN);
+
+ mode_set1 = BIT(PMU_CLR_CORE) | BIT(PMU_CLR_CPUP);
+
+ if (level == ROCKCHIP_ARM_OFF_LOGIC_DEEP) {
+ /* arm off, logic deep sleep */
+ mode_set |= BIT(PMU_BUS_PD_EN) |
+ BIT(PMU_DDR1IO_RET_EN) | BIT(PMU_DDR0IO_RET_EN) |
+ BIT(PMU_OSC_24M_DIS) | BIT(PMU_PMU_USE_LF) |
+ BIT(PMU_ALIVE_USE_LF) | BIT(PMU_PLL_PD_EN);
+
+ mode_set1 |= BIT(PMU_CLR_ALIVE) | BIT(PMU_CLR_BUS) |
+ BIT(PMU_CLR_PERI) | BIT(PMU_CLR_DMA);
+
+ } else {
+ /*
+ * arm off, logic normal
+ * if pmu_clk_core_src_gate_en is not set,
+ * wakeup will be error
+ */
+ mode_set |= BIT(PMU_CLK_CORE_SRC_GATE_EN);
+ }
+
+ regmap_write(pmu_regmap, RK3288_PMU_PWRMODE_CON, mode_set);
+
extra blank line not needed
Done in v7
+ regmap_write(pmu_regmap, RK3288_PMU_PWRMODE_CON1, mode_set1);
+}
+
+static void rk3288_slp_mode_set_resume(void)
+{
+ regmap_write(pmu_regmap, RK3288_PMU_PWRMODE_CON,
+ rk3288_pmu_pwr_mode_con);
extra blank line
Done in v7
+ regmap_write(sgrf_regmap, RK3288_SGRF_SOC_CON0,
+ rk3288_sgrf_soc_con0 | BIT(SGRF_FAST_BOOT_EN + 16));
here is magic bit 24 again.
Done in v7
+}
+
+/*
+ * level: used to control the level of sleep mode.
+ * ROCKCHIP_ARM_OFF_LOGIC_NORMAL : arm is off and logic is in normal
+ * sleep mode.
+ * ROCKCHIP_ARM_OFF_LOGIC_DEEP : arm is off and logic is in deep sleep mode
IMO, these descriptions don't add any value over the already descriptive
names of the #defines
have removed it in v7
+ */
+static void rk3288_save_settings(u32 level)
+{
+ rk3288_fill_in_bootram(level);
+
+ rk3288_slp_mode_set(level);
+}
+
+static void rk3288_restore_settings(void)
+{
+ rk3288_slp_mode_set_resume();
+}
These 'save' and 'restore' functions seem like unnecessary levels of
redirection, especially if 'fill_in_bootram' goes away.
IMO, probably best to just move the calls into _suspend_enter().
Done in v7
.
.
.
diff --git a/arch/arm/mach-rockchip/pm.h b/arch/arm/mach-rockchip/pm.h
new file mode 100644
index 0000000..781ea9d
--- /dev/null
+++ b/arch/arm/mach-rockchip/pm.h
@@ -0,0 +1,103 @@
+/*
+ * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
+ * Author: Tony Xie <tony.xie@xxxxxxxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef __MACH_ROCKCHIP_PM_H
+#define __MACH_ROCKCHIP_PM_H
+
+extern unsigned long rkpm_bootdata_cpusp;
+extern unsigned long rkpm_bootdata_cpu_code;
+extern unsigned long rkpm_bootdata_l2ctlr_f;
+extern unsigned long rkpm_bootdata_l2ctlr;
+extern unsigned long rkpm_bootdata_ddr_code;
+extern unsigned long rkpm_bootdata_ddr_data;
+extern unsigned long rk3288_bootram_sz;
+/* The function selction of low power mode */
+/* arm is off and logic is in normal sleep mode */
+#define ROCKCHIP_ARM_OFF_LOGIC_NORMAL (0)
+/* arm is off and logic is in deep sleep mode */
+#define ROCKCHIP_ARM_OFF_LOGIC_DEEP (1)
These defines only seem to be used inside pm.c, so should be moved there
(and maybe turned into an enum?)
have removed them to pm.c, but still Macros, I will use enum in v8.
.
.
+ * ddr to sram for system resumeing.
+ * so it is ".data section".
+ */
+.align
+
+ENTRY(rockchip_slp_cpu_resume)
+ setmode PSR_I_BIT | PSR_F_BIT | SVC_MODE, r1 @ set svc, irqs off
+ mrc p15, 0, r1, c0, c0, 5
+ and r1, r1, #0xf
+ cmp r1, #0
+ /* olny cpu0 can continue to run, the others is halt here */
+ beq cpu0run
+secondary_loop:
+ wfe
+ b secondary_loop
+cpu0run:
+ ldr r3, rkpm_bootdata_l2ctlr_f
+ cmp r3, #0
+ beq sp_set
+ ldr r3, rkpm_bootdata_l2ctlr
+ mcr p15, 1, r3, c9, c0, 2
+sp_set:
+ ldr sp, rkpm_bootdata_cpusp
+
+ ldr r1, rkpm_bootdata_ddr_code
+ cmp r1, #0
+ beq res
In this series, 'ddr_code' is always zero, and othrewise unused. I'd
suggest leaving it out of this series entirely for simplicity (and
readability) and add it back in a patch/series where it's actually used.
+ ldr r0, rkpm_bootdata_ddr_data
+ blx r1
And 'ddr_data' is entirely unused.
Yes, the ddr_ have been removed in v7
+res:
+ ldr r1, rkpm_bootdata_cpu_code
+ bx r1
+ENDPROC(rockchip_slp_cpu_resume)
+
+/* Parameters filled in by the kernel */
+
+/* Code to jump to for DDR resume code, or NULL */
+ .global rkpm_bootdata_ddr_code
+rkpm_bootdata_ddr_code:
+ .long 0
+
+/* Data to pass to DDR resume data */
+ .global rkpm_bootdata_ddr_data
+rkpm_bootdata_ddr_data:
+ .long 0
+
+/* Flag for whether to restore L2CTLR on resume */
+ .global rkpm_bootdata_l2ctlr_f
+rkpm_bootdata_l2ctlr_f:
+ .long 0
+
+/* Saved L2CTLR to restore on resume */
+ .global rkpm_bootdata_l2ctlr
+rkpm_bootdata_l2ctlr:
+ .long 0
+
+/* CPU resume SP addr */
+ .globl rkpm_bootdata_cpusp
+rkpm_bootdata_cpusp:
+ .long 0
+
+/* CPU resume function (physical address) */
+ .globl rkpm_bootdata_cpu_code
+rkpm_bootdata_cpu_code:
+ .long 0
+
+ENTRY(rk3288_bootram_sz)
+ .word . - rockchip_slp_cpu_resume
Kevin
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-rockchip
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/