[U-Boot] [RFC PATCH 00/15] cmd: fpga: Fix fpga command handling and add some fpga tests

Hi,
We are using this framework for a while and by adding more and more features it requires small redesigned how commands are handled. This is something what I have put together to improve readability of this code and also remove code which is bogus and completely untested.
The series depends on these patches. https://lists.denx.de/pipermail/u-boot/2018-June/332487.html https://lists.denx.de/pipermail/u-boot/2018-June/332488.html https://lists.denx.de/pipermail/u-boot/2018-June/330651.html
I am sending this as RFC to get a feedback on these changes. I have rebased it on the latest tree and need to run another testing round again.
Thanks, Michal
Michal Simek (15): test/py: Extend fpga command to test all fpga load types cmd: fpga: Move error handling to do_fpga() cmd: fpga: Move fpga_get_op to avoid local function declaration cmd: fpga: Cleanup error handling in connection to FPGA_NONE cmd: fpga: Move parameter checking for loadfs/loads cmd: fpga: Remove parameter checking from fpga loadfs command cmd: fpga: Clean wrong_parms handling cmd: fpga: Create new do_fpga_wrapper for using u-boot subcommands cmd: fpga: Extract fpga info command to separate function cmd: fpga: Fix dump and all direct fpga load commands cmd: fpga: Fix loadfs command cmd: fpga: Fix loadmk command cmd: fpga: Use CMD_RET_FAILURE instead of simple 1 cmd: fpga: Fix loads command MAINTAINERS: Add myself as the FPGA maintainer
MAINTAINERS | 8 + cmd/fpga.c | 606 ++++++++++++++++++++++++--------------------- test/py/tests/test_fpga.py | 516 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 843 insertions(+), 287 deletions(-) create mode 100644 test/py/tests/test_fpga.py

Add support for info, load, loadp, loadb, loadbp, loadmk_legacy, loadmk_legacy_gz, loadmk_fit, loadfs also with variable support.
There are probably missing failed tests.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
test/py/tests/test_fpga.py | 516 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 516 insertions(+) create mode 100644 test/py/tests/test_fpga.py
diff --git a/test/py/tests/test_fpga.py b/test/py/tests/test_fpga.py new file mode 100644 index 000000000000..4d8811662c1e --- /dev/null +++ b/test/py/tests/test_fpga.py @@ -0,0 +1,516 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Copyright (c) 2018, Xilinx Inc. +# +# Michal Simek +# Siva Durga Prasad Paladugu + +import pytest +import re +import random +import u_boot_utils + +""" +Note: This test relies on boardenv_* containing configuration values to define +the network available and files to be used for testing. Without this, this test +will be automatically skipped. + +For example: + +# True if a DHCP server is attached to the network, and should be tested. +env__net_dhcp_server = True + +# A list of environment variables that should be set in order to configure a +# static IP. In this test case we atleast need serverip for performing tftpb +# to get required files. +env__net_static_env_vars = [ + ("ipaddr", "10.0.0.100"), + ("netmask", "255.255.255.0"), + ("serverip", "10.0.0.1"), +] + +# Details regarding the files that may be read from a TFTP server. . +env__fpga_secure_readable_file = { + "fn": "auth_bhdr_ppk1_bit.bin", + "enckupfn": "auth_bhdr_enc_kup_load_bit.bin", + "addr": 0x1000000, + "keyaddr": 0x100000, + "keyfn": "key.txt", +} + +env__fpga_under_test = { + "dev": 0, + "addr" : 0x1000000, + "bitstream_load": "compress.bin", + "bitstream_load_size": 1831960, + "bitstream_loadp": "compress_pr.bin", + "bitstream_loadp_size": 423352, + "bitstream_loadb": "compress.bit", + "bitstream_loadb_size": 1832086, + "bitstream_loadbp": "compress_pr.bit", + "bitstream_loadbp_size": 423491, + "mkimage_legacy": "download.ub", + "mkimage_legacy_size": 13321468, + "mkimage_legacy_gz": "download.gz.ub", + "mkimage_legacy_gz_size": 53632, + "mkimage_fit": "download-fit.ub", + "mkimage_fit_size": 13322784, + "loadfs": "mmc 0 compress.bin", + "loadfs_size": 1831960, + "loadfs_block_size": 0x10000, +} +""" + +import test_net + +def check_dev(u_boot_console): + f = u_boot_console.config.env.get('env__fpga_under_test', None) + if not f: + pytest.skip('No FPGA to test') + + dev = f.get('dev', -1) + if dev < 0: + pytest.fail('No dev specified via env__fpga_under_test') + + return dev, f + +def load_file_from_var(u_boot_console, name): + dev, f = check_dev(u_boot_console) + + addr = f.get('addr', -1) + if addr < 0: + pytest.fail('No address specified via env__fpga_under_test') + + test_net.test_net_dhcp(u_boot_console) + test_net.test_net_setup_static(u_boot_console) + bit = f['%s' % (name)] + bit_size = f['%s_size' % (name)] + + expected_tftp = 'Bytes transferred = %d' % bit_size + output = u_boot_console.run_command('tftpboot %x %s' % (addr, bit)) + assert expected_tftp in output + + return f, dev, addr, bit, bit_size + +###### FPGA FAIL test ###### +expected_usage = 'fpga - loadable FPGA image support' + +@pytest.mark.xfail +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_fail(u_boot_console): + # Test non valid fpga subcommand + expected = 'fpga: non existing command' + output = u_boot_console.run_command('fpga broken 0') + assert expected in output + assert expected_usage in output + +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_help(u_boot_console): + # Just show help + output = u_boot_console.run_command('fpga') + assert expected_usage in output + + +###### FPGA DUMP tests ###### + +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_dump(u_boot_console): + pytest.skip('Not implemented now') + +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_dump_variable(u_boot_console): + # Same as above but via "fpga" variable + pytest.skip('Not implemented now') + +###### FPGA INFO tests ###### + +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_info_fail(u_boot_console): + # Maybe this can be skipped completely + dev, f = check_dev(u_boot_console) + + # Multiple parameters to fpga info should fail + expected = 'fpga: more parameters passed' + output = u_boot_console.run_command('fpga info 0 0') + assert expected in output + assert expected_usage in output + +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_info_list(u_boot_console): + # Maybe this can be skipped completely + dev, f = check_dev(u_boot_console) + + # Code is design in a way that if fpga dev is not passed it should + # return list of all fpga devices in the system + u_boot_console.run_command('setenv fpga') + output = u_boot_console.run_command('fpga info') + assert expected_usage not in output + +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_info(u_boot_console): + dev, f = check_dev(u_boot_console) + + output = u_boot_console.run_command('fpga info %x' % (dev)) + assert expected_usage not in output + +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_info_variable(u_boot_console): + dev, f = check_dev(u_boot_console) + + # + # fpga variable is storing device number which doesn't need to be passed + # + u_boot_console.run_command('setenv fpga %x' % (dev)) + + output = u_boot_console.run_command('fpga info') + # Variable cleanup + u_boot_console.run_command('setenv fpga') + assert expected_usage not in output + +###### FPGA LOAD tests ###### + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_load_fail(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'bitstream_load') + + for cmd in ['dump', 'load', 'loadb']: + # missing dev parameter + #expected = 'fpga: incorrect parameters passed' + output = u_boot_console.run_command('fpga %s %x $filesize' % (cmd, addr)) + #assert expected in output + assert expected_usage in output + + # more parameters - 0 at the end + expected = 'fpga: more parameters passed' + output = u_boot_console.run_command('fpga %s %x %x $filesize 0' % (cmd, dev, addr)) + assert expected in output + assert expected_usage in output + + # 0 address + expected = 'fpga: zero fpga_data address' + output = u_boot_console.run_command('fpga %s %x 0 $filesize' % (cmd, dev)) + assert expected in output + assert expected_usage in output + + # 0 filesize + expected = 'fpga: zero size' + output = u_boot_console.run_command('fpga %s %x %x 0' % (cmd, dev, addr)) + assert expected in output + assert expected_usage in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_load(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'bitstream_load') + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga load %x %x $filesize && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadp') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadp(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'bitstream_load') + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga load %x %x $filesize && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + + # And load also partial bistream + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'bitstream_loadp') + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadp %x %x $filesize && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadb(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'bitstream_loadb') + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadb %x %x $filesize && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadbp') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadbp(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'bitstream_loadb') + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadb %x %x $filesize && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + + # And load also partial bistream in bit format + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'bitstream_loadbp') + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadbp %x %x $filesize && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + +###### FPGA LOADMK tests ###### + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadmk') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadmk_fail(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'mkimage_legacy') + + u_boot_console.run_command('imi %x' % (addr)) + + # load image but pass incorrect address to show error message + expected = 'Unknown image type' + output = u_boot_console.run_command('fpga loadmk %x %x' % (dev, addr + 0x10)) + assert expected in output + assert expected_usage in output + + # Pass more parameters then command expects - 0 at the end + output = u_boot_console.run_command('fpga loadmk %x %x 0' % (dev, addr)) + #assert expected in output + assert expected_usage in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadmk') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadmk_legacy(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'mkimage_legacy') + + u_boot_console.run_command('imi %x' % (addr)) + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadmk %x %x && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + +@pytest.mark.xfail +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadmk') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadmk_legacy_variable(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'mkimage_legacy') + + u_boot_console.run_command('imi %x' % (addr)) + + u_boot_console.run_command('setenv fpga %x' % (dev)) + + # this testcase should cover case which looks like it is supported but dev pointer is broken by loading mkimage address + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadmk %x && echo %s' % (addr, expected_text)) + u_boot_console.run_command('setenv fpga') + assert expected_text in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadmk') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadmk_legacy_variable_fpgadata(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'mkimage_legacy') + + u_boot_console.run_command('imi %x' % (addr)) + + u_boot_console.run_command('setenv fpga %x' % (dev)) + u_boot_console.run_command('setenv fpgadata %x' % (addr)) + + # this testcase should cover case which looks like it is supported but dev pointer is broken by loading mkimage address + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadmk && echo %s' % (expected_text)) + u_boot_console.run_command('setenv fpga') + u_boot_console.run_command('setenv fpgadata') + assert expected_text in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadmk') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadmk_legacy_gz(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'mkimage_legacy_gz') + + u_boot_console.run_command('imi %x' % (addr)) + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadmk %x %x && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadmk') +@pytest.mark.buildconfigspec('fit') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadmk_fit(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'mkimage_fit') + + u_boot_console.run_command('imi %x' % (addr)) + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadmk %x %x:fpga && echo %s' % (dev, addr, expected_text)) + assert expected_text in output + +@pytest.mark.xfail +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadmk') +@pytest.mark.buildconfigspec('fit') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadmk_fit_variable(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'mkimage_fit') + + u_boot_console.run_command('imi %x' % (addr)) + # FIXME this should fail - broken support in past + u_boot_console.run_command('setenv fpga %x' % (dev)) + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadmk %x:fpga && echo %s' % (addr, expected_text)) + u_boot_console.run_command('setenv fpga') + assert expected_text in output + +@pytest.mark.xfail +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_loadmk') +@pytest.mark.buildconfigspec('fit') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadmk_fit_variable_fpgadata(u_boot_console): + f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'mkimage_fit') + + u_boot_console.run_command('imi %x' % (addr)) + + u_boot_console.run_command('setenv fpga %x' % (dev)) + u_boot_console.run_command('setenv fpgadata %x:fpga' % (addr)) + + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadmk && echo %s' % (expected_text)) + u_boot_console.run_command('setenv fpga') + u_boot_console.run_command('setenv fpgadata') + assert expected_text in output + +###### FPGA LOAD tests ###### + +@pytest.mark.buildconfigspec('cmd_fpga') +def test_fpga_loadfs_fail(u_boot_console): + dev, f = check_dev(u_boot_console) + + addr = f.get('addr', -1) + if addr < 0: + pytest.fail('No address specified via env__fpga_under_test') + + bit = f['loadfs'] + bit_size = f['loadfs_size'] + block_size = f['loadfs_block_size'] + + # less params - dev number removed + #expected = 'fpga: incorrect parameters passed' + output = u_boot_console.run_command('fpga loadfs %x %x %x %s' % (addr, bit_size, block_size, bit)) + #assert expected in output + assert expected_usage in output + + # one more param - 0 at the end + # This is the longest command that's why there is no message from cmd/fpga.c + output = u_boot_console.run_command('fpga loadfs %x %x %x %x %s 0' % (dev, addr, bit_size, block_size, bit)) + assert expected_usage in output + + # zero address 0 + expected = 'fpga: zero fpga_data address' + output = u_boot_console.run_command('fpga loadfs %x %x %x %x %s' % (dev, 0, bit_size, block_size, bit)) + assert expected in output + assert expected_usage in output + + # bit_size 0 + expected = 'fpga: zero size' + output = u_boot_console.run_command('fpga loadfs %x %x %x %x %s' % (dev, addr, 0, block_size, bit)) + assert expected in output + assert expected_usage in output + + # block size 0 + # FIXME this should pass but it failing too + output = u_boot_console.run_command('fpga loadfs %x %x %x %x %s' % (dev, addr, bit_size, 0, bit)) + assert expected_usage in output + + # non existing bitstream name + expected = 'Unable to read file noname' + output = u_boot_console.run_command('fpga loadfs %x %x %x %x mmc 0 noname' % (dev, addr, bit_size, block_size)) + assert expected in output + assert expected_usage in output + + # -1 dev number + expected = 'fpga_fsload: Invalid device number -1' + output = u_boot_console.run_command('fpga loadfs %d %x %x %x mmc 0 noname' % (-1, addr, bit_size, block_size)) + assert expected in output + assert expected_usage in output + + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_echo') +def test_fpga_loadfs(u_boot_console): + dev, f = check_dev(u_boot_console) + + addr = f.get('addr', -1) + if addr < 0: + pytest.fail('No address specified via env__fpga_under_test') + + bit = f['loadfs'] + bit_size = f['loadfs_size'] + block_size = f['loadfs_block_size'] + + # This should be done better + expected_text = 'FPGA loaded successfully' + output = u_boot_console.run_command('fpga loadfs %x %x %x %x %s && echo %s' % (dev, addr, bit_size, block_size, bit, expected_text)) + assert expected_text in output + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_load_secure') +@pytest.mark.buildconfigspec('cmd_net') +@pytest.mark.buildconfigspec('cmd_dhcp') +@pytest.mark.buildconfigspec('net') +def test_fpga_secure_bit_auth(u_boot_console): + + test_net.test_net_dhcp(u_boot_console) + test_net.test_net_setup_static(u_boot_console) + + f = u_boot_console.config.env.get('env__fpga_secure_readable_file', None) + if not f: + pytest.skip('No TFTP readable file to read') + + addr = f.get('addr', None) + if not addr: + addr = u_boot_utils.find_ram_base(u_boot_console) + + expected_tftp = 'Bytes transferred = ' + fn = f['fn'] + output = u_boot_console.run_command('tftpboot %x %s' % (addr, fn)) + assert expected_tftp in output + + expected_zynqmpsecure = 'Bitstream successfully loaded' + output = u_boot_console.run_command('fpga loads 0 %x $filesize 0 2' % (addr)) + assert expected_zynqmpsecure in output + + +@pytest.mark.buildconfigspec('cmd_fpga') +@pytest.mark.buildconfigspec('cmd_fpga_load_secure') +@pytest.mark.buildconfigspec('cmd_net') +@pytest.mark.buildconfigspec('cmd_dhcp') +@pytest.mark.buildconfigspec('net') +def test_fpga_secure_bit_img_auth_kup(u_boot_console): + + test_net.test_net_dhcp(u_boot_console) + test_net.test_net_setup_static(u_boot_console) + + f = u_boot_console.config.env.get('env__fpga_secure_readable_file', None) + if not f: + pytest.skip('No TFTP readable file to read') + + keyaddr = f.get('keyaddr', None) + if not keyaddr: + addr = u_boot_utils.find_ram_base(u_boot_console) + expected_tftp = 'Bytes transferred = ' + keyfn = f['keyfn'] + output = u_boot_console.run_command('tftpboot %x %s' % (keyaddr, keyfn)) + assert expected_tftp in output + + addr = f.get('addr', None) + if not addr: + addr = u_boot_utils.find_ram_base(u_boot_console) + expected_tftp = 'Bytes transferred = ' + fn = f['enckupfn'] + output = u_boot_console.run_command('tftpboot %x %s' % (addr, fn)) + assert expected_tftp in output + + expected_zynqmpsecure = 'Bitstream successfully loaded' + output = u_boot_console.run_command('fpga loads 0 %x $filesize 0 1 %x' % (addr, keyaddr)) + assert expected_zynqmpsecure in output

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Add support for info, load, loadp, loadb, loadbp, loadmk_legacy, loadmk_legacy_gz, loadmk_fit, loadfs also with variable support.
There are probably missing failed tests.
Signed-off-by: Michal Simek michal.simek@xilinx.com
test/py/tests/test_fpga.py | 516 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 516 insertions(+) create mode 100644 test/py/tests/test_fpga.py
Reviewed-by: Simon Glass sjg@chromium.org

Clean fpga_get_op() error handling by moving checking/print to do_fpga.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 791fe5cb7718..abe683720285 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -74,6 +74,9 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) op = (int)fpga_get_op(argv[1]);
switch (op) { + case FPGA_NONE: + printf("Unknown fpga operation "%s"\n", argv[1]); + return CMD_RET_USAGE; #if defined(CONFIG_CMD_FPGA_LOADFS) case FPGA_LOADFS: if (argc < 9) @@ -360,9 +363,6 @@ static int fpga_get_op(char *opstr) op = FPGA_LOADS; #endif
- if (op == FPGA_NONE) - printf("Unknown fpga operation "%s"\n", opstr); - return op; }

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Clean fpga_get_op() error handling by moving checking/print to do_fpga.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

Move fpga_get_op() to top of file to remove local function declaration and also remove useless retyping.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 85 ++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 41 insertions(+), 44 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index abe683720285..de8505e9d4c8 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -13,9 +13,6 @@ #include <fs.h> #include <malloc.h>
-/* Local functions */ -static int fpga_get_op(char *opstr); - /* Local defines */ enum { FPGA_NONE = -1, @@ -30,6 +27,46 @@ enum { FPGA_LOADS, };
+/* + * Map op to supported operations. We don't use a table since we + * would just have to relocate it from flash anyway. + */ +static int fpga_get_op(char *opstr) +{ + int op = FPGA_NONE; + + if (!strcmp("info", opstr)) + op = FPGA_INFO; + else if (!strcmp("loadb", opstr)) + op = FPGA_LOADB; + else if (!strcmp("load", opstr)) + op = FPGA_LOAD; +#if defined(CONFIG_CMD_FPGA_LOADP) + else if (!strcmp("loadp", opstr)) + op = FPGA_LOADP; +#endif +#if defined(CONFIG_CMD_FPGA_LOADBP) + else if (!strcmp("loadbp", opstr)) + op = FPGA_LOADBP; +#endif +#if defined(CONFIG_CMD_FPGA_LOADFS) + else if (!strcmp("loadfs", opstr)) + op = FPGA_LOADFS; +#endif +#if defined(CONFIG_CMD_FPGA_LOADMK) + else if (!strcmp("loadmk", opstr)) + op = FPGA_LOADMK; +#endif + else if (!strcmp("dump", opstr)) + op = FPGA_DUMP; +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) + else if (!strcmp("loads", opstr)) + op = FPGA_LOADS; +#endif + + return op; +} + /* ------------------------------------------------------------------------- */ /* command form: * fpga <op> <device number> <data addr> <datasize> @@ -71,7 +108,7 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_USAGE; }
- op = (int)fpga_get_op(argv[1]); + op = fpga_get_op(argv[1]);
switch (op) { case FPGA_NONE: @@ -326,46 +363,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return rc; }
-/* - * Map op to supported operations. We don't use a table since we - * would just have to relocate it from flash anyway. - */ -static int fpga_get_op(char *opstr) -{ - int op = FPGA_NONE; - - if (!strcmp("info", opstr)) - op = FPGA_INFO; - else if (!strcmp("loadb", opstr)) - op = FPGA_LOADB; - else if (!strcmp("load", opstr)) - op = FPGA_LOAD; -#if defined(CONFIG_CMD_FPGA_LOADP) - else if (!strcmp("loadp", opstr)) - op = FPGA_LOADP; -#endif -#if defined(CONFIG_CMD_FPGA_LOADBP) - else if (!strcmp("loadbp", opstr)) - op = FPGA_LOADBP; -#endif -#if defined(CONFIG_CMD_FPGA_LOADFS) - else if (!strcmp("loadfs", opstr)) - op = FPGA_LOADFS; -#endif -#if defined(CONFIG_CMD_FPGA_LOADMK) - else if (!strcmp("loadmk", opstr)) - op = FPGA_LOADMK; -#endif - else if (!strcmp("dump", opstr)) - op = FPGA_DUMP; -#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) - else if (!strcmp("loads", opstr)) - op = FPGA_LOADS; -#endif - - return op; -} - #if defined(CONFIG_CMD_FPGA_LOADFS) || defined(CONFIG_CMD_FPGA_LOAD_SECURE) U_BOOT_CMD(fpga, 9, 1, do_fpga, #else

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Move fpga_get_op() to top of file to remove local function declaration and also remove useless retyping.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 85 ++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 41 insertions(+), 44 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But can this not use a sub-command array?
- Simon

On 19.7.2018 03:32, Simon Glass wrote:
On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Move fpga_get_op() to top of file to remove local function declaration and also remove useless retyping.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 85 ++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 41 insertions(+), 44 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But can this not use a sub-command array?
As you see the whole series is coming to that direction. I just needed to do some preparation steps before doing that.
Thanks, Michal

Hi Michal,
On 19 July 2018 at 00:55, Michal Simek michal.simek@xilinx.com wrote:
On 19.7.2018 03:32, Simon Glass wrote:
On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Move fpga_get_op() to top of file to remove local function declaration and also remove useless retyping.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 85 ++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 41 insertions(+), 44 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But can this not use a sub-command array?
As you see the whole series is coming to that direction. I just needed to do some preparation steps before doing that.
Yes I t see that, thanks.
- Simon

Incorrect command is already handled and FPGA_NONE should be used only one. In case of error CMD_RET_USAGE can be returned directly without any addition logic around.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index de8505e9d4c8..af2f514dca00 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -171,11 +171,10 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
if (dev == FPGA_INVALID_DEVICE) { puts("FPGA device not specified\n"); - op = FPGA_NONE; + return CMD_RET_USAGE; }
switch (op) { - case FPGA_NONE: case FPGA_INFO: break; #if defined(CONFIG_CMD_FPGA_LOADFS) @@ -219,13 +218,10 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
if (wrong_parms) { puts("Wrong parameters for FPGA request\n"); - op = FPGA_NONE; + return CMD_RET_USAGE; }
switch (op) { - case FPGA_NONE: - return CMD_RET_USAGE; - case FPGA_INFO: rc = fpga_info(dev); break;

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Incorrect command is already handled and FPGA_NONE should be used only one. In case of error CMD_RET_USAGE can be returned directly without any addition logic around.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

There is no reason to check parameters in separate switch before main one. This patch is simplifying error path and checking parameters right after assignment.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index af2f514dca00..48902286f1d5 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -123,6 +123,14 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) fpga_fsinfo.interface = argv[6]; fpga_fsinfo.dev_part = argv[7]; fpga_fsinfo.filename = argv[8]; + + /* Blocksize can be zero */ + if (!fpga_fsinfo.interface || !fpga_fsinfo.dev_part || + !fpga_fsinfo.filename) { + puts("ERR: Wrong interface, dev_part or filename\n"); + return CMD_RET_USAGE; + } + argc = 5; break; #endif @@ -136,6 +144,19 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) NULL, 16); fpga_sec_info.encflag = (u8)simple_strtoul(argv[6], NULL, 16); fpga_sec_info.authflag = (u8)simple_strtoul(argv[5], NULL, 16); + + if (fpga_sec_info.authflag >= FPGA_NO_ENC_OR_NO_AUTH && + fpga_sec_info.encflag >= FPGA_NO_ENC_OR_NO_AUTH) { + puts("ERR: Use <fpga load> for NonSecure bitstream\n"); + return CMD_RET_USAGE; + } + + if (fpga_sec_info.encflag == FPGA_ENC_USR_KEY && + !fpga_sec_info.userkey_addr) { + puts("ERR: User key not provided\n"); + return CMD_RET_USAGE; + } + argc = 5; break; #endif @@ -177,29 +198,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) switch (op) { case FPGA_INFO: break; -#if defined(CONFIG_CMD_FPGA_LOADFS) - case FPGA_LOADFS: - /* Blocksize can be zero */ - if (!fpga_fsinfo.interface || !fpga_fsinfo.dev_part || - !fpga_fsinfo.filename) - wrong_parms = 1; - break; -#endif -#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) - case FPGA_LOADS: - if (fpga_sec_info.authflag >= FPGA_NO_ENC_OR_NO_AUTH && - fpga_sec_info.encflag >= FPGA_NO_ENC_OR_NO_AUTH) { - puts("ERR: use <fpga load> for NonSecure bitstream\n"); - wrong_parms = 1; - } - - if (fpga_sec_info.encflag == FPGA_ENC_USR_KEY && - !fpga_sec_info.userkey_addr) { - wrong_parms = 1; - puts("ERR:User key not provided\n"); - } - break; -#endif case FPGA_LOAD: case FPGA_LOADP: case FPGA_LOADB:

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
There is no reason to check parameters in separate switch before main one. This patch is simplifying error path and checking parameters right after assignment.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

Parameter checking is dead code because all the time there must be all params assigned. If they are not assigned there is no 9th parameters passed and checking before return CMD_RET_USAGE.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 48902286f1d5..b03dd9dc0ace 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -124,13 +124,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) fpga_fsinfo.dev_part = argv[7]; fpga_fsinfo.filename = argv[8];
- /* Blocksize can be zero */ - if (!fpga_fsinfo.interface || !fpga_fsinfo.dev_part || - !fpga_fsinfo.filename) { - puts("ERR: Wrong interface, dev_part or filename\n"); - return CMD_RET_USAGE; - } - argc = 5; break; #endif

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Parameter checking is dead code because all the time there must be all params assigned. If they are not assigned there is no 9th parameters passed and checking before return CMD_RET_USAGE.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 7 ------- 1 file changed, 7 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

There is no reason to check parameters in separate switch. Check them directly when they are read. Also there is no reason to check loadmk case separately because fpga_data address must be non zero too.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index b03dd9dc0ace..0e5f4117c02e 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -83,7 +83,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) char *devstr = env_get("fpga"); char *datastr = env_get("fpgadata"); int rc = FPGA_FAIL; - int wrong_parms = 0; #if defined(CONFIG_FIT) const char *fit_uname = NULL; ulong fit_addr; @@ -160,7 +159,10 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) switch (argc) { case 5: /* fpga <op> <dev> <data> <datasize> */ data_size = simple_strtoul(argv[4], NULL, 16); - + if (!data_size) { + puts("Zero data_size\n"); + return CMD_RET_USAGE; + } case 4: /* fpga <op> <dev> <data> */ #if defined(CONFIG_FIT) if (fit_parse_subimage(argv[3], (ulong)fpga_data, @@ -177,7 +179,10 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) (ulong)fpga_data); } debug("%s: fpga_data = 0x%lx\n", __func__, (ulong)fpga_data); - + if (!fpga_data) { + puts("Zero fpga_data address\n"); + return CMD_RET_USAGE; + } case 3: /* fpga <op> <dev | data addr> */ dev = (int)simple_strtoul(argv[2], NULL, 16); debug("%s: device = %d\n", __func__, dev); @@ -190,30 +195,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
switch (op) { case FPGA_INFO: - break; - case FPGA_LOAD: - case FPGA_LOADP: - case FPGA_LOADB: - case FPGA_LOADBP: - case FPGA_DUMP: - if (!fpga_data || !data_size) - wrong_parms = 1; - break; -#if defined(CONFIG_CMD_FPGA_LOADMK) - case FPGA_LOADMK: - if (!fpga_data) - wrong_parms = 1; - break; -#endif - } - - if (wrong_parms) { - puts("Wrong parameters for FPGA request\n"); - return CMD_RET_USAGE; - } - - switch (op) { - case FPGA_INFO: rc = fpga_info(dev); break;

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
There is no reason to check parameters in separate switch. Check them directly when they are read. Also there is no reason to check loadmk case separately because fpga_data address must be non zero too.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

Create command wrapper to clean fpga subcommands. The function logic is taken from cmd_dm.c
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 0e5f4117c02e..ac12af2fa06d 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -331,10 +331,48 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return rc; }
+static cmd_tbl_t fpga_commands[] = { +}; + +static int do_fpga_wrapper(cmd_tbl_t *cmdtp, int flag, int argc, + char *const argv[]) +{ + cmd_tbl_t *fpga_cmd; + int ret; + + if (argc < 2) + return CMD_RET_USAGE; + + fpga_cmd = find_cmd_tbl(argv[1], fpga_commands, + ARRAY_SIZE(fpga_commands)); + + /* This should be removed when all functions are converted */ + if (!fpga_cmd) + return do_fpga(cmdtp, flag, argc, argv); + + /* FIXME This can't be reached till all functions are converted */ + if (!fpga_cmd) { + debug("fpga: non existing command\n"); + return CMD_RET_USAGE; + } + + argc -= 2; + argv += 2; + + if (argc > fpga_cmd->maxargs) { + debug("fpga: more parameters passed\n"); + return CMD_RET_USAGE; + } + + ret = fpga_cmd->cmd(fpga_cmd, flag, argc, argv); + + return cmd_process_error(fpga_cmd, ret); +} + #if defined(CONFIG_CMD_FPGA_LOADFS) || defined(CONFIG_CMD_FPGA_LOAD_SECURE) -U_BOOT_CMD(fpga, 9, 1, do_fpga, +U_BOOT_CMD(fpga, 9, 1, do_fpga_wrapper, #else -U_BOOT_CMD(fpga, 6, 1, do_fpga, +U_BOOT_CMD(fpga, 6, 1, do_fpga_wrapper, #endif "loadable FPGA image support", "[operation type] [device number] [image address] [image size]\n"

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Create command wrapper to clean fpga subcommands. The function logic is taken from cmd_dm.c
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

Move fpga info to U_BOOT_CMD_MKENT subcommand. Also use strtol instead of simple_strtoul. The reason is that if -1 is passed (or fpga info without "fpga" variable) the list of all fpgas is shown. This functionality is in the fpga core but it couldn't be performed.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index ac12af2fa06d..039803870b02 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -13,10 +13,26 @@ #include <fs.h> #include <malloc.h>
+static long do_fpga_get_device(char *arg) +{ + long dev = FPGA_INVALID_DEVICE; + char *devstr = env_get("fpga"); + + if (devstr) + /* Should be strtol to handle -1 cases */ + dev = simple_strtol(devstr, NULL, 16); + + if (arg) + dev = simple_strtol(arg, NULL, 16); + + debug("%s: device = %ld\n", __func__, dev); + + return dev; +} + /* Local defines */ enum { FPGA_NONE = -1, - FPGA_INFO, FPGA_LOAD, FPGA_LOADB, FPGA_DUMP, @@ -35,9 +51,7 @@ static int fpga_get_op(char *opstr) { int op = FPGA_NONE;
- if (!strcmp("info", opstr)) - op = FPGA_INFO; - else if (!strcmp("loadb", opstr)) + if (!strcmp("loadb", opstr)) op = FPGA_LOADB; else if (!strcmp("load", opstr)) op = FPGA_LOAD; @@ -194,10 +208,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) }
switch (op) { - case FPGA_INFO: - rc = fpga_info(dev); - break; - case FPGA_LOAD: rc = fpga_load(dev, fpga_data, data_size, BIT_FULL); break; @@ -331,7 +341,16 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return rc; }
+static int do_fpga_info(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + long dev = do_fpga_get_device(argv[0]); + + return fpga_info(dev); +} + static cmd_tbl_t fpga_commands[] = { + U_BOOT_CMD_MKENT(info, 1, 1, do_fpga_info, "", ""), };
static int do_fpga_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Move fpga info to U_BOOT_CMD_MKENT subcommand. Also use strtol instead of simple_strtoul. The reason is that if -1 is passed (or fpga info without "fpga" variable) the list of all fpgas is shown. This functionality is in the fpga core but it couldn't be performed.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

Separate dump, load, loadb, loadp and loadbp commands to separate functions to make it clear how they are called and what parameters they need.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
Maybe they can be still groupped together with one switch/case but it can be done later.
--- cmd/fpga.c | 166 +++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 122 insertions(+), 44 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 039803870b02..9c715db80512 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -30,15 +30,42 @@ static long do_fpga_get_device(char *arg) return dev; }
+static int do_fpga_check_params(long *dev, long *fpga_data, size_t *data_size, + cmd_tbl_t *cmdtp, int argc, char *const argv[]) +{ + size_t local_data_size; + long local_fpga_data; + + debug("%s %d, %d\n", __func__, argc, cmdtp->maxargs); + + if (argc != cmdtp->maxargs) { + debug("fpga: incorrect parameters passed\n"); + return CMD_RET_USAGE; + } + + *dev = do_fpga_get_device(argv[0]); + + local_fpga_data = simple_strtol(argv[1], NULL, 16); + if (!local_fpga_data) { + debug("fpga: zero fpga_data address\n"); + return CMD_RET_USAGE; + } + *fpga_data = local_fpga_data; + + local_data_size = simple_strtoul(argv[2], NULL, 16); + if (!local_data_size) { + debug("fpga: zero size\n"); + return CMD_RET_USAGE; + } + *data_size = local_data_size; + + return 0; +} + /* Local defines */ enum { FPGA_NONE = -1, - FPGA_LOAD, - FPGA_LOADB, - FPGA_DUMP, FPGA_LOADMK, - FPGA_LOADP, - FPGA_LOADBP, FPGA_LOADFS, FPGA_LOADS, }; @@ -51,28 +78,14 @@ static int fpga_get_op(char *opstr) { int op = FPGA_NONE;
- if (!strcmp("loadb", opstr)) - op = FPGA_LOADB; - else if (!strcmp("load", opstr)) - op = FPGA_LOAD; -#if defined(CONFIG_CMD_FPGA_LOADP) - else if (!strcmp("loadp", opstr)) - op = FPGA_LOADP; -#endif -#if defined(CONFIG_CMD_FPGA_LOADBP) - else if (!strcmp("loadbp", opstr)) - op = FPGA_LOADBP; -#endif #if defined(CONFIG_CMD_FPGA_LOADFS) - else if (!strcmp("loadfs", opstr)) + if (!strcmp("loadfs", opstr)) op = FPGA_LOADFS; #endif #if defined(CONFIG_CMD_FPGA_LOADMK) else if (!strcmp("loadmk", opstr)) op = FPGA_LOADMK; #endif - else if (!strcmp("dump", opstr)) - op = FPGA_DUMP; #if defined(CONFIG_CMD_FPGA_LOAD_SECURE) else if (!strcmp("loads", opstr)) op = FPGA_LOADS; @@ -208,26 +221,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) }
switch (op) { - case FPGA_LOAD: - rc = fpga_load(dev, fpga_data, data_size, BIT_FULL); - break; - -#if defined(CONFIG_CMD_FPGA_LOADP) - case FPGA_LOADP: - rc = fpga_load(dev, fpga_data, data_size, BIT_PARTIAL); - break; -#endif - - case FPGA_LOADB: - rc = fpga_loadbitstream(dev, fpga_data, data_size, BIT_FULL); - break; - -#if defined(CONFIG_CMD_FPGA_LOADBP) - case FPGA_LOADBP: - rc = fpga_loadbitstream(dev, fpga_data, data_size, BIT_PARTIAL); - break; -#endif - #if defined(CONFIG_CMD_FPGA_LOADFS) case FPGA_LOADFS: rc = fpga_fsload(dev, fpga_data, data_size, &fpga_fsinfo); @@ -330,10 +323,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) break; #endif
- case FPGA_DUMP: - rc = fpga_dump(dev, fpga_data, data_size); - break; - default: printf("Unknown operation\n"); return CMD_RET_USAGE; @@ -349,8 +338,97 @@ static int do_fpga_info(cmd_tbl_t *cmdtp, int flag, int argc, return fpga_info(dev); }
+static int do_fpga_dump(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + size_t data_size = 0; + long fpga_data, dev; + int ret; + + ret = do_fpga_check_params(&dev, &fpga_data, &data_size, + cmdtp, argc, argv); + if (ret) + return ret; + + return fpga_dump(dev, (void *)fpga_data, data_size); +} + +static int do_fpga_load(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + size_t data_size = 0; + long fpga_data, dev; + int ret; + + ret = do_fpga_check_params(&dev, &fpga_data, &data_size, + cmdtp, argc, argv); + if (ret) + return ret; + + return fpga_load(dev, (void *)fpga_data, data_size, BIT_FULL); +} + +static int do_fpga_loadb(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + size_t data_size = 0; + long fpga_data, dev; + int ret; + + ret = do_fpga_check_params(&dev, &fpga_data, &data_size, + cmdtp, argc, argv); + if (ret) + return ret; + + return fpga_loadbitstream(dev, (void *)fpga_data, data_size, BIT_FULL); +} + +#if defined(CONFIG_CMD_FPGA_LOADP) +static int do_fpga_loadp(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + size_t data_size = 0; + long fpga_data, dev; + int ret; + + ret = do_fpga_check_params(&dev, &fpga_data, &data_size, + cmdtp, argc, argv); + if (ret) + return ret; + + return fpga_load(dev, (void *)fpga_data, data_size, BIT_PARTIAL); +} +#endif + +#if defined(CONFIG_CMD_FPGA_LOADBP) +static int do_fpga_loadbp(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + size_t data_size = 0; + long fpga_data, dev; + int ret; + + ret = do_fpga_check_params(&dev, &fpga_data, &data_size, + cmdtp, argc, argv); + if (ret) + return ret; + + return fpga_loadbitstream(dev, (void *)fpga_data, data_size, + BIT_PARTIAL); +} +#endif + static cmd_tbl_t fpga_commands[] = { U_BOOT_CMD_MKENT(info, 1, 1, do_fpga_info, "", ""), + U_BOOT_CMD_MKENT(dump, 3, 1, do_fpga_dump, "", ""), + U_BOOT_CMD_MKENT(load, 3, 1, do_fpga_load, "", ""), + U_BOOT_CMD_MKENT(loadb, 3, 1, do_fpga_loadb, "", ""), +#if defined(CONFIG_CMD_FPGA_LOADP) + U_BOOT_CMD_MKENT(loadp, 3, 1, do_fpga_loadp, "", ""), +#endif +#if defined(CONFIG_CMD_FPGA_LOADBP) + U_BOOT_CMD_MKENT(loadbp, 3, 1, do_fpga_loadbp, "", ""), +#endif };
static int do_fpga_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Separate dump, load, loadb, loadp and loadbp commands to separate functions to make it clear how they are called and what parameters they need.
Signed-off-by: Michal Simek michal.simek@xilinx.com
Maybe they can be still groupped together with one switch/case but it can be done later.
cmd/fpga.c | 166 +++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 122 insertions(+), 44 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

loadfs
Revert "loafs"
This reverts commit 4ec8e7247396458dca5d0185b5df308df7b0ef2b.
Add loadfs
This reverts commit d6b0e830b8c7a31d4cc87cfd44e70c41417114bb.
Signed-off-by: Michal Simek michal.simek@xilinx.com
fslo`
---
cmd/fpga.c | 58 ++++++++++++++++++++++++++++------------------------------ 1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 9c715db80512..826f63371a66 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -66,7 +66,6 @@ static int do_fpga_check_params(long *dev, long *fpga_data, size_t *data_size, enum { FPGA_NONE = -1, FPGA_LOADMK, - FPGA_LOADFS, FPGA_LOADS, };
@@ -78,12 +77,8 @@ static int fpga_get_op(char *opstr) { int op = FPGA_NONE;
-#if defined(CONFIG_CMD_FPGA_LOADFS) - if (!strcmp("loadfs", opstr)) - op = FPGA_LOADFS; -#endif #if defined(CONFIG_CMD_FPGA_LOADMK) - else if (!strcmp("loadmk", opstr)) + if (!strcmp("loadmk", opstr)) op = FPGA_LOADMK; #endif #if defined(CONFIG_CMD_FPGA_LOAD_SECURE) @@ -114,10 +109,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) const char *fit_uname = NULL; ulong fit_addr; #endif -#if defined(CONFIG_CMD_FPGA_LOADFS) - fpga_fs_info fpga_fsinfo; - fpga_fsinfo.fstype = FS_TYPE_ANY; -#endif #if defined(CONFIG_CMD_FPGA_LOAD_SECURE) struct fpga_secure_info fpga_sec_info;
@@ -140,19 +131,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) case FPGA_NONE: printf("Unknown fpga operation "%s"\n", argv[1]); return CMD_RET_USAGE; -#if defined(CONFIG_CMD_FPGA_LOADFS) - case FPGA_LOADFS: - if (argc < 9) - return CMD_RET_USAGE; - fpga_fsinfo.blocksize = (unsigned int) - simple_strtoul(argv[5], NULL, 16); - fpga_fsinfo.interface = argv[6]; - fpga_fsinfo.dev_part = argv[7]; - fpga_fsinfo.filename = argv[8]; - - argc = 5; - break; -#endif #if defined(CONFIG_CMD_FPGA_LOAD_SECURE) case FPGA_LOADS: if (argc < 7) @@ -221,18 +199,11 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) }
switch (op) { -#if defined(CONFIG_CMD_FPGA_LOADFS) - case FPGA_LOADFS: - rc = fpga_fsload(dev, fpga_data, data_size, &fpga_fsinfo); - break; -#endif - #if defined(CONFIG_CMD_FPGA_LOAD_SECURE) case FPGA_LOADS: rc = fpga_loads(dev, fpga_data, data_size, &fpga_sec_info); break; #endif - #if defined(CONFIG_CMD_FPGA_LOADMK) case FPGA_LOADMK: switch (genimg_get_format(fpga_data)) { @@ -330,6 +301,30 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return rc; }
+#if defined(CONFIG_CMD_FPGA_LOADFS) +static int do_fpga_loadfs(cmd_tbl_t *cmdtp, int flag, int argc, + char *const argv[]) +{ + size_t data_size = 0; + long fpga_data, dev; + int ret; + fpga_fs_info fpga_fsinfo; + + ret = do_fpga_check_params(&dev, &fpga_data, &data_size, + cmdtp, argc, argv); + if (ret) + return ret; + + fpga_fsinfo.fstype = FS_TYPE_ANY; + fpga_fsinfo.blocksize = (unsigned int)simple_strtoul(argv[3], NULL, 16); + fpga_fsinfo.interface = argv[4]; + fpga_fsinfo.dev_part = argv[5]; + fpga_fsinfo.filename = argv[6]; + + return fpga_fsload(dev, (void *)fpga_data, data_size, &fpga_fsinfo); +} +#endif + static int do_fpga_info(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { @@ -429,6 +424,9 @@ static cmd_tbl_t fpga_commands[] = { #if defined(CONFIG_CMD_FPGA_LOADBP) U_BOOT_CMD_MKENT(loadbp, 3, 1, do_fpga_loadbp, "", ""), #endif +#if defined(CONFIG_CMD_FPGA_LOADFS) + U_BOOT_CMD_MKENT(loadfs, 7, 1, do_fpga_loadfs, "", ""), +#endif };
static int do_fpga_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,

Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 240 ++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 125 insertions(+), 115 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 826f63371a66..8237bbc219b4 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -65,7 +65,6 @@ static int do_fpga_check_params(long *dev, long *fpga_data, size_t *data_size, /* Local defines */ enum { FPGA_NONE = -1, - FPGA_LOADMK, FPGA_LOADS, };
@@ -77,12 +76,8 @@ static int fpga_get_op(char *opstr) { int op = FPGA_NONE;
-#if defined(CONFIG_CMD_FPGA_LOADMK) - if (!strcmp("loadmk", opstr)) - op = FPGA_LOADMK; -#endif #if defined(CONFIG_CMD_FPGA_LOAD_SECURE) - else if (!strcmp("loads", opstr)) + if (!strcmp("loads", opstr)) op = FPGA_LOADS; #endif
@@ -102,24 +97,13 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) int op, dev = FPGA_INVALID_DEVICE; size_t data_size = 0; void *fpga_data = NULL; - char *devstr = env_get("fpga"); - char *datastr = env_get("fpgadata"); int rc = FPGA_FAIL; -#if defined(CONFIG_FIT) - const char *fit_uname = NULL; - ulong fit_addr; -#endif #if defined(CONFIG_CMD_FPGA_LOAD_SECURE) struct fpga_secure_info fpga_sec_info;
memset(&fpga_sec_info, 0, sizeof(fpga_sec_info)); #endif
- if (devstr) - dev = (int) simple_strtoul(devstr, NULL, 16); - if (datastr) - fpga_data = (void *)simple_strtoul(datastr, NULL, 16); - if (argc > 9 || argc < 2) { debug("%s: Too many or too few args (%d)\n", __func__, argc); return CMD_RET_USAGE; @@ -169,15 +153,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_USAGE; } case 4: /* fpga <op> <dev> <data> */ -#if defined(CONFIG_FIT) - if (fit_parse_subimage(argv[3], (ulong)fpga_data, - &fit_addr, &fit_uname)) { - fpga_data = (void *)fit_addr; - debug("* fpga: subimage '%s' from FIT image ", - fit_uname); - debug("at 0x%08lx\n", fit_addr); - } else -#endif { fpga_data = (void *)simple_strtoul(argv[3], NULL, 16); debug("* fpga: cmdline image address = 0x%08lx\n", @@ -204,95 +179,6 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) rc = fpga_loads(dev, fpga_data, data_size, &fpga_sec_info); break; #endif -#if defined(CONFIG_CMD_FPGA_LOADMK) - case FPGA_LOADMK: - switch (genimg_get_format(fpga_data)) { -#if defined(CONFIG_IMAGE_FORMAT_LEGACY) - case IMAGE_FORMAT_LEGACY: - { - image_header_t *hdr = - (image_header_t *)fpga_data; - ulong data; - uint8_t comp; - - comp = image_get_comp(hdr); - if (comp == IH_COMP_GZIP) { -#if defined(CONFIG_GZIP) - ulong image_buf = image_get_data(hdr); - data = image_get_load(hdr); - ulong image_size = ~0UL; - - if (gunzip((void *)data, ~0UL, - (void *)image_buf, - &image_size) != 0) { - puts("GUNZIP: error\n"); - return 1; - } - data_size = image_size; -#else - puts("Gunzip image is not supported\n"); - return 1; -#endif - } else { - data = (ulong)image_get_data(hdr); - data_size = image_get_data_size(hdr); - } - rc = fpga_load(dev, (void *)data, data_size, - BIT_FULL); - } - break; -#endif -#if defined(CONFIG_FIT) - case IMAGE_FORMAT_FIT: - { - const void *fit_hdr = (const void *)fpga_data; - int noffset; - const void *fit_data; - - if (fit_uname == NULL) { - puts("No FIT subimage unit name\n"); - return 1; - } - - if (!fit_check_format(fit_hdr)) { - puts("Bad FIT image format\n"); - return 1; - } - - /* get fpga component image node offset */ - noffset = fit_image_get_node(fit_hdr, - fit_uname); - if (noffset < 0) { - printf("Can't find '%s' FIT subimage\n", - fit_uname); - return 1; - } - - /* verify integrity */ - if (!fit_image_verify(fit_hdr, noffset)) { - puts ("Bad Data Hash\n"); - return 1; - } - - /* get fpga subimage data address and length */ - if (fit_image_get_data(fit_hdr, noffset, - &fit_data, &data_size)) { - puts("Fpga subimage data not found\n"); - return 1; - } - - rc = fpga_load(dev, fit_data, data_size, - BIT_FULL); - } - break; -#endif - default: - puts("** Unknown image type\n"); - rc = FPGA_FAIL; - break; - } - break; -#endif
default: printf("Unknown operation\n"); @@ -413,6 +299,127 @@ static int do_fpga_loadbp(cmd_tbl_t *cmdtp, int flag, int argc, } #endif
+#if defined(CONFIG_CMD_FPGA_LOADMK) +static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + size_t data_size = 0; + void *fpga_data = NULL; +#if defined(CONFIG_FIT) + const char *fit_uname = NULL; + ulong fit_addr; +#endif + ulong dev = do_fpga_get_device(argv[0]); + char *datastr = env_get("fpgadata"); + + if (datastr) + fpga_data = (void *)simple_strtoul(datastr, NULL, 16); + + if (argc == 2) { +#if defined(CONFIG_FIT) + if (fit_parse_subimage(argv[1], (ulong)fpga_data, + &fit_addr, &fit_uname)) { + fpga_data = (void *)fit_addr; + debug("* fpga: subimage '%s' from FIT image ", + fit_uname); + debug("at 0x%08lx\n", fit_addr); + } else +#endif + { + fpga_data = (void *)simple_strtoul(argv[1], NULL, 16); + debug("* fpga: cmdline image address = 0x%08lx\n", + (ulong)fpga_data); + } + debug("%s: fpga_data = 0x%lx\n", __func__, (ulong)fpga_data); + if (!fpga_data) { + puts("Zero fpga_data address\n"); + return CMD_RET_USAGE; + } + } + + switch (genimg_get_format(fpga_data)) { +#if defined(CONFIG_IMAGE_FORMAT_LEGACY) + case IMAGE_FORMAT_LEGACY: + { + image_header_t *hdr = (image_header_t *)fpga_data; + ulong data; + u8 comp; + + comp = image_get_comp(hdr); + if (comp == IH_COMP_GZIP) { +#if defined(CONFIG_GZIP) + ulong image_buf = image_get_data(hdr); + ulong image_size = ~0UL; + + data = image_get_load(hdr); + + if (gunzip((void *)data, ~0UL, + (void *)image_buf, + &image_size) != 0) { + puts("GUNZIP: error\n"); + return 1; + } + data_size = image_size; +#else + puts("Gunzip image is not supported\n"); + return 1; +#endif + } else { + data = (ulong)image_get_data(hdr); + data_size = image_get_data_size(hdr); + } + return fpga_load(dev, (void *)data, data_size, + BIT_FULL); + } +#endif +#if defined(CONFIG_FIT) + case IMAGE_FORMAT_FIT: + { + const void *fit_hdr = (const void *)fpga_data; + int noffset; + const void *fit_data; + + if (!fit_uname) { + puts("No FIT subimage unit name\n"); + return 1; + } + + if (!fit_check_format(fit_hdr)) { + puts("Bad FIT image format\n"); + return 1; + } + + /* get fpga component image node offset */ + noffset = fit_image_get_node(fit_hdr, fit_uname); + if (noffset < 0) { + printf("Can't find '%s' FIT subimage\n", + fit_uname); + return 1; + } + + /* verify integrity */ + if (!fit_image_verify(fit_hdr, noffset)) { + puts("Bad Data Hash\n"); + return 1; + } + + /* get fpga subimage data address and length */ + if (fit_image_get_data(fit_hdr, noffset, &fit_data, + &data_size)) { + puts("Fpga subimage data not found\n"); + return 1; + } + + return fpga_load(dev, fit_data, data_size, BIT_FULL); + } +#endif + default: + puts("** Unknown image type\n"); + return FPGA_FAIL; + } +} +#endif + static cmd_tbl_t fpga_commands[] = { U_BOOT_CMD_MKENT(info, 1, 1, do_fpga_info, "", ""), U_BOOT_CMD_MKENT(dump, 3, 1, do_fpga_dump, "", ""), @@ -427,6 +434,9 @@ static cmd_tbl_t fpga_commands[] = { #if defined(CONFIG_CMD_FPGA_LOADFS) U_BOOT_CMD_MKENT(loadfs, 7, 1, do_fpga_loadfs, "", ""), #endif +#if defined(CONFIG_CMD_FPGA_LOADMK) + U_BOOT_CMD_MKENT(loadmk, 2, 1, do_fpga_loadmk, "", ""), +#endif };
static int do_fpga_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,

Use standard return command failure macro.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 8237bbc219b4..5fabfca39a2b 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -357,12 +357,12 @@ static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc, (void *)image_buf, &image_size) != 0) { puts("GUNZIP: error\n"); - return 1; + return CMD_RET_FAILURE; } data_size = image_size; #else puts("Gunzip image is not supported\n"); - return 1; + return CMD_RET_FAILURE; #endif } else { data = (ulong)image_get_data(hdr); @@ -381,12 +381,12 @@ static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc,
if (!fit_uname) { puts("No FIT subimage unit name\n"); - return 1; + return CMD_RET_FAILURE; }
if (!fit_check_format(fit_hdr)) { puts("Bad FIT image format\n"); - return 1; + return CMD_RET_FAILURE; }
/* get fpga component image node offset */ @@ -394,20 +394,20 @@ static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc, if (noffset < 0) { printf("Can't find '%s' FIT subimage\n", fit_uname); - return 1; + return CMD_RET_FAILURE; }
/* verify integrity */ if (!fit_image_verify(fit_hdr, noffset)) { puts("Bad Data Hash\n"); - return 1; + return CMD_RET_FAILURE; }
/* get fpga subimage data address and length */ if (fit_image_get_data(fit_hdr, noffset, &fit_data, &data_size)) { puts("Fpga subimage data not found\n"); - return 1; + return CMD_RET_FAILURE; }
return fpga_load(dev, fit_data, data_size, BIT_FULL); @@ -415,7 +415,7 @@ static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc, #endif default: puts("** Unknown image type\n"); - return FPGA_FAIL; + return CMD_RET_FAILURE; } } #endif

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
Use standard return command failure macro.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/fpga.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

loads
fpga loads
loads use debug instead of puts
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
cmd/fpga.c | 148 +++++++++++++++---------------------------------------------- 1 file changed, 36 insertions(+), 112 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 5fabfca39a2b..b82f1175b01f 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -62,130 +62,57 @@ static int do_fpga_check_params(long *dev, long *fpga_data, size_t *data_size, return 0; }
-/* Local defines */ -enum { - FPGA_NONE = -1, - FPGA_LOADS, -}; - -/* - * Map op to supported operations. We don't use a table since we - * would just have to relocate it from flash anyway. - */ -static int fpga_get_op(char *opstr) -{ - int op = FPGA_NONE; - #if defined(CONFIG_CMD_FPGA_LOAD_SECURE) - if (!strcmp("loads", opstr)) - op = FPGA_LOADS; -#endif - - return op; -} - -/* ------------------------------------------------------------------------- */ -/* command form: - * fpga <op> <device number> <data addr> <datasize> - * where op is 'load', 'dump', or 'info' - * If there is no device number field, the fpga environment variable is used. - * If there is no data addr field, the fpgadata environment variable is used. - * The info command requires no data address field. - */ -int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) +int do_fpga_loads(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) { - int op, dev = FPGA_INVALID_DEVICE; size_t data_size = 0; - void *fpga_data = NULL; - int rc = FPGA_FAIL; -#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) + long fpga_data, dev; + int ret; struct fpga_secure_info fpga_sec_info;
memset(&fpga_sec_info, 0, sizeof(fpga_sec_info)); -#endif
- if (argc > 9 || argc < 2) { - debug("%s: Too many or too few args (%d)\n", __func__, argc); + if (argc < 5) { + debug("fpga: incorrect parameters passed\n"); return CMD_RET_USAGE; }
- op = fpga_get_op(argv[1]); - - switch (op) { - case FPGA_NONE: - printf("Unknown fpga operation "%s"\n", argv[1]); + if (argc == 6) + fpga_sec_info.userkey_addr = (u8 *)(uintptr_t) + simple_strtoull(argv[5], + NULL, 16); + else + /* + * If 6th parameter is not passed then do_fpga_check_params + * will get 5 instead of expected 6 which means that function + * return CMD_RET_USAGE. Increase number of params +1 to pass + * this. + */ + argc++; + + fpga_sec_info.encflag = (u8)simple_strtoul(argv[4], NULL, 16); + fpga_sec_info.authflag = (u8)simple_strtoul(argv[3], NULL, 16); + + if (fpga_sec_info.authflag >= FPGA_NO_ENC_OR_NO_AUTH && + fpga_sec_info.encflag >= FPGA_NO_ENC_OR_NO_AUTH) { + debug("fpga: Use <fpga load> for NonSecure bitstream\n"); return CMD_RET_USAGE; -#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) - case FPGA_LOADS: - if (argc < 7) - return CMD_RET_USAGE; - if (argc == 8) - fpga_sec_info.userkey_addr = (u8 *)(uintptr_t) - simple_strtoull(argv[7], - NULL, 16); - fpga_sec_info.encflag = (u8)simple_strtoul(argv[6], NULL, 16); - fpga_sec_info.authflag = (u8)simple_strtoul(argv[5], NULL, 16); - - if (fpga_sec_info.authflag >= FPGA_NO_ENC_OR_NO_AUTH && - fpga_sec_info.encflag >= FPGA_NO_ENC_OR_NO_AUTH) { - puts("ERR: Use <fpga load> for NonSecure bitstream\n"); - return CMD_RET_USAGE; - } - - if (fpga_sec_info.encflag == FPGA_ENC_USR_KEY && - !fpga_sec_info.userkey_addr) { - puts("ERR: User key not provided\n"); - return CMD_RET_USAGE; - } - - argc = 5; - break; -#endif - default: - break; }
- switch (argc) { - case 5: /* fpga <op> <dev> <data> <datasize> */ - data_size = simple_strtoul(argv[4], NULL, 16); - if (!data_size) { - puts("Zero data_size\n"); - return CMD_RET_USAGE; - } - case 4: /* fpga <op> <dev> <data> */ - { - fpga_data = (void *)simple_strtoul(argv[3], NULL, 16); - debug("* fpga: cmdline image address = 0x%08lx\n", - (ulong)fpga_data); - } - debug("%s: fpga_data = 0x%lx\n", __func__, (ulong)fpga_data); - if (!fpga_data) { - puts("Zero fpga_data address\n"); - return CMD_RET_USAGE; - } - case 3: /* fpga <op> <dev | data addr> */ - dev = (int)simple_strtoul(argv[2], NULL, 16); - debug("%s: device = %d\n", __func__, dev); - } - - if (dev == FPGA_INVALID_DEVICE) { - puts("FPGA device not specified\n"); + if (fpga_sec_info.encflag == FPGA_ENC_USR_KEY && + !fpga_sec_info.userkey_addr) { + debug("fpga: User key not provided\n"); return CMD_RET_USAGE; }
- switch (op) { -#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) - case FPGA_LOADS: - rc = fpga_loads(dev, fpga_data, data_size, &fpga_sec_info); - break; -#endif + ret = do_fpga_check_params(&dev, &fpga_data, &data_size, + cmdtp, argc, argv); + if (ret) + return ret;
- default: - printf("Unknown operation\n"); - return CMD_RET_USAGE; - } - return rc; + return fpga_loads(dev, (void *)fpga_data, data_size, &fpga_sec_info); } +#endif
#if defined(CONFIG_CMD_FPGA_LOADFS) static int do_fpga_loadfs(cmd_tbl_t *cmdtp, int flag, int argc, @@ -437,6 +364,9 @@ static cmd_tbl_t fpga_commands[] = { #if defined(CONFIG_CMD_FPGA_LOADMK) U_BOOT_CMD_MKENT(loadmk, 2, 1, do_fpga_loadmk, "", ""), #endif +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) + U_BOOT_CMD_MKENT(loads, 6, 1, do_fpga_loads, "", ""), +#endif };
static int do_fpga_wrapper(cmd_tbl_t *cmdtp, int flag, int argc, @@ -450,12 +380,6 @@ static int do_fpga_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,
fpga_cmd = find_cmd_tbl(argv[1], fpga_commands, ARRAY_SIZE(fpga_commands)); - - /* This should be removed when all functions are converted */ - if (!fpga_cmd) - return do_fpga(cmdtp, flag, argc, argv); - - /* FIXME This can't be reached till all functions are converted */ if (!fpga_cmd) { debug("fpga: non existing command\n"); return CMD_RET_USAGE;

FPGA subsystem requires special care that's why it should be maintained via one tree.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS index 570bc6d1a525..3bded710b15c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -372,6 +372,14 @@ F: test/py/tests/test_efi* F: cmd/bootefi.c F: tools/file2include.c
+FPGA +M: Michal Simek michal.simek@xilinx.com +S: Maintained +T: git git://git.denx.de/u-boot-microblaze.git +F: drivers/fpga/ +F: cmd/fpga.c +F: include/fpga.h + FLATTENED DEVICE TREE M: Simon Glass sjg@chromium.org S: Maintained

On 18 July 2018 at 09:16, Michal Simek michal.simek@xilinx.com wrote:
FPGA subsystem requires special care that's why it should be maintained via one tree.
Signed-off-by: Michal Simek michal.simek@xilinx.com
MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
participants (2)
-
Michal Simek
-
Simon Glass