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