[U-Boot-Users] [PATCH] Add Flat OF tree support.

[Wolfgang Denk]

In message <200509021814.08001.pantelis.antoniou at gmail.com> you wrote:
>
> As you well know, as part of the ongoing cleanup of the linux trees
> regarding PPC, it has been decreed that the preferred way to pass 
> bootloader information shall be the flat OF tree.

Hear, hear!

> The following patch (broken in two parts), implements just that.

No, it does not. It does more than this on one hand,  and  less  than
reqired on the other.

Please clean up and resubmit:


> diff --git a/common/Makefile b/common/Makefile
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -51,7 +51,7 @@ COBJS	= main.o ACEX1K.o altera.o bedbug.
>  	  memsize.o miiphybb.o miiphyutil.o \
>  	  s_record.o serial.o soft_i2c.o soft_spi.o spartan2.o \
>  	  usb.o usb_kbd.o usb_storage.o \
> -	  virtex2.o xilinx.o
> +	  virtex2.o xilinx.o ft_build.o

Lists shall be kept sorted. Don't be lazy and just append at the end,
please.


> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
...
> +	/*
> +	 * Linux Kernel Parameters:
> +	 *   r3: ptr to board info data
> +	 *   r4: initrd_start or 0 if no initrd
> +	 *   r5: initrd_end - unused if r4 is 0
> +	 *   r6: Start of command line string
> +	 *   r7: End   of command line string
> +	 */
> +	(*kernel) ((bd_t *)of_flat_tree, initrd_start, initrd_end, cmd_start, cmd_end);

Is it board info data or an OF tree? I think the comment needs to  be
adjusted to match the code.


> diff --git a/common/ft_build.c b/common/ft_build.c
> new file mode 100644
> --- /dev/null
> +++ b/common/ft_build.c
...
> +	/* paste the bd_t at the end of the flat tree */
> +	end = (char *)blob +
> +	    be32_to_cpu(((struct boot_param_header *)blob)->totalsize);
> +	memcpy(end, bd, sizeof(*bd));
> +
> +#ifdef __powerpc__

I understand that *all* of this is PPC specific, so why do you #ifdef
just this part? IMHO all of this should  disappear  for  all  non-PPC
versions?

> +	p = ft_get_prop(blob, "/u-boot-bd_t/memstart", &len);
...
> +	p = ft_get_prop(blob, "/u-boot-bd_t/memsize", &len);
...
> +	p = ft_get_prop(blob, "/u-boot-bd_t/flashstart", &len);
...
> +	p = ft_get_prop(blob, "/u-boot-bd_t/flashsize", &len);
...
> +	p = ft_get_prop(blob, "/u-boot-bd_t/flashoffset", &len);
...
> +	p = ft_get_prop(blob, "/u-boot-bd_t/sramstart", &len);
...
> +	p = ft_get_prop(blob, "/u-boot-bd_t/sramsize", &len);
...

That's alot or repeated string constants, which just waste memory  in
the data segment. Any way for a leaner implementation?

While we are at it: did you check how much this  patch  adds  to  the
memory footprint?

> +#if defined(CONFIG_MPC5xxx)
> +	p = ft_get_prop(blob, "/u-boot-bd_t/mbar_base", &len);
> +	if (p != NULL)
> +		*p = cpu_to_be32(bd->bi_mbar_base);
> +#endif
...
> +#if defined(CONFIG_MPC5xxx)
> +
> +	p = ft_get_prop(blob, "/u-boot-bd_t/ipbfreq", &len);
> +	if (p != NULL)
> +		*p = cpu_to_be32(bd->bi_ipbfreq);
> +
> +	p = ft_get_prop(blob, "/u-boot-bd_t/pcifreq", &len);
> +	if (p != NULL)
> +		*p = cpu_to_be32(bd->bi_pcifreq);
> +#endif

Maybe you can sort these things to minimize the  groups  of  #ifdef's
needed?

> +#if defined(CONFIG_8xx)
> +	clock = gd->cpu_clk;
> +#elif defined(CONFIG_8260)
> +	clock = gd->brg_clk;
> +#else
> +	clock = 0;
> +#endif
> +	p = ft_get_prop(blob, "/cpus/" OF_CPU "/clock-frequency", &len);
> +	if (p != NULL)
> +		*p = cpu_to_be32(clock);

clock = 0 for anything but 8xx and 8260???


> +	p = ft_get_prop(blob, "/cpus/" OF_CPU "/timebase-frequency", &len);
> +	if (p != NULL)
> +		*p = cpu_to_be32(clock / 4);	/* XXX I'm just guessing */

... and probably wrong?



There is no CHANGELOG entry.

Rejected.



> Content-Type: text/plain;
>   charset="us-ascii";
>   name="of-uboot-stxxtc.patch"
> Content-Transfer-Encoding: 7bit
> Content-Disposition: inline;
> 	filename="of-uboot-stxxtc.patch"

There is no CHANGELOG entry.

Rejected.


Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You'll learn something  about  men  and  women  --  the  way  they're
supposed  to  be. Caring for each other, being happy with each other,
being good to each other. That's what we call love. You'll like  that
a lot.
	-- Kirk, "The Apple", stardate 3715.6