Re: [U-Boot] [PATCH 1/3] ARM: socfpga: Add FPGA drivers for Arria 10 FPGA loadfs

On 26.7.2018 09:54, tien.fong.chee@intel.com wrote:

From: Tien Fong Chee tien.fong.chee@intel.com

Add FPGA drivers to support FPGA loadfs to program FPGA. The drivers are designed based on generic firmware loader framework, specific firmware loader handling is defined in fpga_manager_arria10.c. These drivers can handle FPGA program operation from loading RBF image in flash to memory and then to program FPGA.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

.../include/mach/fpga_manager_arria10.h | 22 + drivers/fpga/socfpga_arria10.c | 402 ++++++++++++++++++++ include/altera.h | 4 + 3 files changed, 428 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h index a112453..743d6b7 100644 --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h @@ -7,6 +7,9 @@ #ifndef _FPGA_MANAGER_ARRIA10_H_ #define _FPGA_MANAGER_ARRIA10_H_

+#include <asm/cache.h> +#include <fs_loader.h>

#define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK BIT(0) #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK BIT(1) #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK BIT(2) @@ -88,6 +91,25 @@ struct socfpga_fpga_manager { u32 imgcfg_fifo_status; };

+#if defined(CONFIG_CMD_FPGA_LOADFS) +enum rbf_type {unknown, periph_section, core_section}; +enum rbf_security {invalid, unencrypted, encrypted};

+struct rbf_info {

• enum rbf_type section;
• enum rbf_security security;

+};

+struct fpga_info {

• char *filename;
• int fstype;
• u32 remaining;
• u32 offset;
• struct rbf_info rbfinfo;
• struct image_header header;

+}; +#endif

/* Functions */ int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size); int fpgamgr_program_finish(void); diff --git a/drivers/fpga/socfpga_arria10.c b/drivers/fpga/socfpga_arria10.c index c0252fb..2f9006f 100644 --- a/drivers/fpga/socfpga_arria10.c +++ b/drivers/fpga/socfpga_arria10.c @@ -12,6 +12,13 @@ #include <altera.h> #include <common.h> #include <errno.h> +#include <fat.h> +#include <fs.h> +#include <fdtdec.h> +#include <malloc.h> +#include <part.h> +#include <spl.h> +#include <dm.h> #include <wait_bit.h> #include <watchdog.h>

@@ -21,6 +28,12 @@ #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 +#define PERIPH_RBF 0 +#define CORE_RBF 1

static const struct socfpga_fpga_manager *fpga_manager_base = (void *)SOCFPGA_FPGAMGRREGS_ADDRESS; @@ -30,6 +43,8 @@ static const struct socfpga_system_manager *system_manager_base =

static void fpgamgr_set_cd_ratio(unsigned long ratio);

+struct firmware *fw = NULL;

static uint32_t fpgamgr_get_msel(void) { u32 reg; @@ -471,3 +486,390 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size)

return fpgamgr_program_finish(); }

+#if defined(CONFIG_CMD_FPGA_LOADFS) +const char *get_fpga_filename(const void *fdt, int *len, u32 core) +{

• const char *fpga_filename = NULL;
• const char *cell;
• int nodeoffset;
• 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 in periph.rbf
  

• * -> 2nd dword in core.rbf
  

• */
  

• u32 word_reading_max = 2;
• u32 i;
• for (i = 0; i < word_reading_max; i++) {

• if (*(buffer + i) == RBF_UNENCRYPTED) { /* PERIPH RBF */
  

• 	rbf->security = unencrypted;
  

• }
  

• else if (*(buffer + i) == RBF_ENCRYPTED) {
  

• 	rbf->security = encrypted;
  

• }
  

• else if (*(buffer + i + 1) == RBF_UNENCRYPTED) { /* CORE RBF */
  

• 	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;
  

• } else {
  

• 	rbf->section = unknown;
  

• 	break;
  

• }
  

• }
• return;

+}

+int fs_flash_preinit(struct device_platdata *plat,

•      struct fpga_info *fpgainfo,
  

•      u32 *buffer, u32 *buffer_sizebytes)
  

+{

• u32 *bufferptr_after_header = NULL;
• u32 buffersize_after_header = 0;
• u32 rbf_header_data_size = 0;
• int ret = 0;
• fpgainfo->offset = 0;
• /* To avoid from keeping re-read the contents */
• struct image_header *header = &(fpgainfo->header);
• size_t buffer_size = *buffer_sizebytes;
• u32 *buffer_ptr = (u32 *)*buffer;
• /* Load mkimage header into buffer */
• ret = request_firmware_into_buf(plat,

• 		       fpgainfo->filename,
  

• 		       buffer_ptr,
  

• 		       sizeof(struct image_header),
  

• 		       fpgainfo->offset,
  

• 		       &fw);
  

• if (ret < 0) {

• puts("Failed to read mkimage header from flash.\n");
  

• return -ENOENT;
  

• }

This is suggesting that you are working with legacy uimage not with file itself. It means this is mix between legacy fpga loadmk and fpga loadfs.

• WATCHDOG_RESET();
• memcpy(header, (u_char *)buffer_ptr, sizeof(*header));

any reason not to copy that header directly to your structure?

• if (!image_check_magic(header)) {

• puts("FPGA: Bad Magic Number.\n");
  

• return -EBADF;
  

• }
• if (!image_check_hcrc(header)) {

• puts("FPGA: Bad Header Checksum.\n");
  

• return -EPERM;
  

• }
• /* Getting rbf data size */
• fpgainfo->remaining =

• image_get_data_size(header);
  

• /* Calculate total size of both rbf data with mkimage header */
• rbf_header_data_size = fpgainfo->remaining +

• 		sizeof(struct image_header);
  

• /* Loading to buffer chunk by chunk, normally for OCRAM buffer */
• if (rbf_header_data_size > buffer_size) {

• /* Calculate size of rbf data in the buffer */
  

• buffersize_after_header =
  

• 	buffer_size - sizeof(struct image_header);
  

• fpgainfo->remaining -= buffersize_after_header;
  

• } else {
• /* Loading whole rbf image into buffer, normally for DDR buffer */

• buffer_size = rbf_header_data_size;
  

• /* Calculate size of rbf data in the buffer */
  

• buffersize_after_header =
  

• 	buffer_size - sizeof(struct image_header);
  

• fpgainfo->remaining = 0;
  

• }
• /* Loading mkimage header and rbf data into buffer */
• ret = request_firmware_into_buf(plat,

• 		        fpgainfo->filename,
  

• 		        buffer_ptr,
  

• 		        buffer_size,
  

• 		        fpgainfo->offset,
  

• 			&fw);
  

• if (ret < 0) {

• puts(" Failed to read mkimage header and rbf data ");
  

• puts("from flash.\n");
  

• return -ENOENT;
  

• }
• /*

• * Getting pointer of rbf data starting address where is it
  

• * right after mkimage header
  

• */
  

• bufferptr_after_header =

• (u32 *)((u_char *)buffer_ptr + sizeof(struct image_header));
  

• /* Update next reading rbf data flash offset */
• fpgainfo->offset += buffer_size;
• /*

• * Update the starting addr of rbf data to init FPGA & programming
  

• * into FPGA
  

• */
  

• *buffer = (u32)bufferptr_after_header;
• get_rbf_image_info(&fpgainfo->rbfinfo, (u16 *)bufferptr_after_header);
• /* Update the size of rbf data to be programmed into FPGA */
• *buffer_sizebytes = buffersize_after_header;

+#ifdef CONFIG_CHECK_FPGA_DATA_CRC

• fpgainfo->datacrc =

• crc32(fpgainfo->datacrc,
  

• (u_char *)bufferptr_after_header,
  

• buffersize_after_header);
  

+#endif

+if (fpgainfo->remaining == 0) { +#ifdef CONFIG_CHECK_FPGA_DATA_CRC

• if (fpgainfo->datacrc !=

• image_get_dcrc(&(fpgainfo->header))) {
  

• puts("FPGA: Bad Data Checksum.\n");
  

• return -EPERM;
  

• }

+#endif +}

• return 0;

+}

+int fs_flash_read(struct device_platdata *plat,

•   struct fpga_info *fpgainfo, u32 *buffer,
  

•   u32 *buffer_sizebytes)
  

+{

• int ret = 0;
• /* To avoid from keeping re-read the contents */
• size_t buffer_size = *buffer_sizebytes;
• u32 *buffer_ptr = (u32 *)*buffer;
• u32 flash_addr = fpgainfo->offset;
• /* Buffer allocated in OCRAM */
• /* Read the data by small chunk by chunk. */
• if (fpgainfo->remaining > buffer_size) {

• fpgainfo->remaining -= buffer_size;
  

• }
• else {

• /*
  

•  * Buffer allocated in DDR, larger than rbf data most
  

•  * of the time
  

•  */
  

• buffer_size = fpgainfo->remaining;
  

• fpgainfo->remaining = 0;
  

• }
• ret = request_firmware_into_buf(plat,

• 		        fpgainfo->filename,
  

• 		        buffer_ptr,
  

• 		        buffer_size,
  

• 		        fpgainfo->offset,
  

• 			&fw);
  

• if (ret < 0) {

• puts(" Failed to read rbf data from flash.\n");
  

• return -ENOENT;
  

• }

+#ifdef CONFIG_CHECK_FPGA_DATA_CRC

• fpgainfo->datacrc =

• crc32(fpgainfo->datacrc,
  

• 	(unsigned char *)buffer_ptr, buffer_size);
  

+#endif

+if (fpgainfo->remaining == 0) { +#ifdef CONFIG_CHECK_FPGA_DATA_CRC

• if (fpgainfo->datacrc !=

• image_get_dcrc(&(fpgainfo->header))) {
  

• puts("FPGA: Bad Data Checksum.\n");
  

• return -EPERM;
  

• }

+#endif +}

• /* Update next reading rbf data flash offset */
• flash_addr += buffer_size;
• fpgainfo->offset = flash_addr;
• /* Update the size of rbf data to be programmed into FPGA */
• *buffer_sizebytes = buffer_size;
• return 0;

+}

+int socfpga_loadfs(Altera_desc *desc, const void *buf, size_t bsize,

•    fpga_fs_info *fpga_fsinfo)
  

+{

• struct fpga_info fpgainfo;
• u32 status = 0;
• int ret = 0;
• int len = 0;
• u32 buffer = 0;
• u32 buffer_ori = 0;
• size_t buffer_sizebytes = 0;
• size_t buffer_sizebytes_ori = 0;
• buffer_sizebytes = buffer_sizebytes_ori = bsize;
• buffer = buffer_ori = (u32) buf;
• struct udevice *dev;
• struct device_platdata *plat;
• char *filename = NULL;
• ret = uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &dev);
• if (ret)

• return ret;
  

• plat = dev->platdata;

+/*

• if (strcmp("default", fpga_fsinfo->interface))

• env_set("storage_interface", fpga_fsinfo->interface);
  

• if (strcmp("default", fpga_fsinfo->dev_part))

• env_set("fw_dev_part", fpga_fsinfo->dev_part);
  

+*/

no dead code please.

• if (!strcmp("default", fpga_fsinfo->filename)) {

• filename = env_get("CORE_RBF");
  

• if (filename)
  

• 	fpga_fsinfo->filename = filename;
  

• else {
  

• 	fpga_fsinfo->filename =
  

• 		(char *)get_fpga_filename(gd->fdt_blob,
  

• 					  &len,
  

• 					  CORE_RBF);
  

• }
  

• }
• memset(&fpgainfo, 0, sizeof(fpgainfo));
• fpgainfo.filename = fpga_fsinfo->filename;
• fpgainfo.fstype = fpga_fsinfo->fstype;

This is IMHO not the right way how this should be handled. fpga loadfs command requires this to be passed. If you want to use some default variables I am fine with that but then use generic name and add them to cmd/fpga.c (on the top of my series now) which will be common for all.

• WATCHDOG_RESET();
• /*

• * Note: Both buffer and buffer_sizebytes values can be altered by
  

• * function below.
  

• */
  

• ret = fs_flash_preinit(plat, &fpgainfo, &buffer, &buffer_sizebytes);
• if (ret) {

• release_firmware(fw);
  

• fw = NULL;
  

• return ret;
  

• }
• if (fpgainfo.rbfinfo.section == periph_section) {

• /* Initialize the FPGA Manager */
  

• status = fpgamgr_program_init((u32 *)buffer, buffer_sizebytes);
  

• if (status) {
  

• 	puts("FPGA: Init with periph rbf failed.\n");
  

• 	release_firmware(fw);
  

• 	fw = NULL;
  

• 	return -EPERM;
  

• }
  

• }
• WATCHDOG_RESET();
• /* Transfer data to FPGA Manager */
• fpgamgr_program_write((void *)buffer,

• buffer_sizebytes);
  

• WATCHDOG_RESET();
• while (fpgainfo.remaining) {

• ret = fs_flash_read(plat, &fpgainfo, &buffer_ori,
  

• 	&buffer_sizebytes_ori);
  

• if (ret) {
  

• 	release_firmware(fw);
  

• 	fw = NULL;
  

• 	return ret;
  

• }
  

• /* transfer data to FPGA Manager */
  

• fpgamgr_program_write((void *)buffer_ori,
  

• 	buffer_sizebytes_ori);
  

• WATCHDOG_RESET();
  

• }
• if (fpgainfo.rbfinfo.section == periph_section) {

• if (fpgamgr_wait_early_user_mode() != -ETIMEDOUT) {
  

• 	puts("FPGA: Early Release Succeeded.\n");
  

• }
  

• else {
  

• 	puts("FPGA: Failed to see Early Release.\n");
  

• 	release_firmware(fw);
  

• 	fw = NULL;
  

• 	return -EIO;
  

• }
  

• } else if (fpgainfo.rbfinfo.section == core_section) {

• /* Ensure the FPGA entering config done */
  

• status = fpgamgr_program_finish();
  

• if (status) {
  

• 	release_firmware(fw);
  

• 	fw = NULL;
  

• 	return status;
  

• }
  

• else {
  

• 	puts("FPGA: Enter user mode.\n");
  

• }
  

• } else {

• puts("Config Error: Unsupported FGPA raw binary type.\n");
  

• release_firmware(fw);
  

• fw = NULL;
  

• return -ENOEXEC;
  

• }
• release_firmware(fw);
• fw = NULL;
• return 0;

+} +#endif diff --git a/include/altera.h b/include/altera.h index ead5d3d..cf0c021 100644 --- a/include/altera.h +++ b/include/altera.h @@ -83,6 +83,10 @@ typedef struct { extern int altera_load(Altera_desc *desc, const void *image, size_t size); extern int altera_dump(Altera_desc *desc, const void *buf, size_t bsize); extern int altera_info(Altera_desc *desc); +#if defined(CONFIG_CMD_FPGA_LOADFS) +int altera_loadfs(Altera_desc *desc, const void *buf, size_t bsize,

• 		  fpga_fs_info *fpga_fsinfo);
  

+#endif

/* Board specific implementation specific function types *********************************************************************/

What I see above doesn't look quite good. First think what I want to know is the flow you want to use. If this is based on legacy uimage. That CRC calculation is interesting too.

One thing what I don't like is that we use this command differently then your implementation which is the thing which needs to be fixed. Right now there is no support for load by chunks mkimage image format directly from filesystem and maybe we should create different command for that. In SPL case there should be clear function which should be called to do that. Because right now your flow is more or less duplicating things which are already in SPL.

Thanks, Michal