diff mbox

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

Message ID 20160111125809.GA28300@localhost.localdomain
State Superseded
Headers show

Commit Message

Ladislav Michl Jan. 11, 2016, 12:58 p.m. UTC
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 :)

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
and not by name, it is bringing some inconsistencies with for example ubi
environment code, which is using volume names. Is it worth fixing?
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 ;-)

commit f68d017a4d35bfac3cccd7c7f19ab1c2fe76d908
Author: Ladislav Michl <ladis@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@linux-mips.org>

Comments

Heiko Schocher Jan. 12, 2016, 9:08 a.m. UTC | #1
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@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@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@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
diff mbox

Patch

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@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;
+	info.peb_count = CONFIG_SPL_UBI_MAX_PEBS - info.peb_offset;
+
+#ifdef CONFIG_SPL_OS_BOOT
+	if (!spl_start_uboot()) {
+		volumes[0].name = "kernel";
+		volumes[0].vol_id = CONFIG_SPL_UBI_LOAD_KERNEL_ID;
+		volumes[0].load_addr = (void *)CONFIG_SYS_LOAD_ADDR;
+		volumes[1].name = "args";
+		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);