[PATCH v1 3/3] cmd: bcm: add broadcom error log setup command
Simon Glass
sjg at chromium.org
Mon May 25 19:03:53 CEST 2020
Hi Rayagonda,
On Sun, 17 May 2020 at 02:33, Rayagonda Kokatanur
<rayagonda.kokatanur at broadcom.com> wrote:
>
> From: Vladimir Olovyannikov <vladimir.olovyannikov at broadcom.com>
>
> Add broadcom error log setup command.
>
> Some Broadcom platforms have ability to record event logs by SCP.
> - Add a logsetup command which is used to perform initial configuration
> of this log.
> Move this command to bcm/ directory to be used for Broadcom-specific
> U-boot commands.
> - Add support for Broadcom-specific commands to Kconfig and to Makefile
> in cmd/.
>
> Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov at broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com>
> ---
> board/broadcom/bcmns3/Kconfig | 1 +
> cmd/Kconfig | 2 +
> cmd/Makefile | 2 +
> cmd/bcm/Kconfig | 12 +
> cmd/bcm/Makefile | 4 +
> cmd/bcm/elog.h | 64 +++++
> cmd/bcm/logsetup.c | 432 ++++++++++++++++++++++++++++++++++
> 7 files changed, 517 insertions(+)
> create mode 100644 cmd/bcm/Kconfig
> create mode 100644 cmd/bcm/Makefile
> create mode 100644 cmd/bcm/elog.h
> create mode 100644 cmd/bcm/logsetup.c
[..]
> diff --git a/cmd/bcm/elog.h b/cmd/bcm/elog.h
> new file mode 100644
> index 0000000000..cc36d8739a
> --- /dev/null
> +++ b/cmd/bcm/elog.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2020 Broadcom
> + */
> +
> +#ifndef __ELOG_H__
> +#define __ELOG_H__
> +
> +#define GLOBAL_META_HDR_SIG 0x45524c47
> +#define MAX_REC_COUNT 13
> +#define MAX_REC_FORMAT 1
> +#define MAX_SRC_TYPE 3
> +#define MAX_NVM_TYPE 3
> +/* A special type. Use defaults specified in CRMU config */
> +#define NVM_DEFAULT 0xff
> +
> +/* Max. number of cmd parameters per elog spec */
> +#define PARAM_COUNT 3
> +
> +#define REC_DESC_LENGTH 8
> +
> +enum {
> + LOG_SETUP_CMD_VALIDATE_META,
> + LOG_SETUP_CMD_WRITE_META,
> + LOG_SETUP_CMD_ERASE,
> + LOG_SETUP_CMD_READ,
> + LOG_SETUP_CMD_CHECK
> +};
> +
> +#pragma pack(push, 1)
Please use __packed on each struct instead
> +
> +struct meta_record {
Structures need comments about what each member is for.
> + u8 record_type;
> + u8 record_format;
> + u8 src_mem_type;
> + u8 alt_src_mem_type;
> + u8 nvm_type;
> + char rec_desc[REC_DESC_LENGTH];
> + u64 src_mem_addr;
> + u64 alt_src_mem_addr;
> + u64 rec_addr;
> + u32 rec_size;
> + u32 sector_size;
> + u8 padding[3];
> +};
> +
> +struct global_header {
> + u32 signature;
> + u32 sector_size;
> + u8 revision;
> + u8 rec_count;
> + u16 padding;
> +};
> +
> +struct log_setup {
> + u32 cmd;
> + u32 params[PARAM_COUNT];
> + u32 result;
> + u32 ret_code;
> +};
> +
> +#pragma pack(pop)
> +
> +#endif
> diff --git a/cmd/bcm/logsetup.c b/cmd/bcm/logsetup.c
> new file mode 100644
> index 0000000000..271462311d
> --- /dev/null
> +++ b/cmd/bcm/logsetup.c
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2020 Broadcom
> + */
> +
> +/*
> + * Create a binary file ready to be flashed
> + * as a global meta for logging, from a source file.
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <string.h>
> +#include <asm/io.h>
> +#include <asm/system.h>
> +#include <linux/ctype.h>
> +#include <linux/delay.h>
> +
> +#include "elog.h"
> +
> +#define FILE_LINE_BUF_SIZE 1024
> +#define GLOBAL_PARAM_COUNT 3
> +#define REC_PARAM_COUNT 11
> +
> +#define GLOBAL_NAME "GLOBAL"
> +#define RECORD_NAME "RECORD"
> +
> +#define MCU_IPC_MCU_CMD_ELOG_SETUP 0x84
> +/* SPI write operations can be slow */
> +#define DEFAULT_TIMEOUT_MS 10000
> +
> +#define MCU_IPC_CMD_DONE_MASK 0x80000000
> +#define MCU_IPC_CMD_REPLY_MASK 0x3fff0000
> +#define MCU_IPC_CMD_REPLY_SHIFT 16
> +
> +#define MIN_ARGS_COUNT 3
> +#define MAX_ARGS_COUNT 5
> +#define SEP_STR ","
> +#define SUPPORTED_CMD "-s"
> +#define CHK_HDR_CMD "-c"
> +
> +enum {
> + PARAM_GLOBAL,
> + PARAM_REC
> +};
> +
> +static uintptr_t crmu_mbox0_address;
> +
> +/*
> + * Send a logging command to MCU patch for execution.
> + *
> + * Parameters:
> + * cmd: an IPC command code
> + * param: a pointer to parameters structure for IPC
> + * is_real_cmd: whether command produces a response in the structure.
> + * Only "support" command does not.
Please use the U-Boot style. Also add @return
> + */
> +static int send_crmu_log_cmd(u32 cmd, u32 param, int is_real_cmd)
> +{
> + u32 timeout;
> + u32 val;
> + int ret = CMD_RET_FAILURE;
> + struct log_setup *setup;
> +
> + setup = (struct log_setup *)(uintptr_t)param;
> + timeout = DEFAULT_TIMEOUT_MS;
> +
> + writel(cmd, crmu_mbox0_address);
> + writel(param, crmu_mbox0_address + sizeof(u32));
> +
> + do {
> + mdelay(1);
> + val = readl(is_real_cmd ? (uintptr_t)&setup->ret_code :
> + crmu_mbox0_address);
> + if (val & MCU_IPC_CMD_DONE_MASK) {
> + ret = CMD_RET_SUCCESS;
> + break;
> + }
> +
Drop blank line.
Normally timeouts in U-Boot are done like this:
ulong start = get_timer(0);
do {
...
if (get_timer(start) > DEFAULT_TIMEOUT_MS) {
// print message
return CMD_RET_FAILURE;
} while (1)
> + } while (timeout--);
> +
> + if (ret == CMD_RET_FAILURE) {
> + pr_err("CRMU cmd timeout!\n");
> + return ret;
> + }
> +
> + /* Obtain status */
> + val = (val & MCU_IPC_CMD_REPLY_MASK) >> MCU_IPC_CMD_REPLY_SHIFT;
> + if (val)
> + ret = CMD_RET_FAILURE;
> +
> + return ret;
> +}
> +
> +static char *get_next_param(char **str, char *delimiter)
> +{
> + char *ret = strsep(str, delimiter);
> +
> + if (!ret)
> + return NULL;
> +
> + return strim(ret);
> +}
> +
> +static int count_chars(char *str, char delimiter)
> +{
> + int count = 0;
> +
> + if (!str || (!strlen(str)))
> + return 0;
> +
> + do {
> + if (*str == delimiter)
> + count++;
> +
> + str++;
> + } while (*str);
> +
> + /* Need to consider leftovers (if any) after the last delimiter */
> + count++;
> +
> + return count;
> +}
> +
> +static int do_setup(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> + struct global_header global;
> + struct log_setup setup = {0};
> + u8 *temp;
> + u8 *ptr = NULL;
> + char *buf;
> + u32 line_no = 0;
> + u32 expected_meta_size;
> + u32 data_size;
> + u32 log_offset = 0;
> + u32 num_recs = 0;
> + u32 addr;
> + int global_ready = 0;
> + int ret = CMD_RET_FAILURE;
> + int force = 0;
> +
> + if (argc < MIN_ARGS_COUNT || argc > MAX_ARGS_COUNT)
> + return CMD_RET_USAGE;
> +
> + /* Usage: addr or (-s/-c) mbox0_addr [data_size] [force] */
> + crmu_mbox0_address = simple_strtoul(argv[2], NULL, 16);
> + if (!crmu_mbox0_address) {
> + pr_err("Invalid CRMU mbox0 address\n");
> + return ret;
> + }
> +
> + if (argc == MIN_ARGS_COUNT) {
> + if (strncmp(argv[1], SUPPORTED_CMD, 2) == 0)
> + return send_crmu_log_cmd(MCU_IPC_MCU_CMD_ELOG_SETUP,
> + LOG_SETUP_CMD_CHECK,
> + 0
> + );
Can you close up more of this on the same line? Please fix globally.
> +
> + if (strncmp(argv[1], CHK_HDR_CMD, 2) == 0)
> + return send_crmu_log_cmd(MCU_IPC_MCU_CMD_ELOG_SETUP,
> + (uintptr_t)&setup,
> + 1
> + );
> + }
> +
> + /* Expect 1st parameter as a pointer to metadata */
> + addr = simple_strtoul(argv[1], NULL, 16);
> + if (!addr) {
> + pr_err("Address 0x0 is not allowed\n");
> + return ret;
> + }
> +
> + data_size = simple_strtoul(argv[3], NULL, 16);
> + if (!data_size) {
> + pr_err("Invalid source size\n");
> + return ret;
> + }
> +
> + if (argc == MAX_ARGS_COUNT)
> + force = simple_strtoul(argv[MAX_ARGS_COUNT - 1], NULL, 10);
> +
> + memset(&global, 0, sizeof(struct global_header));
> + expected_meta_size = sizeof(struct global_header) +
> + MAX_REC_COUNT * sizeof(struct meta_record);
> +
> + temp = (u8 *)malloc(expected_meta_size);
> + if (!temp)
> + return ret;
> +
> + memset(temp, 0, expected_meta_size);
> +
> + ptr = temp;
> + buf = (char *)(uintptr_t)addr;
> + /* End of file mark */
> + buf[data_size] = '\0';
> +
> + do {
> + char *type_name;
> + u32 entry_type;
> + char *line;
> + char *value;
> +
> + line_no++;
> + line = get_next_param(&buf, "\n");
> + if (!line)
> + break;
> +
> + if ((!strlen(line)) || line[0] == '#')
> + continue;
> +
> + value = get_next_param(&line, "=");
> +
> + if (!value || (!strlen(value))) {
> + pr_err("Invalid format of line %d\n", line_no);
> + goto free_params;
> + }
> +
> + type_name = GLOBAL_NAME;
> +
> + if (!strncasecmp(value, type_name, strlen(GLOBAL_NAME))) {
> + entry_type = PARAM_GLOBAL;
> + } else {
> + type_name = RECORD_NAME;
> + if (!strncasecmp(value,
> + type_name,
> + strlen(RECORD_NAME)
> + )
> + ) {
> + entry_type = PARAM_REC;
> + } else {
> + pr_err("Unknown type %s, line %d\n",
> + value,
> + line_no
> + );
> + goto free_params;
> + }
> + }
> +
> + if (global_ready && entry_type == PARAM_GLOBAL) {
> + /* Multiple globals are not allowed */
> + pr_err("Multiple GLOBAL records, line %d\n", line_no);
> + goto free_params;
> + }
> +
> + if (entry_type == PARAM_GLOBAL) {
> + /* Parse Global and create global record in outfile */
> + if (count_chars(line, ',') != GLOBAL_PARAM_COUNT) {
> + pr_err("Invalid GLOBAL format, line %d\n",
> + line_no
> + );
> + goto free_params;
> + }
> +
> + global.sector_size =
> + simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + global.revision =
> + (u8)simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + log_offset = simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> +
> + if (!global.sector_size) {
> + pr_err("Invalid GLOBAL, line %d\n", line_no);
> + goto free_params;
> + }
> +
> + global.signature = GLOBAL_META_HDR_SIG;
> + global_ready = 1;
> +
> + /* Shift to the first RECORD header */
> + log_offset += 2 * global.sector_size;
> + ptr += sizeof(struct global_header);
> + } else {
> + struct meta_record rec = {0};
> + char *desc;
> + char *rec_addr_str;
> + int auto_addr = 0;
> +
> + if (count_chars(line, ',') != REC_PARAM_COUNT) {
> + pr_err("Invalid RECORD, line %d\n", line_no);
> + goto free_params;
> + }
> +
> + rec.record_type = (u8)simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + rec.record_format = (u8)simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + rec.src_mem_type = (u8)simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + rec.alt_src_mem_type = (u8)simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + rec.nvm_type = (u8)simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + desc = get_next_param(&line, SEP_STR);
> + if (desc)
> + desc = strim(desc);
> +
> + rec.src_mem_addr = (u64)simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + rec.alt_src_mem_addr = (u64)simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + rec_addr_str = get_next_param(&line, SEP_STR);
> + if (rec_addr_str)
> + rec_addr_str = strim(rec_addr_str);
> +
> + if (!strcmp(rec_addr_str, "auto"))
> + auto_addr = 1;
> + else
> + rec.rec_addr = (u64)simple_strtoul
> + (rec_addr_str,
> + NULL,
> + 0
> + );
> +
> + rec.rec_size = simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + rec.sector_size = simple_strtoul
> + (get_next_param(&line, SEP_STR),
> + NULL,
> + 0
> + );
> + if (auto_addr) {
> + rec.rec_addr = (u64)log_offset;
> + log_offset += rec.rec_size;
> + }
> +
> + /* Sanity checks */
> + if (rec.record_type > MAX_REC_COUNT ||
> + rec.record_format > MAX_REC_FORMAT ||
> + (rec.nvm_type > MAX_NVM_TYPE &&
> + rec.nvm_type != NVM_DEFAULT) ||
> + !rec.rec_size ||
> + !rec.sector_size ||
> + (!desc || !strlen(desc) ||
> + (strlen(desc) > sizeof(rec.rec_desc) + 1)
> + )
> + ) {
> + pr_err("Invalid RECORD %s, line %d\n",
> + desc ? desc : " (no desc)", line_no
> + );
> + goto free_params;
> + }
> +
> + memset(rec.rec_desc, ' ', sizeof(rec.rec_desc));
> + memcpy(rec.rec_desc, desc, strlen(desc));
> + memcpy(ptr, &rec, sizeof(struct meta_record));
> + ptr += sizeof(struct meta_record);
> + num_recs++;
> + }
Consider putting the inner loop in a function. This function is too long!
> +
> + } while (buf && ((uintptr_t)buf < (addr + data_size)));
> +
> + if (!num_recs) {
> + pr_err("No RECORD entry!\n");
> + goto free_params;
> + }
> +
> + if (!global_ready) {
> + pr_err("Global entry was not found!\n");
> + goto free_params;
> + }
> +
> + if (num_recs > MAX_REC_COUNT) {
> + pr_err("Too many records (max %d)\n", MAX_REC_COUNT);
> + goto free_params;
> + }
> +
> + /* Recalculate new expected size */
> + if (num_recs != MAX_REC_COUNT) {
> + expected_meta_size = sizeof(struct global_header) +
> + num_recs * sizeof(struct meta_record);
> + }
> +
> + global.rec_count = num_recs;
> + memcpy(temp, &global, sizeof(struct global_header));
> +
> + setup.params[0] = (uintptr_t)temp;
> + setup.params[1] = expected_meta_size;
> +
> + /* Apply the command via CRMU mailboxes and read the result. */
> + setup.cmd = (force) ? LOG_SETUP_CMD_WRITE_META :
> + LOG_SETUP_CMD_VALIDATE_META;
> +
> + /* Finally, request the MCU patch to perform the command */
> + ret = send_crmu_log_cmd(MCU_IPC_MCU_CMD_ELOG_SETUP,
> + (uintptr_t)(&setup),
> + 1
> + );
> +
> +free_params:
> + free(temp);
> +
> + return ret;
> +}
> +
> +U_BOOT_CMD(logsetup, 5, 1, do_setup, "setup Broadcom MCU logging subsystem",
> + "\taddr mbox0_addr [data_size] [force]\nor\n\t -s(or -c) mbox0_addr"
Seems like there needs to be more docs on what these things mean.
Could be in the help or in this source file, perhaps?
> + );
> --
> 2.17.1
>
Regards,
Simon
More information about the U-Boot
mailing list