On Thu, 2014-02-13 at 01:05 -0600, Tang Yuantian-B29983 wrote: > Thanks for your review. Please see the reply inline. > > Thanks, > Yuantian > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: 2014年2月13日 星期四 8:44 > > To: Tang Yuantian-B29983 > > Cc: Sun York-R58495; Wood Scott-B07421; Li Yang-Leo-R58472; > > t...@theia.denx.de; u-boot@lists.denx.de; Kushwaha Prabhakar-B32579; Jin > > Zhengxiong-R64188 > > Subject: Re: [U-Boot,2/3] mpc85xx: Add deep sleep framework support > > > > On Sun, Jan 26, 2014 at 02:00:40PM +0800, tang yuantian wrote: > > > From: Tang Yuantian <yuantian.t...@freescale.com> > > > > > > When system wakes up from warm reset, control is passed to the primary > > > core that starts executing uboot. After re-initialized some IP blocks, > > > like DDRC, kernel will take responsibility to continue to restore > > > environment it leaves before. > > > > > > Signed-off-by: Tang Yuantian <yuantian.t...@freescale.com> > > > > Is this for some specific mpc85xx chip (e.g. T1040)? I'm pretty sure > > this isn't necessary for deep sleep on mpc8536, for example. > > > Currently, it is used by t1040. But we want it to be more general so that > It can be used by later new chips.
But the mechanism is not the same for all mpc85xx derivatives. You'll need a more specific name. > > > +#ifdef CONFIG_DEEP_SLEEP > > > > CONFIG symbols need to be documented in README... > > > Where should I add this README? Under "85xx CPU Options" in the top-level README. > > > + /* disable the console if boot from warm reset */ > > > + if (in_be32(&gur->scrtsr[0]) & (1 << 3)) > > > + gd->flags |= > > > + GD_FLG_SILENT | GD_FLG_WARM_BOOT | > > GD_FLG_DISABLE_CONSOLE; #endif > > > > ...and it should probably be a more specific CONFIG_SYS symbol that > > indicates the presence of this guts bit. > > > > > diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c > > > b/arch/powerpc/cpu/mpc85xx/fdt.c index 33bc900..b3b9250 100644 > > > --- a/arch/powerpc/cpu/mpc85xx/fdt.c > > > +++ b/arch/powerpc/cpu/mpc85xx/fdt.c > > > @@ -24,6 +24,9 @@ DECLARE_GLOBAL_DATA_PTR; extern void > > > ft_qe_setup(void *blob); extern void ft_fixup_num_cores(void *blob); > > > extern void ft_srio_setup(void *blob); > > > +#ifdef CONFIG_DEEP_SLEEP > > > +extern ulong __bss_end; > > > +#endif > > > > Don't ifdef declarations. > > > > > #ifdef CONFIG_MP > > > #include "mp.h" > > > @@ -35,6 +38,9 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) > > > u32 bootpg = determine_mp_bootpg(NULL); > > > u32 id = get_my_id(); > > > const char *enable_method; > > > +#ifdef CONFIG_DEEP_SLEEP > > > + ulong len; > > > +#endif > > > > > > off = fdt_node_offset_by_prop_value(blob, -1, "device_type", "cpu", > > 4); > > > while (off != -FDT_ERR_NOTFOUND) { > > > @@ -77,6 +83,25 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) > > > "device_type", "cpu", 4); > > > } > > > > > > +#ifdef CONFIG_DEEP_SLEEP > > > + /* > > > + * reserve the memory space for warm reset. > > > + * This space will be re-used next time when boot from deep sleep. > > > + * The space includes bd_t, gd_t, stack and uboot image. > > > + * Reserve 1K for stack. > > > + */ > > > + len = sizeof(bd_t) + sizeof(gd_t) + (1 << 10); > > > + /* round up to 4K */ > > > + len = (len + (4096 - 1)) & ~(4096 - 1); > > > + > > > + off = fdt_add_mem_rsv(blob, gd->relocaddr - len, > > > + len + ((ulong)&__bss_end - gd->relocaddr)); > > > + if (off < 0) > > > + printf("Failed to reserve memory for warm reset: %s\n", > > > + fdt_strerror(off)); > > > + > > > +#endif > > > > Why do you need to reserve memory for this? We didn't need to for deep > > sleep on MPC8313ERDB, which also goes through U-Boot on wake. On > > MPC8313ERDB we transfer control to the kernel before relocating, so U- > > Boot never touches DDR. bd_t, gd_t, and the stack should be in locked > > L1 cache, and the u-boot image should be in flash (or locked CPC if this > > is not a NOR flash boot). > > > In deep sleep many IP blocks are powered off like DDRC, LIODN, cpu. These IPs > are re-initialized after relocating. > So, we must jump to kernel after relocating. The DDR controller is initialized before relocating -- and of course the CPU is powered off on MPC8313ERDB deep sleep as well. LIODNs are a new concern for deep sleep, but that doesn't mean we should go through a bunch of unrelated code to get there. Refactor the LIODN code to be usable before relocation, and not be tied to fdt fixups. > > > +#ifndef CONFIG_DEEP_SLEEP > > > /* Reserve spin table page */ > > > if (spin_tbl_addr < memory_limit) { > > > off = fdt_add_mem_rsv(blob, > > > @@ -108,6 +134,7 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) > > > printf("Failed to reserve memory for spin table: %s\n", > > > fdt_strerror(off)); > > > } > > > +#endif > > > > Explain. > > > Spin_tbl_addr has been reserved already. Where, and why is it different for non-CONFIG_DEEP_SLEEP? > > > #if (CONFIG_SYS_NUM_FMAN == 2) > > > - set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz); > > > - setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl, > > > - fman2_liodn_tbl_sz); > > > +#ifdef CONFIG_DEEP_SLEEP > > > + if ((gd->flags & GD_FLG_WARM_BOOT) == 0) { > > > + set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz); > > > + setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl, > > > + fman2_liodn_tbl_sz); > > > + } > > > +#endif > > > #endif > > > #endif > > > > Aren't you breaking non-CONFIG_DEEP_SLEEP here? > > > No, if deep sleep feature is enabled, we need to further judge whether this > boot is > Coming from deep sleep by GD_FLG_WARM_BOOT flag. My point is that if CONFIG_DEEP_SLEEP is not set, you will be skipping those calls to set_liodn() and setup_fman_liodn_base(). #ifdef foo if (!bar) stuff(); #endif is not equivalent to: #ifdef foo if (!bar) #endif stuff(); Though the latter is better written as something like: bool do_stuff = true; #ifdef foo if (bar) do_stuff = false; #endif if (do_stuff) stuff(); or static inline bool is_bar(void) { #ifdef foo return bar; #else return true; #endif } ... if (!is_bar()) stuff(); > > > diff --git a/arch/powerpc/include/asm/global_data.h > > > b/arch/powerpc/include/asm/global_data.h > > > index 8e59e8b..1ab05ff 100644 > > > --- a/arch/powerpc/include/asm/global_data.h > > > +++ b/arch/powerpc/include/asm/global_data.h > > > @@ -117,6 +117,7 @@ struct arch_global_data { #include > > > <asm-generic/global_data.h> > > > > > > #if 1 > > > +#define GD_FLG_WARM_BOOT 0x10000 > > > > Why is this not with the rest of the GD_FLGs, not numerically contiguous, > > and not commented? What specifically does "warm boot" mean? I think a > > lot of people would expect it to mean something that looks like a reset > > at the software level, while possibly not involving a real hardware reset > > -- coming out of deep sleep is the opposite of that. > > > Archdef document says it is a warm reset boot. So I name it this way. Hardware people sometimes use terminology differently than software people. > Other platforms use the same flag, like x86, although I am not sure > Whether it is for deep sleep. > If you have better name, I love to use it. It's not yet clear to me that a GD flag is needed for this. > > > #define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm > > ("r2") > > > #else /* We could use plain global data, but the resulting code is > > bigger */ > > > #define XTRN_DECLARE_GLOBAL_DATA_PTR extern > > > diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c index > > > 34bbfca..7383e32 100644 > > > --- a/arch/powerpc/lib/board.c > > > +++ b/arch/powerpc/lib/board.c > > > @@ -350,6 +350,13 @@ unsigned long logbuffer_base(void) } #endif > > > > > > +#ifdef CONFIG_DEEP_SLEEP > > > +int check_warmboot(void) > > > +{ > > > + return !!(gd->flags & GD_FLG_WARM_BOOT); } #endif > > > > Why? > > > Why what? Why we need it? > > It is a help function and used by ASM code in which > we can't determine whether it is a warm reset boot. Why don't you just open code it? > > Again, you seem to be breaking the non-CONFIG_DEEP_SLEEP case. > > > No. Yes. :-) > > > +#ifdef CONFIG_DEEP_SLEEP > > > + /* > > > + * restore 128-byte memory space which corrupted > > > + * by DDRC training. > > > + */ > > > + if (gd->flags & GD_FLG_WARM_BOOT) { > > > + src = (u64 *)in_be32(&scfg->sparecr[2]); > > > + dst = (u64 *)(0); > > > + for (i = 0; i < 128/sizeof(u64); i++) { > > > + *dst = *src; > > > + dst++; > > > + src++; > > > + } > > > + } > > > > (u64 *)(0) is a NULL pointer. Dereferencing NULL pointers is undefined > > and GCC has been getting increasingly free with making surprising > > optimizations based on that (such as assuming any code path that it knows > > can reach a NULL dereference is dead code and can be removed). > > > Then how we operate 0 address if not dereferencing NULL pointer? > With an I/O accessor (or other inline asm), a TLB mapping, or using a different memory location. -Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot