[U-Boot] [U-Boot, 2/3] mpc85xx: Add deep sleep framework support
Scott Wood
scottwood at freescale.com
Mon Feb 24 20:11:05 CET 2014
On Mon, 2014-02-24 at 15:47 +0800, Tang Yuantian-B29983 wrote:
> On 2014/2/15 星期六 6:21, Scott Wood wrote:
> > 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.
> OK, will name it as t104x
>
> >>>> +#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.
> Thanks.
>
> >>>> + /* 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.
> where should I put this CONFIG_SYS_'s definition?
Under "85xx CPU Options" in the top-level README. :-)
> >>>> 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.
> There are other IP blocks that need to be re-initialized, like SerDes,
> SMP, PCIe and
> a lot of Errata. I don't want to refactor one by one.
> Besides, coding in this way will not change the current execute flow.
Changing the execution flow is better than adding a bunch of special
cases to the current execution flow.
Some of that reinitialization (e.g. PCIe) should be handled by Linux,
not U-Boot.
> >>>> +#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?
> right above:
>
> + /*
> + * 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.
> + */
>
> If deep sleep is supported, the sbin_table space will be reserved above.
> we don't need to reserved it anymore.
So the same memory is used for the spin table as for the deep sleep
stuff? There's no conflict there?
> >>>> #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();
> You are absolutely right. How didn't I find this?
By not testing with CONFIG_DEEP_SLEEP off? :-)
> >>>> 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.
> How about warm_reset?
"warm reset" and "warm boot" mean pretty much the same thing to me. I'd
stick with "deep sleep" terminology.
> >> 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.
> GD_ flag can be added for our own purpose. We decide to use a GD flag to
> indicate the deep sleep case and I think it worth it.
It depends on how far into the boot sequence we go.
> >>>> #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?
> I can't check the warmboot status in ASM code.
> In order to get the warmboot status in ASM code, I wrote this function.
Why can't you check it in asm code? See lib/asm-offsets.c.
> >>>> + 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.
> we found the zero address has benefit.
> I don't know how to achieve this in inline asm or TLB mapping, could you
> be more specific or write a example for me?
Inline asm would be something like:
asm("stw %1, 0(%0); stw %2, 4(%0)" : "=r" (dst) :
"r" ((u32)(src >> 32)), "r" ((u32)src));
-Scott
More information about the U-Boot
mailing list