[PATCH 1/2] arm/cpu/start.c: Distil some common code in __start().

[PATCH 1/2] arm/cpu/start.c: Distil some common code in __start().

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/cpu/start.c | 49 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
index 8e5097b..7ffde7c 100644
--- a/arch/arm/cpu/start.c
+++ b/arch/arm/cpu/start.c
@@ -53,6 +53,18 @@ void *barebox_arm_boot_dtb(void)
 	return barebox_boot_dtb;
 }

+static uint32_t get_any_boarddata_magic(const void *boarddata)
+{
+	if (get_unaligned_be32(boarddata) == FDT_MAGIC)
+		return FDT_MAGIC;
+
+	if (((struct barebox_arm_boarddata *)boarddata)->magic ==
+	    BAREBOX_ARM_BOARDDATA_MAGIC)
+		return BAREBOX_ARM_BOARDDATA_MAGIC;
+
+	return 0;
+}
+
 static noinline __noreturn void __start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
@@ -89,21 +101,30 @@ static noinline __noreturn void __start(unsigned long membase,
 	}

 	if (boarddata) {
-		if (get_unaligned_be32(boarddata) == FDT_MAGIC) {
-			uint32_t totalsize = get_unaligned_be32(boarddata + 4);
+		uint32_t totalsize;
+		void **var = NULL;
+		const char *name;
+
+		switch (get_any_boarddata_magic(boarddata)) {
+		case FDT_MAGIC:
+			totalsize = get_unaligned_be32(boarddata + 4);
+			var = &barebox_boot_dtb;
+			name = "DTB";
+			break;
+		case BAREBOX_ARM_BOARDDATA_MAGIC:
+			totalsize = sizeof(struct barebox_arm_boarddata);
+			var = &barebox_boarddata;
+			name = "machine type";
+			break;
+		default:
+			break;
+		}
+
+		if (var) {
 			endmem -= ALIGN(totalsize, 64);
-			barebox_boot_dtb = (void *)endmem;
-			pr_debug("found DTB in boarddata, copying to 0x%p\n",
-					barebox_boot_dtb);
-			memcpy(barebox_boot_dtb, boarddata, totalsize);
-		} else if (((struct barebox_arm_boarddata *)boarddata)->magic ==
-				BAREBOX_ARM_BOARDDATA_MAGIC) {
-			endmem -= ALIGN(sizeof(struct barebox_arm_boarddata), 64);
-			barebox_boarddata = (void *)endmem;
-			pr_debug("found machine type in boarddata, copying to 0x%p\n",
-					barebox_boarddata);
-			memcpy(barebox_boarddata, boarddata,
-					sizeof(struct barebox_arm_boarddata));
+			pr_debug("found %s in boarddata, copying to 0x%lu\n",
+				 name, endmem);
+			*var = memcpy((void *)endmem, boarddata, totalsize);
 		}
 	}

--
2.1.4

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] arm/cpu/start.c: Avoid copying device-tree when possible

[Andrey Smirnov]

In case of non-relocatable image device-tree blob should already be
preloaded into memory as a part of Barebox binary upload, so there is
no need to 'memcpy' it again

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/cpu/start.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
index 7ffde7c..c461a73 100644
--- a/arch/arm/cpu/start.c
+++ b/arch/arm/cpu/start.c
@@ -107,6 +107,15 @@ static noinline __noreturn void __start(unsigned long membase,

 		switch (get_any_boarddata_magic(boarddata)) {
 		case FDT_MAGIC:
+			if (!IS_ENABLED(CONFIG_RELOCATABLE)) {
+				/*
+				  If Barebox is not relocatable
+				  there's no need to move data around
+				 */
+				barebox_boot_dtb = boarddata;
+				break;
+			}
+
 			totalsize = get_unaligned_be32(boarddata + 4);
 			var = &barebox_boot_dtb;
 			name = "DTB";
--
2.1.4

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] arm/cpu/start.c: Avoid copying device-tree when possible

[Sascha Hauer]

Hi Andrey,

On Fri, Sep 25, 2015 at 11:02:18PM -0700, Andrey Smirnov wrote:
> In case of non-relocatable image device-tree blob should already be
> preloaded into memory as a part of Barebox binary upload, so there is
> no need to 'memcpy' it again
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/cpu/start.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
> index 7ffde7c..c461a73 100644
> --- a/arch/arm/cpu/start.c
> +++ b/arch/arm/cpu/start.c
> @@ -107,6 +107,15 @@ static noinline __noreturn void __start(unsigned long membase,
> 
>  		switch (get_any_boarddata_magic(boarddata)) {
>  		case FDT_MAGIC:
> +			if (!IS_ENABLED(CONFIG_RELOCATABLE)) {
> +				/*
> +				  If Barebox is not relocatable
> +				  there's no need to move data around
> +				 */
> +				barebox_boot_dtb = boarddata;
> +				break;
> +			}
> +

At this place it is unknown where in memory the fdt is. It could well be
somewhere in the malloc area space, so we need to move it to a safe
place before we setup malloc in the next step.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] arm/cpu/start.c: Avoid copying device-tree when possible

[Andrey Smirnov]

>> +                     }
>> +
>
> At this place it is unknown where in memory the fdt is. It could well be
> somewhere in the malloc area space, so we need to move it to a safe
> place before we setup malloc in the next step.

I didn't do an exhaustive search in the source but it seemed like all
of the callers of barebox_arm_entry() were calling it either with NULL
or some build-in address, so I assumed that this change shouldn't be a
problem for non-relocatable binaries, but you are right, semantics of
the functions does not restrict the location of fdt data.

I'd still like to discuss the possibility of introducing a feature
like that to the codebase. Right now I have a use-case where I use
Barebox as a DDR memory tuning/testing tool on i.MX6Q where I upload
the image to IRAM via JTAG and execute Barebox straight out of SRAM.
With only 256K of total memory availible I just don't have enough RAM
to do this precautionary step of copying ~30K or .dtb. Do you see a
place for a feature like that in upstream code? Something like a
Kconfig option, similar to CONFIG_PBL_FORCE_PIGGYDATA_COPY, that can
be selected on per-board basis and would enable the shortcut?


>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] arm/cpu/start.c: Avoid copying device-tree when possible

[Sascha Hauer]

On Tue, Sep 29, 2015 at 10:52:27AM -0700, Andrey Smirnov wrote:
> >> +                     }
> >> +
> >
> > At this place it is unknown where in memory the fdt is. It could well be
> > somewhere in the malloc area space, so we need to move it to a safe
> > place before we setup malloc in the next step.
> 
> I didn't do an exhaustive search in the source but it seemed like all
> of the callers of barebox_arm_entry() were calling it either with NULL
> or some build-in address, so I assumed that this change shouldn't be a
> problem for non-relocatable binaries, but you are right, semantics of
> the functions does not restrict the location of fdt data.

For most if not all boards the fdt is built into the PBL. The PBL can
run on any place. Take for example the Karo i.MX6 board. The load
address is configured as 0x20000000 which in somewhere in the middle of
the SDRAM. This is where the PBL is executed and thus also the place
where the builtin fdt is stored. Now the PBL extracts the barebox binary
to near the end of SDRAM and jumps there. The fdt is now somewhere in
the middle of SDRAM where it will be overwritten by the malloc pool
sooner or later.

> 
> I'd still like to discuss the possibility of introducing a feature
> like that to the codebase. Right now I have a use-case where I use
> Barebox as a DDR memory tuning/testing tool on i.MX6Q where I upload
> the image to IRAM via JTAG and execute Barebox straight out of SRAM.

I understand your usecase and I think it's worth supporting it.

So what are our options? You could run the tuning/testing completely
from the PBL. We now have console support in the PBL, so you can output
the results. You cannot do any interactive things though. We could add
simple getc() support to the PBL, but something like a shell is out of
reach. Do you need interactive input anyway?
Another possibility would be to make device tree support optional for
i.MX6. It is optional for the other i.MXes for historic reasons, so we
could make it optional for i.MX6 aswell. This would give you another
~30K which is now used by the dtb.
I'm a bit afraid that the regular-barebox-in-SRAM usecase will break
quite frequently upstream because the image gets too big or simply
because some other changes have side effects. For this reason I would
really prefer the PBL way if that's possible for you.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] arm/cpu/start.c: Avoid copying device-tree when possible

[Andrey Smirnov]

>> I'd still like to discuss the possibility of introducing a feature
>> like that to the codebase. Right now I have a use-case where I use
>> Barebox as a DDR memory tuning/testing tool on i.MX6Q where I upload
>> the image to IRAM via JTAG and execute Barebox straight out of SRAM.
>
> I understand your usecase and I think it's worth supporting it.
>
> So what are our options? You could run the tuning/testing completely
> from the PBL. We now have console support in the PBL, so you can output
> the results. You cannot do any interactive things though. We could add
> simple getc() support to the PBL, but something like a shell is out of
> reach. Do you need interactive input anyway?
> Another possibility would be to make device tree support optional for
> i.MX6. It is optional for the other i.MXes for historic reasons, so we
> could make it optional for i.MX6 aswell. This would give you another
> ~30K which is now used by the dtb.
> I'm a bit afraid that the regular-barebox-in-SRAM usecase will break
> quite frequently upstream because the image gets too big or simply
> because some other changes have side effects. For this reason I would
> really prefer the PBL way if that's possible for you.
>

Oh, I don't think I mentioned in my previous e-mail, but I do have a
working Barebox image for that case. The way I have it implemented
right now is a vanilla, single board, no-PBL, no-relocation, i.MX6Q
SabreSD Barebox image with a minimal configuration. The only things I
had to change was device tree file -- default required to much RAM to
instantiate, so I had to trim it down -- and this patch to avoid
copying FTD that is already built-in. Oh, and I also had to disable
MMU, because page table takes about 1MB or RAM(I haven't had a chance
to spend any time trying to modify MMU code to support coarser
1MB-page page table).

The image is intended to be used by EEs to do DRAM related
experiments, so I do need a shell and that was the reason I went with
full Barebox instead of trying to cram it in PBL.

We certainly can make PBL work for that use-case -- and if you recall
I already submitted getc() implementation for it once -- but the
functionality available there is very limited(and as you mentioned
full shell would be out of reach) and everything we do there would
most likely be a code duplication, so I'd rather not go that route.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] arm/cpu/start.c: Avoid copying device-tree when possible

[Sascha Hauer]

On Wed, Sep 30, 2015 at 10:56:20AM -0700, Andrey Smirnov wrote:
> >> I'd still like to discuss the possibility of introducing a feature
> >> like that to the codebase. Right now I have a use-case where I use
> >> Barebox as a DDR memory tuning/testing tool on i.MX6Q where I upload
> >> the image to IRAM via JTAG and execute Barebox straight out of SRAM.
> >
> > I understand your usecase and I think it's worth supporting it.
> >
> > So what are our options? You could run the tuning/testing completely
> > from the PBL. We now have console support in the PBL, so you can output
> > the results. You cannot do any interactive things though. We could add
> > simple getc() support to the PBL, but something like a shell is out of
> > reach. Do you need interactive input anyway?
> > Another possibility would be to make device tree support optional for
> > i.MX6. It is optional for the other i.MXes for historic reasons, so we
> > could make it optional for i.MX6 aswell. This would give you another
> > ~30K which is now used by the dtb.
> > I'm a bit afraid that the regular-barebox-in-SRAM usecase will break
> > quite frequently upstream because the image gets too big or simply
> > because some other changes have side effects. For this reason I would
> > really prefer the PBL way if that's possible for you.
> >
> 
> Oh, I don't think I mentioned in my previous e-mail, but I do have a
> working Barebox image for that case. The way I have it implemented
> right now is a vanilla, single board, no-PBL, no-relocation, i.MX6Q
> SabreSD Barebox image with a minimal configuration. The only things I
> had to change was device tree file -- default required to much RAM to
> instantiate, so I had to trim it down -- and this patch to avoid
> copying FTD that is already built-in. Oh, and I also had to disable
> MMU, because page table takes about 1MB or RAM(I haven't had a chance
> to spend any time trying to modify MMU code to support coarser
> 1MB-page page table).
> 
> The image is intended to be used by EEs to do DRAM related
> experiments, so I do need a shell and that was the reason I went with
> full Barebox instead of trying to cram it in PBL.

Ok, I see. I think your original patch is almost fine, only the test if
you need to copy or not needs adjustment. You have to test if the fdt is
membase < dtb < membase + memsize. If it is you have to copy it,
otherwise it should be fine to use it in place. Would that work?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] arm/cpu/start.c: Avoid copying device-tree when possible

[Andrey Smirnov]

>> Oh, I don't think I mentioned in my previous e-mail, but I do have a
>> working Barebox image for that case. The way I have it implemented
>> right now is a vanilla, single board, no-PBL, no-relocation, i.MX6Q
>> SabreSD Barebox image with a minimal configuration. The only things I
>> had to change was device tree file -- default required to much RAM to
>> instantiate, so I had to trim it down -- and this patch to avoid
>> copying FTD that is already built-in. Oh, and I also had to disable
>> MMU, because page table takes about 1MB or RAM(I haven't had a chance
>> to spend any time trying to modify MMU code to support coarser
>> 1MB-page page table).
>>
>> The image is intended to be used by EEs to do DRAM related
>> experiments, so I do need a shell and that was the reason I went with
>> full Barebox instead of trying to cram it in PBL.
>
> Ok, I see. I think your original patch is almost fine, only the test if
> you need to copy or not needs adjustment. You have to test if the fdt is
> membase < dtb < membase + memsize. If it is you have to copy it,
> otherwise it should be fine to use it in place. Would that work?

Sorry for dropping this conversation. Unfortunately no, I don't think
this would work since in my case 'membase' is set to start of IRAM and
'memsize' is IRAM's size and since I am placing the image in IRAM that
test would come positive and the code would try to copy. What if the
condition was set to !RELOCATABLE && !PBL_IMAGE, so that if we know if
there's no PBL and no relocation we do not copy the data? Would that
be acceptable?

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox