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
- */
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(structimage_header),
fpga_loadfs->offset,&fw);- if (ret < 0) {
debug("FPGA: Failed to read RBF mkimage headerfrom 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(structimage_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(structimage_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 headerand 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(structimage_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;
+}
[...]