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

2015-11-26 Thread Simon Glass 
Hi Jeffy,

On 26 November 2015 at 16:43, Jeffy Chen  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 
> ---
>
>  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

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

2015-11-26 Thread Jeffy Chen 
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 
---

 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(-)

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;
+   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;
+
+   /**
+* 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
+++