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@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@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,bitstream");
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_UNENCRYPTED) {
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
- */
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(structimage_header),
0);- if (ret < 0) {
debug("FPGA: Failed to read image header fromflash.\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 thanavailable buffer.\n");
debug("Please use FIT external data orincreasing 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 wasfound.\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 nodeswere 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 wasfound 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 forFPGA 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,
confs _noffset,
FIT_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/fullbitstream ...\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 programcore ");
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 inFIT.\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 andsize.\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 fromflash.\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,
structfpga_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 fromflash.\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);
[...]