[U-Boot] [PATCHv3 4/4] igep00x0: UBIize

[Heiko Schocher]

Hello Ladislav,

Am 11.01.2016 um 13:58 schrieb Ladislav Michl:
> Hi Heiko,
>
> On Mon, Jan 11, 2016 at 07:20:06AM +0100, Heiko Schocher wrote:
> [...]
>> Could you seperate common changes in "common/*" and your special
>> board changes?
>
> It is done bellow, just common/* part to start discussion...

;-)

>> Beside of that, this patch does not apply ...
>
> Ah, igep00x0 part is based on top of this:
> http://lists.denx.de/pipermail/u-boot/2016-January/240013.html
> I silently hoped to be applied for 2016.01 release, but never mind :)

;-)

Ah, I added them to my automated build, and now it works again :-D

BTW: patch "[U-Boot,PATCHv2,2/5] igep00x0: Cleanup ethernet support"
has a checkpatch warning, search in

http://xeidos.ddns.net/buildbot/builders/smartweb_dfu/builds/43/steps/shell/logs/tbotlog

for "2016-01-12 07:47:08,369"

> Now assumption is that once board switches to UBI, loading u-boot or kernel
> from bare flash does not make a sense anymore, so with CONFIG_SPL_UBI
> all that code in spl_nor.c (reevaluate?!), spl_nand.c and spl_onenand.c
> is not used in favour of spl_ubi.c. As ubispl can load volumes by volume id

I thought about this change too, and I think your assumption is OK
here. Let us bring in this change, and if someone has other needs,
we have to look again at this place .. but I think, if switching to
use UBI, than it makes no sense to read in raw mode ...

Other opinions?

> and not by name, it is bringing some inconsistencies with for example ubi
> environment code, which is using volume names. Is it worth fixing?

It would be nice to have ... yes, if it is easy to do? Also we must
have a look at the codesize.

> All that ubispl_info structure is board specific and there is not much left
> besides initializing it. Also volumes can differ per board basis, so
> providing common function is somewhat questionable. However here it is,
> just to show how does it look like. Suggestions are very welcome as silence
> around this part of patch is a bit suspicious ;-)

Questions are coming if there are users ;-)

I vote for bringing this in, and we will see, where we have to make
things more configurable ... some nitpicks below ...

> commit f68d017a4d35bfac3cccd7c7f19ab1c2fe76d908
> Author: Ladislav Michl <ladis at linux-mips.org>
> Date:   Mon Jan 11 13:08:10 2016 +0100
>
>      spl: support loading from UBI volumes
>
>      Add simple support for loading from UBI volumes.
>      This is just a test and needs to be made configurable
>
>      Signed-off-by: Ladislav Michl <ladis at linux-mips.org>
>
> diff --git a/common/spl/Makefile b/common/spl/Makefile
> index 10a4589..e4535c4 100644
> --- a/common/spl/Makefile
> +++ b/common/spl/Makefile
> @@ -10,10 +10,13 @@
>
>   ifdef CONFIG_SPL_BUILD
>   obj-$(CONFIG_SPL_FRAMEWORK) += spl.o
> -obj-$(CONFIG_SPL_NOR_SUPPORT) += spl_nor.o
>   obj-$(CONFIG_SPL_YMODEM_SUPPORT) += spl_ymodem.o
> +ifndef CONFIG_SPL_UBI
> +obj-$(CONFIG_SPL_NOR_SUPPORT) += spl_nor.o
>   obj-$(CONFIG_SPL_NAND_SUPPORT) += spl_nand.o
>   obj-$(CONFIG_SPL_ONENAND_SUPPORT) += spl_onenand.o
> +endif
> +obj-$(CONFIG_SPL_UBI) += spl_ubi.o
>   obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
>   obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
>   obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 6e6dee7..048a325 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -286,6 +286,18 @@ static int spl_load_image(u32 boot_device)
>   	case BOOT_DEVICE_MMC2_2:
>   		return spl_mmc_load_image(boot_device);
>   #endif
> +#ifdef CONFIG_SPL_UBI
> +#ifdef CONFIG_SPL_NAND_SUPPORT
> +	case BOOT_DEVICE_NAND:
> +#endif
> +#ifdef CONFIG_SPL_ONENAND_SUPPORT
> +	case BOOT_DEVICE_ONENAND:
> +#endif
> +#ifdef CONFIG_SPL_NOR_SUPPORT
> +	case BOOT_DEVICE_NOR:
> +#endif
> +		return spl_ubi_load_image(boot_device);
> +#else
>   #ifdef CONFIG_SPL_NAND_SUPPORT
>   	case BOOT_DEVICE_NAND:
>   		return spl_nand_load_image();
> @@ -298,6 +310,7 @@ static int spl_load_image(u32 boot_device)
>   	case BOOT_DEVICE_NOR:
>   		return spl_nor_load_image();
>   #endif
> +#endif /* CONFIG_SPL_UBI */
>   #ifdef CONFIG_SPL_YMODEM_SUPPORT
>   	case BOOT_DEVICE_UART:
>   		return spl_ymodem_load_image();
> diff --git a/common/spl/spl_ubi.c b/common/spl/spl_ubi.c
> new file mode 100644
> index 0000000..38ddb57
> --- /dev/null
> +++ b/common/spl/spl_ubi.c
> @@ -0,0 +1,73 @@
> +/*
> + * Copyright (C) 2016
> + * Ladislav Michl <ladis at linux-mips.org>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <config.h>
> +#include <nand.h>
> +#include <ubispl.h>
> +#include <spl.h>
> +
> +int spl_ubi_load_image(u32 boot_device)
> +{
> +	int ret;
> +	struct image_header *header;
> +	struct ubispl_info info;
> +	struct ubispl_load volumes[2];
> +
> +#ifdef CONFIG_SPL_NAND_SUPPORT
> +	if (boot_device == BOOT_DEVICE_NAND)
> +		nand_init();
> +#endif
> +
> +	/* TODO: Make it decently configurable */
> +	info.ubi = (struct ubi_scan_info *)
> +		(CONFIG_SYS_SPL_MALLOC_START + CONFIG_SYS_SPL_MALLOC_SIZE);
> +	info.fastmap = 1;
> +	info.read = nand_spl_read_block;
> +
> +	info.peb_offset = 4;
> +	info.peb_size = CONFIG_SYS_NAND_BLOCK_SIZE;
> +	info.vid_offset = 512;
> +	info.leb_start = 2048;

this three values should be configurable!

> +	info.peb_count = CONFIG_SPL_UBI_MAX_PEBS - info.peb_offset;
> +
> +#ifdef CONFIG_SPL_OS_BOOT
> +	if (!spl_start_uboot()) {
> +		volumes[0].name = "kernel";

Also we should have the names configurable ... not for all boards
the kernel is stored in "kernel" ...

> +		volumes[0].vol_id = CONFIG_SPL_UBI_LOAD_KERNEL_ID;
> +		volumes[0].load_addr = (void *)CONFIG_SYS_LOAD_ADDR;
> +		volumes[1].name = "args";

Same here.

> +		volumes[1].vol_id = CONFIG_SPL_UBI_LOAD_ARGS_ID;
> +		volumes[1].load_addr = (void *)CONFIG_SYS_SPL_ARGS_ADDR;
> +
> +		ret = ubispl_load_volumes(&info, volumes, 2);
> +		if (!ret) {
> +			header = (struct image_header *) volumes[0].load_addr;
> +			spl_parse_image_header(header);
> +			puts("Linux loaded.\n");
> +			return 0;
> +		}
> +		puts("Loading Linux failed, falling back to U-Boot.\n");
> +	}
> +#endif
> +	header = (struct image_header *)
> +		(CONFIG_SYS_TEXT_BASE - sizeof(struct image_header));
> +	volumes[0].name = "monitor";
> +	volumes[0].vol_id = CONFIG_SPL_UBI_LOAD_MONITOR_ID;
> +	volumes[0].load_addr = (void *)header;
> +
> +	ret = ubispl_load_volumes(&info, volumes, 1);
> +	if (!ret)
> +		spl_parse_image_header(header);
> +
> +#ifdef CONFIG_SPL_NAND_SUPPORT
> +	if (boot_device == BOOT_DEVICE_NAND)
> +		nand_deselect();
> +#endif
> +
> +	return ret;
> +}
> diff --git a/include/spl.h b/include/spl.h
> index 92cdc04..1ab9295 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -40,6 +40,7 @@ u32 spl_boot_mode(void);
>   void spl_set_header_raw_uboot(void);
>   void spl_parse_image_header(const struct image_header *header);
>   void spl_board_prepare_for_linux(void);
> +int spl_board_ubi_load_image(u32 boot_device);
>   void __noreturn jump_to_image_linux(void *arg);
>   int spl_start_uboot(void);
>   void spl_display_print(void);
> @@ -53,6 +54,9 @@ int spl_onenand_load_image(void);
>   /* NOR SPL functions */
>   int spl_nor_load_image(void);
>
> +/* UBI SPL functions */
> +int spl_ubi_load_image(u32 boot_device);
> +
>   /* MMC SPL functions */
>   int spl_mmc_load_image(u32 boot_device);
>
>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany