Hi Rayagonda,
On Sun, 17 May 2020 at 02:33, Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com wrote:
From: Vladimir Olovyannikov vladimir.olovyannikov@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@broadcom.com Signed-off-by: Rayagonda Kokatanur rayagonda.kokatanur@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;elserec.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