intel_adsp: remove rimage sign() from west flash

`west sign` has been invoked by `west build` (through CMake) since
commit fad2da3, almost one year ago. During that time, this new
workflow has been refined and successfully used by at least two vendors,
multiple CIs across both SOF and Zephyr and many developers.

At the time, the ability to sign from `west flash` was preserved for
backwards compatibility. This means rimage parameters can come from many
different places at once and that rimage can be invoked twice during a
single `west flash` invocation!

Now that Zephyr 3.5 has been released, we need to reduce the number of
rimage use cases and the corresponding validation complexity and
maintenance workload to simplify and accelerate new features like
splitting rimage configuration files (#65411)

Signed-off-by: Marc Herbert <[email protected]>

boards/xtensa/intel_adsp_ace15_mtpm/board.cmake

@@ -6,4 +6,4 @@ board_set_rimage_target(mtl)
 
 set(RIMAGE_SIGN_KEY "otc_private_key_3k.pem" CACHE STRING "default in ace15_mtpm/board.cmake")
 
-include(${ZEPHYR_BASE}/boards/common/intel_adsp.board.cmake)
+board_finalize_runner_args(intel_adsp)

boards/xtensa/intel_adsp_cavs25/board.cmake

@@ -17,4 +17,4 @@ if(CONFIG_BOARD_INTEL_ADSP_CAVS25_TGPH)
 board_set_rimage_target(tgl-h)
 endif()
 
-include(${ZEPHYR_BASE}/boards/common/intel_adsp.board.cmake)
+board_finalize_runner_args(intel_adsp)

boards/xtensa/intel_adsp_cavs25/doc/intel_adsp_generic.rst

@@ -143,12 +143,10 @@ undocumented rimage precedence rules it's best to use only one way at a time.
   ``boards/my/board/board.cmake``, see example in
   ``soc/xtensa/intel_adsp/common/CMakeLists.txt``
 
-For backwards compatibility reasons, you can also pass rimage parameters like
-the path to the tool binary as arguments to
-``west flash`` if the flash target exists for your board. To see a list
-of all arguments to the Intel ADSP runner, run the following after you have
-built the binary. There are multiple arguments related to signing, including a
-key argument.
+Starting with Zephyr 3.6.0, ``west flash`` does not invoke ``west sign``
+anymore and you cannot pass rimage parameters to ``west flash`` anymore. To
+see an up-to-date list of all arguments to the Intel ADSP runner, run the
+following after you have built the binary:
 
 .. code-block:: console
 

scripts/west_commands/runners/intel_adsp.py

@@ -4,6 +4,7 @@
 
 '''Runner for flashing with the Intel ADSP boards.'''
 
+import argparse
 import os
 import sys
 import re
@@ -14,44 +15,32 @@
 from zephyr_ext_common import ZEPHYR_BASE
 
 DEFAULT_CAVSTOOL='soc/xtensa/intel_adsp/tools/cavstool_client.py'
-DEFAULT_SOF_MOD_DIR=os.path.join(ZEPHYR_BASE, '../modules/audio/sof')
-DEFAULT_RIMAGE_TOOL=shutil.which('rimage')
-DEFAULT_CONFIG_DIR=os.path.join(DEFAULT_SOF_MOD_DIR, 'tools/rimage/config')
-DEFAULT_KEY_DIR=os.path.join(DEFAULT_SOF_MOD_DIR, 'keys')
 
+class SignParamError(argparse.Action):
+    'User-friendly feedback when trying to sign with west flash'
+    def __call__(self, parser, namespace, values, option_string=None):
+        parser.error(f'Cannot use "west flash {option_string} ..." any more. ' +
+                     '"west sign" is now called from CMake, see "west sign -h"')
 
 class IntelAdspBinaryRunner(ZephyrBinaryRunner):
     '''Runner front-end for the intel ADSP boards.'''
 
     def __init__(self,
                  cfg,
                  remote_host,
-                 rimage_tool,
-                 config_dir,
-                 default_key,
-                 key,
                  pty,
                  tool_opt,
-                 do_sign,
                  ):
         super().__init__(cfg)
 
         self.remote_host = remote_host
-        self.rimage_tool = rimage_tool
-        self.config_dir = config_dir
         self.bin_fw = os.path.join(cfg.build_dir, 'zephyr', 'zephyr.ri')
 
         self.cavstool = os.path.join(ZEPHYR_BASE, DEFAULT_CAVSTOOL)
         self.platform = os.path.basename(cfg.board_dir)
         self.pty = pty
 
-        if key:
-            self.key = key
-        else:
-            self.key = os.path.join(DEFAULT_KEY_DIR, default_key)
-
         self.tool_opt_args = tool_opt
-        self.do_sign = do_sign
 
     @classmethod
     def name(cls):
@@ -65,72 +54,39 @@ def capabilities(cls):
     def do_add_parser(cls, parser):
         parser.add_argument('--remote-host',
                             help='hostname of the remote targeting ADSP board')
-        parser.add_argument('--rimage-tool', default=DEFAULT_RIMAGE_TOOL,
-                            help='path to the rimage tool')
-        parser.add_argument('--config-dir', default=DEFAULT_CONFIG_DIR,
-                            help='path to the toml config file')
-        parser.add_argument('--default-key',
-                            help='the default basename of the key store in board.cmake')
-        parser.add_argument('--key',
-                            help='specify where the signing key is')
         parser.add_argument('--pty', nargs='?', const="remote-host", type=str,
                             help=''''Capture the output of cavstool.py running on --remote-host \
                             and stream it remotely to west's standard output.''')
 
+        for old_sign_param in [ '--rimage-tool', '--config-dir', '--default-key', '--key']:
+            parser.add_argument(old_sign_param, action=SignParamError,
+                            help='do not use, "west sign" is now called from CMake, see "west sign -h"')
+
     @classmethod
     def tool_opt_help(cls) -> str:
         return """Additional options for run/request service tool,
         e.g. '--lock' """
 
     @classmethod
     def do_create(cls, cfg, args):
-        # We now have `west flash` -> `west build` -> `west sign` so
-        # `west flash` -> `west sign` is not needed anymore; it's also
-        # slower because not concurrent. However, for backwards
-        # compatibility keep signing here if some explicit rimage
-        # --option was passed. Some of these options may differ from the
-        # current `west sign` configuration; we take "precedence" by
-        # running last.
-        do_sign = (
-            args.rimage_tool != DEFAULT_RIMAGE_TOOL or
-            args.config_dir != DEFAULT_CONFIG_DIR or
-            args.key is not None
-        )
         return IntelAdspBinaryRunner(cfg,
                                     remote_host=args.remote_host,
-                                    rimage_tool=args.rimage_tool,
-                                    config_dir=args.config_dir,
-                                    default_key=args.default_key,
-                                    key=args.key,
                                     pty=args.pty,
                                     tool_opt=args.tool_opt,
-                                    do_sign=do_sign,
                                     )
 
     def do_run(self, command, **kwargs):
         self.logger.info('Starting Intel ADSP runner')
 
-        if self.do_sign:
-            self.sign(**kwargs)
-
         if re.search("intel_adsp", self.platform):
             self.require(self.cavstool)
             self.flash(**kwargs)
         else:
             self.logger.error("No suitable platform for running")
             sys.exit(1)
 
-    def sign(self, **kwargs):
-        path_opt = ['-p', f'{self.rimage_tool}'] if self.rimage_tool else []
-        sign_cmd = (
-            ['west', 'sign', '-d', f'{self.cfg.build_dir}', '-t', 'rimage']
-            + path_opt + ['-D', f'{self.config_dir}', '--', '-k', f'{self.key}']
-        )
-        self.logger.info(" ".join(sign_cmd))
-        self.check_call(sign_cmd)
-
     def flash(self, **kwargs):
-        # Generate a hash string for appending to the sending ri file
+        'Generate a hash string for appending to the sending ri file'
         hash_object = hashlib.md5(self.bin_fw.encode())
         random_str = f"{random.getrandbits(64)}".encode()
         hash_object.update(random_str)