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

[Chee, Tien Fong <tien.fong.chee@intel.com>]

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