* Adding support for device tree and command line @ 2016-05-23 21:14 Hauke Mehrtens 2016-05-23 21:34 ` Hauke Mehrtens 0 siblings, 1 reply; 27+ messages in thread From: Hauke Mehrtens @ 2016-05-23 21:14 UTC (permalink / raw) To: linux-mips, Jonas Gorski; +Cc: Daniel Gimpelevich, Mathias Kresin Section 3 of this document defines some interfaces how a boot loader could forward a command line *or* a device tree to the kernel: http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf This allows only a device tree *or* a command line, not both. The Linux kernel also supports an appended device tree. In this case the early code overwrites the fw_args to look like the boot loader added a device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. The problem is when we use an appended device tree and the boot loader adds some important information in the kernel command line. In this case the command line gets overwritten and we do not get this information. This is the case for some lantiq devices were the boot loader provides the mac address to the kernel via the kernel command line. My proposal to solve this problem is to extend the interface and add a option to provide the kernel command line *and* a device tree from the boot loader to the kernel. a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) and fw_arg3 ($a3) with argv and envp. b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv and $a3 = envp. I would prefer solution b). This way we would not loose the kernel command line when appending a device tree and this could also be used by the boot loader if someone wants to. Should I send a patch for this? Hauke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Adding support for device tree and command line 2016-05-23 21:14 Adding support for device tree and command line Hauke Mehrtens @ 2016-05-23 21:34 ` Hauke Mehrtens 2016-05-23 22:12 ` Daniel Gimpelevich 0 siblings, 1 reply; 27+ messages in thread From: Hauke Mehrtens @ 2016-05-23 21:34 UTC (permalink / raw) To: linux-mips, Jonas Gorski; +Cc: Daniel Gimpelevich, Mathias Kresin On 05/23/2016 11:14 PM, Hauke Mehrtens wrote: > Section 3 of this document defines some interfaces how a boot loader > could forward a command line *or* a device tree to the kernel: > http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf > This allows only a device tree *or* a command line, not both. > > The Linux kernel also supports an appended device tree. In this case the > early code overwrites the fw_args to look like the boot loader added a > device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. > > The problem is when we use an appended device tree and the boot loader > adds some important information in the kernel command line. In this case > the command line gets overwritten and we do not get this information. > This is the case for some lantiq devices were the boot loader provides > the mac address to the kernel via the kernel command line. > > My proposal to solve this problem is to extend the interface and add a > option to provide the kernel command line *and* a device tree from the > boot loader to the kernel. > > a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) > and fw_arg3 ($a3) with argv and envp. > > b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv > and $a3 = envp. I just looked a little bit more closely and saw that the command line uses 3 args. One for the count, one argv and one envp. I would then only support device tree + count and argv, so the new interface would not support envp. > > I would prefer solution b). > > This way we would not loose the kernel command line when appending a > device tree and this could also be used by the boot loader if someone > wants to. > > Should I send a patch for this? > > Hauke > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Adding support for device tree and command line 2016-05-23 21:34 ` Hauke Mehrtens @ 2016-05-23 22:12 ` Daniel Gimpelevich 2016-05-24 5:32 ` [RFC PATCH] " Daniel Gimpelevich 0 siblings, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-23 22:12 UTC (permalink / raw) To: Hauke Mehrtens; +Cc: linux-mips, Jonas Gorski, Mathias Kresin On Mon, 2016-05-23 at 23:34 +0200, Hauke Mehrtens wrote: > On 05/23/2016 11:14 PM, Hauke Mehrtens wrote: > > Section 3 of this document defines some interfaces how a boot loader > > could forward a command line *or* a device tree to the kernel: > > http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf > > This allows only a device tree *or* a command line, not both. > > > > The Linux kernel also supports an appended device tree. In this case the > > early code overwrites the fw_args to look like the boot loader added a > > device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. > > > > The problem is when we use an appended device tree and the boot loader > > adds some important information in the kernel command line. In this case > > the command line gets overwritten and we do not get this information. > > This is the case for some lantiq devices were the boot loader provides > > the mac address to the kernel via the kernel command line. > > > > My proposal to solve this problem is to extend the interface and add a > > option to provide the kernel command line *and* a device tree from the > > boot loader to the kernel. > > > > a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) > > and fw_arg3 ($a3) with argv and envp. > > > > b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv > > and $a3 = envp. > > I just looked a little bit more closely and saw that the command line > uses 3 args. One for the count, one argv and one envp. > > I would then only support device tree + count and argv, so the new > interface would not support envp. > > > > > I would prefer solution b). > > > > This way we would not loose the kernel command line when appending a > > device tree and this could also be used by the boot loader if someone > > wants to. > > > > Should I send a patch for this? > > > > Hauke It was because I looked through the above-linked UHI spec that I became concerned about CONFIG_MIPS_RAW_APPENDED_DTB only mimicking, rather than fully implementing, real UHI. In the upstream kernel, the new $a0 == -2 code can be a starting point for adding UHI argv/envp parsing for when a UHI-compliant bootloader is used. However, on the head.S side, what I propose for the lantiq target is to remove CONFIG_MIPS_RAW_APPENDED_DTB from the kernel config, and reintroduce this as a platform patch: https://github.com/openwrt/openwrt/blob/b3158f781f24ac2ec1c0da86479bfc156c52c80b/target/linux/lantiq/patches-4.4/0036-owrt-generic-dtb-image-hack.patch The brcm63xx target could then retain CONFIG_MIPS_RAW_APPENDED_DTB, or not, depending on bootloader specifics there, which I have not investigated, and likewise the various other targets to which CONFIG_MIPS_RAW_APPENDED_DTB has since been extended even though it was apparently initially only an expedient hack only for brcm63xx. Using $a0 = -3 is expressly prohibited in the above UHI document, and using $a2/$a3 "would risk becoming incompatible with existing UHI compliant implementations." ^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH] Re: Adding support for device tree and command line 2016-05-23 22:12 ` Daniel Gimpelevich @ 2016-05-24 5:32 ` Daniel Gimpelevich 2016-05-24 11:27 ` Antony Pavlov 2016-05-26 16:25 ` [RFC PATCH] " Hauke Mehrtens 0 siblings, 2 replies; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-24 5:32 UTC (permalink / raw) To: Hauke Mehrtens; +Cc: linux-mips, Jonas Gorski, Mathias Kresin ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-24 5:32 ` [RFC PATCH] " Daniel Gimpelevich @ 2016-05-24 11:27 ` Antony Pavlov 2016-05-24 15:15 ` Daniel Gimpelevich 2016-05-26 16:25 ` [RFC PATCH] " Hauke Mehrtens 1 sibling, 1 reply; 27+ messages in thread From: Antony Pavlov @ 2016-05-24 11:27 UTC (permalink / raw) To: Daniel Gimpelevich Cc: Hauke Mehrtens, linux-mips, Jonas Gorski, Mathias Kresin On Mon, 23 May 2016 22:32:10 -0700 Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > From 464df9cb918d46fcbe5552b46b2fe7f916fdd0b4 Mon Sep 17 00:00:00 2001 > From: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> > Date: Mon, 23 May 2016 22:19:42 -0700 > Subject: [RFC PATCH] Re: Adding support for device tree and command line > > On Mon, 2016-05-23 at 15:12 -0700, Daniel Gimpelevich wrote: > > On Mon, 2016-05-23 at 23:34 +0200, Hauke Mehrtens wrote: > > > On 05/23/2016 11:14 PM, Hauke Mehrtens wrote: > > > > Section 3 of this document defines some interfaces how a boot loader > > > > could forward a command line *or* a device tree to the kernel: > > > > http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf > > > > This allows only a device tree *or* a command line, not both. > > > > > > > > The Linux kernel also supports an appended device tree. In this case the > > > > early code overwrites the fw_args to look like the boot loader added a > > > > device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. > > > > > > > > The problem is when we use an appended device tree and the boot loader > > > > adds some important information in the kernel command line. In this case > > > > the command line gets overwritten and we do not get this information. > > > > This is the case for some lantiq devices were the boot loader provides > > > > the mac address to the kernel via the kernel command line. > > > > > > > > My proposal to solve this problem is to extend the interface and add a > > > > option to provide the kernel command line *and* a device tree from the > > > > boot loader to the kernel. > > > > > > > > a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) > > > > and fw_arg3 ($a3) with argv and envp. > > > > > > > > b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv > > > > and $a3 = envp. > > > > > > I just looked a little bit more closely and saw that the command line > > > uses 3 args. One for the count, one argv and one envp. > > > > > > I would then only support device tree + count and argv, so the new > > > interface would not support envp. > > > > > > > > > > > I would prefer solution b). > > > > > > > > This way we would not loose the kernel command line when appending a > > > > device tree and this could also be used by the boot loader if someone > > > > wants to. > > > > > > > > Should I send a patch for this? > > > > > > > > Hauke > > > > It was because I looked through the above-linked UHI spec that I became > > concerned about CONFIG_MIPS_RAW_APPENDED_DTB only mimicking, rather than > > fully implementing, real UHI. In the upstream kernel, the new $a0 == -2 > > code can be a starting point for adding UHI argv/envp parsing for when a > > UHI-compliant bootloader is used. However, on the head.S side, what I > > propose for the lantiq target is to remove CONFIG_MIPS_RAW_APPENDED_DTB > > from the kernel config, and reintroduce this as a platform patch: > > https://github.com/openwrt/openwrt/blob/b3158f781f24ac2ec1c0da86479bfc156c52c80b/target/linux/lantiq/patches-4.4/0036-owrt-generic-dtb-image-hack.patch > > The brcm63xx target could then retain CONFIG_MIPS_RAW_APPENDED_DTB, or > > not, depending on bootloader specifics there, which I have not > > investigated, and likewise the various other targets to which > > CONFIG_MIPS_RAW_APPENDED_DTB has since been extended even though it was > > apparently initially only an expedient hack only for brcm63xx. > > > > Using $a0 = -3 is expressly prohibited in the above UHI document, and > > using $a2/$a3 "would risk becoming incompatible with existing UHI > > compliant implementations." > > I have come up with a more elegant solution: Simply move the register > substitution from head.S to just before it matters. You can still > override the boot args using CONFIG_MIPS_CMDLINE_FROM_DTB. > > Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> > --- > arch/mips/bmips/setup.c | 7 +++++++ > arch/mips/boot/compressed/head.S | 16 ---------------- > arch/mips/include/asm/prom.h | 5 +++++ > arch/mips/kernel/head.S | 16 ---------------- > arch/mips/lantiq/prom.c | 7 +++++++ > 5 files changed, 19 insertions(+), 32 deletions(-) > > diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c > index f146d12..2711c36 100644 > --- a/arch/mips/bmips/setup.c > +++ b/arch/mips/bmips/setup.c > @@ -160,6 +160,13 @@ void __init plat_mem_setup(void) > ioport_resource.end = ~0; > > /* intended to somewhat resemble ARM; see Documentation/arm/Booting */ > +#if defined(CONFIG_MIPS_RAW_APPENDED_DTB) ||\ > + defined(CONFIG_MIPS_ZBOOT_APPENDED_DTB) > + if (be32_to_cpup((__be32 *)__appended_dtb) == 0xd00dfeed) { We have too many 0xd00dfeed. Can we introduce some macro aliases, like arch/arm/kernel/head-common.S does? Something like this #define OF_DT_MAGIC 0xd00dfeed > + fw_arg0 = -2; > + fw_arg1 = (unsigned long)__appended_dtb; > + } > +#endif > if (fw_arg0 == 0 && fw_arg1 == 0xffffffff) > dtb = phys_to_virt(fw_arg2); > else if (fw_arg0 == -2) /* UHI interface */ > 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 > diff --git a/arch/mips/include/asm/prom.h b/arch/mips/include/asm/prom.h > index 0b4b668..bd0c5fd 100644 > --- a/arch/mips/include/asm/prom.h > +++ b/arch/mips/include/asm/prom.h > @@ -28,6 +28,11 @@ extern int __dt_register_buses(const char *bus0, const char *bus1); > static inline void device_tree_init(void) { } > #endif /* CONFIG_OF */ > > +#if defined(CONFIG_MIPS_RAW_APPENDED_DTB) ||\ > + defined(CONFIG_MIPS_ZBOOT_APPENDED_DTB) > +extern const char __appended_dtb[]; > +#endif > + > extern char *mips_get_machine_name(void); > extern void mips_set_machine_name(const char *name); > > diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S > index 56e8fed..766205c 100644 > --- a/arch/mips/kernel/head.S > +++ b/arch/mips/kernel/head.S > @@ -93,22 +93,6 @@ NESTED(kernel_entry, 16, sp) # kernel entry point > jr t0 > 0: > > -#ifdef CONFIG_MIPS_RAW_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 a1, t0 > - PTR_LI a0, -2 > -not_found: > -#endif > PTR_LA t0, __bss_start # clear .bss > LONG_S zero, (t0) > PTR_LA t1, __bss_stop - LONGSIZE > diff --git a/arch/mips/lantiq/prom.c b/arch/mips/lantiq/prom.c > index 5f693ac..f454b9d 100644 > --- a/arch/mips/lantiq/prom.c > +++ b/arch/mips/lantiq/prom.c > @@ -74,6 +74,13 @@ void __init plat_mem_setup(void) > > set_io_port_base((unsigned long) KSEG1); > > +#if defined(CONFIG_MIPS_RAW_APPENDED_DTB) ||\ > + defined(CONFIG_MIPS_ZBOOT_APPENDED_DTB) > + if (be32_to_cpup((__be32 *)__appended_dtb) == 0xd00dfeed) { > + fw_arg0 = -2; > + fw_arg1 = (unsigned long)__appended_dtb; > + } > +#endif > if (fw_arg0 == -2) /* UHI interface */ > dtb = (void *)fw_arg1; > else if (__dtb_start != __dtb_end) > -- > 1.9.1 > -- Best regards, Antony Pavlov ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-24 11:27 ` Antony Pavlov @ 2016-05-24 15:15 ` Daniel Gimpelevich 2016-05-24 15:27 ` Daniel Gimpelevich 0 siblings, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-24 15:15 UTC (permalink / raw) To: Antony Pavlov; +Cc: Hauke Mehrtens, linux-mips, Jonas Gorski, Mathias Kresin On Tue, 2016-05-24 at 14:27 +0300, Antony Pavlov wrote: > We have too many 0xd00dfeed. > Can we introduce some macro aliases, like > arch/arm/kernel/head-common.S does? > Something like this > > #define OF_DT_MAGIC 0xd00dfeed And exactly where would you propose to put that? Having the same #define in multiple places is no better than just using the value… ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-24 15:15 ` Daniel Gimpelevich @ 2016-05-24 15:27 ` Daniel Gimpelevich 2016-05-24 16:48 ` Antony Pavlov 0 siblings, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-24 15:27 UTC (permalink / raw) To: Antony Pavlov; +Cc: Hauke Mehrtens, linux-mips, Jonas Gorski, Mathias Kresin On Tue, 2016-05-24 at 08:15 -0700, Daniel Gimpelevich wrote: > On Tue, 2016-05-24 at 14:27 +0300, Antony Pavlov wrote: > > We have too many 0xd00dfeed. > > Can we introduce some macro aliases, like > > arch/arm/kernel/head-common.S does? > > Something like this > > > > #define OF_DT_MAGIC 0xd00dfeed > > And exactly where would you propose to put that? Having the same #define > in multiple places is no better than just using the value… Never mind. It's already in <linux/of_fdt.h>. I will supersede this after making any additional requested changes. What else needs to change? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-24 15:27 ` Daniel Gimpelevich @ 2016-05-24 16:48 ` Antony Pavlov 2016-05-24 17:00 ` Daniel Gimpelevich 2016-05-27 21:06 ` [PATCH v2] " Daniel Gimpelevich 0 siblings, 2 replies; 27+ messages in thread From: Antony Pavlov @ 2016-05-24 16:48 UTC (permalink / raw) To: Daniel Gimpelevich Cc: Hauke Mehrtens, linux-mips, Jonas Gorski, Mathias Kresin On Tue, 24 May 2016 08:27:30 -0700 Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > On Tue, 2016-05-24 at 08:15 -0700, Daniel Gimpelevich wrote: > > On Tue, 2016-05-24 at 14:27 +0300, Antony Pavlov wrote: > > > We have too many 0xd00dfeed. > > > Can we introduce some macro aliases, like > > > arch/arm/kernel/head-common.S does? > > > Something like this > > > > > > #define OF_DT_MAGIC 0xd00dfeed > > > > And exactly where would you propose to put that? Having the same #define > > in multiple places is no better than just using the value… > > Never mind. It's already in <linux/of_fdt.h>. I will supersede this > after making any additional requested changes. What else needs to > change? Also we can drop '#if defined(CONFIG_*' in favour of 'if (IS_ENABLED(CONFIG_*'. -- Best regards, Antony Pavlov ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-24 16:48 ` Antony Pavlov @ 2016-05-24 17:00 ` Daniel Gimpelevich 2016-05-25 3:31 ` Antony Pavlov 2016-05-27 21:06 ` [PATCH v2] " Daniel Gimpelevich 1 sibling, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-24 17:00 UTC (permalink / raw) To: Antony Pavlov; +Cc: Hauke Mehrtens, linux-mips, Jonas Gorski, Mathias Kresin On Tue, 2016-05-24 at 19:48 +0300, Antony Pavlov wrote: > Also we can drop '#if defined(CONFIG_*' in favour of 'if > (IS_ENABLED(CONFIG_*'. > > -- > Best regards, > Antony Pavlov OK. Anything else? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-24 17:00 ` Daniel Gimpelevich @ 2016-05-25 3:31 ` Antony Pavlov 2016-05-25 3:33 ` Daniel Gimpelevich 0 siblings, 1 reply; 27+ messages in thread From: Antony Pavlov @ 2016-05-25 3:31 UTC (permalink / raw) To: Daniel Gimpelevich Cc: Hauke Mehrtens, linux-mips, Jonas Gorski, Mathias Kresin On Tue, 24 May 2016 10:00:49 -0700 Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > On Tue, 2016-05-24 at 19:48 +0300, Antony Pavlov wrote: > > Also we can drop '#if defined(CONFIG_*' in favour of 'if > > (IS_ENABLED(CONFIG_*'. > > > > -- > > Best regards, > > Antony Pavlov > > OK. Anything else? I have nothing more to say just now. At the moment I don't use UHI-enabled bootloader. I'm planning to add UHI support to barebox bootloader (http://www.barebox.org) in a few weeks. What bootloader do you use? -- Best regards, Antony Pavlov ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-25 3:31 ` Antony Pavlov @ 2016-05-25 3:33 ` Daniel Gimpelevich 0 siblings, 0 replies; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-25 3:33 UTC (permalink / raw) To: Antony Pavlov; +Cc: Hauke Mehrtens, linux-mips, Jonas Gorski, Mathias Kresin On Wed, 2016-05-25 at 06:31 +0300, Antony Pavlov wrote: > On Tue, 24 May 2016 10:00:49 -0700 > Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > > > On Tue, 2016-05-24 at 19:48 +0300, Antony Pavlov wrote: > > > Also we can drop '#if defined(CONFIG_*' in favour of 'if > > > (IS_ENABLED(CONFIG_*'. > > > > > > -- > > > Best regards, > > > Antony Pavlov > > > > OK. Anything else? > > I have nothing more to say just now. > At the moment I don't use UHI-enabled bootloader. > I'm planning to add UHI support to barebox bootloader (http://www.barebox.org) > in a few weeks. > > What bootloader do you use? > > -- > Best regards, > Antony Pavlov The device in question has a non-UHI u-boot, and OpenWrt/LEDE have to append a DTB. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] Re: Adding support for device tree and command line 2016-05-24 16:48 ` Antony Pavlov 2016-05-24 17:00 ` Daniel Gimpelevich @ 2016-05-27 21:06 ` Daniel Gimpelevich 2016-05-28 10:31 ` Antony Pavlov 2016-05-29 10:53 ` Jonas Gorski 1 sibling, 2 replies; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-27 21:06 UTC (permalink / raw) To: linux-mips; +Cc: hauke, jogo, openwrt, antonynpavlov On Mon, 23 May 2016 22:32:10 -0700 Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > On Mon, 2016-05-23 at 15:12 -0700, Daniel Gimpelevich wrote: > > On Mon, 2016-05-23 at 23:34 +0200, Hauke Mehrtens wrote: > > > On 05/23/2016 11:14 PM, Hauke Mehrtens wrote: > > > > Section 3 of this document defines some interfaces how a boot loader > > > > could forward a command line *or* a device tree to the kernel: > > > > http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf > > > > This allows only a device tree *or* a command line, not both. > > > > > > > > The Linux kernel also supports an appended device tree. In this case the > > > > early code overwrites the fw_args to look like the boot loader added a > > > > device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. > > > > > > > > The problem is when we use an appended device tree and the boot loader > > > > adds some important information in the kernel command line. In this case > > > > the command line gets overwritten and we do not get this information. > > > > This is the case for some lantiq devices were the boot loader provides > > > > the mac address to the kernel via the kernel command line. > > > > > > > > My proposal to solve this problem is to extend the interface and add a > > > > option to provide the kernel command line *and* a device tree from the > > > > boot loader to the kernel. > > > > > > > > a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) > > > > and fw_arg3 ($a3) with argv and envp. > > > > > > > > b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv > > > > and $a3 = envp. > > > > > > I just looked a little bit more closely and saw that the command line > > > uses 3 args. One for the count, one argv and one envp. > > > > > > I would then only support device tree + count and argv, so the new > > > interface would not support envp. > > > > > > > > > > > I would prefer solution b). > > > > > > > > This way we would not loose the kernel command line when appending a > > > > device tree and this could also be used by the boot loader if someone > > > > wants to. > > > > > > > > Should I send a patch for this? > > > > > > > > Hauke > > > > It was because I looked through the above-linked UHI spec that I became > > concerned about CONFIG_MIPS_RAW_APPENDED_DTB only mimicking, rather than > > fully implementing, real UHI. In the upstream kernel, the new $a0 == -2 > > code can be a starting point for adding UHI argv/envp parsing for when a > > UHI-compliant bootloader is used. However, on the head.S side, what I > > propose for the lantiq target is to remove CONFIG_MIPS_RAW_APPENDED_DTB > > from the kernel config, and reintroduce this as a platform patch: > > https://github.com/openwrt/openwrt/blob/b3158f781f24ac2ec1c0da86479bfc156c52c80b/target/linux/lantiq/patches-4.4/0036-owrt-generic-dtb-image-hack.patch > > The brcm63xx target could then retain CONFIG_MIPS_RAW_APPENDED_DTB, or > > not, depending on bootloader specifics there, which I have not > > investigated, and likewise the various other targets to which > > CONFIG_MIPS_RAW_APPENDED_DTB has since been extended even though it was > > apparently initially only an expedient hack only for brcm63xx. > > > > Using $a0 = -3 is expressly prohibited in the above UHI document, and > > using $a2/$a3 "would risk becoming incompatible with existing UHI > > compliant implementations." > > I have come up with a more elegant solution: Simply move the register > substitution from head.S to just before it matters. You can still > override the boot args using CONFIG_MIPS_CMDLINE_FROM_DTB. Resending with only the changes Antonyn requested, since Hauke doesn't seem to be following up on his concerns anymore. Thanks go to both of them. Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> --- arch/mips/bmips/setup.c | 7 +++++++ arch/mips/boot/compressed/head.S | 16 ---------------- arch/mips/include/asm/prom.h | 5 +++++ arch/mips/kernel/head.S | 16 ---------------- arch/mips/lantiq/prom.c | 7 +++++++ 5 files changed, 19 insertions(+), 32 deletions(-) diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c index f146d12..3a327d4 100644 --- a/arch/mips/bmips/setup.c +++ b/arch/mips/bmips/setup.c @@ -160,6 +160,13 @@ void __init plat_mem_setup(void) ioport_resource.end = ~0; /* intended to somewhat resemble ARM; see Documentation/arm/Booting */ +#if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB)) ||\ + (IS_ENABLED(CONFIG_MIPS_ZBOOT_APPENDED_DTB)) + if (be32_to_cpup((__be32 *)__appended_dtb) == OF_DT_HEADER) { + fw_arg0 = -2; + fw_arg1 = (unsigned long)__appended_dtb; + } +#endif if (fw_arg0 == 0 && fw_arg1 == 0xffffffff) dtb = phys_to_virt(fw_arg2); else if (fw_arg0 == -2) /* UHI interface */ 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 diff --git a/arch/mips/include/asm/prom.h b/arch/mips/include/asm/prom.h index 0b4b668..6c29697 100644 --- a/arch/mips/include/asm/prom.h +++ b/arch/mips/include/asm/prom.h @@ -28,6 +28,11 @@ extern int __dt_register_buses(const char *bus0, const char *bus1); static inline void device_tree_init(void) { } #endif /* CONFIG_OF */ +#if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB)) ||\ + (IS_ENABLED(CONFIG_MIPS_ZBOOT_APPENDED_DTB)) +extern const char __appended_dtb[]; +#endif + extern char *mips_get_machine_name(void); extern void mips_set_machine_name(const char *name); diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S index 56e8fed..766205c 100644 --- a/arch/mips/kernel/head.S +++ b/arch/mips/kernel/head.S @@ -93,22 +93,6 @@ NESTED(kernel_entry, 16, sp) # kernel entry point jr t0 0: -#ifdef CONFIG_MIPS_RAW_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 a1, t0 - PTR_LI a0, -2 -not_found: -#endif PTR_LA t0, __bss_start # clear .bss LONG_S zero, (t0) PTR_LA t1, __bss_stop - LONGSIZE diff --git a/arch/mips/lantiq/prom.c b/arch/mips/lantiq/prom.c index 5f693ac..c355296 100644 --- a/arch/mips/lantiq/prom.c +++ b/arch/mips/lantiq/prom.c @@ -74,6 +74,13 @@ void __init plat_mem_setup(void) set_io_port_base((unsigned long) KSEG1); +#if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB)) ||\ + (IS_ENABLED(CONFIG_MIPS_ZBOOT_APPENDED_DTB)) + if (be32_to_cpup((__be32 *)__appended_dtb) == OF_DT_HEADER) { + fw_arg0 = -2; + fw_arg1 = (unsigned long)__appended_dtb; + } +#endif if (fw_arg0 == -2) /* UHI interface */ dtb = (void *)fw_arg1; else if (__dtb_start != __dtb_end) -- 1.9.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-27 21:06 ` [PATCH v2] " Daniel Gimpelevich @ 2016-05-28 10:31 ` Antony Pavlov 2016-05-28 19:05 ` Daniel Gimpelevich 2016-05-29 10:53 ` Jonas Gorski 1 sibling, 1 reply; 27+ messages in thread From: Antony Pavlov @ 2016-05-28 10:31 UTC (permalink / raw) To: Daniel Gimpelevich; +Cc: linux-mips, hauke, jogo, openwrt On Fri, 27 May 2016 14:06:38 -0700 Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > On Mon, 23 May 2016 22:32:10 -0700 > Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > > On Mon, 2016-05-23 at 15:12 -0700, Daniel Gimpelevich wrote: > > > On Mon, 2016-05-23 at 23:34 +0200, Hauke Mehrtens wrote: > > > > On 05/23/2016 11:14 PM, Hauke Mehrtens wrote: > > > > > Section 3 of this document defines some interfaces how a boot loader > > > > > could forward a command line *or* a device tree to the kernel: > > > > > http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf > > > > > This allows only a device tree *or* a command line, not both. > > > > > > > > > > The Linux kernel also supports an appended device tree. In this case the > > > > > early code overwrites the fw_args to look like the boot loader added a > > > > > device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. > > > > > > > > > > The problem is when we use an appended device tree and the boot loader > > > > > adds some important information in the kernel command line. In this case > > > > > the command line gets overwritten and we do not get this information. > > > > > This is the case for some lantiq devices were the boot loader provides > > > > > the mac address to the kernel via the kernel command line. > > > > > > > > > > My proposal to solve this problem is to extend the interface and add a > > > > > option to provide the kernel command line *and* a device tree from the > > > > > boot loader to the kernel. > > > > > > > > > > a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) > > > > > and fw_arg3 ($a3) with argv and envp. > > > > > > > > > > b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv > > > > > and $a3 = envp. > > > > > > > > I just looked a little bit more closely and saw that the command line > > > > uses 3 args. One for the count, one argv and one envp. > > > > > > > > I would then only support device tree + count and argv, so the new > > > > interface would not support envp. > > > > > > > > > > > > > > I would prefer solution b). > > > > > > > > > > This way we would not loose the kernel command line when appending a > > > > > device tree and this could also be used by the boot loader if someone > > > > > wants to. > > > > > > > > > > Should I send a patch for this? > > > > > > > > > > Hauke > > > > > > It was because I looked through the above-linked UHI spec that I became > > > concerned about CONFIG_MIPS_RAW_APPENDED_DTB only mimicking, rather than > > > fully implementing, real UHI. In the upstream kernel, the new $a0 == -2 > > > code can be a starting point for adding UHI argv/envp parsing for when a > > > UHI-compliant bootloader is used. However, on the head.S side, what I > > > propose for the lantiq target is to remove CONFIG_MIPS_RAW_APPENDED_DTB > > > from the kernel config, and reintroduce this as a platform patch: > > > https://github.com/openwrt/openwrt/blob/b3158f781f24ac2ec1c0da86479bfc156c52c80b/target/linux/lantiq/patches-4.4/0036-owrt-generic-dtb-image-hack.patch > > > The brcm63xx target could then retain CONFIG_MIPS_RAW_APPENDED_DTB, or > > > not, depending on bootloader specifics there, which I have not > > > investigated, and likewise the various other targets to which > > > CONFIG_MIPS_RAW_APPENDED_DTB has since been extended even though it was > > > apparently initially only an expedient hack only for brcm63xx. > > > > > > Using $a0 = -3 is expressly prohibited in the above UHI document, and > > > using $a2/$a3 "would risk becoming incompatible with existing UHI > > > compliant implementations." > > > > I have come up with a more elegant solution: Simply move the register > > substitution from head.S to just before it matters. You can still > > override the boot args using CONFIG_MIPS_CMDLINE_FROM_DTB. > > Resending with only the changes Antonyn requested, since Hauke doesn't seem to > be following up on his concerns anymore. Thanks go to both of them. > > Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> > --- > arch/mips/bmips/setup.c | 7 +++++++ > arch/mips/boot/compressed/head.S | 16 ---------------- > arch/mips/include/asm/prom.h | 5 +++++ > arch/mips/kernel/head.S | 16 ---------------- > arch/mips/lantiq/prom.c | 7 +++++++ > 5 files changed, 19 insertions(+), 32 deletions(-) > > diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c > index f146d12..3a327d4 100644 > --- a/arch/mips/bmips/setup.c > +++ b/arch/mips/bmips/setup.c > @@ -160,6 +160,13 @@ void __init plat_mem_setup(void) > ioport_resource.end = ~0; > > /* intended to somewhat resemble ARM; see Documentation/arm/Booting */ > +#if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB)) ||\ Can we use 'if' instead of preprocessor's '#if' here? If we use regular C 'if' operator with IS_ENABLED() instead of '#if/#ifdef' then the compiler can check all the code. E.g. please see this barebox patch: http://lists.infradead.org/pipermail/barebox/2014-February/017834.html > + (IS_ENABLED(CONFIG_MIPS_ZBOOT_APPENDED_DTB)) > + if (be32_to_cpup((__be32 *)__appended_dtb) == OF_DT_HEADER) { > + fw_arg0 = -2; > + fw_arg1 = (unsigned long)__appended_dtb; > + } > +#endif > if (fw_arg0 == 0 && fw_arg1 == 0xffffffff) > dtb = phys_to_virt(fw_arg2); > else if (fw_arg0 == -2) /* UHI interface */ > 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 > diff --git a/arch/mips/include/asm/prom.h b/arch/mips/include/asm/prom.h > index 0b4b668..6c29697 100644 > --- a/arch/mips/include/asm/prom.h > +++ b/arch/mips/include/asm/prom.h > @@ -28,6 +28,11 @@ extern int __dt_register_buses(const char *bus0, const char *bus1); > static inline void device_tree_init(void) { } > #endif /* CONFIG_OF */ > > +#if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB)) ||\ > + (IS_ENABLED(CONFIG_MIPS_ZBOOT_APPENDED_DTB)) > +extern const char __appended_dtb[]; > +#endif > + > extern char *mips_get_machine_name(void); > extern void mips_set_machine_name(const char *name); > > diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S > index 56e8fed..766205c 100644 > --- a/arch/mips/kernel/head.S > +++ b/arch/mips/kernel/head.S > @@ -93,22 +93,6 @@ NESTED(kernel_entry, 16, sp) # kernel entry point > jr t0 > 0: > > -#ifdef CONFIG_MIPS_RAW_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 a1, t0 > - PTR_LI a0, -2 > -not_found: > -#endif > PTR_LA t0, __bss_start # clear .bss > LONG_S zero, (t0) > PTR_LA t1, __bss_stop - LONGSIZE > diff --git a/arch/mips/lantiq/prom.c b/arch/mips/lantiq/prom.c > index 5f693ac..c355296 100644 > --- a/arch/mips/lantiq/prom.c > +++ b/arch/mips/lantiq/prom.c > @@ -74,6 +74,13 @@ void __init plat_mem_setup(void) > > set_io_port_base((unsigned long) KSEG1); > > +#if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB)) ||\ > + (IS_ENABLED(CONFIG_MIPS_ZBOOT_APPENDED_DTB)) > + if (be32_to_cpup((__be32 *)__appended_dtb) == OF_DT_HEADER) { > + fw_arg0 = -2; > + fw_arg1 = (unsigned long)__appended_dtb; > + } > +#endif > if (fw_arg0 == -2) /* UHI interface */ > dtb = (void *)fw_arg1; > else if (__dtb_start != __dtb_end) > -- > 1.9.1 > -- -- Best regards, Antony Pavlov ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-28 10:31 ` Antony Pavlov @ 2016-05-28 19:05 ` Daniel Gimpelevich 2016-05-29 1:23 ` Daniel Gimpelevich 0 siblings, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-28 19:05 UTC (permalink / raw) To: Antony Pavlov; +Cc: linux-mips, hauke, jogo, openwrt On Sat, 2016-05-28 at 13:31 +0300, Antony Pavlov wrote: > Can we use 'if' instead of preprocessor's '#if' here? > > If we use regular C 'if' operator with IS_ENABLED() instead of > '#if/#ifdef' > then the compiler can check all the code. > > E.g. please see this barebox patch: > > > http://lists.infradead.org/pipermail/barebox/2014-February/017834.html Sigh. I guess I will resubmit again… ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-28 19:05 ` Daniel Gimpelevich @ 2016-05-29 1:23 ` Daniel Gimpelevich 2016-05-30 17:24 ` Antony Pavlov 0 siblings, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-29 1:23 UTC (permalink / raw) To: Antony Pavlov; +Cc: linux-mips, hauke, jogo, openwrt On Sat, 2016-05-28 at 12:05 -0700, Daniel Gimpelevich wrote: > On Sat, 2016-05-28 at 13:31 +0300, Antony Pavlov wrote: > > Can we use 'if' instead of preprocessor's '#if' here? > > > > If we use regular C 'if' operator with IS_ENABLED() instead of > > '#if/#ifdef' > > then the compiler can check all the code. > > > > E.g. please see this barebox patch: > > > > > > http://lists.infradead.org/pipermail/barebox/2014-February/017834.html > > Sigh. I guess I will resubmit again… Upon further review, no, we cannot use 'if' instead of '#if' here. The reference to the appended DTB would throw a linker error if the option to put it there is not enabled. Sorry. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-29 1:23 ` Daniel Gimpelevich @ 2016-05-30 17:24 ` Antony Pavlov 0 siblings, 0 replies; 27+ messages in thread From: Antony Pavlov @ 2016-05-30 17:24 UTC (permalink / raw) To: Daniel Gimpelevich; +Cc: linux-mips, hauke, jogo, openwrt On 29/05/2016, Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > On Sat, 2016-05-28 at 12:05 -0700, Daniel Gimpelevich wrote: >> On Sat, 2016-05-28 at 13:31 +0300, Antony Pavlov wrote: >> > Can we use 'if' instead of preprocessor's '#if' here? >> > >> > If we use regular C 'if' operator with IS_ENABLED() instead of >> > '#if/#ifdef' >> > then the compiler can check all the code. >> > >> > E.g. please see this barebox patch: >> > >> > >> > http://lists.infradead.org/pipermail/barebox/2014-February/017834.html >> >> Sigh. I guess I will resubmit again… > > Upon further review, no, we cannot use 'if' instead of '#if' here. The > reference to the appended DTB would throw a linker error if the option > to put it there is not enabled. Sorry. Please note that modern gcc ignores 'undefined reference' errors inside optimized out code block (e.g. 'if (0) { ... }'). Here is an example: $ echo <<EOF; > esymbol.c int main() { if (CONFIG_ESYMBOL_STUFF) { extern int esymbol; esymbol = 1; } } EOF $ gcc -DCONFIG_ESYMBOL_STUFF=1 esymbol.c /tmp/ccNCGFCS.o: In function `main': esymbol.c:(.text+0x6): undefined reference to `esymbol' collect2: error: ld returned 1 exit status $ gcc -DCONFIG_ESYMBOL_STUFF=0 esymbol.c $ echo $? 0 $ $ gcc --version gcc (Debian 4.9.3-12) 4.9.3 Copyright (C) 2015 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. -- Best regards, Antony Pavlov ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-27 21:06 ` [PATCH v2] " Daniel Gimpelevich 2016-05-28 10:31 ` Antony Pavlov @ 2016-05-29 10:53 ` Jonas Gorski 2016-05-29 18:38 ` Daniel Gimpelevich 2016-05-29 21:19 ` Daniel Gimpelevich 1 sibling, 2 replies; 27+ messages in thread From: Jonas Gorski @ 2016-05-29 10:53 UTC (permalink / raw) To: Daniel Gimpelevich, linux-mips; +Cc: hauke, openwrt, antonynpavlov Hi, On 27.05.2016 23:06, Daniel Gimpelevich wrote: > On Mon, 23 May 2016 22:32:10 -0700 > Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: >> On Mon, 2016-05-23 at 15:12 -0700, Daniel Gimpelevich wrote: >>> On Mon, 2016-05-23 at 23:34 +0200, Hauke Mehrtens wrote: >>>> On 05/23/2016 11:14 PM, Hauke Mehrtens wrote: >>>>> Section 3 of this document defines some interfaces how a boot loader >>>>> could forward a command line *or* a device tree to the kernel: >>>>> http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf >>>>> This allows only a device tree *or* a command line, not both. >>>>> >>>>> The Linux kernel also supports an appended device tree. In this case the >>>>> early code overwrites the fw_args to look like the boot loader added a >>>>> device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. >>>>> >>>>> The problem is when we use an appended device tree and the boot loader >>>>> adds some important information in the kernel command line. In this case >>>>> the command line gets overwritten and we do not get this information. >>>>> This is the case for some lantiq devices were the boot loader provides >>>>> the mac address to the kernel via the kernel command line. >>>>> >>>>> My proposal to solve this problem is to extend the interface and add a >>>>> option to provide the kernel command line *and* a device tree from the >>>>> boot loader to the kernel. >>>>> >>>>> a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) >>>>> and fw_arg3 ($a3) with argv and envp. >>>>> >>>>> b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv >>>>> and $a3 = envp. >>>> >>>> I just looked a little bit more closely and saw that the command line >>>> uses 3 args. One for the count, one argv and one envp. >>>> >>>> I would then only support device tree + count and argv, so the new >>>> interface would not support envp. >>>> >>>>> >>>>> I would prefer solution b). >>>>> >>>>> This way we would not loose the kernel command line when appending a >>>>> device tree and this could also be used by the boot loader if someone >>>>> wants to. >>>>> >>>>> Should I send a patch for this? >>>>> >>>>> Hauke >>> >>> It was because I looked through the above-linked UHI spec that I became >>> concerned about CONFIG_MIPS_RAW_APPENDED_DTB only mimicking, rather than >>> fully implementing, real UHI. In the upstream kernel, the new $a0 == -2 >>> code can be a starting point for adding UHI argv/envp parsing for when a >>> UHI-compliant bootloader is used. However, on the head.S side, what I >>> propose for the lantiq target is to remove CONFIG_MIPS_RAW_APPENDED_DTB >>> from the kernel config, and reintroduce this as a platform patch: >>> https://github.com/openwrt/openwrt/blob/b3158f781f24ac2ec1c0da86479bfc156c52c80b/target/linux/lantiq/patches-4.4/0036-owrt-generic-dtb-image-hack.patch >>> The brcm63xx target could then retain CONFIG_MIPS_RAW_APPENDED_DTB, or >>> not, depending on bootloader specifics there, which I have not >>> investigated, and likewise the various other targets to which >>> CONFIG_MIPS_RAW_APPENDED_DTB has since been extended even though it was >>> apparently initially only an expedient hack only for brcm63xx. >>> >>> Using $a0 = -3 is expressly prohibited in the above UHI document, and >>> using $a2/$a3 "would risk becoming incompatible with existing UHI >>> compliant implementations." >> >> I have come up with a more elegant solution: Simply move the register >> substitution from head.S to just before it matters. You can still >> override the boot args using CONFIG_MIPS_CMDLINE_FROM_DTB. > > Resending with only the changes Antonyn requested, since Hauke doesn't seem to > be following up on his concerns anymore. Thanks go to both of them. > > Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> > --- > arch/mips/bmips/setup.c | 7 +++++++ > arch/mips/boot/compressed/head.S | 16 ---------------- > arch/mips/include/asm/prom.h | 5 +++++ > arch/mips/kernel/head.S | 16 ---------------- > arch/mips/lantiq/prom.c | 7 +++++++ > 5 files changed, 19 insertions(+), 32 deletions(-) > > diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c > index f146d12..3a327d4 100644 > --- a/arch/mips/bmips/setup.c > +++ b/arch/mips/bmips/setup.c > @@ -160,6 +160,13 @@ void __init plat_mem_setup(void) > ioport_resource.end = ~0; > > /* intended to somewhat resemble ARM; see Documentation/arm/Booting */ > +#if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB)) ||\ > + (IS_ENABLED(CONFIG_MIPS_ZBOOT_APPENDED_DTB)) > + if (be32_to_cpup((__be32 *)__appended_dtb) == OF_DT_HEADER) { > + fw_arg0 = -2; > + fw_arg1 = (unsigned long)__appended_dtb; > + } > +#endif This will break/won't compile for ZBOOT_APPENDED_DTB as __appended_dtb is part of the wrapping decompressor, and the kernel has no knowledge of this this symbol. > if (fw_arg0 == 0 && fw_arg1 == 0xffffffff) > dtb = phys_to_virt(fw_arg2); > else if (fw_arg0 == -2) /* UHI interface */ > 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 > - For the ZBOOT appended dtb, maybe the following way would be better: (completely not compile tested, just as "pseudo code") --- a/arch/mips/boot/compressed/decompress.c +++ b/arch/mips/boot/compressed/decompress.c @@ -36,6 +36,10 @@ extern void puthex(unsigned long long val); #define puthex(val) do {} while (0) #endif +#ifdef CONFIG_MIPS_APPENDED_DTB +extern char __appended_dtb[]; +#endif + void error(char *x) { puts("\n\n"); @@ -87,7 +91,7 @@ void __stack_chk_fail(void) void decompress_kernel(unsigned long boot_heap_start) { - unsigned long zimage_start, zimage_size; + unsigned long zimage_start, zimage_size, uncompressed_size; __stack_chk_guard_setup(); @@ -112,7 +116,17 @@ void decompress_kernel(unsigned long boot_heap_start) /* Decompress the kernel with according algorithm */ __decompress((char *)zimage_start, zimage_size, 0, 0, - (void *)VMLINUX_LOAD_ADDRESS_ULL, 0, 0, error); + (void *)VMLINUX_LOAD_ADDRESS_ULL, &uncompressed_size, 0, + error); + +#ifdef CONFIG_MIPS_APPENDED_DTB + if (be32_to_cpup((void *)__appended_dtb) == 0xd00dfeed) { + long size = be32_to_cpup((void *)__appended_dtb + 4); + + memcpy(VMLINUX_LOAD_ADDRESS_ULL + uncompressed_size, + __appended_dtb, size); + } +#endif /* FIXME: should we flush cache here? */ puts("Now, booting the kernel...\n"); Then we don't need to have ZBOOT_APPENDED_DTB at all, and it would work like the normal APPENDED_DTB case (and wouldn't touch a0~a3). > /* Clear BSS */ > PTR_LA a0, _edata > PTR_LA a2, _end > diff --git a/arch/mips/include/asm/prom.h b/arch/mips/include/asm/prom.h > index 0b4b668..6c29697 100644 > --- a/arch/mips/include/asm/prom.h > +++ b/arch/mips/include/asm/prom.h > @@ -28,6 +28,11 @@ extern int __dt_register_buses(const char *bus0, const char *bus1); > static inline void device_tree_init(void) { } > #endif /* CONFIG_OF */ > > +#if (IS_ENABLED(CONFIG_MIPS_RAW_APPENDED_DTB)) ||\ > + (IS_ENABLED(CONFIG_MIPS_ZBOOT_APPENDED_DTB)) > +extern const char __appended_dtb[]; > +#endif > + > extern char *mips_get_machine_name(void); > extern void mips_set_machine_name(const char *name); > > diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S > index 56e8fed..766205c 100644 > --- a/arch/mips/kernel/head.S > +++ b/arch/mips/kernel/head.S > @@ -93,22 +93,6 @@ NESTED(kernel_entry, 16, sp) # kernel entry point > jr t0 > 0: > > -#ifdef CONFIG_MIPS_RAW_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 a1, t0 > - PTR_LI a0, -2 > -not_found: > -#endif Maybe a better solution here would be to create a new symbol fw_passed_dtb and let the code store a1 in there if a0 is -2, or __appended_dtb in case it is valid? Then we wouldn't need to special case APPENDED_DTB for any mach wanting to use it, and they can just check fw_passed_dtb. something like: arch/mips/kernel/head.S: ... #ifdef CONFIG_USE_OF li t1, -2 beq a0, t1, dtb_found move t0, a0 #ifdef CONFIG_MIPS_RAW_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, no_dtb_found nop #endif no_dtb_found: li t0, 0 dtb_found: LONG_S t0, fw_passed_dtb #endif then all that needs to be done for e.g. bmips is: --- 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 and consequently fw_argX remains untouched for either case. Regards Jonas ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-29 10:53 ` Jonas Gorski @ 2016-05-29 18:38 ` Daniel Gimpelevich 2016-05-29 19:01 ` Jonas Gorski 2016-05-29 21:19 ` Daniel Gimpelevich 1 sibling, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-29 18:38 UTC (permalink / raw) To: Jonas Gorski; +Cc: linux-mips, hauke, openwrt, antonynpavlov On Sun, 2016-05-29 at 12:53 +0200, Jonas Gorski wrote: > This will break/won't compile for ZBOOT_APPENDED_DTB as __appended_dtb > is > part of the wrapping decompressor, and the kernel has no knowledge of > this > this symbol. If this were true, it wouldn't compile with the code you added to compressed/head.S, either. You're referencing it as an external symbol, which is exactly the same thing I'm doing here. Your proposed alternatives are functionally almost equivalent to your earlier rejected patches: https://patchwork.linux-mips.org/patch/7274/ https://patchwork.linux-mips.org/patch/7313/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-29 18:38 ` Daniel Gimpelevich @ 2016-05-29 19:01 ` Jonas Gorski 2016-05-29 19:08 ` Daniel Gimpelevich 0 siblings, 1 reply; 27+ messages in thread From: Jonas Gorski @ 2016-05-29 19:01 UTC (permalink / raw) To: Daniel Gimpelevich; +Cc: linux-mips, hauke, openwrt, antonynpavlov On 29.05.2016 20:38, Daniel Gimpelevich wrote: > On Sun, 2016-05-29 at 12:53 +0200, Jonas Gorski wrote: >> This will break/won't compile for ZBOOT_APPENDED_DTB as __appended_dtb >> is >> part of the wrapping decompressor, and the kernel has no knowledge of >> this >> this symbol. > > If this were true, it wouldn't compile with the code you added to > compressed/head.S, either. You're referencing it as an external symbol, > which is exactly the same thing I'm doing here. There are two different __appended_dtb definitions: the one for the (uncompressed) kernel appended one that is visible to the kernel (in kernel/vmlinux.lds.S), and one in boot/compressed/ld.script that is visible only to the wrapping decompressor. Since the wrapping decompressor is built *after* the kernel was compiled and compressed, there is no way to tell the kernel where __appended_dtb is relative to the decompressor, so for ZBOOT_APPENDED_DTB you cannot reference __appended_dtb from kernel code. Your proposed > alternatives are functionally almost equivalent to your earlier rejected > patches: These weren't rejected, just deemed insufficient (mostly by me myself). And only to this one is similar: > https://patchwork.linux-mips.org/patch/7274/ But in contrast to this one, it doesn't populate initial_boot_params auto- matically, but instead still requires the mach to do that (by calling __dt_setup_arch()). I dropped that because IIRC at that time I read that initial_boot_params isn't supposed to be directly accessed. Also not populating initial_boot_params is IMHO better as just because a0 says -2 it doesn't mean a1 references a dtb - that should still be up to the mach to say that it expects a dtb to be passed. > https://patchwork.linux-mips.org/patch/7313/ This one was only missing alignment for the !SMP case but is otherwise equivalent to what is in the kernel now. Jonas ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-29 19:01 ` Jonas Gorski @ 2016-05-29 19:08 ` Daniel Gimpelevich 2016-05-29 19:22 ` Jonas Gorski 0 siblings, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-29 19:08 UTC (permalink / raw) To: Jonas Gorski; +Cc: linux-mips, hauke, openwrt, antonynpavlov On Sun, 2016-05-29 at 21:01 +0200, Jonas Gorski wrote: > On 29.05.2016 20:38, Daniel Gimpelevich wrote: > > On Sun, 2016-05-29 at 12:53 +0200, Jonas Gorski wrote: > >> This will break/won't compile for ZBOOT_APPENDED_DTB as __appended_dtb > >> is > >> part of the wrapping decompressor, and the kernel has no knowledge of > >> this > >> this symbol. > > > > If this were true, it wouldn't compile with the code you added to > > compressed/head.S, either. You're referencing it as an external symbol, > > which is exactly the same thing I'm doing here. > > There are two different __appended_dtb definitions: the one for the > (uncompressed) kernel appended one that is visible to the kernel (in > kernel/vmlinux.lds.S), and one in boot/compressed/ld.script that is > visible only to the wrapping decompressor. > > Since the wrapping decompressor is built *after* the kernel was compiled > and compressed, there is no way to tell the kernel where __appended_dtb is > relative to the decompressor, so for ZBOOT_APPENDED_DTB you cannot > reference __appended_dtb from kernel code. > > Your proposed > > alternatives are functionally almost equivalent to your earlier rejected > > patches: > > These weren't rejected, just deemed insufficient (mostly by me myself). > > And only to this one is similar: > > https://patchwork.linux-mips.org/patch/7274/ > > But in contrast to this one, it doesn't populate initial_boot_params auto- > matically, but instead still requires the mach to do that (by calling > __dt_setup_arch()). I dropped that because IIRC at that time I read that > initial_boot_params isn't supposed to be directly accessed. > Also not populating initial_boot_params is IMHO better as just because > a0 says -2 it doesn't mean a1 references a dtb - that should still be up > to the mach to say that it expects a dtb to be passed. > > > > https://patchwork.linux-mips.org/patch/7313/ > > This one was only missing alignment for the !SMP case but is otherwise > equivalent to what is in the kernel now. > > > Jonas I see, and you are absolutely right in what you now suggest. What escapes me at the moment, though, is how to reconcile all this with UHI. UHI bootloaders may pass an external DTB, and it should be indistinguishable by the boot code from an appended DTB. Thoughts? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-29 19:08 ` Daniel Gimpelevich @ 2016-05-29 19:22 ` Jonas Gorski 2016-05-29 19:26 ` Daniel Gimpelevich 0 siblings, 1 reply; 27+ messages in thread From: Jonas Gorski @ 2016-05-29 19:22 UTC (permalink / raw) To: Daniel Gimpelevich; +Cc: linux-mips, hauke, openwrt, antonynpavlov On 29.05.2016 21:08, Daniel Gimpelevich wrote: > On Sun, 2016-05-29 at 21:01 +0200, Jonas Gorski wrote: >> On 29.05.2016 20:38, Daniel Gimpelevich wrote: >>> On Sun, 2016-05-29 at 12:53 +0200, Jonas Gorski wrote: >>>> This will break/won't compile for ZBOOT_APPENDED_DTB as __appended_dtb >>>> is >>>> part of the wrapping decompressor, and the kernel has no knowledge of >>>> this >>>> this symbol. >>> >>> If this were true, it wouldn't compile with the code you added to >>> compressed/head.S, either. You're referencing it as an external symbol, >>> which is exactly the same thing I'm doing here. >> >> There are two different __appended_dtb definitions: the one for the >> (uncompressed) kernel appended one that is visible to the kernel (in >> kernel/vmlinux.lds.S), and one in boot/compressed/ld.script that is >> visible only to the wrapping decompressor. >> >> Since the wrapping decompressor is built *after* the kernel was compiled >> and compressed, there is no way to tell the kernel where __appended_dtb is >> relative to the decompressor, so for ZBOOT_APPENDED_DTB you cannot >> reference __appended_dtb from kernel code. >> >> Your proposed >>> alternatives are functionally almost equivalent to your earlier rejected >>> patches: >> >> These weren't rejected, just deemed insufficient (mostly by me myself). >> >> And only to this one is similar: >>> https://patchwork.linux-mips.org/patch/7274/ >> >> But in contrast to this one, it doesn't populate initial_boot_params auto- >> matically, but instead still requires the mach to do that (by calling >> __dt_setup_arch()). I dropped that because IIRC at that time I read that >> initial_boot_params isn't supposed to be directly accessed. >> Also not populating initial_boot_params is IMHO better as just because >> a0 says -2 it doesn't mean a1 references a dtb - that should still be up >> to the mach to say that it expects a dtb to be passed. >> >> >>> https://patchwork.linux-mips.org/patch/7313/ >> >> This one was only missing alignment for the !SMP case but is otherwise >> equivalent to what is in the kernel now. >> >> >> Jonas > > I see, and you are absolutely right in what you now suggest. What > escapes me at the moment, though, is how to reconcile all this with UHI. > UHI bootloaders may pass an external DTB, and it should be > indistinguishable by the boot code from an appended DTB. Thoughts? That's what my proposed code does: if a0 is -2, then store a1 in fw_passed_dtb else if appended dtb support is enabled and there is a valid dtb at __appended_dtb, then store the address of it in fw_passed_dtb. else set fw_passed_dtb to 0. so for the kernel/mach side, there is no difference between a UHI passed dtb and a __appended_dtb, both will populate fw_passed_dtb. That still leaves the question which one should be preferred in case both are present. Jonas ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-29 19:22 ` Jonas Gorski @ 2016-05-29 19:26 ` Daniel Gimpelevich 2016-05-29 19:30 ` Jonas Gorski 0 siblings, 1 reply; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-29 19:26 UTC (permalink / raw) To: Jonas Gorski; +Cc: linux-mips, hauke, openwrt, antonynpavlov On Sun, 2016-05-29 at 21:22 +0200, Jonas Gorski wrote: > That still leaves the question which one should be preferred in case > both > are present. In the already merged code, the appended one is preferred, so I would favor that. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-29 19:26 ` Daniel Gimpelevich @ 2016-05-29 19:30 ` Jonas Gorski 0 siblings, 0 replies; 27+ messages in thread From: Jonas Gorski @ 2016-05-29 19:30 UTC (permalink / raw) To: Daniel Gimpelevich; +Cc: linux-mips, hauke, openwrt, antonynpavlov On 29.05.2016 21:26, Daniel Gimpelevich wrote: > On Sun, 2016-05-29 at 21:22 +0200, Jonas Gorski wrote: >> That still leaves the question which one should be preferred in case >> both >> are present. > > In the already merged code, the appended one is preferred, so I would > favor that. > Makes sense. mach code can still check for a0 == -2 and then a1 != fw_passed_dtb to know if the appended one was used or the passed one, in case it needs to know/wants to switch. Jonas ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] Re: Adding support for device tree and command line 2016-05-29 10:53 ` Jonas Gorski 2016-05-29 18:38 ` Daniel Gimpelevich @ 2016-05-29 21:19 ` Daniel Gimpelevich 1 sibling, 0 replies; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-29 21:19 UTC (permalink / raw) To: Jonas Gorski; +Cc: linux-mips, hauke, openwrt, antonynpavlov On Sun, 2016-05-29 at 12:53 +0200, Jonas Gorski wrote: > Maybe a better solution here would be to create a new symbol > fw_passed_dtb and let the code store a1 in there if a0 is -2, or > __appended_dtb in case it is valid? Then we wouldn't need to special > case APPENDED_DTB for any mach wanting to use it, and they can just > check fw_passed_dtb. > > something like: > > arch/mips/kernel/head.S: > ... > > #ifdef CONFIG_USE_OF > li t1, -2 > beq a0, t1, dtb_found > move t0, a0 > > #ifdef CONFIG_MIPS_RAW_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, no_dtb_found > nop > > #endif > no_dtb_found: > li t0, 0 > dtb_found: > LONG_S t0, fw_passed_dtb > #endif That prefers the wrong DTB in case both are present. I propose this instead: #ifdef CONFIG_USE_OF #ifdef CONFIG_MIPS_RAW_APPENDED_DTB PTR_LA t0, __appended_dtb #ifdef CONFIG_CPU_BIG_ENDIAN li t1, 0xd00dfeed #else li t1, 0xedfe0dd0 #endif lw t2, (t0) beq t1, t2, dtb_found nop #endif li t1, -2 beq a0, t1, dtb_found move t0, a1 no_dtb_found: li t0, 0 dtb_found: LONG_S t0, fw_passed_dtb #endif ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-24 5:32 ` [RFC PATCH] " Daniel Gimpelevich 2016-05-24 11:27 ` Antony Pavlov @ 2016-05-26 16:25 ` Hauke Mehrtens 2016-05-26 17:24 ` Daniel Gimpelevich 1 sibling, 1 reply; 27+ messages in thread From: Hauke Mehrtens @ 2016-05-26 16:25 UTC (permalink / raw) To: Daniel Gimpelevich; +Cc: linux-mips, Jonas Gorski, Mathias Kresin On 05/24/2016 07:32 AM, Daniel Gimpelevich wrote: > From 464df9cb918d46fcbe5552b46b2fe7f916fdd0b4 Mon Sep 17 00:00:00 2001 > From: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> > Date: Mon, 23 May 2016 22:19:42 -0700 > Subject: [RFC PATCH] Re: Adding support for device tree and command line > > On Mon, 2016-05-23 at 15:12 -0700, Daniel Gimpelevich wrote: >> On Mon, 2016-05-23 at 23:34 +0200, Hauke Mehrtens wrote: >>> On 05/23/2016 11:14 PM, Hauke Mehrtens wrote: >>>> Section 3 of this document defines some interfaces how a boot loader >>>> could forward a command line *or* a device tree to the kernel: >>>> http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf >>>> This allows only a device tree *or* a command line, not both. >>>> >>>> The Linux kernel also supports an appended device tree. In this case the >>>> early code overwrites the fw_args to look like the boot loader added a >>>> device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. >>>> >>>> The problem is when we use an appended device tree and the boot loader >>>> adds some important information in the kernel command line. In this case >>>> the command line gets overwritten and we do not get this information. >>>> This is the case for some lantiq devices were the boot loader provides >>>> the mac address to the kernel via the kernel command line. >>>> >>>> My proposal to solve this problem is to extend the interface and add a >>>> option to provide the kernel command line *and* a device tree from the >>>> boot loader to the kernel. >>>> >>>> a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) >>>> and fw_arg3 ($a3) with argv and envp. >>>> >>>> b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv >>>> and $a3 = envp. >>> >>> I just looked a little bit more closely and saw that the command line >>> uses 3 args. One for the count, one argv and one envp. >>> >>> I would then only support device tree + count and argv, so the new >>> interface would not support envp. >>> >>>> >>>> I would prefer solution b). >>>> >>>> This way we would not loose the kernel command line when appending a >>>> device tree and this could also be used by the boot loader if someone >>>> wants to. >>>> >>>> Should I send a patch for this? >>>> >>>> Hauke >> >> It was because I looked through the above-linked UHI spec that I became >> concerned about CONFIG_MIPS_RAW_APPENDED_DTB only mimicking, rather than >> fully implementing, real UHI. In the upstream kernel, the new $a0 == -2 >> code can be a starting point for adding UHI argv/envp parsing for when a >> UHI-compliant bootloader is used. However, on the head.S side, what I >> propose for the lantiq target is to remove CONFIG_MIPS_RAW_APPENDED_DTB >> from the kernel config, and reintroduce this as a platform patch: >> https://github.com/openwrt/openwrt/blob/b3158f781f24ac2ec1c0da86479bfc156c52c80b/target/linux/lantiq/patches-4.4/0036-owrt-generic-dtb-image-hack.patch >> The brcm63xx target could then retain CONFIG_MIPS_RAW_APPENDED_DTB, or >> not, depending on bootloader specifics there, which I have not >> investigated, and likewise the various other targets to which >> CONFIG_MIPS_RAW_APPENDED_DTB has since been extended even though it was >> apparently initially only an expedient hack only for brcm63xx. >> >> Using $a0 = -3 is expressly prohibited in the above UHI document, and >> using $a2/$a3 "would risk becoming incompatible with existing UHI >> compliant implementations." > > I have come up with a more elegant solution: Simply move the register > substitution from head.S to just before it matters. You can still > override the boot args using CONFIG_MIPS_CMDLINE_FROM_DTB. > > Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> > --- > arch/mips/bmips/setup.c | 7 +++++++ > arch/mips/boot/compressed/head.S | 16 ---------------- > arch/mips/include/asm/prom.h | 5 +++++ > arch/mips/kernel/head.S | 16 ---------------- > arch/mips/lantiq/prom.c | 7 +++++++ > 5 files changed, 19 insertions(+), 32 deletions(-) I like it in the ARM arch code that the SoC specific code does not have to take care of this stuff. The normal arch code provides the device tree and so on. Can we at least add a function to mips which will read the device tree and the kernel cmd, so I do not have to open code it for each SoC, but only call this function. Hauke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] Re: Adding support for device tree and command line 2016-05-26 16:25 ` [RFC PATCH] " Hauke Mehrtens @ 2016-05-26 17:24 ` Daniel Gimpelevich 0 siblings, 0 replies; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-26 17:24 UTC (permalink / raw) To: Hauke Mehrtens; +Cc: linux-mips, Jonas Gorski, Mathias Kresin On Thu, 2016-05-26 at 18:25 +0200, Hauke Mehrtens wrote: > I like it in the ARM arch code that the SoC specific code does not > have > to take care of this stuff. The normal arch code provides the device > tree and so on. Well, MIPS bootloaders are strikingly more diverse than on ARM… > Can we at least add a function to mips which will read the device tree > and the kernel cmd, so I do not have to open code it for each SoC, but > only call this function. That would take a serious arch overhaul not unlike the migration to DT, and I suspect multiple targets would break. So far, this argc == -2 stuff has been used on only two targets that I could see, so I don't believe it belongs in head.S even with such an overhaul. That said, if _real_ UHI spec compliance gets in, the SoC-specific stuff could just be dropped. In the meantime, I propose this patch with the changes previously suggested. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH] Re: Adding support for device tree and command line @ 2016-05-24 5:19 Daniel Gimpelevich 0 siblings, 0 replies; 27+ messages in thread From: Daniel Gimpelevich @ 2016-05-24 5:19 UTC (permalink / raw) On Mon, 2016-05-23 at 15:12 -0700, Daniel Gimpelevich wrote: > On Mon, 2016-05-23 at 23:34 +0200, Hauke Mehrtens wrote: > > On 05/23/2016 11:14 PM, Hauke Mehrtens wrote: > > > Section 3 of this document defines some interfaces how a boot loader > > > could forward a command line *or* a device tree to the kernel: > > > http://wiki.prplfoundation.org/w/images/4/42/UHI_Reference_Manual.pdf > > > This allows only a device tree *or* a command line, not both. > > > > > > The Linux kernel also supports an appended device tree. In this case the > > > early code overwrites the fw_args to look like the boot loader added a > > > device tree. This is done when CONFIG_MIPS_RAW_APPENDED_DTB is activated. > > > > > > The problem is when we use an appended device tree and the boot loader > > > adds some important information in the kernel command line. In this case > > > the command line gets overwritten and we do not get this information. > > > This is the case for some lantiq devices were the boot loader provides > > > the mac address to the kernel via the kernel command line. > > > > > > My proposal to solve this problem is to extend the interface and add a > > > option to provide the kernel command line *and* a device tree from the > > > boot loader to the kernel. > > > > > > a) use fw_arg0 ($a0) = -2 and fill the unused registers fw_arg2 ($a2) > > > and fw_arg3 ($a3) with argv and envp. > > > > > > b) add a new boot protocol $a0 = -3 with $a1 = DT address, $a2 = argv > > > and $a3 = envp. > > > > I just looked a little bit more closely and saw that the command line > > uses 3 args. One for the count, one argv and one envp. > > > > I would then only support device tree + count and argv, so the new > > interface would not support envp. > > > > > > > > I would prefer solution b). > > > > > > This way we would not loose the kernel command line when appending a > > > device tree and this could also be used by the boot loader if someone > > > wants to. > > > > > > Should I send a patch for this? > > > > > > Hauke > > It was because I looked through the above-linked UHI spec that I became > concerned about CONFIG_MIPS_RAW_APPENDED_DTB only mimicking, rather than > fully implementing, real UHI. In the upstream kernel, the new $a0 == -2 > code can be a starting point for adding UHI argv/envp parsing for when a > UHI-compliant bootloader is used. However, on the head.S side, what I > propose for the lantiq target is to remove CONFIG_MIPS_RAW_APPENDED_DTB > from the kernel config, and reintroduce this as a platform patch: > https://github.com/openwrt/openwrt/blob/b3158f781f24ac2ec1c0da86479bfc156c52c80b/target/linux/lantiq/patches-4.4/0036-owrt-generic-dtb-image-hack.patch > The brcm63xx target could then retain CONFIG_MIPS_RAW_APPENDED_DTB, or > not, depending on bootloader specifics there, which I have not > investigated, and likewise the various other targets to which > CONFIG_MIPS_RAW_APPENDED_DTB has since been extended even though it was > apparently initially only an expedient hack only for brcm63xx. > > Using $a0 = -3 is expressly prohibited in the above UHI document, and > using $a2/$a3 "would risk becoming incompatible with existing UHI > compliant implementations." I have come up with a more elegant solution: Simply move the register substitution from head.S to just before it matters. You can still override the boot args using CONFIG_MIPS_CMDLINE_FROM_DTB. Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> --- arch/mips/bmips/setup.c | 7 +++++++ arch/mips/boot/compressed/head.S | 16 ---------------- arch/mips/include/asm/prom.h | 5 +++++ arch/mips/kernel/head.S | 16 ---------------- arch/mips/lantiq/prom.c | 7 +++++++ 5 files changed, 19 insertions(+), 32 deletions(-) diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c index f146d12..2711c36 100644 --- a/arch/mips/bmips/setup.c +++ b/arch/mips/bmips/setup.c @@ -160,6 +160,13 @@ void __init plat_mem_setup(void) ioport_resource.end = ~0; /* intended to somewhat resemble ARM; see Documentation/arm/Booting */ +#if defined(CONFIG_MIPS_RAW_APPENDED_DTB) ||\ + defined(CONFIG_MIPS_ZBOOT_APPENDED_DTB) + if (be32_to_cpup((__be32 *)__appended_dtb) == 0xd00dfeed) { + fw_arg0 = -2; + fw_arg1 = (unsigned long)__appended_dtb; + } +#endif if (fw_arg0 == 0 && fw_arg1 == 0xffffffff) dtb = phys_to_virt(fw_arg2); else if (fw_arg0 == -2) /* UHI interface */ 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 diff --git a/arch/mips/include/asm/prom.h b/arch/mips/include/asm/prom.h index 0b4b668..bd0c5fd 100644 --- a/arch/mips/include/asm/prom.h +++ b/arch/mips/include/asm/prom.h @@ -28,6 +28,11 @@ extern int __dt_register_buses(const char *bus0, const char *bus1); static inline void device_tree_init(void) { } #endif /* CONFIG_OF */ +#if defined(CONFIG_MIPS_RAW_APPENDED_DTB) ||\ + defined(CONFIG_MIPS_ZBOOT_APPENDED_DTB) +extern const char __appended_dtb[]; +#endif + extern char *mips_get_machine_name(void); extern void mips_set_machine_name(const char *name); diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S index 56e8fed..766205c 100644 --- a/arch/mips/kernel/head.S +++ b/arch/mips/kernel/head.S @@ -93,22 +93,6 @@ NESTED(kernel_entry, 16, sp) # kernel entry point jr t0 0: -#ifdef CONFIG_MIPS_RAW_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 a1, t0 - PTR_LI a0, -2 -not_found: -#endif PTR_LA t0, __bss_start # clear .bss LONG_S zero, (t0) PTR_LA t1, __bss_stop - LONGSIZE diff --git a/arch/mips/lantiq/prom.c b/arch/mips/lantiq/prom.c index 5f693ac..f454b9d 100644 --- a/arch/mips/lantiq/prom.c +++ b/arch/mips/lantiq/prom.c @@ -74,6 +74,13 @@ void __init plat_mem_setup(void) set_io_port_base((unsigned long) KSEG1); +#if defined(CONFIG_MIPS_RAW_APPENDED_DTB) ||\ + defined(CONFIG_MIPS_ZBOOT_APPENDED_DTB) + if (be32_to_cpup((__be32 *)__appended_dtb) == 0xd00dfeed) { + fw_arg0 = -2; + fw_arg1 = (unsigned long)__appended_dtb; + } +#endif if (fw_arg0 == -2) /* UHI interface */ dtb = (void *)fw_arg1; else if (__dtb_start != __dtb_end) -- 1.9.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
end of thread, other threads:[~2016-05-30 17:24 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-23 21:14 Adding support for device tree and command line Hauke Mehrtens 2016-05-23 21:34 ` Hauke Mehrtens 2016-05-23 22:12 ` Daniel Gimpelevich 2016-05-24 5:32 ` [RFC PATCH] " Daniel Gimpelevich 2016-05-24 11:27 ` Antony Pavlov 2016-05-24 15:15 ` Daniel Gimpelevich 2016-05-24 15:27 ` Daniel Gimpelevich 2016-05-24 16:48 ` Antony Pavlov 2016-05-24 17:00 ` Daniel Gimpelevich 2016-05-25 3:31 ` Antony Pavlov 2016-05-25 3:33 ` Daniel Gimpelevich 2016-05-27 21:06 ` [PATCH v2] " Daniel Gimpelevich 2016-05-28 10:31 ` Antony Pavlov 2016-05-28 19:05 ` Daniel Gimpelevich 2016-05-29 1:23 ` Daniel Gimpelevich 2016-05-30 17:24 ` Antony Pavlov 2016-05-29 10:53 ` Jonas Gorski 2016-05-29 18:38 ` Daniel Gimpelevich 2016-05-29 19:01 ` Jonas Gorski 2016-05-29 19:08 ` Daniel Gimpelevich 2016-05-29 19:22 ` Jonas Gorski 2016-05-29 19:26 ` Daniel Gimpelevich 2016-05-29 19:30 ` Jonas Gorski 2016-05-29 21:19 ` Daniel Gimpelevich 2016-05-26 16:25 ` [RFC PATCH] " Hauke Mehrtens 2016-05-26 17:24 ` Daniel Gimpelevich 2016-05-24 5:19 Daniel Gimpelevich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.