[U-Boot] [PATCH 9/10] board/b4qds:Add support of 2 stage NAND boot-loader

[Scott Wood]

On Wed, 2014-04-02 at 09:29 +0530, Prabhakar Kushwaha wrote:
> On 4/2/2014 3:42 AM, Scott Wood wrote:
> > On Mon, 2014-03-31 at 15:35 +0530, Prabhakar Kushwaha wrote:
> >> +void board_init_f(ulong bootflag)
> >> +{
> >> +	u32 plat_ratio, sys_clk, uart_clk;
> >> +	ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR;
> >> +
> >> +	/* Memcpy existing GD at CONFIG_SPL_GD_ADDR */
> >> +	memcpy((void *)CONFIG_SPL_GD_ADDR, (void *)gd, sizeof(gd_t));
> >> +
> >> +	/* Update GD pointer */
> >> +	gd = (gd_t *)(CONFIG_SPL_GD_ADDR);
> >> +	__asm__ __volatile__("" : : : "memory");
> > Explain why this barrier is needed.
> >
> 
> I followed similar to what is present in arch/powerpc/lib/board.c.
> There is is mentioned like
>      /* compiler optimization barrier needed for GCC >= 3.4 */
>      __asm__ __volatile__("":::"memory");
> 
> may i mention this comment here also.

Yes, at least retain the comment when you copy the code.

> >> +	i2c_init_all();
> >> +
> >> +	puts("\n\n");
> >> +
> >> +	gd->ram_size = initdram(0);
> > What is the 0?
> 
> here  0 is board type.
> 
> in arch/powerpc/lib/board.c , we pass it as 0 "dummy arg".
> Same I am following here

Right, that comment was more for York -- why do we have this dummy
argument?

> >> + Run time view of SPL framework :-
> >> + -----------------------------------------------
> >> + Area        | Address                         |
> >> +-----------------------------------------------
> >> + Reserve     | 0xFFFC0000 (32KB)               |
> > s/Reserve/Reserved/
> >
> > What is it reserved for/by?  Something external?  Or does U-Boot just
> > not use that area currently?  In that case just say "Unused".
> Secure boot requires some memory in SRAM in order to put  some headers etc.
> May be I mention in README also

Secure boot is no longer running at this point, so why does it care?

Or do you mean that code in U-Boot uses this for validating the next
image (in which case, it's not reserved, but rather used for that
purpose)?

> >> +NAND Flash memory Map on B4860 and B4420QDS
> >> +------------------------------------------
> >> + Start		 End		Definition			Size
> >> +0x000000	0x0FFFFF	u-boot                          1MB
> >> +0x140000	0x15FFFF	u-boot env                      128KB
> >> +0x160000	0x17FFFF	FMAN Ucode                      128KB
> > What goes at 0x100000?
> 
> I think, ENV can start from here.
> 
> > What if 0x140000 or 0x160000 is a bad block?
> >
> >
> it can be true for any region. I believe it will be taken care by MTD 
> layer to read next good block.

No, it won't.  Imagine if it did, and 0x140000 were bad, and it used
0x160000 instead.  Then where would FMAN Ucode go?

> are you suggesting to have a hole of 1 block between every regions.

There are a variety of options for dealing with this for the
environment.  One of them is leaving a range of blocks (more than two --
consecutive blocks can be bad) within which block skipping can be done.
This can be fairly wasteful, especially if you'd extend it to other
things like microcode.

Another way is to put the environment in UBI, but that means you can't
load the environment during SPL or before relocation.

My suggestion would be to put non-environment things in UBI.  For the
environment, either use CONFIG_ENV_RANGE, or append the environment
(block-aligned) to the U-Boot image so it can participate in its bad
block skipping without having to reserve another large chunk.

Also consider having a redundant environment, IIRC this will need
additional work to function with any of the bad block handling
approaches.

-Scott