[PATCH v5 3/5] cmd: mvebu: bubt: verify A38x target device type

[Stefan Roese]

Hi Joel,

On 24.03.20 15:23, 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>
> 
> ---
> 
> 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
> 
> ---
>   cmd/mvebu/bubt.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index b80b81c82a..b03924ca06 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -107,6 +107,24 @@ struct a38x_main_hdr_v1 {
>   	u8  ext;                   /* 0x1E      */
>   	u8  checksum;              /* 0x1F      */
>   };
> +
> +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"  },
> +	{},
> +};
> +
>   #endif
>   
>   struct bubt_dev {
> @@ -644,6 +662,23 @@ static int check_image_header(void)
>   	return 0;
>   }
>   #elif defined(CONFIG_ARMADA_38X)
> +static int a38x_check_boot_mode(const struct bubt_dev *dst)
> +{
> +	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;
> +
> +	return -ENOEXEC;
> +}
> +
>   static size_t a38x_header_size(const struct a38x_main_hdr_v1 *h)
>   {
>   	if (h->version == 1)
> @@ -697,7 +732,7 @@ static int check_image_header(void)
>   }
>   #endif
>   
> -static int bubt_verify(size_t image_size)
> +static int bubt_verify(const struct bubt_dev *dst)
>   {
>   	int err;
>   
> @@ -708,6 +743,14 @@ static int bubt_verify(size_t image_size)
>   		return err;
>   	}
>   
> +	if (IS_ENABLED(CONFIG_ARMADA_38X)) {
> +		err = a38x_check_boot_mode(dst);

Unfortunately this does not work. I'm getting this warning when
compiling for A37xx boards:

cmd/mvebu/bubt.c: In function ‘bubt_verify’:
cmd/mvebu/bubt.c:758:9: warning: implicit declaration of function 
‘a38x_check_boot_mode’ [-Wimplicit-function-declaration]
    err = a38x_check_boot_mode(dst);
          ^~~~~~~~~~~~~~~~~~~~

So we can't use #ifdef in the function desclaration and IS_ENABLED()
in the "if" line above.

It seems that we need to switch back to the #ifdef in the function
description again. Or unconditianally compile a38x_check_boot_mode().
This might be even better. Please give it a try.

Thanks,
Stefan

> +		if (err) {
> +			puts("Error: image not built for destination device!\n");
> +			return err;
> +		}
> +	}
> +
>   	return 0;
>   }
>   
> @@ -829,7 +872,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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de