[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