[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