| Message ID | 1542796908-7947-3-git-send-email-tien.fong.chee@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | Add support for loading FPGA bitstream | expand |
On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: > From: Tien Fong Chee <tien.fong.chee@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@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. > #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. > +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 > 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 ? > + 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. > 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. > + 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 ? > +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. > + 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 ? > +#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; > +} [...]
On Wed, 2018-11-21 at 15:18 +0100, Marek Vasut wrote: > On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: > > > > From: Tien Fong Chee <tien.fong.chee@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@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. > > > > > + 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? > > > > > 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. > > > > > +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(). > > > > > + 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. > > > > > +#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; > > +} > [...]
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@intel.com wrote: >>> >>> From: Tien Fong Chee <tien.fong.chee@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@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; >>> +} >> [...]
On Fri, 2018-11-23 at 13:28 +0100, Marek Vasut wrote: > 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@intel.com wrote: > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@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@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. Just logical sense, performance sure getting degraded, especially loading full rbf, but may be not that obvious for periph.rbf because of very small size, i can try to measure. If not much difference, i can enable checking in default. > > > > > > > > > > > > > > > > > > + 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 ? What's you means with duplication, they are different thing. How about i change the name to ARRIA10RBF_PERIPH_TYPE and ARRIA10RBF_CORE_TYPE. > > > > > > > > > > > > > > > > > > > > > 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. Noted. Made sense. > > > > > > > > > > > > > > +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. I already try my best to optimize them without compromising consistent implementation for periph rbf, core rbf and full rbf. > > > > > > > > > > > > > > > > > > + 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; > > > > +} > > > [...] >
On 11/26/2018 11:09 AM, Chee, Tien Fong wrote: [...] >>>>> 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. > Just logical sense, performance sure getting degraded, especially > loading full rbf, but may be not that obvious for periph.rbf because of > very small size, i can try to measure. If not much difference, i can > enable checking in default. Hard numbers are the only relevant argument here, please measure. And try it with caches enabled as much as possible, you want users to boot fast. Arria10 is particularly annoyingly slow at booting. >> >>> >>>> >>>>> >>>>> >>>>> + 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 ? > What's you means with duplication, they are different thing. > How about i change the name to ARRIA10RBF_PERIPH_TYPE > and ARRIA10RBF_CORE_TYPE. ARRIA10RBF_PERIPH = (PERIPH_RBF << 15) | 1 same for ... _CORE ... is that a coincidence ? [...]
On Mon, 2018-11-26 at 12:18 +0100, Marek Vasut wrote: > On 11/26/2018 11:09 AM, Chee, Tien Fong wrote: > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > Just logical sense, performance sure getting degraded, especially > > loading full rbf, but may be not that obvious for periph.rbf > > because of > > very small size, i can try to measure. If not much difference, i > > can > > enable checking in default. > Hard numbers are the only relevant argument here, please measure. > And try it with caches enabled as much as possible, you want users to > boot fast. Arria10 is particularly annoyingly slow at booting. sure. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + 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.co > > > > > > m> > > > > > > */ > > > > > > > > > > > > #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 ? > > What's you means with duplication, they are different thing. > > How about i change the name to ARRIA10RBF_PERIPH_TYPE > > and ARRIA10RBF_CORE_TYPE. > ARRIA10RBF_PERIPH = (PERIPH_RBF << 15) | 1 We can't use the flag PERIPH_RBF(similar TRUE/FALSE) for magic content, because they are not related each other. Magic content is defined by HW design. We identify the type of rbf such as periph, and core through this magic content within the rbf. After we getting the type, then only we setting the flag such as PERIPH_RBF to the function. > > same for ... _CORE ... is that a coincidence ? > > [...] >
On 11/27/2018 09:54 AM, Chee, Tien Fong wrote: > On Mon, 2018-11-26 at 12:18 +0100, Marek Vasut wrote: >> On 11/26/2018 11:09 AM, Chee, Tien Fong wrote: >> [...] >>> >>>> >>>>> >>>>>> >>>>>>> >>>>>>> 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. >>> Just logical sense, performance sure getting degraded, especially >>> loading full rbf, but may be not that obvious for periph.rbf >>> because of >>> very small size, i can try to measure. If not much difference, i >>> can >>> enable checking in default. >> Hard numbers are the only relevant argument here, please measure. >> And try it with caches enabled as much as possible, you want users to >> boot fast. Arria10 is particularly annoyingly slow at booting. > sure. >> >>> >>>> >>>> >>>>> >>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> + 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.co >>>>>>> m> >>>>>>> */ >>>>>>> >>>>>>> #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 ? >>> What's you means with duplication, they are different thing. >>> How about i change the name to ARRIA10RBF_PERIPH_TYPE >>> and ARRIA10RBF_CORE_TYPE. >> ARRIA10RBF_PERIPH = (PERIPH_RBF << 15) | 1 > We can't use the flag PERIPH_RBF(similar TRUE/FALSE) for magic content, > because they are not related each other. Magic content is defined by HW > design. You can define the flags to match the HW design, which is probably a good idea ? > We identify the type of rbf such as periph, and core through this magic > content within the rbf. After we getting the type, then only we setting > the flag such as PERIPH_RBF to the function. >> >> same for ... _CORE ... is that a coincidence ? >> >> [...]
On Tue, 2018-11-27 at 13:08 +0100, Marek Vasut wrote: > On 11/27/2018 09:54 AM, Chee, Tien Fong wrote: > > > > On Mon, 2018-11-26 at 12:18 +0100, Marek Vasut wrote: > > > > > > On 11/26/2018 11:09 AM, Chee, Tien Fong wrote: > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > Just logical sense, performance sure getting degraded, > > > > especially > > > > loading full rbf, but may be not that obvious for periph.rbf > > > > because of > > > > very small size, i can try to measure. If not much difference, > > > > i > > > > can > > > > enable checking in default. > > > Hard numbers are the only relevant argument here, please measure. > > > And try it with caches enabled as much as possible, you want > > > users to > > > boot fast. Arria10 is particularly annoyingly slow at booting. > > sure. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + 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.inte > > > > > > > > l.co > > > > > > > > m> > > > > > > > > */ > > > > > > > > > > > > > > > > #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 ? > > > > What's you means with duplication, they are different thing. > > > > How about i change the name to ARRIA10RBF_PERIPH_TYPE > > > > and ARRIA10RBF_CORE_TYPE. > > > ARRIA10RBF_PERIPH = (PERIPH_RBF << 15) | 1 > > We can't use the flag PERIPH_RBF(similar TRUE/FALSE) for magic > > content, > > because they are not related each other. Magic content is defined > > by HW > > design. > You can define the flags to match the HW design, which is probably a > good idea ? I have no strong opinion of this, i can do it. > > > > > We identify the type of rbf such as periph, and core through this > > magic > > content within the rbf. After we getting the type, then only we > > setting > > the flag such as PERIPH_RBF to the function. > > > > > > > > > same for ... _CORE ... is that a coincidence ? > > > > > > [...] >
On 11/28/2018 03:53 PM, Chee, Tien Fong wrote: > On Tue, 2018-11-27 at 13:08 +0100, Marek Vasut wrote: >> On 11/27/2018 09:54 AM, Chee, Tien Fong wrote: >>> >>> On Mon, 2018-11-26 at 12:18 +0100, Marek Vasut wrote: >>>> >>>> On 11/26/2018 11:09 AM, Chee, Tien Fong wrote: >>>> [...] >>>>> >>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> 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. >>>>> Just logical sense, performance sure getting degraded, >>>>> especially >>>>> loading full rbf, but may be not that obvious for periph.rbf >>>>> because of >>>>> very small size, i can try to measure. If not much difference, >>>>> i >>>>> can >>>>> enable checking in default. >>>> Hard numbers are the only relevant argument here, please measure. >>>> And try it with caches enabled as much as possible, you want >>>> users to >>>> boot fast. Arria10 is particularly annoyingly slow at booting. >>> sure. >>>> >>>> >>>>> >>>>> >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> + 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.inte >>>>>>>>> l.co >>>>>>>>> m> >>>>>>>>> */ >>>>>>>>> >>>>>>>>> #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 ? >>>>> What's you means with duplication, they are different thing. >>>>> How about i change the name to ARRIA10RBF_PERIPH_TYPE >>>>> and ARRIA10RBF_CORE_TYPE. >>>> ARRIA10RBF_PERIPH = (PERIPH_RBF << 15) | 1 >>> We can't use the flag PERIPH_RBF(similar TRUE/FALSE) for magic >>> content, >>> because they are not related each other. Magic content is defined >>> by HW >>> design. >> You can define the flags to match the HW design, which is probably a >> good idea ? > I have no strong opinion of this, i can do it. If it can deduplicate things, that'd be good.
diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts index 998d811..f4764d4 100644 --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts @@ -18,6 +18,24 @@ /dts-v1/; #include "socfpga_arria10_socdk.dtsi" +/ { + chosen { + firmware-loader = &fs_loader0; + }; + + fs_loader0: fs-loader@0 { + u-boot,dm-pre-reloc; + compatible = "u-boot,fs-loader"; + phandlepart = <&mmc 1>; + }; +}; + +&fpga_mgr { + u-boot,dm-pre-reloc; + altr,bitstream_periph = "ghrd_10as066n2.periph.rbf.mkimage"; + altr,bitstream_core = "ghrd_10as066n2.core.rbf.mkimage"; +}; + &mmc { u-boot,dm-pre-reloc; status = "okay"; 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 09d13f6..b48bc76 100644 --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h @@ -1,9 +1,12 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* - * Copyright (C) 2017 Intel Corporation <www.intel.com> + * Copyright (C) 2017-2018 Intel Corporation <www.intel.com> * All rights reserved. */ +#include <altera.h> +#include <image.h> + #ifndef _FPGA_MANAGER_ARRIA10_H_ #define _FPGA_MANAGER_ARRIA10_H_ @@ -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 #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}; + +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 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 + 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 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) +{ + const char *fpga_filename = NULL; + const char *cell; + int nodeoffset; + + 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; + +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); + 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); +#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; +} + +int subsequent_loading_rbf_to_buffer(struct device_platdata *plat, + struct fpga_loadfs_info *fpga_loadfs, + u32 *buffer, u32 *buffer_bsize) +{ + int ret = 0; + u32 *buffer_p = (u32 *)*buffer; + + /* Read the RBF chunk by chunk. */ + if (fpga_loadfs->remaining > *buffer_bsize) { + fpga_loadfs->remaining -= *buffer_bsize; + } else { + *buffer_bsize = fpga_loadfs->remaining; + fpga_loadfs->remaining = 0; + } + + ret = request_firmware_into_buf(plat, + fpga_loadfs->fpga_fsinfo->filename, + buffer_p, + *buffer_bsize, + fpga_loadfs->offset, + &fw); + if (ret < 0) { + debug("FPGA: Failed to read RBF from flash.\n"); + return -ENOENT; + } + +#ifdef CONFIG_CHECK_FPGA_DATA_CRC + fpga_loadfs->datacrc = crc32(fpga_loadfs->datacrc, + (unsigned char *)buffer_p, + *buffer_bsize); +#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 +} + + /* Update next reading RBF offset */ + fpga_loadfs->offset += *buffer_bsize; + + return 0; +} + +int socfpga_loadfs(fpga_fs_info *fpga_fsinfo, const void *buf, size_t bsize, + u32 offset) +{ + struct fpga_loadfs_info fpga_loadfs; + int status = 0; + int ret = 0; + u32 buffer = (u32) buf; + u32 buffer_ori = (u32) buf; + size_t buffer_sizebytes = bsize; + size_t buffer_sizebytes_ori = bsize; + size_t total_sizeof_mkimage = sizeof(struct image_header); + struct udevice *dev; + + ret = uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &dev); + if (ret) + return ret; + + memset(&fpga_loadfs, 0, sizeof(fpga_loadfs)); + + WATCHDOG_RESET(); + + fpga_loadfs.fpga_fsinfo = fpga_fsinfo; + fpga_loadfs.offset = offset; + + printf("Start to program FPGA ...\n"); + + /* + * Note: Both buffer and buffer_sizebytes values can be altered by + * function below. + */ + ret = first_loading_rbf_to_buffer(dev->platdata, &fpga_loadfs, &buffer, + &buffer_sizebytes); + if (ret) { + release_firmware(fw); + fw = NULL; + return ret; + } + + if ((fpga_loadfs.rbfinfo.section == core_section) && + !(is_fpgamgr_early_user_mode() && !is_fpgamgr_user_mode())) { + debug("FPGA : FPGA must be in Early Release mode to program "); + debug("core.\n"); + release_firmware(fw); + fw = NULL; + return 0; + } + + /* Disable all signals from hps peripheral controller to fpga */ + writel(0, &system_manager_base->fpgaintf_en_global); + + /* Disable all axi bridges (hps2fpga, lwhps2fpga & fpga2hps) */ + socfpga_bridges_reset(); + + if (fpga_loadfs.rbfinfo.section == periph_section) { + /* Initialize the FPGA Manager */ + status = fpgamgr_program_init((u32 *)buffer, buffer_sizebytes); + if (status) { + debug("FPGA: Init with periph rbf failed.\n"); + release_firmware(fw); + fw = NULL; + return -EPERM; + } + } + + WATCHDOG_RESET(); + + /* Transfer RBF to FPGA Manager */ + fpgamgr_program_write((void *)buffer, buffer_sizebytes); + + total_sizeof_mkimage += buffer_sizebytes; + + WATCHDOG_RESET(); + + while (fpga_loadfs.remaining) { + ret = subsequent_loading_rbf_to_buffer(dev->platdata, + &fpga_loadfs, + &buffer_ori, + &buffer_sizebytes_ori); + + if (ret) { + release_firmware(fw); + fw = NULL; + return ret; + } + + /* Transfer data to FPGA Manager */ + fpgamgr_program_write((void *)buffer_ori, + buffer_sizebytes_ori); + + total_sizeof_mkimage += buffer_sizebytes_ori; + + WATCHDOG_RESET(); + } + + if (fpga_loadfs.rbfinfo.section == periph_section) { + if (fpgamgr_wait_early_user_mode() != -ETIMEDOUT) { + config_pins(gd->fdt_blob, "shared"); + puts("FPGA: Early Release Succeeded.\n"); + } else { + debug("FPGA: Failed to see Early Release.\n"); + release_firmware(fw); + fw = NULL; + return -EIO; + } + + /* For monolithic bitstream */ + if (is_fpgamgr_user_mode()) { + /* Ensure the FPGA entering config done */ + status = fpgamgr_program_finish(); + if (status) { + release_firmware(fw); + fw = NULL; + return status; + } + + config_pins(gd->fdt_blob, "fpga"); + puts("FPGA: Enter user mode.\n"); + } + } else if (fpga_loadfs.rbfinfo.section == core_section) { + /* Ensure the FPGA entering config done */ + status = fpgamgr_program_finish(); + if (status) { + release_firmware(fw); + fw = NULL; + return status; + } + + config_pins(gd->fdt_blob, "fpga"); + puts("FPGA: Enter user mode.\n"); + } else { + debug("Config Error: Unsupported FGPA RBF type.\n"); + release_firmware(fw); + fw = NULL; + return -ENOEXEC; + } + + release_firmware(fw); + fw = NULL; + + return (int)total_sizeof_mkimage; +} + /* - * FPGA Manager to program the FPGA. This is the interface used by FPGA driver. - * Return 0 for sucess, non-zero for error. + * This function is used to load the core RBF from the OCRAM where the location + * of the image is defined in the load property in the FIT image source file + * (.its) */ int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size) { - int status; + unsigned long status; + struct rbf_info rbfinfo; /* disable all signals from hps peripheral controller to fpga */ writel(0, &system_manager_base->fpgaintf_en_global); @@ -461,13 +847,24 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size) /* disable all axi bridge (hps2fpga, lwhps2fpga & fpga2hps) */ socfpga_bridges_reset(); - /* Initialize the FPGA Manager */ - status = fpgamgr_program_init((u32 *)rbf_data, rbf_size); - if (status) - return status; + /* Getting info about RBF types */ + get_rbf_image_info(&rbfinfo, (u16 *)rbf_data); + + if (rbfinfo.section == periph_section) { + /* Initialize the FPGA Manager */ + status = fpgamgr_program_init((u32 *)rbf_data, rbf_size); + if (status) + return status; + } /* Write the RBF data to FPGA Manager */ fpgamgr_program_write(rbf_data, rbf_size); - return fpgamgr_program_finish(); + status = fpgamgr_program_finish(); + if (status) { + config_pins(gd->fdt_blob, "fpga"); + puts("FPGA: Enter user mode.\n"); + } + + return status; }