| Message ID | 1414417650-19402-4-git-send-email-zyw@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris, On Mon, Oct 27, 2014 at 6:47 AM, Chris Zhong <zyw@rock-chips.com> wrote: > save and restore some clks, which might be changed in suspend. > > Signed-off-by: Tony Xie <xxx@rock-chips.com> > Signed-off-by: Chris Zhong <zyw@rock-chips.com> > > --- > > Changes in v5: > - modify comments > > Changes in v4: None > Changes in v3: None > Changes in v2: > - __raw_readl/__raw_writel replaced by readl_relaxed/writel_relaxed > > drivers/clk/rockchip/clk-rk3288.c | 60 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c > index 2327829..94da06c 100644 > --- a/drivers/clk/rockchip/clk-rk3288.c > +++ b/drivers/clk/rockchip/clk-rk3288.c > @@ -16,6 +16,7 @@ > #include <linux/clk-provider.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/syscore_ops.h> > #include <dt-bindings/clock/rk3288-cru.h> > #include "clk.h" > > @@ -762,6 +763,64 @@ static const char *rk3288_critical_clocks[] __initconst = { > "hclk_peri", > }; > > +#ifdef CONFIG_PM_SLEEP > +static void __iomem *rk3288_cru_base; > +static const int rk3288_saved_cru_reg_ids[] = { > + RK3288_MODE_CON, > + RK3288_CLKSEL_CON(0), > + RK3288_CLKSEL_CON(1), > + RK3288_CLKSEL_CON(10), > + RK3288_CLKSEL_CON(33), > + RK3288_CLKSEL_CON(37), > +}; I still haven't had time to figure out exactly why these registers and not others... ...but I think that the code here looks reasonable and I have verified that saving at least some of these registers does matter. I'd vote to land these and if we have to add/remove registers later we can do it. Thus: Reviewed-by: Doug Anderson <dianders@chromium.org> Tested-by: Doug Anderson <dianders@chromium.org>
Chris, On Mon, Oct 27, 2014 at 8:15 PM, Chris Zhong <zyw@rock-chips.com> wrote: > > On 10/28/2014 01:27 AM, Doug Anderson wrote: > > Chris, > > On Mon, Oct 27, 2014 at 6:47 AM, Chris Zhong <zyw@rock-chips.com> wrote: > > save and restore some clks, which might be changed in suspend. > > Signed-off-by: Tony Xie <xxx@rock-chips.com> > Signed-off-by: Chris Zhong <zyw@rock-chips.com> > > --- > > Changes in v5: > - modify comments > > Changes in v4: None > Changes in v3: None > Changes in v2: > - __raw_readl/__raw_writel replaced by readl_relaxed/writel_relaxed > > drivers/clk/rockchip/clk-rk3288.c | 60 > +++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/drivers/clk/rockchip/clk-rk3288.c > b/drivers/clk/rockchip/clk-rk3288.c > index 2327829..94da06c 100644 > --- a/drivers/clk/rockchip/clk-rk3288.c > +++ b/drivers/clk/rockchip/clk-rk3288.c > @@ -16,6 +16,7 @@ > #include <linux/clk-provider.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/syscore_ops.h> > #include <dt-bindings/clock/rk3288-cru.h> > #include "clk.h" > > @@ -762,6 +763,64 @@ static const char *rk3288_critical_clocks[] __initconst > = { > "hclk_peri", > }; > > +#ifdef CONFIG_PM_SLEEP > +static void __iomem *rk3288_cru_base; > +static const int rk3288_saved_cru_reg_ids[] = { > + RK3288_MODE_CON, > + RK3288_CLKSEL_CON(0), > + RK3288_CLKSEL_CON(1), > + RK3288_CLKSEL_CON(10), > + RK3288_CLKSEL_CON(33), > + RK3288_CLKSEL_CON(37), > +}; > > I still haven't had time to figure out exactly why these registers and > not others... > > ...but I think that the code here looks reasonable and I have verified > that saving at least some of these registers does matter. I'd vote to > land these and if we have to add/remove registers later we can do it. > Thus: > > Reviewed-by: Doug Anderson <dianders@chromium.org> > Tested-by: Doug Anderson <dianders@chromium.org> > > > These CLKSEL registers would be changed in maskrom. so we need save and > restore them. > > code in maskrom ? > ... > cruReg->CRU_CLKSEL_CON[0] = (0x9fff<<16) > | (0x0<<15) // CORE select ARM PLL > | (0x0<<8) // core div > | (0x0<<4) // aclk_core_mp_div > | (0x0); // aclk_core_m0_dive > cruReg->CRU_CLKSEL_CON[1] = (0xf3ff<<16) > | (1<<15) // bus_aclk_pll_sel= GPLL > | (0x0<<12) // pd_bus_pclk_div > | (0x0<<8) // pd_bus_hclk_div=1:1 > | (0x0<<3) // pd_bus_aclk_div > | (0x0); // pd_bus_clk_div > cruReg->CRU_CLKSEL_CON[10] = (((0x1<<15)|(0x3<<12)|(0x3<<8)|(0x1f))<<16) > | (0x1<<15) // peri_pll_sel = GPLL > | (0x0<<12) // aclk_periph:pclk_periph = 1:1 > | (0x0<<8) // aclk_periph:hclk_periph = 1:1 > | (0x0); // GPLL:aclk_periph = 1:1 > cruReg->CRU_CLKSEL_CON[33] = (((0x1f<<8)|(0x1f))<<16) > | (0x0<<8) // alive_pclk > | (0x0); // pmu_pclk > cruReg->CRU_CLKSEL_CON[37] = (((0x1f<<9)|(0x1f<<4)|(0x7))<<16) > | (0<<9) //pclk_core_dbg > | (0x0<<4) // atclk_core > | (0x0); // clk_l2ram > ... OK, now it makes sense! When I read: + * Cru will be reset in maskrom when system wake up from fastboot. + * The apll/cpll/gpll will be set into slow mode in maskrom. + * So save them before suspend, restore them after resume. I thought that all of the CRU was being reset. ...but it's only some registers. I guess I would have expected: + * Some CRU registers will be reset in maskrom when the system + * wakes up from fastboot. + * So save them before suspend, restore them after resume. When you go into deep sleep do we need to save and restore more of the registers? I think your current patch series can only want up from the very lightest sleep mode, right?
On 10/28/2014 11:31 AM, Doug Anderson wrote: > Chris, > > On Mon, Oct 27, 2014 at 8:15 PM, Chris Zhong <zyw@rock-chips.com> wrote: >> On 10/28/2014 01:27 AM, Doug Anderson wrote: >> >> Chris, >> >> On Mon, Oct 27, 2014 at 6:47 AM, Chris Zhong <zyw@rock-chips.com> wrote: >> >> save and restore some clks, which might be changed in suspend. >> >> Signed-off-by: Tony Xie <xxx@rock-chips.com> >> Signed-off-by: Chris Zhong <zyw@rock-chips.com> >> >> --- >> >> Changes in v5: >> - modify comments >> >> Changes in v4: None >> Changes in v3: None >> Changes in v2: >> - __raw_readl/__raw_writel replaced by readl_relaxed/writel_relaxed >> >> drivers/clk/rockchip/clk-rk3288.c | 60 >> +++++++++++++++++++++++++++++++++++++ >> 1 file changed, 60 insertions(+) >> >> diff --git a/drivers/clk/rockchip/clk-rk3288.c >> b/drivers/clk/rockchip/clk-rk3288.c >> index 2327829..94da06c 100644 >> --- a/drivers/clk/rockchip/clk-rk3288.c >> +++ b/drivers/clk/rockchip/clk-rk3288.c >> @@ -16,6 +16,7 @@ >> #include <linux/clk-provider.h> >> #include <linux/of.h> >> #include <linux/of_address.h> >> +#include <linux/syscore_ops.h> >> #include <dt-bindings/clock/rk3288-cru.h> >> #include "clk.h" >> >> @@ -762,6 +763,64 @@ static const char *rk3288_critical_clocks[] __initconst >> = { >> "hclk_peri", >> }; >> >> +#ifdef CONFIG_PM_SLEEP >> +static void __iomem *rk3288_cru_base; >> +static const int rk3288_saved_cru_reg_ids[] = { >> + RK3288_MODE_CON, >> + RK3288_CLKSEL_CON(0), >> + RK3288_CLKSEL_CON(1), >> + RK3288_CLKSEL_CON(10), >> + RK3288_CLKSEL_CON(33), >> + RK3288_CLKSEL_CON(37), >> +}; >> >> I still haven't had time to figure out exactly why these registers and >> not others... >> >> ...but I think that the code here looks reasonable and I have verified >> that saving at least some of these registers does matter. I'd vote to >> land these and if we have to add/remove registers later we can do it. >> Thus: >> >> Reviewed-by: Doug Anderson <dianders@chromium.org> >> Tested-by: Doug Anderson <dianders@chromium.org> >> >> >> These CLKSEL registers would be changed in maskrom. so we need save and >> restore them. >> >> code in maskrom ? >> ... >> cruReg->CRU_CLKSEL_CON[0] = (0x9fff<<16) >> | (0x0<<15) // CORE select ARM PLL >> | (0x0<<8) // core div >> | (0x0<<4) // aclk_core_mp_div >> | (0x0); // aclk_core_m0_dive >> cruReg->CRU_CLKSEL_CON[1] = (0xf3ff<<16) >> | (1<<15) // bus_aclk_pll_sel= GPLL >> | (0x0<<12) // pd_bus_pclk_div >> | (0x0<<8) // pd_bus_hclk_div=1:1 >> | (0x0<<3) // pd_bus_aclk_div >> | (0x0); // pd_bus_clk_div >> cruReg->CRU_CLKSEL_CON[10] = (((0x1<<15)|(0x3<<12)|(0x3<<8)|(0x1f))<<16) >> | (0x1<<15) // peri_pll_sel = GPLL >> | (0x0<<12) // aclk_periph:pclk_periph = 1:1 >> | (0x0<<8) // aclk_periph:hclk_periph = 1:1 >> | (0x0); // GPLL:aclk_periph = 1:1 >> cruReg->CRU_CLKSEL_CON[33] = (((0x1f<<8)|(0x1f))<<16) >> | (0x0<<8) // alive_pclk >> | (0x0); // pmu_pclk >> cruReg->CRU_CLKSEL_CON[37] = (((0x1f<<9)|(0x1f<<4)|(0x7))<<16) >> | (0<<9) //pclk_core_dbg >> | (0x0<<4) // atclk_core >> | (0x0); // clk_l2ram >> ... > OK, now it makes sense! When I read: > > + * Cru will be reset in maskrom when system wake up from fastboot. > + * The apll/cpll/gpll will be set into slow mode in maskrom. > + * So save them before suspend, restore them after resume. > > I thought that all of the CRU was being reset. ...but it's only some > registers. I guess I would have expected: > > + * Some CRU registers will be reset in maskrom when the system > + * wakes up from fastboot. > + * So save them before suspend, restore them after resume. TKS, I will add it to the next version patch. > > When you go into deep sleep do we need to save and restore more of the > registers? I think your current patch series can only want up from > the very lightest sleep mode, right? the power supply of CRU is always on, unless power off mode. so don't worry about this issue.
Hi Chris, Am Dienstag, 28. Oktober 2014, 11:15:39 schrieb Chris Zhong: > On 10/28/2014 01:27 AM, Doug Anderson wrote: > > Chris, > > > > On Mon, Oct 27, 2014 at 6:47 AM, Chris Zhong <zyw@rock-chips.com> wrote: > >> save and restore some clks, which might be changed in suspend. > >> > >> Signed-off-by: Tony Xie <xxx@rock-chips.com> > >> Signed-off-by: Chris Zhong <zyw@rock-chips.com> > >> > >> --- > >> > >> Changes in v5: > >> - modify comments > >> > >> Changes in v4: None > >> Changes in v3: None > >> Changes in v2: > >> - __raw_readl/__raw_writel replaced by readl_relaxed/writel_relaxed > >> > >> drivers/clk/rockchip/clk-rk3288.c | 60 > >> +++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) > >> > >> diff --git a/drivers/clk/rockchip/clk-rk3288.c > >> b/drivers/clk/rockchip/clk-rk3288.c index 2327829..94da06c 100644 > >> --- a/drivers/clk/rockchip/clk-rk3288.c > >> +++ b/drivers/clk/rockchip/clk-rk3288.c > >> @@ -16,6 +16,7 @@ > >> > >> #include <linux/clk-provider.h> > >> #include <linux/of.h> > >> #include <linux/of_address.h> > >> > >> +#include <linux/syscore_ops.h> > >> > >> #include <dt-bindings/clock/rk3288-cru.h> > >> #include "clk.h" > >> > >> @@ -762,6 +763,64 @@ static const char *rk3288_critical_clocks[] > >> __initconst = {>> > >> "hclk_peri", > >> > >> }; > >> > >> +#ifdef CONFIG_PM_SLEEP > >> +static void __iomem *rk3288_cru_base; > >> +static const int rk3288_saved_cru_reg_ids[] = { > >> + RK3288_MODE_CON, > >> + RK3288_CLKSEL_CON(0), > >> + RK3288_CLKSEL_CON(1), > >> + RK3288_CLKSEL_CON(10), > >> + RK3288_CLKSEL_CON(33), > >> + RK3288_CLKSEL_CON(37), > >> +}; > > > > I still haven't had time to figure out exactly why these registers and > > not others... > > > > ...but I think that the code here looks reasonable and I have verified > > that saving at least some of these registers does matter. I'd vote to > > land these and if we have to add/remove registers later we can do it. > > Thus: > > > > Reviewed-by: Doug Anderson <dianders@chromium.org> > > Tested-by: Doug Anderson <dianders@chromium.org> > > These CLKSEL registers would be changed in maskrom. so we need save and > restore them. > > code in maskrom ? > ... > cruReg->CRU_CLKSEL_CON[0] = (0x9fff<<16) > > | (0x0<<15) // CORE select ARM PLL > | (0x0<<8) // core div > | (0x0<<4) // aclk_core_mp_div > | (0x0); // aclk_core_m0_dive > > cruReg->CRU_CLKSEL_CON[1] = (0xf3ff<<16) > > | (1<<15) // bus_aclk_pll_sel= GPLL > | (0x0<<12) // pd_bus_pclk_div > | (0x0<<8) // pd_bus_hclk_div=1:1 > | (0x0<<3) // pd_bus_aclk_div > | (0x0); // pd_bus_clk_div > > cruReg->CRU_CLKSEL_CON[10] = (((0x1<<15)|(0x3<<12)|(0x3<<8)|(0x1f))<<16) > > | (0x1<<15) // peri_pll_sel = GPLL > | (0x0<<12) // aclk_periph:pclk_periph = 1:1 > | (0x0<<8) // aclk_periph:hclk_periph = 1:1 > | (0x0); // GPLL:aclk_periph = 1:1 > > cruReg->CRU_CLKSEL_CON[33] = (((0x1f<<8)|(0x1f))<<16) > > | (0x0<<8) // alive_pclk > | (0x0); // pmu_pclk > > cruReg->CRU_CLKSEL_CON[37] = (((0x1f<<9)|(0x1f<<4)|(0x7))<<16) > > | (0<<9) //pclk_core_dbg > | (0x0<<4) // atclk_core > | (0x0); // clk_l2ram does the maskrom of rk3066a and rk3188 do similar things? No need to add it in the current series, but it would be nice if we had this list already anyway so I can make sure older and newer SoCs don't diverge for to long. Heiko
diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c index 2327829..94da06c 100644 --- a/drivers/clk/rockchip/clk-rk3288.c +++ b/drivers/clk/rockchip/clk-rk3288.c @@ -16,6 +16,7 @@ #include <linux/clk-provider.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/syscore_ops.h> #include <dt-bindings/clock/rk3288-cru.h> #include "clk.h" @@ -762,6 +763,64 @@ static const char *rk3288_critical_clocks[] __initconst = { "hclk_peri", }; +#ifdef CONFIG_PM_SLEEP +static void __iomem *rk3288_cru_base; +static const int rk3288_saved_cru_reg_ids[] = { + RK3288_MODE_CON, + RK3288_CLKSEL_CON(0), + RK3288_CLKSEL_CON(1), + RK3288_CLKSEL_CON(10), + RK3288_CLKSEL_CON(33), + RK3288_CLKSEL_CON(37), +}; + +static u32 rk3288_saved_cru_regs[ARRAY_SIZE(rk3288_saved_cru_reg_ids)]; + +/* + * Cru will be reset in maskrom when system wake up from fastboot. + * The apll/cpll/gpll will be set into slow mode in maskrom. + * So save them before suspend, restore them after resume. + */ +static int rk3288_clk_suspend(void) +{ + int i, reg_id; + + for (i = 0; i < ARRAY_SIZE(rk3288_saved_cru_reg_ids); i++) { + reg_id = rk3288_saved_cru_reg_ids[i]; + + rk3288_saved_cru_regs[i] = + readl_relaxed(rk3288_cru_base + reg_id); + } + return 0; +} + +static void rk3288_clk_resume(void) +{ + int i, reg_id; + + for (i = ARRAY_SIZE(rk3288_saved_cru_reg_ids) - 1; i >= 0; i--) { + reg_id = rk3288_saved_cru_reg_ids[i]; + + writel_relaxed(rk3288_saved_cru_regs[i] | 0xffff0000, + rk3288_cru_base + reg_id); + } +} + +static struct syscore_ops rk3288_clk_syscore_ops = { + .suspend = rk3288_clk_suspend, + .resume = rk3288_clk_resume, +}; + +static void rk3288_clk_sleep_init(void __iomem *reg_base) +{ + rk3288_cru_base = reg_base; + register_syscore_ops(&rk3288_clk_syscore_ops); +} + +#else /* CONFIG_PM_SLEEP */ +static void rk3288_clk_sleep_init(void __iomem *reg_base) {} +#endif + static void __init rk3288_clk_init(struct device_node *np) { void __iomem *reg_base; @@ -810,5 +869,6 @@ static void __init rk3288_clk_init(struct device_node *np) ROCKCHIP_SOFTRST_HIWORD_MASK); rockchip_register_restart_notifier(RK3288_GLB_SRST_FST); + rk3288_clk_sleep_init(reg_base); } CLK_OF_DECLARE(rk3288_cru, "rockchip,rk3288-cru", rk3288_clk_init);