[U-Boot] [PATCH 2/9] ARM: socfpga: Add FPGA drivers for Arria 10 FPGA bitstream loading

[Marek Vasut <marex@denx.de>]

On 11/21/2018 11:41 AM, tien.fong.chee at 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;
> +}

[...]
-- 
Best regards,
Marek Vasut