[PATCH v3 0/4] mkimage: sunxi_egon: add riscv support

[PATCH v3 0/4] mkimage: sunxi_egon: add riscv support

[Samuel Holland]

This patchset updates mkimage -T sunxi_egon to be able to generate
an eGON.BT0 image for Allwinner RISC-V SoCs (e.g. D1).

In addition, to keep the compatibility, it will still consider the
architecture to be ARM when no architecture is specified.

This v3 is a minor update to Icenowy's patch series, which I have
also tested. Since the TOC0 patch series touches the same lines in
Makefile.spl, it depends on this series.

Changes in v3:
- Factor out an egon_get_arch() function as suggested by Andre

Icenowy Zheng (4):
  mkimage: add a flag to describe whether -A is specified
  mkimage: sunxi_egon: refactor for multi-architecture support
  mkimage: sunxi_egon: add support for riscv
  sunxi: specify architecture when generating SPL boot image

 scripts/Makefile.spl |  2 +-
 tools/imagetool.h    |  1 +
 tools/mkimage.c      |  1 +
 tools/sunxi_egon.c   | 73 +++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 69 insertions(+), 8 deletions(-)

-- 
2.32.0

[PATCH v3 1/4] mkimage: add a flag to describe whether -A is specified

[Samuel Holland]

From: Icenowy Zheng <icenowy@aosc.io>

The sunxi_egon type used to take no -A argument (because we assume sunxi
targets are all ARM). However, as Allwinner D1 appears as the first
RISC-V sunxi target, we need to support -A; in addition, as external
projects rely on U-Boot mkimage to generate sunxi eGON.BT0 header, we
need to keep compatibility with command line without -A.

As the default value of arch in mkimage is not proper (IH_ARCH_PPC
instead of IH_ARCH_INVALID), to keep more compatibility, add an Aflag
field to image parameters to describe whether an architecture is
explicitly specified.

Reviewed-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v1)

 tools/imagetool.h | 1 +
 tools/mkimage.c   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tools/imagetool.h b/tools/imagetool.h
index e229a34ffc..5dc28312c2 100644
--- a/tools/imagetool.h
+++ b/tools/imagetool.h
@@ -51,6 +51,7 @@ struct image_tool_params {
 	int pflag;
 	int vflag;
 	int xflag;
+	int Aflag;
 	int skipcpy;
 	int os;
 	int arch;
diff --git a/tools/mkimage.c b/tools/mkimage.c
index fbe883ce36..cfd97b046c 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -171,6 +171,7 @@ static void process_args(int argc, char **argv)
 				show_valid_options(IH_ARCH);
 				usage("Invalid architecture");
 			}
+			params.Aflag = 1;
 			break;
 		case 'b':
 			if (add_content(IH_TYPE_FLATDT, optarg)) {
-- 
2.32.0

[PATCH v3 2/4] mkimage: sunxi_egon: refactor for multi-architecture support

[Samuel Holland]

From: Icenowy Zheng <icenowy@aosc.io>

Refactor some functions in mkimage sunxi_egon type, in order to prepare
for adding support for more CPU architectures (e.g. RISC-V). In
addition, compatibility for operation w/o specified architecture is
kept, in this case the architecture is assumed as ARM.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
- Factor out an egon_get_arch() function as suggested by Andre

 tools/sunxi_egon.c | 50 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/tools/sunxi_egon.c b/tools/sunxi_egon.c
index a5299eb6a1..e1c945a12f 100644
--- a/tools/sunxi_egon.c
+++ b/tools/sunxi_egon.c
@@ -14,9 +14,28 @@
  */
 #define PAD_SIZE			8192
 
+static int egon_get_arch(struct image_tool_params *params)
+{
+	if (params->Aflag)
+		return params->arch;
+
+	/* For compatibility, assume ARM when no architecture specified */
+	return IH_ARCH_ARM;
+}
+
 static int egon_check_params(struct image_tool_params *params)
 {
-	/* We just need a binary image file. */
+	/*
+	 * Check whether the architecture is supported.
+	 */
+	switch (egon_get_arch(params)) {
+	case IH_ARCH_ARM:
+		break;
+	default:
+		return EXIT_FAILURE;
+	}
+
+	/* We need a binary image file. */
 	return !params->dflag;
 }
 
@@ -26,9 +45,18 @@ static int egon_verify_header(unsigned char *ptr, int image_size,
 	const struct boot_file_head *header = (void *)ptr;
 	uint32_t length;
 
-	/* First 4 bytes must be an ARM branch instruction. */
-	if ((le32_to_cpu(header->b_instruction) & 0xff000000) != 0xea000000)
-		return EXIT_FAILURE;
+	/*
+	 * First 4 bytes must be a branch instruction of the corresponding
+	 * architecture.
+	 */
+	switch (egon_get_arch(params)) {
+	case IH_ARCH_ARM:
+		if ((le32_to_cpu(header->b_instruction) & 0xff000000) != 0xea000000)
+			return EXIT_FAILURE;
+		break;
+	default:
+		return EXIT_FAILURE; /* Unknown architecture */
+	}
 
 	if (memcmp(header->magic, BOOT0_MAGIC, sizeof(header->magic)))
 		return EXIT_FAILURE;
@@ -77,9 +105,17 @@ static void egon_set_header(void *buf, struct stat *sbuf, int infd,
 	uint32_t checksum = 0, value;
 	int i;
 
-	/* Generate an ARM branch instruction to jump over the header. */
-	value = 0xea000000 | (sizeof(struct boot_file_head) / 4 - 2);
-	header->b_instruction = cpu_to_le32(value);
+	/*
+	 * Different architectures need different first instruction to
+	 * branch to the body.
+	 */
+	switch (egon_get_arch(params)) {
+	case IH_ARCH_ARM:
+		/* Generate an ARM branch instruction to jump over the header. */
+		value = 0xea000000 | (sizeof(struct boot_file_head) / 4 - 2);
+		header->b_instruction = cpu_to_le32(value);
+		break;
+	}
 
 	memcpy(header->magic, BOOT0_MAGIC, sizeof(header->magic));
 	header->check_sum = cpu_to_le32(BROM_STAMP_VALUE);
-- 
2.32.0

[PATCH v3 3/4] mkimage: sunxi_egon: add support for riscv

[Samuel Holland]

From: Icenowy Zheng <icenowy@aosc.io>

There's now a sun20i family in sunxi, which uses RISC-V CPU.

Add support for making eGON.BT0 image for RISC-V.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v1)

 tools/sunxi_egon.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/sunxi_egon.c b/tools/sunxi_egon.c
index e1c945a12f..06e8d67ebc 100644
--- a/tools/sunxi_egon.c
+++ b/tools/sunxi_egon.c
@@ -30,6 +30,7 @@ static int egon_check_params(struct image_tool_params *params)
 	 */
 	switch (egon_get_arch(params)) {
 	case IH_ARCH_ARM:
+	case IH_ARCH_RISCV:
 		break;
 	default:
 		return EXIT_FAILURE;
@@ -54,6 +55,10 @@ static int egon_verify_header(unsigned char *ptr, int image_size,
 		if ((le32_to_cpu(header->b_instruction) & 0xff000000) != 0xea000000)
 			return EXIT_FAILURE;
 		break;
+	case IH_ARCH_RISCV:
+		if ((le32_to_cpu(header->b_instruction) & 0x00000fff) != 0x0000006f)
+			return EXIT_FAILURE;
+		break;
 	default:
 		return EXIT_FAILURE; /* Unknown architecture */
 	}
@@ -115,6 +120,24 @@ static void egon_set_header(void *buf, struct stat *sbuf, int infd,
 		value = 0xea000000 | (sizeof(struct boot_file_head) / 4 - 2);
 		header->b_instruction = cpu_to_le32(value);
 		break;
+	case IH_ARCH_RISCV:
+		/*
+		 * Generate a RISC-V JAL instruction with rd=x0
+		 * (pseudo instruction J, jump without side effects).
+		 *
+		 * The following weird bit operation maps imm[20]
+		 * to inst[31], imm[10:1] to inst[30:21],
+		 * imm[11] to inst[20], imm[19:12] to inst[19:12],
+		 * and imm[0] is dropped (because 1-byte RISC-V instruction
+		 * is not allowed).
+		 */
+		value = 0x0000006f |
+			((sizeof(struct boot_file_head) & 0x00100000) << 11) |
+			((sizeof(struct boot_file_head) & 0x000007fe) << 20) |
+			((sizeof(struct boot_file_head) & 0x00000800) << 9) |
+			((sizeof(struct boot_file_head) & 0x000ff000) << 0);
+		header->b_instruction = cpu_to_le32(value);
+		break;
 	}
 
 	memcpy(header->magic, BOOT0_MAGIC, sizeof(header->magic));
-- 
2.32.0

[PATCH v3 4/4] sunxi: specify architecture when generating SPL boot image

[Samuel Holland]

From: Icenowy Zheng <icenowy@aosc.io>

As mkimage -T sunxi_egon now gains support for -A parameter, specify the
architecture when generating SPL boot image for sunxi.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v1)

 scripts/Makefile.spl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
index 6f26eb1fa1..4cc23799db 100644
--- a/scripts/Makefile.spl
+++ b/scripts/Makefile.spl
@@ -411,7 +411,7 @@ endif
 $(obj)/$(SPL_BIN).sfp: $(obj)/$(SPL_BIN).bin FORCE
 	$(call if_changed,mkimage)
 
-MKIMAGEFLAGS_sunxi-spl.bin = -T sunxi_egon \
+MKIMAGEFLAGS_sunxi-spl.bin = -A $(ARCH) -T sunxi_egon \
 	-n $(CONFIG_DEFAULT_DEVICE_TREE)
 
 OBJCOPYFLAGS_u-boot-spl-dtb.hex := -I binary -O ihex --change-address=$(CONFIG_SPL_TEXT_BASE)
-- 
2.32.0

Re: [PATCH v3 1/4] mkimage: add a flag to describe whether -A is specified

[Andre Przywara]

On Thu, 14 Oct 2021 20:53:04 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi,

> From: Icenowy Zheng <icenowy@aosc.io>
> 
> The sunxi_egon type used to take no -A argument (because we assume sunxi
> targets are all ARM). However, as Allwinner D1 appears as the first
> RISC-V sunxi target, we need to support -A; in addition, as external
> projects rely on U-Boot mkimage to generate sunxi eGON.BT0 header, we
> need to keep compatibility with command line without -A.
> 
> As the default value of arch in mkimage is not proper (IH_ARCH_PPC
> instead of IH_ARCH_INVALID), to keep more compatibility, add an Aflag
> field to image parameters to describe whether an architecture is
> explicitly specified.

So there was some nitpic^Wdiscussion about the uppercase in the variable
name, where Icenowy proposed arch_flag instead. Shall this be changed
still? If that's the only thing, I am happy to do it while merging.
Btw: Shall this (and the TOC0) series go through the sunxi tree?

Cheers,
Andre


> 
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> (no changes since v1)
> 
>  tools/imagetool.h | 1 +
>  tools/mkimage.c   | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/tools/imagetool.h b/tools/imagetool.h
> index e229a34ffc..5dc28312c2 100644
> --- a/tools/imagetool.h
> +++ b/tools/imagetool.h
> @@ -51,6 +51,7 @@ struct image_tool_params {
>  	int pflag;
>  	int vflag;
>  	int xflag;
> +	int Aflag;
>  	int skipcpy;
>  	int os;
>  	int arch;
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index fbe883ce36..cfd97b046c 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -171,6 +171,7 @@ static void process_args(int argc, char **argv)
>  				show_valid_options(IH_ARCH);
>  				usage("Invalid architecture");
>  			}
> +			params.Aflag = 1;
>  			break;
>  		case 'b':
>  			if (add_content(IH_TYPE_FLATDT, optarg)) {

Re: [PATCH v3 1/4] mkimage: add a flag to describe whether -A is specified

[Samuel Holland]

On 10/19/21 5:49 AM, Andre Przywara wrote:
> On Thu, 14 Oct 2021 20:53:04 -0500
> Samuel Holland <samuel@sholland.org> wrote:
> 
> Hi,
> 
>> From: Icenowy Zheng <icenowy@aosc.io>
>>
>> The sunxi_egon type used to take no -A argument (because we assume sunxi
>> targets are all ARM). However, as Allwinner D1 appears as the first
>> RISC-V sunxi target, we need to support -A; in addition, as external
>> projects rely on U-Boot mkimage to generate sunxi eGON.BT0 header, we
>> need to keep compatibility with command line without -A.
>>
>> As the default value of arch in mkimage is not proper (IH_ARCH_PPC
>> instead of IH_ARCH_INVALID), to keep more compatibility, add an Aflag
>> field to image parameters to describe whether an architecture is
>> explicitly specified.
> 
> So there was some nitpic^Wdiscussion about the uppercase in the variable
> name, where Icenowy proposed arch_flag instead. Shall this be changed
> still? If that's the only thing, I am happy to do it while merging.

My understanding of the discussion was that Aflag made the most sense
for consistency, and it was acceptable for that reason, which is why I
left it unchanged in v3.

Regards,
Samuel

> Btw: Shall this (and the TOC0) series go through the sunxi tree?
> 
> Cheers,
> Andre
> 
> 
>>
>> Reviewed-by: Tom Rini <trini@konsulko.com>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> (no changes since v1)
>>
>>  tools/imagetool.h | 1 +
>>  tools/mkimage.c   | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/tools/imagetool.h b/tools/imagetool.h
>> index e229a34ffc..5dc28312c2 100644
>> --- a/tools/imagetool.h
>> +++ b/tools/imagetool.h
>> @@ -51,6 +51,7 @@ struct image_tool_params {
>>  	int pflag;
>>  	int vflag;
>>  	int xflag;
>> +	int Aflag;
>>  	int skipcpy;
>>  	int os;
>>  	int arch;
>> diff --git a/tools/mkimage.c b/tools/mkimage.c
>> index fbe883ce36..cfd97b046c 100644
>> --- a/tools/mkimage.c
>> +++ b/tools/mkimage.c
>> @@ -171,6 +171,7 @@ static void process_args(int argc, char **argv)
>>  				show_valid_options(IH_ARCH);
>>  				usage("Invalid architecture");
>>  			}
>> +			params.Aflag = 1;
>>  			break;
>>  		case 'b':
>>  			if (add_content(IH_TYPE_FLATDT, optarg)) {
> 

Re: [PATCH v3 1/4] mkimage: add a flag to describe whether -A is specified

[Simon Glass]

Hi,

On Tue, 19 Oct 2021 at 07:17, Samuel Holland <samuel@sholland.org> wrote:
>
> On 10/19/21 5:49 AM, Andre Przywara wrote:
> > On Thu, 14 Oct 2021 20:53:04 -0500
> > Samuel Holland <samuel@sholland.org> wrote:
> >
> > Hi,
> >
> >> From: Icenowy Zheng <icenowy@aosc.io>
> >>
> >> The sunxi_egon type used to take no -A argument (because we assume sunxi
> >> targets are all ARM). However, as Allwinner D1 appears as the first
> >> RISC-V sunxi target, we need to support -A; in addition, as external
> >> projects rely on U-Boot mkimage to generate sunxi eGON.BT0 header, we
> >> need to keep compatibility with command line without -A.
> >>
> >> As the default value of arch in mkimage is not proper (IH_ARCH_PPC
> >> instead of IH_ARCH_INVALID), to keep more compatibility, add an Aflag
> >> field to image parameters to describe whether an architecture is
> >> explicitly specified.
> >
> > So there was some nitpic^Wdiscussion about the uppercase in the variable
> > name, where Icenowy proposed arch_flag instead. Shall this be changed
> > still? If that's the only thing, I am happy to do it while merging.
>
> My understanding of the discussion was that Aflag made the most sense
> for consistency, and it was acceptable for that reason, which is why I
> left it unchanged in v3.

That might have been my suggestion of naming the variable after its
meaning rather than the single-character flag.

But I think Tom is happy with it as is, so let's go with it.


- Simon

>
> Regards,
> Samuel
>
> > Btw: Shall this (and the TOC0) series go through the sunxi tree?
> >
> > Cheers,
> > Andre
> >
> >
> >>
> >> Reviewed-by: Tom Rini <trini@konsulko.com>
> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >>  tools/imagetool.h | 1 +
> >>  tools/mkimage.c   | 1 +
> >>  2 files changed, 2 insertions(+)
> >>
> >> diff --git a/tools/imagetool.h b/tools/imagetool.h
> >> index e229a34ffc..5dc28312c2 100644
> >> --- a/tools/imagetool.h
> >> +++ b/tools/imagetool.h
> >> @@ -51,6 +51,7 @@ struct image_tool_params {
> >>      int pflag;
> >>      int vflag;
> >>      int xflag;
> >> +    int Aflag;
> >>      int skipcpy;
> >>      int os;
> >>      int arch;
> >> diff --git a/tools/mkimage.c b/tools/mkimage.c
> >> index fbe883ce36..cfd97b046c 100644
> >> --- a/tools/mkimage.c
> >> +++ b/tools/mkimage.c
> >> @@ -171,6 +171,7 @@ static void process_args(int argc, char **argv)
> >>                              show_valid_options(IH_ARCH);
> >>                              usage("Invalid architecture");
> >>                      }
> >> +                    params.Aflag = 1;
> >>                      break;
> >>              case 'b':
> >>                      if (add_content(IH_TYPE_FLATDT, optarg)) {
> >
>