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

> +
> +/*
> + * 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.


-- 
- Daniel

Reply via email to