[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
Mon Nov 26 10:09:08 UTC 2018
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 <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.
> 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;
> > > > +}
> > > [...]
>
More information about the U-Boot
mailing list