Hi Rayagonda,

On Sun, 17 May 2020 at 02:33, Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com wrote:

From: Sheetal Tigadoli sheetal.tigadoli@broadcom.com

Add error logging support in uboot for ns3 platform.

We log the bootup msgs from all bootstages(BL2, BL31, BL33, and Linux) on to DDR. When a watchdog is triggered from any of the bootstages, CRMU copies these logs to QSPI error logging space.

Later when doing the post-mortem analysis, we parse the QSPI error log space.

Is there a doc somewhere describing the format? If not you should add something to the top of your source file.

Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com Signed-off-by: Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com

---

common/Kconfig | 8 +++++++ common/Makefile | 1 + common/bcm_elog.c | 49 +++++++++++++++++++++++++++++++++++++++ common/console.c | 22 ++++++++++++++++++ configs/bcm_ns3_defconfig | 1 + include/bcm_elog.h | 37 +++++++++++++++++++++++++++++ 6 files changed, 118 insertions(+) create mode 100644 common/bcm_elog.c create mode 100644 include/bcm_elog.h

diff --git a/common/Kconfig b/common/Kconfig index 30cba15948..3980ba31e0 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -634,6 +634,14 @@ config SYS_STDIO_DEREGISTER removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly.

+config BCM_ELOG

•   bool "Broadcom error logging support"
  

•   default n
  

Not needed

•   help
  

•     Enables broadcom error logging support to be used with brcm
  

•     platforms, say Y to this option to enable the logging support.
  

•     If unsure, say N.
  

endmenu

menu "Logging" diff --git a/common/Makefile b/common/Makefile index 2e7a090588..dced769dcf 100644 --- a/common/Makefile +++ b/common/Makefile @@ -95,6 +95,7 @@ else obj-$(CONFIG_SPL_SERIAL_SUPPORT) += console.o endif else +obj-$(CONFIG_BCM_ELOG) += bcm_elog.o obj-y += console.o endif # CONFIG_SPL_BUILD

diff --git a/common/bcm_elog.c b/common/bcm_elog.c new file mode 100644 index 0000000000..8e89a500b9 --- /dev/null +++ b/common/bcm_elog.c @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright 2020 Broadcom.
• */

+#include <bcm_elog.h>

+/* Log one character */ +int log2ddr(const char ch) +{

•   u32 offset, len;
  

•   uintptr_t base = BCM_ELOG_UBOOT_BASE;
  

This should really be in a driver. Have you looked at logging? See for example log_syslog.c which is an example of writing log info obtained from the log() function.

If that doesn't work then perhaps have a UCLASS_MISC driver that you can write to?

•   offset = readl(base + BCM_ELOG_OFF_OFFSET);
  

•   len = readl(base + BCM_ELOG_LEN_OFFSET);
  

•   writeb(ch, base + offset);
  

•   offset++;
  

•   /* log buffer is now full and need to wrap around */
  

•   if (offset >= BCM_ELOG_UBOOT_SIZE)
  

•           offset = BCM_ELOG_HEADER_LEN;
  

•   /* only increment length when log buffer is not full */
  

•   if (len < BCM_ELOG_UBOOT_SIZE - BCM_ELOG_HEADER_LEN)
  

•           len++;
  

•   writel(offset, base + BCM_ELOG_OFF_OFFSET);
  

•   writel(len, base + BCM_ELOG_LEN_OFFSET);
  

•   return 0;
  

+}

+/* Routine to initialize error logging */ +void bcm_elog_init(uintptr_t base, uint32_t size) +{

•   u32 val;
  

•   /*
  

•    * If a valid signature is found, it means logging is already
  

•    * initialize. In this case, we should not re-initialize the entry
  

•    * header in the designated memory
  

•    */
  

•   val = readl(base + BCM_ELOG_SIG_OFFSET);
  

•   if (val != BCM_ELOG_SIG_VAL) {
  

•           writel(base + BCM_ELOG_SIG_OFFSET, BCM_ELOG_SIG_VAL);
  

•           writel(base + BCM_ELOG_OFF_OFFSET, BCM_ELOG_HEADER_LEN);
  

•           writel(base + BCM_ELOG_LEN_OFFSET, 0);
  

•   }
  

+} diff --git a/common/console.c b/common/console.c index e398530a13..a65fdc16c2 100644 --- a/common/console.c +++ b/common/console.c @@ -20,6 +20,10 @@ #include <env_internal.h> #include <watchdog.h>

+#ifdef CONFIG_BCM_ELOG +#include <bcm_elog.h> +#endif

Can't add this to common code, sorry.

Hopefully U-Boot's logging can help here?

DECLARE_GLOBAL_DATA_PTR;

static int on_console(const char *name, const char *value, enum env_op op, @@ -536,6 +540,9 @@ void putc(const char c) if (!gd->have_console) return pre_console_putc(c);

+#ifdef CONFIG_BCM_ELOG

•   log2ddr(c);
  

+#endif if (gd->flags & GD_FLG_DEVINIT) { /* Send to the standard output */ fputc(stdout, c); @@ -587,6 +594,17 @@ void puts(const char *s) if (!gd->have_console) return pre_console_puts(s);

+#ifdef CONFIG_BCM_ELOG

•   {
  

•           const char *tmp = s;
  

•           while (*tmp) {
  

•                   int c = *tmp++;
  

•                   log2ddr(c);
  

•           }
  

•   }
  

+#endif if (gd->flags & GD_FLG_DEVINIT) { /* Send to the standard output */ fputs(stdout, s); @@ -790,6 +808,10 @@ int console_init_f(void)

    print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT1_SERIAL);

+#ifdef CONFIG_BCM_ELOG

•   bcm_elog_init(BCM_ELOG_UBOOT_BASE, BCM_ELOG_UBOOT_SIZE);
  

+#endif

•   return 0;
  

}

diff --git a/configs/bcm_ns3_defconfig b/configs/bcm_ns3_defconfig index 13fe9d439e..a2201bf0c9 100644 --- a/configs/bcm_ns3_defconfig +++ b/configs/bcm_ns3_defconfig @@ -14,6 +14,7 @@ CONFIG_LOGLEVEL=7 CONFIG_SILENT_CONSOLE=y CONFIG_SILENT_U_BOOT_ONLY=y # CONFIG_SILENT_CONSOLE_UPDATE_ON_SET is not set +CONFIG_BCM_ELOG=y CONFIG_SUPPORT_RAW_INITRD=y # CONFIG_DISPLAY_CPUINFO is not set CONFIG_HUSH_PARSER=y diff --git a/include/bcm_elog.h b/include/bcm_elog.h new file mode 100644 index 0000000000..7ba99f1cf7 --- /dev/null +++ b/include/bcm_elog.h @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Copyright 2020 Broadcom.
• */

+#ifndef __BCM_ELOG_H__ +#define __BCM_ELOG_H__

+#include <asm/io.h> +#include <linux/types.h>

+/* Default AP error logging base address */ +#ifndef ELOG_AP_UART_LOG_BASE +#define ELOG_AP_UART_LOG_BASE 0x8f110000 +#endif

+/* Reserve 16K to store error logs */ +#define BCM_ELOG_UBOOT_BASE ELOG_AP_UART_LOG_BASE +#define BCM_ELOG_UBOOT_SIZE 0x4000

This should go in the devicetree node for your driver.

+/* error logging signature */ +#define BCM_ELOG_SIG_OFFSET 0x0000 +#define BCM_ELOG_SIG_VAL 0x75767971

+/* current logging offset that points to where new logs should be added */ +#define BCM_ELOG_OFF_OFFSET 0x0004

+/* current logging length (excluding header) */ +#define BCM_ELOG_LEN_OFFSET 0x0008

+#define BCM_ELOG_HEADER_LEN 12

+int log2ddr(const char ch); +void bcm_elog_init(uintptr_t base, uint32_t size);

+#endif /* __BCM_ELOG_H__ */

2.17.1

Regards, Simon

On Sun, 17 May 2020 at 02:33, Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com wrote:

From: Vladimir Olovyannikov vladimir.olovyannikov@broadcom.com

Extend Kconfig for the board with board-specific commands selection.

Signed-off-by: Vladimir Olovyannikov vladimir.olovyannikov@broadcom.com Signed-off-by: Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com

---

board/broadcom/bcmns3/Kconfig | 4 ++++ 1 file changed, 4 insertions(+)

Reviewed-by: Simon Glass sjg@chromium.org

Please see below

diff --git a/board/broadcom/bcmns3/Kconfig b/board/broadcom/bcmns3/Kconfig index 84daad9415..10547f71de 100644 --- a/board/broadcom/bcmns3/Kconfig +++ b/board/broadcom/bcmns3/Kconfig @@ -12,6 +12,10 @@ config SYS_SOC config SYS_CONFIG_NAME default "bcm_ns3"

+config CMD_BCM_EXT_UTILS

•   bool "Enable Broadcom-specific u-boot commands"
  

U-Boot

What sort of commands? You could add some details here.

•   default y
  

config CHIMP_OPTEE bool "Enable secure ChiMP firmware loading" depends on OPTEE -- 2.17.1

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;
  

•                   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