[U-Boot] [U-Boot, 2/3] mpc85xx: Add deep sleep framework support
Scott Wood
scottwood at freescale.com
Fri Feb 14 23:21:25 CET 2014
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;
> > Tang at theia.denx.de; u-boot at 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.tang at 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.Tang at 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
More information about the U-Boot
mailing list