* 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
* 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
* [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-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: [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
* [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.