[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[Luis Araneda]

This series implements FPGA bitstream loading from SPL.
Programming the FPGA from the SPL is necessary on some boards,
like the Zybo, because the FPGA fabric routes the I2C bus to an
EEPROM for reading the Ethernet MAC address.

The bitstream is loaded from a FIT image into a dynamically-allocated
memory before programming the FPGA.

Additionally, some fixes are applied to compile the zynqpl driver
for SPL, and to properly detect an FPGA from the SPL.

I'm not sure if the fixes are correct or I'm missing something,
hence the RFC.

Tested on a Digilent Zybo Z7-20 board

Luis Araneda (4):
  spl: fit: display a message when an FPGA image is loaded
  drivers: fpga: zynqpl: fix compilation with SPL
  arm: zynq: spl: fix FPGA initialization
  arm: zynq: spl: implement FPGA load from FIT

 arch/arm/mach-zynq/spl.c | 42 ++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_fit.c     |  1 +
 drivers/fpga/zynqpl.c    |  4 ++--
 3 files changed, 45 insertions(+), 2 deletions(-)

-- 
2.18.0

[U-Boot] [RFC PATCH 1/4] spl: fit: display a message when an FPGA image is loaded

[Luis Araneda]

A message should be displayed if an image is loaded
to an FPGA, because the hardware might have changed,
and the user should be informed

Signed-off-by: Luis Araneda <luaraneda@gmail.com>
---
 common/spl/spl_fit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 5b51a28a08..9eabb1c105 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -412,6 +412,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 			printf("%s: Cannot load the FPGA: %i\n", __func__, ret);
 			return ret;
 		}
+		puts("FPGA image loaded from FIT\n");
 		node = -1;
 	}
 #endif
-- 
2.18.0

[U-Boot] [RFC PATCH 1/4] spl: fit: display a message when an FPGA image is loaded

[Michal Simek]

On 18.7.2018 09:41, Luis Araneda wrote:
> A message should be displayed if an image is loaded
> to an FPGA, because the hardware might have changed,
> and the user should be informed
> 
> Signed-off-by: Luis Araneda <luaraneda@gmail.com>
> ---
>  common/spl/spl_fit.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 5b51a28a08..9eabb1c105 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -412,6 +412,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>  			printf("%s: Cannot load the FPGA: %i\n", __func__, ret);
>  			return ret;
>  		}
> +		puts("FPGA image loaded from FIT\n");
>  		node = -1;
>  	}
>  #endif
> 

This looks good.

M

[U-Boot] [RFC PATCH 2/4] drivers: fpga: zynqpl: fix compilation with SPL

[Luis Araneda]

Disable the use of function zynq_loadfs when compiling
the driver for the SPL, as the following filesystem
functions are not found by the linker:
- fs_set_blk_dev
- fs_read
- fs_set_blk_dev
- fs_read
- fs_read

Signed-off-by: Luis Araneda <luaraneda@gmail.com>
---
 drivers/fpga/zynqpl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/zynqpl.c b/drivers/fpga/zynqpl.c
index fd37d18c7f..8e49c7010e 100644
--- a/drivers/fpga/zynqpl.c
+++ b/drivers/fpga/zynqpl.c
@@ -410,7 +410,7 @@ static int zynq_load(xilinx_desc *desc, const void *buf, size_t bsize,
 	return FPGA_SUCCESS;
 }
 
-#if defined(CONFIG_CMD_FPGA_LOADFS)
+#if defined(CONFIG_CMD_FPGA_LOADFS) && !defined(CONFIG_SPL_BUILD)
 static int zynq_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
 		       fpga_fs_info *fsinfo)
 {
@@ -493,7 +493,7 @@ static int zynq_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
 
 struct xilinx_fpga_op zynq_op = {
 	.load = zynq_load,
-#if defined(CONFIG_CMD_FPGA_LOADFS)
+#if defined(CONFIG_CMD_FPGA_LOADFS) && !defined(CONFIG_SPL_BUILD)
 	.loadfs = zynq_loadfs,
 #endif
 };
-- 
2.18.0

[U-Boot] [RFC PATCH 2/4] drivers: fpga: zynqpl: fix compilation with SPL

[Michal Simek]

On 18.7.2018 09:41, Luis Araneda wrote:
> Disable the use of function zynq_loadfs when compiling
> the driver for the SPL, as the following filesystem
> functions are not found by the linker:
> - fs_set_blk_dev
> - fs_read
> - fs_set_blk_dev
> - fs_read
> - fs_read
> 
> Signed-off-by: Luis Araneda <luaraneda@gmail.com>
> ---
>  drivers/fpga/zynqpl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/zynqpl.c b/drivers/fpga/zynqpl.c
> index fd37d18c7f..8e49c7010e 100644
> --- a/drivers/fpga/zynqpl.c
> +++ b/drivers/fpga/zynqpl.c
> @@ -410,7 +410,7 @@ static int zynq_load(xilinx_desc *desc, const void *buf, size_t bsize,
>  	return FPGA_SUCCESS;
>  }
>  
> -#if defined(CONFIG_CMD_FPGA_LOADFS)
> +#if defined(CONFIG_CMD_FPGA_LOADFS) && !defined(CONFIG_SPL_BUILD)
>  static int zynq_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
>  		       fpga_fs_info *fsinfo)
>  {
> @@ -493,7 +493,7 @@ static int zynq_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
>  
>  struct xilinx_fpga_op zynq_op = {
>  	.load = zynq_load,
> -#if defined(CONFIG_CMD_FPGA_LOADFS)
> +#if defined(CONFIG_CMD_FPGA_LOADFS) && !defined(CONFIG_SPL_BUILD)
>  	.loadfs = zynq_loadfs,
>  #endif
>  };
> 

This looks good.

M

[U-Boot] [RFC PATCH 3/4] arm: zynq: spl: fix FPGA initialization

[Luis Araneda]

commit 4aba5fb857c1 ("arm: zynq: Rework FPGA initialization")
moved FPGA initialization from board_init() to arch_early_init_r(),
which is not called as part of the SPL

Fix this by calling arch_early_init_r() in the spl_board_init()
function, so the FPGA is correctly initialized

Signed-off-by: Luis Araneda <luaraneda@gmail.com>
---
 arch/arm/mach-zynq/spl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mach-zynq/spl.c b/arch/arm/mach-zynq/spl.c
index 83297d6c69..9b7c0be951 100644
--- a/arch/arm/mach-zynq/spl.c
+++ b/arch/arm/mach-zynq/spl.c
@@ -29,6 +29,9 @@ void board_init_f(ulong dummy)
 void spl_board_init(void)
 {
 	preloader_console_init();
+#if defined(CONFIG_ARCH_EARLY_INIT_R) && defined(CONFIG_SPL_FPGA_SUPPORT)
+	arch_early_init_r();
+#endif
 	board_init();
 }
 #endif
-- 
2.18.0

[U-Boot] [RFC PATCH 3/4] arm: zynq: spl: fix FPGA initialization

[Michal Simek]

On 18.7.2018 09:41, Luis Araneda wrote:
> commit 4aba5fb857c1 ("arm: zynq: Rework FPGA initialization")
> moved FPGA initialization from board_init() to arch_early_init_r(),
> which is not called as part of the SPL
> 
> Fix this by calling arch_early_init_r() in the spl_board_init()
> function, so the FPGA is correctly initialized
> 
> Signed-off-by: Luis Araneda <luaraneda@gmail.com>
> ---
>  arch/arm/mach-zynq/spl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/mach-zynq/spl.c b/arch/arm/mach-zynq/spl.c
> index 83297d6c69..9b7c0be951 100644
> --- a/arch/arm/mach-zynq/spl.c
> +++ b/arch/arm/mach-zynq/spl.c
> @@ -29,6 +29,9 @@ void board_init_f(ulong dummy)
>  void spl_board_init(void)
>  {
>  	preloader_console_init();
> +#if defined(CONFIG_ARCH_EARLY_INIT_R) && defined(CONFIG_SPL_FPGA_SUPPORT)
> +	arch_early_init_r();
> +#endif
>  	board_init();
>  }
>  #endif
> 

This looks good.

M

[U-Boot] [RFC PATCH 4/4] arm: zynq: spl: implement FPGA load from FIT

[Luis Araneda]

Implement FPGA bitstream loading from SPL. The bitstream
is loaded from a FIT image into dynamically allocated memory
before programming the FPGA.

Signed-off-by: Luis Araneda <luaraneda@gmail.com>
---
 arch/arm/mach-zynq/spl.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm/mach-zynq/spl.c b/arch/arm/mach-zynq/spl.c
index 9b7c0be951..42907afa94 100644
--- a/arch/arm/mach-zynq/spl.c
+++ b/arch/arm/mach-zynq/spl.c
@@ -4,6 +4,8 @@
  */
 #include <common.h>
 #include <debug_uart.h>
+#include <fpga.h>
+#include <memalign.h>
 #include <spl.h>
 
 #include <asm/io.h>
@@ -93,3 +95,40 @@ int board_fit_config_name_match(const char *name)
 	return 0;
 }
 #endif
+
+#ifdef CONFIG_SPL_FPGA_SUPPORT
+int spl_load_fpga_image(struct spl_load_info *info, size_t length,
+			int nr_sectors, int sector_offset)
+{
+	char *data_buf;
+	int ret;
+
+	data_buf = malloc_cache_aligned(nr_sectors);
+	if (!data_buf) {
+		debug("%s: Cannot reserve memory\n", __func__);
+		return -ENOMEM;
+	}
+
+	if (info->read(info, sector_offset, nr_sectors,
+		       data_buf) != nr_sectors) {
+		debug("%s: Cannot read the FPGA image\n", __func__);
+		free(data_buf);
+		return -EIO;
+	}
+
+	memmove(data_buf, data_buf + (nr_sectors - length), length);
+
+	debug("%s: FPGA image loaded to 0x%p (%zu bytes)\n",
+	      __func__, data_buf, length);
+
+	ret = fpga_load(0, data_buf, length, BIT_FULL);
+	if (ret) {
+		debug("%s: Cannot load the image to the FPGA\n", __func__);
+		free(data_buf);
+		return ret;
+	}
+
+	free(data_buf);
+	return 0;
+}
+#endif
-- 
2.18.0

[U-Boot] [RFC PATCH 4/4] arm: zynq: spl: implement FPGA load from FIT

[Michal Simek]

On 18.7.2018 09:41, Luis Araneda wrote:
> Implement FPGA bitstream loading from SPL. The bitstream
> is loaded from a FIT image into dynamically allocated memory
> before programming the FPGA.
> 
> Signed-off-by: Luis Araneda <luaraneda@gmail.com>
> ---
>  arch/arm/mach-zynq/spl.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/arch/arm/mach-zynq/spl.c b/arch/arm/mach-zynq/spl.c
> index 9b7c0be951..42907afa94 100644
> --- a/arch/arm/mach-zynq/spl.c
> +++ b/arch/arm/mach-zynq/spl.c
> @@ -4,6 +4,8 @@
>   */
>  #include <common.h>
>  #include <debug_uart.h>
> +#include <fpga.h>
> +#include <memalign.h>
>  #include <spl.h>
>  
>  #include <asm/io.h>
> @@ -93,3 +95,40 @@ int board_fit_config_name_match(const char *name)
>  	return 0;
>  }
>  #endif
> +
> +#ifdef CONFIG_SPL_FPGA_SUPPORT
> +int spl_load_fpga_image(struct spl_load_info *info, size_t length,
> +			int nr_sectors, int sector_offset)
> +{
> +	char *data_buf;
> +	int ret;
> +
> +	data_buf = malloc_cache_aligned(nr_sectors);
> +	if (!data_buf) {
> +		debug("%s: Cannot reserve memory\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	if (info->read(info, sector_offset, nr_sectors,
> +		       data_buf) != nr_sectors) {
> +		debug("%s: Cannot read the FPGA image\n", __func__);
> +		free(data_buf);
> +		return -EIO;
> +	}
> +
> +	memmove(data_buf, data_buf + (nr_sectors - length), length);
> +
> +	debug("%s: FPGA image loaded to 0x%p (%zu bytes)\n",
> +	      __func__, data_buf, length);
> +
> +	ret = fpga_load(0, data_buf, length, BIT_FULL);
> +	if (ret) {
> +		debug("%s: Cannot load the image to the FPGA\n", __func__);
> +		free(data_buf);
> +		return ret;
> +	}
> +
> +	free(data_buf);
> +	return 0;
> +}
> +#endif
> 

I was playing  with this a little bit. There is no reason to allocate
any space in malloc area because its/fit should already contain load
address which you should use instead.

Thanks,
Michal

[U-Boot] [RFC PATCH 4/4] arm: zynq: spl: implement FPGA load from FIT

[Luis Araneda]

Hi Michal,

On Wed, Jul 18, 2018 at 9:22 AM Michal Simek <michal.simek@xilinx.com> wrote:
> I was playing  with this a little bit. There is no reason to allocate
> any space in malloc area because its/fit should already contain load
> address which you should use instead.

I initially thought the same, but unfortunately the load address is
not a parameter passed to the function, nor can it be extracted from
the spl_load_info structure.
Like you have probably discovered by now, the spl_load_fpga_image()
function was introduced to support the use case of  boards with
uninitialized DRAM, so the load address was not necessary.
On the other hand, the zynpl driver doesn't have functions to program
the FPGA by chunks, so I had to allocate memory.

Thanks,

Luis Araneda.

[U-Boot] [RFC PATCH 4/4] arm: zynq: spl: implement FPGA load from FIT

[Michal Simek]

Hi,

On 18.7.2018 20:14, Luis Araneda wrote:
> Hi Michal,
> 
> On Wed, Jul 18, 2018 at 9:22 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> I was playing  with this a little bit. There is no reason to allocate
>> any space in malloc area because its/fit should already contain load
>> address which you should use instead.
> 
> I initially thought the same, but unfortunately the load address is
> not a parameter passed to the function, nor can it be extracted from
> the spl_load_info structure.
> Like you have probably discovered by now, the spl_load_fpga_image()
> function was introduced to support the use case of  boards with
> uninitialized DRAM, so the load address was not necessary.
> On the other hand, the zynpl driver doesn't have functions to program
> the FPGA by chunks, so I had to allocate memory.

Feel free to join that thread with Marek.
I think that what we have in u-boot source code is not correct and it is
one ugly hack which has no user and nothing is broken because support
hasn't been merged to mainline.
I have played with that yesterday and send this patch which should be
enough for you to go.
https://lists.denx.de/pipermail/u-boot/2018-July/335169.html

Also on zc706 without FULL_FIT my path in spl_load_fit_image is not
jumping to "if (external_data) {" branch where spl_load_fpga_image is
which is kind of interesting because it looks like you are going there.

Thanks,
Michal

[U-Boot] [RFC PATCH 4/4] arm: zynq: spl: implement FPGA load from FIT

[Luis Araneda]

Hi Michal,

On Thu, Jul 19, 2018 at 2:16 AM Michal Simek <michal.simek@xilinx.com> wrote:
> Also on zc706 without FULL_FIT my path in spl_load_fit_image is not
> jumping to "if (external_data) {" branch where spl_load_fpga_image is
> which is kind of interesting because it looks like you are going there.

To enter the path "if (external_data) {" branch where spl_load_fpga_image is}"
you have to create the FIT image with the "-E" option, I'm creating it with:
> ./tools/mkimage -f <input_its_file> -E <output_file>

I started to use the option because my initial tests weren't entering
the path either, and I found it when analyzing the (generated)
.u-boot.img.cmd file.

I just tested your patch with and without the "-E" option, and it
works on both cases :)

Thanks,

Luis Araneda.

[U-Boot] [RFC PATCH 4/4] arm: zynq: spl: implement FPGA load from FIT

[Michal Simek]

On 19.7.2018 19:22, Luis Araneda wrote:
> Hi Michal,
> 
> On Thu, Jul 19, 2018 at 2:16 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> Also on zc706 without FULL_FIT my path in spl_load_fit_image is not
>> jumping to "if (external_data) {" branch where spl_load_fpga_image is
>> which is kind of interesting because it looks like you are going there.
> 
> To enter the path "if (external_data) {" branch where spl_load_fpga_image is}"
> you have to create the FIT image with the "-E" option, I'm creating it with:
>> ./tools/mkimage -f <input_its_file> -E <output_file>
> 
> I started to use the option because my initial tests weren't entering
> the path either, and I found it when analyzing the (generated)
> .u-boot.img.cmd file.
> 
> I just tested your patch with and without the "-E" option, and it
> works on both cases :)

ok.

Thanks,
Michal

[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[Michal Simek]

On 18.7.2018 09:41, Luis Araneda wrote:
> This series implements FPGA bitstream loading from SPL.
> Programming the FPGA from the SPL is necessary on some boards,
> like the Zybo, because the FPGA fabric routes the I2C bus to an
> EEPROM for reading the Ethernet MAC address.
> 
> The bitstream is loaded from a FIT image into a dynamically-allocated
> memory before programming the FPGA.
> 
> Additionally, some fixes are applied to compile the zynqpl driver
> for SPL, and to properly detect an FPGA from the SPL.
> 
> I'm not sure if the fixes are correct or I'm missing something,
> hence the RFC.
> 
> Tested on a Digilent Zybo Z7-20 board
> 
> Luis Araneda (4):
>   spl: fit: display a message when an FPGA image is loaded
>   drivers: fpga: zynqpl: fix compilation with SPL
>   arm: zynq: spl: fix FPGA initialization
>   arm: zynq: spl: implement FPGA load from FIT
> 
>  arch/arm/mach-zynq/spl.c | 42 ++++++++++++++++++++++++++++++++++++++++
>  common/spl/spl_fit.c     |  1 +
>  drivers/fpga/zynqpl.c    |  4 ++--
>  3 files changed, 45 insertions(+), 2 deletions(-)
> 

Can you please also send defconfig/config changes?
Separate patch is fine.

Thanks,
Michal

[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[Luis Araneda]

Hi Michal,

On Wed, Jul 18, 2018 at 4:00 AM Michal Simek <michal.simek@xilinx.com> wrote:
> Can you please also send defconfig/config changes?
> Separate patch is fine.

The changes required to the defconfigs test/support this are:
CONFIG_SPL_LOAD_FIT=y
CONFIG_SPL_FPGA_SUPPORT=y

I didn't send them because just changing the defconfig isn't enough, I
had to manually create a FIT image to replace the default u-boot.img,
which is generated
by U-Boot's Makefile.
The .its file (to generate the FIT image) has the following FPGA node:
fpga-1 {
  description = "Zybo Z7-20 FPGA image";
  data = /incbin/("zybo-z7-20-preboot.bin");
  type = "fpga";
  arch = "arm";
  compression = "none";
  load = <0x30000000>;
};

I run-tested the changes on a Zybo Z7-20, and compile-tested on a Zybo.

I have an idea to automate the FIT generation. The build system could
scan for the existence of a file, for example
"board/xilinx/zynq/<board>/preboot.bin", and add the fpga node
automatically (to .its) if the file exists. Because I think that
storing .bin files in the U-Boot repository is infeasible. That will
require additions and modifications to the current way the build
system works, and I'm still thinking how to implement them.

By the way, sorry for changing the subject, but I realized that you
use two e-mail accounts. Do you have a preferred one? I'm asking
because I sent the Zybo-Z7 support series (and the I2C DM one) to the
@monstr.eu account, which you hasn't responded yet, and this series to
the @xilinx.com account, which you replied quickly. Maybe you are just
waiting for other people to review/test it.

Thanks,

Luis Araneda.

[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[Michal Simek]

On 18.7.2018 20:02, Luis Araneda wrote:
> Hi Michal,
> 
> On Wed, Jul 18, 2018 at 4:00 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> Can you please also send defconfig/config changes?
>> Separate patch is fine.
> 
> The changes required to the defconfigs test/support this are:
> CONFIG_SPL_LOAD_FIT=y
> CONFIG_SPL_FPGA_SUPPORT=y
> 
> I didn't send them because just changing the defconfig isn't enough,

It should be enough. It is configuration option and just enabling that
feature. You should still be able to use just u-boot.img in legacy or
fit format without any issue.


> I
> had to manually create a FIT image to replace the default u-boot.img,
> which is generated
> by U-Boot's Makefile.
> The .its file (to generate the FIT image) has the following FPGA node:
> fpga-1 {
>   description = "Zybo Z7-20 FPGA image";
>   data = /incbin/("zybo-z7-20-preboot.bin");
>   type = "fpga";
>   arch = "arm";
>   compression = "none";
>   load = <0x30000000>;
> };

But that's separate issue how to automate building image which contain
fpga.
I think it will be good if you can look at my patch and also compare
boot up time when you setup compression to gzip. I expect some changes
in connection to this code.

	if (IS_ENABLED(CONFIG_SPL_OS_BOOT)	&&
	    IS_ENABLED(CONFIG_SPL_GZIP)		&&
	    image_comp == IH_COMP_GZIP		&&
	    type == IH_TYPE_KERNEL) {

And I would expect that copying smaller fit with unziping bitstream will
be faster then what you have now. Especially on boards which bigger fpga.


> I run-tested the changes on a Zybo Z7-20, and compile-tested on a Zybo.
> 
> I have an idea to automate the FIT generation. The build system could
> scan for the existence of a file, for example
> "board/xilinx/zynq/<board>/preboot.bin", and add the fpga node
> automatically (to .its) if the file exists. Because I think that
> storing .bin files in the U-Boot repository is infeasible. That will
> require additions and modifications to the current way the build
> system works, and I'm still thinking how to implement them.

Take a look at pmufw handling for zynqmp for inspiration. But again
that's different issue and there shouldn't be a problem to enable this
feature without fit generation.

> 
> By the way, sorry for changing the subject, but I realized that you
> use two e-mail accounts. Do you have a preferred one? I'm asking
> because I sent the Zybo-Z7 support series (and the I2C DM one) to the
> @monstr.eu account, which you hasn't responded yet, and this series to
> the @xilinx.com account, which you replied quickly. Maybe you are just
> waiting for other people to review/test it.

My reactions on xilinx.com should be much faster because I am trying to
keep number of emails low compare to my second email where all mailing
lists are coming.
I will look at eeprom series.

Thanks,
Michal

[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[Luis Araneda]

Hi Michal,

On Thu, Jul 19, 2018 at 2:23 AM Michal Simek <michal.simek@xilinx.com> wrote:
> On 18.7.2018 20:02, Luis Araneda wrote:
> > [...]
> > I didn't send them because just changing the defconfig isn't enough,
>
> It should be enough. It is configuration option and just enabling that
> feature. You should still be able to use just u-boot.img in legacy or
> fit format without any issue.

Ok. Should I send a patch only for the Zybo or all zynq boards? Also,
for than one board, should I create one patch per board or only one
big patch for all of them?

> I think it will be good if you can look at my patch and also compare
> boot up time when you setup compression to gzip. I expect some changes
> in connection to this code.
>
>         if (IS_ENABLED(CONFIG_SPL_OS_BOOT)      &&
>             IS_ENABLED(CONFIG_SPL_GZIP)         &&
>             image_comp == IH_COMP_GZIP          &&
>             type == IH_TYPE_KERNEL) {
>
> And I would expect that copying smaller fit with unziping bitstream will
> be faster then what you have now. Especially on boards which bigger fpga.

I made some modifications to make gzip work, and another one dirty
(non-upstremeable) to make external data work.
Additionally, I added time reporting on three places. The
modifications are attached.

I tested several bitstreams, with different compression levels:
> gzip -c -n <comp_level> <bin_file> > <out_file>

The results, for a fit image with embedded data, are:
file           size (bytes) time1 time2 time3
uncompressed   2,434,112     567   597   623
compressed -1    446,028     208  1165  1190
compressed -4    407,764     205  1063  1088
compressed -5    398,346     203  1094  1119
compressed -9    376,821     200  1141  1166

The time for a fit image with external data (-E option for mkimage) is
~100 time units (ms?) less, and time1 remains constant at ~12 time
units.

At least on my setup (Zybo Z7-20), gzip just increase the boot time.

> > I have an idea to automate the FIT generation. The build system could
> > scan for the existence of a file, for example
> > "board/xilinx/zynq/<board>/preboot.bin", and add the fpga node
> > automatically (to .its) if the file exists. Because I think that
> > storing .bin files in the U-Boot repository is infeasible. That will
> > require additions and modifications to the current way the build
> > system works, and I'm still thinking how to implement them.
>
> Take a look at pmufw handling for zynqmp for inspiration.

Will do, thanks for the heads up.

Thanks,

Luis Araneda.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gzip_test.patch
Type: text/x-patch
Size: 2137 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180719/9160dcbd/attachment.bin>

[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[Michal Simek]

Hi,

On 20.7.2018 01:37, Luis Araneda wrote:
> Hi Michal,
> 
> On Thu, Jul 19, 2018 at 2:23 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> On 18.7.2018 20:02, Luis Araneda wrote:
>>> [...]
>>> I didn't send them because just changing the defconfig isn't enough,
>>
>> It should be enough. It is configuration option and just enabling that
>> feature. You should still be able to use just u-boot.img in legacy or
>> fit format without any issue.
> 
> Ok. Should I send a patch only for the Zybo or all zynq boards? Also,
> for than one board, should I create one patch per board or only one
> big patch for all of them?

We need that functionality first but then enable it for all boards is
fine for me and via one patch.

> 
>> I think it will be good if you can look at my patch and also compare
>> boot up time when you setup compression to gzip. I expect some changes
>> in connection to this code.
>>
>>         if (IS_ENABLED(CONFIG_SPL_OS_BOOT)      &&
>>             IS_ENABLED(CONFIG_SPL_GZIP)         &&
>>             image_comp == IH_COMP_GZIP          &&
>>             type == IH_TYPE_KERNEL) {
>>
>> And I would expect that copying smaller fit with unziping bitstream will
>> be faster then what you have now. Especially on boards which bigger fpga.
> 
> I made some modifications to make gzip work, and another one dirty
> (non-upstremeable) to make external data work.
> Additionally, I added time reporting on three places. The
> modifications are attached.
> 
> I tested several bitstreams, with different compression levels:
>> gzip -c -n <comp_level> <bin_file> > <out_file>
> 
> The results, for a fit image with embedded data, are:
> file           size (bytes) time1 time2 time3
> uncompressed   2,434,112     567   597   623
> compressed -1    446,028     208  1165  1190
> compressed -4    407,764     205  1063  1088
> compressed -5    398,346     203  1094  1119
> compressed -9    376,821     200  1141  1166
> 
> The time for a fit image with external data (-E option for mkimage) is
> ~100 time units (ms?) less, and time1 remains constant at ~12 time
> units.

Can you please be more specific what time1/time2 and time3 means?

> 
> At least on my setup (Zybo Z7-20), gzip just increase the boot time.

It could be because of bitstream size is quite small for this chip.

Thanks,
Michal

[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[Luis Araneda]

Hi Michal,

On Fri, Jul 20, 2018 at 6:38 AM Michal Simek <michal.simek@xilinx.com> wrote:
> On 20.7.2018 01:37, Luis Araneda wrote:
> > Hi Michal,
> >
> > On Thu, Jul 19, 2018 at 2:23 AM Michal Simek <michal.simek@xilinx.com> wrote:
> We need that functionality first but then enable it for all boards is
> fine for me and via one patch.

Ok

> Can you please be more specific what time1/time2 and time3 means?

The exact location of time 1/2/3 are on the attached diff file, and
they are placed within the spl_load_simple_fit() function.
They represent, roughly:
- time1: Time to load the the FIT image
- time2: Time to extract (and decompress)
  the FPGA image from the FIT image
- time3: Time to program the FPGA

Thanks,

Luis Araneda.

[U-Boot] [RFC PATCH 0/4] arm: zynq: implement FPGA load from SPL

[Michal Simek]

Hi,

On 20.7.2018 18:17, Luis Araneda wrote:
> Hi Michal,
> 
> On Fri, Jul 20, 2018 at 6:38 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> On 20.7.2018 01:37, Luis Araneda wrote:
>>> Hi Michal,
>>>
>>> On Thu, Jul 19, 2018 at 2:23 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> We need that functionality first but then enable it for all boards is
>> fine for me and via one patch.
> 
> Ok
> 
>> Can you please be more specific what time1/time2 and time3 means?
> 
> The exact location of time 1/2/3 are on the attached diff file, and
> they are placed within the spl_load_simple_fit() function.
> They represent, roughly:
> - time1: Time to load the the FIT image
> - time2: Time to extract (and decompress)
>   the FPGA image from the FIT image
> - time3: Time to program the FPGA

Sorry I missed that attachment.

First of all I have sent patch for that gzip.

On zc706 with 13MB bitstream size this looks much better.

file         size (bytes) time1 time2 time3
uncompressed  13869613    2533  2694  4422
compressed -9   599149    144   765   2491

This is SD boot mode and initial time depends on SD you use.

Thanks,
Michal