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

[Marek Vasut <marex@denx.de>]

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 .

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


-- 
Best regards,
Marek Vasut