Hi Daniel,

On 27.11.20 20:01, Daniel Schwierzeck wrote:
Am Mittwoch, den 28.10.2020, 15:10 +0100 schrieb Stefan Roese:
Add a tool to update or insert an Octeon specific header into the U-Boot
image. This is needed e.g. for booting via SPI NOR, eMMC and NAND.

While working on this, move enum cvmx_board_types_enum and
cvmx_board_type_to_string() to cvmx-bootloader.h and remove the
unreferenced (unsupported) board definition.

Signed-off-by: Stefan Roese <s...@denx.de>
Cc: Aaron Williams <awilli...@marvell.com>
Cc: Chandrakala Chavva <ccha...@marvell.com>
Cc: Daniel Schwierzeck <daniel.schwierz...@gmail.com>
---
v2:
- No change
- Azure build success here:
   https://dev.azure.com/sr0718/u-boot/_build/results?buildId=59&view=results

  .../mach-octeon/include/mach/cvmx-bootinfo.h  | 222 ---------
  .../include/mach/cvmx-bootloader.h            | 172 +++++++
  tools/Makefile                                |   3 +
  tools/update_octeon_header.c                  | 450 ++++++++++++++++++
  4 files changed, 625 insertions(+), 222 deletions(-)
  create mode 100644 arch/mips/mach-octeon/include/mach/cvmx-bootloader.h
  create mode 100644 tools/update_octeon_header.c

...

diff --git a/tools/Makefile b/tools/Makefile
index 51123fd929..253a6b9706 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -206,6 +206,9 @@ hostprogs-y += proftool
  hostprogs-$(CONFIG_STATIC_RELA) += relocate-rela
  hostprogs-$(CONFIG_RISCV) += prelink-riscv
+hostprogs-$(CONFIG_ARCH_OCTEON) += update_octeon_header
+update_octeon_header-objs := update_octeon_header.o lib/crc32.o

entry in tools/.gitignore is missing

+
  hostprogs-y += fdtgrep
  fdtgrep-objs += $(LIBFDT_OBJS) common/fdt_region.o fdtgrep.o
diff --git a/tools/update_octeon_header.c b/tools/update_octeon_header.c
new file mode 100644
index 0000000000..964036216d
--- /dev/null
+++ b/tools/update_octeon_header.c
@@ -0,0 +1,450 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Marvell International Ltd.
+ */
+
+#include <stdio.h>
+#include <stdint.h>
+#include <stddef.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <getopt.h>
+#include <arpa/inet.h>
+
+#include <u-boot/crc.h>
+
+#include "mkimage.h"
+
+#include "../arch/mips/mach-octeon/include/mach/cvmx-bootloader.h"
+
+void usage(void);

this needs some code cleanup:
- drop the forward declarations and reorder the functions when
necessary
- make all functions static
- drop dead code
- drop debug() macros convert the useful ones to printf
- maybe use fprintf(STDERR, ...) for error messages

there are also some bugs:
- tool crashes with segfault when called without arguments
- tool crashes when called with a long option but no value
- the bugs above will also be detected by clang-tidy, suggestions for
fixing below

Many thanks for the extensive code review. I'll try to address all
comments shortly.

Thanks,
Stefan

+
+/*
+ * Do some funky stuff here to get a compile time error if the size of the
+ * bootloader_header structure has changed.
+ */
+#define compile_time_assert(cond, msg) char msg[(cond) ? 1 : -1]
+compile_time_assert(sizeof(struct bootloader_header) == 192,
+                   bootloader_header_size_changed);

there is BUILD_BUG_ON_MSG() in linux/build_bug.h
or compiletime_assert() in linux/compiler.h

+
+#define BUF_SIZE       (16 * 1024)
+#define NAME_LEN       100
+
+#ifndef WOFFSETOF
+/* word offset */
+#define WOFFSETOF(type, elem)  (offsetof(type, elem) / 4)
+#endif
+
+#define cvmx_cpu_to_be64(x)    (x)
+
+static int stage2_flag;
+static int stage_1_5_flag;
+static int stage_1_flag;
+
+int lookup_board_type(char *board_name)
+{
+       int i;
+       int board_type = 0;
+       char *substr = NULL;
+
+       /* Detect stage 2 bootloader boards */
+       if (strcasestr(board_name, "_stage2")) {
+               debug("Stage 2 bootloader detected from substring %s in name 
%s\n",
+                     "_stage2", board_name);
+               stage2_flag = 1;
+       } else {
+               debug("Stage 2 bootloader NOT detected from name \"%s\"\n",
+                     board_name);
+       }
+
+       if (strcasestr(board_name, "_stage1")) {
+               debug("Stage 1 bootloader detected from substring %s in name 
%s\n",
+                     "_stage1", board_name);
+               stage_1_flag = 1;
+       }
+
+       /* Generic is a special case since there are numerous sub-types */
+       if (!strncasecmp("generic", board_name, strlen("generic")))
+               return CVMX_BOARD_TYPE_GENERIC;
+
+       /*
+        * If we're an eMMC stage 2 bootloader, cut off the _emmc_stage2
+        * part of the name.
+        */
+       substr = strcasestr(board_name, "_emmc_stage2");
+       if (substr && (substr[strlen("_emmc_stage2")] == '\0')) {
+               /*return CVMX_BOARD_TYPE_GENERIC;*/
+
+               printf("  Converting board name %s to ", board_name);
+               *substr = '\0';
+               printf("%s\n", board_name);
+       }
+
+       /*
+        * If we're a NAND stage 2 bootloader, cut off the _nand_stage2
+        * part of the name.
+        */
+       substr = strcasestr(board_name, "_nand_stage2");
+       if (substr && (substr[strlen("_nand_stage2")] == '\0')) {
+               /*return CVMX_BOARD_TYPE_GENERIC;*/
+
+               printf("  Converting board name %s to ", board_name);
+               *substr = '\0';
+               printf("%s\n", board_name);
+       }
+
+       /*
+        * If we're a SPI stage 2 bootloader, cut off the _spi_stage2
+        * part of the name.
+        */
+       substr = strcasestr(board_name, "_spi_stage2");
+       if (substr && (substr[strlen("_spi_stage2")] == '\0')) {
+               printf("  Converting board name %s to ", board_name);
+               *substr = '\0';
+               printf("%s\n", board_name);
+       }
+
+       for (i = CVMX_BOARD_TYPE_NULL; i < CVMX_BOARD_TYPE_MAX; i++)
+               if (!strcasecmp(cvmx_board_type_to_string(i), board_name))
+                       board_type = i;
+
+       for (i = CVMX_BOARD_TYPE_CUST_DEFINED_MIN;
+            i < CVMX_BOARD_TYPE_CUST_DEFINED_MAX; i++)
+               if (!strncasecmp(cvmx_board_type_to_string(i), board_name,
+                                strlen(cvmx_board_type_to_string(i))))
+                       board_type = i;
+
+       for (i = CVMX_BOARD_TYPE_CUST_PRIVATE_MIN;
+            i < CVMX_BOARD_TYPE_CUST_PRIVATE_MAX; i++)
+               if (!strncasecmp(cvmx_board_type_to_string(i), board_name,
+                                strlen(cvmx_board_type_to_string(i))))
+                       board_type = i;
+
+       return board_type;
+}
+
+/* Getoptions variables must be global */
+static int failsafe_flag;
+static int pciboot_flag;
+static int env_flag;
+
+int main(int argc, char *argv[])
+{
+       int fd;
+       uint8_t buf[BUF_SIZE];
+       uint32_t data_crc = 0;
+       int len;
+       int data_len = 0;
+       struct bootloader_header header;
+       char filename[NAME_LEN];
+       int i;
+       int option_index = 0;   /* getopt_long stores the option index here. */
+       char board_name[NAME_LEN] = { 0 };
+       char tmp_board_name[NAME_LEN] = { 0 };
+       int c;
+       int board_type = 0;
+       unsigned long long address = 0;
+       ssize_t ret;
+       const char *type_str = NULL;
+       int hdr_size = sizeof(struct bootloader_header);

the tool has two positional arguments, so you should bail out early.
This will also fix the segfault

         if (argc < 3) {
                usage();
                return -1;
        }

+
+       debug("header size is: %d bytes\n", hdr_size);
+
+       /* Parse command line options using getopt_long */
+       while (1) {
+               static struct option long_options[] = {
+                       /* These options set a flag. */
+                       {"failsafe", no_argument, &failsafe_flag, 1},
+                       {"pciboot", no_argument, &pciboot_flag, 1},
+                       {"nandstage2", no_argument, &stage2_flag, 1},
+                       {"spistage2", no_argument, &stage2_flag, 1},
+                       {"norstage2", no_argument, &stage2_flag, 1},
+                       {"stage2", no_argument, &stage2_flag, 1},
+                       {"stage1.5", no_argument, &stage_1_5_flag, 1},
+                       {"stage1", no_argument, &stage_1_flag, 1},
+                       {"environment", no_argument, &env_flag, 1},
+                       /* These options don't set a flag.
+                        * We distinguish them by their indices.
+                        */
+                       {"board", required_argument, 0, 0},
+                       {"text_base", required_argument, 0, 0},
+                       {0, 0, 0, 0}
+               };

this should be moved out of main() and marked as static const

+
+               c = getopt_long(argc, (char *const *)argv, "h",

cast is not needed

+                               long_options, &option_index);
+
+               /* Detect the end of the options. */
+               if (c == -1)
+                       break;
+
+               switch (c) {
+                       /* All long options handled in case 0 */
+               case 0:
+                       /* If this option set a flag, do nothing else now. */
+                       if (long_options[option_index].flag != 0)
+                               break;
+                       debug("option(l) %s", long_options[option_index].name);
+                       if (optarg)
+                               debug(" with arg %s", optarg);
+                       debug("\n");

should be

                         if (!optarg) {
                                usage();
                                return -1;
                        }
                        debug(" with arg %s\n", optarg);

the code below depends on optarg and passing a long option without
value is an error and should be notified to the user

+
+                       if (!strcmp(long_options[option_index].name, "board")) {
+                               if (strlen(optarg) >= NAME_LEN) {
+                                       printf("strncpy() issue detected!");
+                                       exit(-1);
+                               }
+                               strncpy(board_name, optarg, NAME_LEN);
+
+                               debug("Using user supplied board name: %s\n",
+                                     board_name);
+                       } else if (!strcmp(long_options[option_index].name,
+                                          "text_base")) {
+                               address = strtoull(optarg, NULL, 0);
+                               debug("Address of image is: 0x%llx\n",
+                                     (unsigned long long)address);
+                               if (!(address & 0xFFFFFFFFULL << 32)) {
+                                       if (address & 1 << 31) {
+                                               address |= 0xFFFFFFFFULL << 32;
+                                               debug("Converting address to 64 bit 
compatibility space: 0x%llx\n",
+                                                     address);
+                                       }
+                               }
+                       }
+                       break;
+
+               case 'h':
+               case '?':
+                       /* getopt_long already printed an error message. */
+                       usage();
+                       return -1;
+
+               default:
+                       abort();
+               }
+       }
+
+       if (optind < argc) {
+               /*
+                * We only support one argument - an optional bootloader
+                * file name
+                */
+               if (argc - optind > 2) {
+                       debug("non-option ARGV-elements: ");
+                       while (optind < argc)
+                               debug("%s ", argv[optind++]);
+                       debug("\n");
+                       usage();
+                       return -1;
+               }
+       }
+
+       if (strlen(argv[optind]) >= NAME_LEN) {
+               printf("strncpy() issue detected!");
+               exit(-1);
+       }
+       strncpy(filename, argv[optind], NAME_LEN);
+
+       if (board_name[0] == '\0') {
+               if (strlen(argv[optind + 1]) >= NAME_LEN) {
+                       printf("strncpy() issue detected!");
+                       exit(-1);
+               }
+               strncpy(board_name, argv[optind + 1], NAME_LEN);
+       }
+
+       if (strlen(board_name) >= NAME_LEN) {
+               printf("strncpy() issue detected!");
+               exit(-1);
+       }
+       strncpy(tmp_board_name, board_name, NAME_LEN);
+
+       fd = open(filename, O_RDWR);
+       if (fd < 0) {
+               printf("Unable to open file: %s\n", filename);
+               exit(-1);
+       }
+
+       if (failsafe_flag)
+               debug("Setting failsafe flag\n");
+
+       if (strlen(board_name)) {
+               int offset = 0;
+
+               debug("Supplied board name of: %s\n", board_name);
+
+               if (strstr(board_name, "failsafe")) {
+                       failsafe_flag = 1;
+                       debug("Setting failsafe flag based on board name\n");
+               }
+               /* Skip leading octeon_ if present. */
+               if (!strncmp(board_name, "octeon_", 7))
+                       offset = 7;
+
+               /*
+                * Check to see if 'failsafe' is in the name.  If so, set the
+                * failsafe flag.  Also, ignore extra trailing characters on
+                * passed parameter when comparing against board names.
+                * We actually use the configuration name from u-boot, so it
+                * may have some other variant names.  Variants other than
+                * failsafe _must_ be passed to this program explicitly
+                */
+
+               board_type = lookup_board_type(board_name + offset);
+               if (!board_type) {
+                       /* Retry with 'cust_' prefix to catch boards that are
+                        * in the customer section (such as nb5)
+                        */
+                       sprintf(tmp_board_name, "cust_%s", board_name + offset);
+                       board_type = lookup_board_type(tmp_board_name);
+               }
+
+               /* reset to original value */
+               strncpy(tmp_board_name, board_name, NAME_LEN);
+               if (!board_type) {
+                       /*
+                        * Retry with 'cust_private_' prefix to catch boards
+                        * that are in the customer private section
+                        */
+                       sprintf(tmp_board_name, "cust_private_%s",
+                               board_name + offset);
+                       board_type = lookup_board_type(tmp_board_name);
+               }
+
+               if (!board_type) {
+                       printf("ERROR: unable to determine board type\n");
+                       exit(-1);
+               }
+               debug("Board type is: %d: %s\n", board_type,
+                     cvmx_board_type_to_string(board_type));
+       } else {
+               printf("Board name must be specified!\n");
+               exit(-1);
+       }
+
+       /*
+        * Check to see if there is either an existing header, or that there
+        * are zero valued bytes where we want to put the header
+        */
+       len = read(fd, buf, BUF_SIZE);
+       if (len > 0) {
+               /*
+                * Copy the header, as the first word (jump instruction, needs
+                * to remain the same.
+                */
+               memcpy(&header, buf, hdr_size);
+               /*
+                * Check to see if we have zero bytes (excluding first 4, which
+                * are the jump instruction)
+                */
+               for (i = 1; i < hdr_size / 4; i++) {
+                       if (((uint32_t *)buf)[i]) {
+                               printf("ERROR: non-zero word found %x in location %d 
required for header, aborting\n",
+                                      ((uint32_t *)buf)[i], i);
+                               exit(-1);
+                       }
+               }
+               debug("Zero bytes found in header location, adding header.\n");
+
+       } else {
+               printf("Unable to read from file %s\n", filename);
+               exit(-1);
+       }
+
+       /* Read data bytes and generate CRC */
+       lseek(fd, hdr_size, SEEK_SET);
+
+       while ((len = read(fd, buf, BUF_SIZE)) > 0) {
+               data_crc = crc32(data_crc, buf, len);
+               data_len += len;
+       }
+       printf("CRC of data: 0x%x, length: %d\n", data_crc, data_len);
+
+       /* Now create the new header */
+       header.magic = htonl(BOOTLOADER_HEADER_MAGIC);
+       header.maj_rev = htons(BOOTLOADER_HEADER_CURRENT_MAJOR_REV);
+       header.min_rev = htons(BOOTLOADER_HEADER_CURRENT_MINOR_REV);
+       header.dlen = htonl(data_len);
+       header.dcrc = htonl(data_crc);
+       header.board_type = htons(board_type);
+       header.address = cvmx_cpu_to_be64(address);

cvmx_cpu_to_be64() is a no-op, is this intentional? If yes, this macro
can be removed. Otherwise you can use cpu_to_be64(). All cpu_to_be*()
functions are automatically available in tools/ directory.




Viele Grüße,
Stefan

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de

Reply via email to