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
