[U-Boot] [PATCH 2/9] ARM: socfpga: Add FPGA drivers for Arria 10 FPGA bitstream loading

Marek Vasut marex at denx.de
Fri Nov 23 12:28:20 UTC 2018


On 11/23/2018 10:43 AM, Chee, Tien Fong wrote:
> On Wed, 2018-11-21 at 15:18 +0100, Marek Vasut wrote:
>> On 11/21/2018 11:41 AM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>
>>> Add FPGA driver to support program FPGA with FPGA bitstream loading
>>> from
>>> filesystem. The driver are designed based on generic firmware
>>> loader
>>> framework. The driver can handle FPGA program operation from
>>> loading FPGA
>>> bitstream in flash to memory and then to program FPGA.
>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>> [...]
>>
>>>
>>> @@ -51,6 +54,8 @@
>>>  #define ALT_FPGAMGR_IMGCFG_CTL_02_CFGWIDTH_SET_MSK		
>>> BIT(24)
>>>  #define ALT_FPGAMGR_IMGCFG_CTL_02_CDRATIO_LSB			
>>> 16
>>>  
>>> +#define PERIPH_RBF						
>>> 0
>>> +#define CORE_RBF						1
>> Enum, use something with specific prefix.
> Noted.
>>
>>>
>>>  #ifndef __ASSEMBLY__
>>>  
>>>  struct socfpga_fpga_manager {
>>> @@ -88,12 +93,33 @@ struct socfpga_fpga_manager {
>>>  	u32  imgcfg_fifo_status;
>>>  };
>>>  
>>> +enum rbf_type {unknown, periph_section, core_section};
>>> +enum rbf_security {invalid, unencrypted, encrypted};
>> enum should use one line per entry.
> Noted.
>>
>>>
>>> +struct rbf_info {
>>> +	enum rbf_type section;
>>> +	enum rbf_security security;
>>> +};
>>> +
>>> +struct fpga_loadfs_info {
>>> +	fpga_fs_info *fpga_fsinfo;
>>> +	u32 remaining;
>>> +	u32 offset;
>>> +	u32 datacrc;
>>> +	struct rbf_info rbfinfo;
>>> +	struct image_header header;
>>> +};
>>> +
>>>  /* 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);
>>> -
>>> +int is_fpgamgr_early_user_mode(void);
>>> +const char *get_fpga_filename(const void *fdt, int *len, u32
>>> core);
>>> +void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer);
>>> +int socfpga_loadfs(fpga_fs_info *fpga_fsinfo, const void *buf,
>>> size_t bsize,
>>> +			u32 offset);
>>>  #endif /* __ASSEMBLY__ */
>>>  
>>>  #endif /* _FPGA_MANAGER_ARRIA10_H_ */
>>> diff --git a/configs/socfpga_arria10_defconfig
>>> b/configs/socfpga_arria10_defconfig
>>> index 6ebda81..f88910c 100644
>>> --- a/configs/socfpga_arria10_defconfig
>>> +++ b/configs/socfpga_arria10_defconfig
>>> @@ -27,8 +27,17 @@ CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
>>>  # CONFIG_EFI_PARTITION is not set
>>>  CONFIG_DEFAULT_DEVICE_TREE="socfpga_arria10_socdk_sdmmc"
>>>  CONFIG_ENV_IS_IN_MMC=y
>>> +CONFIG_SPL_ENV_SUPPORT=y
>>>  CONFIG_SPL_DM=y
>>>  CONFIG_SPL_DM_SEQ_ALIAS=y
>>> +CONFIG_SPL_DM_MMC=y
>>> +CONFIG_SPL_MMC_SUPPORT=y
>>> +CONFIG_SPL_FS_SUPPORT=y
>>> +CONFIG_SPL_EXT_SUPPORT=y
>>> +CONFIG_SPL_FAT_SUPPORT=y
>>> +CONFIG_SPL_DRIVERS_MISC_SUPPORT=y
>>> +CONFIG_FS_FAT_MAX_CLUSTSIZE=16384
>>> +CONFIG_FS_LOADER=y
>> Separate patch
> Okay.
>>
>>>
>>>  CONFIG_FPGA_SOCFPGA=y
>>>  CONFIG_DM_GPIO=y
>>>  CONFIG_DWAPB_GPIO=y
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>> index 50e9019..06a8204 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -21,6 +21,15 @@ config FPGA_SOCFPGA
>>>  
>>>  	  This provides common functionality for Gen5 and Arria10
>>> devices.
>>>  
>>> +config CHECK_FPGA_DATA_CRC
>> config FPGA_SOCFPGA_A10_CRC_CHECK
>>
>> What is this for and why shouldn't this be ON by default ?
> Both periph.rbf or full.rbf are wrapped with mkimage. So, this CRC
> checking can be used to check the integrity of the loading bitstream
> data against checksum from mkimage. It is off for the sake of
> performance.

Is there a measurable performance degradation ? I presume that's because
caches are disabled at that point, yes? Enable caches and see if that helps.

>>>
>>> +	bool "Enable CRC cheking on Arria10 FPGA bistream"
>>> +	depends on FPGA_SOCFPGA
>>> +	help
>>> +	 Say Y here to enable the CRC checking on Arria 10 FPGA
>>> bitstream
>>> +
>>> +	 This provides CRC checking to ensure integrated of Arria
>>> 10 FPGA
>>> +	 bitstream is programmed into FPGA.
>>> +
>>>  config FPGA_CYCLON2
>>>  	bool "Enable Altera FPGA driver for Cyclone II"
>>>  	depends on FPGA_ALTERA
>>> diff --git a/drivers/fpga/socfpga_arria10.c
>>> b/drivers/fpga/socfpga_arria10.c
>>> index 114dd91..d9ad237 100644
>>> --- a/drivers/fpga/socfpga_arria10.c
>>> +++ b/drivers/fpga/socfpga_arria10.c
>>> @@ -1,6 +1,6 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>>  /*
>>> - * Copyright (C) 2017 Intel Corporation <www.intel.com>
>>> + * Copyright (C) 2017-2018 Intel Corporation <www.intel.com>
>>>   */
>>>  
>>>  #include <asm/io.h>
>>> @@ -10,8 +10,10 @@
>>>  #include <asm/arch/sdram.h>
>>>  #include <asm/arch/misc.h>
>>>  #include <altera.h>
>>> +#include <asm/arch/pinmux.h>
>>>  #include <common.h>
>>>  #include <errno.h>
>>> +#include <fs_loader.h>
>>>  #include <wait_bit.h>
>>>  #include <watchdog.h>
>>>  
>>> @@ -21,6 +23,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
>> This looks awfully similar to the PERIPH_RBF and CORE_RBF above.
> PERIPH_RBF and CORE_RBF are the flags, so i can change them to enum.
> But above #define are magic content used to identify the bistream type.
> If above #define are not suitable, what can you suggest?

Maybe you can just align those two to avoid duplication ?

>>
>>>
>>>  static const struct socfpga_fpga_manager *fpga_manager_base =
>>>  		(void *)SOCFPGA_FPGAMGRREGS_ADDRESS;
>>> @@ -64,7 +70,7 @@ static int wait_for_user_mode(void)
>>>  		1, FPGA_TIMEOUT_MSEC, false);
>>>  }
>>>  
>>> -static int is_fpgamgr_early_user_mode(void)
>>> +int is_fpgamgr_early_user_mode(void)
>>>  {
>>>  	return (readl(&fpga_manager_base->imgcfg_stat) &
>>>  		ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK
>>> ) != 0;
>>> @@ -447,13 +453,393 @@ int fpgamgr_program_finish(void)
>>>  	return 0;
>>>  }
>>>  
>>> +const char *get_fpga_filename(const void *fdt, int *len, u32 core)
>> static, fix globally .
>>
>>>
>>> +{
>>> +	const char *fpga_filename = NULL;
>>> +	const char *cell;
>>> +	int nodeoffset;
>> ofnode_read_string() , use ofnode globally.
> Noted.
>>
>>>
>>> +	nodeoffset = fdtdec_next_compatible(fdt, 0,
>>> +			 COMPAT_ALTERA_SOCFPGA_FPGA0);
>>> +	if (nodeoffset >= 0) {
>>> +		if (core) {
>>> +			cell = fdt_getprop(fdt,
>>> +					nodeoffset,
>>> +					"altr,bitstream_core",
>>> +					len);
>>> +		} else {
>>> +			cell = fdt_getprop(fdt, nodeoffset,
>>> +					"altr,bitstream_periph",
>>> len);
>>> +		}
>>> +
>>> +		if (cell)
>>> +			fpga_filename = cell;
>>> +	}
>>> +
>>> +	return fpga_filename;
>>> +}
>>> +
>>> +void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer)
>>> +{
>>> +	/*
>>> +	 * Magic ID starting at:
>>> +	 * -> 1st dword[15:0] in periph.rbf
>>> +	 * -> 2nd dword[15:0] in core.rbf
>>> +	 * Note: dword == 32 bits
>>> +	 */
>>> +	u32 word_reading_max = 2;
>>> +	u32 i;
>>> +
>>> +	for (i = 0; i < word_reading_max; i++) {
>>> +		if (*(buffer + i) == RBF_UNENCRYPTED) {
>>> +			rbf->security = unencrypted;
>>> +		} else if (*(buffer + i) == RBF_ENCRYPTED) {
>>> +			rbf->security = encrypted;
>>> +		} else if (*(buffer + i + 1) == RBF_UNENCRYPTED) {
>>> +			rbf->security = unencrypted;
>>> +		} else if (*(buffer + i + 1) == RBF_ENCRYPTED) {
>>> +			rbf->security = encrypted;
>>> +		} else {
>>> +			rbf->security = invalid;
>>> +			continue;
>>> +		}
>>> +
>>> +		/* PERIPH RBF(buffer + i + 1), CORE RBF(buffer + i
>>> + 2) */
>>> +		if (*(buffer + i + 1) == ARRIA10RBF_PERIPH) {
>>> +			rbf->section = periph_section;
>>> +			break;
>>> +		} else if (*(buffer + i + 1) == ARRIA10RBF_CORE) {
>>> +			rbf->section = core_section;
>>> +			break;
>>> +		} else if (*(buffer + i + 2) == ARRIA10RBF_PERIPH)
>>> {
>>> +			rbf->section = periph_section;
>>> +			break;
>>> +		} else if (*(buffer + i + 2) == ARRIA10RBF_CORE) {
>>> +			rbf->section = core_section;
>>> +			break;
>>> +		}
>>> +
>>> +		rbf->section = unknown;
>>> +		break;
>>> +
>>> +		WATCHDOG_RESET();
>>> +	}
>>> +}
>>> +
>>> +static struct firmware *fw;
>> What is this static variable doing here ?
> A place for storing firmware and its attribute data. This would be
> shared across a few helper functions which contain firmware loader.

Why is it not in the device data instead ? If you had multiple FPGAs in
the system, this would likely be randomly overwritten . Such static
variables shouldn't be needed in DM/DT capable system.

>>> +int first_loading_rbf_to_buffer(struct device_platdata *plat,
>>> +				struct fpga_loadfs_info
>>> *fpga_loadfs,
>>> +				u32 *buffer, u32 *buffer_bsize)
>>> +{
>>> +	u32 *buffer_p_after_header = NULL;
>>> +	u32 buffersz_after_header = 0;
>>> +	u32 totalsz_header_rbf = 0;
>>> +	u32 *buffer_p = (u32 *)*buffer;
>>> +	struct image_header *header = &(fpga_loadfs->header);
>>> +	size_t buffer_size = *buffer_bsize;
>>> +	int ret = 0;
>>> +
>>> +	/* Load mkimage header into buffer */
>>> +	ret = request_firmware_into_buf(plat,
>>> +					fpga_loadfs->fpga_fsinfo-
>>>> filename,
>>> +					header,
>>> +					sizeof(struct
>>> image_header),
>>> +					fpga_loadfs->offset,
>>> +					&fw);
>>> +	if (ret < 0) {
>>> +		debug("FPGA: Failed to read RBF mkimage header
>>> from flash.\n");
>>> +		return -ENOENT;
>>> +	}
>>> +
>>> +	WATCHDOG_RESET();
>>> +
>>> +	if (!image_check_magic(header)) {
>>> +		debug("FPGA: Bad Magic Number.\n");
>>> +		return -EBADF;
>>> +	}
>>> +
>>> +	if (!image_check_hcrc(header)) {
>>> +		debug("FPGA: Bad Header Checksum.\n");
>>> +		return -EPERM;
>>> +	}
>>> +
>>> +	/* Getting RBF data size from mkimage header */
>>> +	fpga_loadfs->remaining = image_get_data_size(header);
>>> +
>>> +	/* Calculate total size of both RBF with mkimage header */
>>> +	totalsz_header_rbf = fpga_loadfs->remaining +
>>> +				sizeof(struct image_header);
>>> +
>>> +	/*
>>> +	 * Determine buffer size vs RBF size, and calculating
>>> number of
>>> +	 * chunk by chunk transfer is required due to smaller
>>> buffer size
>>> +	 * compare to RBF
>>> +	 */
>>> +	if (totalsz_header_rbf > buffer_size) {
>>> +		/* Calculate size of RBF in the buffer */
>>> +		buffersz_after_header =
>>> +			buffer_size - sizeof(struct image_header);
>>> +		fpga_loadfs->remaining -= buffersz_after_header;
>>> +	} else {
>>> +		/* Loading whole RBF into buffer */
>>> +		buffer_size = totalsz_header_rbf;
>>> +		/* Calculate size of RBF in the buffer */
>>> +		buffersz_after_header =
>>> +			buffer_size - sizeof(struct image_header);
>>> +		fpga_loadfs->remaining = 0;
>>> +	}
>>> +
>>> +	/* Loading mkimage header and RBFinto buffer */
>>> +	ret = request_firmware_into_buf(plat,
>>> +					fpga_loadfs->fpga_fsinfo-
>>>> filename,
>>> +					buffer_p,
>>> +					buffer_size,
>>> +					fpga_loadfs->offset,
>>> +					&fw);
>> This looks like some unwound loop , since the
>> request_firmware_into_buf() is called twice. This probably works for
>> the
>> 300 kiB core RBF, but doesn't work for the 16 MiB full RBF. Also,
>> for()
>> cycle should be used somehow.
> This function is mainly for checking the mkimage header, searching the
> location of the bitstream image from the 1st chunk of reading data and
> updating the read location.
> The remaining of the bitstream data after 1st chunk would be processed
> by the function called subsequent_loading_rbf_to_buffer().

I wonder if this can be somehow optimized, so it's not such a long
function with repeated pieces of code.

>>>
>>> +	if (ret < 0) {
>>> +		debug("FPGA: Failed to read RBF mkimage header and
>>> RBF from ");
>>> +		debug("flash.\n");
>>> +		return -ENOENT;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Getting pointer of RBF starting address where it's
>>> +	 * right after mkimage header
>>> +	 */
>>> +	buffer_p_after_header =
>>> +		(u32 *)((u_char *)buffer_p + sizeof(struct
>>> image_header));
>>> +
>>> +	/* Update next reading RBF offset */
>>> +	fpga_loadfs->offset += buffer_size;
>>> +
>>> +	/* Getting info about RBF types */
>>> +	get_rbf_image_info(&fpga_loadfs->rbfinfo, (u16
>>> *)buffer_p_after_header);
>>> +
>>> +	/*
>>> +	 * Update the starting addr of RBF to init FPGA &
>>> programming RBF
>>> +	 * into FPGA
>>> +	 */
>>> +	*buffer = (u32)buffer_p_after_header;
>>> +
>>> +	/* Update the size of RBF to be programmed into FPGA */
>>> +	*buffer_bsize = buffersz_after_header;
>>> +
>>> +#ifdef CONFIG_CHECK_FPGA_DATA_CRC
>>> +	fpga_loadfs->datacrc = crc32(fpga_loadfs->datacrc,
>>> +					(u_char
>>> *)buffer_p_after_header,
>>> +					buffersz_after_header);
>> Why is this not ON by default ?
> It is off for the sake of performance.

Do you have some hard numbers to support this claim ?

>>
>>>
>>> +#endif
>>> +
>>> +if (fpga_loadfs->remaining == 0) {
>>> +#ifdef CONFIG_CHECK_FPGA_DATA_CRC
>>> +	if (fpga_loadfs->datacrc != image_get_dcrc(&(fpga_loadfs-
>>>> header))) {
>>> +		debug("FPGA: Bad Data Checksum.\n");
>>> +		return -EPERM;
>>> +	}
>>> +#endif
>>> +}
>>> +	return 0;
>>> +}
>> [...]


-- 
Best regards,
Marek Vasut


More information about the U-Boot mailing list