From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Mon, 25 May 2020 11:03:53 -0600 Subject: [PATCH v1 3/3] cmd: bcm: add broadcom error log setup command In-Reply-To: <20200517083257.22191-4-rayagonda.kokatanur@broadcom.com> References: <20200517083257.22191-1-rayagonda.kokatanur@broadcom.com> <20200517083257.22191-4-rayagonda.kokatanur@broadcom.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Rayagonda, On Sun, 17 May 2020 at 02:33, Rayagonda Kokatanur wrote: > > From: Vladimir Olovyannikov > > 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 > Signed-off-by: Rayagonda Kokatanur > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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