[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

Generic firmware loader framework contains some common functionality
which is reusable by any specific driver file system firmware loader.
Specific driver file system firmware loader handling can be defined
with both weak function fsloader_preprocess and fs_loading.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 common/Makefile                         |   7 +-
 common/splash_source.c                  | 110 +++++++++++++++++++++++++-------
 configs/socfpga_arria10_defconfig       |   2 +
 include/configs/socfpga_arria10_socdk.h |   3 +
 include/splash.h                        |   3 +
 5 files changed, 99 insertions(+), 26 deletions(-)

diff --git a/common/Makefile b/common/Makefile
index 801ea31..965a217 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -47,8 +47,6 @@ obj-$(CONFIG_MTD_NOR_FLASH) += flash.o
 obj-$(CONFIG_CMD_KGDB) += kgdb.o kgdb_stubs.o
 obj-$(CONFIG_I2C_EDID) += edid.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
-obj-y += splash.o
-obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o
 ifndef CONFIG_DM_VIDEO
 obj-$(CONFIG_LCD) += lcd.o lcd_console.o
 endif
@@ -102,7 +100,6 @@ endif
 obj-y += image.o
 obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
 obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
-obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
 obj-$(CONFIG_FIT_EMBED) += boot_fit.o common_fit.o
 obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += image-sig.o
 obj-$(CONFIG_IO_TRACE) += iotrace.o
@@ -130,3 +127,7 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
 obj-y += command.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+
+obj-y += splash.o
+obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o
+obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
diff --git a/common/splash_source.c b/common/splash_source.c
index e0defde..1413945 100644
--- a/common/splash_source.c
+++ b/common/splash_source.c
@@ -109,7 +109,7 @@ splash_address_too_high:
 	return -EFAULT;
 }
 
-static int splash_select_fs_dev(struct splash_location *location)
+int splash_select_fs_dev(struct splash_location *location)
 {
 	int res;
 
@@ -140,6 +140,7 @@ static int splash_select_fs_dev(struct splash_location *location)
 	return res;
 }
 
+#ifndef CONFIG_SPL_BUILD
 #ifdef CONFIG_USB_STORAGE
 static int splash_init_usb(void)
 {
@@ -175,6 +176,7 @@ static inline int splash_init_sata(void)
 	return -ENOSYS;
 }
 #endif
+#endif
 
 #ifdef CONFIG_CMD_UBIFS
 static int splash_mount_ubifs(struct splash_location *location)
@@ -213,22 +215,99 @@ static inline int splash_umount_ubifs(void)
 
 #define SPLASH_SOURCE_DEFAULT_FILE_NAME		"splash.bmp"
 
-static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
+/**
+ * fsloader_preprocess - Any prepocessing before calling filesystem loader.
+ *
+ * @locations:		An array of supported splash locations.
+ * @file_info:		Description and attributes to the image.
+ *			Could be structure pointer, and any type pointer.
+ * @filename:		Image filename in flashes.
+ * @bmp_load_addr:	Target memory location image loaded to.
+ *
+ * @return:	If 0, processing is succesfull. Filename pointer contains
+ *		valid filename.
+ *		If -ve, processing is failed.
+ */
+__weak int fsloader_preprocess(struct splash_location *location,
+			       void *file_info, char **filename,
+			       u32 bmp_load_addr)
 {
 	int res = 0;
 	loff_t bmp_size;
-	loff_t actread;
 	char *splash_file;
 
-	splash_file = env_get("splashfile");
+	splash_file = env_get((char *)file_info);
+
 	if (!splash_file)
 		splash_file = SPLASH_SOURCE_DEFAULT_FILE_NAME;
 
+	res = splash_select_fs_dev(location);
+	if (res) {
+		error("Error : Failed to select FS device\n");
+		return -EPERM;
+	}
+
+	res = fs_size(splash_file, &bmp_size);
+	if (res) {
+		error("Error (%d): cannot determine file size\n", res);
+		return -ENOENT;
+	}
+
+	if ((u32)bmp_load_addr + bmp_size >= gd->start_addr_sp) {
+		error("Error: splashimage address too high. Data overwrites ");
+		error("U-Boot and/or placed beyond DRAM boundaries.\n");
+		res = -EFAULT;
+		return -EFAULT;
+	}
+
+	*filename = splash_file;
+
+	return res;
+}
+
+/**
+ * fs_loading - This place is for implementing whaterver blob + whatever
+ *		specific driver to the HW such as program raw binary file to
+ *		FPGA.
+ *
+ * @locations:		An array of supported splash locations.
+ * @file_info:		Description and attributes to the image.
+ *			Could be structure pointer, and any type pointer.
+ * @filename:		Image filename in flashes.
+ * @bmp_load_addr:	Target memory location image loaded to.
+ * @bsize:		Size of target memory location.
+ *
+ * @return:	If 0, loading is succesfull. Filename pointer contains
+ *		valid filename.
+ *		If non-zero, loading is failed.
+ */
+__weak int fs_loading(struct splash_location *location, void *file_info,
+		      char *filename, u32 bmp_load_addr, size_t bsize)
+{
+	loff_t actread;
+
+	return fs_read(filename, (u32)bmp_load_addr, 0, 0, &actread);
+}
+
+int splash_load_fs(struct splash_location *location, void *file_info,
+		   u32 bmp_load_addr, size_t bsize)
+{
+	int res = 0;
+	char *splash_file = NULL;
+
+	res = fsloader_preprocess(location, file_info, &splash_file,
+				  bmp_load_addr);
+
+	if (res)
+		goto out;
+
+#ifndef CONFIG_SPL_BUILD
 	if (location->storage == SPLASH_STORAGE_USB)
 		res = splash_init_usb();
 
 	if (location->storage == SPLASH_STORAGE_SATA)
 		res = splash_init_sata();
+#endif
 
 	if (location->ubivol != NULL)
 		res = splash_mount_ubifs(location);
@@ -236,24 +315,8 @@ static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
 	if (res)
 		return res;
 
-	res = splash_select_fs_dev(location);
-	if (res)
-		goto out;
-
-	res = fs_size(splash_file, &bmp_size);
-	if (res) {
-		printf("Error (%d): cannot determine file size\n", 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");
-		res = -EFAULT;
-		goto out;
-	}
-
-	splash_select_fs_dev(location);
-	res = fs_read(splash_file, bmp_load_addr, 0, 0, &actread);
+	res = fs_loading(location, file_info, splash_file, bmp_load_addr,
+			bsize);
 
 out:
 	if (location->ubivol != NULL)
@@ -405,7 +468,8 @@ int splash_source_load(struct splash_location *locations, uint size)
 	if (splash_location->flags == SPLASH_STORAGE_RAW)
 		return splash_load_raw(splash_location, bmp_load_addr);
 	else if (splash_location->flags == SPLASH_STORAGE_FS)
-		return splash_load_fs(splash_location, bmp_load_addr);
+		return splash_load_fs(splash_location, "splashfile",
+				      bmp_load_addr, 0);
 #ifdef CONFIG_FIT
 	else if (splash_location->flags == SPLASH_STORAGE_FIT)
 		return splash_load_fit(splash_location, bmp_load_addr);
diff --git a/configs/socfpga_arria10_defconfig b/configs/socfpga_arria10_defconfig
index 4c73d73..2db34b5 100644
--- a/configs/socfpga_arria10_defconfig
+++ b/configs/socfpga_arria10_defconfig
@@ -7,6 +7,8 @@ CONFIG_DEFAULT_DEVICE_TREE="socfpga_arria10_socdk_sdmmc"
 CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="console=ttyS0,115200"
 CONFIG_DEFAULT_FDT_FILE="socfpga_arria10_socdk_sdmmc.dtb"
+CONFIG_FIT=y
+CONFIG_SPL_FIT=y
 CONFIG_SPL=y
 CONFIG_SPL_FPGA_SUPPORT=y
 CONFIG_CMD_BOOTZ=y
diff --git a/include/configs/socfpga_arria10_socdk.h b/include/configs/socfpga_arria10_socdk.h
index 83718dd..9566d46 100644
--- a/include/configs/socfpga_arria10_socdk.h
+++ b/include/configs/socfpga_arria10_socdk.h
@@ -47,6 +47,9 @@
  */
 #define CONFIG_SYS_MAX_FLASH_BANKS     1
 
+/* Generic FS loader */
+#define CONFIG_SPLASH_SOURCE
+
 /* The rest of the configuration is shared */
 #include <configs/socfpga_common.h>
 
diff --git a/include/splash.h b/include/splash.h
index 228aff4..83a6890 100644
--- a/include/splash.h
+++ b/include/splash.h
@@ -59,6 +59,9 @@ static inline int splash_source_load(struct splash_location *locations,
 #endif
 
 int splash_screen_prepare(void);
+int splash_select_fs_dev(struct splash_location *location);
+int splash_load_fs(struct splash_location *location, void *file_info,
+		   u32 bmp_load_addr, size_t bsize);
 
 #ifdef CONFIG_SPLASH_SCREEN_ALIGN
 void splash_get_pos(int *x, int *y);
-- 
2.2.0

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Marek Vasut]

On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> Generic firmware loader framework contains some common functionality
> which is reusable by any specific driver file system firmware loader.
> Specific driver file system firmware loader handling can be defined
> with both weak function fsloader_preprocess and fs_loading.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

Why did you put everything into splash_source.c ? If this is supposed to
be a generic loader, I'm sure not everyone would want to enable splash
screen support to get generic firmware loader support.

The API should looks more like the linux firmware API.

> ---
>  common/Makefile                         |   7 +-
>  common/splash_source.c                  | 110 +++++++++++++++++++++++++-------
>  configs/socfpga_arria10_defconfig       |   2 +
>  include/configs/socfpga_arria10_socdk.h |   3 +
>  include/splash.h                        |   3 +
>  5 files changed, 99 insertions(+), 26 deletions(-)
> 
> diff --git a/common/Makefile b/common/Makefile
> index 801ea31..965a217 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -47,8 +47,6 @@ obj-$(CONFIG_MTD_NOR_FLASH) += flash.o
>  obj-$(CONFIG_CMD_KGDB) += kgdb.o kgdb_stubs.o
>  obj-$(CONFIG_I2C_EDID) += edid.o
>  obj-$(CONFIG_KALLSYMS) += kallsyms.o
> -obj-y += splash.o
> -obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o
>  ifndef CONFIG_DM_VIDEO
>  obj-$(CONFIG_LCD) += lcd.o lcd_console.o
>  endif
> @@ -102,7 +100,6 @@ endif
>  obj-y += image.o
>  obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
>  obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
> -obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
>  obj-$(CONFIG_FIT_EMBED) += boot_fit.o common_fit.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += image-sig.o
>  obj-$(CONFIG_IO_TRACE) += iotrace.o
> @@ -130,3 +127,7 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
>  obj-y += command.o
>  obj-y += s_record.o
>  obj-y += xyzModem.o
> +
> +obj-y += splash.o
> +obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o
> +obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
> diff --git a/common/splash_source.c b/common/splash_source.c
> index e0defde..1413945 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c
> @@ -109,7 +109,7 @@ splash_address_too_high:
>  	return -EFAULT;
>  }
>  
> -static int splash_select_fs_dev(struct splash_location *location)
> +int splash_select_fs_dev(struct splash_location *location)
>  {
>  	int res;
>  
> @@ -140,6 +140,7 @@ static int splash_select_fs_dev(struct splash_location *location)
>  	return res;
>  }
>  
> +#ifndef CONFIG_SPL_BUILD
>  #ifdef CONFIG_USB_STORAGE
>  static int splash_init_usb(void)
>  {
> @@ -175,6 +176,7 @@ static inline int splash_init_sata(void)
>  	return -ENOSYS;
>  }
>  #endif
> +#endif
>  
>  #ifdef CONFIG_CMD_UBIFS
>  static int splash_mount_ubifs(struct splash_location *location)
> @@ -213,22 +215,99 @@ static inline int splash_umount_ubifs(void)
>  
>  #define SPLASH_SOURCE_DEFAULT_FILE_NAME		"splash.bmp"
>  
> -static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
> +/**
> + * fsloader_preprocess - Any prepocessing before calling filesystem loader.
> + *
> + * @locations:		An array of supported splash locations.
> + * @file_info:		Description and attributes to the image.
> + *			Could be structure pointer, and any type pointer.
> + * @filename:		Image filename in flashes.
> + * @bmp_load_addr:	Target memory location image loaded to.
> + *
> + * @return:	If 0, processing is succesfull. Filename pointer contains
> + *		valid filename.
> + *		If -ve, processing is failed.
> + */
> +__weak int fsloader_preprocess(struct splash_location *location,
> +			       void *file_info, char **filename,
> +			       u32 bmp_load_addr)
>  {
>  	int res = 0;
>  	loff_t bmp_size;
> -	loff_t actread;
>  	char *splash_file;
>  
> -	splash_file = env_get("splashfile");
> +	splash_file = env_get((char *)file_info);
> +
>  	if (!splash_file)
>  		splash_file = SPLASH_SOURCE_DEFAULT_FILE_NAME;
>  
> +	res = splash_select_fs_dev(location);
> +	if (res) {
> +		error("Error : Failed to select FS device\n");
> +		return -EPERM;
> +	}
> +
> +	res = fs_size(splash_file, &bmp_size);
> +	if (res) {
> +		error("Error (%d): cannot determine file size\n", res);
> +		return -ENOENT;
> +	}
> +
> +	if ((u32)bmp_load_addr + bmp_size >= gd->start_addr_sp) {
> +		error("Error: splashimage address too high. Data overwrites ");
> +		error("U-Boot and/or placed beyond DRAM boundaries.\n");
> +		res = -EFAULT;
> +		return -EFAULT;
> +	}
> +
> +	*filename = splash_file;
> +
> +	return res;
> +}
> +
> +/**
> + * fs_loading - This place is for implementing whaterver blob + whatever
> + *		specific driver to the HW such as program raw binary file to
> + *		FPGA.
> + *
> + * @locations:		An array of supported splash locations.
> + * @file_info:		Description and attributes to the image.
> + *			Could be structure pointer, and any type pointer.
> + * @filename:		Image filename in flashes.
> + * @bmp_load_addr:	Target memory location image loaded to.
> + * @bsize:		Size of target memory location.
> + *
> + * @return:	If 0, loading is succesfull. Filename pointer contains
> + *		valid filename.
> + *		If non-zero, loading is failed.
> + */
> +__weak int fs_loading(struct splash_location *location, void *file_info,
> +		      char *filename, u32 bmp_load_addr, size_t bsize)
> +{
> +	loff_t actread;
> +
> +	return fs_read(filename, (u32)bmp_load_addr, 0, 0, &actread);
> +}
> +
> +int splash_load_fs(struct splash_location *location, void *file_info,
> +		   u32 bmp_load_addr, size_t bsize)
> +{
> +	int res = 0;
> +	char *splash_file = NULL;
> +
> +	res = fsloader_preprocess(location, file_info, &splash_file,
> +				  bmp_load_addr);
> +
> +	if (res)
> +		goto out;
> +
> +#ifndef CONFIG_SPL_BUILD
>  	if (location->storage == SPLASH_STORAGE_USB)
>  		res = splash_init_usb();
>  
>  	if (location->storage == SPLASH_STORAGE_SATA)
>  		res = splash_init_sata();
> +#endif
>  
>  	if (location->ubivol != NULL)
>  		res = splash_mount_ubifs(location);
> @@ -236,24 +315,8 @@ static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
>  	if (res)
>  		return res;
>  
> -	res = splash_select_fs_dev(location);
> -	if (res)
> -		goto out;
> -
> -	res = fs_size(splash_file, &bmp_size);
> -	if (res) {
> -		printf("Error (%d): cannot determine file size\n", 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");
> -		res = -EFAULT;
> -		goto out;
> -	}
> -
> -	splash_select_fs_dev(location);
> -	res = fs_read(splash_file, bmp_load_addr, 0, 0, &actread);
> +	res = fs_loading(location, file_info, splash_file, bmp_load_addr,
> +			bsize);
>  
>  out:
>  	if (location->ubivol != NULL)
> @@ -405,7 +468,8 @@ int splash_source_load(struct splash_location *locations, uint size)
>  	if (splash_location->flags == SPLASH_STORAGE_RAW)
>  		return splash_load_raw(splash_location, bmp_load_addr);
>  	else if (splash_location->flags == SPLASH_STORAGE_FS)
> -		return splash_load_fs(splash_location, bmp_load_addr);
> +		return splash_load_fs(splash_location, "splashfile",
> +				      bmp_load_addr, 0);
>  #ifdef CONFIG_FIT
>  	else if (splash_location->flags == SPLASH_STORAGE_FIT)
>  		return splash_load_fit(splash_location, bmp_load_addr);
> diff --git a/configs/socfpga_arria10_defconfig b/configs/socfpga_arria10_defconfig
> index 4c73d73..2db34b5 100644
> --- a/configs/socfpga_arria10_defconfig
> +++ b/configs/socfpga_arria10_defconfig
> @@ -7,6 +7,8 @@ CONFIG_DEFAULT_DEVICE_TREE="socfpga_arria10_socdk_sdmmc"
>  CONFIG_USE_BOOTARGS=y
>  CONFIG_BOOTARGS="console=ttyS0,115200"
>  CONFIG_DEFAULT_FDT_FILE="socfpga_arria10_socdk_sdmmc.dtb"
> +CONFIG_FIT=y
> +CONFIG_SPL_FIT=y
>  CONFIG_SPL=y
>  CONFIG_SPL_FPGA_SUPPORT=y
>  CONFIG_CMD_BOOTZ=y
> diff --git a/include/configs/socfpga_arria10_socdk.h b/include/configs/socfpga_arria10_socdk.h
> index 83718dd..9566d46 100644
> --- a/include/configs/socfpga_arria10_socdk.h
> +++ b/include/configs/socfpga_arria10_socdk.h
> @@ -47,6 +47,9 @@
>   */
>  #define CONFIG_SYS_MAX_FLASH_BANKS     1
>  
> +/* Generic FS loader */
> +#define CONFIG_SPLASH_SOURCE
> +
>  /* The rest of the configuration is shared */
>  #include <configs/socfpga_common.h>
>  
> diff --git a/include/splash.h b/include/splash.h
> index 228aff4..83a6890 100644
> --- a/include/splash.h
> +++ b/include/splash.h
> @@ -59,6 +59,9 @@ static inline int splash_source_load(struct splash_location *locations,
>  #endif
>  
>  int splash_screen_prepare(void);
> +int splash_select_fs_dev(struct splash_location *location);
> +int splash_load_fs(struct splash_location *location, void *file_info,
> +		   u32 bmp_load_addr, size_t bsize);
>  
>  #ifdef CONFIG_SPLASH_SCREEN_ALIGN
>  void splash_get_pos(int *x, int *y);
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Marek Vasut]

On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> Generic firmware loader framework contains some common functionality
> which is reusable by any specific driver file system firmware loader.
> Specific driver file system firmware loader handling can be defined
> with both weak function fsloader_preprocess and fs_loading.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

Note that this should be sent separately from the SoCFPGA stuff.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Sel, 2017-10-31 at 12:01 +0100, Marek Vasut wrote:
> On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > Generic firmware loader framework contains some common
> > functionality
> > which is reusable by any specific driver file system firmware
> > loader.
> > Specific driver file system firmware loader handling can be defined
> > with both weak function fsloader_preprocess and fs_loading.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> Why did you put everything into splash_source.c ? If this is supposed
> to
> be a generic loader, I'm sure not everyone would want to enable
> splash
> screen support to get generic firmware loader support.
> 
> The API should looks more like the linux firmware API.
> 
Initially, i created the new file called loadfs.c, which contains
common codes and generic fs firmware loader. I plan to replace fs
loader in splash_source.c at seperate patchset.

Then, i changed the codes directly on splash_source.c based on your
comment. May be i misunderstood your meaning.
> > 
> > ---
> >  common/Makefile                         |   7 +-
> >  common/splash_source.c                  | 110
> > +++++++++++++++++++++++++-------
> >  configs/socfpga_arria10_defconfig       |   2 +
> >  include/configs/socfpga_arria10_socdk.h |   3 +
> >  include/splash.h                        |   3 +
> >  5 files changed, 99 insertions(+), 26 deletions(-)
> > 
> > diff --git a/common/Makefile b/common/Makefile
> > index 801ea31..965a217 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -47,8 +47,6 @@ obj-$(CONFIG_MTD_NOR_FLASH) += flash.o
> >  obj-$(CONFIG_CMD_KGDB) += kgdb.o kgdb_stubs.o
> >  obj-$(CONFIG_I2C_EDID) += edid.o
> >  obj-$(CONFIG_KALLSYMS) += kallsyms.o
> > -obj-y += splash.o
> > -obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o
> >  ifndef CONFIG_DM_VIDEO
> >  obj-$(CONFIG_LCD) += lcd.o lcd_console.o
> >  endif
> > @@ -102,7 +100,6 @@ endif
> >  obj-y += image.o
> >  obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
> >  obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
> > -obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
> >  obj-$(CONFIG_FIT_EMBED) += boot_fit.o common_fit.o
> >  obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += image-sig.o
> >  obj-$(CONFIG_IO_TRACE) += iotrace.o
> > @@ -130,3 +127,7 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
> >  obj-y += command.o
> >  obj-y += s_record.o
> >  obj-y += xyzModem.o
> > +
> > +obj-y += splash.o
> > +obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o
> > +obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
> > diff --git a/common/splash_source.c b/common/splash_source.c
> > index e0defde..1413945 100644
> > --- a/common/splash_source.c
> > +++ b/common/splash_source.c
> > @@ -109,7 +109,7 @@ splash_address_too_high:
> >  	return -EFAULT;
> >  }
> >  
> > -static int splash_select_fs_dev(struct splash_location *location)
> > +int splash_select_fs_dev(struct splash_location *location)
> >  {
> >  	int res;
> >  
> > @@ -140,6 +140,7 @@ static int splash_select_fs_dev(struct
> > splash_location *location)
> >  	return res;
> >  }
> >  
> > +#ifndef CONFIG_SPL_BUILD
> >  #ifdef CONFIG_USB_STORAGE
> >  static int splash_init_usb(void)
> >  {
> > @@ -175,6 +176,7 @@ static inline int splash_init_sata(void)
> >  	return -ENOSYS;
> >  }
> >  #endif
> > +#endif
> >  
> >  #ifdef CONFIG_CMD_UBIFS
> >  static int splash_mount_ubifs(struct splash_location *location)
> > @@ -213,22 +215,99 @@ static inline int splash_umount_ubifs(void)
> >  
> >  #define SPLASH_SOURCE_DEFAULT_FILE_NAME		"splash.bmp
> > "
> >  
> > -static int splash_load_fs(struct splash_location *location, u32
> > bmp_load_addr)
> > +/**
> > + * fsloader_preprocess - Any prepocessing before calling
> > filesystem loader.
> > + *
> > + * @locations:		An array of supported splash
> > locations.
> > + * @file_info:		Description and attributes to the
> > image.
> > + *			Could be structure pointer, and any type
> > pointer.
> > + * @filename:		Image filename in flashes.
> > + * @bmp_load_addr:	Target memory location image loaded to.
> > + *
> > + * @return:	If 0, processing is succesfull. Filename
> > pointer contains
> > + *		valid filename.
> > + *		If -ve, processing is failed.
> > + */
> > +__weak int fsloader_preprocess(struct splash_location *location,
> > +			       void *file_info, char **filename,
> > +			       u32 bmp_load_addr)
> >  {
> >  	int res = 0;
> >  	loff_t bmp_size;
> > -	loff_t actread;
> >  	char *splash_file;
> >  
> > -	splash_file = env_get("splashfile");
> > +	splash_file = env_get((char *)file_info);
> > +
> >  	if (!splash_file)
> >  		splash_file = SPLASH_SOURCE_DEFAULT_FILE_NAME;
> >  
> > +	res = splash_select_fs_dev(location);
> > +	if (res) {
> > +		error("Error : Failed to select FS device\n");
> > +		return -EPERM;
> > +	}
> > +
> > +	res = fs_size(splash_file, &bmp_size);
> > +	if (res) {
> > +		error("Error (%d): cannot determine file size\n",
> > res);
> > +		return -ENOENT;
> > +	}
> > +
> > +	if ((u32)bmp_load_addr + bmp_size >= gd->start_addr_sp) {
> > +		error("Error: splashimage address too high. Data
> > overwrites ");
> > +		error("U-Boot and/or placed beyond DRAM
> > boundaries.\n");
> > +		res = -EFAULT;
> > +		return -EFAULT;
> > +	}
> > +
> > +	*filename = splash_file;
> > +
> > +	return res;
> > +}
> > +
> > +/**
> > + * fs_loading - This place is for implementing whaterver blob +
> > whatever
> > + *		specific driver to the HW such as program raw
> > binary file to
> > + *		FPGA.
> > + *
> > + * @locations:		An array of supported splash
> > locations.
> > + * @file_info:		Description and attributes to the
> > image.
> > + *			Could be structure pointer, and any type
> > pointer.
> > + * @filename:		Image filename in flashes.
> > + * @bmp_load_addr:	Target memory location image loaded to.
> > + * @bsize:		Size of target memory location.
> > + *
> > + * @return:	If 0, loading is succesfull. Filename pointer
> > contains
> > + *		valid filename.
> > + *		If non-zero, loading is failed.
> > + */
> > +__weak int fs_loading(struct splash_location *location, void
> > *file_info,
> > +		      char *filename, u32 bmp_load_addr, size_t
> > bsize)
> > +{
> > +	loff_t actread;
> > +
> > +	return fs_read(filename, (u32)bmp_load_addr, 0, 0,
> > &actread);
> > +}
> > +
> > +int splash_load_fs(struct splash_location *location, void
> > *file_info,
> > +		   u32 bmp_load_addr, size_t bsize)
> > +{
> > +	int res = 0;
> > +	char *splash_file = NULL;
> > +
> > +	res = fsloader_preprocess(location, file_info,
> > &splash_file,
> > +				  bmp_load_addr);
> > +
> > +	if (res)
> > +		goto out;
> > +
> > +#ifndef CONFIG_SPL_BUILD
> >  	if (location->storage == SPLASH_STORAGE_USB)
> >  		res = splash_init_usb();
> >  
> >  	if (location->storage == SPLASH_STORAGE_SATA)
> >  		res = splash_init_sata();
> > +#endif
> >  
> >  	if (location->ubivol != NULL)
> >  		res = splash_mount_ubifs(location);
> > @@ -236,24 +315,8 @@ static int splash_load_fs(struct
> > splash_location *location, u32 bmp_load_addr)
> >  	if (res)
> >  		return res;
> >  
> > -	res = splash_select_fs_dev(location);
> > -	if (res)
> > -		goto out;
> > -
> > -	res = fs_size(splash_file, &bmp_size);
> > -	if (res) {
> > -		printf("Error (%d): cannot determine file size\n",
> > 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");
> > -		res = -EFAULT;
> > -		goto out;
> > -	}
> > -
> > -	splash_select_fs_dev(location);
> > -	res = fs_read(splash_file, bmp_load_addr, 0, 0, &actread);
> > +	res = fs_loading(location, file_info, splash_file,
> > bmp_load_addr,
> > +			bsize);
> >  
> >  out:
> >  	if (location->ubivol != NULL)
> > @@ -405,7 +468,8 @@ int splash_source_load(struct splash_location
> > *locations, uint size)
> >  	if (splash_location->flags == SPLASH_STORAGE_RAW)
> >  		return splash_load_raw(splash_location,
> > bmp_load_addr);
> >  	else if (splash_location->flags == SPLASH_STORAGE_FS)
> > -		return splash_load_fs(splash_location,
> > bmp_load_addr);
> > +		return splash_load_fs(splash_location,
> > "splashfile",
> > +				      bmp_load_addr, 0);
> >  #ifdef CONFIG_FIT
> >  	else if (splash_location->flags == SPLASH_STORAGE_FIT)
> >  		return splash_load_fit(splash_location,
> > bmp_load_addr);
> > diff --git a/configs/socfpga_arria10_defconfig
> > b/configs/socfpga_arria10_defconfig
> > index 4c73d73..2db34b5 100644
> > --- a/configs/socfpga_arria10_defconfig
> > +++ b/configs/socfpga_arria10_defconfig
> > @@ -7,6 +7,8 @@
> > CONFIG_DEFAULT_DEVICE_TREE="socfpga_arria10_socdk_sdmmc"
> >  CONFIG_USE_BOOTARGS=y
> >  CONFIG_BOOTARGS="console=ttyS0,115200"
> >  CONFIG_DEFAULT_FDT_FILE="socfpga_arria10_socdk_sdmmc.dtb"
> > +CONFIG_FIT=y
> > +CONFIG_SPL_FIT=y
> >  CONFIG_SPL=y
> >  CONFIG_SPL_FPGA_SUPPORT=y
> >  CONFIG_CMD_BOOTZ=y
> > diff --git a/include/configs/socfpga_arria10_socdk.h
> > b/include/configs/socfpga_arria10_socdk.h
> > index 83718dd..9566d46 100644
> > --- a/include/configs/socfpga_arria10_socdk.h
> > +++ b/include/configs/socfpga_arria10_socdk.h
> > @@ -47,6 +47,9 @@
> >   */
> >  #define CONFIG_SYS_MAX_FLASH_BANKS     1
> >  
> > +/* Generic FS loader */
> > +#define CONFIG_SPLASH_SOURCE
> > +
> >  /* The rest of the configuration is shared */
> >  #include <configs/socfpga_common.h>
> >  
> > diff --git a/include/splash.h b/include/splash.h
> > index 228aff4..83a6890 100644
> > --- a/include/splash.h
> > +++ b/include/splash.h
> > @@ -59,6 +59,9 @@ static inline int splash_source_load(struct
> > splash_location *locations,
> >  #endif
> >  
> >  int splash_screen_prepare(void);
> > +int splash_select_fs_dev(struct splash_location *location);
> > +int splash_load_fs(struct splash_location *location, void
> > *file_info,
> > +		   u32 bmp_load_addr, size_t bsize);
> >  
> >  #ifdef CONFIG_SPLASH_SCREEN_ALIGN
> >  void splash_get_pos(int *x, int *y);
> > 
> 

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Sel, 2017-10-31 at 12:02 +0100, Marek Vasut wrote:
> On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > Generic firmware loader framework contains some common
> > functionality
> > which is reusable by any specific driver file system firmware
> > loader.
> > Specific driver file system firmware loader handling can be defined
> > with both weak function fsloader_preprocess and fs_loading.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> Note that this should be sent separately from the SoCFPGA stuff.
> 
This patch separate from SoCFPGA stuff. But, it is required by later
patch. FPGA loadfs implemented based on this patch.

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Marek Vasut]

On 10/31/2017 12:15 PM, Chee, Tien Fong wrote:
> On Sel, 2017-10-31 at 12:01 +0100, Marek Vasut wrote:
>> On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>
>>> Generic firmware loader framework contains some common
>>> functionality
>>> which is reusable by any specific driver file system firmware
>>> loader.
>>> Specific driver file system firmware loader handling can be defined
>>> with both weak function fsloader_preprocess and fs_loading.
>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>> Why did you put everything into splash_source.c ? If this is supposed
>> to
>> be a generic loader, I'm sure not everyone would want to enable
>> splash
>> screen support to get generic firmware loader support.
>>
>> The API should looks more like the linux firmware API.
>>
> Initially, i created the new file called loadfs.c, which contains
> common codes and generic fs firmware loader. I plan to replace fs
> loader in splash_source.c at seperate patchset.

No.

> Then, i changed the codes directly on splash_source.c based on your
> comment. May be i misunderstood your meaning.

I would never suggest such a thing, sorry. This should be done properly
from the getgo, with proper API and in a separate file. All the
consumers should be converted to that API.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Marek Vasut]

On 10/31/2017 12:16 PM, Chee, Tien Fong wrote:
> On Sel, 2017-10-31 at 12:02 +0100, Marek Vasut wrote:
>> On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>
>>> Generic firmware loader framework contains some common
>>> functionality
>>> which is reusable by any specific driver file system firmware
>>> loader.
>>> Specific driver file system firmware loader handling can be defined
>>> with both weak function fsloader_preprocess and fs_loading.
>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>> Note that this should be sent separately from the SoCFPGA stuff.
>>
> This patch separate from SoCFPGA stuff. But, it is required by later
> patch. FPGA loadfs implemented based on this patch.

You can note that in the cover letter.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Sel, 2017-10-31 at 12:35 +0100, Marek Vasut wrote:
> On 10/31/2017 12:16 PM, Chee, Tien Fong wrote:
> > 
> > On Sel, 2017-10-31 at 12:02 +0100, Marek Vasut wrote:
> > > 
> > > On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > 
> > > > Generic firmware loader framework contains some common
> > > > functionality
> > > > which is reusable by any specific driver file system firmware
> > > > loader.
> > > > Specific driver file system firmware loader handling can be
> > > > defined
> > > > with both weak function fsloader_preprocess and fs_loading.
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > Note that this should be sent separately from the SoCFPGA stuff.
> > > 
> > This patch separate from SoCFPGA stuff. But, it is required by
> > later
> > patch. FPGA loadfs implemented based on this patch.
> You can note that in the cover letter.
> 
Okay.

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Sel, 2017-10-31 at 12:35 +0100, Marek Vasut wrote:
> On 10/31/2017 12:15 PM, Chee, Tien Fong wrote:
> > 
> > On Sel, 2017-10-31 at 12:01 +0100, Marek Vasut wrote:
> > > 
> > > On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > 
> > > > Generic firmware loader framework contains some common
> > > > functionality
> > > > which is reusable by any specific driver file system firmware
> > > > loader.
> > > > Specific driver file system firmware loader handling can be
> > > > defined
> > > > with both weak function fsloader_preprocess and fs_loading.
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > Why did you put everything into splash_source.c ? If this is
> > > supposed
> > > to
> > > be a generic loader, I'm sure not everyone would want to enable
> > > splash
> > > screen support to get generic firmware loader support.
> > > 
> > > The API should looks more like the linux firmware API.
> > > 
> > Initially, i created the new file called loadfs.c, which contains
> > common codes and generic fs firmware loader. I plan to replace fs
> > loader in splash_source.c at seperate patchset.
> No.
> 
> > 
> > Then, i changed the codes directly on splash_source.c based on your
> > comment. May be i misunderstood your meaning.
> I would never suggest such a thing, sorry. This should be done
> properly
> from the getgo, with proper API and in a separate file. All the
> consumers should be converted to that API.
> 
Okay, that means what i did in ver3 is what you say. Just i included
the generic firmware loader into series because this is required by
fpga loadfs, then i plan to convert splash_loader to use it at later
separate patchset.

[U-Boot] [PATCH v4 06/20] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Sel, 2017-10-31 at 12:35 +0100, Marek Vasut wrote:
> On 10/31/2017 12:15 PM, Chee, Tien Fong wrote:
> > 
> > On Sel, 2017-10-31 at 12:01 +0100, Marek Vasut wrote:
> > > 
> > > On 10/31/2017 11:52 AM, tien.fong.chee at intel.com wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > 
> > > > Generic firmware loader framework contains some common
> > > > functionality
> > > > which is reusable by any specific driver file system firmware
> > > > loader.
> > > > Specific driver file system firmware loader handling can be
> > > > defined
> > > > with both weak function fsloader_preprocess and fs_loading.
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > Why did you put everything into splash_source.c ? If this is
> > > supposed
> > > to
> > > be a generic loader, I'm sure not everyone would want to enable
> > > splash
> > > screen support to get generic firmware loader support.
> > > 
> > > The API should looks more like the linux firmware API.
> > > 
> > Initially, i created the new file called loadfs.c, which contains
> > common codes and generic fs firmware loader. I plan to replace fs
> > loader in splash_source.c at seperate patchset.
> No.
> 
> > 
> > Then, i changed the codes directly on splash_source.c based on your
> > comment. May be i misunderstood your meaning.
> I would never suggest such a thing, sorry. This should be done
> properly
> from the getgo, with proper API and in a separate file. All the
> consumers should be converted to that API.
> 
I will send out separate patchset for generic firmware loader and then
switching splash_source.c to use it. Once generic firmware loader is
accepted, i will resend current series in ver5 based on accepted
generic firmware loader. In the means time, you can help me to review
current series which patch is not related to fpga loadfs.

Thanks.