[PATCH V1] board: xilinx: zynq: Add a new cmd to run postconfig

[Michal Simek]

On 12/13/24 09:52, xueweiwujxw wrote:
> This patch adds a new command named "post_config" to execute the
> "ps7_post_config" function.
> 
> Signed-off-by: xueweiwujxw <xueweiwujxw at gmail.com>

first of all please spell your name properly here.

> ---
>   board/xilinx/zynq/Kconfig              | 17 +++++++++++++++++
>   board/xilinx/zynq/Makefile             |  2 +-
>   board/xilinx/zynq/cmds.c               | 20 ++++++++++++++++++++
>   test/py/tests/test_zynq_post_config.py | 16 ++++++++++++++++
>   4 files changed, 54 insertions(+), 1 deletion(-)
>   create mode 100644 test/py/tests/test_zynq_post_config.py

When the patch is applied.
make xilinx_zynq_virt_defconfig
make

   CC      arch/arm/cpu/armv7/syslib.o
board/xilinx/zynq/cmds.c:21:10: fatal error: ps7_init_gpl.h: No such file or 
directory
    21 | #include "ps7_init_gpl.h"
       |          ^~~~~~~~~~~~~~~~
compilation terminated.

Please look below.

> 
> diff --git a/board/xilinx/zynq/Kconfig b/board/xilinx/zynq/Kconfig
> index d6f40631cc..a1624c71c0 100644
> --- a/board/xilinx/zynq/Kconfig
> +++ b/board/xilinx/zynq/Kconfig
> @@ -30,4 +30,21 @@ config CMD_ZYNQ_RSA
>   	  and authentication feature enabled while generating
>   	  BOOT.BIN using Xilinx bootgen tool.
>   
> +config CMD_POST_CONFIG
> +	bool "Enable post_config command"
> +	default y
> +	help
> +	  This option enables the 'post_config' command in U-Boot.
> +	  The 'post_config' command is used to execute the PS7 post-configuration
> +	  process after loading an FPGA bitstream file on Xilinx Zynq devices.
> +	  This command ensures that the programmable logic (PL) and
> +	  processing system (PS) are properly synchronized after
> +	  FPGA configuration. It is specifically useful in designs where
> +	  additional initialization steps are required for the PS to
> +	  correctly interact with the configured PL.

ps7_init() is called only from SPL in current configuration.
And ps7_post_config() is called from spl_board_prepare_for_boot() already.
It means cases where SPL loads bitstream should be working just fine.

Then we can switch to case where bitstream is loaded via u-boot command.
The biggest question is if your ps7_init_gpl is aligned with the design you are 
actually loading. Likely yes but in general it doesn't need to be.

Then we are getting to another question if calling ps7_post_config in this 
context make sense or if it is better to do it in a different way.

I think you should work on better description why you think that this should be 
called/done. And it is not clear what else you want to do after bitstream is 
loaded. If this is about enabling ps-pl interface and getting pl out of reset we 
can talk about it.


> +	  This option is only applicable to Xilinx Zynq platforms and

That's what it is said already because this file is sourced only when Zynq is 
selected.

> +	  has no effect on other architectures. If you are not using the
> +	  PS7 subsystem or do not require post-configuration steps,
> +	  this option can be disabled to reduce the size of the U-Boot binary.
> +
>   endif
> diff --git a/board/xilinx/zynq/Makefile b/board/xilinx/zynq/Makefile
> index f40fe382f5..366a4d931a 100644
> --- a/board/xilinx/zynq/Makefile
> +++ b/board/xilinx/zynq/Makefile
> @@ -32,7 +32,7 @@ endif
>   endif
>   
>   ifndef CONFIG_XPL_BUILD
> -obj-$(CONFIG_CMD_ZYNQ) += cmds.o
> +obj-$(CONFIG_CMD_ZYNQ) += $(init-objs) cmds.o

No idea what you are trying to do here.

>   obj-$(CONFIG_CMD_ZYNQ_RSA) += bootimg.o
>   endif
>   
> diff --git a/board/xilinx/zynq/cmds.c b/board/xilinx/zynq/cmds.c
> index 05ecb75406..60278fcdf6 100644
> --- a/board/xilinx/zynq/cmds.c
> +++ b/board/xilinx/zynq/cmds.c
> @@ -18,6 +18,7 @@
>   #include <zynqpl.h>
>   #include <fpga.h>
>   #include <zynq_bootimg.h>
> +#include "ps7_init_gpl.h"
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -548,3 +549,22 @@ U_BOOT_LONGHELP(zynq,
>   U_BOOT_CMD(zynq,	6,	0,	do_zynq,
>   	   "Zynq specific commands", zynq_help_text
>   );
> +
> +static int do_ps7_fpga_post_config(struct cmd_tbl *cmdtp, int flag, int argc,
> +				   char *const argv[])
> +{
> +	int ret = PS7_INIT_SUCCESS;
> +
> +	if (IS_ENABLED(CONFIG_CMD_POST_CONFIG))

Clearly wrong. You should enable this command only when Kconfig is enabled.

> +		ret = ps7_post_config();
> +
> +	if (ret != PS7_INIT_SUCCESS)
> +		puts("ERROR:run board ps7 post config failed.\n");
> +	else
> +		puts("INFO: run board ps7 post config succeed.\n");

you have return value from command. No need to be verbose. But you can but under 
DEBUG only.

> +	return ret;

use cmd_process_error()

> +}
> +
> +U_BOOT_CMD(post_config,	1,	0,	do_ps7_fpga_post_config,
> +	   "run ps7 post config after fpga load bin file",	""
> +);

Obviously own command make no sense. You have zynq command already and this 
would be it's subcommand.


> diff --git a/test/py/tests/test_zynq_post_config.py b/test/py/tests/test_zynq_post_config.py
> new file mode 100644
> index 0000000000..4493f8f190
> --- /dev/null
> +++ b/test/py/tests/test_zynq_post_config.py
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Test for the "post_config" command
> +#
> +# xueweiwujxw

This should be full copyright with name, year, etc

> +
> +import pytest
> +
> + at pytest.mark.buildconfigspec('cmd_post_config')
> +def test_post_config(u_boot_console):
> +    """Test the 'post_config' command on Zynq platform"""
> +
> +    output = u_boot_console.run_command("post_config")
> +
> +    assert "INFO: run board ps7 post config succeed." in output
> +

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP/Versal ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal/Versal NET SoCs
TF-A maintainer - Xilinx ZynqMP/Versal/Versal NET SoCs