[{"id":1769846,"web_url":"http://patchwork.ozlabs.org/comment/1769846/","msgid":"","list_archive_url":null,"date":"2017-09-18T03:45:00","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":64981,"url":"http://patchwork.ozlabs.org/api/people/64981/","name":"Bin Meng","email":"bmeng.cn@gmail.com"},"content":"Hi Simon,\n\nOn Sun, Sep 17, 2017 at 5:23 AM, Simon Glass wrote:\n> Add the logging header file and implementation with some configuration\n> options to control it.\n>\n> Signed-off-by: Simon Glass \n> ---\n>\n> MAINTAINERS | 9 ++\n> common/Kconfig | 56 +++++++++\n> common/Makefile | 1 +\n> common/log.c | 246 +++++++++++++++++++++++++++++++++++++\n> include/asm-generic/global_data.h | 5 +\n> include/log.h | 247 ++++++++++++++++++++++++++++++++++++--\n> 6 files changed, 555 insertions(+), 9 deletions(-)\n> create mode 100644 common/log.c\n>\n> diff --git a/MAINTAINERS b/MAINTAINERS\n> index 04acf2b89d..eb420afa8d 100644\n> --- a/MAINTAINERS\n> +++ b/MAINTAINERS\n> @@ -290,6 +290,15 @@ S: Maintained\n> T: git git://git.denx.de/u-boot-i2c.git\n> F: drivers/i2c/\n>\n> +LOGGING\n> +M: Simon Glass \n> +S: Maintained\n> +T: git git://git.denx.de/u-boot.git\n> +F: common/log.c\n> +F: cmd/log.c\n> +F: test/log/log_test.c\n> +F: test/py/tests/test_log.py\n\ntest/log/log_test.c and test/py/tests/test_log.py have not been\nintroduced at this point.\n\n> +\n> MICROBLAZE\n> M: Michal Simek \n> S: Maintained\n> diff --git a/common/Kconfig b/common/Kconfig\n> index 4d8cae9610..cbccc8ae26 100644\n> --- a/common/Kconfig\n> +++ b/common/Kconfig\n> @@ -384,6 +384,62 @@ config SYS_STDIO_DEREGISTER\n>\n> endmenu\n>\n> +menu \"Logging\"\n> +\n> +config LOG\n> + bool \"Enable logging support\"\n> + help\n> + This enables support for logging of status and debug messages. These\n> + can be displayed on the console, recorded in a memory buffer, or\n> + discarded if not needed. Logging supports various categories and\n> + levels of severity.\n> +\n> +config SPL_LOG\n> + bool \"Enable logging support in SPL\"\n> + help\n> + This enables support for logging of status and debug messages. These\n> + can be displayed on the console, recorded in a memory buffer, or\n> + discarded if not needed. Logging supports various categories and\n> + levels of severity.\n> +\n> +config LOG_MAX_LEVEL\n> + int \"Maximum log level to record\"\n> + depends on LOG\n> + default 5\n> + help\n> + This selects the maximum log level that will be recorded. Any value\n> + higher than this will be ignored. If possible log statements below\n> + this level will be discarded at build time. Levels:\n> +\n> + 0 - panic\n> + 1 - critical\n> + 2 - error\n> + 3 - warning\n> + 4 - note\n> + 5 - info\n> + 6 - detail\n> + 7 - debug\n> +\n> +config LOG_SPL_MAX_LEVEL\n> + int \"Maximum log level to record in SPL\"\n> + depends on SPL_LOG\n> + default 3\n> + help\n> + This selects the maximum log level that will be recorded. Any value\n> + higher than this will be ignored. If possible log statements below\n> + this level will be discarded at build time. Levels:\n> +\n> + 0 - panic\n> + 1 - critical\n> + 2 - error\n> + 3 - warning\n> + 4 - note\n> + 5 - info\n> + 6 - detail\n> + 7 - debug\n> +\n> +endmenu\n> +\n> config DTB_RESELECT\n> bool \"Support swapping dtbs at a later point in boot\"\n> depends on FIT_EMBED\n> diff --git a/common/Makefile b/common/Makefile\n> index 1b56cf9a70..d37c8d5636 100644\n> --- a/common/Makefile\n> +++ b/common/Makefile\n> @@ -128,5 +128,6 @@ obj-y += cli.o\n> obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_simple.o cli_readline.o\n> obj-$(CONFIG_CMD_DFU) += dfu.o\n> obj-y += command.o\n> +obj-$(CONFIG_$(SPL_)LOG) += log.o\n> obj-y += s_record.o\n> obj-y += xyzModem.o\n> diff --git a/common/log.c b/common/log.c\n> new file mode 100644\n> index 0000000000..6bf2219d38\n> --- /dev/null\n> +++ b/common/log.c\n> @@ -0,0 +1,246 @@\n> +/*\n> + * Logging support\n> + *\n> + * Copyright (c) 2017 Google, Inc\n> + * Written by Simon Glass \n> + *\n> + * SPDX-License-Identifier: GPL-2.0+\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +DECLARE_GLOBAL_DATA_PTR;\n> +\n> +static struct log_device *log_device_find_by_name(const char *drv_name)\n> +{\n> + struct log_device *ldev;\n> +\n> + list_for_each_entry(ldev, &gd->log_head, sibling_node) {\n> + if (!strcmp(drv_name, ldev->drv->name))\n> + return ldev;\n> + }\n> +\n> + return NULL;\n> +}\n> +\n> +/**\n> + * log_has_cat() - check if a log category exists within a list\n> + *\n> + * @cat_list: List of categories to check, at most LOGF_MAX_CATEGORIES entries\n> + * long, terminated by LC_END if fewer\n> + * @cat: Category to search for\n> + * @return true if @cat is in @cat_list, else false\n> + */\n> +static bool log_has_cat(enum log_category_t cat_list[], enum log_category_t cat)\n> +{\n> + int i;\n> +\n> + for (i = 0; i < LOGF_MAX_CATEGORIES && cat_list[i] != LOGC_END; i++) {\n> + if (cat_list[i] == cat)\n> + return true;\n> + }\n> +\n> + return false;\n> +}\n> +\n> +/**\n> + * log_has_file() - check if a file is with a list\n> + *\n> + * @file_list: List of files to check, separated by comma\n> + * @file: File to check for. This string is matched against the end of each\n> + * file in the list, i.e. ignoring any preceeding path. The list is\n> + * intended to consist of relative pathnames, e.g. common/main.c,cmd/log.c\n> + * @return true if @file is in @file_list, else false\n> + */\n> +static bool log_has_file(const char *file_list, const char *file)\n> +{\n> + int file_len = strlen(file);\n> + const char *s, *p;\n> + int substr_len;\n> +\n> + for (s = file_list; *s; s = p + (*p != '\\0')) {\n> + p = strchrnul(s, ',');\n> + substr_len = p - s;\n> + if (file_len >= substr_len &&\n> + !strncmp(file + file_len - substr_len, s, substr_len))\n> + return true;\n> + }\n> +\n> + return false;\n> +}\n> +\n> +/**\n> + * log_passes_filters() - check if a log record passes the filters for a device\n> + *\n> + * @ldev: Log device to check\n> + * @rec: Log record to check\n> + * @return true if @rec is not blocked by the filters in @ldev, false if it is\n> + */\n> +static bool log_passes_filters(struct log_device *ldev, struct log_rec *rec)\n> +{\n> + struct log_filter *filt;\n> +\n> + /* If there are no filters, filter on the default log level */\n> + if (list_empty(&ldev->filter_head)) {\n> + if (rec->level > gd->default_log_level)\n> + return false;\n> + return true;\n> + }\n> +\n> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {\n> + if (rec->level > filt->max_level)\n> + continue;\n> + if ((filt->flags & LOGFF_HAS_CAT) &&\n> + !log_has_cat(filt->cat_list, rec->cat))\n> + continue;\n> + if (filt->file_list &&\n> + !log_has_file(filt->file_list, rec->file))\n> + continue;\n> + return true;\n> + }\n> +\n> + return false;\n> +}\n> +\n> +/**\n> + * log_dispatch() - Send a log record to all log devices for processing\n> + *\n> + * The log record is sent to each log device in turn, skipping those which have\n> + * filters which block the record\n> + *\n> + * @rec: Log record to dispatch\n> + * @return 0 (meaning success)\n> + */\n> +static int log_dispatch(struct log_rec *rec)\n> +{\n> + struct log_device *ldev;\n> +\n> + list_for_each_entry(ldev, &gd->log_head, sibling_node) {\n> + if (log_passes_filters(ldev, rec))\n> + ldev->drv->emit(ldev, rec);\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,\n> + int line, const char *func, const char *fmt, ...)\n> +{\n> + char buf[CONFIG_SYS_CBSIZE];\n> + struct log_rec rec;\n> + va_list args;\n> +\n> + rec.cat = cat;\n> + rec.level = level;\n> + rec.file = file;\n> + rec.line = line;\n> + rec.func = func;\n> + va_start(args, fmt);\n> + vsnprintf(buf, sizeof(buf), fmt, args);\n> + va_end(args);\n> + rec.msg = buf;\n> + if (!gd || !(gd->flags & GD_FLG_LOG_READY)) {\n> + if (gd)\n> + gd->log_drop_count++;\n> + return -ENOSYS;\n> + }\n> + log_dispatch(&rec);\n> +\n> + return 0;\n> +}\n> +\n> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],\n> + enum log_level_t max_level, const char *file_list)\n> +{\n> + struct log_filter *filt;\n> + struct log_device *ldev;\n> + int i;\n> +\n> + ldev = log_device_find_by_name(drv_name);\n> + if (!ldev)\n> + return -ENOENT;\n> + filt = (struct log_filter *)calloc(1, sizeof(*filt));\n> + if (!filt)\n> + return -ENOMEM;\n> +\n> + if (cat_list) {\n> + filt->flags |= LOGFF_HAS_CAT;\n> + for (i = 0; ; i++) {\n> + if (i == ARRAY_SIZE(filt->cat_list))\n> + return -ENOSPC;\n> + filt->cat_list[i] = cat_list[i];\n> + if (cat_list[i] == LOGC_END)\n> + break;\n> + }\n> + }\n> + filt->max_level = max_level;\n> + if (file_list) {\n> + filt->file_list = strdup(file_list);\n> + if (!filt->file_list)\n> + goto nomem;\n> + }\n> + filt->filter_num = ldev->next_filter_num++;\n> + INIT_LIST_HEAD(&filt->sibling_node);\n> + list_add_tail(&filt->sibling_node, &ldev->filter_head);\n> +\n> + return filt->filter_num;\n> +\n> +nomem:\n> + free(filt);\n> + return -ENOMEM;\n> +}\n> +\n> +int log_remove_filter(const char *drv_name, int filter_num)\n> +{\n> + struct log_filter *filt;\n> + struct log_device *ldev;\n> +\n> + ldev = log_device_find_by_name(drv_name);\n> + if (!ldev)\n> + return -ENOENT;\n> +\n> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {\n> + if (filt->filter_num == filter_num) {\n> + list_del(&filt->sibling_node);\n> + free(filt);\n> +\n> + return 0;\n> + }\n> + }\n> +\n> + return -ENOENT;\n> +}\n> +\n> +int log_init(void)\n> +{\n> + struct log_driver *drv = ll_entry_start(struct log_driver, log_driver);\n> + const int count = ll_entry_count(struct log_driver, log_driver);\n> + struct log_driver *end = drv + count;\n> +\n> + /*\n> + * We cannot add runtime data to the driver since it is likely stored\n> + * in rodata. Instead, set up a 'device' corresponding to each driver.\n> + * We only support having a single device.\n> + */\n> + INIT_LIST_HEAD((struct list_head *)&gd->log_head);\n> + while (drv < end) {\n> + struct log_device *ldev;\n> +\n> + ldev = calloc(1, sizeof(*ldev));\n> + if (!ldev) {\n> + debug(\"%s: Cannot allocate memory\\n\", __func__);\n> + return -ENOMEM;\n> + }\n> + INIT_LIST_HEAD(&ldev->sibling_node);\n> + INIT_LIST_HEAD(&ldev->filter_head);\n> + ldev->drv = drv;\n> + list_add_tail(&ldev->sibling_node,\n> + (struct list_head *)&gd->log_head);\n> + drv++;\n> + }\n> + gd->default_log_level = LOGL_INFO;\n\nShouldn't this be the Kconfig option CONFIG_LOG_MAX_LEVEL?\n\n> +\n> + return 0;\n> +}\n> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h\n> index 79197acfa4..77755dbb06 100644\n> --- a/include/asm-generic/global_data.h\n> +++ b/include/asm-generic/global_data.h\n> @@ -114,6 +114,11 @@ typedef struct global_data {\n> struct bootstage_data *bootstage; /* Bootstage information */\n> struct bootstage_data *new_bootstage; /* Relocated bootstage info */\n> #endif\n> +#ifdef CONFIG_LOG\n> + int log_drop_count; /* Number of dropped log messages */\n> + int default_log_level; /* For devices with no filters */\n> + struct list_head log_head; /* List of struct log_device */\n> +#endif\n> } gd_t;\n> #endif\n>\n> diff --git a/include/log.h b/include/log.h\n> index 4101a74161..fb6a196202 100644\n> --- a/include/log.h\n> +++ b/include/log.h\n> @@ -10,6 +10,82 @@\n> #ifndef __LOG_H\n> #define __LOG_H\n>\n> +#include \n> +#include \n> +\n> +/** Log levels supported, ranging from most to least important */\n> +enum log_level_t {\n> + LOGL_PANIC = 0,\n> + LOGL_CRIT,\n> + LOGL_ERR,\n> + LOGL_WARN,\n> + LOGL_NOTE,\n> + LOGL_INFO,\n> + LOGL_DETAIL,\n> + LOGL_DEBUG,\n> +\n> + LOGL_COUNT,\n> + LOGL_FIRST = LOGL_PANIC,\n> + LOGL_MAX = LOGL_DEBUG,\n> +};\n> +\n> +/**\n> + * Log categories supported. Most of these correspond to uclasses (i.e.\n> + * enum uclass_id) but there are also some more generic categories\n> + */\n> +enum log_category_t {\n> + LOGC_FIRST = 0, /* First part mirrors UCLASS_... */\n> +\n> + LOGC_NONE = UCLASS_COUNT,\n> + LOGC_ARCH,\n> + LOGC_BOARD,\n> + LOGC_CORE,\n> + LOGC_DT,\n> +\n> + LOGC_COUNT,\n> + LOGC_END,\n> +};\n> +\n> +/**\n> + * _log() - Internal function to emit a new log record\n> + *\n> + * @cat: Category of log record (indicating which subsystem generated it)\n> + * @level: Level of log record (indicating its severity)\n> + * @file: File name of file where log record was generated\n> + * @line: Line number in file where log record was generated\n> + * @func: Function where log record was generated\n> + * @fmt: printf() format string for log record\n> + * @...: Optional parameters, according to the format string @fmt\n> + * @return 0 if log record was emitted, -ve on error\n> + */\n> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,\n> + int line, const char *func, const char *fmt, ...);\n> +\n> +/* Define this at the top of a file to add a prefix to debug messages */\n> +#ifndef pr_fmt\n> +#define pr_fmt(fmt) fmt\n> +#endif\n> +\n> +/* Use a default category if this file does not supply one */\n> +#ifndef LOG_CATEGORY\n> +#define LOG_CATEGORY LOGC_NONE\n> +#endif\n> +\n> +#if CONFIG_VAL(LOG_MAX_LEVEL)\n> +#define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)\n> +#else\n> +#define _LOG_MAX_LEVEL LOGL_INFO\n> +#endif\n> +\n> +/* Emit a log record if the level is less that the maximum */\n> +#define log(_cat, _level, _fmt, _args...) ({ \\\n> + int _l = _level; \\\n> + if (_l > _LOG_MAX_LEVEL) \\\n> + continue; \\\n> + _log(_cat, _l, __FILE__, __LINE__, __func__, \\\n> + pr_fmt(_fmt), ##_args); \\\n> + })\n> +\n> #ifdef DEBUG\n> #define _DEBUG 1\n> #else\n> @@ -22,10 +98,19 @@\n> #define _SPL_BUILD 0\n> #endif\n>\n> -/* Define this at the top of a file to add a prefix to debug messages */\n> -#ifndef pr_fmt\n> -#define pr_fmt(fmt) fmt\n> -#endif\n> +#if !_DEBUG && CONFIG_IS_ENABLED(LOG)\n> +\n> +#define debug_cond(cond, fmt, args...) \\\n> + do { \\\n> + if (1) \\\n> + log(LOG_CATEGORY, LOGL_DEBUG, fmt, ##args); \\\n> + } while (0)\n> +\n> +#define error(fmt, args...) do { \\\n> + log(LOG_CATEGORY, LOGL_ERR, fmt, ##args); \\\n> +} while (0)\n> +\n> +#else /* _DEBUG */\n>\n> /*\n> * Output a debug text when condition \"cond\" is met. The \"cond\" should be\n> @@ -38,6 +123,13 @@\n> printf(pr_fmt(fmt), ##args); \\\n> } while (0)\n>\n> +#define error(fmt, args...) do { \\\n> + printf(\"ERROR: \" pr_fmt(fmt) \"\\nat %s:%d/%s()\\n\", \\\n> + ##args, __FILE__, __LINE__, __func__); \\\n> +} while (0)\n> +\n> +#endif /* _DEBUG */\n> +\n> /* Show a message if DEBUG is defined in a file */\n> #define debug(fmt, args...) \\\n> debug_cond(_DEBUG, fmt, ##args)\n> @@ -61,11 +153,6 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line,\n> ({ if (!(x) && _DEBUG) \\\n> __assert_fail(#x, __FILE__, __LINE__, __func__); })\n>\n> -#define error(fmt, args...) do { \\\n> - printf(\"ERROR: \" pr_fmt(fmt) \"\\nat %s:%d/%s()\\n\", \\\n> - ##args, __FILE__, __LINE__, __func__); \\\n> -} while (0)\n> -\n> #ifndef BUG\n> #define BUG() do { \\\n> printf(\"BUG: failure at %s:%d/%s()!\\n\", __FILE__, __LINE__, \\\n> @@ -76,4 +163,146 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line,\n> while (0)\n> #endif /* BUG */\n>\n> +/**\n> + * struct log_rec - a single log record\n> + *\n> + * Holds information about a single record in the log\n> + *\n> + * Members marked as 'not allocated' are stored as pointers and the caller is\n> + * responsible for making sure that the data pointed to is not overwritten.\n> + * Memebers marked as 'allocated' are allocated (e.g. via strdup()) by the log\n> + * system.\n> + *\n> + * @cat: Category, representing a uclass or part of U-Boot\n> + * @level: Severity level, less severe is higher\n> + * @file: Name of file where the log record was generated (not allocated)\n> + * @line: Line number where the log record was generated\n> + * @func: Function where the log record was generated (not allocated)\n> + * @msg: Log message (allocated)\n> + */\n> +struct log_rec {\n> + enum log_category_t cat;\n> + enum log_level_t level;\n> + const char *file;\n> + int line;\n> + const char *func;\n> + const char *msg;\n> +};\n> +\n> +struct log_device;\n> +\n> +/**\n> + * struct log_driver - a driver which accepts and processes log records\n> + *\n> + * @name: Name of driver\n> + */\n> +struct log_driver {\n> + const char *name;\n> + /**\n> + * emit() - emit a log record\n> + *\n> + * Called by the log system to pass a log record to a particular driver\n> + * for processing. The filter is checked before calling this function.\n> + */\n> + int (*emit)(struct log_device *ldev, struct log_rec *rec);\n> +};\n> +\n\nSo we are creating a new type of non-DM driver which is log-specific?\nHow about we add this emit to the existing uclass driver that can be\nused as the log driver? (eg: blk devices with file system?)\n\n> +/**\n> + * struct log_device - an instance of a log driver\n> + *\n> + * Since drivers are set up at build-time we need to have a separate device for\n> + * the run-time aspects of drivers (currently just a list of filters to apply\n> + * to records send to this device).\n> + *\n> + * @next_filter_num: Seqence number of next filter filter added (0=no filters\n> + * yet). This increments with each new filter on the device, but never\n> + * decrements\n> + * @drv: Pointer to driver for this device\n> + * @filter_head: List of filters for this device\n> + * @sibling_node: Next device in the list of all devices\n> + */\n> +struct log_device {\n> + int next_filter_num;\n> + struct log_driver *drv;\n> + struct list_head filter_head;\n> + struct list_head sibling_node;\n> +};\n> +\n> +enum {\n> + LOGF_MAX_CATEGORIES = 5, /* maximum categories per filter */\n> +};\n> +\n> +enum log_filter_flags {\n> + LOGFF_HAS_CAT = 1 << 0, /* Filter has a category list */\n> +};\n> +\n> +/**\n> + * struct log_filter - criterial to filter out log messages\n> + *\n> + * @filter_num: Sequence number of this filter. This is returned when adding a\n> + * new filter, and must be provided when removing a previously added\n> + * filter.\n> + * @flags: Flags for this filter (LOGFF_...)\n> + * @cat_list: List of categories to allow (terminated by LOGC_none). If empty\n> + * then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries\n> + * can be provided\n> + * @max_level: Maximum log level to allow\n> + * @file_list: List of files to allow, separated by comma. If NULL then all\n> + * files are permitted\n> + * @sibling_node: Next filter in the list of filters for this log device\n> + */\n> +struct log_filter {\n> + int filter_num;\n> + int flags;\n> + enum log_category_t cat_list[LOGF_MAX_CATEGORIES];\n> + enum log_level_t max_level;\n> + const char *file_list;\n> + struct list_head sibling_node;\n> +};\n> +\n> +#define LOG_DRIVER(_name) \\\n> + ll_entry_declare(struct log_driver, _name, log_driver)\n> +\n> +/**\n> + * log_add_filter() - Add a new filter to a log device\n> + *\n> + * @drv_name: Driver name to add the filter to (since each driver only has a\n> + * single device)\n> + * @cat_list: List of categories to allow (terminated by LOGC_none). If empty\n> + * then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries\n> + * can be provided\n> + * @max_level: Maximum log level to allow\n> + * @file_list: List of files to allow, separated by comma. If NULL then all\n> + * files are permitted\n> + * @return the sequence number of the new filter (>=0) if the filter was added,\n> + * or a -ve value on error\n> + */\n> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],\n> + enum log_level_t max_level, const char *file_list);\n> +\n> +/**\n> + * log_remove_filter() - Remove a filter from a log device\n> + *\n> + * @drv_name: Driver name to remove the filter from (since each driver only has\n> + * a single device)\n> + * @filter_num: Filter number to remove (as returned by log_add_filter())\n> + * @return 0 if the filter was removed, -ENOENT if either the driver or the\n> + * filter number was not found\n> + */\n> +int log_remove_filter(const char *drv_name, int filter_num);\n> +\n> +#if CONFIG_IS_ENABLED(LOG)\n> +/**\n> + * log_init() - Set up the log system ready for use\n> + *\n> + * @return 0 if OK, -ENOMEM if out of memory\n> + */\n> +int log_init(void);\n> +#else\n> +static inline int log_init(void)\n> +{\n> + return 0;\n> +}\n> +#endif\n> +\n> #endif\n> --\n\nRegards,\nBin","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"g+tO/OBU\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwX2d3VD0z9s3T\n\tfor ;\n\tMon, 18 Sep 2017 13:45:13 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 463AEC21EDC; Mon, 18 Sep 2017 03:45:12 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id DBFAAC21D57;\n\tMon, 18 Sep 2017 03:45:04 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 0BA23C21D57; Mon, 18 Sep 2017 03:45:03 +0000 (UTC)","from mail-wm0-f48.google.com (mail-wm0-f48.google.com\n\t[74.125.82.48])\n\tby lists.denx.de (Postfix) with ESMTPS id E16B7C21D19\n\tfor ; Mon, 18 Sep 2017 03:45:01 +0000 (UTC)","by mail-wm0-f48.google.com with SMTP id v142so18721729wmv.5\n\tfor ; Sun, 17 Sep 2017 20:45:01 -0700 (PDT)","by 10.223.145.3 with HTTP; Sun, 17 Sep 2017 20:45:00 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=FREEMAIL_FROM,\n\tRCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=gYPmtwCeyPzYxs9mB+jQ/GC0Hsq3lVHqfmyegPCibKM=;\n\tb=g+tO/OBUANBsoq0RoTSccXeehqbny5i1TKhCYhRWGBapj5GycxeOvSQij0l7h+NB+E\n\tLHBay3B0MSBDY4SradD6+SMYyA+GRhl4TFOVdLyizlhvcSe0/kpJTy6o/+FbjZiDUo0i\n\tQZ997KaKdphFDpfgeLyXzVI3ceukjw9CfMGlbHAlJzOOlG7e4hebSZa1imtUo2DvDO+6\n\tAwhi8as5utq4HoL9Ew8nsbkWpKwpGLwgd7MOoVibPam28fpyTJ6Sw0LC1BPesNhGLBSu\n\tG0TAUXeNw02+qivhUYVKfRxO9p2EpZ7g2CePGRyj10Yjvl9LMXs7pj+ZkYRvN1Q1BjOU\n\t8Shw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=gYPmtwCeyPzYxs9mB+jQ/GC0Hsq3lVHqfmyegPCibKM=;\n\tb=gfe80o/gPhhbbLq7qbHPCWlZRwXAWZ/Hztj2xN9EX+LP/dKLPvUNdoZQhuZwARBKlZ\n\twMLLbpFnEh5t0Ur9Ksd0yopQIL9rtW3tXQAIGtY9KTUs1xLp6CMxTbibvFfMmQtOMNyi\n\tCg864VrbWo1mTHjp+Jrw2Y/UHKi462UAUEZCzAYJRua0stvpt7tTw0zHeYGCKOAO3WO0\n\tv0qR2iJ1sEeAwL+PVUZVD2sRZ/LlFro1VlbKGtsTLhMCE8DY3A7kLj5qGY1Gnmix38gb\n\tjKjl5rcXQIeyUHAP0vSF7tP07X5Yp0kZzKZ7kHKcoLN/rJ6GIjl7SWlqInl5sufWpPRi\n\tPf8g==","X-Gm-Message-State":"AHPjjUj4aPouCNW42yEDq8v8e6P2iiixIBQXcqg9jOyNGLRfAZfFkEiE\n\thPkSyPTxwZuQ+UbcrHDzU8xr9lsjquNGoBBn4pw=","X-Google-Smtp-Source":"AOwi7QCBGY3I+EVNcBadMiCxArFZG6cJJehK1iYnqtvye3sd8mg67dCe53zhUm5GKfmBaoD+OWpgoCKzUI+ilqUUF6E=","X-Received":"by 10.28.130.131 with SMTP id e125mr8547152wmd.125.1505706301157;\n\tSun, 17 Sep 2017 20:45:01 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170916212331.170463-7-sjg@chromium.org>","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>","From":"Bin Meng ","Date":"Mon, 18 Sep 2017 11:45:00 +0800","Message-ID":"","To":"Simon Glass ","Cc":"Tom Rini , Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Stefan Roese , =?utf-8?b?xYF1a2Fz?=\n\t=?utf-8?q?z_Majewski?= ,\n\tAndy Yan , Maxime Ripard\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1771518,"web_url":"http://patchwork.ozlabs.org/comment/1771518/","msgid":"","list_archive_url":null,"date":"2017-09-20T02:51:01","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":65882,"url":"http://patchwork.ozlabs.org/api/people/65882/","name":"Masahiro Yamada","email":"yamada.masahiro@socionext.com"},"content":"Hi Simon,\n\n\n2017-09-17 6:23 GMT+09:00 Simon Glass :\n\n>\n> +menu \"Logging\"\n> +\n> +config LOG\n> + bool \"Enable logging support\"\n> + help\n> + This enables support for logging of status and debug messages. These\n> + can be displayed on the console, recorded in a memory buffer, or\n> + discarded if not needed. Logging supports various categories and\n> + levels of severity.\n> +\n> +config SPL_LOG\n> + bool \"Enable logging support in SPL\"\n> + help\n> + This enables support for logging of status and debug messages. These\n> + can be displayed on the console, recorded in a memory buffer, or\n> + discarded if not needed. Logging supports various categories and\n> + levels of severity.\n\n\nPlease note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n\nSince commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\nCONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\nwhen building for TPL.\n\nSince that commit, if you add SPL_ prefixed option,\nyou need to add a TPL_ one as well.\n\nI cannot believe why such a commit was accepted.\n\n\n\n\n> +config LOG_MAX_LEVEL\n> + int \"Maximum log level to record\"\n> + depends on LOG\n> + default 5\n> + help\n> + This selects the maximum log level that will be recorded. Any value\n> + higher than this will be ignored. If possible log statements below\n> + this level will be discarded at build time. Levels:\n> +\n> + 0 - panic\n> + 1 - critical\n> + 2 - error\n> + 3 - warning\n> + 4 - note\n> + 5 - info\n> + 6 - detail\n> + 7 - debug\n\n\nPlease do not invent our own for U-Boot.\nJust use Linux log level.\n\n 0 (KERN_EMERG) system is unusable\n 1 (KERN_ALERT) action must be taken immediately\n 2 (KERN_CRIT) critical conditions\n 3 (KERN_ERR) error conditions\n 4 (KERN_WARNING) warning conditions\n 5 (KERN_NOTICE) normal but significant condition\n 6 (KERN_INFO) informational\n 7 (KERN_DEBUG) debug-level messages\n\n\n\n\n> +config LOG_SPL_MAX_LEVEL\n> + int \"Maximum log level to record in SPL\"\n> + depends on SPL_LOG\n> + default 3\n> + help\n> + This selects the maximum log level that will be recorded. Any value\n> + higher than this will be ignored. If possible log statements below\n> + this level will be discarded at build time. Levels:\n> +\n> + 0 - panic\n> + 1 - critical\n> + 2 - error\n> + 3 - warning\n> + 4 - note\n> + 5 - info\n> + 6 - detail\n> + 7 - debug\n>\n\n\nIf you want to use CONFIG_VAL(LOG_MAX_LEVEL),\nthis must be SPL_LOG_MAX_LEVEL.\n(this coding mistake is now hidden by another mistake)\n\nAgain, you will probably end up with TPL_LOG_MAX_LEVEL.\n\n\n\n\n> +\n> +/**\n> + * log_dispatch() - Send a log record to all log devices for processing\n> + *\n> + * The log record is sent to each log device in turn, skipping those which have\n> + * filters which block the record\n> + *\n> + * @rec: Log record to dispatch\n> + * @return 0 (meaning success)\n> + */\n> +static int log_dispatch(struct log_rec *rec)\n> +{\n> + struct log_device *ldev;\n> +\n> + list_for_each_entry(ldev, &gd->log_head, sibling_node) {\n> + if (log_passes_filters(ldev, rec))\n> + ldev->drv->emit(ldev, rec);\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,\n> + int line, const char *func, const char *fmt, ...)\n> +{\n> + char buf[CONFIG_SYS_CBSIZE];\n> + struct log_rec rec;\n> + va_list args;\n> +\n> + rec.cat = cat;\n> + rec.level = level;\n> + rec.file = file;\n> + rec.line = line;\n> + rec.func = func;\n> + va_start(args, fmt);\n> + vsnprintf(buf, sizeof(buf), fmt, args);\n> + va_end(args);\n> + rec.msg = buf;\n> + if (!gd || !(gd->flags & GD_FLG_LOG_READY)) {\n> + if (gd)\n> + gd->log_drop_count++;\n> + return -ENOSYS;\n> + }\n> + log_dispatch(&rec);\n> +\n> + return 0;\n> +}\n> +\n> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],\n> + enum log_level_t max_level, const char *file_list)\n> +{\n> + struct log_filter *filt;\n> + struct log_device *ldev;\n> + int i;\n> +\n> + ldev = log_device_find_by_name(drv_name);\n> + if (!ldev)\n> + return -ENOENT;\n> + filt = (struct log_filter *)calloc(1, sizeof(*filt));\n> + if (!filt)\n> + return -ENOMEM;\n> +\n> + if (cat_list) {\n> + filt->flags |= LOGFF_HAS_CAT;\n> + for (i = 0; ; i++) {\n> + if (i == ARRAY_SIZE(filt->cat_list))\n> + return -ENOSPC;\n> + filt->cat_list[i] = cat_list[i];\n> + if (cat_list[i] == LOGC_END)\n> + break;\n> + }\n> + }\n> + filt->max_level = max_level;\n> + if (file_list) {\n> + filt->file_list = strdup(file_list);\n> + if (!filt->file_list)\n> + goto nomem;\n> + }\n> + filt->filter_num = ldev->next_filter_num++;\n> + INIT_LIST_HEAD(&filt->sibling_node);\n> + list_add_tail(&filt->sibling_node, &ldev->filter_head);\n> +\n> + return filt->filter_num;\n> +\n> +nomem:\n> + free(filt);\n> + return -ENOMEM;\n> +}\n> +\n> +int log_remove_filter(const char *drv_name, int filter_num)\n> +{\n> + struct log_filter *filt;\n> + struct log_device *ldev;\n> +\n> + ldev = log_device_find_by_name(drv_name);\n> + if (!ldev)\n> + return -ENOENT;\n> +\n> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {\n> + if (filt->filter_num == filter_num) {\n> + list_del(&filt->sibling_node);\n> + free(filt);\n> +\n> + return 0;\n> + }\n> + }\n> +\n> + return -ENOENT;\n> +}\n\n\nI am not sure if this luxury filter is needed.\nAfter all, the purpose is debugging.\nThe printf() debugging is useful, but only during testing.\nOnce quality code is established, most of debug message should\ngenerally be removed from upstream code.\n\n\n\n> +int log_init(void)\n> +{\n> + struct log_driver *drv = ll_entry_start(struct log_driver, log_driver);\n> + const int count = ll_entry_count(struct log_driver, log_driver);\n> + struct log_driver *end = drv + count;\n> +\n> + /*\n> + * We cannot add runtime data to the driver since it is likely stored\n> + * in rodata. Instead, set up a 'device' corresponding to each driver.\n> + * We only support having a single device.\n> + */\n> + INIT_LIST_HEAD((struct list_head *)&gd->log_head);\n> + while (drv < end) {\n> + struct log_device *ldev;\n> +\n> + ldev = calloc(1, sizeof(*ldev));\n> + if (!ldev) {\n> + debug(\"%s: Cannot allocate memory\\n\", __func__);\n> + return -ENOMEM;\n> + }\n> + INIT_LIST_HEAD(&ldev->sibling_node);\n\nThis INIT_LIST_HEAD() is unneeded.\n\n\n\n> + INIT_LIST_HEAD(&ldev->filter_head);\n> + ldev->drv = drv;\n> + list_add_tail(&ldev->sibling_node,\n> + (struct list_head *)&gd->log_head);\n\nI know that this (struct list_head *) cast is needed,\nbut it is very unfortunate.\nI believe gd_t is design mistake in the first place,\nbut U-Boot has gone to unfixable already...\n\n\n\n\n\n> +#if CONFIG_VAL(LOG_MAX_LEVEL)\n> +#define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)\n> +#else\n> +#define _LOG_MAX_LEVEL LOGL_INFO\n> +#endif\n\n\nI cannot understand your intention of #if CONFIG_VAL(LOG_MAX_LEVEL).\nFrom your Kconfig entry,\n\n 0 - panic\n 1 - critical\n 2 - error\n 3 - warning\n 4 - note\n 5 - info\n 6 - detail\n 7 - debug\n\nI expect CONFIG_VAL(LOG_MAX_LEVEL) is an integer value\nrange 0 to 7.\n\nIf the log level is 0 (= critial), the conditional is false, so\n_LOG_MAX_LEVEL is set to \"info\" level.\n\nWhy is this #if necessary?\n\n\n\n\n\n\n\n\n> +/* Emit a log record if the level is less that the maximum */\n> +#define log(_cat, _level, _fmt, _args...) ({ \\\n> + int _l = _level; \\\n> + if (_l > _LOG_MAX_LEVEL) \\\n> + continue; \\\n> + _log(_cat, _l, __FILE__, __LINE__, __func__, \\\n> + pr_fmt(_fmt), ##_args); \\\n> + })\n\n\nThis is neither while() nor for().\nThe bare use of \"continue\" made me really surprised.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=nifty.com header.i=@nifty.com\n\theader.b=\"X/MgzTRI\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxkmH3q1Jz9s78\n\tfor ;\n\tWed, 20 Sep 2017 12:51:59 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 4C928C21D76; Wed, 20 Sep 2017 02:51:54 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id DB85EC21C35;\n\tWed, 20 Sep 2017 02:51:50 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid E8C3EC21C35; Wed, 20 Sep 2017 02:51:48 +0000 (UTC)","from conssluserg-05.nifty.com (conssluserg-05.nifty.com\n\t[210.131.2.90]) by lists.denx.de (Postfix) with ESMTPS id D8B97C21C34\n\tfor ; Wed, 20 Sep 2017 02:51:47 +0000 (UTC)","from mail-yw0-f169.google.com (mail-yw0-f169.google.com\n\t[209.85.161.169]) (authenticated)\n\tby conssluserg-05.nifty.com with ESMTP id v8K2pgMW005295\n\tfor ; Wed, 20 Sep 2017 11:51:43 +0900","by mail-yw0-f169.google.com with SMTP id x131so1016642ywa.10\n\tfor ; Tue, 19 Sep 2017 19:51:43 -0700 (PDT)","by 10.37.170.198 with HTTP; Tue, 19 Sep 2017 19:51:01 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=T_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Filter":"OpenDKIM Filter v2.10.3 conssluserg-05.nifty.com v8K2pgMW005295","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nifty.com;\n\ts=dec2015msa; t=1505875903;\n\tbh=ybGiF18xI6QO1bBH75h3bOjL30RqlgutHYiHGJ3dBN4=;\n\th=In-Reply-To:References:From:Date:Subject:To:Cc:From;\n\tb=X/MgzTRIjHYLnRHW2ufpQIIM7bNzHvpS408NJ91/5NNB0MCM2Ow3wUqmcqxA2r9GH\n\tIOMyhrPegFI6MghR8OGEGXWbLtnZ6PVU4TYpon992YX4ftt6Avxc5x9cHSNTUTe6AI\n\t+kou7ev314hlV0HtFON/STPjQ+S3MkCSp1+2WD5uoz2MXbW+96iFYqqPpcUsSDIzy2\n\tpMm9tsyZr+cPeN3RfYedQ+HdbMm22vGowKktM2W/3WMtEedPejKjyP0mg7DN4pf+Me\n\tD0hzkQQoJpmkrI8bcBSgbaz5/wr8oN3ugXVWu0iUActGobfbJAgt7I2jQcg4TveDbD\n\t+ijfVGWuUYutA==","X-Nifty-SrcIP":"[209.85.161.169]","X-Gm-Message-State":"AHPjjUjUGTJLT+ZLLP7RbajCfSaxKmLdDpANkHk5a1WEalTlv+8tsuOH\n\tCQZd/1seVdUZeBEU/8ivZrYt9Q93d/vrL0uMi9g=","X-Google-Smtp-Source":"AOwi7QBlbxMbZGRMgs/GnZUzAD8y8hN2d9cjzcgORQ+/e9cYtRYKSvlaMzoPPnDWAV+GKz9/NlAXX2n54yHTt+w+5MA=","X-Received":"by 10.129.95.138 with SMTP id t132mr2656903ywb.52.1505875902275; \n\tTue, 19 Sep 2017 19:51:42 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170916212331.170463-7-sjg@chromium.org>","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>","From":"Masahiro Yamada ","Date":"Wed, 20 Sep 2017 11:51:01 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Simon Glass ","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Stefan Roese ,\n\tU-Boot Mailing List , Andy Yan\n\t, Maxime Ripard\n\t, Andy Shevchenko\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1771875,"web_url":"http://patchwork.ozlabs.org/comment/1771875/","msgid":"","list_archive_url":null,"date":"2017-09-20T13:49:53","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":6170,"url":"http://patchwork.ozlabs.org/api/people/6170/","name":"Simon Glass","email":"sjg@chromium.org"},"content":"Hi Masahiro,\n\nOn 19 September 2017 at 20:51, Masahiro Yamada\n wrote:\n> Hi Simon,\n>\n>\n> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>\n>>\n>> +menu \"Logging\"\n>> +\n>> +config LOG\n>> + bool \"Enable logging support\"\n>> + help\n>> + This enables support for logging of status and debug messages. These\n>> + can be displayed on the console, recorded in a memory buffer, or\n>> + discarded if not needed. Logging supports various categories and\n>> + levels of severity.\n>> +\n>> +config SPL_LOG\n>> + bool \"Enable logging support in SPL\"\n>> + help\n>> + This enables support for logging of status and debug messages. These\n>> + can be displayed on the console, recorded in a memory buffer, or\n>> + discarded if not needed. Logging supports various categories and\n>> + levels of severity.\n>\n>\n> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>\n> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n> when building for TPL.\n>\n> Since that commit, if you add SPL_ prefixed option,\n> you need to add a TPL_ one as well.\n>\n> I cannot believe why such a commit was accepted.\n\nWell either way is strange. it is strange that SPL is enabled for TPL\nwhen really they are separate.\n\nWe could revert that commit. But how do you think all of this SPL/TPL\ncontrol should actually work? What is intended?\n\nBut I'm OK with not having logging in TPL until we need it.\n\n>\n>\n>\n>\n>> +config LOG_MAX_LEVEL\n>> + int \"Maximum log level to record\"\n>> + depends on LOG\n>> + default 5\n>> + help\n>> + This selects the maximum log level that will be recorded. Any value\n>> + higher than this will be ignored. If possible log statements below\n>> + this level will be discarded at build time. Levels:\n>> +\n>> + 0 - panic\n>> + 1 - critical\n>> + 2 - error\n>> + 3 - warning\n>> + 4 - note\n>> + 5 - info\n>> + 6 - detail\n>> + 7 - debug\n>\n>\n> Please do not invent our own for U-Boot.\n> Just use Linux log level.\n>\n> 0 (KERN_EMERG) system is unusable\n> 1 (KERN_ALERT) action must be taken immediately\n> 2 (KERN_CRIT) critical conditions\n> 3 (KERN_ERR) error conditions\n> 4 (KERN_WARNING) warning conditions\n> 5 (KERN_NOTICE) normal but significant condition\n> 6 (KERN_INFO) informational\n> 7 (KERN_DEBUG) debug-level messages\n\nYes I looked hard at that. The first three seem hard to distinguish in\nU-Boot, but we can keep them I suppose. But most of my problem is with\nthe last two. INFO is what I plan to use for normal printf() output.\nDEBUG is obviously for debugging and often involves vaste amounts of\nstuff (e.g. logging every access to an MMC peripheral). We need\nsomething in between. It could list the accesses to device at a high\nlevel (e.g API calls) but not every little register access.\n\nSo I don't think the Linux levels are suitable at the high end. We\ncould go up to 8 I suppose, instead of trying to save one at the\nbottom?\n\n>\n>\n>\n>\n>> +config LOG_SPL_MAX_LEVEL\n>> + int \"Maximum log level to record in SPL\"\n>> + depends on SPL_LOG\n>> + default 3\n>> + help\n>> + This selects the maximum log level that will be recorded. Any value\n>> + higher than this will be ignored. If possible log statements below\n>> + this level will be discarded at build time. Levels:\n>> +\n>> + 0 - panic\n>> + 1 - critical\n>> + 2 - error\n>> + 3 - warning\n>> + 4 - note\n>> + 5 - info\n>> + 6 - detail\n>> + 7 - debug\n>>\n>\n>\n> If you want to use CONFIG_VAL(LOG_MAX_LEVEL),\n> this must be SPL_LOG_MAX_LEVEL.\n> (this coding mistake is now hidden by another mistake)\n\nOops, yes.\n\n>\n> Again, you will probably end up with TPL_LOG_MAX_LEVEL.\n>\n>\n>\n>\n>> +\n>> +/**\n>> + * log_dispatch() - Send a log record to all log devices for processing\n>> + *\n>> + * The log record is sent to each log device in turn, skipping those which have\n>> + * filters which block the record\n>> + *\n>> + * @rec: Log record to dispatch\n>> + * @return 0 (meaning success)\n>> + */\n>> +static int log_dispatch(struct log_rec *rec)\n>> +{\n>> + struct log_device *ldev;\n>> +\n>> + list_for_each_entry(ldev, &gd->log_head, sibling_node) {\n>> + if (log_passes_filters(ldev, rec))\n>> + ldev->drv->emit(ldev, rec);\n>> + }\n>> +\n>> + return 0;\n>> +}\n>> +\n>> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,\n>> + int line, const char *func, const char *fmt, ...)\n>> +{\n>> + char buf[CONFIG_SYS_CBSIZE];\n>> + struct log_rec rec;\n>> + va_list args;\n>> +\n>> + rec.cat = cat;\n>> + rec.level = level;\n>> + rec.file = file;\n>> + rec.line = line;\n>> + rec.func = func;\n>> + va_start(args, fmt);\n>> + vsnprintf(buf, sizeof(buf), fmt, args);\n>> + va_end(args);\n>> + rec.msg = buf;\n>> + if (!gd || !(gd->flags & GD_FLG_LOG_READY)) {\n>> + if (gd)\n>> + gd->log_drop_count++;\n>> + return -ENOSYS;\n>> + }\n>> + log_dispatch(&rec);\n>> +\n>> + return 0;\n>> +}\n>> +\n>> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],\n>> + enum log_level_t max_level, const char *file_list)\n>> +{\n>> + struct log_filter *filt;\n>> + struct log_device *ldev;\n>> + int i;\n>> +\n>> + ldev = log_device_find_by_name(drv_name);\n>> + if (!ldev)\n>> + return -ENOENT;\n>> + filt = (struct log_filter *)calloc(1, sizeof(*filt));\n>> + if (!filt)\n>> + return -ENOMEM;\n>> +\n>> + if (cat_list) {\n>> + filt->flags |= LOGFF_HAS_CAT;\n>> + for (i = 0; ; i++) {\n>> + if (i == ARRAY_SIZE(filt->cat_list))\n>> + return -ENOSPC;\n>> + filt->cat_list[i] = cat_list[i];\n>> + if (cat_list[i] == LOGC_END)\n>> + break;\n>> + }\n>> + }\n>> + filt->max_level = max_level;\n>> + if (file_list) {\n>> + filt->file_list = strdup(file_list);\n>> + if (!filt->file_list)\n>> + goto nomem;\n>> + }\n>> + filt->filter_num = ldev->next_filter_num++;\n>> + INIT_LIST_HEAD(&filt->sibling_node);\n>> + list_add_tail(&filt->sibling_node, &ldev->filter_head);\n>> +\n>> + return filt->filter_num;\n>> +\n>> +nomem:\n>> + free(filt);\n>> + return -ENOMEM;\n>> +}\n>> +\n>> +int log_remove_filter(const char *drv_name, int filter_num)\n>> +{\n>> + struct log_filter *filt;\n>> + struct log_device *ldev;\n>> +\n>> + ldev = log_device_find_by_name(drv_name);\n>> + if (!ldev)\n>> + return -ENOENT;\n>> +\n>> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {\n>> + if (filt->filter_num == filter_num) {\n>> + list_del(&filt->sibling_node);\n>> + free(filt);\n>> +\n>> + return 0;\n>> + }\n>> + }\n>> +\n>> + return -ENOENT;\n>> +}\n>\n>\n> I am not sure if this luxury filter is needed.\n> After all, the purpose is debugging.\n> The printf() debugging is useful, but only during testing.\n> Once quality code is established, most of debug message should\n> generally be removed from upstream code.\n\nBut not logging, and this is the point. It is very useful to have\nbasic logging information recorded for every boot, and the normal\nprintf() output is not detailed enough. For example for verified boot\nI want to see details about boot failures and what went wrong (key\nverification, etc.). So I expect that at least INFO (and probably\nDETAIL) logging would be useful to record for such a system, even if\nit does not appear on the console (in fact the console would normally\nbe disabled in such a system).\n\n>\n>\n>\n>> +int log_init(void)\n>> +{\n>> + struct log_driver *drv = ll_entry_start(struct log_driver, log_driver);\n>> + const int count = ll_entry_count(struct log_driver, log_driver);\n>> + struct log_driver *end = drv + count;\n>> +\n>> + /*\n>> + * We cannot add runtime data to the driver since it is likely stored\n>> + * in rodata. Instead, set up a 'device' corresponding to each driver.\n>> + * We only support having a single device.\n>> + */\n>> + INIT_LIST_HEAD((struct list_head *)&gd->log_head);\n>> + while (drv < end) {\n>> + struct log_device *ldev;\n>> +\n>> + ldev = calloc(1, sizeof(*ldev));\n>> + if (!ldev) {\n>> + debug(\"%s: Cannot allocate memory\\n\", __func__);\n>> + return -ENOMEM;\n>> + }\n>> + INIT_LIST_HEAD(&ldev->sibling_node);\n>\n> This INIT_LIST_HEAD() is unneeded.\n\nHow come? If I don't do that how will the list be set up?\n\n>\n>\n>\n>> + INIT_LIST_HEAD(&ldev->filter_head);\n>> + ldev->drv = drv;\n>> + list_add_tail(&ldev->sibling_node,\n>> + (struct list_head *)&gd->log_head);\n>\n> I know that this (struct list_head *) cast is needed,\n> but it is very unfortunate.\n> I believe gd_t is design mistake in the first place,\n> but U-Boot has gone to unfixable already...\n\nWith driver model I added DM_ROOT_NON_CONST. I suppose we could use\nsomething like that.\n\nBut perhaps we should start dropping the 'volatile' in gd? I'm not\nsure why it is needed.\n\n>\n>\n>\n>\n>\n>> +#if CONFIG_VAL(LOG_MAX_LEVEL)\n>> +#define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)\n>> +#else\n>> +#define _LOG_MAX_LEVEL LOGL_INFO\n>> +#endif\n>\n>\n> I cannot understand your intention of #if CONFIG_VAL(LOG_MAX_LEVEL).\n> From your Kconfig entry,\n>\n> 0 - panic\n> 1 - critical\n> 2 - error\n> 3 - warning\n> 4 - note\n> 5 - info\n> 6 - detail\n> 7 - debug\n>\n> I expect CONFIG_VAL(LOG_MAX_LEVEL) is an integer value\n> range 0 to 7.\n>\n> If the log level is 0 (= critial), the conditional is false, so\n> _LOG_MAX_LEVEL is set to \"info\" level.\n>\n> Why is this #if necessary?\n\nIf we don't have CONFIG_LOG enabled then this value will not exist.\n\n>\n>\n>\n>\n>\n>\n>\n>\n>> +/* Emit a log record if the level is less that the maximum */\n>> +#define log(_cat, _level, _fmt, _args...) ({ \\\n>> + int _l = _level; \\\n>> + if (_l > _LOG_MAX_LEVEL) \\\n>> + continue; \\\n>> + _log(_cat, _l, __FILE__, __LINE__, __func__, \\\n>> + pr_fmt(_fmt), ##_args); \\\n>> + })\n>\n>\n> This is neither while() nor for().\n> The bare use of \"continue\" made me really surprised.\n\nYes that may not be supported. I'll fix it.\n\nRegards,\nSimon","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"v15lasaA\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"A5IF8k8e\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xy1Qs4YwPz9sP1\n\tfor ;\n\tWed, 20 Sep 2017 23:52:53 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 735AAC21F49; Wed, 20 Sep 2017 13:51:03 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id C9D33C21F19;\n\tWed, 20 Sep 2017 13:50:24 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid E112CC21F6C; Wed, 20 Sep 2017 13:50:19 +0000 (UTC)","from mail-qt0-f177.google.com (mail-qt0-f177.google.com\n\t[209.85.216.177])\n\tby lists.denx.de (Postfix) with ESMTPS id 69C70C21F61\n\tfor ; Wed, 20 Sep 2017 13:50:15 +0000 (UTC)","by mail-qt0-f177.google.com with SMTP id i13so2793692qtc.11\n\tfor ; Wed, 20 Sep 2017 06:50:15 -0700 (PDT)","by 10.200.37.200 with HTTP; Wed, 20 Sep 2017 06:49:53 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID autolearn=unavailable\n\tautolearn_force=no version=3.4.0","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=JCv4XYyrXEaA/NQXURkf6Kg4uB3Hv1uhn9qsEd07wGk=;\n\tb=v15lasaAratYpiC+eFCf1oAhZh2IyXbTNxbDLV9UjGeYmb5dy2Qr6NU9Jp9xnUUlFp\n\teKuVJ/e4uon3D9+Li/ucahVTY/NhQqwR9hXudmRO6ei8pBeFbeCjNwL81OwdYEaMn7tx\n\tZPAxDXhLY7iGjrVWJghq8fdwy5N+73p84s9izqA1CjaypFZrJjZSr/DyvdGGALCIbplo\n\tCc//AhDSWGYWV1ZQYs7zGyMcPMt5bNe+HX9BjXU7dKoSEOEL8lvrNQup0cwfm4C+Wb35\n\tV1Xqce4gi1KxSiRyIKGnVZa9SXwMCxfVTgFj6dwi3QiEEVK5NXp8cF8tTqeUZ2AU7rDL\n\tmddg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=JCv4XYyrXEaA/NQXURkf6Kg4uB3Hv1uhn9qsEd07wGk=;\n\tb=A5IF8k8eMwAkQdv2ledkDLtY2iEhVzAoP3TqM0i4Ch1APC6drMN0ZgNB22rDAhlTnA\n\tLDw575ANHunY+If3MiH0R1K4z4cUJWwD3NriaEu3krzDObYAH3YmjDXI90wtCKLHEKao\n\tjJ/ol8vlCxmIFSw/QFDY1kYu5T29AGHvuWges="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=JCv4XYyrXEaA/NQXURkf6Kg4uB3Hv1uhn9qsEd07wGk=;\n\tb=aGbchoZIsgBmKsdTgkcI2LUa8OO+Q1J23RA96xk8XcdA3CPxVy68g5vY3MkXOwjTMN\n\tlQdBc0mNAjsoDjTxW6eisBB2XdYEV+Vm8/EcN2UaplP14hK4JBWooSA+k+E1S6Dxene/\n\tPYN2/jRT54V/EFq677JOi33v1ik0Hf5UzniBObtbkX/24ClAF3iHNBNgxUP2PyqNxFd1\n\tCRRBaQn9OkODWZyTOOKnke/xnLVjrsLT+EdiWkuPqWt8NUdWi12USOz4BIoj3XCfyH9P\n\tD21iRDbvsJ3uKTY2HzHwGrRnItoOZCRUBlozRUWQF3qBqxfnRckx9ffdhYnOq+UuNggI\n\tOmMA==","X-Gm-Message-State":"AHPjjUjZmE2YbP/+9u8zO6Bv8E/Z6r1q1OZkkLIqTqC2//FcqF8f31po\n\tm+L48YOCKvxAswl5Rc3TFCX7yC8qVXjyUCo0jSqo1g==","X-Google-Smtp-Source":"AOwi7QD973I63lHoSbXD5yrSN7Cq7NwfNUGVkjdCnNAvdk59P4T3dE4Tm+0X+dQ2tXsBjEpCQWag2kcmIYsYfLkgkQo=","X-Received":"by 10.237.35.238 with SMTP id k43mr7643181qtc.214.1505915413916; \n\tWed, 20 Sep 2017 06:50:13 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t","From":"Simon Glass ","Date":"Wed, 20 Sep 2017 07:49:53 -0600","X-Google-Sender-Auth":"gyt3YGucim1X52FHIR2VKWIT0Kc","Message-ID":"","To":"Masahiro Yamada ","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Stefan Roese ,\n\tU-Boot Mailing List , Andy Yan\n\t, Maxime Ripard\n\t, Andy Shevchenko\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1771876,"web_url":"http://patchwork.ozlabs.org/comment/1771876/","msgid":"","list_archive_url":null,"date":"2017-09-20T13:50:06","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":6170,"url":"http://patchwork.ozlabs.org/api/people/6170/","name":"Simon Glass","email":"sjg@chromium.org"},"content":"Hi Bin,\n\nOn 17 September 2017 at 21:45, Bin Meng wrote:\n> Hi Simon,\n>\n> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass wrote:\n>> Add the logging header file and implementation with some configuration\n>> options to control it.\n>>\n>> Signed-off-by: Simon Glass \n>> ---\n>>\n>> MAINTAINERS | 9 ++\n>> common/Kconfig | 56 +++++++++\n>> common/Makefile | 1 +\n>> common/log.c | 246 +++++++++++++++++++++++++++++++++++++\n>> include/asm-generic/global_data.h | 5 +\n>> include/log.h | 247 ++++++++++++++++++++++++++++++++++++--\n>> 6 files changed, 555 insertions(+), 9 deletions(-)\n>> create mode 100644 common/log.c\n>>\n>> diff --git a/MAINTAINERS b/MAINTAINERS\n>> index 04acf2b89d..eb420afa8d 100644\n>> --- a/MAINTAINERS\n>> +++ b/MAINTAINERS\n>> @@ -290,6 +290,15 @@ S: Maintained\n>> T: git git://git.denx.de/u-boot-i2c.git\n>> F: drivers/i2c/\n>>\n>> +LOGGING\n>> +M: Simon Glass \n>> +S: Maintained\n>> +T: git git://git.denx.de/u-boot.git\n>> +F: common/log.c\n>> +F: cmd/log.c\n>> +F: test/log/log_test.c\n>> +F: test/py/tests/test_log.py\n>\n> test/log/log_test.c and test/py/tests/test_log.py have not been\n> introduced at this point.\n\nOK I can tweak that,\n[..]\n\n>> +/**\n>> + * struct log_driver - a driver which accepts and processes log records\n>> + *\n>> + * @name: Name of driver\n>> + */\n>> +struct log_driver {\n>> + const char *name;\n>> + /**\n>> + * emit() - emit a log record\n>> + *\n>> + * Called by the log system to pass a log record to a particular driver\n>> + * for processing. The filter is checked before calling this function.\n>> + */\n>> + int (*emit)(struct log_device *ldev, struct log_rec *rec);\n>> +};\n>> +\n>\n> So we are creating a new type of non-DM driver which is log-specific?\n> How about we add this emit to the existing uclass driver that can be\n> used as the log driver? (eg: blk devices with file system?)\n\nYes that's right. I think I can link it to DM once it starts up, but a\nlogging of DM start-up is useful.\n\nRe your idea are you saying add an emit() method to UCLASS_BLK?\n\nRegards,\nSimon","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"maW0dzWO\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"XIPlcLWZ\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xy1RW1zsJz9s7f\n\tfor ;\n\tWed, 20 Sep 2017 23:53:27 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid DFA94C21F45; Wed, 20 Sep 2017 13:51:55 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id A3CDEC21F3C;\n\tWed, 20 Sep 2017 13:51:53 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 97D40C21F56; Wed, 20 Sep 2017 13:50:32 +0000 (UTC)","from mail-qt0-f181.google.com (mail-qt0-f181.google.com\n\t[209.85.216.181])\n\tby lists.denx.de (Postfix) with ESMTPS id DA560C21EAE\n\tfor ; Wed, 20 Sep 2017 13:50:28 +0000 (UTC)","by mail-qt0-f181.google.com with SMTP id i50so2822244qtf.0\n\tfor ; Wed, 20 Sep 2017 06:50:28 -0700 (PDT)","by 10.200.37.200 with HTTP; Wed, 20 Sep 2017 06:50:06 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW,\n\tRCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID autolearn=unavailable\n\tautolearn_force=no version=3.4.0","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=gqdKV7zkgGlc2r04GwZJwl8u0itwIHSSYrt4y/rywSc=;\n\tb=maW0dzWOUprU0Cv6X0Gzv8W67BtOu+9pNIhuFdlI0wIGrgj4s7aqtTby+3+Jswtauh\n\tePI6jzMKELi49WSlr3xf9TKnR+Y+vdc5fH2CApZ3hQCuwj3jNhXlgF5CjMcFvVKONp76\n\tJqj3Do9nNFhCZBxh2yVRe33Ro9TNgFzsTJHoCdRBpZx+PPeLJWZZ+onThvDEzHWHOV8e\n\tk6NLRpcjmZcbZAFuOvR6oMZSoeQ+A5tsaxf0PiGtjFcYWoOYx7k/IX2Y3ZCguEQweWkr\n\tK4HpXDAGiV3DEtwtm4BvARwSli/fxdveTK1w76AuKdnsotn4JGEjWP0sOgwSINITjNGT\n\tpuCQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=gqdKV7zkgGlc2r04GwZJwl8u0itwIHSSYrt4y/rywSc=;\n\tb=XIPlcLWZ2dUFTVvoolVvGTJhmyZIgEa3EEQetDk3ykEI/iWTcRje4GX6lNwdrQ5m2m\n\tD3ZxYkqdTYon67jqE9G3E94rLjsV3fBfHN6ydpTZEsIhmQ33W1PNxJ2X1hP9y5f9nDo+\n\tdIZKS9VGEqs0LiAn/odX+a8ulAzzNGYIKNpDs="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=gqdKV7zkgGlc2r04GwZJwl8u0itwIHSSYrt4y/rywSc=;\n\tb=lhJh+HRWB+kDCZ6KF5uzk30TH/M/KMeTWKZxRdERSZyxPgUBReZhexMszfGTKWJjhw\n\trCv1RwrJxqeOboQyxFrVisTT2csUBeLbbbo5IxIHCmhIoRDn+m7j2xlOG77srpGSlh2o\n\tCZ7PRu010zIewrPKaa5virC+ggsH6XDG7J7eYr5IXGe+7xTsGX7qFLJWPNJmVd9tb38w\n\tSD+3PtGlRtZRgXmpo6XZ6WByVAPAjHz/7LRxuUCG8Necchf7SUbrpRxvVkAW6LDfRi/D\n\tXpIB86jJoTTy9Cf0NzJpsujFE5WIzP1L4yBZMlaK6LqDCtFsu6omS1jBkqlXm1dCx9LT\n\tByRQ==","X-Gm-Message-State":"AHPjjUgPadHbdReWi0lXpYRbl3ylKkNd1TV2xKinQtIZ4yFs8xYOmhHS\n\tvuc/ibMfQ9TqfXVX9wLsZD1wKJfR56b7P1aBIRnc1xWZ","X-Google-Smtp-Source":"AOwi7QAakNGG00fkuRXmbbXHt1oIJDy713uvZmoTap0mis53gJAASaITg2SHiiBpPgQNc3wczoZtNNkV/VwO52kduoo=","X-Received":"by 10.200.5.132 with SMTP id a4mr7701521qth.318.1505915427573;\n\tWed, 20 Sep 2017 06:50:27 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t","From":"Simon Glass ","Date":"Wed, 20 Sep 2017 07:50:06 -0600","X-Google-Sender-Auth":"eH1WKsxpdn-J9Cku2wQQmPcOXYI","Message-ID":"","To":"Bin Meng ","Cc":"Tom Rini , Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Stefan Roese , =?utf-8?b?xYF1a2Fz?=\n\t=?utf-8?q?z_Majewski?= ,\n\tAndy Yan , Maxime Ripard\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1771930,"web_url":"http://patchwork.ozlabs.org/comment/1771930/","msgid":"<4B670262-EFFA-4649-A656-4F189AB1A156@theobroma-systems.com>","list_archive_url":null,"date":"2017-09-20T14:37:23","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":53488,"url":"http://patchwork.ozlabs.org/api/people/53488/","name":"Philipp Tomsich","email":"philipp.tomsich@theobroma-systems.com"},"content":"Masahiro & Simon,\n\n> On 20 Sep 2017, at 15:49, Simon Glass wrote:\n> \n> Hi Masahiro,\n> \n> On 19 September 2017 at 20:51, Masahiro Yamada\n> wrote:\n>> Hi Simon,\n>> \n>> \n>> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>> \n>>> \n>>> +menu \"Logging\"\n>>> +\n>>> +config LOG\n>>> + bool \"Enable logging support\"\n>>> + help\n>>> + This enables support for logging of status and debug messages. These\n>>> + can be displayed on the console, recorded in a memory buffer, or\n>>> + discarded if not needed. Logging supports various categories and\n>>> + levels of severity.\n>>> +\n>>> +config SPL_LOG\n>>> + bool \"Enable logging support in SPL\"\n>>> + help\n>>> + This enables support for logging of status and debug messages. These\n>>> + can be displayed on the console, recorded in a memory buffer, or\n>>> + discarded if not needed. Logging supports various categories and\n>>> + levels of severity.\n>> \n>> \n>> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>> \n>> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n>> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n>> when building for TPL.\n>> \n>> Since that commit, if you add SPL_ prefixed option,\n>> you need to add a TPL_ one as well.\n>> \n>> I cannot believe why such a commit was accepted.\n> \n> Well either way is strange. it is strange that SPL is enabled for TPL\n> when really they are separate.\n> \n> We could revert that commit. But how do you think all of this SPL/TPL\n> control should actually work? What is intended?\n> \n> But I'm OK with not having logging in TPL until we need it.\n\nIf we don’t differentiate between TPL_ and SPL_, we’ll eventually run into\nsize issues for TPL and the $(SPL_TPL_) mechanism will not match the\nCONFIG_IS_ENABLED() mechanism.\n\nI don’t think that anyone will miss this much in TPL and that this can be\nsafely left off for TPL (if space was not at a premium in TPL, then why\nhave a TPL at all…)\n\n> \n>> \n>> \n>> \n>> \n>>> +config LOG_MAX_LEVEL\n>>> + int \"Maximum log level to record\"\n>>> + depends on LOG\n>>> + default 5\n>>> + help\n>>> + This selects the maximum log level that will be recorded. Any value\n>>> + higher than this will be ignored. If possible log statements below\n>>> + this level will be discarded at build time. Levels:\n>>> +\n>>> + 0 - panic\n>>> + 1 - critical\n>>> + 2 - error\n>>> + 3 - warning\n>>> + 4 - note\n>>> + 5 - info\n>>> + 6 - detail\n>>> + 7 - debug\n>> \n>> \n>> Please do not invent our own for U-Boot.\n>> Just use Linux log level.\n>> \n>> 0 (KERN_EMERG) system is unusable\n>> 1 (KERN_ALERT) action must be taken immediately\n>> 2 (KERN_CRIT) critical conditions\n>> 3 (KERN_ERR) error conditions\n>> 4 (KERN_WARNING) warning conditions\n>> 5 (KERN_NOTICE) normal but significant condition\n>> 6 (KERN_INFO) informational\n>> 7 (KERN_DEBUG) debug-level messages\n> \n> Yes I looked hard at that. The first three seem hard to distinguish in\n> U-Boot, but we can keep them I suppose. But most of my problem is with\n> the last two. INFO is what I plan to use for normal printf() output.\n> DEBUG is obviously for debugging and often involves vaste amounts of\n> stuff (e.g. logging every access to an MMC peripheral). We need\n> something in between. It could list the accesses to device at a high\n> level (e.g API calls) but not every little register access.\n> \n> So I don't think the Linux levels are suitable at the high end. We\n> could go up to 8 I suppose, instead of trying to save one at the\n> bottom?\n\nI would expect KERN_NOTICE to be used for normal printf output, KERN_INFO\nfor more verbose output and DEBUG would be the granularity for tracing through\ndrivers (i.e. the MMC example given).\n\nRegards,\nPhilipp.\n\n> \n>> \n>> \n>> \n>> \n>>> +config LOG_SPL_MAX_LEVEL\n>>> + int \"Maximum log level to record in SPL\"\n>>> + depends on SPL_LOG\n>>> + default 3\n>>> + help\n>>> + This selects the maximum log level that will be recorded. Any value\n>>> + higher than this will be ignored. If possible log statements below\n>>> + this level will be discarded at build time. Levels:\n>>> +\n>>> + 0 - panic\n>>> + 1 - critical\n>>> + 2 - error\n>>> + 3 - warning\n>>> + 4 - note\n>>> + 5 - info\n>>> + 6 - detail\n>>> + 7 - debug\n>>> \n>> \n>> \n>> If you want to use CONFIG_VAL(LOG_MAX_LEVEL),\n>> this must be SPL_LOG_MAX_LEVEL.\n>> (this coding mistake is now hidden by another mistake)\n> \n> Oops, yes.\n> \n>> \n>> Again, you will probably end up with TPL_LOG_MAX_LEVEL.\n>> \n>> \n>> \n>> \n>>> +\n>>> +/**\n>>> + * log_dispatch() - Send a log record to all log devices for processing\n>>> + *\n>>> + * The log record is sent to each log device in turn, skipping those which have\n>>> + * filters which block the record\n>>> + *\n>>> + * @rec: Log record to dispatch\n>>> + * @return 0 (meaning success)\n>>> + */\n>>> +static int log_dispatch(struct log_rec *rec)\n>>> +{\n>>> + struct log_device *ldev;\n>>> +\n>>> + list_for_each_entry(ldev, &gd->log_head, sibling_node) {\n>>> + if (log_passes_filters(ldev, rec))\n>>> + ldev->drv->emit(ldev, rec);\n>>> + }\n>>> +\n>>> + return 0;\n>>> +}\n>>> +\n>>> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,\n>>> + int line, const char *func, const char *fmt, ...)\n>>> +{\n>>> + char buf[CONFIG_SYS_CBSIZE];\n>>> + struct log_rec rec;\n>>> + va_list args;\n>>> +\n>>> + rec.cat = cat;\n>>> + rec.level = level;\n>>> + rec.file = file;\n>>> + rec.line = line;\n>>> + rec.func = func;\n>>> + va_start(args, fmt);\n>>> + vsnprintf(buf, sizeof(buf), fmt, args);\n>>> + va_end(args);\n>>> + rec.msg = buf;\n>>> + if (!gd || !(gd->flags & GD_FLG_LOG_READY)) {\n>>> + if (gd)\n>>> + gd->log_drop_count++;\n>>> + return -ENOSYS;\n>>> + }\n>>> + log_dispatch(&rec);\n>>> +\n>>> + return 0;\n>>> +}\n>>> +\n>>> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],\n>>> + enum log_level_t max_level, const char *file_list)\n>>> +{\n>>> + struct log_filter *filt;\n>>> + struct log_device *ldev;\n>>> + int i;\n>>> +\n>>> + ldev = log_device_find_by_name(drv_name);\n>>> + if (!ldev)\n>>> + return -ENOENT;\n>>> + filt = (struct log_filter *)calloc(1, sizeof(*filt));\n>>> + if (!filt)\n>>> + return -ENOMEM;\n>>> +\n>>> + if (cat_list) {\n>>> + filt->flags |= LOGFF_HAS_CAT;\n>>> + for (i = 0; ; i++) {\n>>> + if (i == ARRAY_SIZE(filt->cat_list))\n>>> + return -ENOSPC;\n>>> + filt->cat_list[i] = cat_list[i];\n>>> + if (cat_list[i] == LOGC_END)\n>>> + break;\n>>> + }\n>>> + }\n>>> + filt->max_level = max_level;\n>>> + if (file_list) {\n>>> + filt->file_list = strdup(file_list);\n>>> + if (!filt->file_list)\n>>> + goto nomem;\n>>> + }\n>>> + filt->filter_num = ldev->next_filter_num++;\n>>> + INIT_LIST_HEAD(&filt->sibling_node);\n>>> + list_add_tail(&filt->sibling_node, &ldev->filter_head);\n>>> +\n>>> + return filt->filter_num;\n>>> +\n>>> +nomem:\n>>> + free(filt);\n>>> + return -ENOMEM;\n>>> +}\n>>> +\n>>> +int log_remove_filter(const char *drv_name, int filter_num)\n>>> +{\n>>> + struct log_filter *filt;\n>>> + struct log_device *ldev;\n>>> +\n>>> + ldev = log_device_find_by_name(drv_name);\n>>> + if (!ldev)\n>>> + return -ENOENT;\n>>> +\n>>> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {\n>>> + if (filt->filter_num == filter_num) {\n>>> + list_del(&filt->sibling_node);\n>>> + free(filt);\n>>> +\n>>> + return 0;\n>>> + }\n>>> + }\n>>> +\n>>> + return -ENOENT;\n>>> +}\n>> \n>> \n>> I am not sure if this luxury filter is needed.\n>> After all, the purpose is debugging.\n>> The printf() debugging is useful, but only during testing.\n>> Once quality code is established, most of debug message should\n>> generally be removed from upstream code.\n> \n> But not logging, and this is the point. It is very useful to have\n> basic logging information recorded for every boot, and the normal\n> printf() output is not detailed enough. For example for verified boot\n> I want to see details about boot failures and what went wrong (key\n> verification, etc.). So I expect that at least INFO (and probably\n> DETAIL) logging would be useful to record for such a system, even if\n> it does not appear on the console (in fact the console would normally\n> be disabled in such a system).\n> \n>> \n>> \n>> \n>>> +int log_init(void)\n>>> +{\n>>> + struct log_driver *drv = ll_entry_start(struct log_driver, log_driver);\n>>> + const int count = ll_entry_count(struct log_driver, log_driver);\n>>> + struct log_driver *end = drv + count;\n>>> +\n>>> + /*\n>>> + * We cannot add runtime data to the driver since it is likely stored\n>>> + * in rodata. Instead, set up a 'device' corresponding to each driver.\n>>> + * We only support having a single device.\n>>> + */\n>>> + INIT_LIST_HEAD((struct list_head *)&gd->log_head);\n>>> + while (drv < end) {\n>>> + struct log_device *ldev;\n>>> +\n>>> + ldev = calloc(1, sizeof(*ldev));\n>>> + if (!ldev) {\n>>> + debug(\"%s: Cannot allocate memory\\n\", __func__);\n>>> + return -ENOMEM;\n>>> + }\n>>> + INIT_LIST_HEAD(&ldev->sibling_node);\n>> \n>> This INIT_LIST_HEAD() is unneeded.\n> \n> How come? If I don't do that how will the list be set up?\n> \n>> \n>> \n>> \n>>> + INIT_LIST_HEAD(&ldev->filter_head);\n>>> + ldev->drv = drv;\n>>> + list_add_tail(&ldev->sibling_node,\n>>> + (struct list_head *)&gd->log_head);\n>> \n>> I know that this (struct list_head *) cast is needed,\n>> but it is very unfortunate.\n>> I believe gd_t is design mistake in the first place,\n>> but U-Boot has gone to unfixable already...\n> \n> With driver model I added DM_ROOT_NON_CONST. I suppose we could use\n> something like that.\n> \n> But perhaps we should start dropping the 'volatile' in gd? I'm not\n> sure why it is needed.\n> \n>> \n>> \n>> \n>> \n>> \n>>> +#if CONFIG_VAL(LOG_MAX_LEVEL)\n>>> +#define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)\n>>> +#else\n>>> +#define _LOG_MAX_LEVEL LOGL_INFO\n>>> +#endif\n>> \n>> \n>> I cannot understand your intention of #if CONFIG_VAL(LOG_MAX_LEVEL).\n>> From your Kconfig entry,\n>> \n>> 0 - panic\n>> 1 - critical\n>> 2 - error\n>> 3 - warning\n>> 4 - note\n>> 5 - info\n>> 6 - detail\n>> 7 - debug\n>> \n>> I expect CONFIG_VAL(LOG_MAX_LEVEL) is an integer value\n>> range 0 to 7.\n>> \n>> If the log level is 0 (= critial), the conditional is false, so\n>> _LOG_MAX_LEVEL is set to \"info\" level.\n>> \n>> Why is this #if necessary?\n> \n> If we don't have CONFIG_LOG enabled then this value will not exist.\n> \n>> \n>> \n>> \n>> \n>> \n>> \n>> \n>> \n>>> +/* Emit a log record if the level is less that the maximum */\n>>> +#define log(_cat, _level, _fmt, _args...) ({ \\\n>>> + int _l = _level; \\\n>>> + if (_l > _LOG_MAX_LEVEL) \\\n>>> + continue; \\\n>>> + _log(_cat, _l, __FILE__, __LINE__, __func__, \\\n>>> + pr_fmt(_fmt), ##_args); \\\n>>> + })\n>> \n>> \n>> This is neither while() nor for().\n>> The bare use of \"continue\" made me really surprised.\n> \n> Yes that may not be supported. I'll fix it.\n> \n> Regards,\n> Simon\n> _______________________________________________\n> U-Boot mailing list\n> U-Boot@lists.denx.de\n> https://lists.denx.de/listinfo/u-boot","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xy2QW43lhz9sNr\n\tfor ;\n\tThu, 21 Sep 2017 00:37:39 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 49999C21EC8; Wed, 20 Sep 2017 14:37:36 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 7A883C21EC8;\n\tWed, 20 Sep 2017 14:37:32 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 94F13C21EC8; Wed, 20 Sep 2017 14:37:31 +0000 (UTC)","from mail.theobroma-systems.com (vegas.theobroma-systems.com\n\t[144.76.126.164])\n\tby lists.denx.de (Postfix) with ESMTPS id 28932C21DA2\n\tfor ; Wed, 20 Sep 2017 14:37:31 +0000 (UTC)","from 89-104-28-141.customer.bnet.at ([89.104.28.141]:54557\n\thelo=[192.168.2.129]) by mail.theobroma-systems.com with esmtpsa\n\t(TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80)\n\t(envelope-from )\n\tid 1dug7w-0003v5-FS; Wed, 20 Sep 2017 16:37:24 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"*","X-Spam-Status":"No, score=1.0 required=5.0 tests=HK_NAME_DR autolearn=no\n\tautolearn_force=no version=3.4.0","Mime-Version":"1.0 (Mac OS X Mail 10.3 \\(3273\\))","From":"\"Dr. Philipp Tomsich\" ","In-Reply-To":"","Date":"Wed, 20 Sep 2017 16:37:23 +0200","Message-Id":"<4B670262-EFFA-4649-A656-4F189AB1A156@theobroma-systems.com>","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t","To":"Simon Glass ","X-Mailer":"Apple Mail (2.3273)","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Andy Yan , Maxime\n\tRipard , Stefan Roese , \n\tFranklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1771934,"web_url":"http://patchwork.ozlabs.org/comment/1771934/","msgid":"","list_archive_url":null,"date":"2017-09-20T14:41:07","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":64981,"url":"http://patchwork.ozlabs.org/api/people/64981/","name":"Bin Meng","email":"bmeng.cn@gmail.com"},"content":"Hi Simon,\n\nOn Wed, Sep 20, 2017 at 9:50 PM, Simon Glass wrote:\n> Hi Bin,\n>\n> On 17 September 2017 at 21:45, Bin Meng wrote:\n>> Hi Simon,\n>>\n>> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass wrote:\n>>> Add the logging header file and implementation with some configuration\n>>> options to control it.\n>>>\n>>> Signed-off-by: Simon Glass \n>>> ---\n>>>\n>>> MAINTAINERS | 9 ++\n>>> common/Kconfig | 56 +++++++++\n>>> common/Makefile | 1 +\n>>> common/log.c | 246 +++++++++++++++++++++++++++++++++++++\n>>> include/asm-generic/global_data.h | 5 +\n>>> include/log.h | 247 ++++++++++++++++++++++++++++++++++++--\n>>> 6 files changed, 555 insertions(+), 9 deletions(-)\n>>> create mode 100644 common/log.c\n>>>\n>>> diff --git a/MAINTAINERS b/MAINTAINERS\n>>> index 04acf2b89d..eb420afa8d 100644\n>>> --- a/MAINTAINERS\n>>> +++ b/MAINTAINERS\n>>> @@ -290,6 +290,15 @@ S: Maintained\n>>> T: git git://git.denx.de/u-boot-i2c.git\n>>> F: drivers/i2c/\n>>>\n>>> +LOGGING\n>>> +M: Simon Glass \n>>> +S: Maintained\n>>> +T: git git://git.denx.de/u-boot.git\n>>> +F: common/log.c\n>>> +F: cmd/log.c\n>>> +F: test/log/log_test.c\n>>> +F: test/py/tests/test_log.py\n>>\n>> test/log/log_test.c and test/py/tests/test_log.py have not been\n>> introduced at this point.\n>\n> OK I can tweak that,\n> [..]\n>\n>>> +/**\n>>> + * struct log_driver - a driver which accepts and processes log records\n>>> + *\n>>> + * @name: Name of driver\n>>> + */\n>>> +struct log_driver {\n>>> + const char *name;\n>>> + /**\n>>> + * emit() - emit a log record\n>>> + *\n>>> + * Called by the log system to pass a log record to a particular driver\n>>> + * for processing. The filter is checked before calling this function.\n>>> + */\n>>> + int (*emit)(struct log_device *ldev, struct log_rec *rec);\n>>> +};\n>>> +\n>>\n>> So we are creating a new type of non-DM driver which is log-specific?\n>> How about we add this emit to the existing uclass driver that can be\n>> used as the log driver? (eg: blk devices with file system?)\n>\n> Yes that's right. I think I can link it to DM once it starts up, but a\n> logging of DM start-up is useful.\n>\n> Re your idea are you saying add an emit() method to UCLASS_BLK?\n>\n\nYep, something like\n\n#ifdef CONFIG_LOG\n .log_emit = xxx_log_emit,\n#endif\n\nProbably we need a flag to find out which driver provides such log\nfunctionality.\n\nRegards,\nBin","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"K4u+1TL5\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xy2Vy1lwdz9sNr\n\tfor ;\n\tThu, 21 Sep 2017 00:41:28 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 09E50C21F77; Wed, 20 Sep 2017 14:41:13 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 71FBEC21EC8;\n\tWed, 20 Sep 2017 14:41:10 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 20584C21EC8; Wed, 20 Sep 2017 14:41:09 +0000 (UTC)","from mail-wm0-f65.google.com (mail-wm0-f65.google.com\n\t[74.125.82.65])\n\tby lists.denx.de (Postfix) with ESMTPS id 9C7CFC21DA2\n\tfor ; Wed, 20 Sep 2017 14:41:08 +0000 (UTC)","by mail-wm0-f65.google.com with SMTP id i131so2654798wma.1\n\tfor ; Wed, 20 Sep 2017 07:41:08 -0700 (PDT)","by 10.223.145.3 with HTTP; Wed, 20 Sep 2017 07:41:07 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=FREEMAIL_FROM,\n\tRCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=b1zPVb7BRQrEcguD3TubP5ypY2PIxuBEX7CiBE870Z4=;\n\tb=K4u+1TL5HS9FSTvVYOMfNsYGzogh/QogEk5s/g+KKnLONrAF0xfTf3aXBZVld9Hdco\n\tG8G2DomgNONrn5F59m8R3kmaGh3M8TVxB4ZkJPnChQQ792rj+MqvWVXJ8NBiJUHU9MTr\n\thk1qbjVXunFDYz8pgp8OcQjIp6I3d55G88VcpTzC4NL09kA1vaE238zfFFYMiBoZjQRy\n\tYzXJGYIzfuYbuPA992ElIXugLQfkPM3XMUknkaY0Z+11bHrHJZ8Qp0FwO2nv+F4Ypz/x\n\tXA3XBRGvdCROCHOUojSlC6UYe3Xg1nCf5B+yo/VbIXwNqk1UnlCruBBAaotbstoIw4Uu\n\tRHgA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=b1zPVb7BRQrEcguD3TubP5ypY2PIxuBEX7CiBE870Z4=;\n\tb=abmjRVmcxxHP6cVuMcaLAvo5l5rR4m6r0j//qYIr2z/7tCuLGz+4sqgEJ/KE9DK4Yw\n\tN6S8EGSPZNTWkohmXxyfOIIeGghOd3Cdy5WN645g7oiNAUA/wgpWVpUPuV4gcv4inCWL\n\ttm6Ux5LXHWoHSjUbhRvxvxvm3mDpxhIHz2F804Kwbmvtb4cbiCC5ezMxvCzP4gpR/eRh\n\tK8HGpkJwyQ6Z/1YKxw+uC/ry23CAwA+sZ/OtRbKCZoss+zmj5ym5kI3JkWlHLncO1FJc\n\tTyJQW+zI/2h45vqKWv4pOmZm0ZtURwE1ZvkIoq7rN9Uq/UO/BCaCbkmIhYaJkfSeNrDa\n\ttChw==","X-Gm-Message-State":"AHPjjUijUD3jatLh/iND34stB3GvGcV7CIoBCT9mfNTjNflTcvoAysDM\n\t4O0bVEZCXE1Cx4j8HFN2+y0NPtE9qhoh1SwDFtU=","X-Google-Smtp-Source":"AOwi7QA/fpSDTalTPGUu+FfGaeo1uWnjamLjUWzikrVFusZI41rlilJZNBFiOzJBN5cJfH1k5KoeuY/TkfdOsGg7jCo=","X-Received":"by 10.28.238.218 with SMTP id j87mr4441683wmi.44.1505918468201; \n\tWed, 20 Sep 2017 07:41:08 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t","From":"Bin Meng ","Date":"Wed, 20 Sep 2017 22:41:07 +0800","Message-ID":"","To":"Simon Glass ","Cc":"Tom Rini , Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Stefan Roese , =?utf-8?b?xYF1a2Fz?=\n\t=?utf-8?q?z_Majewski?= ,\n\tAndy Yan , Maxime Ripard\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1771947,"web_url":"http://patchwork.ozlabs.org/comment/1771947/","msgid":"<8FD7D10A-1AC7-455B-9E85-EC58B5E92D76@theobroma-systems.com>","list_archive_url":null,"date":"2017-09-20T14:51:56","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":53488,"url":"http://patchwork.ozlabs.org/api/people/53488/","name":"Philipp Tomsich","email":"philipp.tomsich@theobroma-systems.com"},"content":"> On 20 Sep 2017, at 16:41, Bin Meng wrote:\n> \n> Hi Simon,\n> \n> On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass wrote:\n>> Hi Bin,\n>> \n>> On 17 September 2017 at 21:45, Bin Meng wrote:\n>>> Hi Simon,\n>>> \n>>> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass wrote:\n>>>> Add the logging header file and implementation with some configuration\n>>>> options to control it.\n>>>> \n>>>> Signed-off-by: Simon Glass \n>>>> ---\n>>>> \n>>>> MAINTAINERS | 9 ++\n>>>> common/Kconfig | 56 +++++++++\n>>>> common/Makefile | 1 +\n>>>> common/log.c | 246 +++++++++++++++++++++++++++++++++++++\n>>>> include/asm-generic/global_data.h | 5 +\n>>>> include/log.h | 247 ++++++++++++++++++++++++++++++++++++--\n>>>> 6 files changed, 555 insertions(+), 9 deletions(-)\n>>>> create mode 100644 common/log.c\n>>>> \n>>>> diff --git a/MAINTAINERS b/MAINTAINERS\n>>>> index 04acf2b89d..eb420afa8d 100644\n>>>> --- a/MAINTAINERS\n>>>> +++ b/MAINTAINERS\n>>>> @@ -290,6 +290,15 @@ S: Maintained\n>>>> T: git git://git.denx.de/u-boot-i2c.git\n>>>> F: drivers/i2c/\n>>>> \n>>>> +LOGGING\n>>>> +M: Simon Glass \n>>>> +S: Maintained\n>>>> +T: git git://git.denx.de/u-boot.git\n>>>> +F: common/log.c\n>>>> +F: cmd/log.c\n>>>> +F: test/log/log_test.c\n>>>> +F: test/py/tests/test_log.py\n>>> \n>>> test/log/log_test.c and test/py/tests/test_log.py have not been\n>>> introduced at this point.\n>> \n>> OK I can tweak that,\n>> [..]\n>> \n>>>> +/**\n>>>> + * struct log_driver - a driver which accepts and processes log records\n>>>> + *\n>>>> + * @name: Name of driver\n>>>> + */\n>>>> +struct log_driver {\n>>>> + const char *name;\n>>>> + /**\n>>>> + * emit() - emit a log record\n>>>> + *\n>>>> + * Called by the log system to pass a log record to a particular driver\n>>>> + * for processing. The filter is checked before calling this function.\n>>>> + */\n>>>> + int (*emit)(struct log_device *ldev, struct log_rec *rec);\n>>>> +};\n>>>> +\n>>> \n>>> So we are creating a new type of non-DM driver which is log-specific?\n>>> How about we add this emit to the existing uclass driver that can be\n>>> used as the log driver? (eg: blk devices with file system?)\n>> \n>> Yes that's right. I think I can link it to DM once it starts up, but a\n>> logging of DM start-up is useful.\n>> \n>> Re your idea are you saying add an emit() method to UCLASS_BLK?\n>> \n> \n> Yep, something like\n> \n> #ifdef CONFIG_LOG\n> .log_emit = xxx_log_emit,\n> #endif\n> \n> Probably we need a flag to find out which driver provides such log\n> functionality.\n\nI had started to sketch (but have been able to fully flesh this out, due to \nother priorities intervening) a mechanism for MISC devices that might\nbe a good fit for this.\n\nMy work centers around the idea of having devices comply to “protocols”\nand was aimed at distinguishing between an efuse-device and a GbE \nRGMII-control (one where we need to adjust the RGMII clock depending\non the link rate the PHY negotiated) device.\n\nI.e. I had started to implement something along the lines of:\n\n\t/**\n\t * misc_supports - tests if a misc device complies to a given protocol\n\t *\n\t * protocols can either be ‘how data is processed after calling write()’,\n\t * ‘how data is presented for a read()’ or ‘what ioctl-values are supported’.\n\t * The assumption is that protocols can be versioned and higher versions\n\t * offer full backward compatibility (i.e. a client for “Ethernet PHY control, v1”\n\t * can work with a driver implementing v1 or any higher version).\n\t */\n\tbool misc_supports(struct udevice *dev, const char *protocol, u32 version);\n\nand a way to register a list of protocols as part of the misc uclass-priv for\nany given driver.\n\nThis would allow us to enumerate all MISC devices until we find one that\ncomplies to the ‘logging’-protocol and then invoke write or ioctl on it.\n\nRegards,\nPhilipp.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xy2lG2brpz9sNr\n\tfor ;\n\tThu, 21 Sep 2017 00:52:10 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 24C08C21F7D; Wed, 20 Sep 2017 14:52:06 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id F1411C21EC8;\n\tWed, 20 Sep 2017 14:52:03 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 5FFD9C21EC8; Wed, 20 Sep 2017 14:52:03 +0000 (UTC)","from mail.theobroma-systems.com (vegas.theobroma-systems.com\n\t[144.76.126.164])\n\tby lists.denx.de (Postfix) with ESMTPS id 13CE4C21DA2\n\tfor ; Wed, 20 Sep 2017 14:52:03 +0000 (UTC)","from 89-104-28-141.customer.bnet.at ([89.104.28.141]:54663\n\thelo=[192.168.2.129]) by mail.theobroma-systems.com with esmtpsa\n\t(TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80)\n\t(envelope-from )\n\tid 1dugM2-0004NT-D7; Wed, 20 Sep 2017 16:51:58 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"*","X-Spam-Status":"No, score=1.0 required=5.0 tests=HK_NAME_DR autolearn=no\n\tautolearn_force=no version=3.4.0","Mime-Version":"1.0 (Mac OS X Mail 10.3 \\(3273\\))","From":"\"Dr. Philipp Tomsich\" ","In-Reply-To":"","Date":"Wed, 20 Sep 2017 16:51:56 +0200","Message-Id":"<8FD7D10A-1AC7-455B-9E85-EC58B5E92D76@theobroma-systems.com>","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t","To":"Bin Meng ","X-Mailer":"Apple Mail (2.3273)","Cc":"Tom Rini , Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Stefan Roese , =?utf-8?b?xYF1a2Fz?=\n\t=?utf-8?q?z_Majewski?= ,\n\tAndy Yan , Maxime Ripard\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1772094,"web_url":"http://patchwork.ozlabs.org/comment/1772094/","msgid":"","list_archive_url":null,"date":"2017-09-20T17:19:58","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":65882,"url":"http://patchwork.ozlabs.org/api/people/65882/","name":"Masahiro Yamada","email":"yamada.masahiro@socionext.com"},"content":"Hi Simon,\n\n\n2017-09-20 22:49 GMT+09:00 Simon Glass :\n> Hi Masahiro,\n>\n> On 19 September 2017 at 20:51, Masahiro Yamada\n> wrote:\n>> Hi Simon,\n>>\n>>\n>> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>>\n>>>\n>>> +menu \"Logging\"\n>>> +\n>>> +config LOG\n>>> + bool \"Enable logging support\"\n>>> + help\n>>> + This enables support for logging of status and debug messages. These\n>>> + can be displayed on the console, recorded in a memory buffer, or\n>>> + discarded if not needed. Logging supports various categories and\n>>> + levels of severity.\n>>> +\n>>> +config SPL_LOG\n>>> + bool \"Enable logging support in SPL\"\n>>> + help\n>>> + This enables support for logging of status and debug messages. These\n>>> + can be displayed on the console, recorded in a memory buffer, or\n>>> + discarded if not needed. Logging supports various categories and\n>>> + levels of severity.\n>>\n>>\n>> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>>\n>> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n>> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n>> when building for TPL.\n>>\n>> Since that commit, if you add SPL_ prefixed option,\n>> you need to add a TPL_ one as well.\n>>\n>> I cannot believe why such a commit was accepted.\n>\n> Well either way is strange. it is strange that SPL is enabled for TPL\n> when really they are separate.\n>\n> We could revert that commit. But how do you think all of this SPL/TPL\n> control should actually work? What is intended?\n>\n> But I'm OK with not having logging in TPL until we need it.\n\nI will explain it in another mail.\n\n\n>>\n>>\n>>\n>>\n>>> +config LOG_MAX_LEVEL\n>>> + int \"Maximum log level to record\"\n>>> + depends on LOG\n>>> + default 5\n>>> + help\n>>> + This selects the maximum log level that will be recorded. Any value\n>>> + higher than this will be ignored. If possible log statements below\n>>> + this level will be discarded at build time. Levels:\n>>> +\n>>> + 0 - panic\n>>> + 1 - critical\n>>> + 2 - error\n>>> + 3 - warning\n>>> + 4 - note\n>>> + 5 - info\n>>> + 6 - detail\n>>> + 7 - debug\n>>\n>>\n>> Please do not invent our own for U-Boot.\n>> Just use Linux log level.\n>>\n>> 0 (KERN_EMERG) system is unusable\n>> 1 (KERN_ALERT) action must be taken immediately\n>> 2 (KERN_CRIT) critical conditions\n>> 3 (KERN_ERR) error conditions\n>> 4 (KERN_WARNING) warning conditions\n>> 5 (KERN_NOTICE) normal but significant condition\n>> 6 (KERN_INFO) informational\n>> 7 (KERN_DEBUG) debug-level messages\n>\n> Yes I looked hard at that. The first three seem hard to distinguish in\n> U-Boot, but we can keep them I suppose. But most of my problem is with\n> the last two. INFO is what I plan to use for normal printf() output.\n> DEBUG is obviously for debugging and often involves vaste amounts of\n> stuff (e.g. logging every access to an MMC peripheral). We need\n> something in between. It could list the accesses to device at a high\n> level (e.g API calls) but not every little register access.\n>\n> So I don't think the Linux levels are suitable at the high end. We\n> could go up to 8 I suppose, instead of trying to save one at the\n> bottom?\n\n\nIn fact, Linux has one more for debug.\n dev_vdbg() is widely used in Linux.\n\nIf you like, we can add one more level:\n\n 8 (KERN_VDEBUG) verbose debug messages\n\n\nPerhaps, logging every access to an MMC peripheral\nmight belong to the vdbg level.\n\n\n\nBTW, what do you mean \"INFO is what I plan to use for normal printf() output\"\n\nIs that mean printf() is equivalent to pr_info()?\nIf loglevel is 6 or smaller, will all print() be silent?\nIf so, probably we can not use command line interface.\n\n\n\n\n\n>>\n>>\n>>\n>>\n>>> +\n>>> +/**\n>>> + * log_dispatch() - Send a log record to all log devices for processing\n>>> + *\n>>> + * The log record is sent to each log device in turn, skipping those which have\n>>> + * filters which block the record\n>>> + *\n>>> + * @rec: Log record to dispatch\n>>> + * @return 0 (meaning success)\n>>> + */\n>>> +static int log_dispatch(struct log_rec *rec)\n>>> +{\n>>> + struct log_device *ldev;\n>>> +\n>>> + list_for_each_entry(ldev, &gd->log_head, sibling_node) {\n>>> + if (log_passes_filters(ldev, rec))\n>>> + ldev->drv->emit(ldev, rec);\n>>> + }\n>>> +\n>>> + return 0;\n>>> +}\n>>> +\n>>> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,\n>>> + int line, const char *func, const char *fmt, ...)\n>>> +{\n>>> + char buf[CONFIG_SYS_CBSIZE];\n>>> + struct log_rec rec;\n>>> + va_list args;\n>>> +\n>>> + rec.cat = cat;\n>>> + rec.level = level;\n>>> + rec.file = file;\n>>> + rec.line = line;\n>>> + rec.func = func;\n>>> + va_start(args, fmt);\n>>> + vsnprintf(buf, sizeof(buf), fmt, args);\n>>> + va_end(args);\n>>> + rec.msg = buf;\n>>> + if (!gd || !(gd->flags & GD_FLG_LOG_READY)) {\n>>> + if (gd)\n>>> + gd->log_drop_count++;\n>>> + return -ENOSYS;\n>>> + }\n>>> + log_dispatch(&rec);\n>>> +\n>>> + return 0;\n>>> +}\n>>> +\n>>> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],\n>>> + enum log_level_t max_level, const char *file_list)\n>>> +{\n>>> + struct log_filter *filt;\n>>> + struct log_device *ldev;\n>>> + int i;\n>>> +\n>>> + ldev = log_device_find_by_name(drv_name);\n>>> + if (!ldev)\n>>> + return -ENOENT;\n>>> + filt = (struct log_filter *)calloc(1, sizeof(*filt));\n>>> + if (!filt)\n>>> + return -ENOMEM;\n>>> +\n>>> + if (cat_list) {\n>>> + filt->flags |= LOGFF_HAS_CAT;\n>>> + for (i = 0; ; i++) {\n>>> + if (i == ARRAY_SIZE(filt->cat_list))\n>>> + return -ENOSPC;\n>>> + filt->cat_list[i] = cat_list[i];\n>>> + if (cat_list[i] == LOGC_END)\n>>> + break;\n>>> + }\n>>> + }\n>>> + filt->max_level = max_level;\n>>> + if (file_list) {\n>>> + filt->file_list = strdup(file_list);\n>>> + if (!filt->file_list)\n>>> + goto nomem;\n>>> + }\n>>> + filt->filter_num = ldev->next_filter_num++;\n>>> + INIT_LIST_HEAD(&filt->sibling_node);\n>>> + list_add_tail(&filt->sibling_node, &ldev->filter_head);\n>>> +\n>>> + return filt->filter_num;\n>>> +\n>>> +nomem:\n>>> + free(filt);\n>>> + return -ENOMEM;\n>>> +}\n>>> +\n>>> +int log_remove_filter(const char *drv_name, int filter_num)\n>>> +{\n>>> + struct log_filter *filt;\n>>> + struct log_device *ldev;\n>>> +\n>>> + ldev = log_device_find_by_name(drv_name);\n>>> + if (!ldev)\n>>> + return -ENOENT;\n>>> +\n>>> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {\n>>> + if (filt->filter_num == filter_num) {\n>>> + list_del(&filt->sibling_node);\n>>> + free(filt);\n>>> +\n>>> + return 0;\n>>> + }\n>>> + }\n>>> +\n>>> + return -ENOENT;\n>>> +}\n>>\n>>\n>> I am not sure if this luxury filter is needed.\n>> After all, the purpose is debugging.\n>> The printf() debugging is useful, but only during testing.\n>> Once quality code is established, most of debug message should\n>> generally be removed from upstream code.\n>\n> But not logging, and this is the point. It is very useful to have\n> basic logging information recorded for every boot, and the normal\n> printf() output is not detailed enough. For example for verified boot\n> I want to see details about boot failures and what went wrong (key\n> verification, etc.).\n\nThis should be warn level, (or notice at least)\nnot info.\n\n\n> So I expect that at least INFO (and probably\n> DETAIL) logging would be useful to record for such a system, even if\n> it does not appear on the console (in fact the console would normally\n> be disabled in such a system).\n\nOK. Logging is useful, but your example will be used\nwith info level.\n\nI still do not understand the necessity of\ncategory / file filtering.\n\n\n>>\n>>\n>>\n>>> +int log_init(void)\n>>> +{\n>>> + struct log_driver *drv = ll_entry_start(struct log_driver, log_driver);\n>>> + const int count = ll_entry_count(struct log_driver, log_driver);\n>>> + struct log_driver *end = drv + count;\n>>> +\n>>> + /*\n>>> + * We cannot add runtime data to the driver since it is likely stored\n>>> + * in rodata. Instead, set up a 'device' corresponding to each driver.\n>>> + * We only support having a single device.\n>>> + */\n>>> + INIT_LIST_HEAD((struct list_head *)&gd->log_head);\n>>> + while (drv < end) {\n>>> + struct log_device *ldev;\n>>> +\n>>> + ldev = calloc(1, sizeof(*ldev));\n>>> + if (!ldev) {\n>>> + debug(\"%s: Cannot allocate memory\\n\", __func__);\n>>> + return -ENOMEM;\n>>> + }\n>>> + INIT_LIST_HEAD(&ldev->sibling_node);\n>>\n>> This INIT_LIST_HEAD() is unneeded.\n>\n> How come? If I don't do that how will the list be set up?\n\nBecause ldev->sibling_node is not a list_head.\nIt is used as a list node.\n\n\nSee include/linux/list.h\nfor the implementation of list_add_tail().\n\nThe argument \"head\" must be initialized in advance,\nbut \"new\" does not need initialization.\n\n\n\n>>\n>>\n>>\n>>> + INIT_LIST_HEAD(&ldev->filter_head);\n>>> + ldev->drv = drv;\n>>> + list_add_tail(&ldev->sibling_node,\n>>> + (struct list_head *)&gd->log_head);\n\nHere, gd->log_head is a head, so\nINIT_LIST_HEAD((struct list_head *)&gd->log_head);\nis correct.\n\nINIT_LIST_HEAD(&ldev->sibling_node);\nis non-sense.\n\n\n\n\n\n>> I know that this (struct list_head *) cast is needed,\n>> but it is very unfortunate.\n>> I believe gd_t is design mistake in the first place,\n>> but U-Boot has gone to unfixable already...\n>\n> With driver model I added DM_ROOT_NON_CONST. I suppose we could use\n> something like that.\n>\n> But perhaps we should start dropping the 'volatile' in gd? I'm not\n> sure why it is needed.\n\nMe neither.\n\n\n>>\n>>\n>>\n>>\n>>\n>>> +#if CONFIG_VAL(LOG_MAX_LEVEL)\n>>> +#define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)\n>>> +#else\n>>> +#define _LOG_MAX_LEVEL LOGL_INFO\n>>> +#endif\n>>\n>>\n>> I cannot understand your intention of #if CONFIG_VAL(LOG_MAX_LEVEL).\n>> From your Kconfig entry,\n>>\n>> 0 - panic\n>> 1 - critical\n>> 2 - error\n>> 3 - warning\n>> 4 - note\n>> 5 - info\n>> 6 - detail\n>> 7 - debug\n>>\n>> I expect CONFIG_VAL(LOG_MAX_LEVEL) is an integer value\n>> range 0 to 7.\n>>\n>> If the log level is 0 (= critial), the conditional is false, so\n>> _LOG_MAX_LEVEL is set to \"info\" level.\n>>\n>> Why is this #if necessary?\n>\n> If we don't have CONFIG_LOG enabled then this value will not exist.\n>\n\nThis implementation is wrong.\n\nYou are abusing the fact that\nU-Boot does not specify -Wundef option.\n\n\n--------------------->8-----------------\n-Wundef\n Warn if an undefined identifier is evaluated in an #if directive.\nSuch identifiers are replaced with zero.\n--------------------->8-----------------\n\n\n\nRight, U-Boot does not give -Wundef,\nbut it is just because -Wundef causes too many warnings.\n\nIdeally, we should fix the code,\nthen enable the option like Linux.\n\nWe should use #ifdef for testing a option that may or may not be defined.\nUsing #if for options that may not be defined is a bad coding way.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=nifty.com header.i=@nifty.com\n\theader.b=\"bHn7JNIV\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xy63G2pm6z9s0Z\n\tfor ;\n\tThu, 21 Sep 2017 03:21:12 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid CB156C21E51; Wed, 20 Sep 2017 17:21:09 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 2C521C21CB3;\n\tWed, 20 Sep 2017 17:21:04 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 0CDDBC21CB3; Wed, 20 Sep 2017 17:21:03 +0000 (UTC)","from conssluserg-06.nifty.com (conssluserg-06.nifty.com\n\t[210.131.2.91]) by lists.denx.de (Postfix) with ESMTPS id E359AC21C35\n\tfor ; Wed, 20 Sep 2017 17:21:01 +0000 (UTC)","from mail-yw0-f171.google.com (mail-yw0-f171.google.com\n\t[209.85.161.171]) (authenticated)\n\tby conssluserg-06.nifty.com with ESMTP id v8KHKerO032668\n\tfor ; Thu, 21 Sep 2017 02:20:40 +0900","by mail-yw0-f171.google.com with SMTP id w22so2363867ywa.13\n\tfor ; Wed, 20 Sep 2017 10:20:40 -0700 (PDT)","by 10.37.170.198 with HTTP; Wed, 20 Sep 2017 10:19:58 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=T_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Filter":"OpenDKIM Filter v2.10.3 conssluserg-06.nifty.com v8KHKerO032668","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nifty.com;\n\ts=dec2015msa; t=1505928041;\n\tbh=tk+g1sCdqBg1IeoN7MFgJxC2H3PV7UWoDyUCMFqy4ow=;\n\th=In-Reply-To:References:From:Date:Subject:To:Cc:From;\n\tb=bHn7JNIV57VMx47g7wJ4L07blqxy+Aj8h7cuQJ/zoqG1gebyH8QRTIiCkFQiEN6Dy\n\t4RTa7PZTWHrU1NtpFLS8szmkkwuZu5KlECf2I+tLccmuRrXWsTZEahtK7PWB7iwATb\n\tRggfr704eelahsIWTKwuLsSbPwQnSZPfH0LckaiqyPuRZAouqjbBYE8XV4rdVtxyC+\n\toA5dsOWEboUL70tVHBj5X+rj0m8CqtojE5IRRvcuBD/HXYoy2T3t0AieQmTlE2B9um\n\tsZGOZDE9iQy+TPIF3KpNcOvSUmqOrOFAFJXhnCzaf9SxJx80j2xD78KrZ7h1xQj7PG\n\t00xDGXX7iF9GQ==","X-Nifty-SrcIP":"[209.85.161.171]","X-Gm-Message-State":"AHPjjUgEOxwA5nRoBMTPITDaXgOKY046bkXpif8HuQ3P5VACR7KOxrOL\n\tVW8+svc4k73UJw+TALY/yza0PtH/G7jz+lA5BsM=","X-Google-Smtp-Source":"AOwi7QCdH8dgZScka+MFp3n4fCZXFgyumM84ABUdy/9rYW7acyeSi9OKp2b79F0pOeDV9WD0/mJPpGtes7YVWS5gWzo=","X-Received":"by 10.37.39.197 with SMTP id n188mr4397137ybn.119.1505928039514; \n\tWed, 20 Sep 2017 10:20:39 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t","From":"Masahiro Yamada ","Date":"Thu, 21 Sep 2017 02:19:58 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Simon Glass ","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Andy Yan , Maxime\n\tRipard , Stefan Roese , \n\tFranklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1772106,"web_url":"http://patchwork.ozlabs.org/comment/1772106/","msgid":"","list_archive_url":null,"date":"2017-09-20T17:34:01","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":65882,"url":"http://patchwork.ozlabs.org/api/people/65882/","name":"Masahiro Yamada","email":"yamada.masahiro@socionext.com"},"content":"2017-09-20 23:37 GMT+09:00 Dr. Philipp Tomsich\n:\n> Masahiro & Simon,\n>\n>> On 20 Sep 2017, at 15:49, Simon Glass wrote:\n>>\n>> Hi Masahiro,\n>>\n>> On 19 September 2017 at 20:51, Masahiro Yamada\n>> wrote:\n>>> Hi Simon,\n>>>\n>>>\n>>> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>>>\n>>>>\n>>>> +menu \"Logging\"\n>>>> +\n>>>> +config LOG\n>>>> + bool \"Enable logging support\"\n>>>> + help\n>>>> + This enables support for logging of status and debug messages. These\n>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>> + discarded if not needed. Logging supports various categories and\n>>>> + levels of severity.\n>>>> +\n>>>> +config SPL_LOG\n>>>> + bool \"Enable logging support in SPL\"\n>>>> + help\n>>>> + This enables support for logging of status and debug messages. These\n>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>> + discarded if not needed. Logging supports various categories and\n>>>> + levels of severity.\n>>>\n>>>\n>>> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>>>\n>>> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n>>> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n>>> when building for TPL.\n>>>\n>>> Since that commit, if you add SPL_ prefixed option,\n>>> you need to add a TPL_ one as well.\n>>>\n>>> I cannot believe why such a commit was accepted.\n>>\n>> Well either way is strange. it is strange that SPL is enabled for TPL\n>> when really they are separate.\n>>\n>> We could revert that commit. But how do you think all of this SPL/TPL\n>> control should actually work? What is intended?\n>>\n>> But I'm OK with not having logging in TPL until we need it.\n>\n> If we don’t differentiate between TPL_ and SPL_, we’ll eventually run into\n> size issues for TPL and the $(SPL_TPL_) mechanism will not match the\n> CONFIG_IS_ENABLED() mechanism.\n>\n> I don’t think that anyone will miss this much in TPL and that this can be\n> safely left off for TPL (if space was not at a premium in TPL, then why\n> have a TPL at all…)\n\n\nThe motivation of TPL is\nthe image size is really limited\nfor the secondary boot loader in some cases.\n\n\nInstead of:\n SPL -> TPL -> U-Boot full\n\nI'd rather want to see:\n -> SPL -> U-Boot full\n\n\n is implemented in an ad-hoc way under board or soc directory.\nIf the space is premium, there is no room for DM, DT-ctrl in the .\n\nThen, remove the current TPL support.\n\n\n\nIt was only some old freescale boards that used TPL,\nso I was expecting they would die out sooner or later.\n\nRecently, lion-rk3368_defconfig was added wit TPL.\n\nOther rk3368 boards do not enable TPL.\n\nWhy does that board need TPL?\n\n\n\n\n\n>>>\n>>>\n>>>\n>>>\n>>>> +config LOG_MAX_LEVEL\n>>>> + int \"Maximum log level to record\"\n>>>> + depends on LOG\n>>>> + default 5\n>>>> + help\n>>>> + This selects the maximum log level that will be recorded. Any value\n>>>> + higher than this will be ignored. If possible log statements below\n>>>> + this level will be discarded at build time. Levels:\n>>>> +\n>>>> + 0 - panic\n>>>> + 1 - critical\n>>>> + 2 - error\n>>>> + 3 - warning\n>>>> + 4 - note\n>>>> + 5 - info\n>>>> + 6 - detail\n>>>> + 7 - debug\n>>>\n>>>\n>>> Please do not invent our own for U-Boot.\n>>> Just use Linux log level.\n>>>\n>>> 0 (KERN_EMERG) system is unusable\n>>> 1 (KERN_ALERT) action must be taken immediately\n>>> 2 (KERN_CRIT) critical conditions\n>>> 3 (KERN_ERR) error conditions\n>>> 4 (KERN_WARNING) warning conditions\n>>> 5 (KERN_NOTICE) normal but significant condition\n>>> 6 (KERN_INFO) informational\n>>> 7 (KERN_DEBUG) debug-level messages\n>>\n>> Yes I looked hard at that. The first three seem hard to distinguish in\n>> U-Boot, but we can keep them I suppose. But most of my problem is with\n>> the last two. INFO is what I plan to use for normal printf() output.\n>> DEBUG is obviously for debugging and often involves vaste amounts of\n>> stuff (e.g. logging every access to an MMC peripheral). We need\n>> something in between. It could list the accesses to device at a high\n>> level (e.g API calls) but not every little register access.\n>>\n>> So I don't think the Linux levels are suitable at the high end. We\n>> could go up to 8 I suppose, instead of trying to save one at the\n>> bottom?\n>\n> I would expect KERN_NOTICE to be used for normal printf output, KERN_INFO\n> for more verbose output and DEBUG would be the granularity for tracing through\n> drivers (i.e. the MMC example given).\n>\n\n\nIn my opinion, printf() and pr_*() should be different concept.\n\n\nprintf() (and puts(), putc(), etc.) should be turned on/off\nby CONFIG_SILENT_CONSOLE.\nThese are direct access to console,\nso they should always and immediately print messages\nunless CONFIG_SILENT_CONSOLE is defined.\nWe may want to use the command interface regardless of\nCONFIG_LOG / CONFIG_LOGLEVEL.\n\n\n\npr_*() are log functions that are controlled by CONFIG_LOGLEVEL.\nThese can send the messages to the console directly,\nor to the ring buffer, or wherever implemented in the log device.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=nifty.com header.i=@nifty.com\n\theader.b=\"VzpQ/iCJ\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xy6MJ1JPvz9s0g\n\tfor ;\n\tThu, 21 Sep 2017 03:35:08 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid D8CA5C21E5D; Wed, 20 Sep 2017 17:34:57 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 4900DC21D55;\n\tWed, 20 Sep 2017 17:34:51 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 07698C21D5F; Wed, 20 Sep 2017 17:34:49 +0000 (UTC)","from conssluserg-05.nifty.com (conssluserg-05.nifty.com\n\t[210.131.2.90]) by lists.denx.de (Postfix) with ESMTPS id DB9E5C21D55\n\tfor ; Wed, 20 Sep 2017 17:34:48 +0000 (UTC)","from mail-yw0-f179.google.com (mail-yw0-f179.google.com\n\t[209.85.161.179]) (authenticated)\n\tby conssluserg-05.nifty.com with ESMTP id v8KHYg2R022779\n\tfor ; Thu, 21 Sep 2017 02:34:43 +0900","by mail-yw0-f179.google.com with SMTP id p10so2408908ywh.8\n\tfor ; Wed, 20 Sep 2017 10:34:43 -0700 (PDT)","by 10.37.170.198 with HTTP; Wed, 20 Sep 2017 10:34:01 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=T_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Filter":"OpenDKIM Filter v2.10.3 conssluserg-05.nifty.com v8KHYg2R022779","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nifty.com;\n\ts=dec2015msa; t=1505928883;\n\tbh=rC3ioHiKfGJxTwgq7Fa4C/NOS9Ld73R/v0Yhcd/+A8s=;\n\th=In-Reply-To:References:From:Date:Subject:To:Cc:From;\n\tb=VzpQ/iCJQIiLX/np7UB2daICZLCokH7XHyfUu9R5hYTv+2iiYMujDom3q7cFvn4RE\n\t5/hYTZJnS2lz5vqmaY5t66300g61z7xu3AxvYQ/pnIjs3lrtaZA1v9XrydjNcKN0lc\n\t9xDCw2E+C47RsHGujXTieTb0SLWyNP06nqdvos+WSamR+YoQNwPjXfc6tm8WUe+wNY\n\tTUZ87gmAycDJEc3pe6eAwcyo/wX41x6xwyOnAG5hSFXwrn9Z+THi3ol3CHoD4CJcgC\n\tWfdmOaJldy3SHgoNjIwidLeOHr+/tikyVmse3x3E9/w9Nw5LvwVlWsIowGfvbf1Ld9\n\tsQt5oYHFivGaA==","X-Nifty-SrcIP":"[209.85.161.179]","X-Gm-Message-State":"AHPjjUg379pkte9b9Ih91jay82HPu3mpZzbrfu9yVT4cCK+5qxuWjH7B\n\tX8CnAhDpdTbe3OEUvrqFTzhcIro+Q6pDN6yNc4Y=","X-Google-Smtp-Source":"AOwi7QCpGUcBDeFHtz+cE+EOXgikdvErL/SUbi7SeLEjoW9Ev6E63jSVs6hV1mLltmh8kZlxs2uwjxEndJhlJWmIcRQ=","X-Received":"by 10.129.95.138 with SMTP id t132mr4431093ywb.52.1505928882055; \n\tWed, 20 Sep 2017 10:34:42 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<4B670262-EFFA-4649-A656-4F189AB1A156@theobroma-systems.com>","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t<4B670262-EFFA-4649-A656-4F189AB1A156@theobroma-systems.com>","From":"Masahiro Yamada ","Date":"Thu, 21 Sep 2017 02:34:01 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"\"Dr. Philipp Tomsich\" ","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Stefan Roese ,\n\tU-Boot Mailing List , Andy Yan\n\t, Maxime Ripard\n\t, Andy Shevchenko\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1772119,"web_url":"http://patchwork.ozlabs.org/comment/1772119/","msgid":"<26FA5DC9-9257-47F9-B15B-5ABC21EBCFAC@theobroma-systems.com>","list_archive_url":null,"date":"2017-09-20T17:51:42","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":53488,"url":"http://patchwork.ozlabs.org/api/people/53488/","name":"Philipp Tomsich","email":"philipp.tomsich@theobroma-systems.com"},"content":"Masahiro,\n\n> On 20 Sep 2017, at 19:34, Masahiro Yamada wrote:\n> \n> 2017-09-20 23:37 GMT+09:00 Dr. Philipp Tomsich\n> :\n>> Masahiro & Simon,\n>> \n>>> On 20 Sep 2017, at 15:49, Simon Glass wrote:\n>>> \n>>> Hi Masahiro,\n>>> \n>>> On 19 September 2017 at 20:51, Masahiro Yamada\n>>> wrote:\n>>>> Hi Simon,\n>>>> \n>>>> \n>>>> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>>>> \n>>>>> \n>>>>> +menu \"Logging\"\n>>>>> +\n>>>>> +config LOG\n>>>>> + bool \"Enable logging support\"\n>>>>> + help\n>>>>> + This enables support for logging of status and debug messages. These\n>>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>>> + discarded if not needed. Logging supports various categories and\n>>>>> + levels of severity.\n>>>>> +\n>>>>> +config SPL_LOG\n>>>>> + bool \"Enable logging support in SPL\"\n>>>>> + help\n>>>>> + This enables support for logging of status and debug messages. These\n>>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>>> + discarded if not needed. Logging supports various categories and\n>>>>> + levels of severity.\n>>>> \n>>>> \n>>>> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>>>> \n>>>> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n>>>> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n>>>> when building for TPL.\n>>>> \n>>>> Since that commit, if you add SPL_ prefixed option,\n>>>> you need to add a TPL_ one as well.\n>>>> \n>>>> I cannot believe why such a commit was accepted.\n>>> \n>>> Well either way is strange. it is strange that SPL is enabled for TPL\n>>> when really they are separate.\n>>> \n>>> We could revert that commit. But how do you think all of this SPL/TPL\n>>> control should actually work? What is intended?\n>>> \n>>> But I'm OK with not having logging in TPL until we need it.\n>> \n>> If we don’t differentiate between TPL_ and SPL_, we’ll eventually run into\n>> size issues for TPL and the $(SPL_TPL_) mechanism will not match the\n>> CONFIG_IS_ENABLED() mechanism.\n>> \n>> I don’t think that anyone will miss this much in TPL and that this can be\n>> safely left off for TPL (if space was not at a premium in TPL, then why\n>> have a TPL at all…)\n> \n> \n> The motivation of TPL is\n> the image size is really limited\n> for the secondary boot loader in some cases.\n> \n> \n> Instead of:\n> SPL -> TPL -> U-Boot full\n\nNote that this was retro-actively defined to be\n\tTPL -> SPL -> U-Boot full\nby Tom at some point and reiterated in\n\thttps://lists.denx.de/pipermail/u-boot/2017-July/299266.html\n\n> I'd rather want to see:\n> -> SPL -> U-Boot full\n\nI would agree, that the stage before the SPL could be chip-specific.\nHowever, this is unlikely to happen quickly as some of the infrastructure \n(e.g. OF_PLATDATA) would have to be easily available to this new TPL\nframework.\n\n> is implemented in an ad-hoc way under board or soc directory.\n> If the space is premium, there is no room for DM, DT-ctrl in the .\n> \n> Then, remove the current TPL support.\n> \n> \n> \n> It was only some old freescale boards that used TPL,\n> so I was expecting they would die out sooner or later.\n> \n> Recently, lion-rk3368_defconfig was added wit TPL.\n> \n> Other rk3368 boards do not enable TPL.\n\nOther RK3368 boards do not have DRAM init support yet (they still use\nthe proprietary vendor blobs and then boot the full U-Boot stage). This\nis gated by the resource availability on Rockchip’s end to add support\nfor those features of the DRAM controller that can not be tested on Lion.\n\n> Why does that board need TPL?\n\nThe RK3368’s boot-ROM loads only 0x7000 bytes as its first stage and\nrequires this to (a) set up clocking and (b) initialise the DRAM controller.\nThe SPL-stage on the RK3368 then runs out of DRAM (still loaded by\nthe boot-ROM), but the size-limit is somewhat relaxed.\n\nOn the RK3368, we take the liberty of reusing as much framework code\nas possible in the TPL (resulting in 21k binary) and using OF_PLATDATA:\nthe features reused include DM, DM_REGMAP, DM_SYSCON, DM_CLK, \nDM_RAM and DM_TIMER. With this, we can both use the same drivers\nas for SPL and full U-Boot and then have a SPL stage that does not rely\nof OF_PLATDATA (but has full OF_CONTROL).\n\nNote that TPL is required for most of the Rockchip boards, if we want to \ndo the DRAM initialisation in U-Boot… those chips that already have their\nDRAM controller drivers merged, usually have a TPL stage (with the\nexception of the RK3399, which supports 192kB for its first stage).\n\nRegards,\nPhilipp.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xy6kn5s5wz9t2V\n\tfor ;\n\tThu, 21 Sep 2017 03:52:00 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid E7E5FC21DDD; Wed, 20 Sep 2017 17:51:55 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id C71A6C21C5D;\n\tWed, 20 Sep 2017 17:51:51 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 89192C21C5D; Wed, 20 Sep 2017 17:51:50 +0000 (UTC)","from mail.theobroma-systems.com (vegas.theobroma-systems.com\n\t[144.76.126.164])\n\tby lists.denx.de (Postfix) with ESMTPS id 49C79C21C35\n\tfor ; Wed, 20 Sep 2017 17:51:50 +0000 (UTC)","from 89-104-28-141.customer.bnet.at ([89.104.28.141]:59552\n\thelo=[192.168.2.129]) by mail.theobroma-systems.com with esmtpsa\n\t(TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80)\n\t(envelope-from )\n\tid 1duj9z-0001jJ-P9; Wed, 20 Sep 2017 19:51:43 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"*","X-Spam-Status":"No, score=1.0 required=5.0 tests=HK_NAME_DR autolearn=no\n\tautolearn_force=no version=3.4.0","Mime-Version":"1.0 (Mac OS X Mail 10.3 \\(3273\\))","From":"\"Dr. Philipp Tomsich\" ","In-Reply-To":"","Date":"Wed, 20 Sep 2017 19:51:42 +0200","Message-Id":"<26FA5DC9-9257-47F9-B15B-5ABC21EBCFAC@theobroma-systems.com>","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t<4B670262-EFFA-4649-A656-4F189AB1A156@theobroma-systems.com>\n\t","To":"Masahiro Yamada ","X-Mailer":"Apple Mail (2.3273)","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Stefan Roese ,\n\tU-Boot Mailing List , Andy Yan\n\t, Maxime Ripard\n\t, Andy Shevchenko\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1772409,"web_url":"http://patchwork.ozlabs.org/comment/1772409/","msgid":"","list_archive_url":null,"date":"2017-09-21T04:58:48","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":6170,"url":"http://patchwork.ozlabs.org/api/people/6170/","name":"Simon Glass","email":"sjg@chromium.org"},"content":"Hi Bin,\n\nOn 20 September 2017 at 08:41, Bin Meng wrote:\n>\n> Hi Simon,\n>\n> On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass wrote:\n> > Hi Bin,\n> >\n> > On 17 September 2017 at 21:45, Bin Meng wrote:\n> >> Hi Simon,\n> >>\n> >> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass wrote:\n> >>> Add the logging header file and implementation with some configuration\n> >>> options to control it.\n> >>>\n> >>> Signed-off-by: Simon Glass \n> >>> ---\n> >>>\n> >>> MAINTAINERS | 9 ++\n> >>> common/Kconfig | 56 +++++++++\n> >>> common/Makefile | 1 +\n> >>> common/log.c | 246 +++++++++++++++++++++++++++++++++++++\n> >>> include/asm-generic/global_data.h | 5 +\n> >>> include/log.h | 247 ++++++++++++++++++++++++++++++++++++--\n> >>> 6 files changed, 555 insertions(+), 9 deletions(-)\n> >>> create mode 100644 common/log.c\n> >>>\n> >>> diff --git a/MAINTAINERS b/MAINTAINERS\n> >>> index 04acf2b89d..eb420afa8d 100644\n> >>> --- a/MAINTAINERS\n> >>> +++ b/MAINTAINERS\n> >>> @@ -290,6 +290,15 @@ S: Maintained\n> >>> T: git git://git.denx.de/u-boot-i2c.git\n> >>> F: drivers/i2c/\n> >>>\n> >>> +LOGGING\n> >>> +M: Simon Glass \n> >>> +S: Maintained\n> >>> +T: git git://git.denx.de/u-boot.git\n> >>> +F: common/log.c\n> >>> +F: cmd/log.c\n> >>> +F: test/log/log_test.c\n> >>> +F: test/py/tests/test_log.py\n> >>\n> >> test/log/log_test.c and test/py/tests/test_log.py have not been\n> >> introduced at this point.\n> >\n> > OK I can tweak that,\n> > [..]\n> >\n> >>> +/**\n> >>> + * struct log_driver - a driver which accepts and processes log records\n> >>> + *\n> >>> + * @name: Name of driver\n> >>> + */\n> >>> +struct log_driver {\n> >>> + const char *name;\n> >>> + /**\n> >>> + * emit() - emit a log record\n> >>> + *\n> >>> + * Called by the log system to pass a log record to a particular driver\n> >>> + * for processing. The filter is checked before calling this function.\n> >>> + */\n> >>> + int (*emit)(struct log_device *ldev, struct log_rec *rec);\n> >>> +};\n> >>> +\n> >>\n> >> So we are creating a new type of non-DM driver which is log-specific?\n> >> How about we add this emit to the existing uclass driver that can be\n> >> used as the log driver? (eg: blk devices with file system?)\n> >\n> > Yes that's right. I think I can link it to DM once it starts up, but a\n> > logging of DM start-up is useful.\n> >\n> > Re your idea are you saying add an emit() method to UCLASS_BLK?\n> >\n>\n> Yep, something like\n>\n> #ifdef CONFIG_LOG\n> .log_emit = xxx_log_emit,\n> #endif\n>\n> Probably we need a flag to find out which driver provides such log\n> functionality.\n\nSo what would the block driver do in that function? I guess I don't\nunderstand what it is for. Can you please give me more information?\n\nRegards,\nSimon","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"PLLh/mSv\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"DzWB2jX+\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xyPYc6tc6z9ryv\n\tfor ;\n\tThu, 21 Sep 2017 15:00:04 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 9BC4FC21F0D; Thu, 21 Sep 2017 04:59:46 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 75A36C21F05;\n\tThu, 21 Sep 2017 04:59:43 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 94E79C21E85; Thu, 21 Sep 2017 04:59:11 +0000 (UTC)","from mail-qk0-f172.google.com (mail-qk0-f172.google.com\n\t[209.85.220.172])\n\tby lists.denx.de (Postfix) with ESMTPS id 52390C21EFE\n\tfor ; Thu, 21 Sep 2017 04:59:10 +0000 (UTC)","by mail-qk0-f172.google.com with SMTP id b82so4737068qkc.4\n\tfor ; Wed, 20 Sep 2017 21:59:10 -0700 (PDT)","by 10.200.37.200 with HTTP; Wed, 20 Sep 2017 21:58:48 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_MSPIKE_H2,\n\tT_DKIM_INVALID autolearn=unavailable autolearn_force=no\n\tversion=3.4.0","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=wuKx1JaJ/WoLh5Rn3PN8NFKDzizdUO/grrYxTG6lynI=;\n\tb=PLLh/mSvOxOCbFpQNHc3NOLS0Wr8ehHmUtSUPXVeHowTj051fzp1c1Go5lePk0Uny4\n\t1swpWtDkOsYCPMiiyMaRvZMktn63tOoDDBI4Ji8B7FswmykHAEWT+JVzVITmGJlZhyeq\n\tc94GRwUXNtk+JaZEUvbYnYicjJbn/n0SUVmvJEFoGvuI7BylXl9E6xRa2qH5Ps68B65Q\n\tFPi1YUeiGWJ4+kA+JE9YWIxS4iLSPpRl8PJzjjtYYYe8C4M0DlUQ0RryipTKACe1hGtM\n\tMz1lM4+zAnKCx8ivI2WoyDrrBzhpwIBdimW6zUDloN1kWtVcBYlECYS6dipK2C4SI4gj\n\ttB1Q==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=wuKx1JaJ/WoLh5Rn3PN8NFKDzizdUO/grrYxTG6lynI=;\n\tb=DzWB2jX+ZvCSA9I4sYpH1hTr58cM+PfvQIoTjaJWkHcaYHnmJRl3mWWM5J0YmuXVcH\n\tIBtH2f4UcOWI5nPXNbVxDacoEb7cTWdCU+jLhN+AmDQHsqJXcofS5ipT8bKjyUsBCxwk\n\tIwRN0mSnn80fBLcVxiPNtGu2023+rz5LF7kF0="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=wuKx1JaJ/WoLh5Rn3PN8NFKDzizdUO/grrYxTG6lynI=;\n\tb=S5zCr+0L5csUHh4jfvOaehPO0eQkB1oDltyc0NQelQrFAYbddSefuBnoo/b7c9260q\n\tGUpGEzTZ61EEuCAaPBWf+aj2zAHRhY32lkOMWFMfQf+0JXymDRwAnTkYS9a+s7JZJmdR\n\tlSll/aKvpEawFSEtn1+sQ4HzuLiv+TwTO1Uuyj3sVK9B5vX+btyv+2ZxoSATCMTJFzwN\n\tBrtMrmcAvCCIB+L6F4fShg8DdscvlThUIjQrFAHRqMOVJ5jR2OuaKMhvVLlVOaPzXBph\n\tswp2/4IiBjTygB2uUX5oeJCdmoJRMGM2EIIhezzWky3ambZnVuoZyaIqyesdfGjkqy/3\n\tAATw==","X-Gm-Message-State":"AHPjjUhYITSWeqM0g9233ncxUT4yro6ytvI/96qYIVexFtRMKBn4EJDb\n\t01j7LtugADQapyVAqKdadYy3B7xzfUsEXnvS1OjiWA==","X-Google-Smtp-Source":"AOwi7QDMkS33PaY4xFDjW23hvCp38xvGywKENhsuMfTYqQVshuyB9C3xE5yzyGged2jru8zwFdjUL8LKyW2vkCbaDL4=","X-Received":"by 10.55.122.130 with SMTP id v124mr1385694qkc.209.1505969948995;\n\tWed, 20 Sep 2017 21:59:08 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t","From":"Simon Glass ","Date":"Wed, 20 Sep 2017 22:58:48 -0600","X-Google-Sender-Auth":"uPhusob0z_mykSyfoEFhyk4QH7Q","Message-ID":"","To":"Bin Meng ","Cc":"Tom Rini , Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Stefan Roese , =?utf-8?b?xYF1a2Fz?=\n\t=?utf-8?q?z_Majewski?= ,\n\tAndy Yan , Maxime Ripard\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1773600,"web_url":"http://patchwork.ozlabs.org/comment/1773600/","msgid":"","list_archive_url":null,"date":"2017-09-22T13:37:54","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":64981,"url":"http://patchwork.ozlabs.org/api/people/64981/","name":"Bin Meng","email":"bmeng.cn@gmail.com"},"content":"Hi Simon,\n\nOn Thu, Sep 21, 2017 at 12:58 PM, Simon Glass wrote:\n> Hi Bin,\n>\n> On 20 September 2017 at 08:41, Bin Meng wrote:\n>>\n>> Hi Simon,\n>>\n>> On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass wrote:\n>> > Hi Bin,\n>> >\n>> > On 17 September 2017 at 21:45, Bin Meng wrote:\n>> >> Hi Simon,\n>> >>\n>> >> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass wrote:\n>> >>> Add the logging header file and implementation with some configuration\n>> >>> options to control it.\n>> >>>\n>> >>> Signed-off-by: Simon Glass \n>> >>> ---\n>> >>>\n>> >>> MAINTAINERS | 9 ++\n>> >>> common/Kconfig | 56 +++++++++\n>> >>> common/Makefile | 1 +\n>> >>> common/log.c | 246 +++++++++++++++++++++++++++++++++++++\n>> >>> include/asm-generic/global_data.h | 5 +\n>> >>> include/log.h | 247 ++++++++++++++++++++++++++++++++++++--\n>> >>> 6 files changed, 555 insertions(+), 9 deletions(-)\n>> >>> create mode 100644 common/log.c\n>> >>>\n>> >>> diff --git a/MAINTAINERS b/MAINTAINERS\n>> >>> index 04acf2b89d..eb420afa8d 100644\n>> >>> --- a/MAINTAINERS\n>> >>> +++ b/MAINTAINERS\n>> >>> @@ -290,6 +290,15 @@ S: Maintained\n>> >>> T: git git://git.denx.de/u-boot-i2c.git\n>> >>> F: drivers/i2c/\n>> >>>\n>> >>> +LOGGING\n>> >>> +M: Simon Glass \n>> >>> +S: Maintained\n>> >>> +T: git git://git.denx.de/u-boot.git\n>> >>> +F: common/log.c\n>> >>> +F: cmd/log.c\n>> >>> +F: test/log/log_test.c\n>> >>> +F: test/py/tests/test_log.py\n>> >>\n>> >> test/log/log_test.c and test/py/tests/test_log.py have not been\n>> >> introduced at this point.\n>> >\n>> > OK I can tweak that,\n>> > [..]\n>> >\n>> >>> +/**\n>> >>> + * struct log_driver - a driver which accepts and processes log records\n>> >>> + *\n>> >>> + * @name: Name of driver\n>> >>> + */\n>> >>> +struct log_driver {\n>> >>> + const char *name;\n>> >>> + /**\n>> >>> + * emit() - emit a log record\n>> >>> + *\n>> >>> + * Called by the log system to pass a log record to a particular driver\n>> >>> + * for processing. The filter is checked before calling this function.\n>> >>> + */\n>> >>> + int (*emit)(struct log_device *ldev, struct log_rec *rec);\n>> >>> +};\n>> >>> +\n>> >>\n>> >> So we are creating a new type of non-DM driver which is log-specific?\n>> >> How about we add this emit to the existing uclass driver that can be\n>> >> used as the log driver? (eg: blk devices with file system?)\n>> >\n>> > Yes that's right. I think I can link it to DM once it starts up, but a\n>> > logging of DM start-up is useful.\n>> >\n>> > Re your idea are you saying add an emit() method to UCLASS_BLK?\n>> >\n>>\n>> Yep, something like\n>>\n>> #ifdef CONFIG_LOG\n>> .log_emit = xxx_log_emit,\n>> #endif\n>>\n>> Probably we need a flag to find out which driver provides such log\n>> functionality.\n>\n> So what would the block driver do in that function? I guess I don't\n> understand what it is for. Can you please give me more information?\n>\n\nThe blk driver can save the logs to a predefined block address for\npermanent storage. Or file system driver can send the logs to a file.\n\nRegards,\nBin","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"kJOCI76i\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzF0t01vWz9sPk\n\tfor ;\n\tFri, 22 Sep 2017 23:38:05 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid B03D1C21F90; Fri, 22 Sep 2017 13:38:00 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id C6246C21D56;\n\tFri, 22 Sep 2017 13:37:57 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 67BE3C21D56; Fri, 22 Sep 2017 13:37:56 +0000 (UTC)","from mail-wr0-f194.google.com (mail-wr0-f194.google.com\n\t[209.85.128.194])\n\tby lists.denx.de (Postfix) with ESMTPS id EDB41C21C4A\n\tfor ; Fri, 22 Sep 2017 13:37:55 +0000 (UTC)","by mail-wr0-f194.google.com with SMTP id b9so667158wra.0\n\tfor ; Fri, 22 Sep 2017 06:37:55 -0700 (PDT)","by 10.223.145.3 with HTTP; Fri, 22 Sep 2017 06:37:54 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=FREEMAIL_FROM,\n\tRCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,\n\tT_DKIM_INVALID autolearn=unavailable\n\tautolearn_force=no version=3.4.0","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=7UIpkam0D425/bg+wqf7hwOWGWV9lWguPzVP7EPEo1s=;\n\tb=kJOCI76i2j7yH6nBqCySmSZpn9DJ7xOs3Bi6WnUybGjc0yV/pVh4zD7UfZEwA3gdm8\n\tSqG5NIdU9clhwyxzvz6w97PrcWSF6+lTuxcdZbOciB8CxwnG/LPLqJBMnHhfC+GtcNBO\n\ttYDHif2qMLqNc0y9UofeVU7r8YwqegbYLJ4yKLpri10QZWDbYCXC75AcQPlzipk36GGU\n\tjxLiAHSfXXP+/XhxkpqsB6yoTBzcvn1cHI8Oyh1l7eAD6LpN7h92LItldacf8IL5Fj62\n\twCV+W5D/gFD4TJ1+1cnfzdNC8sdWxo0/BZTGFznNRqQjyhBFS6vedsflcBgsgxDLSCQ1\n\tIZpw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=7UIpkam0D425/bg+wqf7hwOWGWV9lWguPzVP7EPEo1s=;\n\tb=NYbU7tC8XbWA9d7KUHjIM0Dry4EpJVOcA2aqizmKug9b/OjmJKFXjDMO1rZhvHJ12I\n\t/zXx1LzWq84Xf02txs/YoF6UeiyEpgInNse7BfYEpvKMoLmYi6ChOuDyV2l3GGtvIgyO\n\tFtetgbOlhbosdFIRQ08YdvAEUwFWgT1M+jvSikfjuIwxp0HHyk1rhCtWZbWgjd2o8gWe\n\tuqCSInm4QOkwaQPdYQuKUGgX7RCZ0d51iq0YwaKFp+dqFT9NCpClsHt/SKcPuc9DPTTj\n\tJ57i4c8Lttwp68F/YFg9TH5vhGfS6kINgPXUUJFUGqWr+R7NNqBBbkQViVv4/pJpRopu\n\tII6Q==","X-Gm-Message-State":"AHPjjUjnp98MDQoEtDr47gUy3EFsPnoURF7RukKXmm4mKAyN3dvpHHGr\n\tKlbd7kjQdx6B7um6zOTMSSyVff7tk8ZarJGFVdI=","X-Google-Smtp-Source":"AOwi7QB7kCV5t9eiq6jvTb1xB78ZFJ0AX27fOgBgsMS7ujKh9u0J7MTE/29ZnwGK4ZodhIwxQU00ZmqTeh7ENbKeF94=","X-Received":"by 10.223.176.46 with SMTP id f43mr5201850wra.206.1506087475599; \n\tFri, 22 Sep 2017 06:37:55 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t\n\t","From":"Bin Meng ","Date":"Fri, 22 Sep 2017 21:37:54 +0800","Message-ID":"","To":"Simon Glass ","Cc":"Tom Rini , Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Stefan Roese , =?utf-8?b?xYF1a2Fz?=\n\t=?utf-8?q?z_Majewski?= ,\n\tAndy Yan , Maxime Ripard\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1774358,"web_url":"http://patchwork.ozlabs.org/comment/1774358/","msgid":"","list_archive_url":null,"date":"2017-09-25T02:14:43","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":6170,"url":"http://patchwork.ozlabs.org/api/people/6170/","name":"Simon Glass","email":"sjg@chromium.org"},"content":"Hi Bin,\n\nOn 22 September 2017 at 07:37, Bin Meng wrote:\n> Hi Simon,\n>\n> On Thu, Sep 21, 2017 at 12:58 PM, Simon Glass wrote:\n>> Hi Bin,\n>>\n>> On 20 September 2017 at 08:41, Bin Meng wrote:\n>>>\n>>> Hi Simon,\n>>>\n>>> On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass wrote:\n>>> > Hi Bin,\n>>> >\n>>> > On 17 September 2017 at 21:45, Bin Meng wrote:\n>>> >> Hi Simon,\n>>> >>\n>>> >> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass wrote:\n>>> >>> Add the logging header file and implementation with some configuration\n>>> >>> options to control it.\n>>> >>>\n>>> >>> Signed-off-by: Simon Glass \n>>> >>> ---\n>>> >>>\n>>> >>> MAINTAINERS | 9 ++\n>>> >>> common/Kconfig | 56 +++++++++\n>>> >>> common/Makefile | 1 +\n>>> >>> common/log.c | 246 +++++++++++++++++++++++++++++++++++++\n>>> >>> include/asm-generic/global_data.h | 5 +\n>>> >>> include/log.h | 247 ++++++++++++++++++++++++++++++++++++--\n>>> >>> 6 files changed, 555 insertions(+), 9 deletions(-)\n>>> >>> create mode 100644 common/log.c\n>>> >>>\n>>> >>> diff --git a/MAINTAINERS b/MAINTAINERS\n>>> >>> index 04acf2b89d..eb420afa8d 100644\n>>> >>> --- a/MAINTAINERS\n>>> >>> +++ b/MAINTAINERS\n>>> >>> @@ -290,6 +290,15 @@ S: Maintained\n>>> >>> T: git git://git.denx.de/u-boot-i2c.git\n>>> >>> F: drivers/i2c/\n>>> >>>\n>>> >>> +LOGGING\n>>> >>> +M: Simon Glass \n>>> >>> +S: Maintained\n>>> >>> +T: git git://git.denx.de/u-boot.git\n>>> >>> +F: common/log.c\n>>> >>> +F: cmd/log.c\n>>> >>> +F: test/log/log_test.c\n>>> >>> +F: test/py/tests/test_log.py\n>>> >>\n>>> >> test/log/log_test.c and test/py/tests/test_log.py have not been\n>>> >> introduced at this point.\n>>> >\n>>> > OK I can tweak that,\n>>> > [..]\n>>> >\n>>> >>> +/**\n>>> >>> + * struct log_driver - a driver which accepts and processes log records\n>>> >>> + *\n>>> >>> + * @name: Name of driver\n>>> >>> + */\n>>> >>> +struct log_driver {\n>>> >>> + const char *name;\n>>> >>> + /**\n>>> >>> + * emit() - emit a log record\n>>> >>> + *\n>>> >>> + * Called by the log system to pass a log record to a particular driver\n>>> >>> + * for processing. The filter is checked before calling this function.\n>>> >>> + */\n>>> >>> + int (*emit)(struct log_device *ldev, struct log_rec *rec);\n>>> >>> +};\n>>> >>> +\n>>> >>\n>>> >> So we are creating a new type of non-DM driver which is log-specific?\n>>> >> How about we add this emit to the existing uclass driver that can be\n>>> >> used as the log driver? (eg: blk devices with file system?)\n>>> >\n>>> > Yes that's right. I think I can link it to DM once it starts up, but a\n>>> > logging of DM start-up is useful.\n>>> >\n>>> > Re your idea are you saying add an emit() method to UCLASS_BLK?\n>>> >\n>>>\n>>> Yep, something like\n>>>\n>>> #ifdef CONFIG_LOG\n>>> .log_emit = xxx_log_emit,\n>>> #endif\n>>>\n>>> Probably we need a flag to find out which driver provides such log\n>>> functionality.\n>>\n>> So what would the block driver do in that function? I guess I don't\n>> understand what it is for. Can you please give me more information?\n>>\n>\n> The blk driver can save the logs to a predefined block address for\n> permanent storage. Or file system driver can send the logs to a file.\n\nOK I see. IMO this is not really a function of a block device. I don't\nsee what an emit() method in a USB block device (for example) would do\ndifferently from one in a SATA block device.\n\nWe could have a single 'log_blk' driver which:\n\n- allows configuration of what blocks are used for the log\n- writes blocks there (perhaps wrapping if it runs out of space)\n\nFor file systems we could have a 'log_fs' driver which:\n\n- allows configuration of log file path\n- writes blocks into that file (perhaps starting a new file and\nrotating the log if things get over a limit\n\nRegards,\nSimon","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"I7MtSkdh\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"FcQi9Y0n\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y0p4y0wp7z9sRq\n\tfor ;\n\tMon, 25 Sep 2017 12:32:02 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid C68ACC221F3; Mon, 25 Sep 2017 02:18:38 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id E2097C21F49;\n\tMon, 25 Sep 2017 02:18:21 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 0ED9DC21C4F; Mon, 25 Sep 2017 02:15:09 +0000 (UTC)","from mail-qk0-f170.google.com (mail-qk0-f170.google.com\n\t[209.85.220.170])\n\tby lists.denx.de (Postfix) with ESMTPS id 11073C22007\n\tfor ; Mon, 25 Sep 2017 02:15:05 +0000 (UTC)","by mail-qk0-f170.google.com with SMTP id z143so5384270qkb.3\n\tfor ; Sun, 24 Sep 2017 19:15:05 -0700 (PDT)","by 10.200.37.200 with HTTP; Sun, 24 Sep 2017 19:14:43 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID autolearn=unavailable\n\tautolearn_force=no version=3.4.0","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=Gewm4pozTJMpI4HNB6GRjsejAhKg78IOlshp/I3JAjg=;\n\tb=I7MtSkdhmcNZQv5PA9NJaC/LnR0NerjQm1Vavm7Ngs7bn/o+k6Sl9ePOrtIsYOx8cM\n\thHmLOXKRIfuI7tK775OCOXPa03D4H0g90q8mHyMYhA4q1/V08fu36v4y1eJQumwbGHo1\n\tdAVVdDW2x3n+DmEdPGh5c1oExbiq1/VtOj9HJoc4pv9NK29CyFgBirGS7CwPExHZ8B3/\n\t98H31E+TATPy+yT8uq9zbzKrlihM+IVFX3GH2sDeteo7NJ+8bq7itEz7Sm3p3iAv+DLg\n\t1AyZRCHW2wq4G0pi9h1E7TWGW5tggCrJY1OKnh6eONBJFUTGeKOH7rKMJczQhVisNqw+\n\t5KfQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=Gewm4pozTJMpI4HNB6GRjsejAhKg78IOlshp/I3JAjg=;\n\tb=FcQi9Y0nMZi2kwLDDI5XXDf/WSrDciaHHxffZsyvf6X78YVl6qWr9FGe3pW0ohXUJh\n\ta7W33CeI5E/m0BKpN/TeL2SAI5v1GIob8p9phUvHfyQefJXf8lq+mrs6LnPIAhtqg70u\n\tdTubjJ/9wH/Y0/4E78UqpuVGIudy3f1ePamQM="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=Gewm4pozTJMpI4HNB6GRjsejAhKg78IOlshp/I3JAjg=;\n\tb=ERa2kI6dA5wmp/ssumSjk/CfkjS1tpM9yQbUgfmustci8E9CkFe9Px5skgz6H9jzZZ\n\t2W8THkDxgrCyB64a54avkxZ4z9I3lPbdzkqp/sH4c3kbN5H2VDNsueNGmL7x7xRm50vg\n\tJ0SN0ksyKVaDQyJbAEEXYAXpzH1Ade/cYPzEVnIw+D0/sfo2NnZNQvexpXZR4nfRgNyK\n\t/SkRWRy/PEpqZH0+p1h2dkQjxCoqVJkGI7KU6Ni+2157uDHeMKWoRDZgKoO0OD3ltiaT\n\tfk7YP4Z13dq4GeWbdKWoIv45bBHAg8h35TatTA7+82lx89VdgN+t9lec6dopITKQWqdk\n\t5sKw==","X-Gm-Message-State":"AHPjjUhVDQukyJSlnSU+EE3LU+pMHCMyLGtrcJFa/Mui9peV3LhCdrsJ\n\tRPk6tY3HT/y3vRdjqDpBPxc0GFxAKTl7MmnNNXXnzmpU","X-Google-Smtp-Source":"AOwi7QCmLT2RHLvnfU9t43auKvyj7R57kSLCqK59Ekfj5yMiGY+KYZo3/94gd4sgRObK/MxbXEKeWv8h7hA4gFoZE40=","X-Received":"by 10.55.76.205 with SMTP id z196mr8662525qka.252.1506305703724; \n\tSun, 24 Sep 2017 19:15:03 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t\n\t\n\t","From":"Simon Glass ","Date":"Sun, 24 Sep 2017 22:14:43 -0400","X-Google-Sender-Auth":"UoIcqL7-9iQhULuCAtUijEkJazw","Message-ID":"","To":"Bin Meng ","Cc":"Tom Rini , Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Stefan Roese , =?utf-8?b?xYF1a2Fz?=\n\t=?utf-8?q?z_Majewski?= ,\n\tAndy Yan , Maxime Ripard\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1775790,"web_url":"http://patchwork.ozlabs.org/comment/1775790/","msgid":"","list_archive_url":null,"date":"2017-09-26T19:10:22","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":6170,"url":"http://patchwork.ozlabs.org/api/people/6170/","name":"Simon Glass","email":"sjg@chromium.org"},"content":"Hi Masahiro,\n\nOn 20 September 2017 at 11:19, Masahiro Yamada\n wrote:\n> Hi Simon,\n>\n>\n> 2017-09-20 22:49 GMT+09:00 Simon Glass :\n>> Hi Masahiro,\n>>\n>> On 19 September 2017 at 20:51, Masahiro Yamada\n>> wrote:\n>>> Hi Simon,\n>>>\n>>>\n>>> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>>>\n>>>>\n>>>> +menu \"Logging\"\n>>>> +\n>>>> +config LOG\n>>>> + bool \"Enable logging support\"\n>>>> + help\n>>>> + This enables support for logging of status and debug messages. These\n>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>> + discarded if not needed. Logging supports various categories and\n>>>> + levels of severity.\n>>>> +\n>>>> +config SPL_LOG\n>>>> + bool \"Enable logging support in SPL\"\n>>>> + help\n>>>> + This enables support for logging of status and debug messages. These\n>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>> + discarded if not needed. Logging supports various categories and\n>>>> + levels of severity.\n>>>\n>>>\n>>> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>>>\n>>> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n>>> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n>>> when building for TPL.\n>>>\n>>> Since that commit, if you add SPL_ prefixed option,\n>>> you need to add a TPL_ one as well.\n>>>\n>>> I cannot believe why such a commit was accepted.\n>>\n>> Well either way is strange. it is strange that SPL is enabled for TPL\n>> when really they are separate.\n>>\n>> We could revert that commit. But how do you think all of this SPL/TPL\n>> control should actually work? What is intended?\n>>\n>> But I'm OK with not having logging in TPL until we need it.\n>\n> I will explain it in another mail.\n>\n>\n>>>\n>>>\n>>>\n>>>\n>>>> +config LOG_MAX_LEVEL\n>>>> + int \"Maximum log level to record\"\n>>>> + depends on LOG\n>>>> + default 5\n>>>> + help\n>>>> + This selects the maximum log level that will be recorded. Any value\n>>>> + higher than this will be ignored. If possible log statements below\n>>>> + this level will be discarded at build time. Levels:\n>>>> +\n>>>> + 0 - panic\n>>>> + 1 - critical\n>>>> + 2 - error\n>>>> + 3 - warning\n>>>> + 4 - note\n>>>> + 5 - info\n>>>> + 6 - detail\n>>>> + 7 - debug\n>>>\n>>>\n>>> Please do not invent our own for U-Boot.\n>>> Just use Linux log level.\n>>>\n>>> 0 (KERN_EMERG) system is unusable\n>>> 1 (KERN_ALERT) action must be taken immediately\n>>> 2 (KERN_CRIT) critical conditions\n>>> 3 (KERN_ERR) error conditions\n>>> 4 (KERN_WARNING) warning conditions\n>>> 5 (KERN_NOTICE) normal but significant condition\n>>> 6 (KERN_INFO) informational\n>>> 7 (KERN_DEBUG) debug-level messages\n>>\n>> Yes I looked hard at that. The first three seem hard to distinguish in\n>> U-Boot, but we can keep them I suppose. But most of my problem is with\n>> the last two. INFO is what I plan to use for normal printf() output.\n>> DEBUG is obviously for debugging and often involves vaste amounts of\n>> stuff (e.g. logging every access to an MMC peripheral). We need\n>> something in between. It could list the accesses to device at a high\n>> level (e.g API calls) but not every little register access.\n>>\n>> So I don't think the Linux levels are suitable at the high end. We\n>> could go up to 8 I suppose, instead of trying to save one at the\n>> bottom?\n>\n>\n> In fact, Linux has one more for debug.\n> dev_vdbg() is widely used in Linux.\n>\n> If you like, we can add one more level:\n>\n> 8 (KERN_VDEBUG) verbose debug messages\n>\n>\n> Perhaps, logging every access to an MMC peripheral\n> might belong to the vdbg level.\n\nI like the idea of having a log level for message contents (bytes) and\nanother for I/O access. So I will add two more in v2.\n\n>\n>\n>\n> BTW, what do you mean \"INFO is what I plan to use for normal printf() output\"\n>\n> Is that mean printf() is equivalent to pr_info()?\n> If loglevel is 6 or smaller, will all print() be silent?\n> If so, probably we can not use command line interface.\n\nI mean that I want to (later) add a feature that logs normal printf()\noutput. If the console is silent then it would still be logged. Maybe\none day log functions will be used instead of printf(), but for now\nthis provides a useful way to make things wok.\n\n>\n>\n>\n>\n>\n>>>\n>>>\n>>>\n>>>\n>>>> +\n>>>> +/**\n>>>> + * log_dispatch() - Send a log record to all log devices for processing\n>>>> + *\n>>>> + * The log record is sent to each log device in turn, skipping those which have\n>>>> + * filters which block the record\n>>>> + *\n>>>> + * @rec: Log record to dispatch\n>>>> + * @return 0 (meaning success)\n>>>> + */\n>>>> +static int log_dispatch(struct log_rec *rec)\n>>>> +{\n>>>> + struct log_device *ldev;\n>>>> +\n>>>> + list_for_each_entry(ldev, &gd->log_head, sibling_node) {\n>>>> + if (log_passes_filters(ldev, rec))\n>>>> + ldev->drv->emit(ldev, rec);\n>>>> + }\n>>>> +\n>>>> + return 0;\n>>>> +}\n>>>> +\n>>>> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,\n>>>> + int line, const char *func, const char *fmt, ...)\n>>>> +{\n>>>> + char buf[CONFIG_SYS_CBSIZE];\n>>>> + struct log_rec rec;\n>>>> + va_list args;\n>>>> +\n>>>> + rec.cat = cat;\n>>>> + rec.level = level;\n>>>> + rec.file = file;\n>>>> + rec.line = line;\n>>>> + rec.func = func;\n>>>> + va_start(args, fmt);\n>>>> + vsnprintf(buf, sizeof(buf), fmt, args);\n>>>> + va_end(args);\n>>>> + rec.msg = buf;\n>>>> + if (!gd || !(gd->flags & GD_FLG_LOG_READY)) {\n>>>> + if (gd)\n>>>> + gd->log_drop_count++;\n>>>> + return -ENOSYS;\n>>>> + }\n>>>> + log_dispatch(&rec);\n>>>> +\n>>>> + return 0;\n>>>> +}\n>>>> +\n>>>> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],\n>>>> + enum log_level_t max_level, const char *file_list)\n>>>> +{\n>>>> + struct log_filter *filt;\n>>>> + struct log_device *ldev;\n>>>> + int i;\n>>>> +\n>>>> + ldev = log_device_find_by_name(drv_name);\n>>>> + if (!ldev)\n>>>> + return -ENOENT;\n>>>> + filt = (struct log_filter *)calloc(1, sizeof(*filt));\n>>>> + if (!filt)\n>>>> + return -ENOMEM;\n>>>> +\n>>>> + if (cat_list) {\n>>>> + filt->flags |= LOGFF_HAS_CAT;\n>>>> + for (i = 0; ; i++) {\n>>>> + if (i == ARRAY_SIZE(filt->cat_list))\n>>>> + return -ENOSPC;\n>>>> + filt->cat_list[i] = cat_list[i];\n>>>> + if (cat_list[i] == LOGC_END)\n>>>> + break;\n>>>> + }\n>>>> + }\n>>>> + filt->max_level = max_level;\n>>>> + if (file_list) {\n>>>> + filt->file_list = strdup(file_list);\n>>>> + if (!filt->file_list)\n>>>> + goto nomem;\n>>>> + }\n>>>> + filt->filter_num = ldev->next_filter_num++;\n>>>> + INIT_LIST_HEAD(&filt->sibling_node);\n>>>> + list_add_tail(&filt->sibling_node, &ldev->filter_head);\n>>>> +\n>>>> + return filt->filter_num;\n>>>> +\n>>>> +nomem:\n>>>> + free(filt);\n>>>> + return -ENOMEM;\n>>>> +}\n>>>> +\n>>>> +int log_remove_filter(const char *drv_name, int filter_num)\n>>>> +{\n>>>> + struct log_filter *filt;\n>>>> + struct log_device *ldev;\n>>>> +\n>>>> + ldev = log_device_find_by_name(drv_name);\n>>>> + if (!ldev)\n>>>> + return -ENOENT;\n>>>> +\n>>>> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {\n>>>> + if (filt->filter_num == filter_num) {\n>>>> + list_del(&filt->sibling_node);\n>>>> + free(filt);\n>>>> +\n>>>> + return 0;\n>>>> + }\n>>>> + }\n>>>> +\n>>>> + return -ENOENT;\n>>>> +}\n>>>\n>>>\n>>> I am not sure if this luxury filter is needed.\n>>> After all, the purpose is debugging.\n>>> The printf() debugging is useful, but only during testing.\n>>> Once quality code is established, most of debug message should\n>>> generally be removed from upstream code.\n>>\n>> But not logging, and this is the point. It is very useful to have\n>> basic logging information recorded for every boot, and the normal\n>> printf() output is not detailed enough. For example for verified boot\n>> I want to see details about boot failures and what went wrong (key\n>> verification, etc.).\n>\n> This should be warn level, (or notice at least)\n> not info.\n\nSure, but the the fact that it failed is not very useful. We know\nthat. What is useful is the events leading up to that failure and\nthose are probably not written to the console. For example if a key to\nverify was it because the key was invalid, the image was not found,\nthe TPM had wrong data, ...?\n\n>\n>\n>> So I expect that at least INFO (and probably\n>> DETAIL) logging would be useful to record for such a system, even if\n>> it does not appear on the console (in fact the console would normally\n>> be disabled in such a system).\n>\n> OK. Logging is useful, but your example will be used\n> with info level.\n>\n> I still do not understand the necessity of\n> category / file filtering.\n\nCategory is for knowing which subsystem generated the log message. It\nmakes no sense to log MMC messages if you are hunting for an error in\nUSB. It clutters up the log. Also, it allows a post-processing tool to\nfilter log messages in a sensible, human-friendly way, even if both\ncategories are recorded.\n\nFile filtering is useful I think in the interim, while nothing uses\ncategories. We can filter on particular files to pick out the messages\nfrom those files. Even when we do have proper categories everywhere,\nit might still be useful. I am not sure yet.\n\n>\n>\n>>>\n>>>\n>>>\n>>>> +int log_init(void)\n>>>> +{\n>>>> + struct log_driver *drv = ll_entry_start(struct log_driver, log_driver);\n>>>> + const int count = ll_entry_count(struct log_driver, log_driver);\n>>>> + struct log_driver *end = drv + count;\n>>>> +\n>>>> + /*\n>>>> + * We cannot add runtime data to the driver since it is likely stored\n>>>> + * in rodata. Instead, set up a 'device' corresponding to each driver.\n>>>> + * We only support having a single device.\n>>>> + */\n>>>> + INIT_LIST_HEAD((struct list_head *)&gd->log_head);\n>>>> + while (drv < end) {\n>>>> + struct log_device *ldev;\n>>>> +\n>>>> + ldev = calloc(1, sizeof(*ldev));\n>>>> + if (!ldev) {\n>>>> + debug(\"%s: Cannot allocate memory\\n\", __func__);\n>>>> + return -ENOMEM;\n>>>> + }\n>>>> + INIT_LIST_HEAD(&ldev->sibling_node);\n>>>\n>>> This INIT_LIST_HEAD() is unneeded.\n>>\n>> How come? If I don't do that how will the list be set up?\n>\n> Because ldev->sibling_node is not a list_head.\n> It is used as a list node.\n>\n>\n> See include/linux/list.h\n> for the implementation of list_add_tail().\n>\n> The argument \"head\" must be initialized in advance,\n> but \"new\" does not need initialization.\n\nOK I see. Not at all obvious from reading the header file comments.\n\n>\n>\n>\n>>>\n>>>\n>>>\n>>>> + INIT_LIST_HEAD(&ldev->filter_head);\n>>>> + ldev->drv = drv;\n>>>> + list_add_tail(&ldev->sibling_node,\n>>>> + (struct list_head *)&gd->log_head);\n>\n> Here, gd->log_head is a head, so\n> INIT_LIST_HEAD((struct list_head *)&gd->log_head);\n> is correct.\n>\n> INIT_LIST_HEAD(&ldev->sibling_node);\n> is non-sense.\n>\n>\n>\n>\n>\n>>> I know that this (struct list_head *) cast is needed,\n>>> but it is very unfortunate.\n>>> I believe gd_t is design mistake in the first place,\n>>> but U-Boot has gone to unfixable already...\n>>\n>> With driver model I added DM_ROOT_NON_CONST. I suppose we could use\n>> something like that.\n>>\n>> But perhaps we should start dropping the 'volatile' in gd? I'm not\n>> sure why it is needed.\n>\n> Me neither.\n\nOK well something to add to the list.\n\n>\n>\n>>>\n>>>\n>>>\n>>>\n>>>\n>>>> +#if CONFIG_VAL(LOG_MAX_LEVEL)\n>>>> +#define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)\n>>>> +#else\n>>>> +#define _LOG_MAX_LEVEL LOGL_INFO\n>>>> +#endif\n>>>\n>>>\n>>> I cannot understand your intention of #if CONFIG_VAL(LOG_MAX_LEVEL).\n>>> From your Kconfig entry,\n>>>\n>>> 0 - panic\n>>> 1 - critical\n>>> 2 - error\n>>> 3 - warning\n>>> 4 - note\n>>> 5 - info\n>>> 6 - detail\n>>> 7 - debug\n>>>\n>>> I expect CONFIG_VAL(LOG_MAX_LEVEL) is an integer value\n>>> range 0 to 7.\n>>>\n>>> If the log level is 0 (= critial), the conditional is false, so\n>>> _LOG_MAX_LEVEL is set to \"info\" level.\n>>>\n>>> Why is this #if necessary?\n>>\n>> If we don't have CONFIG_LOG enabled then this value will not exist.\n>>\n>\n> This implementation is wrong.\n>\n> You are abusing the fact that\n> U-Boot does not specify -Wundef option.\n>\n>\n> --------------------->8-----------------\n> -Wundef\n> Warn if an undefined identifier is evaluated in an #if directive.\n> Such identifiers are replaced with zero.\n> --------------------->8-----------------\n>\n>\n>\n> Right, U-Boot does not give -Wundef,\n> but it is just because -Wundef causes too many warnings.\n>\n> Ideally, we should fix the code,\n> then enable the option like Linux.\n>\n> We should use #ifdef for testing a option that may or may not be defined.\n> Using #if for options that may not be defined is a bad coding way.\n>\n\nOK I see, will fix.\n\nRegards,\nSimon","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"s82F/Npo\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"VeKWIw2M\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y1rCm0wymz9t5c\n\tfor ;\n\tWed, 27 Sep 2017 05:11:31 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 84DF6C21F3B; Tue, 26 Sep 2017 19:11:01 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id BC0D2C21F5C;\n\tTue, 26 Sep 2017 19:10:57 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 63A49C21F41; Tue, 26 Sep 2017 19:10:49 +0000 (UTC)","from mail-qt0-f181.google.com (mail-qt0-f181.google.com\n\t[209.85.216.181])\n\tby lists.denx.de (Postfix) with ESMTPS id AF400C21E57\n\tfor ; Tue, 26 Sep 2017 19:10:44 +0000 (UTC)","by mail-qt0-f181.google.com with SMTP id f15so11456377qtf.7\n\tfor ; Tue, 26 Sep 2017 12:10:44 -0700 (PDT)","by 10.200.37.200 with HTTP; Tue, 26 Sep 2017 12:10:22 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW,\n\tRCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID autolearn=unavailable\n\tautolearn_force=no version=3.4.0","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=nisZ3MXAXHlzvuI4etNDJ3U7qJFY0bt4JxG0+/uvSHs=;\n\tb=s82F/NpoiDxltug7+5n0tKHETrmycsjbhfCePC3dk2Ck3DSSjb/0X+G7YZbA5n1BWw\n\tTMkGRE39agvX7jHb+swExu5GfNAUFYi8DA+rxnnPngs6l3WygtSeJYur7A87wRPuBst2\n\tzYCm80yynrWW973hHBpfiMVmcZjj0Fu9qt0MuzgdaDYUo1xPIwyHu6bOCH8h0M7fPw+s\n\tRGDRonG+BygaJfpHXW6F+U06MaeLRztdUghyh9Q7WQuxMe7tBxD9DZ3wquA8jlOmJunV\n\tkX/k/yLLyIA3hALQMT4ZDx8Psj6SeTjhcR5WvsqSWwVq+pMQDjSQlP9BxsERUcRb6f2M\n\t2AiQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=nisZ3MXAXHlzvuI4etNDJ3U7qJFY0bt4JxG0+/uvSHs=;\n\tb=VeKWIw2MK6xe1nfY+7e/f0+I6PbSNZ/QHew/XH9we9B0QThOqJamlcUaHinNd/R9P8\n\trD9MlpwDbZyXaAJHamcqxuAPGnRW70NJ0UiKQEbolUtOAh16d9+GOFu7NHAObUp0RuTs\n\t+jZjm0cxp7GxTgFdzFNB0eU/lugl9wm6ZgPyY="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=nisZ3MXAXHlzvuI4etNDJ3U7qJFY0bt4JxG0+/uvSHs=;\n\tb=oK1IaqE6yAsS0Lj40sOnq2H/NRmkMNjo77rSjPzvz7x8d3B9XJ6l1GXgOoAY7ItkzX\n\ttdbD1PP8xwokgMqYJoNfZAoK1KEZsuzP3J7Q7CxKodeLRdwSMX1212FNT/bCK53aCEq9\n\tAOpnMf3DADhIBKNzuoBQhkPleOxPbpgJCeDvxopAJa2cRMcEiiUWW+LxtuRLy0g5Yjeg\n\t45mtpGfARHwOlZVVhcjUFHccLKlky5Y4KlcQ2/JWmsOW273XlpVCYDXuGj8jkcOOoHv+\n\tH/Xi6AHkkYqnlIIzTXTsguxl2WSgEqDCQt50eDj0fkhboRp3ECfHFNVXepY1Pj2ipwdP\n\td2Sg==","X-Gm-Message-State":"AHPjjUj3dUSBCFhTl7cPblk7OddNmAn5erJcsyRk59gy70CopZ2QH5Mc\n\tcUmcXGMVs9Nmu+7sgtdu1UOHUOdZrHXNDirivx+aCQ==","X-Google-Smtp-Source":"AOwi7QCPM1AFtD6cdiEPhR17mCdVZNKICs6lxHVJ6xGuxAuPxm8kD3sMqTd50q6UQYOCaLSi4yqmgUHj1ety1NpZwEc=","X-Received":"by 10.200.23.252 with SMTP id r57mr17978722qtk.76.1506453043202; \n\tTue, 26 Sep 2017 12:10:43 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t","From":"Simon Glass ","Date":"Tue, 26 Sep 2017 13:10:22 -0600","X-Google-Sender-Auth":"ogklOECWR7YXT6ECAMU5WqJCsPY","Message-ID":"","To":"Masahiro Yamada ","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Andy Yan , Maxime\n\tRipard , Stefan Roese , \n\tFranklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1776463,"web_url":"http://patchwork.ozlabs.org/comment/1776463/","msgid":"","list_archive_url":null,"date":"2017-09-27T16:19:26","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":65882,"url":"http://patchwork.ozlabs.org/api/people/65882/","name":"Masahiro Yamada","email":"yamada.masahiro@socionext.com"},"content":"Hi Philipp,\n\n\n2017-09-21 2:51 GMT+09:00 Dr. Philipp Tomsich\n:\n> Masahiro,\n>\n>> On 20 Sep 2017, at 19:34, Masahiro Yamada wrote:\n>>\n>> 2017-09-20 23:37 GMT+09:00 Dr. Philipp Tomsich\n>> :\n>>> Masahiro & Simon,\n>>>\n>>>> On 20 Sep 2017, at 15:49, Simon Glass wrote:\n>>>>\n>>>> Hi Masahiro,\n>>>>\n>>>> On 19 September 2017 at 20:51, Masahiro Yamada\n>>>> wrote:\n>>>>> Hi Simon,\n>>>>>\n>>>>>\n>>>>> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>>>>>\n>>>>>>\n>>>>>> +menu \"Logging\"\n>>>>>> +\n>>>>>> +config LOG\n>>>>>> + bool \"Enable logging support\"\n>>>>>> + help\n>>>>>> + This enables support for logging of status and debug messages. These\n>>>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>>>> + discarded if not needed. Logging supports various categories and\n>>>>>> + levels of severity.\n>>>>>> +\n>>>>>> +config SPL_LOG\n>>>>>> + bool \"Enable logging support in SPL\"\n>>>>>> + help\n>>>>>> + This enables support for logging of status and debug messages. These\n>>>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>>>> + discarded if not needed. Logging supports various categories and\n>>>>>> + levels of severity.\n>>>>>\n>>>>>\n>>>>> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>>>>>\n>>>>> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n>>>>> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n>>>>> when building for TPL.\n>>>>>\n>>>>> Since that commit, if you add SPL_ prefixed option,\n>>>>> you need to add a TPL_ one as well.\n>>>>>\n>>>>> I cannot believe why such a commit was accepted.\n>>>>\n>>>> Well either way is strange. it is strange that SPL is enabled for TPL\n>>>> when really they are separate.\n>>>>\n>>>> We could revert that commit. But how do you think all of this SPL/TPL\n>>>> control should actually work? What is intended?\n>>>>\n>>>> But I'm OK with not having logging in TPL until we need it.\n>>>\n>>> If we don’t differentiate between TPL_ and SPL_, we’ll eventually run into\n>>> size issues for TPL and the $(SPL_TPL_) mechanism will not match the\n>>> CONFIG_IS_ENABLED() mechanism.\n>>>\n>>> I don’t think that anyone will miss this much in TPL and that this can be\n>>> safely left off for TPL (if space was not at a premium in TPL, then why\n>>> have a TPL at all…)\n>>\n>>\n>> The motivation of TPL is\n>> the image size is really limited\n>> for the secondary boot loader in some cases.\n>>\n>>\n>> Instead of:\n>> SPL -> TPL -> U-Boot full\n>\n> Note that this was retro-actively defined to be\n> TPL -> SPL -> U-Boot full\n> by Tom at some point and reiterated in\n> https://lists.denx.de/pipermail/u-boot/2017-July/299266.html\n\n\n\nThanks. I did not know that this flip had already happened.\n\n\nIn fact, I am probably the first man who suggested it.\n\n\nHere is the history.\n\n\n1. Scott Wood introduced TPL to support some freescale chip,\n which had only 4KB memory footprint for the second loader\n https://www.denx.de/wiki/pub/U-Boot/MiniSummitELCE2013/tpl-presentation.pdf\n\n At this point, the boot order was: SPL -> TPL -> U-Boot\n And TPL means \"Tertiary Program Loader\".\n\n\n2. I imported Kbuild and Kconfig to U-Boot.\n\n\n3. During the migration of Kconfig, I noticed\n switching the order of SPL / TPL has advantages.\n\n The \"Tiny Program Loader\" was mentioned first in this mail:\n https://lists.denx.de/pipermail/u-boot/2015-August/222900.html\n\n\n4. When Simon started to move CONFIG_TPL,\n I suggested it once again\n http://patchwork.ozlabs.org/patch/662396/\n\n\n\nSo, I'd like to make \"TPL -> SPL\" legitimate.\n\n\nMore ideally, I hope this is done outside of luxury frameworks.\nNo DM, no OF_CONTROL, then CONFIG_TPL_* are all gone.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=nifty.com header.i=@nifty.com\n\theader.b=\"jrzs5ByJ\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2NMx4j1lz9t5l\n\tfor ;\n\tThu, 28 Sep 2017 02:20:29 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 68F31C21DD7; Wed, 27 Sep 2017 16:20:22 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 08BE6C21D5D;\n\tWed, 27 Sep 2017 16:20:19 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 97DF3C21D5D; Wed, 27 Sep 2017 16:20:17 +0000 (UTC)","from conssluserg-03.nifty.com (conssluserg-03.nifty.com\n\t[210.131.2.82]) by lists.denx.de (Postfix) with ESMTPS id E2F37C21D5B\n\tfor ; Wed, 27 Sep 2017 16:20:13 +0000 (UTC)","from mail-yw0-f180.google.com (mail-yw0-f180.google.com\n\t[209.85.161.180]) (authenticated)\n\tby conssluserg-03.nifty.com with ESMTP id v8RGK7PL027770\n\tfor ; Thu, 28 Sep 2017 01:20:08 +0900","by mail-yw0-f180.google.com with SMTP id u11so9681081ywh.7\n\tfor ; Wed, 27 Sep 2017 09:20:08 -0700 (PDT)","by 10.37.170.198 with HTTP; Wed, 27 Sep 2017 09:19:26 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=T_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Filter":"OpenDKIM Filter v2.10.3 conssluserg-03.nifty.com v8RGK7PL027770","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nifty.com;\n\ts=dec2015msa; t=1506529208;\n\tbh=FkCDtmdtqptXX4Kb1LaA27oOHapQ4FhoK7eaqgLIJFA=;\n\th=In-Reply-To:References:From:Date:Subject:To:Cc:From;\n\tb=jrzs5ByJkOs0b4wTmP8ZASl7apKAsgbl6GJizqa9ziuBIaeHRSAEveNA4d/yTkFTN\n\tF4vgu2UsM13RInDG4YH7tXSPHmdp+y+Wk3CMTbBDahgvSEGLCX+1zAcQ8W3NRdh089\n\tsUz/uSNvF38CliKvA+kE//OoOc7jaKFwAPskXxuzjYtHc43oHf8GNz4tAcJ1tZLUhc\n\tSrtcGKPZIJMK9biZPDFnaasQdXiYQT+B2alLzD1TnZs0UFC6vG8338MMjDoDQgZ2sk\n\te560RfbaF96StgKD4zt8V/uIZm8FGaQJCJx9VYeBgAamplUPEixQfHJMZCA7Z+kilF\n\tFxneXkJUX+oIw==","X-Nifty-SrcIP":"[209.85.161.180]","X-Gm-Message-State":"AHPjjUii88maBE4CqGELCK8TgA8fLiA3h5GsNbCoyVBGjWmywSCMy2rK\n\ttFtmlSkVTJUW94NtzrTYgbyw5OfFDRZsMfq4RIo=","X-Google-Smtp-Source":"AOwi7QDezNP6psT4s/+PfeOn1PhH2binsASV7QgDfNsFRxOu6ebW6MHPcthsiyc6P3bVBODVA+Y/lKeY2jJxzbvWdtE=","X-Received":"by 10.37.163.161 with SMTP id e30mr1291597ybi.107.1506529206926; \n\tWed, 27 Sep 2017 09:20:06 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<26FA5DC9-9257-47F9-B15B-5ABC21EBCFAC@theobroma-systems.com>","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t<4B670262-EFFA-4649-A656-4F189AB1A156@theobroma-systems.com>\n\t\n\t<26FA5DC9-9257-47F9-B15B-5ABC21EBCFAC@theobroma-systems.com>","From":"Masahiro Yamada ","Date":"Thu, 28 Sep 2017 01:19:26 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"\"Dr. Philipp Tomsich\" ","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Andy Shevchenko\n\t, U-Boot Mailing List\n\t, Andy Yan , Maxime\n\tRipard , Stefan Roese , \n\tFranklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1776485,"web_url":"http://patchwork.ozlabs.org/comment/1776485/","msgid":"","list_archive_url":null,"date":"2017-09-27T17:11:57","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":65882,"url":"http://patchwork.ozlabs.org/api/people/65882/","name":"Masahiro Yamada","email":"yamada.masahiro@socionext.com"},"content":"Hi Simon,\n\n\n2017-09-27 4:10 GMT+09:00 Simon Glass :\n> Hi Masahiro,\n>\n> On 20 September 2017 at 11:19, Masahiro Yamada\n> wrote:\n>> Hi Simon,\n>>\n>>\n>> 2017-09-20 22:49 GMT+09:00 Simon Glass :\n>>> Hi Masahiro,\n>>>\n>>> On 19 September 2017 at 20:51, Masahiro Yamada\n>>> wrote:\n>>>> Hi Simon,\n>>>>\n>>>>\n>>>> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>>>>\n>>>>>\n>>>>> +menu \"Logging\"\n>>>>> +\n>>>>> +config LOG\n>>>>> + bool \"Enable logging support\"\n>>>>> + help\n>>>>> + This enables support for logging of status and debug messages. These\n>>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>>> + discarded if not needed. Logging supports various categories and\n>>>>> + levels of severity.\n>>>>> +\n>>>>> +config SPL_LOG\n>>>>> + bool \"Enable logging support in SPL\"\n>>>>> + help\n>>>>> + This enables support for logging of status and debug messages. These\n>>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>>> + discarded if not needed. Logging supports various categories and\n>>>>> + levels of severity.\n>>>>\n>>>>\n>>>> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>>>>\n>>>> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n>>>> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n>>>> when building for TPL.\n>>>>\n>>>> Since that commit, if you add SPL_ prefixed option,\n>>>> you need to add a TPL_ one as well.\n>>>>\n>>>> I cannot believe why such a commit was accepted.\n>>>\n>>> Well either way is strange. it is strange that SPL is enabled for TPL\n>>> when really they are separate.\n>>>\n>>> We could revert that commit. But how do you think all of this SPL/TPL\n>>> control should actually work? What is intended?\n>>>\n>>> But I'm OK with not having logging in TPL until we need it.\n>>\n>> I will explain it in another mail.\n>>\n>>\n>>>>\n>>>>\n>>>>\n>>>>\n>>>>> +config LOG_MAX_LEVEL\n>>>>> + int \"Maximum log level to record\"\n>>>>> + depends on LOG\n>>>>> + default 5\n>>>>> + help\n>>>>> + This selects the maximum log level that will be recorded. Any value\n>>>>> + higher than this will be ignored. If possible log statements below\n>>>>> + this level will be discarded at build time. Levels:\n>>>>> +\n>>>>> + 0 - panic\n>>>>> + 1 - critical\n>>>>> + 2 - error\n>>>>> + 3 - warning\n>>>>> + 4 - note\n>>>>> + 5 - info\n>>>>> + 6 - detail\n>>>>> + 7 - debug\n>>>>\n>>>>\n>>>> Please do not invent our own for U-Boot.\n>>>> Just use Linux log level.\n>>>>\n>>>> 0 (KERN_EMERG) system is unusable\n>>>> 1 (KERN_ALERT) action must be taken immediately\n>>>> 2 (KERN_CRIT) critical conditions\n>>>> 3 (KERN_ERR) error conditions\n>>>> 4 (KERN_WARNING) warning conditions\n>>>> 5 (KERN_NOTICE) normal but significant condition\n>>>> 6 (KERN_INFO) informational\n>>>> 7 (KERN_DEBUG) debug-level messages\n>>>\n>>> Yes I looked hard at that. The first three seem hard to distinguish in\n>>> U-Boot, but we can keep them I suppose. But most of my problem is with\n>>> the last two. INFO is what I plan to use for normal printf() output.\n>>> DEBUG is obviously for debugging and often involves vaste amounts of\n>>> stuff (e.g. logging every access to an MMC peripheral). We need\n>>> something in between. It could list the accesses to device at a high\n>>> level (e.g API calls) but not every little register access.\n>>>\n>>> So I don't think the Linux levels are suitable at the high end. We\n>>> could go up to 8 I suppose, instead of trying to save one at the\n>>> bottom?\n>>\n>>\n>> In fact, Linux has one more for debug.\n>> dev_vdbg() is widely used in Linux.\n>>\n>> If you like, we can add one more level:\n>>\n>> 8 (KERN_VDEBUG) verbose debug messages\n>>\n>>\n>> Perhaps, logging every access to an MMC peripheral\n>> might belong to the vdbg level.\n>\n> I like the idea of having a log level for message contents (bytes) and\n> another for I/O access. So I will add two more in v2.\n>\n>>\n>>\n>>\n>> BTW, what do you mean \"INFO is what I plan to use for normal printf() output\"\n>>\n>> Is that mean printf() is equivalent to pr_info()?\n>> If loglevel is 6 or smaller, will all print() be silent?\n>> If so, probably we can not use command line interface.\n>\n> I mean that I want to (later) add a feature that logs normal printf()\n> output. If the console is silent then it would still be logged. Maybe\n> one day log functions will be used instead of printf(), but for now\n> this provides a useful way to make things wok.\n\n\nIMHO, printf() should be used for command line interface.\n\nAll SoC files, board files should use pr_*(),\nand drivers dev_*().\n\n\n\n>>\n>>\n>>\n>>\n>>\n>>>>\n>>>>\n>>>>\n>>>>\n>>>>> +\n>>>>> +/**\n>>>>> + * log_dispatch() - Send a log record to all log devices for processing\n>>>>> + *\n>>>>> + * The log record is sent to each log device in turn, skipping those which have\n>>>>> + * filters which block the record\n>>>>> + *\n>>>>> + * @rec: Log record to dispatch\n>>>>> + * @return 0 (meaning success)\n>>>>> + */\n>>>>> +static int log_dispatch(struct log_rec *rec)\n>>>>> +{\n>>>>> + struct log_device *ldev;\n>>>>> +\n>>>>> + list_for_each_entry(ldev, &gd->log_head, sibling_node) {\n>>>>> + if (log_passes_filters(ldev, rec))\n>>>>> + ldev->drv->emit(ldev, rec);\n>>>>> + }\n>>>>> +\n>>>>> + return 0;\n>>>>> +}\n>>>>> +\n>>>>> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,\n>>>>> + int line, const char *func, const char *fmt, ...)\n>>>>> +{\n>>>>> + char buf[CONFIG_SYS_CBSIZE];\n>>>>> + struct log_rec rec;\n>>>>> + va_list args;\n>>>>> +\n>>>>> + rec.cat = cat;\n>>>>> + rec.level = level;\n>>>>> + rec.file = file;\n>>>>> + rec.line = line;\n>>>>> + rec.func = func;\n>>>>> + va_start(args, fmt);\n>>>>> + vsnprintf(buf, sizeof(buf), fmt, args);\n>>>>> + va_end(args);\n>>>>> + rec.msg = buf;\n>>>>> + if (!gd || !(gd->flags & GD_FLG_LOG_READY)) {\n>>>>> + if (gd)\n>>>>> + gd->log_drop_count++;\n>>>>> + return -ENOSYS;\n>>>>> + }\n>>>>> + log_dispatch(&rec);\n>>>>> +\n>>>>> + return 0;\n>>>>> +}\n>>>>> +\n>>>>> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],\n>>>>> + enum log_level_t max_level, const char *file_list)\n>>>>> +{\n>>>>> + struct log_filter *filt;\n>>>>> + struct log_device *ldev;\n>>>>> + int i;\n>>>>> +\n>>>>> + ldev = log_device_find_by_name(drv_name);\n>>>>> + if (!ldev)\n>>>>> + return -ENOENT;\n>>>>> + filt = (struct log_filter *)calloc(1, sizeof(*filt));\n>>>>> + if (!filt)\n>>>>> + return -ENOMEM;\n>>>>> +\n>>>>> + if (cat_list) {\n>>>>> + filt->flags |= LOGFF_HAS_CAT;\n>>>>> + for (i = 0; ; i++) {\n>>>>> + if (i == ARRAY_SIZE(filt->cat_list))\n>>>>> + return -ENOSPC;\n>>>>> + filt->cat_list[i] = cat_list[i];\n>>>>> + if (cat_list[i] == LOGC_END)\n>>>>> + break;\n>>>>> + }\n>>>>> + }\n>>>>> + filt->max_level = max_level;\n>>>>> + if (file_list) {\n>>>>> + filt->file_list = strdup(file_list);\n>>>>> + if (!filt->file_list)\n>>>>> + goto nomem;\n>>>>> + }\n>>>>> + filt->filter_num = ldev->next_filter_num++;\n>>>>> + INIT_LIST_HEAD(&filt->sibling_node);\n>>>>> + list_add_tail(&filt->sibling_node, &ldev->filter_head);\n>>>>> +\n>>>>> + return filt->filter_num;\n>>>>> +\n>>>>> +nomem:\n>>>>> + free(filt);\n>>>>> + return -ENOMEM;\n>>>>> +}\n>>>>> +\n>>>>> +int log_remove_filter(const char *drv_name, int filter_num)\n>>>>> +{\n>>>>> + struct log_filter *filt;\n>>>>> + struct log_device *ldev;\n>>>>> +\n>>>>> + ldev = log_device_find_by_name(drv_name);\n>>>>> + if (!ldev)\n>>>>> + return -ENOENT;\n>>>>> +\n>>>>> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {\n>>>>> + if (filt->filter_num == filter_num) {\n>>>>> + list_del(&filt->sibling_node);\n>>>>> + free(filt);\n>>>>> +\n>>>>> + return 0;\n>>>>> + }\n>>>>> + }\n>>>>> +\n>>>>> + return -ENOENT;\n>>>>> +}\n>>>>\n>>>>\n>>>> I am not sure if this luxury filter is needed.\n>>>> After all, the purpose is debugging.\n>>>> The printf() debugging is useful, but only during testing.\n>>>> Once quality code is established, most of debug message should\n>>>> generally be removed from upstream code.\n>>>\n>>> But not logging, and this is the point. It is very useful to have\n>>> basic logging information recorded for every boot, and the normal\n>>> printf() output is not detailed enough. For example for verified boot\n>>> I want to see details about boot failures and what went wrong (key\n>>> verification, etc.).\n>>\n>> This should be warn level, (or notice at least)\n>> not info.\n>\n> Sure, but the the fact that it failed is not very useful. We know\n> that. What is useful is the events leading up to that failure and\n> those are probably not written to the console. For example if a key to\n> verify was it because the key was invalid, the image was not found,\n> the TPM had wrong data, ...?\n>\n>>\n>>\n>>> So I expect that at least INFO (and probably\n>>> DETAIL) logging would be useful to record for such a system, even if\n>>> it does not appear on the console (in fact the console would normally\n>>> be disabled in such a system).\n>>\n>> OK. Logging is useful, but your example will be used\n>> with info level.\n>>\n>> I still do not understand the necessity of\n>> category / file filtering.\n>\n> Category is for knowing which subsystem generated the log message. It\n> makes no sense to log MMC messages if you are hunting for an error in\n> USB. It clutters up the log. Also, it allows a post-processing tool to\n> filter log messages in a sensible, human-friendly way, even if both\n> categories are recorded.\n\n\nI do not think we have much volume for notice or lower loglevel.\n\nThe categorization is useful for debug level\nand you can see the right thing to do\nfor example, in drivers/i2c/Makefile of Linux.\n\nccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG\n\n\n\nThis is already established in Linux\n\n[1] Create an entry for CONFIG_FOO_DEBUG in Kconfig\n[2] Add ccflags-$(CONFIG_FOO_DEBUG) := -DDEBUG\n\nYou can do likewise for DM-core, EFI, or whatever.\n\n\n\n\n> File filtering is useful I think in the interim, while nothing uses\n> categories. We can filter on particular files to pick out the messages\n> from those files. Even when we do have proper categories everywhere,\n> it might still be useful. I am not sure yet.\n\n\nFile filter is pointless because we are generally\nsupposed to add #define DEBUG to each file\nto make pr_dbg() effective.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=nifty.com header.i=@nifty.com\n\theader.b=\"jtV5F+E7\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2PXn1zf4z9tXb\n\tfor ;\n\tThu, 28 Sep 2017 03:13:11 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid C5A80C21D8B; Wed, 27 Sep 2017 17:13:05 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 4340DC21C58;\n\tWed, 27 Sep 2017 17:13:01 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid EB0A6C21C58; Wed, 27 Sep 2017 17:12:59 +0000 (UTC)","from conssluserg-06.nifty.com (conssluserg-06.nifty.com\n\t[210.131.2.91]) by lists.denx.de (Postfix) with ESMTPS id 740F8C21C51\n\tfor ; Wed, 27 Sep 2017 17:12:58 +0000 (UTC)","from mail-yw0-f181.google.com (mail-yw0-f181.google.com\n\t[209.85.161.181]) (authenticated)\n\tby conssluserg-06.nifty.com with ESMTP id v8RHCdxM022572\n\tfor ; Thu, 28 Sep 2017 02:12:39 +0900","by mail-yw0-f181.google.com with SMTP id q80so9772343ywg.2\n\tfor ; Wed, 27 Sep 2017 10:12:39 -0700 (PDT)","by 10.37.170.198 with HTTP; Wed, 27 Sep 2017 10:11:57 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=T_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Filter":"OpenDKIM Filter v2.10.3 conssluserg-06.nifty.com v8RHCdxM022572","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nifty.com;\n\ts=dec2015msa; t=1506532360;\n\tbh=iIkJZ23b/GRbfEBzuQls7Wp16svDj4YoF1HtviZg5eE=;\n\th=In-Reply-To:References:From:Date:Subject:To:Cc:From;\n\tb=jtV5F+E7OTzAwn9lZX+BnNl3gobeRe4za2Oxe8V3RrNo6m5HmRq7YN/AukCDdRx66\n\t9gzUmSbw/jgASkBfjFaAzXeA4lp7jePfoLlCblAA0KpzsR3X8HDthj71f0ncCQdcKP\n\tT1W736QB1nZ+ZddXpiV6jQMJqqxQUWxLJtZFBWcKiGjQp+Cyd4DKkvHR2UHnD01VMA\n\t5cfaXBeeQHjZumiP+R24Lp3LsGOt3++Gxyve4F03xbWgtQmZ/SG/LW4utVVvtc7GiF\n\t2eDoAkFocga5z8sa79P+yPmsD2DfHT0C5dtHRf5GvTpl0+9PZePBD14kBhc9/2dxjq\n\tnHa8RaB5NaWMQ==","X-Nifty-SrcIP":"[209.85.161.181]","X-Gm-Message-State":"AHPjjUj33x9hC1tzpWWCK+Pte4BKQr9DpNDNIjxX8LYO0Gz8070Oi3s1\n\tej15Ma4oHWyQGFKFkx8mP81S0if6OnCgRj0FAXo=","X-Google-Smtp-Source":"AOwi7QA2ceYldgzuaOx80HI+N+6lT436Oc2witzj0oF5zNQTnzBigDjP6hl2W/Ir6PtKgZuUwrXZi3MWU/AgSqKdpS8=","X-Received":"by 10.129.155.209 with SMTP id\n\ts200mr1386294ywg.233.1506532358225; \n\tWed, 27 Sep 2017 10:12:38 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t\n\t","From":"Masahiro Yamada ","Date":"Thu, 28 Sep 2017 02:11:57 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Simon Glass ","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Stefan Roese ,\n\tU-Boot Mailing List , Andy Yan\n\t, Maxime Ripard\n\t, Andy Shevchenko\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1777005,"web_url":"http://patchwork.ozlabs.org/comment/1777005/","msgid":"","list_archive_url":null,"date":"2017-09-28T12:39:59","subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","submitter":{"id":6170,"url":"http://patchwork.ozlabs.org/api/people/6170/","name":"Simon Glass","email":"sjg@chromium.org"},"content":"Hi Masahiro,\n\nOn 27 September 2017 at 11:11, Masahiro Yamada\n wrote:\n> Hi Simon,\n>\n>\n> 2017-09-27 4:10 GMT+09:00 Simon Glass :\n>> Hi Masahiro,\n>>\n>> On 20 September 2017 at 11:19, Masahiro Yamada\n>> wrote:\n>>> Hi Simon,\n>>>\n>>>\n>>> 2017-09-20 22:49 GMT+09:00 Simon Glass :\n>>>> Hi Masahiro,\n>>>>\n>>>> On 19 September 2017 at 20:51, Masahiro Yamada\n>>>> wrote:\n>>>>> Hi Simon,\n>>>>>\n>>>>>\n>>>>> 2017-09-17 6:23 GMT+09:00 Simon Glass :\n>>>>>\n>>>>>>\n>>>>>> +menu \"Logging\"\n>>>>>> +\n>>>>>> +config LOG\n>>>>>> + bool \"Enable logging support\"\n>>>>>> + help\n>>>>>> + This enables support for logging of status and debug messages. These\n>>>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>>>> + discarded if not needed. Logging supports various categories and\n>>>>>> + levels of severity.\n>>>>>> +\n>>>>>> +config SPL_LOG\n>>>>>> + bool \"Enable logging support in SPL\"\n>>>>>> + help\n>>>>>> + This enables support for logging of status and debug messages. These\n>>>>>> + can be displayed on the console, recorded in a memory buffer, or\n>>>>>> + discarded if not needed. Logging supports various categories and\n>>>>>> + levels of severity.\n>>>>>\n>>>>>\n>>>>> Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.\n>>>>>\n>>>>> Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,\n>>>>> CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG\n>>>>> when building for TPL.\n>>>>>\n>>>>> Since that commit, if you add SPL_ prefixed option,\n>>>>> you need to add a TPL_ one as well.\n>>>>>\n>>>>> I cannot believe why such a commit was accepted.\n>>>>\n>>>> Well either way is strange. it is strange that SPL is enabled for TPL\n>>>> when really they are separate.\n>>>>\n>>>> We could revert that commit. But how do you think all of this SPL/TPL\n>>>> control should actually work? What is intended?\n>>>>\n>>>> But I'm OK with not having logging in TPL until we need it.\n>>>\n>>> I will explain it in another mail.\n>>>\n>>>\n>>>>>\n>>>>>\n>>>>>\n>>>>>\n>>>>>> +config LOG_MAX_LEVEL\n>>>>>> + int \"Maximum log level to record\"\n>>>>>> + depends on LOG\n>>>>>> + default 5\n>>>>>> + help\n>>>>>> + This selects the maximum log level that will be recorded. Any value\n>>>>>> + higher than this will be ignored. If possible log statements below\n>>>>>> + this level will be discarded at build time. Levels:\n>>>>>> +\n>>>>>> + 0 - panic\n>>>>>> + 1 - critical\n>>>>>> + 2 - error\n>>>>>> + 3 - warning\n>>>>>> + 4 - note\n>>>>>> + 5 - info\n>>>>>> + 6 - detail\n>>>>>> + 7 - debug\n>>>>>\n>>>>>\n>>>>> Please do not invent our own for U-Boot.\n>>>>> Just use Linux log level.\n>>>>>\n>>>>> 0 (KERN_EMERG) system is unusable\n>>>>> 1 (KERN_ALERT) action must be taken immediately\n>>>>> 2 (KERN_CRIT) critical conditions\n>>>>> 3 (KERN_ERR) error conditions\n>>>>> 4 (KERN_WARNING) warning conditions\n>>>>> 5 (KERN_NOTICE) normal but significant condition\n>>>>> 6 (KERN_INFO) informational\n>>>>> 7 (KERN_DEBUG) debug-level messages\n>>>>\n>>>> Yes I looked hard at that. The first three seem hard to distinguish in\n>>>> U-Boot, but we can keep them I suppose. But most of my problem is with\n>>>> the last two. INFO is what I plan to use for normal printf() output.\n>>>> DEBUG is obviously for debugging and often involves vaste amounts of\n>>>> stuff (e.g. logging every access to an MMC peripheral). We need\n>>>> something in between. It could list the accesses to device at a high\n>>>> level (e.g API calls) but not every little register access.\n>>>>\n>>>> So I don't think the Linux levels are suitable at the high end. We\n>>>> could go up to 8 I suppose, instead of trying to save one at the\n>>>> bottom?\n>>>\n>>>\n>>> In fact, Linux has one more for debug.\n>>> dev_vdbg() is widely used in Linux.\n>>>\n>>> If you like, we can add one more level:\n>>>\n>>> 8 (KERN_VDEBUG) verbose debug messages\n>>>\n>>>\n>>> Perhaps, logging every access to an MMC peripheral\n>>> might belong to the vdbg level.\n>>\n>> I like the idea of having a log level for message contents (bytes) and\n>> another for I/O access. So I will add two more in v2.\n>>\n>>>\n>>>\n>>>\n>>> BTW, what do you mean \"INFO is what I plan to use for normal printf() output\"\n>>>\n>>> Is that mean printf() is equivalent to pr_info()?\n>>> If loglevel is 6 or smaller, will all print() be silent?\n>>> If so, probably we can not use command line interface.\n>>\n>> I mean that I want to (later) add a feature that logs normal printf()\n>> output. If the console is silent then it would still be logged. Maybe\n>> one day log functions will be used instead of printf(), but for now\n>> this provides a useful way to make things wok.\n>\n>\n> IMHO, printf() should be used for command line interface.\n\nI suppose that makes sense, but in the interim we have a lot of\nprintf() stuff printing errors / messages.\n\n>\n> All SoC files, board files should use pr_*(),\n> and drivers dev_*().\n\nOK, for dev_*() we can generate the category automatically, but for\npr_*() we cannot. So those would just get the default category for the\nfile.\n\n> I do not think we have much volume for notice or lower loglevel.\n\nWe have loads and loads of debug() things. If you mean that they will\nnormally be compiled out, yes. But I think we will see a trend to\nadding more debugging into the code for when things go wrong. I'd like\nto be able to do a 'debug' build and get all of that info recorded,\neven if it is not displayed on the console.\n\n>\n> The categorization is useful for debug level\n> and you can see the right thing to do\n> for example, in drivers/i2c/Makefile of Linux.\n>\n> ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG\n\nThat is a build-time thing, though.\n\n>\n>\n>\n> This is already established in Linux\n>\n> [1] Create an entry for CONFIG_FOO_DEBUG in Kconfig\n> [2] Add ccflags-$(CONFIG_FOO_DEBUG) := -DDEBUG\n>\n> You can do likewise for DM-core, EFI, or whatever.\n\nThat is not the (whole) use case for this log feature. Here you are\nsuggesting that we add an option to debug each uclass at run-time. I\nthink that might be quite useful to allow build-time control of what\ncategories are logged, but I think it is not a core feature needed\nright now.\n\n>\n>\n>\n>\n>> File filtering is useful I think in the interim, while nothing uses\n>> categories. We can filter on particular files to pick out the messages\n>> from those files. Even when we do have proper categories everywhere,\n>> it might still be useful. I am not sure yet.\n>\n>\n> File filter is pointless because we are generally\n> supposed to add #define DEBUG to each file\n> to make pr_dbg() effective.\n\nWith logging if you set the max level to DEBUG you don't need to do\nthat. You can filter at run-time.\n\nRegards,\nSimon","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"PMQqxxDB\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"J2Y9YpCX\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2vRh4389z9tXj\n\tfor ;\n\tThu, 28 Sep 2017 22:40:32 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 4CB58C21DCA; Thu, 28 Sep 2017 12:40:29 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 302CEC21D84;\n\tThu, 28 Sep 2017 12:40:26 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid A349AC21D84; Thu, 28 Sep 2017 12:40:24 +0000 (UTC)","from mail-qt0-f171.google.com (mail-qt0-f171.google.com\n\t[209.85.216.171])\n\tby lists.denx.de (Postfix) with ESMTPS id 4E6E1C21D09\n\tfor ; Thu, 28 Sep 2017 12:40:22 +0000 (UTC)","by mail-qt0-f171.google.com with SMTP id l25so1367890qtf.13\n\tfor ; Thu, 28 Sep 2017 05:40:22 -0700 (PDT)","by 10.200.37.200 with HTTP; Thu, 28 Sep 2017 05:39:59 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID autolearn=unavailable\n\tautolearn_force=no version=3.4.0","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=+ZLus4hN9CKrD4nwVckiPggQTR1h/r0acHDQ/AKUxHY=;\n\tb=PMQqxxDB9UnuZdQPjx9y7+enVO2tu7tEqlEwtB8wwSgyr66K7LQQbmQUFzTT11v8rQ\n\tZZLQwF0zqDqyF6xTkj1TInp016LtWBfGwuU1LErAsO0tczZAeQaFXRUvW8dO9sjoqCao\n\teQdCZ/wD1RvRsSRvMCLFWiBTulVxzZLy72oukRdHKUxTxr7BO5hOqBAocEGPv6mYkZ5p\n\tUY3VemjP06B7dMTX7gm5zLyhd4/f03kkDOmIDZJQi0y+cIPmnNq2de+3VGb2v4lkv9Kh\n\teiYPikJoS4Qn7KLXV7r6UnHOUNRAXgS0/z8DV/xihQjimJiRbKATTEeAEhAw96wvWvMa\n\tiAcg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=+ZLus4hN9CKrD4nwVckiPggQTR1h/r0acHDQ/AKUxHY=;\n\tb=J2Y9YpCXgf8G995fN3j9oWZfrMl8vBtkkR7Cbo1Jxqp4SGeNAMpPHFxe8VkU/BRl8+\n\t47a+pEvkbYKB4rXhoaohSkqG1q4eRjeAQM8FSKSwBgJ1Tv6oA3Tj37Mn+sUYY5zC80tr\n\tXFK+Ot/83gtheu+GQl6/wUFRamQE9JhyXH8I0="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=+ZLus4hN9CKrD4nwVckiPggQTR1h/r0acHDQ/AKUxHY=;\n\tb=ESd2n+pf9+s2EdaAy6PNRKkI03uOlnpqK+UPkw5RiJ9BXunYfSCS5vj1gKuQylR8ng\n\t3V/qJ4A2+msnyi4WP5VfHsybXssHsrRyBc8U1uvImp+Dcd5fDDtp9NdhNulorz90Mmwe\n\tQs7i+lWGG+8KG55GUDXHija2SqRC/n+sQZTnTZUkO8oQI2yN7xTO+qBskB822U1KsW0c\n\twPSF/oKSifN3CJAFA60TEj0MIRqY9QaF6WhUykwA8h9+Hd1lMKOgyan1LFVs1rpAXI8l\n\tcZqRalaAdpuAqnkIeEOZQ/umkOrSC04gYllYuvBfZW84EziGrdi8GRt+BagiE0bbgjrC\n\tC5zQ==","X-Gm-Message-State":"AMCzsaVyBZ1vLm1uQkqSwEdTG6CNw3v1jRCDLiT+Z4xp1W/F7QMGB8qh\n\tmwqRYzQ5wqeD03xMguFL8PMwxqnT6S9CyrQX9tFDbA==","X-Google-Smtp-Source":"AOwi7QAlwtXZJ8rGtah0W2iPGIiWaFrn6e+NAfisSV5K2Doqmy8xR+vNXTp7+M8Oj7c4+gHN8VzA1EOofkUrmGtEnus=","X-Received":"by 10.200.63.154 with SMTP id d26mr668504qtk.212.1506602420587; \n\tThu, 28 Sep 2017 05:40:20 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170916212331.170463-1-sjg@chromium.org>\n\t<20170916212331.170463-7-sjg@chromium.org>\n\t\n\t\n\t\n\t\n\t","From":"Simon Glass ","Date":"Thu, 28 Sep 2017 06:39:59 -0600","X-Google-Sender-Auth":"PyNcp1FvAOOZsrFBCV0OxfTw7-c","Message-ID":"","To":"Masahiro Yamada ","Cc":"Tom Rini , =?utf-8?q?=C5=81ukasz_Majewski?=\n\t, Stefan Roese ,\n\tU-Boot Mailing List , Andy Yan\n\t, Maxime Ripard\n\t, Andy Shevchenko\n\t, Franklin ","Subject":"Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}}]