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

Heiko Schocher hs at denx.de
Wed Jan 13 06:09:01 CET 2016


Hello Ladislav,

Am 12.01.2016 um 23:17 schrieb Ladislav Michl:
> Hi Heiko,
>
> On Tue, Jan 12, 2016 at 10:08:21AM +0100, Heiko Schocher wrote:
>> Am 11.01.2016 um 13:58 schrieb Ladislav Michl:
>>> On Mon, Jan 11, 2016 at 07:20:06AM +0100, Heiko Schocher wrote:
>>>> 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"
>
> Hmm, I do not agree with warning as I consider code pretty readable :)

Its a warning only ...
Yes, I am also unhappy to adding blanks in such cases ... maybe this
should be discussed in a seperate thread?

> Anyway, there is similar "unaligned case" few lines bellow. Perhaps send
> additional patch to fix them both?

If you fix it, then please in a v4 patch, thanks!

>>> 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.
>
> It seems Thomas had a good reason to use volume ids, see ubi_scan_vid_hdr
> called from ipl_scan.

Yes indeed ... so let it as it is.

>>> 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 ...
>
> Okay, I need to be able to load also bare zImage. This change is independent
> and possibly controversial, so I'll put it here for disscusion :)

Please post it as a RFC patch seperate, with for example Marek in Cc,
so he can speak up here too.

>
> commit c092b3c0627dd8d4b3f3d756c58b53fcf205587f
> Author: Ladislav Michl <ladis at linux-mips.org>
> Date:   Tue Jan 12 16:37:04 2016 +0100
>
>      spl: zImage support in Falcon mode
>
>      Other payload than uImage is currently considered to be raw U-Boot
>      image. Check also for zImage in Falcon mode.
>
>      Signed-off-by: Ladislav Michl <ladis at linux-mips.org>
>
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index f3db7b5..07a9019 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -26,11 +26,13 @@ endif
>   obj-$(CONFIG_CPU_V7M) += cmd_boot.o
>   obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o
>   obj-$(CONFIG_CMD_BOOTM) += bootm.o
> +obj-$(CONFIG_CMD_BOOTM) += zimage.o
>   obj-$(CONFIG_SYS_L2_PL310) += cache-pl310.o
>   obj-$(CONFIG_USE_ARCH_MEMSET) += memset.o
>   obj-$(CONFIG_USE_ARCH_MEMCPY) += memcpy.o
>   else
>   obj-$(CONFIG_SPL_FRAMEWORK) += spl.o
> +obj-$(CONFIG_SPL_FRAMEWORK) += zimage.o
>   endif
>   obj-$(CONFIG_SEMIHOSTING) += semihosting.o
>
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index a477cae..fbfc0ad 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -348,38 +348,6 @@ int do_bootm_linux(int flag, int argc, char * const argv[],
>   	return 0;
>   }
>
> -#ifdef CONFIG_CMD_BOOTZ
> -
> -struct zimage_header {
> -	uint32_t	code[9];
> -	uint32_t	zi_magic;
> -	uint32_t	zi_start;
> -	uint32_t	zi_end;
> -};
> -
> -#define	LINUX_ARM_ZIMAGE_MAGIC	0x016f2818
> -
> -int bootz_setup(ulong image, ulong *start, ulong *end)
> -{
> -	struct zimage_header *zi;
> -
> -	zi = (struct zimage_header *)map_sysmem(image, 0);
> -	if (zi->zi_magic != LINUX_ARM_ZIMAGE_MAGIC) {
> -		puts("Bad Linux ARM zImage magic!\n");
> -		return 1;
> -	}
> -
> -	*start = zi->zi_start;
> -	*end = zi->zi_end;
> -
> -	printf("Kernel image @ %#08lx [ %#08lx - %#08lx ]\n", image, *start,
> -	      *end);
> -
> -	return 0;
> -}
> -
> -#endif	/* CONFIG_CMD_BOOTZ */
> -
>   #if defined(CONFIG_BOOTM_VXWORKS)
>   void boot_prep_vxworks(bootm_headers_t *images)
>   {
> diff --git a/arch/arm/lib/zimage.c b/arch/arm/lib/zimage.c
> new file mode 100644
> index 0000000..f870d72
> --- /dev/null
> +++ b/arch/arm/lib/zimage.c
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright (C) 2016
> + * Ladislav Michl <ladis at linux-mips.org>
> + *
> + * bootz code:
> + * Copyright (C) 2012 Marek Vasut <marek.vasut at gmail.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +#include <common.h>
> +
> +#define	LINUX_ARM_ZIMAGE_MAGIC	0x016f2818
> +
> +struct arm_z_header {
> +	uint32_t	code[9];
> +	uint32_t	zi_magic;
> +	uint32_t	zi_start;
> +	uint32_t	zi_end;
> +} __attribute__ ((__packed__));
> +
> +int bootz_setup(ulong image, ulong *start, ulong *end)
> +{
> +	struct arm_z_header *zi = (struct arm_z_header *) image;
> +
> +	if (zi->zi_magic != LINUX_ARM_ZIMAGE_MAGIC) {
> +#ifndef CONFIG_SPL_FRAMEWORK
> +		puts("Bad Linux ARM zImage magic!\n");
> +#endif
> +		return 1;
> +	}
> +
> +	*start = zi->zi_start;
> +	*end = zi->zi_end;
> +#ifndef CONFIG_SPL_FRAMEWORK
> +	printf("Kernel image @ %#08lx [ %#08lx - %#08lx ]\n", image, *start,
> +	      *end);
> +#endif
> +
> +	return 0;
> +}
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 7665105..342c3b8 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -52,6 +52,15 @@ __weak int spl_start_uboot(void)
>   	puts("SPL: Direct Linux boot not active!\n");
>   	return 1;
>   }
> +
> +/*
> + * Weak default function for arch specific zImage check. Return zero
> + * and fill start and end address if image is recognized.
> + */
> +int __weak bootz_setup(ulong image, ulong *start, ulong *end)
> +{
> +	 return 1;
> +}
>   #endif
>
>   /*
> @@ -112,6 +121,20 @@ void spl_parse_image_header(const struct image_header *header)
>   		 */
>   		panic("** no mkimage signature but raw image not supported");
>   #else
> +#ifdef CONFIG_SPL_OS_BOOT
> +		ulong start, end;
> +
> +		if (!bootz_setup((ulong)header, &start, &end)) {
> +			spl_image.name = "Linux";
> +			spl_image.os = IH_OS_LINUX;
> +			spl_image.load_addr = (u32)header;
> +			spl_image.entry_point = (u32)header;
> +			spl_image.size = end - start;
> +			debug("spl: payload zImage, load addr: 0x%x size: %d\n",
> +				spl_image.load_addr, spl_image.size);
> +			return;
> +		}
> +#endif
>   		/* Signature not found - assume u-boot.bin */
>   		debug("mkimage signature not found - ih_magic = %x\n",
>   			header->ih_magic);
>
>>> 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!
>
> Sure and it is done in next version.

Thanks!

>>> +	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" ...
>
> It is not needed here to make them configurable, because current code
> loads volumes by id not by name. Current code loads volumes by id for
> a sake of simplicity. Perhaps we could drop name field as it is used
> only for pretty print?

Ah, correct ... I should try your patches on a board ... also it is
in board specific code, so Ok for me. Just if it is only for pretty
print .. drop it and save some bytes?

So I wait for your v4 round, thanks for your work!

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


More information about the U-Boot mailing list