[U-Boot] [PATCH v1 2/3] rockchip: mkimage: use imagename to select spl hdr & spl size

Simon Glass sjg at chromium.org
Fri Nov 27 04:57:28 CET 2015


Hi Jeffy,

On 26 November 2015 at 16:43, Jeffy Chen <jeffy.chen at rock-chips.com> wrote:
> Our chips may have different spl size and spl header, so
> use imagename(passed by "mkimage -n") to select them now.
>
> Signed-off-by: Jeffy Chen <jeffy.chen at rock-chips.com>
> ---
>
>  tools/rkcommon.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  tools/rkcommon.h | 32 +++++++++++++++++++++++++-
>  tools/rkimage.c  |  4 +++-
>  tools/rksd.c     | 21 +++++++----------
>  tools/rkspi.c    | 18 +++++++--------
>  5 files changed, 117 insertions(+), 27 deletions(-)
>

Looks good - a few minor improvement requests below...

> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index 9e2173f..d36fd6b 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -40,16 +40,81 @@ struct header0_info {
>         uint8_t reserved2[2];
>  };
>
> +/**
> + * struct spl_info - spl info for each chip
> + *
> + * @imagename:         Image name(passed by "mkimage -n")
> + * @spl_hdr:           Boot ROM requires a 4-bytes spl header
> + * @spl_size:          Spl size(include extra 4-bytes spl header)
> + */
> +struct spl_info {
> +       const char *imagename;
> +       const char *spl_hdr;
> +       const uint32_t spl_size;
> +};
> +
> +static struct spl_info spl_infos[] = {
> +       { "rk3036", "RK30", 0x1000 },
> +       { "rk3288", "RK32", 0x8000 },
> +};
> +
>  static unsigned char rc4_key[16] = {
>         124, 78, 3, 4, 85, 5, 9, 7,
>         45, 44, 123, 56, 23, 13, 23, 17
>  };
>
> -int rkcommon_set_header(void *buf, uint file_size)
> +int rkcommon_check_params(struct image_tool_params *params)
> +{
> +       int i = 0;

blank line here, and you can drop the '= 0'.

> +       for (i = 0; i < ARRAY_SIZE(spl_infos); i++)
> +               if (!strncmp(params->imagename, spl_infos[i].imagename, 6))
> +                       return 0;
> +
> +       fprintf(stderr, "ERROR: imagename (%s) is not supported!\n",
> +               strlen(params->imagename) > 0 ? params->imagename : "NULL");
> +
> +       fprintf(stderr, "Available imagename:");
> +       for (i = 0; i < ARRAY_SIZE(spl_infos); i++)
> +               fprintf(stderr, "\t%s", spl_infos[i].imagename);
> +       fprintf(stderr, "\n");
> +
> +       return -1;
> +}
> +
> +static struct spl_info *rkcommon_get_spl_info(char *imagename)
> +{
> +       int i = 0;
> +       for (i = 0; i < ARRAY_SIZE(spl_infos); i++)
> +               if (!strncmp(imagename, spl_infos[i].imagename, 6))
> +                       return spl_infos + i;

It seems unnecessary to scan again, but I suppose the only alternative
is to store rockchip-specific data in struct image_tool_params.

So what you have seems reasonable, but please can you put the loop in
a common function?

> +
> +       /**

/*

Please only use /** for comments for a function or a struct.

> +        * should not reach here, unsupported imagename will
> +        * return error in rkcommon_check_params.
> +        */
> +       return spl_infos;
> +}
> +
> +const char *rkcommon_get_spl_hdr(struct image_tool_params *params)
> +{
> +       struct spl_info *info = rkcommon_get_spl_info(params->imagename);
> +
> +       return info->spl_hdr;
> +}
> +
> +int rkcommon_get_spl_size(struct image_tool_params *params)
> +{
> +       struct spl_info *info = rkcommon_get_spl_info(params->imagename);
> +
> +       return info->spl_size;
> +}
> +
> +int rkcommon_set_header(void *buf, uint file_size,
> +                       struct image_tool_params *params)
>  {
>         struct header0_info *hdr;
>
> -       if (file_size > RK_MAX_CODE1_SIZE)
> +       if (file_size > rkcommon_get_spl_size(params))
>                 return -ENOSPC;
>
>         memset(buf,  '\0', RK_INIT_OFFSET * RK_BLK_SIZE);
> diff --git a/tools/rkcommon.h b/tools/rkcommon.h
> index 0fc1e96..c69540f 100644
> --- a/tools/rkcommon.h
> +++ b/tools/rkcommon.h
> @@ -12,9 +12,38 @@ enum {
>         RK_BLK_SIZE             = 512,
>         RK_INIT_OFFSET          = 4,
>         RK_MAX_BOOT_SIZE        = 512 << 10,
> +       RK_SPL_HDR_START        = RK_INIT_OFFSET * RK_BLK_SIZE,
> +       RK_SPL_HDR_SIZE         = 4,
> +       RK_SPL_START            = RK_SPL_HDR_START + RK_SPL_HDR_SIZE,
> +       RK_IMAGE_HEADER_LEN     = RK_SPL_START,
>  };
>
>  /**
> + * rkcommon_check_params() - check params
> + *
> + * @return 0 if OK, -1 if ERROR.
> + */
> +int rkcommon_check_params(struct image_tool_params *params);
> +
> +/**
> + * rkcommon_get_spl_hdr() - get 4-bytes spl hdr for a Rockchip boot image
> + *
> + * Rockchip's bootrom requires the spl loader to start with a 4-bytes
> + * header. The content of this header depends on the chip type.
> + */
> +const char *rkcommon_get_spl_hdr(struct image_tool_params *params);
> +
> +/**
> + * rkcommon_get_spl_size() - get spl size for a Rockchip boot image
> + *
> + * Different chip may have different sram size. And if we want to jump
> + * back to the bootrom after spl, we may need to reserve some sram space
> + * for the bootrom.
> + * The spl loader size should be sram size minus reserved size(if needed)
> + */
> +int rkcommon_get_spl_size(struct image_tool_params *params);
> +
> +/**
>   * rkcommon_set_header() - set up the header for a Rockchip boot image
>   *
>   * This sets up a 2KB header which can be interpreted by the Rockchip boot ROM.
> @@ -23,6 +52,7 @@ enum {
>   * @file_size: Size of the file we want the boot ROM to load, in bytes
>   * @return 0 if OK, -ENOSPC if too large
>   */
> -int rkcommon_set_header(void *buf, uint file_size);
> +int rkcommon_set_header(void *buf, uint file_size,
> +                       struct image_tool_params *params);
>
>  #endif
> diff --git a/tools/rkimage.c b/tools/rkimage.c
> index 7b292f4..f9fdcfa 100644
> --- a/tools/rkimage.c
> +++ b/tools/rkimage.c
> @@ -9,6 +9,7 @@
>
>  #include "imagetool.h"
>  #include <image.h>
> +#include "rkcommon.h"
>
>  static uint32_t header;
>
> @@ -30,7 +31,8 @@ static void rkimage_print_header(const void *buf)
>  static void rkimage_set_header(void *buf, struct stat *sbuf, int ifd,
>                                struct image_tool_params *params)
>  {
> -       memcpy(buf, "RK32", 4);
> +       memcpy(buf + RK_SPL_HDR_START, rkcommon_get_spl_hdr(params),
> +              RK_SPL_HDR_SIZE);
>  }
>
>  static int rkimage_extract_subimage(void *buf, struct image_tool_params *params)
> diff --git a/tools/rksd.c b/tools/rksd.c
> index 39f5c75..5d0c400 100644
> --- a/tools/rksd.c
> +++ b/tools/rksd.c
> @@ -13,17 +13,11 @@
>  #include "mkimage.h"
>  #include "rkcommon.h"
>
> -enum {
> -       RKSD_SPL_HDR_START      = RK_INIT_OFFSET * RK_BLK_SIZE,
> -       RKSD_SPL_START          = RKSD_SPL_HDR_START + 4,
> -       RKSD_HEADER_LEN         = RKSD_SPL_START,
> -};
> -
> -static char dummy_hdr[RKSD_HEADER_LEN];
> +static char dummy_hdr[RK_IMAGE_HEADER_LEN];
>
>  static int rksd_check_params(struct image_tool_params *params)
>  {
> -       return 0;
> +       return rkcommon_check_params(params);

Can you drop this function and use rkcommon_check_params in the
U_BOOT_IMAGE_TYPE below? Same with rkspi.

>  }
>
>  static int rksd_verify_header(unsigned char *buf,  int size,
> @@ -42,15 +36,16 @@ static void rksd_set_header(void *buf,  struct stat *sbuf,  int ifd,
>         unsigned int size;
>         int ret;
>
> -       size = params->file_size - RKSD_SPL_HDR_START;
> -       ret = rkcommon_set_header(buf, size);
> +       size = params->file_size - RK_SPL_HDR_START;
> +       ret = rkcommon_set_header(buf, size, params);
>         if (ret) {
>                 /* TODO(sjg at chromium.org): This method should return an error */
>                 printf("Warning: SPL image is too large (size %#x) and will not boot\n",
>                        size);
>         }
>
> -       memcpy(buf + RKSD_SPL_HDR_START, "RK32", 4);
> +       memcpy(buf + RK_SPL_HDR_START, rkcommon_get_spl_hdr(params),
> +              RK_SPL_HDR_SIZE);
>  }
>
>  static int rksd_extract_subimage(void *buf,  struct image_tool_params *params)
> @@ -72,7 +67,7 @@ static int rksd_vrec_header(struct image_tool_params *params,
>  {
>         int pad_size;
>
> -       pad_size = RKSD_SPL_HDR_START + RK_MAX_CODE1_SIZE;
> +       pad_size = RK_SPL_HDR_START + rkcommon_get_spl_size(params);
>         debug("pad_size %x\n", pad_size);
>
>         return pad_size - params->file_size;
> @@ -84,7 +79,7 @@ static int rksd_vrec_header(struct image_tool_params *params,
>  U_BOOT_IMAGE_TYPE(
>         rksd,
>         "Rockchip SD Boot Image support",
> -       RKSD_HEADER_LEN,
> +       RK_IMAGE_HEADER_LEN,
>         dummy_hdr,
>         rksd_check_params,
>         rksd_verify_header,
> diff --git a/tools/rkspi.c b/tools/rkspi.c
> index eb8119b..8c4d649 100644
> --- a/tools/rkspi.c
> +++ b/tools/rkspi.c
> @@ -14,17 +14,14 @@
>  #include "rkcommon.h"
>
>  enum {
> -       RKSPI_SPL_HDR_START     = RK_INIT_OFFSET * RK_BLK_SIZE,
> -       RKSPI_SPL_START         = RKSPI_SPL_HDR_START + 4,
> -       RKSPI_HEADER_LEN        = RKSPI_SPL_START,
>         RKSPI_SECT_LEN          = RK_BLK_SIZE * 4,
>  };
>
> -static char dummy_hdr[RKSPI_HEADER_LEN];
> +static char dummy_hdr[RK_IMAGE_HEADER_LEN];
>
>  static int rkspi_check_params(struct image_tool_params *params)
>  {
> -       return 0;
> +       return rkcommon_check_params(params);
>  }
>
>  static int rkspi_verify_header(unsigned char *buf, int size,
> @@ -45,7 +42,7 @@ static void rkspi_set_header(void *buf, struct stat *sbuf, int ifd,
>         int ret;
>
>         size = params->orig_file_size;
> -       ret = rkcommon_set_header(buf, size);
> +       ret = rkcommon_set_header(buf, size, params);
>         debug("size %x\n", size);
>         if (ret) {
>                 /* TODO(sjg at chromium.org): This method should return an error */
> @@ -53,7 +50,8 @@ static void rkspi_set_header(void *buf, struct stat *sbuf, int ifd,
>                        size);
>         }
>
> -       memcpy(buf + RKSPI_SPL_HDR_START, "RK32", 4);
> +       memcpy(buf + RK_SPL_HDR_START, rkcommon_get_spl_hdr(params),
> +              RK_SPL_HDR_SIZE);
>
>         /*
>          * Spread the image out so we only use the first 2KB of each 4KB
> @@ -89,12 +87,12 @@ static int rkspi_vrec_header(struct image_tool_params *params,
>  {
>         int pad_size;
>
> -       pad_size = (RK_MAX_CODE1_SIZE + 0x7ff) / 0x800 * 0x800;
> +       pad_size = (rkcommon_get_spl_size(params) + 0x7ff) / 0x800 * 0x800;
>         params->orig_file_size = pad_size;
>
>         /* We will double the image size due to the SPI format */
>         pad_size *= 2;
> -       pad_size += RKSPI_SPL_HDR_START;
> +       pad_size += RK_SPL_HDR_START;
>         debug("pad_size %x\n", pad_size);
>
>         return pad_size - params->file_size;
> @@ -106,7 +104,7 @@ static int rkspi_vrec_header(struct image_tool_params *params,
>  U_BOOT_IMAGE_TYPE(
>         rkspi,
>         "Rockchip SPI Boot Image support",
> -       RKSPI_HEADER_LEN,
> +       RK_IMAGE_HEADER_LEN,
>         dummy_hdr,
>         rkspi_check_params,
>         rkspi_verify_header,
> --
> 2.1.4
>
>

Regards,
Simon


More information about the U-Boot mailing list