[U-Boot] [PATCH v8 4/8] ARM: socfpga: Add FPGA drivers for Arria 10 FPGA bitstream loading

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Thu, 2019-02-14 at 11:41 +0100, Marek Vasut wrote:
> On 2/14/19 7:44 AM, Chee, Tien Fong wrote:
> > 
> > On Wed, 2019-02-13 at 17:20 +0100, Marek Vasut wrote:
> > > 
> > > On 2/13/19 3:18 PM, 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>
> > > > 
> > > > ---
> > > > 
> > > > changes for v8
> > > > - Added codes to discern bitstream type based on fpga node
> > > > name.
> > > > 
> > > > changes for v7
> > > > - Restructure the FPGA driver to support both peripheral
> > > > bitstream
> > > > and core
> > > >   bitstream bundled into FIT image.
> > > > - Support loadable property for core bitstream. User can set
> > > > loadable
> > > >   in DDR for better performance. This loading would be done in
> > > > one
> > > > large
> > > >   chunk instead of chunk by chunk loading with small memory
> > > > buffer.
> > > > ---
> > > >  arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts       |  17 +
> > > >  .../include/mach/fpga_manager_arria10.h            |  39 +-
> > > >  drivers/fpga/socfpga_arria10.c                     | 467
> > > > ++++++++++++++++++++-
> > > >  3 files changed, 500 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> > > > b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> > > > index 998d811..14f1967 100644
> > > > --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> > > > +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> > > > @@ -18,6 +18,23 @@
> > > >  /dts-v1/;
> > > >  #include "socfpga_arria10_socdk.dtsi"
> > > >  
> > > > +/ {
> > > > +	chosen {
> > > > +		firmware-loader = &fs_loader0;
> > > Should be a phandle.
> > Can we change this label to phandle stage by stage, may be after
> > fpga
> > driver? This requires time working on firmware loader.
> We should fix this as soon as possible, otherwise people might find
> this
> bad example and wonder why it doesn't work once this is changed.
Okay.
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +	};
> > > > +
> > > > +	fs_loader0: fs-loader at 0 {
> > > > +		u-boot,dm-pre-reloc;
> > > > +		compatible = "u-boot,fs-loader";
> > > > +		phandlepart = <&mmc 1>;
> > > > +	};
> > > > +};
> > > > +
> > > > +&fpga_mgr {
> > > > +	u-boot,dm-pre-reloc;
> > > > +	altr,bitstream = "fit_spl_fpga.itb";
> > > > +};
> > > > +
> > > >  &mmc {
> > > >  	u-boot,dm-pre-reloc;
> > > >  	status = "okay";
> [...]
> 
> > 
> > > 
> > > > 
> > > > - * FPGA Manager to program the FPGA. This is the interface
> > > > used by
> > > > FPGA driver.
> > > > - * Return 0 for sucess, non-zero for error.
> > > > - */
> > > > +char *get_fpga_filename(const void *fdt, int *len)
> > > > +{
> > > > +	char *fpga_filename = NULL;
> > > > +	int node_offset;
> > > > +
> > > > +	fdtdec_find_aliases_for_id(gd->fdt_blob, "fpga_mgr",
> > > > +				COMPAT_ALTERA_SOCFPGA_FPGA0,
> > > > +				&node_offset, 1);
> > > > +
> > > > +	ofnode fpgamgr_node = offset_to_ofnode(node_offset);
> > > > +
> > > > +	if (ofnode_valid(fpgamgr_node))
> > > > +		fpga_filename = (char
> > > > *)ofnode_read_string(fpgamgr_node,
> > > > +						"altr,bitstrea
> > > > m");
> > > > +
> > > > +
> > > Why is the cast needed ?
> > The return string would be eventually set to the char *filename in
> > common struct fpga_fsinfo. So, the cast here is to avoid the
> > warning
> > from compiler.
> I presume if the compiler generates a warning, it's for a reason.
> What
> warning is that ?
> 
> > 
> > > 
> > >  Drop the two newlines.
> > Okay.
> > > 
> > > 
> > > > 
> > > > 
> > > > +	return fpga_filename;
> > > > +}
> > > > +
> > > > +static 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) ==
> > > > FPGA_SOCFPGA_A10_RBF_UNENCRYPTED) {
> > > > +			rbf->security = unencrypted;
> > > > +		} else if (*(buffer + i) ==
> > > > FPGA_SOCFPGA_A10_RBF_ENCRYPTED) {
> > > > +			rbf->security = encrypted;
> > > > +		} else if (*(buffer + i + 1) ==
> > > > +				FPGA_SOCFPGA_A10_RBF_UNENCRYPT
> > > > ED)
> > > > {
> > > > +			rbf->security = unencrypted;
> > > > +		} else if (*(buffer + i + 1) ==
> > > > +				FPGA_SOCFPGA_A10_RBF_ENCRYPTED
> > > > ) {
> > > > +			rbf->security = encrypted;
> > > > +		} else {
> > > > +			rbf->security = invalid;
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		/* PERIPH RBF(buffer + i + 1), CORE RBF(buffer
> > > > + i
> > > > + 2) */
> > > > +		if (*(buffer + i + 1) ==
> > > > FPGA_SOCFPGA_A10_RBF_PERIPH) {
> > > > +			rbf->section = periph_section;
> > > > +			break;
> > > > +		} else if (*(buffer + i + 1) ==
> > > > FPGA_SOCFPGA_A10_RBF_CORE) {
> > > > +			rbf->section = core_section;
> > > > +			break;
> > > > +		} else if (*(buffer + i + 2) ==
> > > > FPGA_SOCFPGA_A10_RBF_PERIPH) {
> > > > +			rbf->section = periph_section;
> > > > +			break;
> > > > +		} else if (*(buffer + i + 2) ==
> > > > FPGA_SOCFPGA_A10_RBF_CORE) {
> > > > +			rbf->section = core_section;
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		rbf->section = unknown;
> > > > +		break;
> > > > +
> > > > +		WATCHDOG_RESET();
> > > > +	}
> > > > +}
> > > > +
> > > > +#ifdef CONFIG_FS_LOADER
> > > > +static int first_loading_rbf_to_buffer(struct udevice *dev,
> > > > +				struct fpga_loadfs_info
> > > > *fpga_loadfs,
> > > > +				u32 *buffer, size_t
> > > > *buffer_bsize)
> > > > +{
> > > > +	u32 *buffer_p = (u32 *)*buffer;
> > > > +	u32 *loadable = buffer_p;
> > > > +	size_t buffer_size = *buffer_bsize;
> > > > +	size_t fit_size;
> > > > +	int ret, i, count;
> > > > +	int confs_noffset, images_noffset;
> > > > +	int rbf_offset;
> > > > +	int rbf_size;
> > > > +	const char *fpga_node_name = NULL;
> > > > +	const char *uname = NULL;
> > > > +
> > > > +	/* Load image header into buffer */
> > > > +	ret = request_firmware_into_buf(dev,
> > > > +					fpga_loadfs-
> > > > >fpga_fsinfo-
> > > > > 
> > > > > filename,
> > > > +					buffer_p,
> > > > +					sizeof(struct
> > > > image_header),
> > > > +					0);
> > > > +	if (ret < 0) {
> > > > +		debug("FPGA: Failed to read image header from
> > > > flash.\n");
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	if (image_get_magic((struct image_header *)buffer_p)
> > > > !=
> > > > FDT_MAGIC) {
> > > > +		debug("FPGA: No FDT magic was found.\n");
> > > > +		return -EBADF;
> > > > +	}
> > > > +
> > > > +	fit_size = fdt_totalsize(buffer_p);
> > > > +
> > > > +	if (fit_size > buffer_size) {
> > > > +		debug("FPGA: FIT image is larger than
> > > > available
> > > > buffer.\n");
> > > > +		debug("Please use FIT external data or
> > > > increasing
> > > > buffer.\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	/* Load entire FIT into buffer */
> > > > +	ret = request_firmware_into_buf(dev,
> > > > +					fpga_loadfs-
> > > > >fpga_fsinfo-
> > > > > 
> > > > > filename,
> > > > +					buffer_p,
> > > > +					fit_size,
> > > > +					0);
> > > > +
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = fit_check_format(buffer_p);
> > > > +	if (!ret) {
> > > > +		debug("FPGA: No valid FIT image was
> > > > found.\n");
> > > > +		return -EBADF;
> > > > +	}
> > > > +
> > > > +	confs_noffset = fdt_path_offset(buffer_p,
> > > > FIT_CONFS_PATH);
> > > > +	images_noffset = fdt_path_offset(buffer_p,
> > > > FIT_IMAGES_PATH);
> > > > +	if (confs_noffset < 0 || images_noffset < 0) {
> > > > +		debug("FPGA: No Configurations or images nodes
> > > > were found.\n");
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	/* Get default configuration unit name from default
> > > > property */
> > > > +	confs_noffset = fit_conf_get_node(buffer_p, NULL);
> > > > +	if (confs_noffset < 0) {
> > > > +		debug("FPGA: No default configuration was
> > > > found in
> > > > config.\n");
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	count = fit_conf_get_prop_node_count(buffer_p,
> > > > confs_noffset,
> > > > +					    FIT_FPGA_PROP);
> > > > +
> > > > +	if (count < 0) {
> > > > +		debug("FPGA: Invalid configuration format for
> > > > FPGA
> > > > node.\n");
> > > > +		return count;
> > > > +	} else {
> > > > +		debug("FPGA: FPGA node count: %d\n", count);
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < count; i++) {
> > > > +		images_noffset =
> > > > fit_conf_get_prop_node_index(buffer_p,
> > > > +							     c
> > > > onfs
> > > > _noffset,
> > > > +							     F
> > > > IT_F
> > > > PGA_PROP, i);
> > > > +		uname = fit_get_name(buffer_p, images_noffset,
> > > > NULL);
> > > > +		if (uname) {
> > > > +			debug("FPGA: %s\n", uname);
> > > > +
> > > > +			if (strstr(uname, "fpga-periph") &&
> > > > +				(!is_fpgamgr_early_user_mode()
> > > > ||
> > > > +				is_fpgamgr_user_mode())) {
> > > > +				fpga_node_name = uname;
> > > > +				printf("FPGA: Start to program
> > > > ");
> > > > +				printf("peripheral/full
> > > > bitstream
> > > > ...\n");
> > > > +				break;
> > > > +			} else if (strstr(uname, "fpga-core")
> > > > &&
> > > > +					(is_fpgamgr_early_user
> > > > _mod
> > > > e() &&
> > > > +					!is_fpgamgr_user_mode(
> > > > )))
> > > > {
> > > > +				fpga_node_name = uname;
> > > > +				printf("FPGA: Start to program
> > > > core ");
> > > > +				printf("bitstream ...\n");
> > > > +				break;
> > > > +			}
> > > > +		}
> > > > +		WATCHDOG_RESET();
> > > > +	}
> > > > +
> > > > +	if (!fpga_node_name) {
> > > > +		debug("FPGA: No suitable bitstream was found,
> > > > count: %d.\n", i);
> > > > +		return 1;
> > > > +	}
> > > > +
> > > > +	images_noffset = fit_image_get_node(buffer_p,
> > > > fpga_node_name);
> > > > +	if (images_noffset < 0) {
> > > > +		debug("FPGA: No node '%s' was found in
> > > > FIT.\n",
> > > > +		     fpga_node_name);
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	ret = fit_image_get_data_position(buffer_p,
> > > > images_noffset,
> > > > +					 &rbf_offset);
> > > > +	if (ret < 0) {
> > > > +		debug("FPGA: No data position was found
> > > > (err=%d).\n", ret);
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	ret = fit_image_get_data_size(buffer_p,
> > > > images_noffset,
> > > > &rbf_size);
> > > > +	if (ret < 0) {
> > > > +		debug("FPGA: No data size was found
> > > > (err=%d).\n",
> > > > ret);
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	ret = fit_image_get_load(buffer_p, images_noffset,
> > > > (ulong
> > > > *)loadable);
> > > > +	if (ret < 0) {
> > > > +		debug("FPGA: No loadable was found
> > > > (err=%d).\n",
> > > > ret);
> > > > +		debug("FPGA: Using default buffer and
> > > > size.\n");
> > > > +	} else {
> > > > +		buffer_p = (u32 *)*loadable;
> > > > +		buffer_size = rbf_size;
> > > > +		debug("FPGA: Found loadable address =
> > > > 0x%x.\n",
> > > > *loadable);
> > > > +	}
> > > > +
> > > > +	debug("FPGA: External data: offset = 0x%x, size =
> > > > 0x%x.\n",
> > > > +	      rbf_offset, rbf_size);
> > > > +
> > > > +	fpga_loadfs->remaining = rbf_size;
> > > > +
> > > > +	/*
> > > > +	 * Determine buffer size vs bitstream size, and
> > > > calculating number of
> > > > +	 * chunk by chunk transfer is required due to smaller
> > > > buffer size
> > > > +	 * compare to bitstream
> > > > +	 */
> > > > +	if (rbf_size <= buffer_size) {
> > > > +		/* Loading whole bitstream into buffer */
> > > > +		buffer_size = rbf_size;
> > > > +		fpga_loadfs->remaining = 0;
> > > > +	} else {
> > > > +		fpga_loadfs->remaining -= buffer_size;
> > > > +	}
> > > Shouldn't all this parsing and calculation be done by the
> > > firmware
> > > loader code ?
> > The calculation here is to determine the available memory size can
> > be
> > used, it could be size from OCRAM buffer, or DDR.
> It seems rather that the code is parsing the fitImage structures ?
> How is that related to such calculations ?
The driver checking the load property of core image, if there is a
destination address is defined, driver would use DDR memory instead of
OCRAM buffer, hence driver need to update both buffer size and
remaining when DDR is used, so that whole bitstream can be loaded to
DDR instead of chunk by chunk loading.
> 
> > 
> > > 
> > > > 
> > > > 
> > > > +	fpga_loadfs->offset = rbf_offset;
> > > > +	/* Loading bitstream into buffer */
> > > > +	ret = request_firmware_into_buf(dev,
> > > > +					fpga_loadfs-
> > > > >fpga_fsinfo-
> > > > > 
> > > > > filename,
> > > > +					buffer_p,
> > > > +					buffer_size,
> > > > +					fpga_loadfs->offset);
> > > > +	if (ret < 0) {
> > > > +		debug("FPGA: Failed to read bitstream from
> > > > flash.\n");
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	/* Getting info about bitstream types */
> > > > +	get_rbf_image_info(&fpga_loadfs->rbfinfo, (u16
> > > > *)buffer_p);
> > > > +
> > > > +	/* Update next reading bitstream offset */
> > > > +	fpga_loadfs->offset += buffer_size;
> > > > +
> > > > +	/* Update the final addr for bitstream */
> > > > +	*buffer = (u32)buffer_p;
> > > > +
> > > > +	/* Update the size of bitstream to be programmed into
> > > > FPGA
> > > > */
> > > > +	*buffer_bsize = buffer_size;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int subsequent_loading_rbf_to_buffer(struct udevice
> > > > *dev,
> > > > +					struct
> > > > fpga_loadfs_info
> > > > *fpga_loadfs,
> > > > +					u32 *buffer, size_t
> > > > *buffer_bsize)
> > > > +{
> > > > +	int ret = 0;
> > > > +	u32 *buffer_p = (u32 *)*buffer;
> > > > +
> > > > +	/* Read the bitstream chunk by chunk. */
> > > > +	if (fpga_loadfs->remaining > *buffer_bsize) {
> > > > +		fpga_loadfs->remaining -= *buffer_bsize;
> > > > +	} else {
> > > > +		*buffer_bsize = fpga_loadfs->remaining;
> > > > +		fpga_loadfs->remaining = 0;
> > > > +	}
> > > > +
> > > > +	ret = request_firmware_into_buf(dev,
> > > > +					fpga_loadfs-
> > > > >fpga_fsinfo-
> > > > > 
> > > > > filename,
> > > > +					buffer_p,
> > > > +					*buffer_bsize,
> > > > +					fpga_loadfs->offset);
> > > > +	if (ret < 0) {
> > > > +		debug("FPGA: Failed to read bitstream from
> > > > flash.\n");
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	/* Update next reading bitstream offset */
> > > > +	fpga_loadfs->offset += *buffer_bsize;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int socfpga_loadfs(fpga_fs_info *fpga_fsinfo, const void *buf,
> > > > size_t bsize,
> > > > +			u32 offset)
> > > > +{
> > > > +	struct fpga_loadfs_info fpga_loadfs;
> > > > +	int status = 0;
> > > > +	int ret = 0;
> > > > +	u32 buffer = (u32)buf;
> > > This will fail on arm64 , look at uintptr_t .
> > This driver is only used by A10 which is arm32. You want me to use
> > (u32)(uintptr_t)buf?
> If you want to cast pointer to integer type, yes, that's uintptr_t .
Okay.
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +	size_t buffer_sizebytes = bsize;
> > > > +	size_t buffer_sizebytes_ori = bsize;
> > > > +	size_t total_sizeof_image = 0;
> > > > +	struct udevice *dev;
> > > > +
> > > > +	ret = uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0,
> > > > &dev);
> > > Shouldn't the firmware loaded instance be obtained via the DT
> > > phandle
> > > ?
> > It just to get the device activated. The firmware loaded itself
> > would
> > go to choosen node(default) for getting the label. I can change to
> > the
> > phandle, may be after this?
> Except this always activates the first device in the list.
How about this after i change the label to phandle?

chosen_node = ofnode_path("/chosen");
phandle_p = ofnode_get_property(chosen_node, "firmware-loader", &size);
phandle = fdt32_to_cpu(*phandle_p);
ret = uclass_get_device_by_phandle_id(UCLASS_FS_FIRMWARE_LOADER,
					     phandle, &dev);
> [...]