[U-Boot] [PATCH 4/4] PXA: Adapt Voipac PXA270 to OneNAND SPL

[Scott Wood]

On 10/31/2011 08:23 AM, Marek Vasut wrote:
> Signed-off-by: Marek Vasut <marek.vasut at gmail.com>
> Cc: Albert ARIBAUD <albert.u.boot at aribaud.net>
> ---
>  board/vpac270/Makefile    |    6 ++
>  board/vpac270/onenand.c   |  136 +++++++++++++++++++++++++++++++++++++++++++++
>  board/vpac270/vpac270.c   |    2 +
>  include/configs/vpac270.h |   25 +++++++--
>  4 files changed, 164 insertions(+), 5 deletions(-)
>  create mode 100644 board/vpac270/onenand.c
> 
> diff --git a/board/vpac270/Makefile b/board/vpac270/Makefile
> index b5c60fd..f25822f 100644
> --- a/board/vpac270/Makefile
> +++ b/board/vpac270/Makefile
> @@ -23,7 +23,13 @@ include $(TOPDIR)/config.mk
>  
>  LIB	= $(obj)lib$(BOARD).o
>  
> +ifndef	CONFIG_SPL_BUILD
>  COBJS	:= vpac270.o
> +endif
> +
> +ifdef	CONFIG_SPL_BUILD
> +COBJS	:= onenand.o
> +endif

else?

>  SRCS	:= $(COBJS:.o=.c)
>  OBJS	:= $(addprefix $(obj),$(COBJS))
> diff --git a/board/vpac270/onenand.c b/board/vpac270/onenand.c
> new file mode 100644
> index 0000000..50de2ab
> --- /dev/null
> +++ b/board/vpac270/onenand.c
> @@ -0,0 +1,136 @@
> +/*
> + * Voipac PXA270 OneNAND SPL
> + *
> + * Copyright (C) 2011 Marek Vasut <marek.vasut at gmail.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <config.h>
> +#include <asm/io.h>
> +#include <onenand_uboot.h>
> +
> +extern void pxa_dram_init(void);
> +
> +inline void spl_copy_self(void)
> +{
> +	extern uint32_t _end;
> +	struct spl_onenand_data data;
> +	uint32_t page;
> +	uint32_t total_bytes = (uint32_t)&_end - CONFIG_SPL_TEXT_BASE;
> +	uint32_t total_pages;
> +	uint8_t *addr = (uint8_t *)CONFIG_SPL_TEXT_BASE;
> +	int ret;
> +
> +	spl_onenand_get_geometry(&data);
> +
> +	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> +	total_pages = total_bytes >> 11;
> +	if (data.pagesize == 4096)
> +		total_pages >>= 1;

total_bytes / 2048 and total_pages / 2 are more readable and should
generate exactly the same code.

> +	for (page = 0; page <= total_pages; page++) {
> +		ret = spl_onenand_read_page(0, page, addr, data.pagesize);
> +		if (ret)
> +			total_pages++;
> +		else
> +			addr += data.pagesize;
> +	}
> +}

You want to skip to the next block if spl_onenand_read_page() fails
(which can occur after you've already read some of the block).

How much of this is board-specific?

> +inline void spl_copy_uboot(void)
> +{
> +	uint8_t *addr = (uint8_t *)CONFIG_SYS_TEXT_BASE;
> +	struct spl_onenand_data data;
> +	uint32_t total_pages;
> +	uint32_t block;
> +	uint32_t page, rpage;
> +	int ret;
> +
> +	spl_onenand_get_geometry(&data);
> +
> +	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> +	total_pages = CONFIG_SPL_ONENAND_LOAD_SIZE >> 11;
> +	page = CONFIG_SPL_ONENAND_LOAD_ADDR >> 11;
> +	if (data.pagesize == 4096) {
> +		total_pages >>= 1;
> +		page >>= 1;
> +	}
> +
> +	for (; page <= total_pages; page++) {
> +		block = page >> 6;
> +		rpage = page & 0xff;
> +		ret = spl_onenand_read_page(block, rpage, addr, data.pagesize);
> +		if (ret)
> +			total_pages++;
> +		else
> +			addr += data.pagesize;
> +	}
> +}

What is so different about this compared to spl_copy_self, that warrants
such duplication?  Can't you just pass in offset, length, and
destination as parameters?  Or just have the OneNAND SPL driver export
nand_spl_load_image(), as any other NAND SPL driver would?

> +inline void board_init_f(unsigned long unused)
> +{
> +	uint32_t tmp;
> +
> +	asm volatile("mov %0, pc" : "=r"(tmp));
> +	tmp >>= 24;
> +
> +	/* The code runs from OneNAND RAM, copy SPL to SRAM and execute it. */
> +	if (tmp == 0) {
> +		spl_copy_self();
> +		asm volatile("mov pc, %0" : : "r"(CONFIG_SPL_TEXT_BASE));
> +	}

Is it not possible to use a simple memcpy for spl_copy_self()?  If the
CPU can run the code, you'd think it could read it.

> +inline void board_init_r(gd_t *id, ulong dest_addr)
> +{
> +	for (;;)
> +		;
> +}

This doesn't seem like a useful board_init_r().  If you don't need it,
maybe make sure it's not called, and save yourself some bytes in the
SPL.  Likewise for the other stub functions, where practical.

> +inline int printf(const char *fmt, ...)
> +{
> +	return 0;
> +}
> +
> +inline void __coloured_LED_init(void) {}
> +inline void __red_LED_on(void) {}
> +void coloured_LED_init(void)
> +	__attribute__((weak, alias("__coloured_LED_init")));
> +void red_LED_on(void)
> +	__attribute__((weak, alias("__red_LED_on")));
> +void hang(void) __attribute__ ((noreturn));
> +void hang(void)
> +{
> +	for (;;)
> +		;
> +}
> +
> +inline void icache_disable(void) {}
> +inline void dcache_disable(void) {}

Why are you specifying inline on just about everything, even functions
that are not used in this file?

Why are you not specifying static on things that are not needed outside
this file?

> diff --git a/board/vpac270/vpac270.c b/board/vpac270/vpac270.c
> index 43bbdff..f146009 100644
> --- a/board/vpac270/vpac270.c
> +++ b/board/vpac270/vpac270.c
> @@ -56,7 +56,9 @@ struct serial_device *default_serial_console(void)
>  extern void pxa_dram_init(void);
>  int dram_init(void)
>  {
> +#ifndef	CONFIG_ONENAND
>  	pxa_dram_init();
> +#endif
>  	gd->ram_size = PHYS_SDRAM_1_SIZE;
>  	return 0;
>  }

Should this really be about whether OneNAND support is present, or
should it be based on whether you're using the OneNAND SPL?

-Scott