diff mbox series

[U-Boot,03/19] arm: socfpga: Add driver for flash to program FPGA

Message ID 1504003561-6290-4-git-send-email-tien.fong.chee@intel.com
State Superseded
Delegated to: Marek Vasut
Headers show
Series Add FPGA, SDRAM drivers and booting to U-boot | expand

Commit Message

Chee, Tien Fong Aug. 29, 2017, 10:45 a.m. UTC
From: Tien Fong Chee <tien.fong.chee@intel.com>

This driver handles FPGA program operation from flash loading
RBF to memory and then to program FPGA.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 .../include/mach/fpga_manager_arria10.h            |   27 ++
 drivers/fpga/socfpga_arria10.c                     |  386 +++++++++++++++++++-
 include/altera.h                                   |    6 +
 include/configs/socfpga_common.h                   |    4 +
 4 files changed, 422 insertions(+), 1 deletions(-)

Comments

Marek Vasut Aug. 29, 2017, 11:55 a.m. UTC | #1
On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> This driver handles FPGA program operation from flash loading
> RBF to memory and then to program FPGA.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> ---
>  .../include/mach/fpga_manager_arria10.h            |   27 ++
>  drivers/fpga/socfpga_arria10.c                     |  386 +++++++++++++++++++-
>  include/altera.h                                   |    6 +
>  include/configs/socfpga_common.h                   |    4 +
>  4 files changed, 422 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
> index 9cbf696..93a9122 100644
> --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
> +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
> @@ -8,6 +8,8 @@
>  #ifndef _FPGA_MANAGER_ARRIA10_H_
>  #define _FPGA_MANAGER_ARRIA10_H_
>  
> +#include <asm/cache.h>
> +
>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK		BIT(0)
>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK	BIT(1)
>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 		BIT(2)
> @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {
>  	u32  imgcfg_fifo_status;
>  };
>  
> +#if defined(CONFIG_CMD_FPGA_LOADFS)
> +enum rbf_type {unknown, periph_section, core_section};
> +enum rbf_security {invalid, unencrypted, encrypted};
> +
> +struct rbf_info {
> +	enum rbf_type section;
> +	enum rbf_security security;
> +};
> +
> +struct flash_info {
> +	char *interface;
> +	char *dev_part;
> +	char *filename;
> +	int fstype;
> +	u32 remaining;
> +	u32 flash_offset;
> +	struct rbf_info rbfinfo;
> +	struct image_header header;
> +};
> +#endif
> +
>  /* Functions */
>  int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);
>  int fpgamgr_program_finish(void);
>  int is_fpgamgr_user_mode(void);
>  int fpgamgr_wait_early_user_mode(void);
> +#if defined(CONFIG_CMD_FPGA_LOADFS)
> +const char *get_cff_filename(const void *fdt, int *len, u32 core);
> +const char *get_cff_devpart(const void *fdt, int *len);
> +#endif
>  
>  #endif /* __ASSEMBLY__ */
>  
> diff --git a/drivers/fpga/socfpga_arria10.c b/drivers/fpga/socfpga_arria10.c
> index 5c1a68a..90c55e5 100644
> --- a/drivers/fpga/socfpga_arria10.c
> +++ b/drivers/fpga/socfpga_arria10.c
> @@ -13,6 +13,12 @@
>  #include <altera.h>
>  #include <common.h>
>  #include <errno.h>
> +#include <fat.h>
> +#include <fs.h>
> +#include <fdtdec.h>
> +#include <malloc.h>
> +#include <part.h>
> +#include <spl.h>
>  #include <wait_bit.h>
>  #include <watchdog.h>
>  
> @@ -22,6 +28,10 @@
>  #define COMPRESSION_OFFSET	229
>  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in ms */
>  #define FPGA_TIMEOUT_CNT	0x1000000
> +#define RBF_UNENCRYPTED		0xa65c
> +#define RBF_ENCRYPTED		0xa65d
> +#define ARRIA10RBF_PERIPH	0x0001
> +#define ARRIA10RBF_CORE		0x8001
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -118,7 +128,7 @@ static int wait_for_nconfig_pin_and_nstatus_pin(void)
>  	return wait_for_bit(__func__,
>  			    &fpga_manager_base->imgcfg_stat,
>  			    mask,
> -			    false, FPGA_TIMEOUT_MSEC, false);
> +			    true, FPGA_TIMEOUT_MSEC, false);
>  }
>  
>  static int wait_for_f2s_nstatus_pin(unsigned long value)
> @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_CMD_FPGA_LOADFS)
> +const char *get_cff_filename(const void *fdt, int *len, u32 core)
> +{
> +	const char *cff_filename = NULL;
> +	const char *cell;
> +	int nodeoffset;
> +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
> +
> +	if (nodeoffset >= 0) {
> +		if (core)
> +			cell = fdt_getprop(fdt,
> +					nodeoffset,
> +					"cffcore-file",
> +					len);
> +		else
> +			cell = fdt_getprop(fdt, nodeoffset, "cff-file", len);

This should be a property of the FPGA , not the system . You can have
multiple FPGAs and then this would become a problem.

> +
> +		if (cell)
> +			cff_filename = cell;
> +	}
> +
> +	return cff_filename;
> +}
> +
> +const char *get_cff_devpart(const void *fdt, int *len)
> +{
> +	const char *cff_devpart = NULL;
> +	const char *cell;
> +	int nodeoffset;
> +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
> +
> +		cell = fdt_getprop(fdt, nodeoffset, "cff_devpart", len);

Indent ? What is this new undocumented DT node about ?

> +		if (cell)
> +			cff_devpart = cell;
> +
> +	return cff_devpart;
> +}
> +
> +void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer)
> +{
> +	/*
> +	  Magic ID starting at:
> +	   -> 1st dword in periph.rbf
> +	   -> 2nd dword in core.rbf
> +	*/

Checkpatch should complain about incorrect multiline comment style here ...

> +	u32 word_reading_max = 2;
> +	u32 i;
> +
> +	for(i = 0; i < word_reading_max; i++)
> +	{
> +		if(RBF_UNENCRYPTED == *(buffer + i)) /* PERIPH RBF */
> +			rbf->security = unencrypted;
> +		else if (RBF_ENCRYPTED == *(buffer + i))
> +			rbf->security = encrypted;
> +		else if (RBF_UNENCRYPTED == *(buffer + i + 1)) /* CORE RBF */
> +					rbf->security = unencrypted;
> +		else if (RBF_ENCRYPTED == *(buffer + i + 1))
> +					rbf->security = encrypted;
> +		else {
> +			rbf->security = invalid;
> +			continue;
> +		}
> +
> +		/* PERIPH RBF */
> +		if (ARRIA10RBF_PERIPH == *(buffer + i + 1)) {
> +			rbf->section = periph_section;
> +			break;
> +		}
> +		else if (ARRIA10RBF_CORE == *(buffer + i + 1)) {
> +			rbf->section = core_section;
> +			break;
> +		} /* CORE RBF */
> +		else if (ARRIA10RBF_PERIPH == *(buffer + i + 2)) {
> +			rbf->section = periph_section;
> +			break;
> +		}
> +		else if (ARRIA10RBF_CORE == *(buffer + i + 2)) {
> +			rbf->section = core_section;
> +			break;
> +		}
> +		else {

} else { ... coding style ...

> +			rbf->section = unknown;
> +			break;
> +		}
> +	}
> +
> +	return;
> +}
> +
> +static int flash_read(struct flash_info *flashinfo,
> +	u32 size_read,
> +	u32 *buffer_ptr)
> +{
> +	size_t ret = EEXIST;
> +	loff_t actread = 0;
> +
> +#ifdef CONFIG_FS_FAT
> +		ret = fat_read_file(flashinfo->filename,
> +				buffer_ptr, flashinfo->flash_offset,
> +				 size_read, &actread);
> +#endif

How can a generic FPGA driver depend on random FS functionality ?
This is broken ...

> +		if (ret || actread != size_read) {
> +			printf("Failed to read %s from flash %d ",
> +				flashinfo->filename,
> +				 ret);
> +			printf("!= %d.\n", size_read);
> +			return -EPERM;
> +		} else
> +			ret = actread;
> +
> +	return ret;
> +}
> +
> +static int fs_flash_preinit(struct flash_info *flashinfo,
> +	u32 *buffer, u32 *buffer_sizebytes)

Is this an FPGA driver or MTD driver ?

> +{
> +	u32 *bufferptr_after_header = NULL;
> +	u32 buffersize_after_header = 0;
> +	u32 rbf_header_data_size = 0;
> +	int ret = 0;
> +
> +	flashinfo->flash_offset = 0;
> +
> +	/* To avoid from keeping re-read the contents */
> +	struct image_header *header = &(flashinfo->header);
> +	size_t buffer_size = *buffer_sizebytes;
> +	u32 *buffer_ptr = (u32 *)*buffer;
> +
> +

Two newlines ... fix

> +	 /* Load mkimage header into buffer */
> +	ret = flash_read(flashinfo,
> +			sizeof(struct image_header), buffer_ptr);
> +
> +	if (0 >= ret) {
> +		printf(" Failed to read mkimage header from flash.\n");
> +		return -ENOENT;
> +	}
> +
> +	WATCHDOG_RESET();
> +
> +	memcpy(header, (u_char *)buffer_ptr, sizeof(*header));
> +
> +	if (!image_check_magic(header)) {
> +		printf("FPGA: Bad Magic Number.\n");
> +		return -EBADF;
> +	}
> +
> +	if (!image_check_hcrc(header)) {
> +		printf("FPGA: Bad Header Checksum.\n");
> +		return -EPERM;
> +	}
> +
> +	/* Getting rbf data size */
> +	flashinfo->remaining =
> +		image_get_data_size(header);
> +
> +	/* Calculate total size of both rbf data with mkimage header */
> +	rbf_header_data_size = flashinfo->remaining +
> +				sizeof(struct image_header);
> +
> +	/* Loading to buffer chunk by chunk, normally for OCRAM buffer */
> +	if (rbf_header_data_size > buffer_size) {
> +		/* Calculate size of rbf data in the buffer */
> +		buffersize_after_header =
> +			buffer_size - sizeof(struct image_header);
> +		flashinfo->remaining -= buffersize_after_header;
> +	} else {
> +	/* Loading whole rbf image into buffer, normally for DDR buffer */
> +		buffer_size = rbf_header_data_size;
> +		/* Calculate size of rbf data in the buffer */
> +		buffersize_after_header =
> +			buffer_size - sizeof(struct image_header);
> +		flashinfo->remaining = 0;
> +	}
> +
> +	/* Loading mkimage header and rbf data into buffer */
> +	ret = flash_read(flashinfo, buffer_size, buffer_ptr);
> +
> +	if (0 >= ret) {
> +		printf(" Failed to read mkimage header and rbf data ");
> +		printf("from flash.\n");
> +		return -ENOENT;
> +	}
> +
> +	/* Getting pointer of rbf data starting address where is it
> +	   right after mkimage header */
> +	bufferptr_after_header =
> +		(u32 *)((u_char *)buffer_ptr + sizeof(struct image_header));
> +
> +	/* Update next reading rbf data flash offset */
> +	flashinfo->flash_offset += buffer_size;
> +
> +	/* Update the starting addr of rbf data to init FPGA & programming
> +	   into FPGA */
> +	*buffer = (u32)bufferptr_after_header;
> +
> +	get_rbf_image_info(&flashinfo->rbfinfo, (u16 *)bufferptr_after_header);
> +
> +	/* Update the size of rbf data to be programmed into FPGA */
> +	*buffer_sizebytes = buffersize_after_header;
> +
> +#ifdef CONFIG_CHECK_FPGA_DATA_CRC
> +	flashinfo->datacrc =
> +		crc32(flashinfo->datacrc,
> +		(u_char *)bufferptr_after_header,
> +		buffersize_after_header);
> +#endif
> +
> +if (0 == flashinfo->remaining) {
> +#ifdef CONFIG_CHECK_FPGA_DATA_CRC
> +	if (flashinfo->datacrc !=
> +		image_get_dcrc(&(flashinfo->header))) {
> +		printf("FPGA: Bad Data Checksum.\n");
> +		return -EPERM;
> +	}
> +#endif
> +}
> +	return 0;
> +}
> +
> +static int fs_flash_read(struct flash_info *flashinfo, u32 *buffer,
> +	u32 *buffer_sizebytes)
> +{
> +	int ret = 0;
> +	/* To avoid from keeping re-read the contents */
> +	size_t buffer_size = *buffer_sizebytes;
> +	u32 *buffer_ptr = (u32 *)*buffer;
> +	u32 flash_addr = flashinfo->flash_offset;
> +
> +	/* Buffer allocated in OCRAM */
> +	/* Read the data by small chunk by chunk. */
> +	if (flashinfo->remaining > buffer_size)
> +		flashinfo->remaining -= buffer_size;
> +	else {
> +		/* Buffer allocated in DDR, larger than rbf data most
> +		  of the time */
> +		buffer_size = flashinfo->remaining;
> +		flashinfo->remaining = 0;
> +	}
> +
> +	ret = flash_read(flashinfo, buffer_size, buffer_ptr);
> +
> +	if (0 >= ret) {
> +		printf(" Failed to read rbf data from flash.\n");
> +		return -ENOENT;
> +	}
> +
> +#ifdef CONFIG_CHECK_FPGA_DATA_CRC
> +	flashinfo->datacrc =
> +		crc32(flashinfo->datacrc,
> +			(unsigned char *)buffer_ptr, buffer_size);
> +#endif
> +
> +if (0 == flashinfo->remaining) {
> +#ifdef CONFIG_CHECK_FPGA_DATA_CRC
> +	if (flashinfo->datacrc !=
> +		image_get_dcrc(&(flashinfo->header))) {
> +		printf("FPGA: Bad Data Checksum.\n");
> +		return -EPERM;
> +	}
> +#endif
> +}
> +	/* Update next reading rbf data flash offset */
> +	flash_addr += buffer_size;
> +
> +	flashinfo->flash_offset = flash_addr;
> +
> +	/* Update the size of rbf data to be programmed into FPGA */
> +	*buffer_sizebytes = buffer_size;
> +
> +	return 0;
> +}
> +
>  /*
>   * FPGA Manager to program the FPGA. This is the interface used by FPGA driver.
>   * Return 0 for sucess, non-zero for error.
> @@ -469,6 +754,7 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size)
>  
>  	/* Initialize the FPGA Manager */
>  	status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
> +
>  	if (status)
>  		return status;
>  
> @@ -477,3 +763,101 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size)
>  
>  	return fpgamgr_program_finish();
>  }
> +
> +int socfpga_loadfs(Altera_desc *desc, const void *buf, size_t bsize,
> +		   fpga_fs_info *fpga_fsinfo)
> +{
> +	u32 buffer = 0;
> +	u32 buffer_ori = 0;
> +	size_t buffer_sizebytes = 0;
> +	size_t buffer_sizebytes_ori = 0;
> +	struct flash_info flashinfo;
> +	u32 status = 0;
> +	int ret = 0;
> +
> +	memset(&flashinfo, 0, sizeof(flashinfo));
> +
> +	if (fpga_fsinfo->filename == NULL) {
> +		printf("no peripheral RBF filename specified.\n");
> +		return -EINVAL;
> +	}
> +
> +	WATCHDOG_RESET();
> +
> +	buffer_sizebytes = buffer_sizebytes_ori = bsize;
> +	buffer = buffer_ori = (u32) buf;
> +	flashinfo.interface = fpga_fsinfo->interface;
> +	flashinfo.dev_part = fpga_fsinfo->dev_part;
> +	flashinfo.filename = fpga_fsinfo->filename;
> +	flashinfo.fstype = fpga_fsinfo->fstype;
> +
> +#ifndef CONFIG_SPL_BUILD
> +	if (fs_set_blk_dev(flashinfo.interface, flashinfo.dev_part,
> +				 flashinfo.fstype))
> +	return FPGA_FAIL;
> +#endif
> +
> +	/* Note: Both buffer and buffer_sizebytes values can be altered by
> +	   function below. */
> +	ret = fs_flash_preinit(&flashinfo, &buffer, &buffer_sizebytes);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (periph_section == flashinfo.rbfinfo.section) {
> +		/* Initialize the FPGA Manager */
> +		status = fpgamgr_program_init((u32 *)buffer, buffer_sizebytes);
> +		if (status) {
> +			printf("FPGA: Init with periph rbf failed with error. ");
> +			printf("code %d\n", status);
> +			return -EPERM;
> +		}
> +	}
> +
> +	WATCHDOG_RESET();
> +
> +	/* Transfer data to FPGA Manager */
> +	fpgamgr_program_write((void *)buffer,
> +		buffer_sizebytes);
> +
> +	WATCHDOG_RESET();
> +
> +	while (flashinfo.remaining) {
> +		ret = fs_flash_read(&flashinfo, &buffer_ori,
> +			&buffer_sizebytes_ori);
> +
> +		if (ret)
> +			return ret;
> +
> +		/* transfer data to FPGA Manager */
> +		fpgamgr_program_write((void *)buffer_ori,
> +			buffer_sizebytes_ori);
> +
> +		WATCHDOG_RESET();
> +	}
> +
> +	if (periph_section == flashinfo.rbfinfo.section) {
> +		if (-ETIMEDOUT != fpgamgr_wait_early_user_mode())
> +			printf("FPGA: Early Release Succeeded.\n");
> +		else {
> +			printf("FPGA: Failed to see Early Release.\n");
> +			return -EIO;
> +		}
> +	} else if (core_section == flashinfo.rbfinfo.section) {
> +		/* Ensure the FPGA entering config done */
> +		status = fpgamgr_program_finish();
> +		if (status)
> +			return status;
> +		else
> +			printf("FPGA: Enter user mode.\n");
> +
> +	} else {
> +		printf("Config Error: Unsupported FGPA raw binary type.\n");
> +		return -ENOEXEC;
> +	}
> +
> +	WATCHDOG_RESET();
> +	return 1;
> +
> +}
> +#endif
> diff --git a/include/altera.h b/include/altera.h
> index 48d3eb7..0597e8a 100644
> --- a/include/altera.h
> +++ b/include/altera.h
> @@ -84,6 +84,10 @@ typedef struct {
>  extern int altera_load(Altera_desc *desc, const void *image, size_t size);
>  extern int altera_dump(Altera_desc *desc, const void *buf, size_t bsize);
>  extern int altera_info(Altera_desc *desc);
> +#if defined(CONFIG_CMD_FPGA_LOADFS)
> +int altera_loadfs(Altera_desc *desc, const void *buf, size_t bsize,
> +		   fpga_fs_info *fpga_fsinfo);
> +#endif
>  
>  /* Board specific implementation specific function types
>   *********************************************************************/
> @@ -111,6 +115,8 @@ typedef struct {
>  
>  #ifdef CONFIG_FPGA_SOCFPGA
>  int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size);
> +int socfpga_loadfs(Altera_desc *desc, const void *buf, size_t bsize,
> +		   fpga_fs_info *fpga_fsinfo);
>  #endif
>  
>  #ifdef CONFIG_FPGA_STRATIX_V
> diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
> index 9be9e79..c15d244 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -27,7 +27,11 @@
>   */
>  #define CONFIG_NR_DRAM_BANKS		1
>  #define PHYS_SDRAM_1			0x0
> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>  #define CONFIG_SYS_MALLOC_LEN		(64 * 1024 * 1024)
> +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> +#define CONFIG_SYS_MALLOC_LEN		(128 * 1024 * 1024)
> +#endif

128 MiB malloc area is nonsense, even those 64 MiB are iffy. Why would
you ever need that in a bootloader ?

>  #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1
>  #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1_SIZE
>  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>
Chee, Tien Fong Aug. 30, 2017, 8:05 a.m. UTC | #2
On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:
> On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:

> > 

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

> > 

> > This driver handles FPGA program operation from flash loading

> > RBF to memory and then to program FPGA.

> > 

> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

> > ---

> >  .../include/mach/fpga_manager_arria10.h            |   27 ++

> >  drivers/fpga/socfpga_arria10.c                     |  386

> > +++++++++++++++++++-

> >  include/altera.h                                   |    6 +

> >  include/configs/socfpga_common.h                   |    4 +

> >  4 files changed, 422 insertions(+), 1 deletions(-)

> > 

> > diff --git a/arch/arm/mach-

> > socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-

> > socfpga/include/mach/fpga_manager_arria10.h

> > index 9cbf696..93a9122 100644

> > --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h

> > +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h

> > @@ -8,6 +8,8 @@

> >  #ifndef _FPGA_MANAGER_ARRIA10_H_

> >  #define _FPGA_MANAGER_ARRIA10_H_

> >  

> > +#include <asm/cache.h>

> > +

> >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK		

> > BIT(0)

> >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK	

> > BIT(1)

> >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 		

> > BIT(2)

> > @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {

> >  	u32  imgcfg_fifo_status;

> >  };

> >  

> > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > +enum rbf_type {unknown, periph_section, core_section};

> > +enum rbf_security {invalid, unencrypted, encrypted};

> > +

> > +struct rbf_info {

> > +	enum rbf_type section;

> > +	enum rbf_security security;

> > +};

> > +

> > +struct flash_info {

> > +	char *interface;

> > +	char *dev_part;

> > +	char *filename;

> > +	int fstype;

> > +	u32 remaining;

> > +	u32 flash_offset;

> > +	struct rbf_info rbfinfo;

> > +	struct image_header header;

> > +};

> > +#endif

> > +

> >  /* Functions */

> >  int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);

> >  int fpgamgr_program_finish(void);

> >  int is_fpgamgr_user_mode(void);

> >  int fpgamgr_wait_early_user_mode(void);

> > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > +const char *get_cff_filename(const void *fdt, int *len, u32 core);

> > +const char *get_cff_devpart(const void *fdt, int *len);

> > +#endif

> >  

> >  #endif /* __ASSEMBLY__ */

> >  

> > diff --git a/drivers/fpga/socfpga_arria10.c

> > b/drivers/fpga/socfpga_arria10.c

> > index 5c1a68a..90c55e5 100644

> > --- a/drivers/fpga/socfpga_arria10.c

> > +++ b/drivers/fpga/socfpga_arria10.c

> > @@ -13,6 +13,12 @@

> >  #include <altera.h>

> >  #include <common.h>

> >  #include <errno.h>

> > +#include <fat.h>

> > +#include <fs.h>

> > +#include <fdtdec.h>

> > +#include <malloc.h>

> > +#include <part.h>

> > +#include <spl.h>

> >  #include <wait_bit.h>

> >  #include <watchdog.h>

> >  

> > @@ -22,6 +28,10 @@

> >  #define COMPRESSION_OFFSET	229

> >  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in ms */

> >  #define FPGA_TIMEOUT_CNT	0x1000000

> > +#define RBF_UNENCRYPTED		0xa65c

> > +#define RBF_ENCRYPTED		0xa65d

> > +#define ARRIA10RBF_PERIPH	0x0001

> > +#define ARRIA10RBF_CORE		0x8001

> >  

> >  DECLARE_GLOBAL_DATA_PTR;

> >  

> > @@ -118,7 +128,7 @@ static int

> > wait_for_nconfig_pin_and_nstatus_pin(void)

> >  	return wait_for_bit(__func__,

> >  			    &fpga_manager_base->imgcfg_stat,

> >  			    mask,

> > -			    false, FPGA_TIMEOUT_MSEC, false);

> > +			    true, FPGA_TIMEOUT_MSEC, false);

> >  }

> >  

> >  static int wait_for_f2s_nstatus_pin(unsigned long value)

> > @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)

> >  	return 0;

> >  }

> >  

> > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > +const char *get_cff_filename(const void *fdt, int *len, u32 core)

> > +{

> > +	const char *cff_filename = NULL;

> > +	const char *cell;

> > +	int nodeoffset;

> > +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");

> > +

> > +	if (nodeoffset >= 0) {

> > +		if (core)

> > +			cell = fdt_getprop(fdt,

> > +					nodeoffset,

> > +					"cffcore-file",

> > +					len);

> > +		else

> > +			cell = fdt_getprop(fdt, nodeoffset, "cff-

> > file", len);

> This should be a property of the FPGA , not the system . You can have

> multiple FPGAs and then this would become a problem.

> 

This setting is for the only one FPGA inside our SoCFPGA. For external
multiple FPGAs programming, user is adviced to store the FPGA filename
in environment variable and programming FPGA with fpga loadfs command.

Please note that, peripheral rbf and partition are required in SPL to
set up DDR before booting to U-boot.

> > 

> > +

> > +		if (cell)

> > +			cff_filename = cell;

> > +	}

> > +

> > +	return cff_filename;

> > +}

> > +

> > +const char *get_cff_devpart(const void *fdt, int *len)

> > +{

> > +	const char *cff_devpart = NULL;

> > +	const char *cell;

> > +	int nodeoffset;

> > +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");

> > +

> > +		cell = fdt_getprop(fdt, nodeoffset, "cff_devpart",

> > len);

> Indent ? What is this new undocumented DT node about ?

> 

You can look the dtbinding doc on patch 12.
> > 

> > +		if (cell)

> > +			cff_devpart = cell;

> > +

> > +	return cff_devpart;

> > +}

> > +

> > +void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer)

> > +{

> > +	/*

> > +	  Magic ID starting at:

> > +	   -> 1st dword in periph.rbf

> > +	   -> 2nd dword in core.rbf

> > +	*/

> Checkpatch should complain about incorrect multiline comment style

> here ...

> 

Okay. I will fix that.
> > 

> > +	u32 word_reading_max = 2;

> > +	u32 i;

> > +

> > +	for(i = 0; i < word_reading_max; i++)

> > +	{

> > +		if(RBF_UNENCRYPTED == *(buffer + i)) /* PERIPH RBF

> > */

> > +			rbf->security = unencrypted;

> > +		else if (RBF_ENCRYPTED == *(buffer + i))

> > +			rbf->security = encrypted;

> > +		else if (RBF_UNENCRYPTED == *(buffer + i + 1)) /*

> > CORE RBF */

> > +					rbf->security =

> > unencrypted;

> > +		else if (RBF_ENCRYPTED == *(buffer + i + 1))

> > +					rbf->security = encrypted;

> > +		else {

> > +			rbf->security = invalid;

> > +			continue;

> > +		}

> > +

> > +		/* PERIPH RBF */

> > +		if (ARRIA10RBF_PERIPH == *(buffer + i + 1)) {

> > +			rbf->section = periph_section;

> > +			break;

> > +		}

> > +		else if (ARRIA10RBF_CORE == *(buffer + i + 1)) {

> > +			rbf->section = core_section;

> > +			break;

> > +		} /* CORE RBF */

> > +		else if (ARRIA10RBF_PERIPH == *(buffer + i + 2)) {

> > +			rbf->section = periph_section;

> > +			break;

> > +		}

> > +		else if (ARRIA10RBF_CORE == *(buffer + i + 2)) {

> > +			rbf->section = core_section;

> > +			break;

> > +		}

> > +		else {

> } else { ... coding style ...

> 

Okay, i will fix that. This apply to else if?
> > 

> > +			rbf->section = unknown;

> > +			break;

> > +		}

> > +	}

> > +

> > +	return;

> > +}

> > +

> > +static int flash_read(struct flash_info *flashinfo,

> > +	u32 size_read,

> > +	u32 *buffer_ptr)

> > +{

> > +	size_t ret = EEXIST;

> > +	loff_t actread = 0;

> > +

> > +#ifdef CONFIG_FS_FAT

> > +		ret = fat_read_file(flashinfo->filename,

> > +				buffer_ptr, flashinfo-

> > >flash_offset,

> > +				 size_read, &actread);

> > +#endif

> How can a generic FPGA driver depend on random FS functionality ?

> This is broken ...

> 

random FS? There would having FAT FS for SDMMC, and UBI FS for QSPI and
NAND(implement later). May be i can replace #ifdef CONFIG_FS_FAT witht
the codes below, what do you think?
	bootdev.boot_device = spl_boot_device();

	if(BOOT_DEVICE_MMC1 == bootdev.boot_device)
	{ ... }
> > 

> > +		if (ret || actread != size_read) {

> > +			printf("Failed to read %s from flash %d ",

> > +				flashinfo->filename,

> > +				 ret);

> > +			printf("!= %d.\n", size_read);

> > +			return -EPERM;

> > +		} else

> > +			ret = actread;

> > +

> > +	return ret;

> > +}

> > +

> > +static int fs_flash_preinit(struct flash_info *flashinfo,

> > +	u32 *buffer, u32 *buffer_sizebytes)

> Is this an FPGA driver or MTD driver ?

> 

This is FPGA driver. Reading header of rbf, getting filesize and
adjusting the beginning offset of RBF. 
> > 

> > +{

> > +	u32 *bufferptr_after_header = NULL;

> > +	u32 buffersize_after_header = 0;

> > +	u32 rbf_header_data_size = 0;

> > +	int ret = 0;

> > +

> > +	flashinfo->flash_offset = 0;

> > +

> > +	/* To avoid from keeping re-read the contents */

> > +	struct image_header *header = &(flashinfo->header);

> > +	size_t buffer_size = *buffer_sizebytes;

> > +	u32 *buffer_ptr = (u32 *)*buffer;

> > +

> > +

> Two newlines ... fix

> 

Okay, i will remove.
> > 

> > +	 /* Load mkimage header into buffer */

> > +	ret = flash_read(flashinfo,

> > +			sizeof(struct image_header), buffer_ptr);

> > +

> > +	if (0 >= ret) {

> > +		printf(" Failed to read mkimage header from

> > flash.\n");

> > +		return -ENOENT;

> > +	}

> > +

> > +	WATCHDOG_RESET();

> > +

> > +	memcpy(header, (u_char *)buffer_ptr, sizeof(*header));

> > +

> > +	if (!image_check_magic(header)) {

> > +		printf("FPGA: Bad Magic Number.\n");

> > +		return -EBADF;

> > +	}

> > +

> > +	if (!image_check_hcrc(header)) {

> > +		printf("FPGA: Bad Header Checksum.\n");

> > +		return -EPERM;

> > +	}

> > +

> > +	/* Getting rbf data size */

> > +	flashinfo->remaining =

> > +		image_get_data_size(header);

> > +

> > +	/* Calculate total size of both rbf data with mkimage

> > header */

> > +	rbf_header_data_size = flashinfo->remaining +

> > +				sizeof(struct image_header);

> > +

> > +	/* Loading to buffer chunk by chunk, normally for OCRAM

> > buffer */

> > +	if (rbf_header_data_size > buffer_size) {

> > +		/* Calculate size of rbf data in the buffer */

> > +		buffersize_after_header =

> > +			buffer_size - sizeof(struct image_header);

> > +		flashinfo->remaining -= buffersize_after_header;

> > +	} else {

> > +	/* Loading whole rbf image into buffer, normally for DDR

> > buffer */

> > +		buffer_size = rbf_header_data_size;

> > +		/* Calculate size of rbf data in the buffer */

> > +		buffersize_after_header =

> > +			buffer_size - sizeof(struct image_header);

> > +		flashinfo->remaining = 0;

> > +	}

> > +

> > +	/* Loading mkimage header and rbf data into buffer */

> > +	ret = flash_read(flashinfo, buffer_size, buffer_ptr);

> > +

> > +	if (0 >= ret) {

> > +		printf(" Failed to read mkimage header and rbf

> > data ");

> > +		printf("from flash.\n");

> > +		return -ENOENT;

> > +	}

> > +

> > +	/* Getting pointer of rbf data starting address where is

> > it

> > +	   right after mkimage header */

> > +	bufferptr_after_header =

> > +		(u32 *)((u_char *)buffer_ptr + sizeof(struct

> > image_header));

> > +

> > +	/* Update next reading rbf data flash offset */

> > +	flashinfo->flash_offset += buffer_size;

> > +

> > +	/* Update the starting addr of rbf data to init FPGA &

> > programming

> > +	   into FPGA */

> > +	*buffer = (u32)bufferptr_after_header;

> > +

> > +	get_rbf_image_info(&flashinfo->rbfinfo, (u16

> > *)bufferptr_after_header);

> > +

> > +	/* Update the size of rbf data to be programmed into FPGA

> > */

> > +	*buffer_sizebytes = buffersize_after_header;

> > +

> > +#ifdef CONFIG_CHECK_FPGA_DATA_CRC

> > +	flashinfo->datacrc =

> > +		crc32(flashinfo->datacrc,

> > +		(u_char *)bufferptr_after_header,

> > +		buffersize_after_header);

> > +#endif

> > +

> > +if (0 == flashinfo->remaining) {

> > +#ifdef CONFIG_CHECK_FPGA_DATA_CRC

> > +	if (flashinfo->datacrc !=

> > +		image_get_dcrc(&(flashinfo->header))) {

> > +		printf("FPGA: Bad Data Checksum.\n");

> > +		return -EPERM;

> > +	}

> > +#endif

> > +}

> > +	return 0;

> > +}

> > +

> > +static int fs_flash_read(struct flash_info *flashinfo, u32

> > *buffer,

> > +	u32 *buffer_sizebytes)

> > +{

> > +	int ret = 0;

> > +	/* To avoid from keeping re-read the contents */

> > +	size_t buffer_size = *buffer_sizebytes;

> > +	u32 *buffer_ptr = (u32 *)*buffer;

> > +	u32 flash_addr = flashinfo->flash_offset;

> > +

> > +	/* Buffer allocated in OCRAM */

> > +	/* Read the data by small chunk by chunk. */

> > +	if (flashinfo->remaining > buffer_size)

> > +		flashinfo->remaining -= buffer_size;

> > +	else {

> > +		/* Buffer allocated in DDR, larger than rbf data

> > most

> > +		  of the time */

> > +		buffer_size = flashinfo->remaining;

> > +		flashinfo->remaining = 0;

> > +	}

> > +

> > +	ret = flash_read(flashinfo, buffer_size, buffer_ptr);

> > +

> > +	if (0 >= ret) {

> > +		printf(" Failed to read rbf data from flash.\n");

> > +		return -ENOENT;

> > +	}

> > +

> > +#ifdef CONFIG_CHECK_FPGA_DATA_CRC

> > +	flashinfo->datacrc =

> > +		crc32(flashinfo->datacrc,

> > +			(unsigned char *)buffer_ptr, buffer_size);

> > +#endif

> > +

> > +if (0 == flashinfo->remaining) {

> > +#ifdef CONFIG_CHECK_FPGA_DATA_CRC

> > +	if (flashinfo->datacrc !=

> > +		image_get_dcrc(&(flashinfo->header))) {

> > +		printf("FPGA: Bad Data Checksum.\n");

> > +		return -EPERM;

> > +	}

> > +#endif

> > +}

> > +	/* Update next reading rbf data flash offset */

> > +	flash_addr += buffer_size;

> > +

> > +	flashinfo->flash_offset = flash_addr;

> > +

> > +	/* Update the size of rbf data to be programmed into FPGA

> > */

> > +	*buffer_sizebytes = buffer_size;

> > +

> > +	return 0;

> > +}

> > +

> >  /*

> >   * FPGA Manager to program the FPGA. This is the interface used by

> > FPGA driver.

> >   * Return 0 for sucess, non-zero for error.

> > @@ -469,6 +754,7 @@ int socfpga_load(Altera_desc *desc, const void

> > *rbf_data, size_t rbf_size)

> >  

> >  	/* Initialize the FPGA Manager */

> >  	status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);

> > +

> >  	if (status)

> >  		return status;

> >  

> > @@ -477,3 +763,101 @@ int socfpga_load(Altera_desc *desc, const

> > void *rbf_data, size_t rbf_size)

> >  

> >  	return fpgamgr_program_finish();

> >  }

> > +

> > +int socfpga_loadfs(Altera_desc *desc, const void *buf, size_t

> > bsize,

> > +		   fpga_fs_info *fpga_fsinfo)

> > +{

> > +	u32 buffer = 0;

> > +	u32 buffer_ori = 0;

> > +	size_t buffer_sizebytes = 0;

> > +	size_t buffer_sizebytes_ori = 0;

> > +	struct flash_info flashinfo;

> > +	u32 status = 0;

> > +	int ret = 0;

> > +

> > +	memset(&flashinfo, 0, sizeof(flashinfo));

> > +

> > +	if (fpga_fsinfo->filename == NULL) {

> > +		printf("no peripheral RBF filename specified.\n");

> > +		return -EINVAL;

> > +	}

> > +

> > +	WATCHDOG_RESET();

> > +

> > +	buffer_sizebytes = buffer_sizebytes_ori = bsize;

> > +	buffer = buffer_ori = (u32) buf;

> > +	flashinfo.interface = fpga_fsinfo->interface;

> > +	flashinfo.dev_part = fpga_fsinfo->dev_part;

> > +	flashinfo.filename = fpga_fsinfo->filename;

> > +	flashinfo.fstype = fpga_fsinfo->fstype;

> > +

> > +#ifndef CONFIG_SPL_BUILD

> > +	if (fs_set_blk_dev(flashinfo.interface,

> > flashinfo.dev_part,

> > +				 flashinfo.fstype))

> > +	return FPGA_FAIL;

> > +#endif

> > +

> > +	/* Note: Both buffer and buffer_sizebytes values can be

> > altered by

> > +	   function below. */

> > +	ret = fs_flash_preinit(&flashinfo, &buffer,

> > &buffer_sizebytes);

> > +

> > +	if (ret)

> > +		return ret;

> > +

> > +	if (periph_section == flashinfo.rbfinfo.section) {

> > +		/* Initialize the FPGA Manager */

> > +		status = fpgamgr_program_init((u32 *)buffer,

> > buffer_sizebytes);

> > +		if (status) {

> > +			printf("FPGA: Init with periph rbf failed

> > with error. ");

> > +			printf("code %d\n", status);

> > +			return -EPERM;

> > +		}

> > +	}

> > +

> > +	WATCHDOG_RESET();

> > +

> > +	/* Transfer data to FPGA Manager */

> > +	fpgamgr_program_write((void *)buffer,

> > +		buffer_sizebytes);

> > +

> > +	WATCHDOG_RESET();

> > +

> > +	while (flashinfo.remaining) {

> > +		ret = fs_flash_read(&flashinfo, &buffer_ori,

> > +			&buffer_sizebytes_ori);

> > +

> > +		if (ret)

> > +			return ret;

> > +

> > +		/* transfer data to FPGA Manager */

> > +		fpgamgr_program_write((void *)buffer_ori,

> > +			buffer_sizebytes_ori);

> > +

> > +		WATCHDOG_RESET();

> > +	}

> > +

> > +	if (periph_section == flashinfo.rbfinfo.section) {

> > +		if (-ETIMEDOUT != fpgamgr_wait_early_user_mode())

> > +			printf("FPGA: Early Release

> > Succeeded.\n");

> > +		else {

> > +			printf("FPGA: Failed to see Early

> > Release.\n");

> > +			return -EIO;

> > +		}

> > +	} else if (core_section == flashinfo.rbfinfo.section) {

> > +		/* Ensure the FPGA entering config done */

> > +		status = fpgamgr_program_finish();

> > +		if (status)

> > +			return status;

> > +		else

> > +			printf("FPGA: Enter user mode.\n");

> > +

> > +	} else {

> > +		printf("Config Error: Unsupported FGPA raw binary

> > type.\n");

> > +		return -ENOEXEC;

> > +	}

> > +

> > +	WATCHDOG_RESET();

> > +	return 1;

> > +

> > +}

> > +#endif

> > diff --git a/include/altera.h b/include/altera.h

> > index 48d3eb7..0597e8a 100644

> > --- a/include/altera.h

> > +++ b/include/altera.h

> > @@ -84,6 +84,10 @@ typedef struct {

> >  extern int altera_load(Altera_desc *desc, const void *image,

> > size_t size);

> >  extern int altera_dump(Altera_desc *desc, const void *buf, size_t

> > bsize);

> >  extern int altera_info(Altera_desc *desc);

> > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > +int altera_loadfs(Altera_desc *desc, const void *buf, size_t

> > bsize,

> > +		   fpga_fs_info *fpga_fsinfo);

> > +#endif

> >  

> >  /* Board specific implementation specific function types

> >  

> > *******************************************************************

> > **/

> > @@ -111,6 +115,8 @@ typedef struct {

> >  

> >  #ifdef CONFIG_FPGA_SOCFPGA

> >  int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t

> > rbf_size);

> > +int socfpga_loadfs(Altera_desc *desc, const void *buf, size_t

> > bsize,

> > +		   fpga_fs_info *fpga_fsinfo);

> >  #endif

> >  

> >  #ifdef CONFIG_FPGA_STRATIX_V

> > diff --git a/include/configs/socfpga_common.h

> > b/include/configs/socfpga_common.h

> > index 9be9e79..c15d244 100644

> > --- a/include/configs/socfpga_common.h

> > +++ b/include/configs/socfpga_common.h

> > @@ -27,7 +27,11 @@

> >   */

> >  #define CONFIG_NR_DRAM_BANKS		1

> >  #define PHYS_SDRAM_1			0x0

> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)

> >  #define CONFIG_SYS_MALLOC_LEN		(64 * 1024 * 1024)

> > +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)

> > +#define CONFIG_SYS_MALLOC_LEN		(128 * 1024 * 1024)

> > +#endif

> 128 MiB malloc area is nonsense, even those 64 MiB are iffy. Why

> would

> you ever need that in a bootloader ?

> 

This is min require to malloc the buffer in SDRAM for core rbf. Less
than this value, something would going wrong, i'm not recall what issue
because i already tested this quite long time ago, problem related to
FAT and malloc.
> > 

> >  #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1

> >  #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1_SIZE

> >  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)

> > 

>
Marek Vasut Aug. 30, 2017, 8:52 a.m. UTC | #3
On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:
> On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:
>> On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>
>>> This driver handles FPGA program operation from flash loading
>>> RBF to memory and then to program FPGA.
>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>> ---
>>>  .../include/mach/fpga_manager_arria10.h            |   27 ++
>>>  drivers/fpga/socfpga_arria10.c                     |  386
>>> +++++++++++++++++++-
>>>  include/altera.h                                   |    6 +
>>>  include/configs/socfpga_common.h                   |    4 +
>>>  4 files changed, 422 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-
>>> socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-
>>> socfpga/include/mach/fpga_manager_arria10.h
>>> index 9cbf696..93a9122 100644
>>> --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
>>> +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
>>> @@ -8,6 +8,8 @@
>>>  #ifndef _FPGA_MANAGER_ARRIA10_H_
>>>  #define _FPGA_MANAGER_ARRIA10_H_
>>>  
>>> +#include <asm/cache.h>
>>> +
>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK		
>>> BIT(0)
>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK	
>>> BIT(1)
>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 		
>>> BIT(2)
>>> @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {
>>>  	u32  imgcfg_fifo_status;
>>>  };
>>>  
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +enum rbf_type {unknown, periph_section, core_section};
>>> +enum rbf_security {invalid, unencrypted, encrypted};
>>> +
>>> +struct rbf_info {
>>> +	enum rbf_type section;
>>> +	enum rbf_security security;
>>> +};
>>> +
>>> +struct flash_info {
>>> +	char *interface;
>>> +	char *dev_part;
>>> +	char *filename;
>>> +	int fstype;
>>> +	u32 remaining;
>>> +	u32 flash_offset;
>>> +	struct rbf_info rbfinfo;
>>> +	struct image_header header;
>>> +};
>>> +#endif
>>> +
>>>  /* Functions */
>>>  int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);
>>>  int fpgamgr_program_finish(void);
>>>  int is_fpgamgr_user_mode(void);
>>>  int fpgamgr_wait_early_user_mode(void);
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +const char *get_cff_filename(const void *fdt, int *len, u32 core);
>>> +const char *get_cff_devpart(const void *fdt, int *len);
>>> +#endif
>>>  
>>>  #endif /* __ASSEMBLY__ */
>>>  
>>> diff --git a/drivers/fpga/socfpga_arria10.c
>>> b/drivers/fpga/socfpga_arria10.c
>>> index 5c1a68a..90c55e5 100644
>>> --- a/drivers/fpga/socfpga_arria10.c
>>> +++ b/drivers/fpga/socfpga_arria10.c
>>> @@ -13,6 +13,12 @@
>>>  #include <altera.h>
>>>  #include <common.h>
>>>  #include <errno.h>
>>> +#include <fat.h>
>>> +#include <fs.h>
>>> +#include <fdtdec.h>
>>> +#include <malloc.h>
>>> +#include <part.h>
>>> +#include <spl.h>
>>>  #include <wait_bit.h>
>>>  #include <watchdog.h>
>>>  
>>> @@ -22,6 +28,10 @@
>>>  #define COMPRESSION_OFFSET	229
>>>  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in ms */
>>>  #define FPGA_TIMEOUT_CNT	0x1000000
>>> +#define RBF_UNENCRYPTED		0xa65c
>>> +#define RBF_ENCRYPTED		0xa65d
>>> +#define ARRIA10RBF_PERIPH	0x0001
>>> +#define ARRIA10RBF_CORE		0x8001
>>>  
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>  
>>> @@ -118,7 +128,7 @@ static int
>>> wait_for_nconfig_pin_and_nstatus_pin(void)
>>>  	return wait_for_bit(__func__,
>>>  			    &fpga_manager_base->imgcfg_stat,
>>>  			    mask,
>>> -			    false, FPGA_TIMEOUT_MSEC, false);
>>> +			    true, FPGA_TIMEOUT_MSEC, false);
>>>  }
>>>  
>>>  static int wait_for_f2s_nstatus_pin(unsigned long value)
>>> @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)
>>>  	return 0;
>>>  }
>>>  
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +const char *get_cff_filename(const void *fdt, int *len, u32 core)
>>> +{
>>> +	const char *cff_filename = NULL;
>>> +	const char *cell;
>>> +	int nodeoffset;
>>> +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
>>> +
>>> +	if (nodeoffset >= 0) {
>>> +		if (core)
>>> +			cell = fdt_getprop(fdt,
>>> +					nodeoffset,
>>> +					"cffcore-file",
>>> +					len);
>>> +		else
>>> +			cell = fdt_getprop(fdt, nodeoffset, "cff-
>>> file", len);
>> This should be a property of the FPGA , not the system . You can have
>> multiple FPGAs and then this would become a problem.
>>
> This setting is for the only one FPGA inside our SoCFPGA.

You just said it yourself, it is for the only FPGA in your SOCFPGA ,
thus it is a property of the FPGA , not a chosen .

> For external
> multiple FPGAs programming, user is adviced to store the FPGA filename
> in environment variable and programming FPGA with fpga loadfs command.
> 
> Please note that, peripheral rbf and partition are required in SPL to
> set up DDR before booting to U-boot.
> 
>>>
>>> +
>>> +		if (cell)
>>> +			cff_filename = cell;
>>> +	}
>>> +
>>> +	return cff_filename;
>>> +}
>>> +
>>> +const char *get_cff_devpart(const void *fdt, int *len)
>>> +{
>>> +	const char *cff_devpart = NULL;
>>> +	const char *cell;
>>> +	int nodeoffset;
>>> +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
>>> +
>>> +		cell = fdt_getprop(fdt, nodeoffset, "cff_devpart",
>>> len);
>> Indent ? What is this new undocumented DT node about ?
>>
> You can look the dtbinding doc on patch 12.

Ugh, the patch should be in the series before this one.

>>>
>>> +		if (cell)
>>> +			cff_devpart = cell;
>>> +
>>> +	return cff_devpart;
>>> +}
>>> +
>>> +void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer)
>>> +{
>>> +	/*
>>> +	  Magic ID starting at:
>>> +	   -> 1st dword in periph.rbf
>>> +	   -> 2nd dword in core.rbf
>>> +	*/
>> Checkpatch should complain about incorrect multiline comment style
>> here ...
>>
> Okay. I will fix that.

Can you please run checkpatch before submitting your patches ?

>>>
>>> +	u32 word_reading_max = 2;
>>> +	u32 i;
>>> +
>>> +	for(i = 0; i < word_reading_max; i++)
>>> +	{
>>> +		if(RBF_UNENCRYPTED == *(buffer + i)) /* PERIPH RBF
>>> */
>>> +			rbf->security = unencrypted;
>>> +		else if (RBF_ENCRYPTED == *(buffer + i))
>>> +			rbf->security = encrypted;
>>> +		else if (RBF_UNENCRYPTED == *(buffer + i + 1)) /*
>>> CORE RBF */
>>> +					rbf->security =
>>> unencrypted;
>>> +		else if (RBF_ENCRYPTED == *(buffer + i + 1))
>>> +					rbf->security = encrypted;
>>> +		else {
>>> +			rbf->security = invalid;
>>> +			continue;
>>> +		}
>>> +
>>> +		/* PERIPH RBF */
>>> +		if (ARRIA10RBF_PERIPH == *(buffer + i + 1)) {
>>> +			rbf->section = periph_section;
>>> +			break;
>>> +		}
>>> +		else if (ARRIA10RBF_CORE == *(buffer + i + 1)) {
>>> +			rbf->section = core_section;
>>> +			break;
>>> +		} /* CORE RBF */
>>> +		else if (ARRIA10RBF_PERIPH == *(buffer + i + 2)) {
>>> +			rbf->section = periph_section;
>>> +			break;
>>> +		}
>>> +		else if (ARRIA10RBF_CORE == *(buffer + i + 2)) {
>>> +			rbf->section = core_section;
>>> +			break;
>>> +		}
>>> +		else {
>> } else { ... coding style ...
>>
> Okay, i will fix that. This apply to else if?
>>>
>>> +			rbf->section = unknown;
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	return;
>>> +}
>>> +
>>> +static int flash_read(struct flash_info *flashinfo,
>>> +	u32 size_read,
>>> +	u32 *buffer_ptr)
>>> +{
>>> +	size_t ret = EEXIST;
>>> +	loff_t actread = 0;
>>> +
>>> +#ifdef CONFIG_FS_FAT
>>> +		ret = fat_read_file(flashinfo->filename,
>>> +				buffer_ptr, flashinfo-
>>>> flash_offset,
>>> +				 size_read, &actread);
>>> +#endif
>> How can a generic FPGA driver depend on random FS functionality ?
>> This is broken ...
>>
> random FS? There would having FAT FS for SDMMC, and UBI FS for QSPI and
> NAND(implement later).

Driver should not depend on specific FS.

> May be i can replace #ifdef CONFIG_FS_FAT witht
> the codes below, what do you think?
> 	bootdev.boot_device = spl_boot_device();
> 
> 	if(BOOT_DEVICE_MMC1 == bootdev.boot_device)
> 	{ ... }

Nor should it depend on random boot device.

>>>
>>> +		if (ret || actread != size_read) {
>>> +			printf("Failed to read %s from flash %d ",
>>> +				flashinfo->filename,
>>> +				 ret);
>>> +			printf("!= %d.\n", size_read);
>>> +			return -EPERM;
>>> +		} else
>>> +			ret = actread;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int fs_flash_preinit(struct flash_info *flashinfo,
>>> +	u32 *buffer, u32 *buffer_sizebytes)
>> Is this an FPGA driver or MTD driver ?
>>
> This is FPGA driver. Reading header of rbf, getting filesize and
> adjusting the beginning offset of RBF. 

Why do you pass around the struct flashinfo then ?

[...]

>>> diff --git a/include/configs/socfpga_common.h
>>> b/include/configs/socfpga_common.h
>>> index 9be9e79..c15d244 100644
>>> --- a/include/configs/socfpga_common.h
>>> +++ b/include/configs/socfpga_common.h
>>> @@ -27,7 +27,11 @@
>>>   */
>>>  #define CONFIG_NR_DRAM_BANKS		1
>>>  #define PHYS_SDRAM_1			0x0
>>> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>  #define CONFIG_SYS_MALLOC_LEN		(64 * 1024 * 1024)
>>> +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>> +#define CONFIG_SYS_MALLOC_LEN		(128 * 1024 * 1024)
>>> +#endif
>> 128 MiB malloc area is nonsense, even those 64 MiB are iffy. Why
>> would
>> you ever need that in a bootloader ?
>>
> This is min require to malloc the buffer in SDRAM for core rbf. Less
> than this value, something would going wrong, i'm not recall what issue
> because i already tested this quite long time ago, problem related to
> FAT and malloc.

Isn't the loadfs supposed to read the data from the FS and program them
into the FPGA without having to read the whole bitstream into the RAM ?

>>>
>>>  #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1
>>>  #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1_SIZE
>>>  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>
Chee, Tien Fong Sept. 4, 2017, 7:08 a.m. UTC | #4
On Rab, 2017-08-30 at 10:52 +0200, Marek Vasut wrote:
> On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:

> > 

> > On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:

> > > 

> > > On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:

> > > > 

> > > > 

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

> > > > 

> > > > This driver handles FPGA program operation from flash loading

> > > > RBF to memory and then to program FPGA.

> > > > 

> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

> > > > ---

> > > >  .../include/mach/fpga_manager_arria10.h            |   27 ++

> > > >  drivers/fpga/socfpga_arria10.c                     |  386

> > > > +++++++++++++++++++-

> > > >  include/altera.h                                   |    6 +

> > > >  include/configs/socfpga_common.h                   |    4 +

> > > >  4 files changed, 422 insertions(+), 1 deletions(-)

> > > > 

> > > > diff --git a/arch/arm/mach-

> > > > socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-

> > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > index 9cbf696..93a9122 100644

> > > > --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h

> > > > +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h

> > > > @@ -8,6 +8,8 @@

> > > >  #ifndef _FPGA_MANAGER_ARRIA10_H_

> > > >  #define _FPGA_MANAGER_ARRIA10_H_

> > > >  

> > > > +#include <asm/cache.h>

> > > > +

> > > >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK		

> > > > BIT(0)

> > > >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK	

> > > > BIT(1)

> > > >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 		

> > > > BIT(2)

> > > > @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {

> > > >  	u32  imgcfg_fifo_status;

> > > >  };

> > > >  

> > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > +enum rbf_type {unknown, periph_section, core_section};

> > > > +enum rbf_security {invalid, unencrypted, encrypted};

> > > > +

> > > > +struct rbf_info {

> > > > +	enum rbf_type section;

> > > > +	enum rbf_security security;

> > > > +};

> > > > +

> > > > +struct flash_info {

> > > > +	char *interface;

> > > > +	char *dev_part;

> > > > +	char *filename;

> > > > +	int fstype;

> > > > +	u32 remaining;

> > > > +	u32 flash_offset;

> > > > +	struct rbf_info rbfinfo;

> > > > +	struct image_header header;

> > > > +};

> > > > +#endif

> > > > +

> > > >  /* Functions */

> > > >  int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);

> > > >  int fpgamgr_program_finish(void);

> > > >  int is_fpgamgr_user_mode(void);

> > > >  int fpgamgr_wait_early_user_mode(void);

> > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > +const char *get_cff_filename(const void *fdt, int *len, u32

> > > > core);

> > > > +const char *get_cff_devpart(const void *fdt, int *len);

> > > > +#endif

> > > >  

> > > >  #endif /* __ASSEMBLY__ */

> > > >  

> > > > diff --git a/drivers/fpga/socfpga_arria10.c

> > > > b/drivers/fpga/socfpga_arria10.c

> > > > index 5c1a68a..90c55e5 100644

> > > > --- a/drivers/fpga/socfpga_arria10.c

> > > > +++ b/drivers/fpga/socfpga_arria10.c

> > > > @@ -13,6 +13,12 @@

> > > >  #include <altera.h>

> > > >  #include <common.h>

> > > >  #include <errno.h>

> > > > +#include <fat.h>

> > > > +#include <fs.h>

> > > > +#include <fdtdec.h>

> > > > +#include <malloc.h>

> > > > +#include <part.h>

> > > > +#include <spl.h>

> > > >  #include <wait_bit.h>

> > > >  #include <watchdog.h>

> > > >  

> > > > @@ -22,6 +28,10 @@

> > > >  #define COMPRESSION_OFFSET	229

> > > >  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in ms */

> > > >  #define FPGA_TIMEOUT_CNT	0x1000000

> > > > +#define RBF_UNENCRYPTED		0xa65c

> > > > +#define RBF_ENCRYPTED		0xa65d

> > > > +#define ARRIA10RBF_PERIPH	0x0001

> > > > +#define ARRIA10RBF_CORE		0x8001

> > > >  

> > > >  DECLARE_GLOBAL_DATA_PTR;

> > > >  

> > > > @@ -118,7 +128,7 @@ static int

> > > > wait_for_nconfig_pin_and_nstatus_pin(void)

> > > >  	return wait_for_bit(__func__,

> > > >  			    &fpga_manager_base->imgcfg_stat,

> > > >  			    mask,

> > > > -			    false, FPGA_TIMEOUT_MSEC, false);

> > > > +			    true, FPGA_TIMEOUT_MSEC, false);

> > > >  }

> > > >  

> > > >  static int wait_for_f2s_nstatus_pin(unsigned long value)

> > > > @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)

> > > >  	return 0;

> > > >  }

> > > >  

> > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > +const char *get_cff_filename(const void *fdt, int *len, u32

> > > > core)

> > > > +{

> > > > +	const char *cff_filename = NULL;

> > > > +	const char *cell;

> > > > +	int nodeoffset;

> > > > +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");

> > > > +

> > > > +	if (nodeoffset >= 0) {

> > > > +		if (core)

> > > > +			cell = fdt_getprop(fdt,

> > > > +					nodeoffset,

> > > > +					"cffcore-file",

> > > > +					len);

> > > > +		else

> > > > +			cell = fdt_getprop(fdt, nodeoffset,

> > > > "cff-

> > > > file", len);

> > > This should be a property of the FPGA , not the system . You can

> > > have

> > > multiple FPGAs and then this would become a problem.

> > > 

> > This setting is for the only one FPGA inside our SoCFPGA.

> You just said it yourself, it is for the only FPGA in your SOCFPGA ,

> thus it is a property of the FPGA , not a chosen .

> 

Okay, what i trying to tell is that there is no multiple FPGAs in our
SOCFPGA. The filename is not any hardware properties, it is just a info
to tell SPL and U-boot which file to look for programming FPGA.
According to chosen node document, chosen node doesn't represent a real
HW, but serves as place for passing data. This is why our BSP tool put
the filename info here, the file is named by user in our tool, and this
info would be consumed by SPL to program FPGA.
What do you think?
> > 

> > For external

> > multiple FPGAs programming, user is adviced to store the FPGA

> > filename

> > in environment variable and programming FPGA with fpga loadfs

> > command.

> > 

> > Please note that, peripheral rbf and partition are required in SPL

> > to

> > set up DDR before booting to U-boot.

> > 

> > > 

> > > > 

> > > > 

> > > > +

> > > > +		if (cell)

> > > > +			cff_filename = cell;

> > > > +	}

> > > > +

> > > > +	return cff_filename;

> > > > +}

> > > > +

> > > > +const char *get_cff_devpart(const void *fdt, int *len)

> > > > +{

> > > > +	const char *cff_devpart = NULL;

> > > > +	const char *cell;

> > > > +	int nodeoffset;

> > > > +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");

> > > > +

> > > > +		cell = fdt_getprop(fdt, nodeoffset,

> > > > "cff_devpart",

> > > > len);

> > > Indent ? What is this new undocumented DT node about ?

> > > 

> > You can look the dtbinding doc on patch 12.

> Ugh, the patch should be in the series before this one.

> 

Okay, noted.
> > 

> > > 

> > > > 

> > > > 

> > > > +		if (cell)

> > > > +			cff_devpart = cell;

> > > > +

> > > > +	return cff_devpart;

> > > > +}

> > > > +

> > > > +void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer)

> > > > +{

> > > > +	/*

> > > > +	  Magic ID starting at:

> > > > +	   -> 1st dword in periph.rbf

> > > > +	   -> 2nd dword in core.rbf

> > > > +	*/

> > > Checkpatch should complain about incorrect multiline comment

> > > style

> > > here ...

> > > 

> > Okay. I will fix that.

> Can you please run checkpatch before submitting your patches ?

> 

Okay, sure.
> > 

> > > 

> > > > 

> > > > 

> > > > +	u32 word_reading_max = 2;

> > > > +	u32 i;

> > > > +

> > > > +	for(i = 0; i < word_reading_max; i++)

> > > > +	{

> > > > +		if(RBF_UNENCRYPTED == *(buffer + i)) /* PERIPH

> > > > RBF

> > > > */

> > > > +			rbf->security = unencrypted;

> > > > +		else if (RBF_ENCRYPTED == *(buffer + i))

> > > > +			rbf->security = encrypted;

> > > > +		else if (RBF_UNENCRYPTED == *(buffer + i + 1))

> > > > /*

> > > > CORE RBF */

> > > > +					rbf->security =

> > > > unencrypted;

> > > > +		else if (RBF_ENCRYPTED == *(buffer + i + 1))

> > > > +					rbf->security =

> > > > encrypted;

> > > > +		else {

> > > > +			rbf->security = invalid;

> > > > +			continue;

> > > > +		}

> > > > +

> > > > +		/* PERIPH RBF */

> > > > +		if (ARRIA10RBF_PERIPH == *(buffer + i + 1)) {

> > > > +			rbf->section = periph_section;

> > > > +			break;

> > > > +		}

> > > > +		else if (ARRIA10RBF_CORE == *(buffer + i + 1))

> > > > {

> > > > +			rbf->section = core_section;

> > > > +			break;

> > > > +		} /* CORE RBF */

> > > > +		else if (ARRIA10RBF_PERIPH == *(buffer + i +

> > > > 2)) {

> > > > +			rbf->section = periph_section;

> > > > +			break;

> > > > +		}

> > > > +		else if (ARRIA10RBF_CORE == *(buffer + i + 2))

> > > > {

> > > > +			rbf->section = core_section;

> > > > +			break;

> > > > +		}

> > > > +		else {

> > > } else { ... coding style ...

> > > 

> > Okay, i will fix that. This apply to else if?

> > > 

> > > > 

> > > > 

> > > > +			rbf->section = unknown;

> > > > +			break;

> > > > +		}

> > > > +	}

> > > > +

> > > > +	return;

> > > > +}

> > > > +

> > > > +static int flash_read(struct flash_info *flashinfo,

> > > > +	u32 size_read,

> > > > +	u32 *buffer_ptr)

> > > > +{

> > > > +	size_t ret = EEXIST;

> > > > +	loff_t actread = 0;

> > > > +

> > > > +#ifdef CONFIG_FS_FAT

> > > > +		ret = fat_read_file(flashinfo->filename,

> > > > +				buffer_ptr, flashinfo-

> > > > > 

> > > > > flash_offset,

> > > > +				 size_read, &actread);

> > > > +#endif

> > > How can a generic FPGA driver depend on random FS functionality ?

> > > This is broken ...

> > > 

> > random FS? There would having FAT FS for SDMMC, and UBI FS for QSPI

> > and

> > NAND(implement later).

> Driver should not depend on specific FS.

> 

I afraid to use fs_read, need to enable more FS features, which would
take a lot memory, could be run out of it in SPL. Do you think it is
good to try in SPL?
> > 

> > May be i can replace #ifdef CONFIG_FS_FAT witht

> > the codes below, what do you think?

> > 	bootdev.boot_device = spl_boot_device();

> > 

> > 	if(BOOT_DEVICE_MMC1 == bootdev.boot_device)

> > 	{ ... }

> Nor should it depend on random boot device.

> 

> > 

> > > 

> > > > 

> > > > 

> > > > +		if (ret || actread != size_read) {

> > > > +			printf("Failed to read %s from flash

> > > > %d ",

> > > > +				flashinfo->filename,

> > > > +				 ret);

> > > > +			printf("!= %d.\n", size_read);

> > > > +			return -EPERM;

> > > > +		} else

> > > > +			ret = actread;

> > > > +

> > > > +	return ret;

> > > > +}

> > > > +

> > > > +static int fs_flash_preinit(struct flash_info *flashinfo,

> > > > +	u32 *buffer, u32 *buffer_sizebytes)

> > > Is this an FPGA driver or MTD driver ?

> > > 

> > This is FPGA driver. Reading header of rbf, getting filesize and

> > adjusting the beginning offset of RBF. 

> Why do you pass around the struct flashinfo then ?

> 

This structure contains all infos about the rbf required by FPGA driver
to read from flash and programming FPGA.
> [...]

> 

> > 

> > > 

> > > > 

> > > > diff --git a/include/configs/socfpga_common.h

> > > > b/include/configs/socfpga_common.h

> > > > index 9be9e79..c15d244 100644

> > > > --- a/include/configs/socfpga_common.h

> > > > +++ b/include/configs/socfpga_common.h

> > > > @@ -27,7 +27,11 @@

> > > >   */

> > > >  #define CONFIG_NR_DRAM_BANKS		1

> > > >  #define PHYS_SDRAM_1			0x0

> > > > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)

> > > >  #define CONFIG_SYS_MALLOC_LEN		(64 * 1024 *

> > > > 1024)

> > > > +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)

> > > > +#define CONFIG_SYS_MALLOC_LEN		(128 * 1024 *

> > > > 1024)

> > > > +#endif

> > > 128 MiB malloc area is nonsense, even those 64 MiB are iffy. Why

> > > would

> > > you ever need that in a bootloader ?

> > > 

> > This is min require to malloc the buffer in SDRAM for core rbf.

> > Less

> > than this value, something would going wrong, i'm not recall what

> > issue

> > because i already tested this quite long time ago, problem related

> > to

> > FAT and malloc.

> Isn't the loadfs supposed to read the data from the FS and program

> them

> into the FPGA without having to read the whole bitstream into the RAM

> ?

> 

The operation is either reading data from FS into memory and program
into FPGA chunk by chunk if memory is smaller than data.
or
Reading whole data from FS into memory and program whole into FPGA if
memory is larger than data.
> > 

> > > 

> > > > 

> > > > 

> > > >  #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1

> > > >  #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1_SIZ

> > > > E

> > > >  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)

> > > > 

>
Marek Vasut Sept. 4, 2017, 9:39 a.m. UTC | #5
On 09/04/2017 09:08 AM, Chee, Tien Fong wrote:
> On Rab, 2017-08-30 at 10:52 +0200, Marek Vasut wrote:
>> On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:
>>>
>>> On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:
>>>>
>>>> On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:
>>>>>
>>>>>
>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>
>>>>> This driver handles FPGA program operation from flash loading
>>>>> RBF to memory and then to program FPGA.
>>>>>
>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>> ---
>>>>>  .../include/mach/fpga_manager_arria10.h            |   27 ++
>>>>>  drivers/fpga/socfpga_arria10.c                     |  386
>>>>> +++++++++++++++++++-
>>>>>  include/altera.h                                   |    6 +
>>>>>  include/configs/socfpga_common.h                   |    4 +
>>>>>  4 files changed, 422 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-
>>>>> socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-
>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>> index 9cbf696..93a9122 100644
>>>>> --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
>>>>> +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
>>>>> @@ -8,6 +8,8 @@
>>>>>  #ifndef _FPGA_MANAGER_ARRIA10_H_
>>>>>  #define _FPGA_MANAGER_ARRIA10_H_
>>>>>  
>>>>> +#include <asm/cache.h>
>>>>> +
>>>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK		
>>>>> BIT(0)
>>>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK	
>>>>> BIT(1)
>>>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 		
>>>>> BIT(2)
>>>>> @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {
>>>>>  	u32  imgcfg_fifo_status;
>>>>>  };
>>>>>  
>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>> +enum rbf_type {unknown, periph_section, core_section};
>>>>> +enum rbf_security {invalid, unencrypted, encrypted};
>>>>> +
>>>>> +struct rbf_info {
>>>>> +	enum rbf_type section;
>>>>> +	enum rbf_security security;
>>>>> +};
>>>>> +
>>>>> +struct flash_info {
>>>>> +	char *interface;
>>>>> +	char *dev_part;
>>>>> +	char *filename;
>>>>> +	int fstype;
>>>>> +	u32 remaining;
>>>>> +	u32 flash_offset;
>>>>> +	struct rbf_info rbfinfo;
>>>>> +	struct image_header header;
>>>>> +};
>>>>> +#endif
>>>>> +
>>>>>  /* Functions */
>>>>>  int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);
>>>>>  int fpgamgr_program_finish(void);
>>>>>  int is_fpgamgr_user_mode(void);
>>>>>  int fpgamgr_wait_early_user_mode(void);
>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>> +const char *get_cff_filename(const void *fdt, int *len, u32
>>>>> core);
>>>>> +const char *get_cff_devpart(const void *fdt, int *len);
>>>>> +#endif
>>>>>  
>>>>>  #endif /* __ASSEMBLY__ */
>>>>>  
>>>>> diff --git a/drivers/fpga/socfpga_arria10.c
>>>>> b/drivers/fpga/socfpga_arria10.c
>>>>> index 5c1a68a..90c55e5 100644
>>>>> --- a/drivers/fpga/socfpga_arria10.c
>>>>> +++ b/drivers/fpga/socfpga_arria10.c
>>>>> @@ -13,6 +13,12 @@
>>>>>  #include <altera.h>
>>>>>  #include <common.h>
>>>>>  #include <errno.h>
>>>>> +#include <fat.h>
>>>>> +#include <fs.h>
>>>>> +#include <fdtdec.h>
>>>>> +#include <malloc.h>
>>>>> +#include <part.h>
>>>>> +#include <spl.h>
>>>>>  #include <wait_bit.h>
>>>>>  #include <watchdog.h>
>>>>>  
>>>>> @@ -22,6 +28,10 @@
>>>>>  #define COMPRESSION_OFFSET	229
>>>>>  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in ms */
>>>>>  #define FPGA_TIMEOUT_CNT	0x1000000
>>>>> +#define RBF_UNENCRYPTED		0xa65c
>>>>> +#define RBF_ENCRYPTED		0xa65d
>>>>> +#define ARRIA10RBF_PERIPH	0x0001
>>>>> +#define ARRIA10RBF_CORE		0x8001
>>>>>  
>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>  
>>>>> @@ -118,7 +128,7 @@ static int
>>>>> wait_for_nconfig_pin_and_nstatus_pin(void)
>>>>>  	return wait_for_bit(__func__,
>>>>>  			    &fpga_manager_base->imgcfg_stat,
>>>>>  			    mask,
>>>>> -			    false, FPGA_TIMEOUT_MSEC, false);
>>>>> +			    true, FPGA_TIMEOUT_MSEC, false);
>>>>>  }
>>>>>  
>>>>>  static int wait_for_f2s_nstatus_pin(unsigned long value)
>>>>> @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>> +const char *get_cff_filename(const void *fdt, int *len, u32
>>>>> core)
>>>>> +{
>>>>> +	const char *cff_filename = NULL;
>>>>> +	const char *cell;
>>>>> +	int nodeoffset;
>>>>> +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
>>>>> +
>>>>> +	if (nodeoffset >= 0) {
>>>>> +		if (core)
>>>>> +			cell = fdt_getprop(fdt,
>>>>> +					nodeoffset,
>>>>> +					"cffcore-file",
>>>>> +					len);
>>>>> +		else
>>>>> +			cell = fdt_getprop(fdt, nodeoffset,
>>>>> "cff-
>>>>> file", len);
>>>> This should be a property of the FPGA , not the system . You can
>>>> have
>>>> multiple FPGAs and then this would become a problem.
>>>>
>>> This setting is for the only one FPGA inside our SoCFPGA.
>> You just said it yourself, it is for the only FPGA in your SOCFPGA ,
>> thus it is a property of the FPGA , not a chosen .
>>
> Okay, what i trying to tell is that there is no multiple FPGAs in our
> SOCFPGA. The filename is not any hardware properties, it is just a info
> to tell SPL and U-boot which file to look for programming FPGA.

What would happen if you attached an FPGA over ie. SPI or PCIe ?
Then you have two FPGAs in the system and you need to describe them in
the DT and your "chosen" approach breaks down.

> According to chosen node document, chosen node doesn't represent a real
> HW, but serves as place for passing data. This is why our BSP tool put
> the filename info here, the file is named by user in our tool, and this
> info would be consumed by SPL to program FPGA.
> What do you think?

Your BSP tool is broken.

[...]

>>>> How can a generic FPGA driver depend on random FS functionality ?
>>>> This is broken ...
>>>>
>>> random FS? There would having FAT FS for SDMMC, and UBI FS for QSPI
>>> and
>>> NAND(implement later).
>> Driver should not depend on specific FS.
>>
> I afraid to use fs_read, need to enable more FS features, which would
> take a lot memory, could be run out of it in SPL. Do you think it is
> good to try in SPL?

Don't you have like 256k for the SPL on Gen10 ?

[...]
Chee, Tien Fong Sept. 5, 2017, 5:53 a.m. UTC | #6
On Isn, 2017-09-04 at 11:39 +0200, Marek Vasut wrote:
> On 09/04/2017 09:08 AM, Chee, Tien Fong wrote:

> > 

> > On Rab, 2017-08-30 at 10:52 +0200, Marek Vasut wrote:

> > > 

> > > On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:

> > > > 

> > > > 

> > > > On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:

> > > > > 

> > > > > 

> > > > > On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:

> > > > > > 

> > > > > > 

> > > > > > 

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

> > > > > > 

> > > > > > This driver handles FPGA program operation from flash

> > > > > > loading

> > > > > > RBF to memory and then to program FPGA.

> > > > > > 

> > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

> > > > > > ---

> > > > > >  .../include/mach/fpga_manager_arria10.h            |   27

> > > > > > ++

> > > > > >  drivers/fpga/socfpga_arria10.c                     |  386

> > > > > > +++++++++++++++++++-

> > > > > >  include/altera.h                                   |    6

> > > > > > +

> > > > > >  include/configs/socfpga_common.h                   |    4

> > > > > > +

> > > > > >  4 files changed, 422 insertions(+), 1 deletions(-)

> > > > > > 

> > > > > > diff --git a/arch/arm/mach-

> > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > b/arch/arm/mach-

> > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > index 9cbf696..93a9122 100644

> > > > > > --- a/arch/arm/mach-

> > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > +++ b/arch/arm/mach-

> > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > @@ -8,6 +8,8 @@

> > > > > >  #ifndef _FPGA_MANAGER_ARRIA10_H_

> > > > > >  #define _FPGA_MANAGER_ARRIA10_H_

> > > > > >  

> > > > > > +#include <asm/cache.h>

> > > > > > +

> > > > > >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK	

> > > > > > 	

> > > > > > BIT(0)

> > > > > >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK

> > > > > > 	

> > > > > > BIT(1)

> > > > > >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 	

> > > > > > 	

> > > > > > BIT(2)

> > > > > > @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {

> > > > > >  	u32  imgcfg_fifo_status;

> > > > > >  };

> > > > > >  

> > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > +enum rbf_type {unknown, periph_section, core_section};

> > > > > > +enum rbf_security {invalid, unencrypted, encrypted};

> > > > > > +

> > > > > > +struct rbf_info {

> > > > > > +	enum rbf_type section;

> > > > > > +	enum rbf_security security;

> > > > > > +};

> > > > > > +

> > > > > > +struct flash_info {

> > > > > > +	char *interface;

> > > > > > +	char *dev_part;

> > > > > > +	char *filename;

> > > > > > +	int fstype;

> > > > > > +	u32 remaining;

> > > > > > +	u32 flash_offset;

> > > > > > +	struct rbf_info rbfinfo;

> > > > > > +	struct image_header header;

> > > > > > +};

> > > > > > +#endif

> > > > > > +

> > > > > >  /* Functions */

> > > > > >  int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);

> > > > > >  int fpgamgr_program_finish(void);

> > > > > >  int is_fpgamgr_user_mode(void);

> > > > > >  int fpgamgr_wait_early_user_mode(void);

> > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > +const char *get_cff_filename(const void *fdt, int *len,

> > > > > > u32

> > > > > > core);

> > > > > > +const char *get_cff_devpart(const void *fdt, int *len);

> > > > > > +#endif

> > > > > >  

> > > > > >  #endif /* __ASSEMBLY__ */

> > > > > >  

> > > > > > diff --git a/drivers/fpga/socfpga_arria10.c

> > > > > > b/drivers/fpga/socfpga_arria10.c

> > > > > > index 5c1a68a..90c55e5 100644

> > > > > > --- a/drivers/fpga/socfpga_arria10.c

> > > > > > +++ b/drivers/fpga/socfpga_arria10.c

> > > > > > @@ -13,6 +13,12 @@

> > > > > >  #include <altera.h>

> > > > > >  #include <common.h>

> > > > > >  #include <errno.h>

> > > > > > +#include <fat.h>

> > > > > > +#include <fs.h>

> > > > > > +#include <fdtdec.h>

> > > > > > +#include <malloc.h>

> > > > > > +#include <part.h>

> > > > > > +#include <spl.h>

> > > > > >  #include <wait_bit.h>

> > > > > >  #include <watchdog.h>

> > > > > >  

> > > > > > @@ -22,6 +28,10 @@

> > > > > >  #define COMPRESSION_OFFSET	229

> > > > > >  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in ms */

> > > > > >  #define FPGA_TIMEOUT_CNT	0x1000000

> > > > > > +#define RBF_UNENCRYPTED		0xa65c

> > > > > > +#define RBF_ENCRYPTED		0xa65d

> > > > > > +#define ARRIA10RBF_PERIPH	0x0001

> > > > > > +#define ARRIA10RBF_CORE		0x8001

> > > > > >  

> > > > > >  DECLARE_GLOBAL_DATA_PTR;

> > > > > >  

> > > > > > @@ -118,7 +128,7 @@ static int

> > > > > > wait_for_nconfig_pin_and_nstatus_pin(void)

> > > > > >  	return wait_for_bit(__func__,

> > > > > >  			    &fpga_manager_base-

> > > > > > >imgcfg_stat,

> > > > > >  			    mask,

> > > > > > -			    false, FPGA_TIMEOUT_MSEC,

> > > > > > false);

> > > > > > +			    true, FPGA_TIMEOUT_MSEC,

> > > > > > false);

> > > > > >  }

> > > > > >  

> > > > > >  static int wait_for_f2s_nstatus_pin(unsigned long value)

> > > > > > @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)

> > > > > >  	return 0;

> > > > > >  }

> > > > > >  

> > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > +const char *get_cff_filename(const void *fdt, int *len,

> > > > > > u32

> > > > > > core)

> > > > > > +{

> > > > > > +	const char *cff_filename = NULL;

> > > > > > +	const char *cell;

> > > > > > +	int nodeoffset;

> > > > > > +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");

> > > > > > +

> > > > > > +	if (nodeoffset >= 0) {

> > > > > > +		if (core)

> > > > > > +			cell = fdt_getprop(fdt,

> > > > > > +					nodeoffset,

> > > > > > +					"cffcore-file",

> > > > > > +					len);

> > > > > > +		else

> > > > > > +			cell = fdt_getprop(fdt,

> > > > > > nodeoffset,

> > > > > > "cff-

> > > > > > file", len);

> > > > > This should be a property of the FPGA , not the system . You

> > > > > can

> > > > > have

> > > > > multiple FPGAs and then this would become a problem.

> > > > > 

> > > > This setting is for the only one FPGA inside our SoCFPGA.

> > > You just said it yourself, it is for the only FPGA in your

> > > SOCFPGA ,

> > > thus it is a property of the FPGA , not a chosen .

> > > 

> > Okay, what i trying to tell is that there is no multiple FPGAs in

> > our

> > SOCFPGA. The filename is not any hardware properties, it is just a

> > info

> > to tell SPL and U-boot which file to look for programming FPGA.

> What would happen if you attached an FPGA over ie. SPI or PCIe ?

> Then you have two FPGAs in the system and you need to describe them

> in

> the DT and your "chosen" approach breaks down.

> 

We only need to decribe the internal FPGA of SoCFPGA in DT such as
"chosen" to specify which file should SPL looking for getting SDRAM up
with programming correct periph rbf into FPGA. When the SDRAM is up
running, then only SPL can load U-boot and booting from there.
The rest of external multiple FPGAs can be configured through U-boot,
such as boot script, interactive in console or from PC host. Other than
that, external FPGA can also be configured through Linux.
> > 

> > According to chosen node document, chosen node doesn't represent a

> > real

> > HW, but serves as place for passing data. This is why our BSP tool

> > put

> > the filename info here, the file is named by user in our tool, and

> > this

> > info would be consumed by SPL to program FPGA.

> > What do you think?

> Your BSP tool is broken.

> 

The BSP tool is used to describe internal FPGA in SOCFPGA. Other
external FPGAs other than SOCFPGA itself, it can be programmed through
U-boot.
> [...]

> 

> > 

> > > 

> > > > 

> > > > > 

> > > > > How can a generic FPGA driver depend on random FS

> > > > > functionality ?

> > > > > This is broken ...

> > > > > 

> > > > random FS? There would having FAT FS for SDMMC, and UBI FS for

> > > > QSPI

> > > > and

> > > > NAND(implement later).

> > > Driver should not depend on specific FS.

> > > 

> > I afraid to use fs_read, need to enable more FS features, which

> > would

> > take a lot memory, could be run out of it in SPL. Do you think it

> > is

> > good to try in SPL?

> Don't you have like 256k for the SPL on Gen10 ?

> 

Yes, i afraid it wouldn't be enough when we enable all NAND and QSPI
flashes feature. Our stretch goal is to have all flashes with one
defconfig as we did in gen5.
> [...]

>
Marek Vasut Sept. 5, 2017, 9:04 a.m. UTC | #7
On 09/05/2017 07:53 AM, Chee, Tien Fong wrote:
> On Isn, 2017-09-04 at 11:39 +0200, Marek Vasut wrote:
>> On 09/04/2017 09:08 AM, Chee, Tien Fong wrote:
>>>
>>> On Rab, 2017-08-30 at 10:52 +0200, Marek Vasut wrote:
>>>>
>>>> On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:
>>>>>
>>>>>
>>>>> On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>>
>>>>>>> This driver handles FPGA program operation from flash
>>>>>>> loading
>>>>>>> RBF to memory and then to program FPGA.
>>>>>>>
>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>> ---
>>>>>>>  .../include/mach/fpga_manager_arria10.h            |   27
>>>>>>> ++
>>>>>>>  drivers/fpga/socfpga_arria10.c                     |  386
>>>>>>> +++++++++++++++++++-
>>>>>>>  include/altera.h                                   |    6
>>>>>>> +
>>>>>>>  include/configs/socfpga_common.h                   |    4
>>>>>>> +
>>>>>>>  4 files changed, 422 insertions(+), 1 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm/mach-
>>>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>>>> b/arch/arm/mach-
>>>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>>>> index 9cbf696..93a9122 100644
>>>>>>> --- a/arch/arm/mach-
>>>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>>>> +++ b/arch/arm/mach-
>>>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>>>> @@ -8,6 +8,8 @@
>>>>>>>  #ifndef _FPGA_MANAGER_ARRIA10_H_
>>>>>>>  #define _FPGA_MANAGER_ARRIA10_H_
>>>>>>>  
>>>>>>> +#include <asm/cache.h>
>>>>>>> +
>>>>>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK	
>>>>>>> 	
>>>>>>> BIT(0)
>>>>>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK
>>>>>>> 	
>>>>>>> BIT(1)
>>>>>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 	
>>>>>>> 	
>>>>>>> BIT(2)
>>>>>>> @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {
>>>>>>>  	u32  imgcfg_fifo_status;
>>>>>>>  };
>>>>>>>  
>>>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>>>> +enum rbf_type {unknown, periph_section, core_section};
>>>>>>> +enum rbf_security {invalid, unencrypted, encrypted};
>>>>>>> +
>>>>>>> +struct rbf_info {
>>>>>>> +	enum rbf_type section;
>>>>>>> +	enum rbf_security security;
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct flash_info {
>>>>>>> +	char *interface;
>>>>>>> +	char *dev_part;
>>>>>>> +	char *filename;
>>>>>>> +	int fstype;
>>>>>>> +	u32 remaining;
>>>>>>> +	u32 flash_offset;
>>>>>>> +	struct rbf_info rbfinfo;
>>>>>>> +	struct image_header header;
>>>>>>> +};
>>>>>>> +#endif
>>>>>>> +
>>>>>>>  /* Functions */
>>>>>>>  int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);
>>>>>>>  int fpgamgr_program_finish(void);
>>>>>>>  int is_fpgamgr_user_mode(void);
>>>>>>>  int fpgamgr_wait_early_user_mode(void);
>>>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>>>> +const char *get_cff_filename(const void *fdt, int *len,
>>>>>>> u32
>>>>>>> core);
>>>>>>> +const char *get_cff_devpart(const void *fdt, int *len);
>>>>>>> +#endif
>>>>>>>  
>>>>>>>  #endif /* __ASSEMBLY__ */
>>>>>>>  
>>>>>>> diff --git a/drivers/fpga/socfpga_arria10.c
>>>>>>> b/drivers/fpga/socfpga_arria10.c
>>>>>>> index 5c1a68a..90c55e5 100644
>>>>>>> --- a/drivers/fpga/socfpga_arria10.c
>>>>>>> +++ b/drivers/fpga/socfpga_arria10.c
>>>>>>> @@ -13,6 +13,12 @@
>>>>>>>  #include <altera.h>
>>>>>>>  #include <common.h>
>>>>>>>  #include <errno.h>
>>>>>>> +#include <fat.h>
>>>>>>> +#include <fs.h>
>>>>>>> +#include <fdtdec.h>
>>>>>>> +#include <malloc.h>
>>>>>>> +#include <part.h>
>>>>>>> +#include <spl.h>
>>>>>>>  #include <wait_bit.h>
>>>>>>>  #include <watchdog.h>
>>>>>>>  
>>>>>>> @@ -22,6 +28,10 @@
>>>>>>>  #define COMPRESSION_OFFSET	229
>>>>>>>  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in ms */
>>>>>>>  #define FPGA_TIMEOUT_CNT	0x1000000
>>>>>>> +#define RBF_UNENCRYPTED		0xa65c
>>>>>>> +#define RBF_ENCRYPTED		0xa65d
>>>>>>> +#define ARRIA10RBF_PERIPH	0x0001
>>>>>>> +#define ARRIA10RBF_CORE		0x8001
>>>>>>>  
>>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>>  
>>>>>>> @@ -118,7 +128,7 @@ static int
>>>>>>> wait_for_nconfig_pin_and_nstatus_pin(void)
>>>>>>>  	return wait_for_bit(__func__,
>>>>>>>  			    &fpga_manager_base-
>>>>>>>> imgcfg_stat,
>>>>>>>  			    mask,
>>>>>>> -			    false, FPGA_TIMEOUT_MSEC,
>>>>>>> false);
>>>>>>> +			    true, FPGA_TIMEOUT_MSEC,
>>>>>>> false);
>>>>>>>  }
>>>>>>>  
>>>>>>>  static int wait_for_f2s_nstatus_pin(unsigned long value)
>>>>>>> @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>>>> +const char *get_cff_filename(const void *fdt, int *len,
>>>>>>> u32
>>>>>>> core)
>>>>>>> +{
>>>>>>> +	const char *cff_filename = NULL;
>>>>>>> +	const char *cell;
>>>>>>> +	int nodeoffset;
>>>>>>> +	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
>>>>>>> +
>>>>>>> +	if (nodeoffset >= 0) {
>>>>>>> +		if (core)
>>>>>>> +			cell = fdt_getprop(fdt,
>>>>>>> +					nodeoffset,
>>>>>>> +					"cffcore-file",
>>>>>>> +					len);
>>>>>>> +		else
>>>>>>> +			cell = fdt_getprop(fdt,
>>>>>>> nodeoffset,
>>>>>>> "cff-
>>>>>>> file", len);
>>>>>> This should be a property of the FPGA , not the system . You
>>>>>> can
>>>>>> have
>>>>>> multiple FPGAs and then this would become a problem.
>>>>>>
>>>>> This setting is for the only one FPGA inside our SoCFPGA.
>>>> You just said it yourself, it is for the only FPGA in your
>>>> SOCFPGA ,
>>>> thus it is a property of the FPGA , not a chosen .
>>>>
>>> Okay, what i trying to tell is that there is no multiple FPGAs in
>>> our
>>> SOCFPGA. The filename is not any hardware properties, it is just a
>>> info
>>> to tell SPL and U-boot which file to look for programming FPGA.
>> What would happen if you attached an FPGA over ie. SPI or PCIe ?
>> Then you have two FPGAs in the system and you need to describe them
>> in
>> the DT and your "chosen" approach breaks down.
>>
> We only need to decribe the internal FPGA of SoCFPGA in DT

This is incorrect. You describe the hardware in DT, if you have multiple
FPGAs, then your approach breaks down.

> such as
> "chosen" to specify which file should SPL looking for getting SDRAM up
> with programming correct periph rbf into FPGA. When the SDRAM is up
> running, then only SPL can load U-boot and booting from there.
> The rest of external multiple FPGAs can be configured through U-boot,
> such as boot script, interactive in console or from PC host. Other than
> that, external FPGA can also be configured through Linux.
>>>
>>> According to chosen node document, chosen node doesn't represent a
>>> real
>>> HW, but serves as place for passing data. This is why our BSP tool
>>> put
>>> the filename info here, the file is named by user in our tool, and
>>> this
>>> info would be consumed by SPL to program FPGA.
>>> What do you think?
>> Your BSP tool is broken.
>>
> The BSP tool is used to describe internal FPGA in SOCFPGA. Other
> external FPGAs other than SOCFPGA itself, it can be programmed through
> U-boot.

The BSP tool is broken if it generates broken DT, do I have to repeat
myself ?

[...]
Chee, Tien Fong Sept. 5, 2017, 9:23 a.m. UTC | #8
On Sel, 2017-09-05 at 11:04 +0200, Marek Vasut wrote:
> On 09/05/2017 07:53 AM, Chee, Tien Fong wrote:

> > 

> > On Isn, 2017-09-04 at 11:39 +0200, Marek Vasut wrote:

> > > 

> > > On 09/04/2017 09:08 AM, Chee, Tien Fong wrote:

> > > > 

> > > > 

> > > > On Rab, 2017-08-30 at 10:52 +0200, Marek Vasut wrote:

> > > > > 

> > > > > 

> > > > > On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

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

> > > > > > > > 

> > > > > > > > This driver handles FPGA program operation from flash

> > > > > > > > loading

> > > > > > > > RBF to memory and then to program FPGA.

> > > > > > > > 

> > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com

> > > > > > > > >

> > > > > > > > ---

> > > > > > > >  .../include/mach/fpga_manager_arria10.h            |  

> > > > > > > >  27

> > > > > > > > ++

> > > > > > > >  drivers/fpga/socfpga_arria10.c                     |  

> > > > > > > > 386

> > > > > > > > +++++++++++++++++++-

> > > > > > > >  include/altera.h                                   |  

> > > > > > > >   6

> > > > > > > > +

> > > > > > > >  include/configs/socfpga_common.h                   |  

> > > > > > > >   4

> > > > > > > > +

> > > > > > > >  4 files changed, 422 insertions(+), 1 deletions(-)

> > > > > > > > 

> > > > > > > > diff --git a/arch/arm/mach-

> > > > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > > > b/arch/arm/mach-

> > > > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > > > index 9cbf696..93a9122 100644

> > > > > > > > --- a/arch/arm/mach-

> > > > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > > > +++ b/arch/arm/mach-

> > > > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > > > @@ -8,6 +8,8 @@

> > > > > > > >  #ifndef _FPGA_MANAGER_ARRIA10_H_

> > > > > > > >  #define _FPGA_MANAGER_ARRIA10_H_

> > > > > > > >  

> > > > > > > > +#include <asm/cache.h>

> > > > > > > > +

> > > > > > > >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK	

> > > > > > > > 	

> > > > > > > > BIT(0)

> > > > > > > >  #define

> > > > > > > > ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK

> > > > > > > > 	

> > > > > > > > BIT(1)

> > > > > > > >  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 	

> > > > > > > > 	

> > > > > > > > BIT(2)

> > > > > > > > @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {

> > > > > > > >  	u32  imgcfg_fifo_status;

> > > > > > > >  };

> > > > > > > >  

> > > > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > > > +enum rbf_type {unknown, periph_section, core_section};

> > > > > > > > +enum rbf_security {invalid, unencrypted, encrypted};

> > > > > > > > +

> > > > > > > > +struct rbf_info {

> > > > > > > > +	enum rbf_type section;

> > > > > > > > +	enum rbf_security security;

> > > > > > > > +};

> > > > > > > > +

> > > > > > > > +struct flash_info {

> > > > > > > > +	char *interface;

> > > > > > > > +	char *dev_part;

> > > > > > > > +	char *filename;

> > > > > > > > +	int fstype;

> > > > > > > > +	u32 remaining;

> > > > > > > > +	u32 flash_offset;

> > > > > > > > +	struct rbf_info rbfinfo;

> > > > > > > > +	struct image_header header;

> > > > > > > > +};

> > > > > > > > +#endif

> > > > > > > > +

> > > > > > > >  /* Functions */

> > > > > > > >  int fpgamgr_program_init(u32 * rbf_data, size_t

> > > > > > > > rbf_size);

> > > > > > > >  int fpgamgr_program_finish(void);

> > > > > > > >  int is_fpgamgr_user_mode(void);

> > > > > > > >  int fpgamgr_wait_early_user_mode(void);

> > > > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > > > +const char *get_cff_filename(const void *fdt, int

> > > > > > > > *len,

> > > > > > > > u32

> > > > > > > > core);

> > > > > > > > +const char *get_cff_devpart(const void *fdt, int

> > > > > > > > *len);

> > > > > > > > +#endif

> > > > > > > >  

> > > > > > > >  #endif /* __ASSEMBLY__ */

> > > > > > > >  

> > > > > > > > diff --git a/drivers/fpga/socfpga_arria10.c

> > > > > > > > b/drivers/fpga/socfpga_arria10.c

> > > > > > > > index 5c1a68a..90c55e5 100644

> > > > > > > > --- a/drivers/fpga/socfpga_arria10.c

> > > > > > > > +++ b/drivers/fpga/socfpga_arria10.c

> > > > > > > > @@ -13,6 +13,12 @@

> > > > > > > >  #include <altera.h>

> > > > > > > >  #include <common.h>

> > > > > > > >  #include <errno.h>

> > > > > > > > +#include <fat.h>

> > > > > > > > +#include <fs.h>

> > > > > > > > +#include <fdtdec.h>

> > > > > > > > +#include <malloc.h>

> > > > > > > > +#include <part.h>

> > > > > > > > +#include <spl.h>

> > > > > > > >  #include <wait_bit.h>

> > > > > > > >  #include <watchdog.h>

> > > > > > > >  

> > > > > > > > @@ -22,6 +28,10 @@

> > > > > > > >  #define COMPRESSION_OFFSET	229

> > > > > > > >  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in

> > > > > > > > ms */

> > > > > > > >  #define FPGA_TIMEOUT_CNT	0x1000000

> > > > > > > > +#define RBF_UNENCRYPTED		0xa65c

> > > > > > > > +#define RBF_ENCRYPTED		0xa65d

> > > > > > > > +#define ARRIA10RBF_PERIPH	0x0001

> > > > > > > > +#define ARRIA10RBF_CORE		0x8001

> > > > > > > >  

> > > > > > > >  DECLARE_GLOBAL_DATA_PTR;

> > > > > > > >  

> > > > > > > > @@ -118,7 +128,7 @@ static int

> > > > > > > > wait_for_nconfig_pin_and_nstatus_pin(void)

> > > > > > > >  	return wait_for_bit(__func__,

> > > > > > > >  			    &fpga_manager_base-

> > > > > > > > > 

> > > > > > > > > imgcfg_stat,

> > > > > > > >  			    mask,

> > > > > > > > -			    false, FPGA_TIMEOUT_MSEC,

> > > > > > > > false);

> > > > > > > > +			    true, FPGA_TIMEOUT_MSEC,

> > > > > > > > false);

> > > > > > > >  }

> > > > > > > >  

> > > > > > > >  static int wait_for_f2s_nstatus_pin(unsigned long

> > > > > > > > value)

> > > > > > > > @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)

> > > > > > > >  	return 0;

> > > > > > > >  }

> > > > > > > >  

> > > > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > > > +const char *get_cff_filename(const void *fdt, int

> > > > > > > > *len,

> > > > > > > > u32

> > > > > > > > core)

> > > > > > > > +{

> > > > > > > > +	const char *cff_filename = NULL;

> > > > > > > > +	const char *cell;

> > > > > > > > +	int nodeoffset;

> > > > > > > > +	nodeoffset = fdt_subnode_offset(fdt, 0,

> > > > > > > > "chosen");

> > > > > > > > +

> > > > > > > > +	if (nodeoffset >= 0) {

> > > > > > > > +		if (core)

> > > > > > > > +			cell = fdt_getprop(fdt,

> > > > > > > > +					nodeoffset,

> > > > > > > > +					"cffcore-

> > > > > > > > file",

> > > > > > > > +					len);

> > > > > > > > +		else

> > > > > > > > +			cell = fdt_getprop(fdt,

> > > > > > > > nodeoffset,

> > > > > > > > "cff-

> > > > > > > > file", len);

> > > > > > > This should be a property of the FPGA , not the system .

> > > > > > > You

> > > > > > > can

> > > > > > > have

> > > > > > > multiple FPGAs and then this would become a problem.

> > > > > > > 

> > > > > > This setting is for the only one FPGA inside our SoCFPGA.

> > > > > You just said it yourself, it is for the only FPGA in your

> > > > > SOCFPGA ,

> > > > > thus it is a property of the FPGA , not a chosen .

> > > > > 

> > > > Okay, what i trying to tell is that there is no multiple FPGAs

> > > > in

> > > > our

> > > > SOCFPGA. The filename is not any hardware properties, it is

> > > > just a

> > > > info

> > > > to tell SPL and U-boot which file to look for programming FPGA.

> > > What would happen if you attached an FPGA over ie. SPI or PCIe ?

> > > Then you have two FPGAs in the system and you need to describe

> > > them

> > > in

> > > the DT and your "chosen" approach breaks down.

> > > 

> > We only need to decribe the internal FPGA of SoCFPGA in DT

> This is incorrect. You describe the hardware in DT, if you have

> multiple

> FPGAs, then your approach breaks down.

> 

Let me clarify, we don't have FPGA HW properties described in DT, only
the RBF files are defined under chosen node. The files are defined only
apply for FPGA inside SOCFPGA.
Multiple external FPGA are configured through U-boot.
> > 

> > such as

> > "chosen" to specify which file should SPL looking for getting SDRAM

> > up

> > with programming correct periph rbf into FPGA. When the SDRAM is up

> > running, then only SPL can load U-boot and booting from there.

> > The rest of external multiple FPGAs can be configured through U-

> > boot,

> > such as boot script, interactive in console or from PC host. Other

> > than

> > that, external FPGA can also be configured through Linux.

> > > 

> > > > 

> > > > 

> > > > According to chosen node document, chosen node doesn't

> > > > represent a

> > > > real

> > > > HW, but serves as place for passing data. This is why our BSP

> > > > tool

> > > > put

> > > > the filename info here, the file is named by user in our tool,

> > > > and

> > > > this

> > > > info would be consumed by SPL to program FPGA.

> > > > What do you think?

> > > Your BSP tool is broken.

> > > 

> > The BSP tool is used to describe internal FPGA in SOCFPGA. Other

> > external FPGAs other than SOCFPGA itself, it can be programmed

> > through

> > U-boot.

> The BSP tool is broken if it generates broken DT, do I have to repeat

> myself ?

> 

BSP tool is only generate the RBF filename for FPGA inside SOCFPGA.
Multiple external FPGA are configured through U-boot.
> [...]

>
Marek Vasut Sept. 5, 2017, 9:36 a.m. UTC | #9
On 09/05/2017 11:23 AM, Chee, Tien Fong wrote:
> On Sel, 2017-09-05 at 11:04 +0200, Marek Vasut wrote:
>> On 09/05/2017 07:53 AM, Chee, Tien Fong wrote:
>>>
>>> On Isn, 2017-09-04 at 11:39 +0200, Marek Vasut wrote:
>>>>
>>>> On 09/04/2017 09:08 AM, Chee, Tien Fong wrote:
>>>>>
>>>>>
>>>>> On Rab, 2017-08-30 at 10:52 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 08/29/2017 12:45 PM, tien.fong.chee@intel.com wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>>>>
>>>>>>>>> This driver handles FPGA program operation from flash
>>>>>>>>> loading
>>>>>>>>> RBF to memory and then to program FPGA.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com
>>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>  .../include/mach/fpga_manager_arria10.h            |  
>>>>>>>>>  27
>>>>>>>>> ++
>>>>>>>>>  drivers/fpga/socfpga_arria10.c                     |  
>>>>>>>>> 386
>>>>>>>>> +++++++++++++++++++-
>>>>>>>>>  include/altera.h                                   |  
>>>>>>>>>   6
>>>>>>>>> +
>>>>>>>>>  include/configs/socfpga_common.h                   |  
>>>>>>>>>   4
>>>>>>>>> +
>>>>>>>>>  4 files changed, 422 insertions(+), 1 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/mach-
>>>>>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>>>>>> b/arch/arm/mach-
>>>>>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>>>>>> index 9cbf696..93a9122 100644
>>>>>>>>> --- a/arch/arm/mach-
>>>>>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>>>>>> +++ b/arch/arm/mach-
>>>>>>>>> socfpga/include/mach/fpga_manager_arria10.h
>>>>>>>>> @@ -8,6 +8,8 @@
>>>>>>>>>  #ifndef _FPGA_MANAGER_ARRIA10_H_
>>>>>>>>>  #define _FPGA_MANAGER_ARRIA10_H_
>>>>>>>>>  
>>>>>>>>> +#include <asm/cache.h>
>>>>>>>>> +
>>>>>>>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK	
>>>>>>>>> 	
>>>>>>>>> BIT(0)
>>>>>>>>>  #define
>>>>>>>>> ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK
>>>>>>>>> 	
>>>>>>>>> BIT(1)
>>>>>>>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 	
>>>>>>>>> 	
>>>>>>>>> BIT(2)
>>>>>>>>> @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {
>>>>>>>>>  	u32  imgcfg_fifo_status;
>>>>>>>>>  };
>>>>>>>>>  
>>>>>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>>>>>> +enum rbf_type {unknown, periph_section, core_section};
>>>>>>>>> +enum rbf_security {invalid, unencrypted, encrypted};
>>>>>>>>> +
>>>>>>>>> +struct rbf_info {
>>>>>>>>> +	enum rbf_type section;
>>>>>>>>> +	enum rbf_security security;
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +struct flash_info {
>>>>>>>>> +	char *interface;
>>>>>>>>> +	char *dev_part;
>>>>>>>>> +	char *filename;
>>>>>>>>> +	int fstype;
>>>>>>>>> +	u32 remaining;
>>>>>>>>> +	u32 flash_offset;
>>>>>>>>> +	struct rbf_info rbfinfo;
>>>>>>>>> +	struct image_header header;
>>>>>>>>> +};
>>>>>>>>> +#endif
>>>>>>>>> +
>>>>>>>>>  /* Functions */
>>>>>>>>>  int fpgamgr_program_init(u32 * rbf_data, size_t
>>>>>>>>> rbf_size);
>>>>>>>>>  int fpgamgr_program_finish(void);
>>>>>>>>>  int is_fpgamgr_user_mode(void);
>>>>>>>>>  int fpgamgr_wait_early_user_mode(void);
>>>>>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>>>>>> +const char *get_cff_filename(const void *fdt, int
>>>>>>>>> *len,
>>>>>>>>> u32
>>>>>>>>> core);
>>>>>>>>> +const char *get_cff_devpart(const void *fdt, int
>>>>>>>>> *len);
>>>>>>>>> +#endif
>>>>>>>>>  
>>>>>>>>>  #endif /* __ASSEMBLY__ */
>>>>>>>>>  
>>>>>>>>> diff --git a/drivers/fpga/socfpga_arria10.c
>>>>>>>>> b/drivers/fpga/socfpga_arria10.c
>>>>>>>>> index 5c1a68a..90c55e5 100644
>>>>>>>>> --- a/drivers/fpga/socfpga_arria10.c
>>>>>>>>> +++ b/drivers/fpga/socfpga_arria10.c
>>>>>>>>> @@ -13,6 +13,12 @@
>>>>>>>>>  #include <altera.h>
>>>>>>>>>  #include <common.h>
>>>>>>>>>  #include <errno.h>
>>>>>>>>> +#include <fat.h>
>>>>>>>>> +#include <fs.h>
>>>>>>>>> +#include <fdtdec.h>
>>>>>>>>> +#include <malloc.h>
>>>>>>>>> +#include <part.h>
>>>>>>>>> +#include <spl.h>
>>>>>>>>>  #include <wait_bit.h>
>>>>>>>>>  #include <watchdog.h>
>>>>>>>>>  
>>>>>>>>> @@ -22,6 +28,10 @@
>>>>>>>>>  #define COMPRESSION_OFFSET	229
>>>>>>>>>  #define FPGA_TIMEOUT_MSEC	1000  /* timeout in
>>>>>>>>> ms */
>>>>>>>>>  #define FPGA_TIMEOUT_CNT	0x1000000
>>>>>>>>> +#define RBF_UNENCRYPTED		0xa65c
>>>>>>>>> +#define RBF_ENCRYPTED		0xa65d
>>>>>>>>> +#define ARRIA10RBF_PERIPH	0x0001
>>>>>>>>> +#define ARRIA10RBF_CORE		0x8001
>>>>>>>>>  
>>>>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>>>>  
>>>>>>>>> @@ -118,7 +128,7 @@ static int
>>>>>>>>> wait_for_nconfig_pin_and_nstatus_pin(void)
>>>>>>>>>  	return wait_for_bit(__func__,
>>>>>>>>>  			    &fpga_manager_base-
>>>>>>>>>>
>>>>>>>>>> imgcfg_stat,
>>>>>>>>>  			    mask,
>>>>>>>>> -			    false, FPGA_TIMEOUT_MSEC,
>>>>>>>>> false);
>>>>>>>>> +			    true, FPGA_TIMEOUT_MSEC,
>>>>>>>>> false);
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>>  static int wait_for_f2s_nstatus_pin(unsigned long
>>>>>>>>> value)
>>>>>>>>> @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)
>>>>>>>>>  	return 0;
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>>>>>>>> +const char *get_cff_filename(const void *fdt, int
>>>>>>>>> *len,
>>>>>>>>> u32
>>>>>>>>> core)
>>>>>>>>> +{
>>>>>>>>> +	const char *cff_filename = NULL;
>>>>>>>>> +	const char *cell;
>>>>>>>>> +	int nodeoffset;
>>>>>>>>> +	nodeoffset = fdt_subnode_offset(fdt, 0,
>>>>>>>>> "chosen");
>>>>>>>>> +
>>>>>>>>> +	if (nodeoffset >= 0) {
>>>>>>>>> +		if (core)
>>>>>>>>> +			cell = fdt_getprop(fdt,
>>>>>>>>> +					nodeoffset,
>>>>>>>>> +					"cffcore-
>>>>>>>>> file",
>>>>>>>>> +					len);
>>>>>>>>> +		else
>>>>>>>>> +			cell = fdt_getprop(fdt,
>>>>>>>>> nodeoffset,
>>>>>>>>> "cff-
>>>>>>>>> file", len);
>>>>>>>> This should be a property of the FPGA , not the system .
>>>>>>>> You
>>>>>>>> can
>>>>>>>> have
>>>>>>>> multiple FPGAs and then this would become a problem.
>>>>>>>>
>>>>>>> This setting is for the only one FPGA inside our SoCFPGA.
>>>>>> You just said it yourself, it is for the only FPGA in your
>>>>>> SOCFPGA ,
>>>>>> thus it is a property of the FPGA , not a chosen .
>>>>>>
>>>>> Okay, what i trying to tell is that there is no multiple FPGAs
>>>>> in
>>>>> our
>>>>> SOCFPGA. The filename is not any hardware properties, it is
>>>>> just a
>>>>> info
>>>>> to tell SPL and U-boot which file to look for programming FPGA.
>>>> What would happen if you attached an FPGA over ie. SPI or PCIe ?
>>>> Then you have two FPGAs in the system and you need to describe
>>>> them
>>>> in
>>>> the DT and your "chosen" approach breaks down.
>>>>
>>> We only need to decribe the internal FPGA of SoCFPGA in DT
>> This is incorrect. You describe the hardware in DT, if you have
>> multiple
>> FPGAs, then your approach breaks down.
>>
> Let me clarify, we don't have FPGA HW properties described in DT, only
> the RBF files are defined under chosen node. The files are defined only
> apply for FPGA inside SOCFPGA.
> Multiple external FPGA are configured through U-boot.
>>>
>>> such as
>>> "chosen" to specify which file should SPL looking for getting SDRAM
>>> up
>>> with programming correct periph rbf into FPGA. When the SDRAM is up
>>> running, then only SPL can load U-boot and booting from there.
>>> The rest of external multiple FPGAs can be configured through U-
>>> boot,
>>> such as boot script, interactive in console or from PC host. Other
>>> than
>>> that, external FPGA can also be configured through Linux.
>>>>
>>>>>
>>>>>
>>>>> According to chosen node document, chosen node doesn't
>>>>> represent a
>>>>> real
>>>>> HW, but serves as place for passing data. This is why our BSP
>>>>> tool
>>>>> put
>>>>> the filename info here, the file is named by user in our tool,
>>>>> and
>>>>> this
>>>>> info would be consumed by SPL to program FPGA.
>>>>> What do you think?
>>>> Your BSP tool is broken.
>>>>
>>> The BSP tool is used to describe internal FPGA in SOCFPGA. Other
>>> external FPGAs other than SOCFPGA itself, it can be programmed
>>> through
>>> U-boot.
>> The BSP tool is broken if it generates broken DT, do I have to repeat
>> myself ?
>>
> BSP tool is only generate the RBF filename for FPGA inside SOCFPGA.
> Multiple external FPGA are configured through U-boot.

What happens if you have FPGA connector over SPI ?

>> [...]
Chee, Tien Fong Sept. 6, 2017, 5:06 a.m. UTC | #10
On Sel, 2017-09-05 at 11:36 +0200, Marek Vasut wrote:
> On 09/05/2017 11:23 AM, Chee, Tien Fong wrote:

> > 

> > On Sel, 2017-09-05 at 11:04 +0200, Marek Vasut wrote:

> > > 

> > > On 09/05/2017 07:53 AM, Chee, Tien Fong wrote:

> > > > 

> > > > 

> > > > On Isn, 2017-09-04 at 11:39 +0200, Marek Vasut wrote:

> > > > > 

> > > > > 

> > > > > On 09/04/2017 09:08 AM, Chee, Tien Fong wrote:

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > On Rab, 2017-08-30 at 10:52 +0200, Marek Vasut wrote:

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > On 08/29/2017 12:45 PM, tien.fong.chee@intel.com

> > > > > > > > > wrote:

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

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

> > > > > > > > > > 

> > > > > > > > > > This driver handles FPGA program operation from

> > > > > > > > > > flash

> > > > > > > > > > loading

> > > > > > > > > > RBF to memory and then to program FPGA.

> > > > > > > > > > 

> > > > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel

> > > > > > > > > > .com

> > > > > > > > > > > 

> > > > > > > > > > > 

> > > > > > > > > > ---

> > > > > > > > > >  .../include/mach/fpga_manager_arria10.h           

> > > > > > > > > >  |  

> > > > > > > > > >  27

> > > > > > > > > > ++

> > > > > > > > > >  drivers/fpga/socfpga_arria10.c                    

> > > > > > > > > >  |  

> > > > > > > > > > 386

> > > > > > > > > > +++++++++++++++++++-

> > > > > > > > > >  include/altera.h                                  

> > > > > > > > > >  |  

> > > > > > > > > >   6

> > > > > > > > > > +

> > > > > > > > > >  include/configs/socfpga_common.h                  

> > > > > > > > > >  |  

> > > > > > > > > >   4

> > > > > > > > > > +

> > > > > > > > > >  4 files changed, 422 insertions(+), 1 deletions(-)

> > > > > > > > > > 

> > > > > > > > > > diff --git a/arch/arm/mach-

> > > > > > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > > > > > b/arch/arm/mach-

> > > > > > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > > > > > index 9cbf696..93a9122 100644

> > > > > > > > > > --- a/arch/arm/mach-

> > > > > > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > > > > > +++ b/arch/arm/mach-

> > > > > > > > > > socfpga/include/mach/fpga_manager_arria10.h

> > > > > > > > > > @@ -8,6 +8,8 @@

> > > > > > > > > >  #ifndef _FPGA_MANAGER_ARRIA10_H_

> > > > > > > > > >  #define _FPGA_MANAGER_ARRIA10_H_

> > > > > > > > > >  

> > > > > > > > > > +#include <asm/cache.h>

> > > > > > > > > > +

> > > > > > > > > >  #define

> > > > > > > > > > ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK	

> > > > > > > > > > 	

> > > > > > > > > > BIT(0)

> > > > > > > > > >  #define

> > > > > > > > > > ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK

> > > > > > > > > > 	

> > > > > > > > > > BIT(1)

> > > > > > > > > >  #define

> > > > > > > > > > ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 	

> > > > > > > > > > 	

> > > > > > > > > > BIT(2)

> > > > > > > > > > @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {

> > > > > > > > > >  	u32  imgcfg_fifo_status;

> > > > > > > > > >  };

> > > > > > > > > >  

> > > > > > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > > > > > +enum rbf_type {unknown, periph_section,

> > > > > > > > > > core_section};

> > > > > > > > > > +enum rbf_security {invalid, unencrypted,

> > > > > > > > > > encrypted};

> > > > > > > > > > +

> > > > > > > > > > +struct rbf_info {

> > > > > > > > > > +	enum rbf_type section;

> > > > > > > > > > +	enum rbf_security security;

> > > > > > > > > > +};

> > > > > > > > > > +

> > > > > > > > > > +struct flash_info {

> > > > > > > > > > +	char *interface;

> > > > > > > > > > +	char *dev_part;

> > > > > > > > > > +	char *filename;

> > > > > > > > > > +	int fstype;

> > > > > > > > > > +	u32 remaining;

> > > > > > > > > > +	u32 flash_offset;

> > > > > > > > > > +	struct rbf_info rbfinfo;

> > > > > > > > > > +	struct image_header header;

> > > > > > > > > > +};

> > > > > > > > > > +#endif

> > > > > > > > > > +

> > > > > > > > > >  /* Functions */

> > > > > > > > > >  int fpgamgr_program_init(u32 * rbf_data, size_t

> > > > > > > > > > rbf_size);

> > > > > > > > > >  int fpgamgr_program_finish(void);

> > > > > > > > > >  int is_fpgamgr_user_mode(void);

> > > > > > > > > >  int fpgamgr_wait_early_user_mode(void);

> > > > > > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > > > > > +const char *get_cff_filename(const void *fdt, int

> > > > > > > > > > *len,

> > > > > > > > > > u32

> > > > > > > > > > core);

> > > > > > > > > > +const char *get_cff_devpart(const void *fdt, int

> > > > > > > > > > *len);

> > > > > > > > > > +#endif

> > > > > > > > > >  

> > > > > > > > > >  #endif /* __ASSEMBLY__ */

> > > > > > > > > >  

> > > > > > > > > > diff --git a/drivers/fpga/socfpga_arria10.c

> > > > > > > > > > b/drivers/fpga/socfpga_arria10.c

> > > > > > > > > > index 5c1a68a..90c55e5 100644

> > > > > > > > > > --- a/drivers/fpga/socfpga_arria10.c

> > > > > > > > > > +++ b/drivers/fpga/socfpga_arria10.c

> > > > > > > > > > @@ -13,6 +13,12 @@

> > > > > > > > > >  #include <altera.h>

> > > > > > > > > >  #include <common.h>

> > > > > > > > > >  #include <errno.h>

> > > > > > > > > > +#include <fat.h>

> > > > > > > > > > +#include <fs.h>

> > > > > > > > > > +#include <fdtdec.h>

> > > > > > > > > > +#include <malloc.h>

> > > > > > > > > > +#include <part.h>

> > > > > > > > > > +#include <spl.h>

> > > > > > > > > >  #include <wait_bit.h>

> > > > > > > > > >  #include <watchdog.h>

> > > > > > > > > >  

> > > > > > > > > > @@ -22,6 +28,10 @@

> > > > > > > > > >  #define COMPRESSION_OFFSET	229

> > > > > > > > > >  #define FPGA_TIMEOUT_MSEC	1000  /* timeout

> > > > > > > > > > in

> > > > > > > > > > ms */

> > > > > > > > > >  #define FPGA_TIMEOUT_CNT	0x1000000

> > > > > > > > > > +#define RBF_UNENCRYPTED		0xa65c

> > > > > > > > > > +#define RBF_ENCRYPTED		0xa65d

> > > > > > > > > > +#define ARRIA10RBF_PERIPH	0x0001

> > > > > > > > > > +#define ARRIA10RBF_CORE		0x8001

> > > > > > > > > >  

> > > > > > > > > >  DECLARE_GLOBAL_DATA_PTR;

> > > > > > > > > >  

> > > > > > > > > > @@ -118,7 +128,7 @@ static int

> > > > > > > > > > wait_for_nconfig_pin_and_nstatus_pin(void)

> > > > > > > > > >  	return wait_for_bit(__func__,

> > > > > > > > > >  			    &fpga_manager_base-

> > > > > > > > > > > 

> > > > > > > > > > > 

> > > > > > > > > > > imgcfg_stat,

> > > > > > > > > >  			    mask,

> > > > > > > > > > -			    false,

> > > > > > > > > > FPGA_TIMEOUT_MSEC,

> > > > > > > > > > false);

> > > > > > > > > > +			    true,

> > > > > > > > > > FPGA_TIMEOUT_MSEC,

> > > > > > > > > > false);

> > > > > > > > > >  }

> > > > > > > > > >  

> > > > > > > > > >  static int wait_for_f2s_nstatus_pin(unsigned long

> > > > > > > > > > value)

> > > > > > > > > > @@ -453,6 +463,281 @@ int

> > > > > > > > > > fpgamgr_program_finish(void)

> > > > > > > > > >  	return 0;

> > > > > > > > > >  }

> > > > > > > > > >  

> > > > > > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)

> > > > > > > > > > +const char *get_cff_filename(const void *fdt, int

> > > > > > > > > > *len,

> > > > > > > > > > u32

> > > > > > > > > > core)

> > > > > > > > > > +{

> > > > > > > > > > +	const char *cff_filename = NULL;

> > > > > > > > > > +	const char *cell;

> > > > > > > > > > +	int nodeoffset;

> > > > > > > > > > +	nodeoffset = fdt_subnode_offset(fdt, 0,

> > > > > > > > > > "chosen");

> > > > > > > > > > +

> > > > > > > > > > +	if (nodeoffset >= 0) {

> > > > > > > > > > +		if (core)

> > > > > > > > > > +			cell = fdt_getprop(fdt,

> > > > > > > > > > +					nodeoffset

> > > > > > > > > > ,

> > > > > > > > > > +					"cffcore-

> > > > > > > > > > file",

> > > > > > > > > > +					len);

> > > > > > > > > > +		else

> > > > > > > > > > +			cell = fdt_getprop(fdt,

> > > > > > > > > > nodeoffset,

> > > > > > > > > > "cff-

> > > > > > > > > > file", len);

> > > > > > > > > This should be a property of the FPGA , not the

> > > > > > > > > system .

> > > > > > > > > You

> > > > > > > > > can

> > > > > > > > > have

> > > > > > > > > multiple FPGAs and then this would become a problem.

> > > > > > > > > 

> > > > > > > > This setting is for the only one FPGA inside our

> > > > > > > > SoCFPGA.

> > > > > > > You just said it yourself, it is for the only FPGA in

> > > > > > > your

> > > > > > > SOCFPGA ,

> > > > > > > thus it is a property of the FPGA , not a chosen .

> > > > > > > 

> > > > > > Okay, what i trying to tell is that there is no multiple

> > > > > > FPGAs

> > > > > > in

> > > > > > our

> > > > > > SOCFPGA. The filename is not any hardware properties, it is

> > > > > > just a

> > > > > > info

> > > > > > to tell SPL and U-boot which file to look for programming

> > > > > > FPGA.

> > > > > What would happen if you attached an FPGA over ie. SPI or

> > > > > PCIe ?

> > > > > Then you have two FPGAs in the system and you need to

> > > > > describe

> > > > > them

> > > > > in

> > > > > the DT and your "chosen" approach breaks down.

> > > > > 

> > > > We only need to decribe the internal FPGA of SoCFPGA in DT

> > > This is incorrect. You describe the hardware in DT, if you have

> > > multiple

> > > FPGAs, then your approach breaks down.

> > > 

> > Let me clarify, we don't have FPGA HW properties described in DT,

> > only

> > the RBF files are defined under chosen node. The files are defined

> > only

> > apply for FPGA inside SOCFPGA.

> > Multiple external FPGA are configured through U-boot.

> > > 

> > > > 

> > > > 

> > > > such as

> > > > "chosen" to specify which file should SPL looking for getting

> > > > SDRAM

> > > > up

> > > > with programming correct periph rbf into FPGA. When the SDRAM

> > > > is up

> > > > running, then only SPL can load U-boot and booting from there.

> > > > The rest of external multiple FPGAs can be configured through

> > > > U-

> > > > boot,

> > > > such as boot script, interactive in console or from PC host.

> > > > Other

> > > > than

> > > > that, external FPGA can also be configured through Linux.

> > > > > 

> > > > > 

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > According to chosen node document, chosen node doesn't

> > > > > > represent a

> > > > > > real

> > > > > > HW, but serves as place for passing data. This is why our

> > > > > > BSP

> > > > > > tool

> > > > > > put

> > > > > > the filename info here, the file is named by user in our

> > > > > > tool,

> > > > > > and

> > > > > > this

> > > > > > info would be consumed by SPL to program FPGA.

> > > > > > What do you think?

> > > > > Your BSP tool is broken.

> > > > > 

> > > > The BSP tool is used to describe internal FPGA in SOCFPGA.

> > > > Other

> > > > external FPGAs other than SOCFPGA itself, it can be programmed

> > > > through

> > > > U-boot.

> > > The BSP tool is broken if it generates broken DT, do I have to

> > > repeat

> > > myself ?

> > > 

> > BSP tool is only generate the RBF filename for FPGA inside SOCFPGA.

> > Multiple external FPGA are configured through U-boot.

> What happens if you have FPGA connector over SPI ?

> 

I assume you are saying FPGA connected to EPCQ, and this is one of the
external FPGA configuration, like PCIE. For any external FPGA
configuration, FPGA itself/external HOST would get the FPGA data from
storage such as EPCQ and configuring the FPGA without HPS/Bootloader
intervine. SPL/U-boot would skip the FPGA configuration process when
they see the mode is set to external FPGA configuration.

I know you want a DT to describe all FPGAs with FPGA node and their own
data filename in every node. But, at this moment, all external FPGAs
chip other than SOCFPGA itself are configured through U-boot(script &
env variable), or external FPGA configuration method.

How about i just create a FPGA node for SOCFPGA, with FPGA data
filename within the node?
> > 

> > > 

> > > [...]

>
Marek Vasut Sept. 6, 2017, 7:10 a.m. UTC | #11
On 09/06/2017 07:06 AM, Chee, Tien Fong wrote:
[...]
>>>>> The BSP tool is used to describe internal FPGA in SOCFPGA.
>>>>> Other
>>>>> external FPGAs other than SOCFPGA itself, it can be programmed
>>>>> through
>>>>> U-boot.
>>>> The BSP tool is broken if it generates broken DT, do I have to
>>>> repeat
>>>> myself ?
>>>>
>>> BSP tool is only generate the RBF filename for FPGA inside SOCFPGA.
>>> Multiple external FPGA are configured through U-boot.
>> What happens if you have FPGA connector over SPI ?
>>
> I assume you are saying FPGA connected to EPCQ

No, I mean FPGA connected over SPI bus.

>, and this is one of the
> external FPGA configuration, like PCIE. For any external FPGA
> configuration, FPGA itself/external HOST would get the FPGA data from
> storage such as EPCQ and configuring the FPGA without HPS/Bootloader
> intervine. SPL/U-boot would skip the FPGA configuration process when
> they see the mode is set to external FPGA configuration.
> 
> I know you want a DT to describe all FPGAs with FPGA node and their own
> data filename in every node. But, at this moment, all external FPGAs
> chip other than SOCFPGA itself are configured through U-boot(script &
> env variable), or external FPGA configuration method.

I want DT which describes hardware and is not misdesigned crap.
Just because the BSP tool is broken does not mean I can allow upstream
to accept that, no way.

> How about i just create a FPGA node for SOCFPGA, with FPGA data
> filename within the node?

Isn't that what the FPGA manager in mainline Linux does already ?
Chee, Tien Fong Sept. 6, 2017, 7:15 a.m. UTC | #12
On Rab, 2017-09-06 at 09:10 +0200, Marek Vasut wrote:
> On 09/06/2017 07:06 AM, Chee, Tien Fong wrote:

> [...]

> > 

> > > 

> > > > 

> > > > > 

> > > > > > 

> > > > > > The BSP tool is used to describe internal FPGA in SOCFPGA.

> > > > > > Other

> > > > > > external FPGAs other than SOCFPGA itself, it can be

> > > > > > programmed

> > > > > > through

> > > > > > U-boot.

> > > > > The BSP tool is broken if it generates broken DT, do I have

> > > > > to

> > > > > repeat

> > > > > myself ?

> > > > > 

> > > > BSP tool is only generate the RBF filename for FPGA inside

> > > > SOCFPGA.

> > > > Multiple external FPGA are configured through U-boot.

> > > What happens if you have FPGA connector over SPI ?

> > > 

> > I assume you are saying FPGA connected to EPCQ

> No, I mean FPGA connected over SPI bus.

> 

> > 

> > , and this is one of the

> > external FPGA configuration, like PCIE. For any external FPGA

> > configuration, FPGA itself/external HOST would get the FPGA data

> > from

> > storage such as EPCQ and configuring the FPGA without

> > HPS/Bootloader

> > intervine. SPL/U-boot would skip the FPGA configuration process

> > when

> > they see the mode is set to external FPGA configuration.

> > 

> > I know you want a DT to describe all FPGAs with FPGA node and their

> > own

> > data filename in every node. But, at this moment, all external

> > FPGAs

> > chip other than SOCFPGA itself are configured through U-boot(script 

> > &

> > env variable), or external FPGA configuration method.

> I want DT which describes hardware and is not misdesigned crap.

> Just because the BSP tool is broken does not mean I can allow

> upstream

> to accept that, no way.

> 

> > 

> > How about i just create a FPGA node for SOCFPGA, with FPGA data

> > filename within the node?

> Isn't that what the FPGA manager in mainline Linux does already ?

> 

Yeah, i will port from Linux DTS, and filenames added into the node.
diff mbox series

Patch

diff --git a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
index 9cbf696..93a9122 100644
--- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
+++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
@@ -8,6 +8,8 @@ 
 #ifndef _FPGA_MANAGER_ARRIA10_H_
 #define _FPGA_MANAGER_ARRIA10_H_
 
+#include <asm/cache.h>
+
 #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK		BIT(0)
 #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK	BIT(1)
 #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 		BIT(2)
@@ -89,11 +91,36 @@  struct socfpga_fpga_manager {
 	u32  imgcfg_fifo_status;
 };
 
+#if defined(CONFIG_CMD_FPGA_LOADFS)
+enum rbf_type {unknown, periph_section, core_section};
+enum rbf_security {invalid, unencrypted, encrypted};
+
+struct rbf_info {
+	enum rbf_type section;
+	enum rbf_security security;
+};
+
+struct flash_info {
+	char *interface;
+	char *dev_part;
+	char *filename;
+	int fstype;
+	u32 remaining;
+	u32 flash_offset;
+	struct rbf_info rbfinfo;
+	struct image_header header;
+};
+#endif
+
 /* Functions */
 int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);
 int fpgamgr_program_finish(void);
 int is_fpgamgr_user_mode(void);
 int fpgamgr_wait_early_user_mode(void);
+#if defined(CONFIG_CMD_FPGA_LOADFS)
+const char *get_cff_filename(const void *fdt, int *len, u32 core);
+const char *get_cff_devpart(const void *fdt, int *len);
+#endif
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/drivers/fpga/socfpga_arria10.c b/drivers/fpga/socfpga_arria10.c
index 5c1a68a..90c55e5 100644
--- a/drivers/fpga/socfpga_arria10.c
+++ b/drivers/fpga/socfpga_arria10.c
@@ -13,6 +13,12 @@ 
 #include <altera.h>
 #include <common.h>
 #include <errno.h>
+#include <fat.h>
+#include <fs.h>
+#include <fdtdec.h>
+#include <malloc.h>
+#include <part.h>
+#include <spl.h>
 #include <wait_bit.h>
 #include <watchdog.h>
 
@@ -22,6 +28,10 @@ 
 #define COMPRESSION_OFFSET	229
 #define FPGA_TIMEOUT_MSEC	1000  /* timeout in ms */
 #define FPGA_TIMEOUT_CNT	0x1000000
+#define RBF_UNENCRYPTED		0xa65c
+#define RBF_ENCRYPTED		0xa65d
+#define ARRIA10RBF_PERIPH	0x0001
+#define ARRIA10RBF_CORE		0x8001
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -118,7 +128,7 @@  static int wait_for_nconfig_pin_and_nstatus_pin(void)
 	return wait_for_bit(__func__,
 			    &fpga_manager_base->imgcfg_stat,
 			    mask,
-			    false, FPGA_TIMEOUT_MSEC, false);
+			    true, FPGA_TIMEOUT_MSEC, false);
 }
 
 static int wait_for_f2s_nstatus_pin(unsigned long value)
@@ -453,6 +463,281 @@  int fpgamgr_program_finish(void)
 	return 0;
 }
 
+#if defined(CONFIG_CMD_FPGA_LOADFS)
+const char *get_cff_filename(const void *fdt, int *len, u32 core)
+{
+	const char *cff_filename = NULL;
+	const char *cell;
+	int nodeoffset;
+	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
+
+	if (nodeoffset >= 0) {
+		if (core)
+			cell = fdt_getprop(fdt,
+					nodeoffset,
+					"cffcore-file",
+					len);
+		else
+			cell = fdt_getprop(fdt, nodeoffset, "cff-file", len);
+
+		if (cell)
+			cff_filename = cell;
+	}
+
+	return cff_filename;
+}
+
+const char *get_cff_devpart(const void *fdt, int *len)
+{
+	const char *cff_devpart = NULL;
+	const char *cell;
+	int nodeoffset;
+	nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
+
+		cell = fdt_getprop(fdt, nodeoffset, "cff_devpart", len);
+
+		if (cell)
+			cff_devpart = cell;
+
+	return cff_devpart;
+}
+
+void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer)
+{
+	/*
+	  Magic ID starting at:
+	   -> 1st dword in periph.rbf
+	   -> 2nd dword in core.rbf
+	*/
+	u32 word_reading_max = 2;
+	u32 i;
+
+	for(i = 0; i < word_reading_max; i++)
+	{
+		if(RBF_UNENCRYPTED == *(buffer + i)) /* PERIPH RBF */
+			rbf->security = unencrypted;
+		else if (RBF_ENCRYPTED == *(buffer + i))
+			rbf->security = encrypted;
+		else if (RBF_UNENCRYPTED == *(buffer + i + 1)) /* CORE RBF */
+					rbf->security = unencrypted;
+		else if (RBF_ENCRYPTED == *(buffer + i + 1))
+					rbf->security = encrypted;
+		else {
+			rbf->security = invalid;
+			continue;
+		}
+
+		/* PERIPH RBF */
+		if (ARRIA10RBF_PERIPH == *(buffer + i + 1)) {
+			rbf->section = periph_section;
+			break;
+		}
+		else if (ARRIA10RBF_CORE == *(buffer + i + 1)) {
+			rbf->section = core_section;
+			break;
+		} /* CORE RBF */
+		else if (ARRIA10RBF_PERIPH == *(buffer + i + 2)) {
+			rbf->section = periph_section;
+			break;
+		}
+		else if (ARRIA10RBF_CORE == *(buffer + i + 2)) {
+			rbf->section = core_section;
+			break;
+		}
+		else {
+			rbf->section = unknown;
+			break;
+		}
+	}
+
+	return;
+}
+
+static int flash_read(struct flash_info *flashinfo,
+	u32 size_read,
+	u32 *buffer_ptr)
+{
+	size_t ret = EEXIST;
+	loff_t actread = 0;
+
+#ifdef CONFIG_FS_FAT
+		ret = fat_read_file(flashinfo->filename,
+				buffer_ptr, flashinfo->flash_offset,
+				 size_read, &actread);
+#endif
+
+		if (ret || actread != size_read) {
+			printf("Failed to read %s from flash %d ",
+				flashinfo->filename,
+				 ret);
+			printf("!= %d.\n", size_read);
+			return -EPERM;
+		} else
+			ret = actread;
+
+	return ret;
+}
+
+static int fs_flash_preinit(struct flash_info *flashinfo,
+	u32 *buffer, u32 *buffer_sizebytes)
+{
+	u32 *bufferptr_after_header = NULL;
+	u32 buffersize_after_header = 0;
+	u32 rbf_header_data_size = 0;
+	int ret = 0;
+
+	flashinfo->flash_offset = 0;
+
+	/* To avoid from keeping re-read the contents */
+	struct image_header *header = &(flashinfo->header);
+	size_t buffer_size = *buffer_sizebytes;
+	u32 *buffer_ptr = (u32 *)*buffer;
+
+
+	 /* Load mkimage header into buffer */
+	ret = flash_read(flashinfo,
+			sizeof(struct image_header), buffer_ptr);
+
+	if (0 >= ret) {
+		printf(" Failed to read mkimage header from flash.\n");
+		return -ENOENT;
+	}
+
+	WATCHDOG_RESET();
+
+	memcpy(header, (u_char *)buffer_ptr, sizeof(*header));
+
+	if (!image_check_magic(header)) {
+		printf("FPGA: Bad Magic Number.\n");
+		return -EBADF;
+	}
+
+	if (!image_check_hcrc(header)) {
+		printf("FPGA: Bad Header Checksum.\n");
+		return -EPERM;
+	}
+
+	/* Getting rbf data size */
+	flashinfo->remaining =
+		image_get_data_size(header);
+
+	/* Calculate total size of both rbf data with mkimage header */
+	rbf_header_data_size = flashinfo->remaining +
+				sizeof(struct image_header);
+
+	/* Loading to buffer chunk by chunk, normally for OCRAM buffer */
+	if (rbf_header_data_size > buffer_size) {
+		/* Calculate size of rbf data in the buffer */
+		buffersize_after_header =
+			buffer_size - sizeof(struct image_header);
+		flashinfo->remaining -= buffersize_after_header;
+	} else {
+	/* Loading whole rbf image into buffer, normally for DDR buffer */
+		buffer_size = rbf_header_data_size;
+		/* Calculate size of rbf data in the buffer */
+		buffersize_after_header =
+			buffer_size - sizeof(struct image_header);
+		flashinfo->remaining = 0;
+	}
+
+	/* Loading mkimage header and rbf data into buffer */
+	ret = flash_read(flashinfo, buffer_size, buffer_ptr);
+
+	if (0 >= ret) {
+		printf(" Failed to read mkimage header and rbf data ");
+		printf("from flash.\n");
+		return -ENOENT;
+	}
+
+	/* Getting pointer of rbf data starting address where is it
+	   right after mkimage header */
+	bufferptr_after_header =
+		(u32 *)((u_char *)buffer_ptr + sizeof(struct image_header));
+
+	/* Update next reading rbf data flash offset */
+	flashinfo->flash_offset += buffer_size;
+
+	/* Update the starting addr of rbf data to init FPGA & programming
+	   into FPGA */
+	*buffer = (u32)bufferptr_after_header;
+
+	get_rbf_image_info(&flashinfo->rbfinfo, (u16 *)bufferptr_after_header);
+
+	/* Update the size of rbf data to be programmed into FPGA */
+	*buffer_sizebytes = buffersize_after_header;
+
+#ifdef CONFIG_CHECK_FPGA_DATA_CRC
+	flashinfo->datacrc =
+		crc32(flashinfo->datacrc,
+		(u_char *)bufferptr_after_header,
+		buffersize_after_header);
+#endif
+
+if (0 == flashinfo->remaining) {
+#ifdef CONFIG_CHECK_FPGA_DATA_CRC
+	if (flashinfo->datacrc !=
+		image_get_dcrc(&(flashinfo->header))) {
+		printf("FPGA: Bad Data Checksum.\n");
+		return -EPERM;
+	}
+#endif
+}
+	return 0;
+}
+
+static int fs_flash_read(struct flash_info *flashinfo, u32 *buffer,
+	u32 *buffer_sizebytes)
+{
+	int ret = 0;
+	/* To avoid from keeping re-read the contents */
+	size_t buffer_size = *buffer_sizebytes;
+	u32 *buffer_ptr = (u32 *)*buffer;
+	u32 flash_addr = flashinfo->flash_offset;
+
+	/* Buffer allocated in OCRAM */
+	/* Read the data by small chunk by chunk. */
+	if (flashinfo->remaining > buffer_size)
+		flashinfo->remaining -= buffer_size;
+	else {
+		/* Buffer allocated in DDR, larger than rbf data most
+		  of the time */
+		buffer_size = flashinfo->remaining;
+		flashinfo->remaining = 0;
+	}
+
+	ret = flash_read(flashinfo, buffer_size, buffer_ptr);
+
+	if (0 >= ret) {
+		printf(" Failed to read rbf data from flash.\n");
+		return -ENOENT;
+	}
+
+#ifdef CONFIG_CHECK_FPGA_DATA_CRC
+	flashinfo->datacrc =
+		crc32(flashinfo->datacrc,
+			(unsigned char *)buffer_ptr, buffer_size);
+#endif
+
+if (0 == flashinfo->remaining) {
+#ifdef CONFIG_CHECK_FPGA_DATA_CRC
+	if (flashinfo->datacrc !=
+		image_get_dcrc(&(flashinfo->header))) {
+		printf("FPGA: Bad Data Checksum.\n");
+		return -EPERM;
+	}
+#endif
+}
+	/* Update next reading rbf data flash offset */
+	flash_addr += buffer_size;
+
+	flashinfo->flash_offset = flash_addr;
+
+	/* Update the size of rbf data to be programmed into FPGA */
+	*buffer_sizebytes = buffer_size;
+
+	return 0;
+}
+
 /*
  * FPGA Manager to program the FPGA. This is the interface used by FPGA driver.
  * Return 0 for sucess, non-zero for error.
@@ -469,6 +754,7 @@  int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size)
 
 	/* Initialize the FPGA Manager */
 	status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
+
 	if (status)
 		return status;
 
@@ -477,3 +763,101 @@  int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size)
 
 	return fpgamgr_program_finish();
 }
+
+int socfpga_loadfs(Altera_desc *desc, const void *buf, size_t bsize,
+		   fpga_fs_info *fpga_fsinfo)
+{
+	u32 buffer = 0;
+	u32 buffer_ori = 0;
+	size_t buffer_sizebytes = 0;
+	size_t buffer_sizebytes_ori = 0;
+	struct flash_info flashinfo;
+	u32 status = 0;
+	int ret = 0;
+
+	memset(&flashinfo, 0, sizeof(flashinfo));
+
+	if (fpga_fsinfo->filename == NULL) {
+		printf("no peripheral RBF filename specified.\n");
+		return -EINVAL;
+	}
+
+	WATCHDOG_RESET();
+
+	buffer_sizebytes = buffer_sizebytes_ori = bsize;
+	buffer = buffer_ori = (u32) buf;
+	flashinfo.interface = fpga_fsinfo->interface;
+	flashinfo.dev_part = fpga_fsinfo->dev_part;
+	flashinfo.filename = fpga_fsinfo->filename;
+	flashinfo.fstype = fpga_fsinfo->fstype;
+
+#ifndef CONFIG_SPL_BUILD
+	if (fs_set_blk_dev(flashinfo.interface, flashinfo.dev_part,
+				 flashinfo.fstype))
+	return FPGA_FAIL;
+#endif
+
+	/* Note: Both buffer and buffer_sizebytes values can be altered by
+	   function below. */
+	ret = fs_flash_preinit(&flashinfo, &buffer, &buffer_sizebytes);
+
+	if (ret)
+		return ret;
+
+	if (periph_section == flashinfo.rbfinfo.section) {
+		/* Initialize the FPGA Manager */
+		status = fpgamgr_program_init((u32 *)buffer, buffer_sizebytes);
+		if (status) {
+			printf("FPGA: Init with periph rbf failed with error. ");
+			printf("code %d\n", status);
+			return -EPERM;
+		}
+	}
+
+	WATCHDOG_RESET();
+
+	/* Transfer data to FPGA Manager */
+	fpgamgr_program_write((void *)buffer,
+		buffer_sizebytes);
+
+	WATCHDOG_RESET();
+
+	while (flashinfo.remaining) {
+		ret = fs_flash_read(&flashinfo, &buffer_ori,
+			&buffer_sizebytes_ori);
+
+		if (ret)
+			return ret;
+
+		/* transfer data to FPGA Manager */
+		fpgamgr_program_write((void *)buffer_ori,
+			buffer_sizebytes_ori);
+
+		WATCHDOG_RESET();
+	}
+
+	if (periph_section == flashinfo.rbfinfo.section) {
+		if (-ETIMEDOUT != fpgamgr_wait_early_user_mode())
+			printf("FPGA: Early Release Succeeded.\n");
+		else {
+			printf("FPGA: Failed to see Early Release.\n");
+			return -EIO;
+		}
+	} else if (core_section == flashinfo.rbfinfo.section) {
+		/* Ensure the FPGA entering config done */
+		status = fpgamgr_program_finish();
+		if (status)
+			return status;
+		else
+			printf("FPGA: Enter user mode.\n");
+
+	} else {
+		printf("Config Error: Unsupported FGPA raw binary type.\n");
+		return -ENOEXEC;
+	}
+
+	WATCHDOG_RESET();
+	return 1;
+
+}
+#endif
diff --git a/include/altera.h b/include/altera.h
index 48d3eb7..0597e8a 100644
--- a/include/altera.h
+++ b/include/altera.h
@@ -84,6 +84,10 @@  typedef struct {
 extern int altera_load(Altera_desc *desc, const void *image, size_t size);
 extern int altera_dump(Altera_desc *desc, const void *buf, size_t bsize);
 extern int altera_info(Altera_desc *desc);
+#if defined(CONFIG_CMD_FPGA_LOADFS)
+int altera_loadfs(Altera_desc *desc, const void *buf, size_t bsize,
+		   fpga_fs_info *fpga_fsinfo);
+#endif
 
 /* Board specific implementation specific function types
  *********************************************************************/
@@ -111,6 +115,8 @@  typedef struct {
 
 #ifdef CONFIG_FPGA_SOCFPGA
 int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size);
+int socfpga_loadfs(Altera_desc *desc, const void *buf, size_t bsize,
+		   fpga_fs_info *fpga_fsinfo);
 #endif
 
 #ifdef CONFIG_FPGA_STRATIX_V
diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
index 9be9e79..c15d244 100644
--- a/include/configs/socfpga_common.h
+++ b/include/configs/socfpga_common.h
@@ -27,7 +27,11 @@ 
  */
 #define CONFIG_NR_DRAM_BANKS		1
 #define PHYS_SDRAM_1			0x0
+#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
 #define CONFIG_SYS_MALLOC_LEN		(64 * 1024 * 1024)
+#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
+#define CONFIG_SYS_MALLOC_LEN		(128 * 1024 * 1024)
+#endif
 #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1
 #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1_SIZE
 #if defined(CONFIG_TARGET_SOCFPGA_GEN5)