From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Mon, 26 Nov 2018 10:09:08 +0000 Subject: [U-Boot] [PATCH 2/9] ARM: socfpga: Add FPGA drivers for Arria 10 FPGA bitstream loading In-Reply-To: <64c191cb-925e-f4c8-df75-efc81ff3d656@denx.de> References: <1542796908-7947-1-git-send-email-tien.fong.chee@intel.com> <1542796908-7947-3-git-send-email-tien.fong.chee@intel.com> <23b93c57-daf8-78e0-684f-2fe006ba8e45@denx.de> <1542966198.10129.20.camel@intel.com> <64c191cb-925e-f4c8-df75-efc81ff3d656@denx.de> Message-ID: <1543226946.10014.11.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 at intel.com wrote: > > > > > > > > > > > > From: Tien Fong Chee > > > > > > > > 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 > > > [...] > > > > > > > > > > > > > > > @@ -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 > > > > + * Copyright (C) 2017-2018 Intel Corporation > > > >   */ > > > >   > > > >  #include > > > > @@ -10,8 +10,10 @@ > > > >  #include > > > >  #include > > > >  #include > > > > +#include > > > >  #include > > > >  #include > > > > +#include > > > >  #include > > > >  #include > > > >   > > > > @@ -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; > > > > +} > > > [...] >