On 27/01/17 21:29, Simon Glass wrote:

Hi Andre,

On 19 January 2017 at 18:53, Andre Przywara andre.przywara@arm.com wrote:

...

Currently the SPL FIT loader uses the spl_fit_select_fdt() function to find the offset to the right DTB within the FIT image. For this it iterates over all subnodes of the /configuration node in the FIT tree and compares all "description" strings therein using a board specific matching function. If that finds a match, it uses the string in the "fdt" property of that subnode to locate the matching subnode in the /images node, which points to the DTB data. Now this works very well, but is quite specific to cover this particular use case. To open up the door for a more generic usage, let's split this function into:

1. a function that just returns the node offset for the matching configuration node (spl_fit_find_config_node())
2. a function that returns the image data any given property in a given configuration node points to, additionally using a given index into a possbile list of strings (spl_fit_select_index())

This allows us to replace the specific function above by asking for the image the _first string of the "fdt" property_ in the matching configuration subnode points to.

This patch introduces no functional changes, it just refactors the code to allow reusing it later.

(diff is overly clever here and produces a hard-to-read patch, so I recommend to throw a look at the result instead).

Signed-off-by: Andre Przywara andre.przywara@arm.com

common/spl/spl_fit.c | 82 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 51 insertions(+), 31 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

Thanks a lot!

Please check below.

....

...

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index aae556f..c4e2f02 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -22,13 +22,11 @@ static ulong fdt_getprop_u32(const void *fdt, int node, const char *prop) return fdt32_to_cpu(*cell); }

-static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp) +static int spl_fit_find_config_node(const void *fdt) {

•   const char *name, *fdt_name;
  

•   int conf, node, fdt_node;
  

•   int len;
  

•   const char *name;
  

•   int conf, node, len;
  
  

•   *fdt_offsetp = 0;
    conf = fdt_path_offset(fdt, FIT_CONFS_PATH);
    if (conf < 0) {
            debug("%s: Cannot find /configurations node: %d\n", __func__,
  

@@ -50,39 +48,60 @@ static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp) continue;

            debug("Selecting config '%s'", name);

•           fdt_name = fdt_getprop(fdt, node, FIT_FDT_PROP, &len);
  

•           if (!fdt_name) {
  

•                   debug("%s: Cannot find fdt name property: %d\n",
  

•                         __func__, len);
  

•                   return -EINVAL;
  

•           }
  
  

•           debug(", fdt '%s'\n", fdt_name);
  

•           fdt_node = fdt_subnode_offset(fdt, images, fdt_name);
  

•           if (fdt_node < 0) {
  

•                   debug("%s: Cannot find fdt node '%s': %d\n",
  

•                         __func__, fdt_name, fdt_node);
  

•                   return -EINVAL;
  

•           return node;
  

•   }
  

•   return -1;
  

+}

+static int spl_fit_select_index(const void *fit, int images, int *offsetp,

•                           const char *type, int index)
  

+{

•   const char *name, *img_name;
  

•   int node, conf_node;
  

•   int len, i;
  

•   *offsetp = 0;
  

•   conf_node = spl_fit_find_config_node(fit);
  

•   if (conf_node < 0) {
  

+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

•           printf("No matching DT out of these options:\n");
  

•           for (node = fdt_first_subnode(fit, conf_node);
  

•                node >= 0;
  

•                node = fdt_next_subnode(fit, node)) {
  

•                   name = fdt_getprop(fit, node, "description", &len);
  

•                   printf("   %s\n", name);
            }
  

+#endif

•           return -ENOENT;
  

•   }
  
  

•           *fdt_offsetp = fdt_getprop_u32(fdt, fdt_node, "data-offset");
  

•           len = fdt_getprop_u32(fdt, fdt_node, "data-size");
  

•           debug("FIT: Selected '%s'\n", name);
  

•   img_name = fdt_getprop(fit, conf_node, type, &len);
  

•   if (!img_name) {
  

•           debug("cannot find property '%s': %d\n", type, len);
  

•           return -EINVAL;
  

•   }
  
  

•           return len;
  

•   for (i = 0; i < index; i++) {
  

•           img_name = strchr(img_name, '\0') + 1;
  

Don't you need to check against strchr() returning NULL?

I think in a valid DT a string is always terminated by a NUL, but you are right that this code is broken. There is no double NUL at the end as I erroneously thought, instead we have to compare against the "len" value from above.

Thanks for spotting this!

Cheers, Andre.

...

•           if (*img_name == '\0') {
  

•                   debug("no string for index %d\n", index);
  

•                   return -E2BIG;
  

•           }
    }
  
  

-#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

•   printf("No matching DT out of these options:\n");
  

•   for (node = fdt_first_subnode(fdt, conf);
  

•        node >= 0;
  

•        node = fdt_next_subnode(fdt, node)) {
  

•           name = fdt_getprop(fdt, node, "description", &len);
  

•           printf("   %s\n", name);
  

•   debug("%s: '%s'\n", type, img_name);
  

•   node = fdt_subnode_offset(fit, images, img_name);
  

•   if (node < 0) {
  

•           debug("cannot find image node '%s': %d\n", img_name, node);
  

•           return -EINVAL;
    }
  

-#endif

•   return -ENOENT;
  

•   *offsetp = fdt_getprop_u32(fit, node, "data-offset");
  

•   len = fdt_getprop_u32(fit, node, "data-size");
  

•   return len;
  

}

static int get_aligned_image_offset(struct spl_load_info *info, int offset) @@ -218,7 +237,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, memcpy(dst, src, data_size);

    /* Figure out which device tree the board wants to use */

•   fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);
  

•   fdt_len = spl_fit_select_index(fit, images, &fdt_offset,
  

•                                  FIT_FDT_PROP, 0);
    if (fdt_len < 0)
            return fdt_len;
  
  

-- 2.8.2

Regards, Simon

On 22/01/17 07:16, Kever Yang wrote:

Hi Andre,

On 01/20/2017 09:53 AM, Andre Przywara wrote:

...

At the moment we load two images from a FIT image: the actual U-Boot image and the DTB. Both times we have very similar code to deal with alignment requirement the media we load from imposes upon us. Factor out this code into a new function, which we just call twice.

Signed-off-by: Andre Przywara andre.przywara@arm.com

common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------ 1 file changed, 51 insertions(+), 71 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 381ed1f..d4149c5 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, return (data_size + info->bl_len - 1) / info->bl_len; } +static int spl_load_fit_image(struct spl_load_info *info, ulong sector,

•              void *fit, ulong base_offset, int node,
  

•              struct spl_image_info *image_info)
  

+{

• ulong offset;
• size_t length;
• ulong load, entry;
• void *src;
• ulong overhead;
• int nr_sectors;
• offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
• length = fdt_getprop_u32(fit, node, "data-size");
• load = fdt_getprop_u32(fit, node, "load");
• if (load == -1U && image_info)

•    load = image_info->load_addr;
  

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

...

• entry = fdt_getprop_u32(fit, node, "entry");
• overhead = get_aligned_image_overhead(info, offset);
• nr_sectors = get_aligned_image_size(info, overhead + length,

offset);

• if (info->read(info, sector + get_aligned_image_offset(info,

offset),

•           nr_sectors, (void*)load) != nr_sectors)
  

•    return -EIO;
  

• debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,

•      (unsigned long)length);
  

• src = (void *)load + overhead;

+#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process(&src, &length);

+#endif

• memcpy((void*)load, src, length);

For FIT image, we read the first block for header, and then we know the offset and size for images, is it possible to have an option that we can read the image data to load address directly without memcpy(), this helps speed up the boot time.

We do read directly into the load address (see the marked place above). But we have to fix up the alignment: a) The load address may not be cache line aligned. I am not totally convinced we really need this, but the code takes care of this. Shouldn't be an issue in practice since most load addresses are already aligned. b) More importantly we need to observe the block size of the device we read from. mkimage packs the images as tightly as possible into the image, so ATF may start at say offset 0x456 within the FIT image. Given that the FIT image itself starts at a sector boundary, that may be "in the middle" of a sector, which is the smallest addressable unit for block devices. For MMC based storage for instance this is typically 512 bytes. So we read the whole sector and later discard the overhead by copying the image back in memory.

Questions to you: 1) Is the memcpy really an issue? I take it you use bl31.bin only, which is probably between 32KB and 64KB. So does the memcpy really contribute to boot time here? 2) What device do you usually read from? What is the alignment there? For instance SPI flash has typically an alignment of 1 Byte, so no adjustment is actually needed. The memcpy routine bails out if dst==src, so in this case there are no cycles spend on this. 3) Can you tweak mkimage to observe alignment? As I said above, in the moment it packs it tightly, but I don't see why we couldn't align everything in the image at the cost of loosing 511 Bytes per embedded image in the worst case, for instance. This would eventually fold into the same point as question 2): If everything is aligned already, memcpy is a NOP.

So one relatively easy and clean solution would be to introduce an alignment command line option to mkimage. One could optionally specify a value, to which mkimage would align each embedded images into, leaving gaps in between. We would still go over all the calculations in the SPL code, but all the offsets and corrections would end up being 0, so no copying would be necessary anymore.

Does this sound like a plan?

Cheers, Andre.

...

• if (image_info) {

•    image_info->load_addr = load;
  

•    image_info->size = length;
  

•    image_info->entry_point = entry;
  

• }
• return 0;

+}

• int spl_load_simple_fit(struct spl_image_info *spl_image, struct spl_load_info *info, ulong sector, void *fit) { int sectors;

• ulong size, load;

• ulong size; unsigned long count;
• struct spl_image_info image_info; int node, images;

• void *load_ptr;
• int fdt_offset, fdt_len;
• int data_offset, data_size; int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
• int src_sector;
• void *dst, *src; /*
  • Figure out where the external images start. This is the base

for the @@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, return -1; }

• /* Get its information and set up the spl_image structure */
• data_offset = fdt_getprop_u32(fit, node, "data-offset");
• data_size = fdt_getprop_u32(fit, node, "data-size");
• load = fdt_getprop_u32(fit, node, "load");
• debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
• spl_image->load_addr = load;
• spl_image->entry_point = load;

• /* Load the image and set up the spl_image structure */
• spl_load_fit_image(info, sector, fit, base_offset, node, spl_image); spl_image->os = IH_OS_U_BOOT;
  • /*

• * Work out where to place the image. We read it so that the first
  

• * byte will be at 'load'. This may mean we need to load it starting
  

• * before then, since we can only read whole blocks.
  

• */
  

• data_offset += base_offset;
• sectors = get_aligned_image_size(info, data_size, data_offset);
• load_ptr = (void *)load;
• debug("U-Boot size %x, data %p\n", data_size, load_ptr);
• dst = load_ptr;
• /* Read the image */
• src_sector = sector + get_aligned_image_offset(info, data_offset);
• debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",

•      dst, src_sector, sectors);
  

• count = info->read(info, src_sector, sectors, dst);
• if (count != sectors)

•    return -EIO;
  

• debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,

•      data_size);
  

• src = dst + get_aligned_image_overhead(info, data_offset);

-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process((void **)&src, (size_t *)&data_size);

-#endif

• memcpy(dst, src, data_size);

• /* Figure out which device tree the board wants to use */
  node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0);
  if (node < 0) {
      debug("%s: cannot find FDT node\n", __func__);
      return node;
  }
  

• fdt_offset = fdt_getprop_u32(fit, node, "data-offset");
• fdt_len = fdt_getprop_u32(fit, node, "data-size"); /*

• * Read the device tree and place it after the image. There may be
  

• * some extra data before it since we can only read entire blocks.
  

• * And also align the destination address to ARCH_DMA_MINALIGN.
  

• * Read the device tree and place it after the image.
  

• * Align the destination address to ARCH_DMA_MINALIGN.
   */
  

• dst = (void *)((load + data_size + align_len) & ~align_len);
• fdt_offset += base_offset;
• sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
• src_sector = sector + get_aligned_image_offset(info, fdt_offset);
• count = info->read(info, src_sector, sectors, dst);
• debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",

•      dst, src_sector, sectors);
  

• if (count != sectors)

•    return -EIO;
  

• /*

• * Copy the device tree so that it starts immediately after the
  

image.

• * After this we will have the U-Boot image and its device tree
  

ready

• * for us to start.
  

• */
  

• debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,

•      fdt_len);
  

• src = dst + get_aligned_image_overhead(info, fdt_offset);
• dst = load_ptr + data_size;

-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);

-#endif

• memcpy(dst, src, fdt_len);

• image_info.load_addr = spl_image->load_addr + spl_image->size;
• spl_load_fit_image(info, sector, fit, base_offset, node,

&image_info); return 0; }

On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:

At the moment we load two images from a FIT image: the actual U-Boot image and the DTB. Both times we have very similar code to deal with alignment requirement the media we load from imposes upon us. Factor out this code into a new function, which we just call twice.

Signed-off-by: Andre Przywara andre.przywara@arm.com

common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------ 1 file changed, 51 insertions(+), 71 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 381ed1f..d4149c5 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, return (data_size + info->bl_len - 1) / info->bl_len; }

+static int spl_load_fit_image(struct spl_load_info *info, ulong sector,

• 	      void *fit, ulong base_offset, int node,
  

• 	      struct spl_image_info *image_info)
  

+{

• ulong offset;
• size_t length;
• ulong load, entry;
• void *src;
• ulong overhead;
• int nr_sectors;
• offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
• length = fdt_getprop_u32(fit, node, "data-size");
• load = fdt_getprop_u32(fit, node, "load");
• if (load == -1U && image_info)

• load = image_info->load_addr;
  

What if load_addr is not aligned with ARCH_DMA_MINALIGN like in case of DT loading (u-boot's load_addr + size cannot be always aligned with DMA). I keep getting this error when loading DT: "FAT: Misaligned buffer address (808675a0)."

Something similar is required to fix it: http://pastebin.ubuntu.com/23850970/

• entry = fdt_getprop_u32(fit, node, "entry");
• overhead = get_aligned_image_overhead(info, offset);
• nr_sectors = get_aligned_image_size(info, overhead + length, offset);

^^^^ Need not add overhead here as get_aligned_image_size() takes care of adding overhead.

• if (info->read(info, sector + get_aligned_image_offset(info, offset),

•        nr_sectors, (void*)load) != nr_sectors)
  

• return -EIO;
  

• debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,

•      (unsigned long)length);
  

• src = (void *)load + overhead;

+#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process(&src, &length);

+#endif

• memcpy((void*)load, src, length);
• if (image_info) {

• image_info->load_addr = load;
  

• image_info->size = length;
  

• image_info->entry_point = entry;
  

If entry_point is not provided can we default to load_addr? Existing U-Boot fit image generation does not provide entry option. So this patch alone breaks boot.(I understand that it is fixed in next patch but this is breaking git bisectability).

Thanks and regards, Lokesh

• }
• return 0;

+}

int spl_load_simple_fit(struct spl_image_info *spl_image, struct spl_load_info *info, ulong sector, void *fit) { int sectors;

• ulong size, load;

• ulong size; unsigned long count;
• struct spl_image_info image_info; int node, images;

• void *load_ptr;

• int fdt_offset, fdt_len;

• int data_offset, data_size; int base_offset, align_len = ARCH_DMA_MINALIGN - 1;

• int src_sector;

• void *dst, *src;

  /*

  • Figure out where the external images start. This is the base for the

@@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, return -1; }

• /* Get its information and set up the spl_image structure */
• data_offset = fdt_getprop_u32(fit, node, "data-offset");
• data_size = fdt_getprop_u32(fit, node, "data-size");
• load = fdt_getprop_u32(fit, node, "load");
• debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
• spl_image->load_addr = load;
• spl_image->entry_point = load;

• /* Load the image and set up the spl_image structure */
• spl_load_fit_image(info, sector, fit, base_offset, node, spl_image); spl_image->os = IH_OS_U_BOOT;

• /*

• * Work out where to place the image. We read it so that the first
  

• * byte will be at 'load'. This may mean we need to load it starting
  

• * before then, since we can only read whole blocks.
  

• */
  

• data_offset += base_offset;
• sectors = get_aligned_image_size(info, data_size, data_offset);
• load_ptr = (void *)load;
• debug("U-Boot size %x, data %p\n", data_size, load_ptr);
• dst = load_ptr;
• /* Read the image */
• src_sector = sector + get_aligned_image_offset(info, data_offset);
• debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",

•      dst, src_sector, sectors);
  

• count = info->read(info, src_sector, sectors, dst);
• if (count != sectors)

• return -EIO;
  

• debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,

•      data_size);
  

• src = dst + get_aligned_image_overhead(info, data_offset);

-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process((void **)&src, (size_t *)&data_size);

-#endif

• memcpy(dst, src, data_size);

• /* Figure out which device tree the board wants to use */ node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0); if (node < 0) { debug("%s: cannot find FDT node\n", __func__); return node; }

• fdt_offset = fdt_getprop_u32(fit, node, "data-offset");

• fdt_len = fdt_getprop_u32(fit, node, "data-size");

  /*

• * Read the device tree and place it after the image. There may be
  

• * some extra data before it since we can only read entire blocks.
  

• * And also align the destination address to ARCH_DMA_MINALIGN.
  

• * Read the device tree and place it after the image.
  

• * Align the destination address to ARCH_DMA_MINALIGN.
  

  */

• dst = (void *)((load + data_size + align_len) & ~align_len);
• fdt_offset += base_offset;
• sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
• src_sector = sector + get_aligned_image_offset(info, fdt_offset);
• count = info->read(info, src_sector, sectors, dst);
• debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",

•      dst, src_sector, sectors);
  

• if (count != sectors)

• return -EIO;
  

• /*

• * Copy the device tree so that it starts immediately after the image.
  

• * After this we will have the U-Boot image and its device tree ready
  

• * for us to start.
  

• */
  

• debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,

•      fdt_len);
  

• src = dst + get_aligned_image_overhead(info, fdt_offset);
• dst = load_ptr + data_size;

-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);

-#endif

• memcpy(dst, src, fdt_len);

• image_info.load_addr = spl_image->load_addr + spl_image->size;

• spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);

  return 0;

}

Tom,

On Monday 23 January 2017 06:28 PM, Tom Rini wrote:

On Mon, Jan 23, 2017 at 02:23:40PM +0530, Lokesh Vutla wrote:

...

On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:

...

At the moment we load two images from a FIT image: the actual U-Boot image and the DTB. Both times we have very similar code to deal with alignment requirement the media we load from imposes upon us. Factor out this code into a new function, which we just call twice.

Signed-off-by: Andre Przywara andre.przywara@arm.com

common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------ 1 file changed, 51 insertions(+), 71 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 381ed1f..d4149c5 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, return (data_size + info->bl_len - 1) / info->bl_len; }

+static int spl_load_fit_image(struct spl_load_info *info, ulong sector,

• 	      void *fit, ulong base_offset, int node,
  

• 	      struct spl_image_info *image_info)
  

+{

• ulong offset;
• size_t length;
• ulong load, entry;
• void *src;
• ulong overhead;
• int nr_sectors;
• offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
• length = fdt_getprop_u32(fit, node, "data-size");
• load = fdt_getprop_u32(fit, node, "load");
• if (load == -1U && image_info)

• load = image_info->load_addr;
  

What if load_addr is not aligned with ARCH_DMA_MINALIGN like in case of DT loading (u-boot's load_addr + size cannot be always aligned with DMA). I keep getting this error when loading DT: "FAT: Misaligned buffer address (808675a0)."

My immediate concern here is that we've found another way we're going to run into the same old problems of 'large kernel BSS stomps on DT (or initrd) on ARM32 (arm64 won't because the Image format includes end of BSS as a field). If your FDT isn't at base+128MiB (and you have > 128MiB DDR), something is wrong :)

I guess the problem here is FDT for u-boot, which is loaded at the end of u-boot which depends on u-boot's load address and size. I agree that for kernel the DTB is loaded at base + 128MB. or am I missing something?

Thanks and regards, Lokesh

On 23/01/17 08:53, Lokesh Vutla wrote:

Hi Lokesh,

thanks a lot for having a thorough look at it!

...

On Friday 20 January 2017 07:23 AM, Andre Przywara wrote: At the moment we load two images from a FIT image: the actual U-Boot image and the DTB. Both times we have very similar code to deal with alignment requirement the media we load from imposes upon us. Factor out this code into a new function, which we just call twice.

Signed-off-by: Andre Przywara andre.przywara@arm.com

common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------ 1 file changed, 51 insertions(+), 71 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 381ed1f..d4149c5 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, return (data_size + info->bl_len - 1) / info->bl_len; }

+static int spl_load_fit_image(struct spl_load_info *info, ulong sector,

• 	      void *fit, ulong base_offset, int node,
  

• 	      struct spl_image_info *image_info)
  

+{

• ulong offset;
• size_t length;
• ulong load, entry;
• void *src;
• ulong overhead;
• int nr_sectors;
• offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
• length = fdt_getprop_u32(fit, node, "data-size");
• load = fdt_getprop_u32(fit, node, "load");
• if (load == -1U && image_info)

• load = image_info->load_addr;
  

What if load_addr is not aligned with ARCH_DMA_MINALIGN like in case of DT loading (u-boot's load_addr + size cannot be always aligned with DMA). I keep getting this error when loading DT: "FAT: Misaligned buffer address (808675a0)."

Ah, good point, though I didn't encounter this error.

Something similar is required to fix it: http://pastebin.ubuntu.com/23850970/

Yeah, looks like it. I try to bake this in an upcoming release.

...

• entry = fdt_getprop_u32(fit, node, "entry");
• overhead = get_aligned_image_overhead(info, offset);
• nr_sectors = get_aligned_image_size(info, overhead + length, offset);

								     ^^^^

Need not add overhead here as get_aligned_image_size() takes care of adding overhead.

Right, I somehow got lost in translation here, probably during some rebasing/refactoring.

...

• if (info->read(info, sector + get_aligned_image_offset(info, offset),

•        nr_sectors, (void*)load) != nr_sectors)
  

• return -EIO;
  

• debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,

•      (unsigned long)length);
  

• src = (void *)load + overhead;

+#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process(&src, &length);

+#endif

• memcpy((void*)load, src, length);
• if (image_info) {

• image_info->load_addr = load;
  

• image_info->size = length;
  

• image_info->entry_point = entry;
  

If entry_point is not provided can we default to load_addr? Existing U-Boot fit image generation does not provide entry option. So this patch alone breaks boot.(I understand that it is fixed in next patch but this is breaking git bisectability).

Good point, this is indeed a good idea.

Cheers, Andre.

Thanks and regards, Lokesh

...

• }
• return 0;

+}

int spl_load_simple_fit(struct spl_image_info *spl_image, struct spl_load_info *info, ulong sector, void *fit) { int sectors;

• ulong size, load;

• ulong size; unsigned long count;
• struct spl_image_info image_info; int node, images;

• void *load_ptr;

• int fdt_offset, fdt_len;

• int data_offset, data_size; int base_offset, align_len = ARCH_DMA_MINALIGN - 1;

• int src_sector;

• void *dst, *src;

  /*

  • Figure out where the external images start. This is the base for the

@@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, return -1; }

• /* Get its information and set up the spl_image structure */
• data_offset = fdt_getprop_u32(fit, node, "data-offset");
• data_size = fdt_getprop_u32(fit, node, "data-size");
• load = fdt_getprop_u32(fit, node, "load");
• debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
• spl_image->load_addr = load;
• spl_image->entry_point = load;

• /* Load the image and set up the spl_image structure */
• spl_load_fit_image(info, sector, fit, base_offset, node, spl_image); spl_image->os = IH_OS_U_BOOT;

• /*

• * Work out where to place the image. We read it so that the first
  

• * byte will be at 'load'. This may mean we need to load it starting
  

• * before then, since we can only read whole blocks.
  

• */
  

• data_offset += base_offset;
• sectors = get_aligned_image_size(info, data_size, data_offset);
• load_ptr = (void *)load;
• debug("U-Boot size %x, data %p\n", data_size, load_ptr);
• dst = load_ptr;
• /* Read the image */
• src_sector = sector + get_aligned_image_offset(info, data_offset);
• debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",

•      dst, src_sector, sectors);
  

• count = info->read(info, src_sector, sectors, dst);
• if (count != sectors)

• return -EIO;
  

• debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,

•      data_size);
  

• src = dst + get_aligned_image_overhead(info, data_offset);

-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process((void **)&src, (size_t *)&data_size);

-#endif

• memcpy(dst, src, data_size);

• /* Figure out which device tree the board wants to use */ node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0); if (node < 0) { debug("%s: cannot find FDT node\n", __func__); return node; }

• fdt_offset = fdt_getprop_u32(fit, node, "data-offset");

• fdt_len = fdt_getprop_u32(fit, node, "data-size");

  /*

• * Read the device tree and place it after the image. There may be
  

• * some extra data before it since we can only read entire blocks.
  

• * And also align the destination address to ARCH_DMA_MINALIGN.
  

• * Read the device tree and place it after the image.
  

• * Align the destination address to ARCH_DMA_MINALIGN.
  

  */

• dst = (void *)((load + data_size + align_len) & ~align_len);
• fdt_offset += base_offset;
• sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
• src_sector = sector + get_aligned_image_offset(info, fdt_offset);
• count = info->read(info, src_sector, sectors, dst);
• debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",

•      dst, src_sector, sectors);
  

• if (count != sectors)

• return -EIO;
  

• /*

• * Copy the device tree so that it starts immediately after the image.
  

• * After this we will have the U-Boot image and its device tree ready
  

• * for us to start.
  

• */
  

• debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,

•      fdt_len);
  

• src = dst + get_aligned_image_overhead(info, fdt_offset);
• dst = load_ptr + data_size;

-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS

• board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);

-#endif

• memcpy(dst, src, fdt_len);

• image_info.load_addr = spl_image->load_addr + spl_image->size;

• spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);

  return 0;

}

Hi Andre,

Thanks for your patches, this is great help for enable ATF on U-Boot SPL. For ATF use case, we would like to identify which one is bl31 for we need to get entry point for it while we only need load address for other image. Any idea on get this information from different "loadables"?

Thanks, - Kever On 01/20/2017 09:53 AM, Andre Przywara wrote:

So far we were not using the FIT image format to its full potential: The SPL FIT loader was just loading the first image from the /images node plus one of the listed DTBs. Now with the refactored loader code it's easy to load an arbitrary number of images in addition to the two mentioned above. As described in the FIT image source file format description, iterate over all images listed at the "loadables" property in the configuration node and load every image at its desired location. This allows to load any kind of images:

• firmware images to execute before U-Boot proper (for instance ARM Trusted Firmware (ATF))
• firmware images for management processors (SCP, arisc, ...)
• firmware images for devices like WiFi controllers
• bit files for FPGAs
• additional configuration data
• kernels and/or ramdisks

The actual usage of this feature would be platform and/or board specific.

Signed-off-by: Andre Przywara andre.przywara@arm.com

common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index d4149c5..18269f7 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, struct spl_image_info image_info; int node, images; int base_offset, align_len = ARCH_DMA_MINALIGN - 1;

• int index = 0;

  /*

  • Figure out where the external images start. This is the base for the

@@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (node < 0) { debug("could not find firmware image, trying loadables...\n"); node = spl_fit_get_image_node(fit, images, "loadables", 0);

• /*
  

•  * If we pick the U-Boot image from "loadables", start at
  

•  * the second image when later loading additional images.
  

•  */
  

• index = 1;
  

  } if (node < 0) { debug("%s: Cannot find u-boot image node: %d\n",

@@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, image_info.load_addr = spl_image->load_addr + spl_image->size; spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);

• /* Now check if there are more images for us to load */
• for (; ; index++) {

• node = spl_fit_get_image_node(fit, images, "loadables", index);
  

• if (node < 0)
  

• 	break;
  

• spl_load_fit_image(info, sector, fit, base_offset, node,
  

• 		   &image_info);
  

• /*
  

•  * If the "firmware" image did not provide an entry point,
  

•  * use the first valid entry point from the loadables.
  

•  */
  

• if (spl_image->entry_point == -1U &&
  

•     image_info.entry_point != -1U)
  

• 	spl_image->entry_point = image_info.entry_point;
  

• }
• if (spl_image->entry_point == -1U || spl_image->entry_point == 0)

• spl_image->entry_point = spl_image->load_addr;
  

• return 0; }

Hi Andre,

On 01/22/2017 06:58 PM, André Przywara wrote:

On 22/01/17 07:08, Kever Yang wrote:

...

Hi Andre,

 Thanks for your patches, this is great help for enable ATF on U-Boot

SPL. For ATF use case, we would like to identify which one is bl31 for we need to get entry point for it while we only need load address for other image. Any idea on get this information from different "loadables"?

So I thought this use case is covered by the current scheme?

See below ...

...

On 01/20/2017 09:53 AM, Andre Przywara wrote:

...

So far we were not using the FIT image format to its full potential: The SPL FIT loader was just loading the first image from the /images node plus one of the listed DTBs. Now with the refactored loader code it's easy to load an arbitrary number of images in addition to the two mentioned above. As described in the FIT image source file format description, iterate over all images listed at the "loadables" property in the configuration node and load every image at its desired location. This allows to load any kind of images:

• firmware images to execute before U-Boot proper (for instance ARM Trusted Firmware (ATF))
• firmware images for management processors (SCP, arisc, ...)
• firmware images for devices like WiFi controllers
• bit files for FPGAs
• additional configuration data
• kernels and/or ramdisks

The actual usage of this feature would be platform and/or board specific.

Signed-off-by: Andre Przywara andre.przywara@arm.com

common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index d4149c5..18269f7 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, struct spl_image_info image_info; int node, images; int base_offset, align_len = ARCH_DMA_MINALIGN - 1;

• int index = 0; /*
  • Figure out where the external images start. This is the base

for the @@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (node < 0) { debug("could not find firmware image, trying loadables...\n"); node = spl_fit_get_image_node(fit, images, "loadables", 0);

•    /*
  

•     * If we pick the U-Boot image from "loadables", start at
  

•     * the second image when later loading additional images.
  

•     */
  

•    index = 1;
   }
   if (node < 0) {
       debug("%s: Cannot find u-boot image node: %d\n",
  

@@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, image_info.load_addr = spl_image->load_addr + spl_image->size; spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);

• /* Now check if there are more images for us to load */
• for (; ; index++) {

•    node = spl_fit_get_image_node(fit, images, "loadables", index);
  

•    if (node < 0)
  

•        break;
  

•    spl_load_fit_image(info, sector, fit, base_offset, node,
  

•               &image_info);
  

•    /*
  

•     * If the "firmware" image did not provide an entry point,
  

•     * use the first valid entry point from the loadables.
  

•     */
  

•    if (spl_image->entry_point == -1U &&
  

•        image_info.entry_point != -1U)
  

•        spl_image->entry_point = image_info.entry_point;
  

So this part here saves the entry point from the first image which provides an "entry" property in the loadables section, given that "firmware" didn't provide (a legal) one.

So depending on what you want, these are the options: a) You want to branch to U-Boot (default case if mkimage -f auto is used): You either don't specify an explicit entry point at all or provide an "entry" property for the U-Boot "firmware" image. The code below at the end of this function takes care of this. b) You want to branch to something else (bl31.bin): You make sure there is no explicit entry point in "firmware", instead put one "entry" property in the bl31 image description you want to boot. This will then branch to that instead of U-Boot. Check the example .its in patch 10/11 for an example of this.

So does this fit for you? Or is there a problem with this?

So that means only one 'entry' can be present even if there are more than one image, right? I think every "firmware" image is possible to have both "load" and "entry" node, for example if we have bl31, bl32 and bl33(U-Boot) in one FIT image, then we need to fill the bl31_params_t structure with bl32 entry point and U-Boot entry point, so that bl31 able to call bl32 and bl33. That's why I think we need to identify each of the "firmware", but not only know they are "loadables".

Thanks, - Kever

...

...

• }
• if (spl_image->entry_point == -1U || spl_image->entry_point == 0)

•    spl_image->entry_point = spl_image->load_addr;
  

This statement here takes the load address of the "firmware" image as an entry point if none is provided explicitly.

Cheers, Andre.

...

...

•  return 0;
  

  }

On 23/01/17 02:49, Kever Yang wrote:

Hi Andre,

On 01/22/2017 06:58 PM, André Przywara wrote:

...

On 22/01/17 07:08, Kever Yang wrote:

...

Hi Andre,

 Thanks for your patches, this is great help for enable ATF on

U-Boot SPL. For ATF use case, we would like to identify which one is bl31 for we need to get entry point for it while we only need load address for other image. Any idea on get this information from different "loadables"?

So I thought this use case is covered by the current scheme?

See below ...

...

On 01/20/2017 09:53 AM, Andre Przywara wrote:

...

So far we were not using the FIT image format to its full potential: The SPL FIT loader was just loading the first image from the /images node plus one of the listed DTBs. Now with the refactored loader code it's easy to load an arbitrary number of images in addition to the two mentioned above. As described in the FIT image source file format description, iterate over all images listed at the "loadables" property in the configuration node and load every image at its desired location. This allows to load any kind of images:

• firmware images to execute before U-Boot proper (for instance ARM Trusted Firmware (ATF))
• firmware images for management processors (SCP, arisc, ...)
• firmware images for devices like WiFi controllers
• bit files for FPGAs
• additional configuration data
• kernels and/or ramdisks

The actual usage of this feature would be platform and/or board specific.

Signed-off-by: Andre Przywara andre.przywara@arm.com

common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index d4149c5..18269f7 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, struct spl_image_info image_info; int node, images; int base_offset, align_len = ARCH_DMA_MINALIGN - 1;

• int index = 0; /*
  • Figure out where the external images start. This is the base

for the @@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (node < 0) { debug("could not find firmware image, trying loadables...\n"); node = spl_fit_get_image_node(fit, images, "loadables", 0);

•    /*
  

•     * If we pick the U-Boot image from "loadables", start at
  

•     * the second image when later loading additional images.
  

•     */
  

•    index = 1;
   }
   if (node < 0) {
       debug("%s: Cannot find u-boot image node: %d\n",
  

@@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, image_info.load_addr = spl_image->load_addr + spl_image->size; spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);

• /* Now check if there are more images for us to load */
• for (; ; index++) {

•    node = spl_fit_get_image_node(fit, images, "loadables",
  

index);

•    if (node < 0)
  

•        break;
  

•    spl_load_fit_image(info, sector, fit, base_offset, node,
  

•               &image_info);
  

•    /*
  

•     * If the "firmware" image did not provide an entry point,
  

•     * use the first valid entry point from the loadables.
  

•     */
  

•    if (spl_image->entry_point == -1U &&
  

•        image_info.entry_point != -1U)
  

•        spl_image->entry_point = image_info.entry_point;
  

So this part here saves the entry point from the first image which provides an "entry" property in the loadables section, given that "firmware" didn't provide (a legal) one.

So depending on what you want, these are the options: a) You want to branch to U-Boot (default case if mkimage -f auto is used): You either don't specify an explicit entry point at all or provide an "entry" property for the U-Boot "firmware" image. The code below at the end of this function takes care of this. b) You want to branch to something else (bl31.bin): You make sure there is no explicit entry point in "firmware", instead put one "entry" property in the bl31 image description you want to boot. This will then branch to that instead of U-Boot. Check the example .its in patch 10/11 for an example of this.

So does this fit for you? Or is there a problem with this?

So that means only one 'entry' can be present even if there are more than one image, right?

Well, there can be more nodes with entry properties, but the code just takes the first one and ignores the others. Because logically there is only one entry point after the SPL.

I think every "firmware" image is possible to have both "load" and "entry" node, for example if we have bl31, bl32 and bl33(U-Boot) in one FIT image, then we need to fill the bl31_params_t structure with bl32 entry point and U-Boot entry point, so that bl31 able to call bl32 and bl33.

Well, yes, but this is very ATF (or platform) specific. This series aims to use a generic solution, which does not need to be adapted to every platform. At the end of the day U-Boot can only jump once into an image.

I guess we could implement something which waits for a return: "bx" instead of "b", upon return try the next images. If it doesn't return: fair enough, it probably just went ahead with booting on its own. But I am not sure this is overly useful, since any kind of firmware would probably want to setup the system and thus would destroy U-Boot's setup (page tables, stack pointer, etc.) And I think it wouldn't help in your case.

I think what we should do instead is allowing platform specific additions to the loader code. For instance I saw that x86 introduced a "setup" property to hook in some pre-boot code. This is used by U-Boot proper, not SPL, but we could piggy-back on this idea. Maybe a "setup" property in a configuration node points to some special image node, which contains platform specific properties. Each platform is then expected to provide a specialised node parser, which can then deal with this setup node. Along the lines of:

images { atf-setup@1 { data = /incbin/("../../bl31.bin"); atf-bl32 = "bl32@1"; atf-bl33 = "uboot@1"; atf-bl31-params-offset = <0x124>; load = < ... >; entry = < ... >; }; bl32@1 { data = /incbin/("../../bl32.bin"); load = <...>; }; uboot@1 { data = /incbin/("../../u-boot-nodtb.bin"); load = <...>; }; ... }; configurations { config@1 { description = "rk3399-evb"; firmware = "uboot@1"; setup = "atf-setup@1"; loadables = "bl32@1"; fdt = "fdt@1"; }; };

Then have some RK specific parser code that follows the "setup" property (in this case to "atf-setup@1") and reads the special properties in there to do its magic: read the load addresses of the referenced images and setup the structure, its address being in "atf-bl31-params-offset".

This way you could control everything from the FIT image nodes. Ideally this would be even RK agnostic, instead providing a generic ATF solution.

Another possibility would be to have some small shim that gets loaded instead of bl31.bin and which fills up the bl31_params_t structure. Though I guess it would be hard to communicate the parameters.

Or can't you simply hard code the bl32 and bl33 load addresses into bl31? I can imagine you end up with them being loaded on certain addresses (SRAM) only anyway. On Allwinner for instance the U-Boot load address is hardcoded in bl31.bin, so we don't need to take care of this.

That's why I think we need to identify each of the "firmware", but not only know they are "loadables".

I don't know if people like the idea of introducing a lot of special properties into a configuration node, that's why the idea of confining everything into a platform-specific node. Maybe we even use compatible= strings?

Cheers, Andre.

...

...

...

• }
• if (spl_image->entry_point == -1U || spl_image->entry_point == 0)

•    spl_image->entry_point = spl_image->load_addr;
  

This statement here takes the load address of the "firmware" image as an entry point if none is provided explicitly.

Cheers, Andre.

...

...

•  return 0;
  

  }

On 21/01/17 04:24, Siarhei Siamashka wrote:

Hi Siarhei,

On Fri, 20 Jan 2017 01:53:25 +0000 Andre Przywara andre.przywara@arm.com wrote:

...

mksunxiboot limits the size of the resulting SPL binaries to pretty conservative values to cover all SoCs and all boot media (NAND). In preparation for supporting modern SoCs without NAND, which may require a really large SPL, introduce comamnd line parameters to push the possible SPL size to the limits.

Signed-off-by: Andre Przywara andre.przywara@arm.com

Thanks for trying to improve mksunxiboot.

The proposed "--mmc", "--nand" and "--max" options seem to be completely redundant and serve no real purpose. The 8K alignment is perfectly fine for any needs and allows to increase the SPL size to 32K (which is presumably your intention).

Mmmh, interesting, I think my first observation was that adjusting the limit wasn't good enough, but there was some side effect of the alignment connected to the limit not being exactly 32K, but slightly below. So the easiest solution was to just follow the comment, but make this configurable to not break NAND flash.

But as I can't reproduce this anymore, I am all too happy to drop this part.

The only Allwinner SoC that may potentially benefit from the 512 bytes alignment is A13: https://linux-sunxi.org/BROM#U-Boot_SPL_limitations But for the sake of simplicity, we can pretend that A13 has the same 24K SPL size limit as A10 and A20.

Fair enough.

That said, I agree that we should fix the mksunxiboot tool and allow it to create 32K SPL files. Exceeding the SPL size limit is already checked elsewhere (via the linker script). So mksunxiboot does not need to enforce the 24K limit even for A10/A20 boards.

But what about the 0x7600 limit for the older SoCs?: #define SUN4I_SRAM_SIZE 0x7600 /* 0x7748+ is used by BROM */

Or is this snake oil as we limit earlier to 24K already anyway?

So do you hint that this whole patch can eventually be reduced to:

-#define SUN4I_SRAM_SIZE 0x7600 /* 0x7748+ is used by BROM */ +#define SUN4I_SRAM_SIZE 0x8000

Cheers, Andre.

...

---

tools/mksunxiboot.c | 51 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/tools/mksunxiboot.c b/tools/mksunxiboot.c index 0f0b003..fa54bce 100644 --- a/tools/mksunxiboot.c +++ b/tools/mksunxiboot.c @@ -48,37 +48,58 @@ int gen_check_sum(struct boot_file_head *head_p) #define ALIGN(x, a) __ALIGN_MASK((x), (typeof(x))(a)-1) #define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask))

-#define SUN4I_SRAM_SIZE 0x7600 /* 0x7748+ is used by BROM */ -#define SRAM_LOAD_MAX_SIZE (SUN4I_SRAM_SIZE - sizeof(struct boot_file_head)) +#define SUN4I_SRAM_SIZE 0x7600 /* 0x7748+ is used by BROM */ +#define SUNXI_MAX_SRAM_SIZE 0x8000 +#define SRAM_LOAD_MAX_SIZE (SUNXI_MAX_SRAM_SIZE - sizeof(struct boot_file_head))

/*

• BROM (at least on A10 and A20) requires NAND-images to be explicitly aligned
• to a multiple of 8K, and rejects the image otherwise. MMC-images are fine

• • with 512B blocks. To cater for both, align to the largest of the two.

• • with 512B blocks.
  */

-#define BLOCK_SIZE 0x2000 +#define BLOCK_SIZE_NAND 0x2000 +#define BLOCK_SIZE_MMC 0x0200 +#define MAX_BLOCK_SIZE BLOCK_SIZE_NAND

struct boot_img { struct boot_file_head header; char code[SRAM_LOAD_MAX_SIZE];

• char pad[BLOCK_SIZE];

• char pad[MAX_BLOCK_SIZE];

};

int main(int argc, char *argv[]) { int fd_in, fd_out;

• int ac = 1; struct boot_img img; unsigned file_size;

• unsigned size_limit = SUN4I_SRAM_SIZE - sizeof(struct boot_file_head);

• unsigned block_size = BLOCK_SIZE_NAND; int count;

  if (argc < 2) { printf("\tThis program makes an input bin file to sun4i " \ "bootable image.\n" \

•        "\tUsage: %s input_file out_putfile\n", argv[0]);
  

•        "\tUsage: %s input_file [out_putfile]\n", argv[0]);
  

  return EXIT_FAILURE; }

• fd_in = open(argv[1], O_RDONLY);

• if (!strcmp(argv[ac], "--mmc")) {

• block_size = BLOCK_SIZE_MMC;
  

• ac++;
  

• }
• if (!strcmp(argv[ac], "--nand")) {

• block_size = BLOCK_SIZE_NAND;
  

• ac++;
  

• }
• if (!strcmp(argv[ac], "--max")) {

• size_limit = SUNXI_MAX_SRAM_SIZE - sizeof(struct boot_file_head);
  

• ac++;
  

• }
• if (ac >= argc)

• return EXIT_FAILURE;
  

• fd_in = open(argv[ac++], O_RDONLY); if (fd_in < 0) { perror("Open input file"); return EXIT_FAILURE;

@@ -89,15 +110,19 @@ int main(int argc, char *argv[]) /* get input file size */ file_size = lseek(fd_in, 0, SEEK_END);

• if (file_size > SRAM_LOAD_MAX_SIZE) {

• if (file_size > size_limit) { fprintf(stderr, "ERROR: File too large!\n"); return EXIT_FAILURE; }

• fd_out = open(argv[2], O_WRONLY | O_CREAT, 0666);
• if (fd_out < 0) {

• perror("Open output file");
  

• return EXIT_FAILURE;
  

• if (ac >= argc) {

• fd_out = STDOUT_FILENO;
  

• } else {

• fd_out = open(argv[ac], O_WRONLY | O_CREAT, 0666);
  

• if (fd_out < 0) {
  

• 	perror("Open output file");
  

• 	return EXIT_FAILURE;
  

• }
  

  }

  /* read file to buffer to calculate checksum */

@@ -115,7 +140,7 @@ int main(int argc, char *argv[]) & 0x00FFFFFF); memcpy(img.header.magic, BOOT0_MAGIC, 8); /* no '0' termination */ img.header.length =

• ALIGN(file_size + sizeof(struct boot_file_head), BLOCK_SIZE);
  

• ALIGN(file_size + sizeof(struct boot_file_head), block_size);
  

  img.header.b_instruction = cpu_to_le32(img.header.b_instruction); img.header.length = cpu_to_le32(img.header.length);

Hi,

On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:

For a board or platform to support FIT loading in the SPL, it has to provide a board_fit_config_name_match() routine, which helps to select one of possibly multiple DTBs contained in a FIT image. Provide a simple function to cover the two different Pine64 models, which can be easily told apart by looking at the amount of installed RAM.

Signed-off-by: Andre Przywara andre.przywara@arm.com

board/sunxi/board.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 5365638..bbbb826 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd) #endif return 0; }

+#ifdef CONFIG_SPL_LOAD_FIT +int board_fit_config_name_match(const char *name) +{ +#ifdef CONFIG_MACH_SUN50I

• if ((gd->ram_size > 512 * 1024 * 1024))

• return !strcmp(name, "sun50i-a64-pine64-plus");
  

• else

• return !strcmp(name, "sun50i-a64-pine64");
  

+#endif

• return -1;

+}

That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT enabled will be considered a pine64 board?

Maxime

21.01.2017, 05:56, "André Przywara" andre.przywara@arm.com:

On 20/01/17 21:35, Maxime Ripard wrote:

Hi Maxime,

thanks for having a look!

...

On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:

...

For a board or platform to support FIT loading in the SPL, it has to  provide a board_fit_config_name_match() routine, which helps to select  one of possibly multiple DTBs contained in a FIT image.  Provide a simple function to cover the two different Pine64 models,  which can be easily told apart by looking at the amount of installed  RAM.

Signed-off-by: Andre Przywara andre.przywara@arm.com  ---   board/sunxi/board.c | 13 +++++++++++++   1 file changed, 13 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c  index 5365638..bbbb826 100644  --- a/board/sunxi/board.c  +++ b/board/sunxi/board.c  @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)   #endif           return 0;   }  +  +#ifdef CONFIG_SPL_LOAD_FIT  +int board_fit_config_name_match(const char *name)  +{  +#ifdef CONFIG_MACH_SUN50I  + if ((gd->ram_size > 512 * 1024 * 1024))  + return !strcmp(name, "sun50i-a64-pine64-plus");  + else  + return !strcmp(name, "sun50i-a64-pine64");  +#endif  + return -1;  +}

That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT  enabled will be considered a pine64 board?

Yes, at least for now, because that's the only A64 board we officially support so far. And yes again, it is fishy. It is more a demo or a placeholder for now, because you _need_ an implementation of board_fit_config_name_match() once you enable CONFIG_SPL_LOAD_FIT.

You have a Banana Pi M64, right?

And I do not want to change code every time we have a new A64 board...

One solution would be to have only one DTB supported per build, so board_fit_config_name_match() always returns 0.

I think that's what we're did in previous u-boot-sunxi-with-spl.bin .

Another (better) solution would be to store the board name in the SPL header, which could be done later when flashing the image. Then this function would just return strcmp(name, spl_header_name) or 0 if no name is found for whatever reason.

Ideas welcome. To be honest, this .dtb selection is nice, but I am not married to it. It usefulness maybe limited at the moment.

Cheers, Andre.

-- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.

On Fri, 20 Jan 2017 21:55:53 +0000 André Przywara andre.przywara@arm.com wrote:

On 20/01/17 21:35, Maxime Ripard wrote:

Hi Maxime,

thanks for having a look!

...

On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:

...

For a board or platform to support FIT loading in the SPL, it has to provide a board_fit_config_name_match() routine, which helps to select one of possibly multiple DTBs contained in a FIT image. Provide a simple function to cover the two different Pine64 models, which can be easily told apart by looking at the amount of installed RAM.

Signed-off-by: Andre Przywara andre.przywara@arm.com

board/sunxi/board.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 5365638..bbbb826 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd) #endif return 0; }

+#ifdef CONFIG_SPL_LOAD_FIT +int board_fit_config_name_match(const char *name) +{ +#ifdef CONFIG_MACH_SUN50I

• if ((gd->ram_size > 512 * 1024 * 1024))

• return !strcmp(name, "sun50i-a64-pine64-plus");
  

• else

• return !strcmp(name, "sun50i-a64-pine64");
  

+#endif

• return -1;

+}

That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT enabled will be considered a pine64 board?

Yes, at least for now, because that's the only A64 board we officially support so far. And yes again, it is fishy. It is more a demo or a placeholder for now, because you _need_ an implementation of board_fit_config_name_match() once you enable CONFIG_SPL_LOAD_FIT.

One solution would be to have only one DTB supported per build, so board_fit_config_name_match() always returns 0.

Another (better) solution would be to store the board name in the SPL header, which could be done later when flashing the image. Then this function would just return strcmp(name, spl_header_name) or 0 if no name is found for whatever reason.

Yes, this is a good solution in the case if we have some non-removable storage on the board (SPI NOR flash, NAND, EEPROM or something else). We can discuss it separately.

But the current generation of Pine64 boards don't have any non-removable storage yet. My understanding is that you tried to provide the DTB selection, which is based on circumstantial evidences, such as the RAM size. IMHO this is also a valid selection method, because it can reduce the number of required OS image variants.

Yes, this is not urgent and we can indeed temporarily use separate defconfigs for different Pine64 board variants.

Ideas welcome. To be honest, this .dtb selection is nice, but I am not married to it. It usefulness maybe limited at the moment.

The board specific information is normally stored either in the defconfig file or in the device tree, rather than gets hardcoded in the sources. We can probably add some extra constraints information to the device tree. And these extra constraints can be passed to the board_fit_config_name_match() function in some way.

Then the sun50i-a64-pine64 device tree file can specify that this board is expected to have exactly 512 MiB of RAM. Having this information, the board_fit_config_name_match() function will fail to match it if the actual RAM size is wrong.

This approach is similar to what I used in https://github.com/ssvb/sunxi-bootsetup/releases/tag/20141215-sunxi-bootsetu... http://lists.denx.de/pipermail/u-boot/2015-January/202306.html

The sunxi-bootsetup stub used the SoC type id and RAM size to filter the list of potentially matching boards. It worked very nicely in practice.

On Sat, Jan 21, 2017 at 03:15:27PM +0000, André Przywara wrote:

...

...

...

On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:

...

For a board or platform to support FIT loading in the SPL, it has to provide a board_fit_config_name_match() routine, which helps to select one of possibly multiple DTBs contained in a FIT image. Provide a simple function to cover the two different Pine64 models, which can be easily told apart by looking at the amount of installed RAM.

Signed-off-by: Andre Przywara andre.przywara@arm.com

board/sunxi/board.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 5365638..bbbb826 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd) #endif return 0; }

+#ifdef CONFIG_SPL_LOAD_FIT +int board_fit_config_name_match(const char *name) +{ +#ifdef CONFIG_MACH_SUN50I

• if ((gd->ram_size > 512 * 1024 * 1024))

• return !strcmp(name, "sun50i-a64-pine64-plus");
  

• else

• return !strcmp(name, "sun50i-a64-pine64");
  

+#endif

• return -1;

+}

That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT enabled will be considered a pine64 board?

Yes, at least for now, because that's the only A64 board we officially support so far. And yes again, it is fishy. It is more a demo or a placeholder for now, because you _need_ an implementation of board_fit_config_name_match() once you enable CONFIG_SPL_LOAD_FIT.

One solution would be to have only one DTB supported per build, so board_fit_config_name_match() always returns 0.

Another (better) solution would be to store the board name in the SPL header, which could be done later when flashing the image. Then this function would just return strcmp(name, spl_header_name) or 0 if no name is found for whatever reason.

Yes, this is a good solution in the case if we have some non-removable storage on the board (SPI NOR flash, NAND, EEPROM or something else). We can discuss it separately.

I totally agree. I rebased your patch to latest mainline already and will send out something later.

...

But the current generation of Pine64 boards don't have any non-removable storage yet. My understanding is that you tried to provide the DTB selection, which is based on circumstantial evidences, such as the RAM size. IMHO this is also a valid selection method, because it can reduce the number of required OS image variants.

Yes, the point is that for U-Boot's purposes the only difference between the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets this information from the DT only, so there is no point at all for different defconfigs. And the DRAM size is a safe indicator for that difference, at least if we confine our view to Pine64 boards.

Now how other boards fit in here is a separate discussion IMO, so let's solve one problem after the other. I just thought that we could use:

#ifdef CONFIG_SPL_LOAD_FIT int board_fit_config_name_match(const char *name) { return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE); } #endif

I guess an easy way around this would be to add a Kconfig option for the pine64, like I tried to do for the CHIP (but never ended up merging). That way, at least we won't impact the other board, and we can have that default.

Maxime

On Mon, Jan 23, 2017 at 10:55:55PM +0000, André Przywara wrote:

On 23/01/17 17:29, Maxime Ripard wrote:

...

On Sat, Jan 21, 2017 at 03:15:27PM +0000, André Przywara wrote:

...

...

...

...

On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote: > For a board or platform to support FIT loading in the SPL, it has to > provide a board_fit_config_name_match() routine, which helps to select > one of possibly multiple DTBs contained in a FIT image. > Provide a simple function to cover the two different Pine64 models, > which can be easily told apart by looking at the amount of installed > RAM. > > Signed-off-by: Andre Przywara andre.przywara@arm.com > --- > board/sunxi/board.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > index 5365638..bbbb826 100644 > --- a/board/sunxi/board.c > +++ b/board/sunxi/board.c > @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd) > #endif > return 0; > } > + > +#ifdef CONFIG_SPL_LOAD_FIT > +int board_fit_config_name_match(const char *name) > +{ > +#ifdef CONFIG_MACH_SUN50I > + if ((gd->ram_size > 512 * 1024 * 1024)) > + return !strcmp(name, "sun50i-a64-pine64-plus"); > + else > + return !strcmp(name, "sun50i-a64-pine64"); > +#endif > + return -1; > +}

That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT enabled will be considered a pine64 board?

Yes, at least for now, because that's the only A64 board we officially support so far. And yes again, it is fishy. It is more a demo or a placeholder for now, because you _need_ an implementation of board_fit_config_name_match() once you enable CONFIG_SPL_LOAD_FIT.

One solution would be to have only one DTB supported per build, so board_fit_config_name_match() always returns 0.

Another (better) solution would be to store the board name in the SPL header, which could be done later when flashing the image. Then this function would just return strcmp(name, spl_header_name) or 0 if no name is found for whatever reason.

Yes, this is a good solution in the case if we have some non-removable storage on the board (SPI NOR flash, NAND, EEPROM or something else). We can discuss it separately.

I totally agree. I rebased your patch to latest mainline already and will send out something later.

...

But the current generation of Pine64 boards don't have any non-removable storage yet. My understanding is that you tried to provide the DTB selection, which is based on circumstantial evidences, such as the RAM size. IMHO this is also a valid selection method, because it can reduce the number of required OS image variants.

Yes, the point is that for U-Boot's purposes the only difference between the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets this information from the DT only, so there is no point at all for different defconfigs. And the DRAM size is a safe indicator for that difference, at least if we confine our view to Pine64 boards.

Now how other boards fit in here is a separate discussion IMO, so let's solve one problem after the other. I just thought that we could use:

#ifdef CONFIG_SPL_LOAD_FIT int board_fit_config_name_match(const char *name) { return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE); } #endif

I guess an easy way around this would be to add a Kconfig option for the pine64, like I tried to do for the CHIP (but never ended up merging). That way, at least we won't impact the other board, and we can have that default.

Yes, that sounds indeed like a possibility.

I came up with this snippet yesterday (rough copy):

/* Check if there is a DT name stored in the SPL header and use that. */ if (spl->offset_default_dt_name) { cmp_str = SPL_ADDR + spl->offset_default_dt_name; } else { #ifdef CONFIG_DEFAULT_DEVICE_TREE cmp_str = CONFIG_DEFAULT_DEVICE_TREE; #else return 0; #endif };

    /* Differentiate the two Pine64 board DTs by their DRAM size. */
    if (strstr(name, "-pine64") && strstr(cmp_str, "-pine64")) {
            if ((gd->ram_size > 512 * 1024 * 1024))
                    return !strstr(name, "plus");
            else
                    return !!strstr(name, "plus");
    } else {
            return strcmp(name, cmp_str);
    }

That admittedly doesn't look very pretty, but should cover all the cases: DT name from SPL header, default compile time DT, Pine64 check. We could also guard the last part with a CONFIG_SUNXI_PINE64_DETECT symbol to save some space in case this is not needed.

Does that make sense?

Yep, that works for me (with the config option)

Thanks! Maxime

On Fri, Jan 20, 2017 at 01:53:29AM +0000, Andre Przywara wrote:

The Pine64 (as all 64-bit Allwinner boards so far) need to load an ARM Trusted Firmware image beside the actual U-Boot proper. This can now be easily achieved by using the just extended SPL FIT loading support, so enable it in the Pine64 defconfig.

Signed-off-by: Andre Przywara andre.przywara@arm.com

configs/pine64_plus_defconfig | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/configs/pine64_plus_defconfig b/configs/pine64_plus_defconfig index 2374170..e050800 100644 --- a/configs/pine64_plus_defconfig +++ b/configs/pine64_plus_defconfig @@ -3,9 +3,14 @@ CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER=y CONFIG_ARCH_SUNXI=y CONFIG_MACH_SUN50I=y CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pine64-plus" +CONFIG_OF_LIST="sun50i-a64-pine64 sun50i-a64-pine64-plus" # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set CONFIG_CONSOLE_MUX=y CONFIG_SPL=y +CONFIG_FIT=y +CONFIG_SPL_FIT=y +CONFIG_SPL_LOAD_FIT=y +CONFIG_SPL_OF_LIBFDT=y

I see no reason why this shouldn't be enabled by default through Kconfig, those are pretty standard usecases.

Thanks! Maxime