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.c...@intel.com wrote: >>> >>> From: Tien Fong Chee <tien.fong.c...@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.c...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot