[PATCH v1 0/3] add elog support for broadcom NS3 soc

[PATCH v1 0/3] add elog support for broadcom NS3 soc

[Rayagonda Kokatanur]

This is fifth patch set series prepared on top of fourth
patch set ("add optee support for broadcom NS3 soc").

This patch adds support for error logging and corresponding command.

Sheetal Tigadoli (1):
  common: ns3: add error logging support

Vladimir Olovyannikov (2):
  board: ns3: kconfig: extend board kconfig with specific commands
  cmd: bcm: add broadcom error log setup command

 board/broadcom/bcmns3/Kconfig |   5 +
 cmd/Kconfig                   |   2 +
 cmd/Makefile                  |   2 +
 cmd/bcm/Kconfig               |  12 +
 cmd/bcm/Makefile              |   4 +
 cmd/bcm/elog.h                |  64 +++++
 cmd/bcm/logsetup.c            | 432 ++++++++++++++++++++++++++++++++++
 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 +++
 13 files changed, 639 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
 create mode 100644 common/bcm_elog.c
 create mode 100644 include/bcm_elog.h

-- 
2.17.1

[PATCH v1 1/3] common: ns3: add error logging support

[Rayagonda Kokatanur]

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.

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
+	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;
+
+	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
+
 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
+
+/* 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

[PATCH v1 2/3] board: ns3: kconfig: extend board kconfig with specific commands

[Rayagonda Kokatanur]

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(+)

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"
+	default y
+
 config CHIMP_OPTEE
 	bool "Enable secure ChiMP firmware loading"
 	depends on OPTEE
-- 
2.17.1

[PATCH v1 3/3] cmd: bcm: add broadcom error log setup command

[Rayagonda Kokatanur]

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/board/broadcom/bcmns3/Kconfig b/board/broadcom/bcmns3/Kconfig
index 10547f71de..6a8c7bc7db 100644
--- a/board/broadcom/bcmns3/Kconfig
+++ b/board/broadcom/bcmns3/Kconfig
@@ -15,6 +15,7 @@ config SYS_CONFIG_NAME
 config CMD_BCM_EXT_UTILS
 	bool "Enable Broadcom-specific u-boot commands"
 	default y
+	select CMD_BCM_LOGSETUP
 
 config CHIMP_OPTEE
 	bool "Enable secure ChiMP firmware loading"
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 157a33081f..d7b34828fa 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2214,4 +2214,6 @@ config CMD_UBIFS
 	help
 	  UBIFS is a file system for flash devices which works on top of UBI.
 
+source cmd/bcm/Kconfig
+
 endmenu
diff --git a/cmd/Makefile b/cmd/Makefile
index 974ad48b0a..f9925e3a34 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -191,6 +191,8 @@ obj-$(CONFIG_$(SPL_)CMD_TLV_EEPROM) += tlv_eeprom.o
 # core command
 obj-y += nvedit.o
 
+obj-$(CONFIG_CMD_BCM_EXT_UTILS) += bcm/
+
 obj-$(CONFIG_TI_COMMON_CMD_OPTIONS) += ti/
 
 filechk_data_gz = (echo "static const char data_gz[] ="; cat $< | scripts/bin2c; echo ";")
diff --git a/cmd/bcm/Kconfig b/cmd/bcm/Kconfig
new file mode 100644
index 0000000000..189a45004e
--- /dev/null
+++ b/cmd/bcm/Kconfig
@@ -0,0 +1,12 @@
+menu "Broadcom Extended Utilities"
+
+config CMD_BCM_LOGSETUP
+	bool "Command to setup logging on Broadcom boards"
+	depends on TARGET_BCMNS3
+	default n
+	help
+	   Support specific log setup on Broadcom SoCs. This command
+	   allows checking if logging support is present, and update
+	   log sections.
+
+endmenu
diff --git a/cmd/bcm/Makefile b/cmd/bcm/Makefile
new file mode 100644
index 0000000000..96dc8f7ad7
--- /dev/null
+++ b/cmd/bcm/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright 2020 Broadcom
+
+obj-$(CONFIG_CMD_BCM_LOGSETUP) += logsetup.o
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)
+
+struct meta_record {
+	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.
+ */
+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;
+		}
+
+	} 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
+						);
+
+		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++;
+		}
+
+	} 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"
+	  );
-- 
2.17.1

[PATCH v1 1/3] common: ns3: add error logging support

[Simon Glass]

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

[PATCH v1 2/3] board: ns3: kconfig: extend board kconfig with specific commands

[Simon Glass]

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
>

[PATCH v1 3/3] cmd: bcm: add broadcom error log setup command

[Simon Glass]

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

[PATCH v1 1/3] common: ns3: add error logging support

[Tom Rini]

On Sun, May 17, 2020 at 02:02:55PM +0530, Rayagonda Kokatanur 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.
> 
> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

This, and the series that starts with "cmd: bcm: add nitro boot command"
seem to conflict.  Can you please resubmit any further outstanding
patches?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200814/f1f1bce3/attachment.sig>

[PATCH v1 1/3] common: ns3: add error logging support

[Rayagonda Kokatanur]

Hi Tom,


On Sat, Aug 15, 2020 at 1:22 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, May 17, 2020 at 02:02:55PM +0530, Rayagonda Kokatanur 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.
> >
> > Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>
> This, and the series that starts with "cmd: bcm: add nitro boot command"
> seem to conflict.  Can you please resubmit any further outstanding
> patches?  Thanks!

Rebased the patch and sent a v2 patch.

Thanks,
Rayagonda

>
> --
> Tom