[PATCH 0/2] MIPS: keep firmwmare arguments with appended dtb

[PATCH 0/2] MIPS: keep firmwmare arguments with appended dtb

[Jonas Gorski]

The following two patches modify the appended dtb and UHI detection in a
way to allow keeping all four firmware arguments.

Patch one changes the handling of ZBOOT appended dtbs by copying the
appended dtb to the place where the extracted kernel will expect it.
This takes advantage of the fact that the compression Makefile recipe
will always append the uncompressed kernel size to the compressed
kernel if not done by the compression format itself.
This has the nice side effect that we don't need to special case ZBOOT
appended dtb anymore.

Patch two then introduces a new global variable for a UHI passed dtb
address, and stores the appended dtb's address in there if support is
enabled, or the UHI passed address if the arguments match the UHI
interface. It will only do a FDT MAGIC sanity check for appended dtbs,
and not for UHI passed ones, since just because a0 is -2, doesn't mean
a1 will contain a valid address.

Both have been run tested on BMIPS on BCM9EJTAGPRB.

Jonas Gorski (2):
  MIPS: ZBOOT: copy appended dtb to the end of the kernel
  MIPS: store the appended dtb address in a variable

 arch/mips/Kconfig                      | 22 ++--------------------
 arch/mips/ath79/setup.c                |  4 ++--
 arch/mips/bmips/setup.c                |  4 ++--
 arch/mips/boot/compressed/Makefile     |  1 +
 arch/mips/boot/compressed/decompress.c | 21 +++++++++++++++++++++
 arch/mips/boot/compressed/head.S       | 16 ----------------
 arch/mips/include/asm/bootinfo.h       |  4 ++++
 arch/mips/kernel/head.S                | 21 ++++++++++++++-------
 arch/mips/kernel/setup.c               |  4 ++++
 arch/mips/lantiq/prom.c                |  4 ++--
 arch/mips/pic32/pic32mzda/init.c       |  4 ++--
 11 files changed, 54 insertions(+), 51 deletions(-)

-- 
2.1.4

[PATCH 1/2] MIPS: ZBOOT: copy appended dtb to the end of the kernel

[Jonas Gorski]

Instead of rewriting the arguments, just move the appended dtb to where
the decompressed kernel expects it. This eliminates the need for special
casing vmlinuz.bin appended dtb files.

Signed-off-by: Jonas Gorski <jogo@openwrt.org>
---
 arch/mips/Kconfig                      | 22 ++--------------------
 arch/mips/boot/compressed/Makefile     |  1 +
 arch/mips/boot/compressed/decompress.c | 21 +++++++++++++++++++++
 arch/mips/boot/compressed/head.S       | 16 ----------------
 4 files changed, 24 insertions(+), 36 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index ac91939..0d0f71e 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -2885,10 +2885,10 @@ choice
 		  the documented boot protocol using a device tree.
 
 	config MIPS_RAW_APPENDED_DTB
-		bool "vmlinux.bin"
+		bool "vmlinux.bin or vmlinuz.bin"
 		help
 		  With this option, the boot code will look for a device tree binary
-		  DTB) appended to raw vmlinux.bin (without decompressor).
+		  DTB) appended to raw vmlinux.bin or vmlinuz.bin.
 		  (e.g. cat vmlinux.bin <filename>.dtb > vmlinux_w_dtb).
 
 		  This is meant as a backward compatibility convenience for those
@@ -2900,24 +2900,6 @@ choice
 		  look like a DTB header after a reboot if no actual DTB is appended
 		  to vmlinux.bin.  Do not leave this option active in a production kernel
 		  if you don't intend to always append a DTB.
-
-	config MIPS_ZBOOT_APPENDED_DTB
-		bool "vmlinuz.bin"
-		depends on SYS_SUPPORTS_ZBOOT
-		help
-		  With this option, the boot code will look for a device tree binary
-		  DTB) appended to raw vmlinuz.bin (with decompressor).
-		  (e.g. cat vmlinuz.bin <filename>.dtb > vmlinuz_w_dtb).
-
-		  This is meant as a backward compatibility convenience for those
-		  systems with a bootloader that can't be upgraded to accommodate
-		  the documented boot protocol using a device tree.
-
-		  Beware that there is very little in terms of protection against
-		  this option being confused by leftover garbage in memory that might
-		  look like a DTB header after a reboot if no actual DTB is appended
-		  to vmlinuz.bin.  Do not leave this option active in a production kernel
-		  if you don't intend to always append a DTB.
 endchoice
 
 choice
diff --git a/arch/mips/boot/compressed/Makefile b/arch/mips/boot/compressed/Makefile
index 90aca95..f31ec89 100644
--- a/arch/mips/boot/compressed/Makefile
+++ b/arch/mips/boot/compressed/Makefile
@@ -29,6 +29,7 @@ KBUILD_AFLAGS := $(LINUXINCLUDE) $(KBUILD_AFLAGS) -D__ASSEMBLY__ \
 	-DBOOT_HEAP_SIZE=$(BOOT_HEAP_SIZE) \
 	-DKERNEL_ENTRY=$(VMLINUX_ENTRY_ADDRESS)
 
+
 # decompressor objects (linked with vmlinuz)
 vmlinuzobjs-y := $(obj)/head.o $(obj)/decompress.o $(obj)/string.o
 
diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
index 080cd53..e18ab3e 100644
--- a/arch/mips/boot/compressed/decompress.c
+++ b/arch/mips/boot/compressed/decompress.c
@@ -36,6 +36,12 @@ extern void puthex(unsigned long long val);
 #define puthex(val) do {} while (0)
 #endif
 
+#ifdef CONFIG_MIPS_RAW_APPENDED_DTB
+#include <linux/libfdt.h>
+
+extern char __appended_dtb[];
+#endif
+
 void error(char *x)
 {
 	puts("\n\n");
@@ -114,6 +120,21 @@ void decompress_kernel(unsigned long boot_heap_start)
 	__decompress((char *)zimage_start, zimage_size, 0, 0,
 		   (void *)VMLINUX_LOAD_ADDRESS_ULL, 0, 0, error);
 
+#ifdef CONFIG_MIPS_RAW_APPENDED_DTB
+	if (fdt_magic((void *)&__appended_dtb) == FDT_MAGIC) {
+		unsigned int image_size, dtb_size;
+
+		dtb_size = fdt_totalsize((void *)&__appended_dtb);
+
+		/* last four bytes is always image size in little endian */
+		image_size = le32_to_cpup((void *)&__image_end - 4);
+
+		/* copy dtb to where the booted kernel will expect it */
+		memcpy((void *)VMLINUX_LOAD_ADDRESS_ULL + image_size,
+		       __appended_dtb, dtb_size);
+	}
+#endif
+
 	/* FIXME: should we flush cache here? */
 	puts("Now, booting the kernel...\n");
 }
diff --git a/arch/mips/boot/compressed/head.S b/arch/mips/boot/compressed/head.S
index c580e85..409cb48 100644
--- a/arch/mips/boot/compressed/head.S
+++ b/arch/mips/boot/compressed/head.S
@@ -25,22 +25,6 @@ start:
 	move	s2, a2
 	move	s3, a3
 
-#ifdef CONFIG_MIPS_ZBOOT_APPENDED_DTB
-	PTR_LA	t0, __appended_dtb
-#ifdef CONFIG_CPU_BIG_ENDIAN
-	li	t1, 0xd00dfeed
-#else
-	li	t1, 0xedfe0dd0
-#endif
-	lw	t2, (t0)
-	bne	t1, t2, not_found
-	 nop
-
-	move	s1, t0
-	PTR_LI	s0, -2
-not_found:
-#endif
-
 	/* Clear BSS */
 	PTR_LA	a0, _edata
 	PTR_LA	a2, _end
-- 
2.1.4

[PATCH 2/2] MIPS: store the appended dtb address in a variable

[Jonas Gorski]

Instead of rewriting the arguments to match the UHI spec, store the
address of a appended or UHI supplied dtb in fw_supplied_dtb.

That way the original bootloader arguments are kept intact while still
making the use of an appended dtb invisible for mach code.

Mach code can still find out if it is an appended dtb by comparing
fw_arg1 with fw_supplied_dtb.

Signed-off-by: Jonas Gorski <jogo@openwrt.org>
---
 arch/mips/ath79/setup.c          |  4 ++--
 arch/mips/bmips/setup.c          |  4 ++--
 arch/mips/include/asm/bootinfo.h |  4 ++++
 arch/mips/kernel/head.S          | 21 ++++++++++++++-------
 arch/mips/kernel/setup.c         |  4 ++++
 arch/mips/lantiq/prom.c          |  4 ++--
 arch/mips/pic32/pic32mzda/init.c |  4 ++--
 7 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/arch/mips/ath79/setup.c b/arch/mips/ath79/setup.c
index 7adab18..2ec9100 100644
--- a/arch/mips/ath79/setup.c
+++ b/arch/mips/ath79/setup.c
@@ -204,8 +204,8 @@ void __init plat_mem_setup(void)
 	fdt_start = fw_getenvl("fdt_start");
 	if (fdt_start)
 		__dt_setup_arch((void *)KSEG0ADDR(fdt_start));
-	else if (fw_arg0 == -2)
-		__dt_setup_arch((void *)KSEG0ADDR(fw_arg1));
+	else if (fw_passed_dtb)
+		__dt_setup_arch((void *)KSEG0ADDR(fw_passed_dtb));
 
 	if (mips_machtype != ATH79_MACH_GENERIC_OF) {
 		ath79_reset_base = ioremap_nocache(AR71XX_RESET_BASE,
diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c
index f146d12..6776042 100644
--- a/arch/mips/bmips/setup.c
+++ b/arch/mips/bmips/setup.c
@@ -162,8 +162,8 @@ void __init plat_mem_setup(void)
 	/* intended to somewhat resemble ARM; see Documentation/arm/Booting */
 	if (fw_arg0 == 0 && fw_arg1 == 0xffffffff)
 		dtb = phys_to_virt(fw_arg2);
-	else if (fw_arg0 == -2) /* UHI interface */
-		dtb = (void *)fw_arg1;
+	else if (fw_passed_dtb) /* UHI interface */
+		dtb = (void *)fw_passed_dtb;
 	else if (__dtb_start != __dtb_end)
 		dtb = (void *)__dtb_start;
 	else
diff --git a/arch/mips/include/asm/bootinfo.h b/arch/mips/include/asm/bootinfo.h
index 9f67033..ee9f5f2 100644
--- a/arch/mips/include/asm/bootinfo.h
+++ b/arch/mips/include/asm/bootinfo.h
@@ -127,6 +127,10 @@ extern char arcs_cmdline[COMMAND_LINE_SIZE];
  */
 extern unsigned long fw_arg0, fw_arg1, fw_arg2, fw_arg3;
 
+#ifdef CONFIG_USE_OF
+extern unsigned long fw_passed_dtb;
+#endif
+
 /*
  * Platform memory detection hook called by setup_arch
  */
diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S
index 56e8fed..cf05220 100644
--- a/arch/mips/kernel/head.S
+++ b/arch/mips/kernel/head.S
@@ -93,21 +93,24 @@ NESTED(kernel_entry, 16, sp)			# kernel entry point
 	jr	t0
 0:
 
+#ifdef CONFIG_USE_OF
 #ifdef CONFIG_MIPS_RAW_APPENDED_DTB
-	PTR_LA		t0, __appended_dtb
+	PTR_LA		t2, __appended_dtb
 
 #ifdef CONFIG_CPU_BIG_ENDIAN
 	li		t1, 0xd00dfeed
 #else
 	li		t1, 0xedfe0dd0
 #endif
-	lw		t2, (t0)
-	bne		t1, t2, not_found
-	 nop
+	lw		t0, (t2)
+	beq		t0, t1, dtb_found
+#endif
+	li		t1, -2
+	beq		a0, t1, dtb_found
+	move		t2, a1
 
-	move		a1, t0
-	PTR_LI		a0, -2
-not_found:
+	li		t2, 0
+dtb_found:
 #endif
 	PTR_LA		t0, __bss_start		# clear .bss
 	LONG_S		zero, (t0)
@@ -122,6 +125,10 @@ not_found:
 	LONG_S		a2, fw_arg2
 	LONG_S		a3, fw_arg3
 
+#ifdef CONFIG_USE_OF
+	LONG_S		t2, fw_passed_dtb
+#endif
+
 	MTC0		zero, CP0_CONTEXT	# clear context register
 	PTR_LA		$28, init_thread_union
 	/* Set the SP after an empty pt_regs.  */
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index ef408a0..36cf8d6 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -875,6 +875,10 @@ void __init setup_arch(char **cmdline_p)
 unsigned long kernelsp[NR_CPUS];
 unsigned long fw_arg0, fw_arg1, fw_arg2, fw_arg3;
 
+#ifdef CONFIG_USE_OF
+unsigned long fw_passed_dtb;
+#endif
+
 #ifdef CONFIG_DEBUG_FS
 struct dentry *mips_debugfs_dir;
 static int __init debugfs_mips(void)
diff --git a/arch/mips/lantiq/prom.c b/arch/mips/lantiq/prom.c
index 5f693ac..4cbb000 100644
--- a/arch/mips/lantiq/prom.c
+++ b/arch/mips/lantiq/prom.c
@@ -74,8 +74,8 @@ void __init plat_mem_setup(void)
 
 	set_io_port_base((unsigned long) KSEG1);
 
-	if (fw_arg0 == -2) /* UHI interface */
-		dtb = (void *)fw_arg1;
+	if (fw_passed_dtb) /* UHI interface */
+		dtb = (void *)fw_passed_dtb;
 	else if (__dtb_start != __dtb_end)
 		dtb = (void *)__dtb_start;
 	else
diff --git a/arch/mips/pic32/pic32mzda/init.c b/arch/mips/pic32/pic32mzda/init.c
index 775ff90..a794037 100644
--- a/arch/mips/pic32/pic32mzda/init.c
+++ b/arch/mips/pic32/pic32mzda/init.c
@@ -33,8 +33,8 @@ static ulong get_fdtaddr(void)
 {
 	ulong ftaddr = 0;
 
-	if ((fw_arg0 == -2) && fw_arg1 && !fw_arg2 && !fw_arg3)
-		return (ulong)fw_arg1;
+	if (fw_passed_dtb && !fw_arg2 && !fw_arg3)
+		return (ulong)fw_passed_dtb;
 
 	if (__dtb_start < __dtb_end)
 		ftaddr = (ulong)__dtb_start;
-- 
2.1.4

Re: [PATCH 1/2] MIPS: ZBOOT: copy appended dtb to the end of the kernel

[Antony Pavlov]

On Fri, 17 Jun 2016 14:07:39 +0200
Jonas Gorski <jogo@openwrt.org> wrote:

> Instead of rewriting the arguments, just move the appended dtb to where
> the decompressed kernel expects it. This eliminates the need for special
> casing vmlinuz.bin appended dtb files.
> 
> Signed-off-by: Jonas Gorski <jogo@openwrt.org>
> ---
>  arch/mips/Kconfig                      | 22 ++--------------------
>  arch/mips/boot/compressed/Makefile     |  1 +
>  arch/mips/boot/compressed/decompress.c | 21 +++++++++++++++++++++
>  arch/mips/boot/compressed/head.S       | 16 ----------------
>  4 files changed, 24 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index ac91939..0d0f71e 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -2885,10 +2885,10 @@ choice
>  		  the documented boot protocol using a device tree.
>  
>  	config MIPS_RAW_APPENDED_DTB
> -		bool "vmlinux.bin"
> +		bool "vmlinux.bin or vmlinuz.bin"
>  		help
>  		  With this option, the boot code will look for a device tree binary
> -		  DTB) appended to raw vmlinux.bin (without decompressor).
> +		  DTB) appended to raw vmlinux.bin or vmlinuz.bin.
>  		  (e.g. cat vmlinux.bin <filename>.dtb > vmlinux_w_dtb).
>  
>  		  This is meant as a backward compatibility convenience for those
> @@ -2900,24 +2900,6 @@ choice
>  		  look like a DTB header after a reboot if no actual DTB is appended
>  		  to vmlinux.bin.  Do not leave this option active in a production kernel
>  		  if you don't intend to always append a DTB.
> -
> -	config MIPS_ZBOOT_APPENDED_DTB
> -		bool "vmlinuz.bin"
> -		depends on SYS_SUPPORTS_ZBOOT
> -		help
> -		  With this option, the boot code will look for a device tree binary
> -		  DTB) appended to raw vmlinuz.bin (with decompressor).
> -		  (e.g. cat vmlinuz.bin <filename>.dtb > vmlinuz_w_dtb).
> -
> -		  This is meant as a backward compatibility convenience for those
> -		  systems with a bootloader that can't be upgraded to accommodate
> -		  the documented boot protocol using a device tree.
> -
> -		  Beware that there is very little in terms of protection against
> -		  this option being confused by leftover garbage in memory that might
> -		  look like a DTB header after a reboot if no actual DTB is appended
> -		  to vmlinuz.bin.  Do not leave this option active in a production kernel
> -		  if you don't intend to always append a DTB.
>  endchoice
>  
>  choice
> diff --git a/arch/mips/boot/compressed/Makefile b/arch/mips/boot/compressed/Makefile
> index 90aca95..f31ec89 100644
> --- a/arch/mips/boot/compressed/Makefile
> +++ b/arch/mips/boot/compressed/Makefile
> @@ -29,6 +29,7 @@ KBUILD_AFLAGS := $(LINUXINCLUDE) $(KBUILD_AFLAGS) -D__ASSEMBLY__ \
>  	-DBOOT_HEAP_SIZE=$(BOOT_HEAP_SIZE) \
>  	-DKERNEL_ENTRY=$(VMLINUX_ENTRY_ADDRESS)
>  
> +

Could you please remote this extra empty line?

>  # decompressor objects (linked with vmlinuz)
>  vmlinuzobjs-y := $(obj)/head.o $(obj)/decompress.o $(obj)/string.o
>  
> diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
> index 080cd53..e18ab3e 100644
> --- a/arch/mips/boot/compressed/decompress.c
> +++ b/arch/mips/boot/compressed/decompress.c
> @@ -36,6 +36,12 @@ extern void puthex(unsigned long long val);
>  #define puthex(val) do {} while (0)
>  #endif
>  
> +#ifdef CONFIG_MIPS_RAW_APPENDED_DTB

Do we really need this '#ifdef' here?

> +#include <linux/libfdt.h>
> +
> +extern char __appended_dtb[];
> +#endif
> +

-- 
Best regards,
  Antony Pavlov

Re: [PATCH 1/2] MIPS: ZBOOT: copy appended dtb to the end of the kernel

[Jonas Gorski]

Hi Antony,

On 17.06.2016 16:01, Antony Pavlov wrote:
> On Fri, 17 Jun 2016 14:07:39 +0200
> Jonas Gorski <jogo@openwrt.org> wrote:
> 
>> Instead of rewriting the arguments, just move the appended dtb to where
>> the decompressed kernel expects it. This eliminates the need for special
>> casing vmlinuz.bin appended dtb files.
>>
>> Signed-off-by: Jonas Gorski <jogo@openwrt.org>
>> ---
>>  arch/mips/Kconfig                      | 22 ++--------------------
>>  arch/mips/boot/compressed/Makefile     |  1 +
>>  arch/mips/boot/compressed/decompress.c | 21 +++++++++++++++++++++
>>  arch/mips/boot/compressed/head.S       | 16 ----------------
>>  4 files changed, 24 insertions(+), 36 deletions(-)
>>
>> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
>> index ac91939..0d0f71e 100644
>> --- a/arch/mips/Kconfig
>> +++ b/arch/mips/Kconfig
>> @@ -2885,10 +2885,10 @@ choice
>>  		  the documented boot protocol using a device tree.
>>  
>>  	config MIPS_RAW_APPENDED_DTB
>> -		bool "vmlinux.bin"
>> +		bool "vmlinux.bin or vmlinuz.bin"
>>  		help
>>  		  With this option, the boot code will look for a device tree binary
>> -		  DTB) appended to raw vmlinux.bin (without decompressor).
>> +		  DTB) appended to raw vmlinux.bin or vmlinuz.bin.
>>  		  (e.g. cat vmlinux.bin <filename>.dtb > vmlinux_w_dtb).
>>  
>>  		  This is meant as a backward compatibility convenience for those
>> @@ -2900,24 +2900,6 @@ choice
>>  		  look like a DTB header after a reboot if no actual DTB is appended
>>  		  to vmlinux.bin.  Do not leave this option active in a production kernel
>>  		  if you don't intend to always append a DTB.
>> -
>> -	config MIPS_ZBOOT_APPENDED_DTB
>> -		bool "vmlinuz.bin"
>> -		depends on SYS_SUPPORTS_ZBOOT
>> -		help
>> -		  With this option, the boot code will look for a device tree binary
>> -		  DTB) appended to raw vmlinuz.bin (with decompressor).
>> -		  (e.g. cat vmlinuz.bin <filename>.dtb > vmlinuz_w_dtb).
>> -
>> -		  This is meant as a backward compatibility convenience for those
>> -		  systems with a bootloader that can't be upgraded to accommodate
>> -		  the documented boot protocol using a device tree.
>> -
>> -		  Beware that there is very little in terms of protection against
>> -		  this option being confused by leftover garbage in memory that might
>> -		  look like a DTB header after a reboot if no actual DTB is appended
>> -		  to vmlinuz.bin.  Do not leave this option active in a production kernel
>> -		  if you don't intend to always append a DTB.
>>  endchoice
>>  
>>  choice
>> diff --git a/arch/mips/boot/compressed/Makefile b/arch/mips/boot/compressed/Makefile
>> index 90aca95..f31ec89 100644
>> --- a/arch/mips/boot/compressed/Makefile
>> +++ b/arch/mips/boot/compressed/Makefile
>> @@ -29,6 +29,7 @@ KBUILD_AFLAGS := $(LINUXINCLUDE) $(KBUILD_AFLAGS) -D__ASSEMBLY__ \
>>  	-DBOOT_HEAP_SIZE=$(BOOT_HEAP_SIZE) \
>>  	-DKERNEL_ENTRY=$(VMLINUX_ENTRY_ADDRESS)
>>  
>> +
> 
> Could you please remote this extra empty line?

Oops, that must have slipped through. Fixed locally.

>>  # decompressor objects (linked with vmlinuz)
>>  vmlinuzobjs-y := $(obj)/head.o $(obj)/decompress.o $(obj)/string.o
>>  
>> diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
>> index 080cd53..e18ab3e 100644
>> --- a/arch/mips/boot/compressed/decompress.c
>> +++ b/arch/mips/boot/compressed/decompress.c
>> @@ -36,6 +36,12 @@ extern void puthex(unsigned long long val);
>>  #define puthex(val) do {} while (0)
>>  #endif
>>  
>> +#ifdef CONFIG_MIPS_RAW_APPENDED_DTB
> 
> Do we really need this '#ifdef' here?

No we don't, I was under the assumption the __appended_dtb is
only available when appended dtb support is enabled, for for
the decompressor it is always defined. At least gcc doesn't
seem to complain about it being unused in a quick test
compile. Fixed locally.

> 
>> +#include <linux/libfdt.h>

And moved this one to the other #includes.

>> +
>> +extern char __appended_dtb[];
>> +#endif
>> +


Jonas

Re: [PATCH 1/2] MIPS: ZBOOT: copy appended dtb to the end of the kernel

[Antony Pavlov]

On Fri, 17 Jun 2016 16:44:09 +0200
Jonas Gorski <jogo@openwrt.org> wrote:

> Hi Antony,
> 
> On 17.06.2016 16:01, Antony Pavlov wrote:
> > On Fri, 17 Jun 2016 14:07:39 +0200
> > Jonas Gorski <jogo@openwrt.org> wrote:
> > 
> >> Instead of rewriting the arguments, just move the appended dtb to where
> >> the decompressed kernel expects it. This eliminates the need for special
> >> casing vmlinuz.bin appended dtb files.
> >>
> >> Signed-off-by: Jonas Gorski <jogo@openwrt.org>
> >> ---
> >>  arch/mips/Kconfig                      | 22 ++--------------------
> >>  arch/mips/boot/compressed/Makefile     |  1 +
> >>  arch/mips/boot/compressed/decompress.c | 21 +++++++++++++++++++++
> >>  arch/mips/boot/compressed/head.S       | 16 ----------------
> >>  4 files changed, 24 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> >> index ac91939..0d0f71e 100644
> >> --- a/arch/mips/Kconfig
> >> +++ b/arch/mips/Kconfig
> >> @@ -2885,10 +2885,10 @@ choice
> >>  		  the documented boot protocol using a device tree.
> >>  
> >>  	config MIPS_RAW_APPENDED_DTB
> >> -		bool "vmlinux.bin"
> >> +		bool "vmlinux.bin or vmlinuz.bin"
> >>  		help
> >>  		  With this option, the boot code will look for a device tree binary
> >> -		  DTB) appended to raw vmlinux.bin (without decompressor).
> >> +		  DTB) appended to raw vmlinux.bin or vmlinuz.bin.
> >>  		  (e.g. cat vmlinux.bin <filename>.dtb > vmlinux_w_dtb).
> >>  
> >>  		  This is meant as a backward compatibility convenience for those
> >> @@ -2900,24 +2900,6 @@ choice
> >>  		  look like a DTB header after a reboot if no actual DTB is appended
> >>  		  to vmlinux.bin.  Do not leave this option active in a production kernel
> >>  		  if you don't intend to always append a DTB.
> >> -
> >> -	config MIPS_ZBOOT_APPENDED_DTB
> >> -		bool "vmlinuz.bin"
> >> -		depends on SYS_SUPPORTS_ZBOOT
> >> -		help
> >> -		  With this option, the boot code will look for a device tree binary
> >> -		  DTB) appended to raw vmlinuz.bin (with decompressor).
> >> -		  (e.g. cat vmlinuz.bin <filename>.dtb > vmlinuz_w_dtb).
> >> -
> >> -		  This is meant as a backward compatibility convenience for those
> >> -		  systems with a bootloader that can't be upgraded to accommodate
> >> -		  the documented boot protocol using a device tree.
> >> -
> >> -		  Beware that there is very little in terms of protection against
> >> -		  this option being confused by leftover garbage in memory that might
> >> -		  look like a DTB header after a reboot if no actual DTB is appended
> >> -		  to vmlinuz.bin.  Do not leave this option active in a production kernel
> >> -		  if you don't intend to always append a DTB.
> >>  endchoice
> >>  
> >>  choice
> >> diff --git a/arch/mips/boot/compressed/Makefile b/arch/mips/boot/compressed/Makefile
> >> index 90aca95..f31ec89 100644
> >> --- a/arch/mips/boot/compressed/Makefile
> >> +++ b/arch/mips/boot/compressed/Makefile
> >> @@ -29,6 +29,7 @@ KBUILD_AFLAGS := $(LINUXINCLUDE) $(KBUILD_AFLAGS) -D__ASSEMBLY__ \
> >>  	-DBOOT_HEAP_SIZE=$(BOOT_HEAP_SIZE) \
> >>  	-DKERNEL_ENTRY=$(VMLINUX_ENTRY_ADDRESS)
> >>  
> >> +
> > 
> > Could you please remote this extra empty line?
> 
> Oops, that must have slipped through. Fixed locally.
> 
> >>  # decompressor objects (linked with vmlinuz)
> >>  vmlinuzobjs-y := $(obj)/head.o $(obj)/decompress.o $(obj)/string.o
> >>  
> >> diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
> >> index 080cd53..e18ab3e 100644
> >> --- a/arch/mips/boot/compressed/decompress.c
> >> +++ b/arch/mips/boot/compressed/decompress.c
> >> @@ -36,6 +36,12 @@ extern void puthex(unsigned long long val);
> >>  #define puthex(val) do {} while (0)
> >>  #endif
> >>  
> >> +#ifdef CONFIG_MIPS_RAW_APPENDED_DTB
> > 
> > Do we really need this '#ifdef' here?
> 
> No we don't, I was under the assumption the __appended_dtb is
> only available when appended dtb support is enabled, for for
> the decompressor it is always defined. At least gcc doesn't
> seem to complain about it being unused in a quick test
> compile. Fixed locally.

Now the __appended_dtb variable is always available.
You can make the next step and try to transform your

  +#ifdef CONFIG_MIPS_RAW_APPENDED_DTB
  +	if (fdt_magic((void *)&__appended_dtb) == FDT_MAGIC) {
  +		unsigned int image_size, dtb_size;

into something like this

  +     if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB) &&
  +	    fdt_magic((void *)&__appended_dtb) == FDT_MAGIC) {
  +		unsigned int image_size, dtb_size;


Using IS_ENABLED instead of #if/#ifdef the compiler can check
all the code.

-- 
Best regards,
  Antony Pavlov

Re: [PATCH 2/2] MIPS: store the appended dtb address in a variable

[Daniel Gimpelevich]

On Fri, 2016-06-17 at 14:07 +0200, Jonas Gorski wrote:
> +++ b/arch/mips/pic32/pic32mzda/init.c
> @@ -33,8 +33,8 @@ static ulong get_fdtaddr(void)
>  {
>         ulong ftaddr = 0;
>  
> -       if ((fw_arg0 == -2) && fw_arg1 && !fw_arg2 && !fw_arg3)
> -               return (ulong)fw_arg1;
> +       if (fw_passed_dtb && !fw_arg2 && !fw_arg3)
> +               return (ulong)fw_passed_dtb;

This part potentially violates the UHI spec. If fw_passed_dtb is valid
but fw_arg0 > 0, fw_arg2 may still be a valid envp. Comparing otherwise
undefined values to zero is also incorrect.

Re: [PATCH 2/2] MIPS: store the appended dtb address in a variable

[Jonas Gorski]

Hi Daniel,

On 17 June 2016 at 18:56, Daniel Gimpelevich
<daniel@gimpelevich.san-francisco.ca.us> wrote:
> On Fri, 2016-06-17 at 14:07 +0200, Jonas Gorski wrote:
>> +++ b/arch/mips/pic32/pic32mzda/init.c
>> @@ -33,8 +33,8 @@ static ulong get_fdtaddr(void)
>>  {
>>         ulong ftaddr = 0;
>>
>> -       if ((fw_arg0 == -2) && fw_arg1 && !fw_arg2 && !fw_arg3)
>> -               return (ulong)fw_arg1;
>> +       if (fw_passed_dtb && !fw_arg2 && !fw_arg3)
>> +               return (ulong)fw_passed_dtb;
>
> This part potentially violates the UHI spec. If fw_passed_dtb is valid
> but fw_arg0 > 0, fw_arg2 may still be a valid envp. Comparing otherwise
> undefined values to zero is also incorrect.

That was true before and is still true after my changes, so nothing
added by me. This would be something for the pic32 maintainers (or
users) to comment on/fix.


Jonas