Re: [U-Boot] [PATCH] splash_source: add support for ubifs formatted nand

Hi Eran,

This mostly looks good to me, but I think there's a more generic way to address the UBIFS conundrum. See my comments below:

On Thu, Apr 14, 2016 at 09:13:14PM +0300, Eran Matityahu wrote:

Add support for loading splash image from NAND Flash formatted with a (UBI) filesystem.

I'm a little ambivalent about this patch, as it adds support for a specific filesystem rather than a device, due to the nature of NAND and UBI, but it does make some sense to me to add a NAND filesystem option here. It would be nice to get your input about this.

Signed-off-by: Eran Matityahu eran.m@variscite.com Cc: Igor Grinberg grinberg@compulab.co.il Cc: Tom Rini trini@konsulko.com Cc: Nikita Kiryanov nikita@compulab.co.il

---

common/splash_source.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++---- include/splash.h | 6 ++++-- 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/common/splash_source.c b/common/splash_source.c index a09dd4b..7762ee8 100644 --- a/common/splash_source.c +++ b/common/splash_source.c @@ -120,6 +120,9 @@ static int splash_select_fs_dev(struct splash_location *location) case SPLASH_STORAGE_SATA: res = fs_set_blk_dev("sata", location->devpart, FS_TYPE_ANY); break;

• case SPLASH_STORAGE_NAND:

• res = fs_set_blk_dev("ubi", NULL, FS_TYPE_ANY);
  

• break;a
  

It seems to me like this should be FS_TYPE_UBIFS?

default: printf("Error: unsupported location storage.\n"); return -ENODEV; @@ -163,6 +166,35 @@ static inline int splash_init_sata(void) } #endif

+#ifdef CONFIG_CMD_UBIFS +static int splash_mount_ubifs(struct splash_location *location) +{

• int res;
• char cmd[32];
• sprintf(cmd, "ubi part %s", location->mtdpart);
• res = run_command(cmd, 0);
• if (res)

• return res;
  

• sprintf(cmd, "ubifsmount %s", location->ubi_volume);
• res = run_command(cmd, 0);
• return res;

+} +#else +static inline int splash_mount_ubifs(struct splash_location *location) +{

• printf("Cannot load splash image: no UBIFS support\n");
• return -ENOSYS;

+} +#endif

+static inline int splash_umount_ubifs(void) +{

• return run_command("ubifsumount", 0);

+}

I know you prevent this from being called ifndef CONFIG_CMD_UBIFS, but I still think it should have two definitions (ifdef and ifndef) so that it will not depend on the program control flow.

#define SPLASH_SOURCE_DEFAULT_FILE_NAME "splash.bmp"

static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr) @@ -181,26 +213,36 @@ static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr) if (location->storage == SPLASH_STORAGE_SATA) res = splash_init_sata();

• if (location->storage == SPLASH_STORAGE_NAND)

• res = splash_mount_ubifs(location);
  

I think a more generic way of selecting ubifs would be to look at the ubi_volume field. If it's not NULL, then we are dealing with UBIFS. That way we will not have to update this check when NOR flash support is added, and we do not tie NAND fs case to UBIFS (even though there's no good alternative for UBIFS at the moment).

splash_select_fs_dev() would have to check ubi_volume in the SPLASH_STORAGE_NAND case as well.

• if (res) return res;

  res = splash_select_fs_dev(location); if (res)

• return res;
  

• goto out;
  
  

  res = fs_size(splash_file, &bmp_size); if (res) { printf("Error (%d): cannot determine file size\n", res);

• return res;
  

• goto out;
  

  }

  if (bmp_load_addr + bmp_size >= gd->start_addr_sp) { printf("Error: splashimage address too high. Data overwrites U-Boot and/or placed beyond DRAM boundaries.\n");

• return -EFAULT;
  

• res = -EFAULT;
  

• goto out;
  

  }

  splash_select_fs_dev(location);

• return fs_read(splash_file, bmp_load_addr, 0, 0, NULL);

• res = fs_read(splash_file, bmp_load_addr, 0, 0, NULL);

+out:

• if (location->storage == SPLASH_STORAGE_NAND)

• splash_umount_ubifs();
  

• return res;

}

/** diff --git a/include/splash.h b/include/splash.h index f0755ca..5c0da85 100644 --- a/include/splash.h +++ b/include/splash.h @@ -41,8 +41,10 @@ struct splash_location { char *name; enum splash_storage storage; enum splash_flags flags;

• u32 offset; /* offset from start of storage */
• char *devpart; /* Use the load command dev:part conventions */

• u32 offset; /* Offset from start of storage */
• char *devpart; /* Use the load command dev:part conventions */
• char *mtdpart; /* MTD partition for ubi part */
• char *ubi_volume; /* UBI volume-name for ubifsmount */

};

int splash_source_load(struct splash_location *locations, uint size);

1.9.1

Regards, Nikita Kiryanov