diff mbox series

[v6,3/5] cmd: mvebu: bubt: verify A38x target device type

Message ID 20200417153808.2468508-3-mrjoel@lixil.net
State Accepted
Commit 658854a626b56d3ff417453ac842e4486ba5139b
Headers show
Series [v6,1/5] cmd: mvebu: bubt: add A38x support | expand

Commit Message

Joel Johnson April 17, 2020, 3:38 p.m. UTC
Ensure that the device to which an image is being written includes
header information indicating boot support for the destination
device.

This is derived from the support in the SolidRun master-a38x vendor
fork.

Signed-off-by: Joel Johnson <mrjoel at lixil.net>

---

v2 changes:
  - none
v3 changes:
  - use ARRAY_SIZE instead of #define size
v4 changes:
  - none
v5 changes:
  - use if(IS_ENABLED()) for inline check instead of ifdef
v6 changes:
  - reduce #ifdef usages at the tradeoff of otherwise unneccessarily
    exposing A38x internal structs to more than A38x builds.
  - since state is exposed anyway, rename a38x_check_boot_mode to
    bubt_check_boot_mode with provision for per-target customization

    This fixes building across all mvebu targets. Exposing the needed
    a38x_* structs isn't something I'm hugely excited about, but it does
    get the build working and avoids inline #ifdefs as requested. In
    cases where the target being built (all but A38x) doesn't support
    checking the boot mode, rely on the compiler to [mostly] optimize
    the function usage away. Local tests show an increase of only 4
    bytes of rodata in such cases.

---
 cmd/mvebu/bubt.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

Comments

Stefan Roese April 22, 2020, 12:22 p.m. UTC | #1
On 17.04.20 17:38, Joel Johnson wrote:
> Ensure that the device to which an image is being written includes
> header information indicating boot support for the destination
> device.
> 
> This is derived from the support in the SolidRun master-a38x vendor
> fork.
> 
> Signed-off-by: Joel Johnson <mrjoel at lixil.net>

Reviewed-by: Stefan Roese <sr at denx.de>

Thanks,
Stefan

> ---
> 
> v2 changes:
>    - none
> v3 changes:
>    - use ARRAY_SIZE instead of #define size
> v4 changes:
>    - none
> v5 changes:
>    - use if(IS_ENABLED()) for inline check instead of ifdef
> v6 changes:
>    - reduce #ifdef usages at the tradeoff of otherwise unneccessarily
>      exposing A38x internal structs to more than A38x builds.
>    - since state is exposed anyway, rename a38x_check_boot_mode to
>      bubt_check_boot_mode with provision for per-target customization
> 
>      This fixes building across all mvebu targets. Exposing the needed
>      a38x_* structs isn't something I'm hugely excited about, but it does
>      get the build working and avoids inline #ifdefs as requested. In
>      cases where the target being built (all but A38x) doesn't support
>      checking the boot mode, rely on the compiler to [mostly] optimize
>      the function usage away. Local tests show an increase of only 4
>      bytes of rodata in such cases.
> 
> ---
>   cmd/mvebu/bubt.c | 52 ++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index b80b81c82a..be6c5869a9 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -85,7 +85,7 @@ struct mvebu_image_info {
>   	u32	encrypt_start_offset;
>   	u32	encrypt_size;
>   };
> -#elif defined(CONFIG_ARMADA_38X)	/* A38X */
> +#endif
>   
>   /* Structure of the main header, version 1 (Armada 370/38x/XP) */
>   struct a38x_main_hdr_v1 {
> @@ -107,7 +107,23 @@ struct a38x_main_hdr_v1 {
>   	u8  ext;                   /* 0x1E      */
>   	u8  checksum;              /* 0x1F      */
>   };
> -#endif
> +
> +struct a38x_boot_mode {
> +	unsigned int id;
> +	const char *name;
> +};
> +
> +/* The blockid header field values used to indicate boot device of image */
> +struct a38x_boot_mode a38x_boot_modes[] = {
> +	{ 0x4D, "i2c"  },
> +	{ 0x5A, "spi"  },
> +	{ 0x69, "uart" },
> +	{ 0x78, "sata" },
> +	{ 0x8B, "nand" },
> +	{ 0x9C, "pex"  },
> +	{ 0xAE, "mmc"  },
> +	{},
> +};
>   
>   struct bubt_dev {
>   	char name[8];
> @@ -697,7 +713,29 @@ static int check_image_header(void)
>   }
>   #endif
>   
> -static int bubt_verify(size_t image_size)
> +static int bubt_check_boot_mode(const struct bubt_dev *dst)
> +{
> +	if (IS_ENABLED(CONFIG_ARMADA_38X)) {
> +		int mode;
> +		const struct a38x_main_hdr_v1 *hdr =
> +			(struct a38x_main_hdr_v1 *)get_load_addr();
> +
> +		for (mode = 0; mode < ARRAY_SIZE(a38x_boot_modes); mode++) {
> +			if (strcmp(a38x_boot_modes[mode].name, dst->name) == 0)
> +				break;
> +		}
> +
> +		if (a38x_boot_modes[mode].id == hdr->blockid)
> +			return 0;
> +
> +		puts("Error: A38x image not built for destination device!\n");
> +		return -ENOEXEC;
> +	} else {
> +		return 0;
> +	}
> +}
> +
> +static int bubt_verify(const struct bubt_dev *dst)
>   {
>   	int err;
>   
> @@ -708,6 +746,12 @@ static int bubt_verify(size_t image_size)
>   		return err;
>   	}
>   
> +	err = bubt_check_boot_mode(dst);
> +	if (err) {
> +		printf("Error: Image boot mode verification failed\n");
> +		return err;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -829,7 +873,7 @@ int do_bubt_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   	if (!image_size)
>   		return -EIO;
>   
> -	err = bubt_verify(image_size);
> +	err = bubt_verify(dst);
>   	if (err)
>   		return err;
>   
> 


Viele Gr??e,
Stefan
diff mbox series

Patch

diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index b80b81c82a..be6c5869a9 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -85,7 +85,7 @@  struct mvebu_image_info {
 	u32	encrypt_start_offset;
 	u32	encrypt_size;
 };
-#elif defined(CONFIG_ARMADA_38X)	/* A38X */
+#endif
 
 /* Structure of the main header, version 1 (Armada 370/38x/XP) */
 struct a38x_main_hdr_v1 {
@@ -107,7 +107,23 @@  struct a38x_main_hdr_v1 {
 	u8  ext;                   /* 0x1E      */
 	u8  checksum;              /* 0x1F      */
 };
-#endif
+
+struct a38x_boot_mode {
+	unsigned int id;
+	const char *name;
+};
+
+/* The blockid header field values used to indicate boot device of image */
+struct a38x_boot_mode a38x_boot_modes[] = {
+	{ 0x4D, "i2c"  },
+	{ 0x5A, "spi"  },
+	{ 0x69, "uart" },
+	{ 0x78, "sata" },
+	{ 0x8B, "nand" },
+	{ 0x9C, "pex"  },
+	{ 0xAE, "mmc"  },
+	{},
+};
 
 struct bubt_dev {
 	char name[8];
@@ -697,7 +713,29 @@  static int check_image_header(void)
 }
 #endif
 
-static int bubt_verify(size_t image_size)
+static int bubt_check_boot_mode(const struct bubt_dev *dst)
+{
+	if (IS_ENABLED(CONFIG_ARMADA_38X)) {
+		int mode;
+		const struct a38x_main_hdr_v1 *hdr =
+			(struct a38x_main_hdr_v1 *)get_load_addr();
+
+		for (mode = 0; mode < ARRAY_SIZE(a38x_boot_modes); mode++) {
+			if (strcmp(a38x_boot_modes[mode].name, dst->name) == 0)
+				break;
+		}
+
+		if (a38x_boot_modes[mode].id == hdr->blockid)
+			return 0;
+
+		puts("Error: A38x image not built for destination device!\n");
+		return -ENOEXEC;
+	} else {
+		return 0;
+	}
+}
+
+static int bubt_verify(const struct bubt_dev *dst)
 {
 	int err;
 
@@ -708,6 +746,12 @@  static int bubt_verify(size_t image_size)
 		return err;
 	}
 
+	err = bubt_check_boot_mode(dst);
+	if (err) {
+		printf("Error: Image boot mode verification failed\n");
+		return err;
+	}
+
 	return 0;
 }
 
@@ -829,7 +873,7 @@  int do_bubt_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (!image_size)
 		return -EIO;
 
-	err = bubt_verify(image_size);
+	err = bubt_verify(dst);
 	if (err)
 		return err;