Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No way to address a particular FTDI for OpenOCD #22543

Closed
abrodkin opened this issue Feb 6, 2020 · 7 comments
Closed

No way to address a particular FTDI for OpenOCD #22543

abrodkin opened this issue Feb 6, 2020 · 7 comments
Assignees
Labels
Enhancement Changes/Updates/Additions to existing features

Comments

@abrodkin
Copy link
Collaborator

abrodkin commented Feb 6, 2020

OpenOCD runner works perfectly fine for boards with FTDI-based JTAGs while there's only 1 board attached to the host. If multiple similar or the same boards are attached openocd cannot figure out which JTAG probe to use as typically OpenOCD config only filters FTDI devices by USB VID/PID.

One solution to the problem might be in specifying FTDI's serial number via OpenOCD's command like that: ftdi_serial 251642517567. But that this command needs to be executed in between of FTDI interface creation:

interface ftdi

and TAP instantiation

jtag newtap ...

i.e. typically in the middle of board's openocd.cfg. So ideally we just put that command right there in openocd.cfg. But given that serial number is [at least supposed to be] unique we cannot hardcode any value once and for all. Ok, we'll use internal variable, so we'll add something like that:

interface ftdi
ftdi_serial $_FTDI_SERIAL
ftdi_vid_pid 0x0403 0x6010

Ok, but the next question is how to set this new _FTDI_SERIAL variable? That's easy, just issue command like that set _FTDI_SERIAL 251642517567, i.e. on openocd invocation we may do -c 'set _FTDI_SERIAL 251642517567'.

Now looking at OpenOCD runner (scripts/west_commands/runners/openocd.py) I see that it's not obvious how to squeeze yet another command in between openocd binary itself & openocd.cfg.
The first thing that comes to my mind is to add yet another option like "precfg_cmd" and add it like that

diff --git a/boards/arc/iotdk/support/openocd.cfg b/boards/arc/iotdk/support/openocd.cfg
index 045ce07b46..a611af484f 100644
--- a/boards/arc/iotdk/support/openocd.cfg
+++ b/boards/arc/iotdk/support/openocd.cfg
@@ -9,6 +9,7 @@
 # Configure JTAG cable
 # IoT DK has built-in FT2232 chip, which is similar to Digilent HS-1.
 interface ftdi
+ftdi_serial $_FTDI_SERIAL
 ftdi_vid_pid 0x0403 0x6010
 ftdi_layout_init 0x0088 0x008b
 ftdi_channel 1

diff --git a/scripts/west_commands/runners/openocd.py b/scripts/west_commands/runners/openocd.py
index 23e01e3003..368407879b 100644
--- a/scripts/west_commands/runners/openocd.py
+++ b/scripts/west_commands/runners/openocd.py
@@ -18,7 +18,7 @@ class OpenOcdBinaryRunner(ZephyrBinaryRunner):

     def __init__(self, cfg, pre_init=None, pre_load=None,
                  load_cmd=None, verify_cmd=None, post_verify=None,
-                 tui=None, config=None,
+                 tui=None, config=None, precfg_cmd=None,
                  tcl_port=DEFAULT_OPENOCD_TCL_PORT,
                  telnet_port=DEFAULT_OPENOCD_TELNET_PORT,
                  gdb_port=DEFAULT_OPENOCD_GDB_PORT):
@@ -50,6 +50,7 @@ class OpenOcdBinaryRunner(ZephyrBinaryRunner):
         self.gdb_port = gdb_port
         self.gdb_cmd = [cfg.gdb] if cfg.gdb else None
         self.tui_arg = ['-tui'] if tui else []
+        self.precfg_cmd = precfg_cmd

     @classmethod
     def name(cls):
@@ -59,6 +60,8 @@ class OpenOcdBinaryRunner(ZephyrBinaryRunner):
     def do_add_parser(cls, parser):
         parser.add_argument('--config',
                             help='if given, override default config file')
+        parser.add_argument('--precfg-cmd', default="",
+                            help='if given, executs before reading cfg, defaults to empty')
         # Options for flashing:
         parser.add_argument('--cmd-pre-init', action='append',
                             help='''Command to run before calling init;
@@ -93,7 +96,7 @@ class OpenOcdBinaryRunner(ZephyrBinaryRunner):
             pre_init=args.cmd_pre_init,
             pre_load=args.cmd_pre_load, load_cmd=args.cmd_load,
             verify_cmd=args.cmd_verify, post_verify=args.cmd_post_verify,
-            tui=args.tui, config=args.config,
+            tui=args.tui, config=args.config, precfg_cmd=args.precfg_cmd,
             tcl_port=args.tcl_port, telnet_port=args.telnet_port,
             gdb_port=args.gdb_port)

@@ -138,7 +141,7 @@ class OpenOcdBinaryRunner(ZephyrBinaryRunner):
             post_verify_cmd.append("-c")
             post_verify_cmd.append(i)

-        cmd = (self.openocd_cmd + self.cfg_cmd +
+        cmd = (self.openocd_cmd + [self.precfg_cmd] + self.cfg_cmd +
                pre_init_cmd + ['-c', 'init',
                                 '-c', 'targets'] +
                pre_load_cmd + ['-c', 'reset halt',
@@ -161,7 +164,7 @@ class OpenOcdBinaryRunner(ZephyrBinaryRunner):
             pre_init_cmd.append("-c")
             pre_init_cmd.append(i)

-        server_cmd = (self.openocd_cmd + self.cfg_cmd +
+        server_cmd = (self.openocd_cmd + [self.precfg_cmd] + self.cfg_cmd +
                       ['-c', 'tcl_port {}'.format(self.tcl_port),
                        '-c', 'telnet_port {}'.format(self.telnet_port),
                        '-c', 'gdb_port {}'.format(self.gdb_port)] +

Now I at least may specify desired FTDI like that:

west debug --precfg-cmd="-c set _FTDI_SERIAL 251642517567"

Frankly I don't quite like that hack so if there're any better suggestions I'll be happy to discuss it.

@abrodkin abrodkin added the Enhancement Changes/Updates/Additions to existing features label Feb 6, 2020
@galak
Copy link
Collaborator

galak commented Feb 6, 2020

@vanti did you deal with something similar to this?

@mbolivar
Copy link
Contributor

mbolivar commented Feb 6, 2020

Frankly I don't quite like that hack so if there're any better suggestions I'll be happy to discuss it.

The openocd runner has gotten really overgrown over time. It was basically ported from shell to Python and then evolved organically since. It would be nice to see a cleanup of the way it works instead of this proliferation of options, but I personally am not going to work on that anytime soon, as it'd be spare time effort for me :).

So I too think suggestions are welcome.

@abrodkin
Copy link
Collaborator Author

abrodkin commented Feb 6, 2020

@mbolivar @galak so should I create a PR with proposed hack improvement?
Or you'd prefer a clean room implementation of openocd runner?
But given my experience with Python (or rather its complete absence) it might take quite some time, so keep that in mind :)

@vanti
Copy link
Collaborator

vanti commented Feb 6, 2020

I have dealt with something similar in the context of LAVA, where I want to have multiple boards connected. The way I dealt with it was to create an additional .cfg file, board_selection.cfg, with the command:

xds110_serial <serial number>

The TI device that I dealt with uses "xds110_serial" instead of "ftdi_serial". The serial number is set as part of a LAVA board file that configures the board. Then, when running the openocd command, I added an additional command line parameter

openocd -f ti_cc13x2_launchpad.cfg -f board_selection.cfg -c ...

to essentially augment the default config with the additional statement. However, note that LAVA invokes openocd directly, not through 'west', nor does it use openocd.cfg.

The proposed approach here doesn't necessitate a temporary board_selection.cfg file to be created, which is good. I wonder if exposing "--precfg-cmd" and letting the user figure out the command isn't a little unfriendly and prone to typos. How about just running

west debug --serial="324"

and standardizing to passing a variable named $_ZEPHYR_BOARD_SERIAL with the number to openocd.cfg? Then openocd.cfg can just invoke

ftdi_serial $_ZEPHYR_BOARD_SERIAL
or
xds110_serial $_ZEPHYR_BOARD_SERIAL

depending on the platform.

@abrodkin
Copy link
Collaborator Author

abrodkin commented Feb 6, 2020

@vanti thanks for your input!

In fact the first thing that I wanted to do was exactly west debug --serial="324" but then I thought something more universal might make more sense. See what I do I just populate some variables which might be not only WHATEVER_SERIAL but anything else.

Also since west already perfectly captures and reports that serial when creating map.yml we may not even pass anything but rely on whatever availble in map.yml. Though that might not be good if we have more than one board/platform attached to the same host (which IMHO might make perfect sense if we run multiple verifications in parallel, like Zepyr's master branch and some topic branches at the same time).

Anyways if we're in agreement with use of _ZEPHYR_BOARD_SERIAL and west's --serial="324" I may draft a PR the first thing tomorrow.

abrodkin added a commit to foss-for-synopsys-dwc-arc-processors/zephyr that referenced this issue Feb 12, 2020
To be used in setups with multiple boards attached to the same one
host we need to have an ability to specify precisely which JTAG probe
to use for a particular board.

This is done by passing "ftdi_serial XXX" command to OpenOCD.
And the serial ("XXX") is supposed to be passed from higher level,
typically via west's options. And exactly for that we add another
"openocd" runner option "--serial=XXX" which sets
a Tcl's "_ZEPHYR_BOARD_SERIAL" variable that later gets passed
to OpenOCD's "ftdi_serial" command.

See more discussions on the matter here:
zephyrproject-rtos#22543

Signed-off-by: Alexey Brodkin <[email protected]>
jhedberg pushed a commit that referenced this issue Feb 13, 2020
To be used in setups with multiple boards attached to the same one
host we need to have an ability to specify precisely which JTAG probe
to use for a particular board.

This is done by passing "ftdi_serial XXX" command to OpenOCD.
And the serial ("XXX") is supposed to be passed from higher level,
typically via west's options. And exactly for that we add another
"openocd" runner option "--serial=XXX" which sets
a Tcl's "_ZEPHYR_BOARD_SERIAL" variable that later gets passed
to OpenOCD's "ftdi_serial" command.

See more discussions on the matter here:
#22543

Signed-off-by: Alexey Brodkin <[email protected]>
@mbolivar
Copy link
Contributor

@abrodkin I'm closing as I think this is done. Please reopen if you disagree.

@abrodkin
Copy link
Collaborator Author

Indeed both implementation (5a4237e) and example (946ed0e) are in the main tree, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

4 participants