[PATCH 0/4] dm: Print device name in dev_xxx() like Linux
This series adds some additional information to dev_dbg. With this series, and the following options applied in addition to sandbox_defconfig
CONFIG_LOGLEVEL=8 CONFIG_LOG=n CONFIG_CMD_LOG=n CONFIG_CMD_TPM=n
the output of U-Boot is
U-Boot 2020.10-rc4-00006-g19b641dddd-dirty (Sep 12 2020 - 18:15:00 -0400)
Model: sandbox DRAM: 128 MiB pinctrl-gpio sandbox_pinctrl_gpio: set_state_simple op missing leds gpio_led: set_state_simple op missing default_on gpio_led: set_state_simple op missing base-gpios sandbox_gpio: set_state_simple op missing default_off gpio_led: set_state_simple op missing wdt@0 wdt_sandbox: set_state_simple op missing WDT: Started with servicing (60s timeout) MMC: mmc0 mmc_sandbox: set_state_simple op missing mmc1 mmc_sandbox: set_state_simple op missing mmc2 mmc_sandbox: set_state_simple op missing mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD) lcd sandbox_lcd_sdl: set_state_simple op missing In: serial Out: vidconsole Err: vidconsole Model: sandbox hog_input_active_low gpio_hog: set_state_simple op missing hog_input_active_high gpio_hog: set_state_simple op missing hog_output_low gpio_hog: set_state_simple op missing hog_output_high gpio_hog: set_state_simple op missing cros-ec google_cros_ec_sandbox: set_state_simple op missing SCSI: pci_ep pci_ep_sandbox: set_state_simple op missing Net: eth@10002000 eth_sandbox: set_state_simple op missing eth0: eth@10002000eth@10003000 eth_sandbox: set_state_simple op missing , eth5: eth@10003000sbe5 eth_sandbox: set_state_simple op missing , eth3: sbe5eth@10004000 eth_sandbox: set_state_simple op missing , eth6: eth@10004000 bootcount@0 bootcount-rtc: set_state_simple op missing i2c@0 i2c_sandbox: set_state_simple op missing rtc@61 rtc-sandbox: set_state_simple op missing emul i2c_emul_parent_drv: set_state_simple op missing emull sandbox_i2c_rtc_emul: set_state_simple op missing Hit any key to stop autoboot:
When using CONFIG_LOG the output of U-Boot is too verbose to fit in this email. Note that there is a soft dependency on [1] if you would like to test this patch with CONFIG_LOG and a higher LOG_LEVEL than LOGL_INFO.
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=201343
Sean Anderson (4): remoteproc: Remove unused function rproc_elf_sanity_check net: mdio: Supply appropriate devices for dev_dgb calls dm: Use symbolic constants for log levels in dev_xxx() dm: Print device name in dev_xxx() like Linux
drivers/remoteproc/rproc-elf-loader.c | 16 ------ include/dm/device_compat.h | 75 +++++++++++++-------------- include/remoteproc.h | 13 ----- net/mdio-uclass.c | 4 +- 4 files changed, 39 insertions(+), 69 deletions(-)
This function is never used anywhere, and it also tries to log with a nonexistant device.
Signed-off-by: Sean Anderson seanga2@gmail.com ---
drivers/remoteproc/rproc-elf-loader.c | 16 ---------------- include/remoteproc.h | 13 ------------- 2 files changed, 29 deletions(-)
diff --git a/drivers/remoteproc/rproc-elf-loader.c b/drivers/remoteproc/rproc-elf-loader.c index c464ecebb7..b185a6cafb 100644 --- a/drivers/remoteproc/rproc-elf-loader.c +++ b/drivers/remoteproc/rproc-elf-loader.c @@ -158,22 +158,6 @@ int rproc_elf64_sanity_check(ulong addr, ulong size) return 0; }
-/* Basic function to verify ELF image format */ -int rproc_elf_sanity_check(ulong addr, ulong size) -{ - Elf32_Ehdr *ehdr = (Elf32_Ehdr *)addr; - - if (!addr) { - dev_err(dev, "Invalid firmware address\n"); - return -EFAULT; - } - - if (ehdr->e_ident[EI_CLASS] == ELFCLASS64) - return rproc_elf64_sanity_check(addr, size); - else - return rproc_elf32_sanity_check(addr, size); -} - int rproc_elf32_load_image(struct udevice *dev, unsigned long addr, ulong size) { Elf32_Ehdr *ehdr; /* Elf header structure pointer */ diff --git a/include/remoteproc.h b/include/remoteproc.h index a903acb9b2..74d01723f6 100644 --- a/include/remoteproc.h +++ b/include/remoteproc.h @@ -226,19 +226,6 @@ int rproc_elf32_sanity_check(ulong addr, ulong size); */ int rproc_elf64_sanity_check(ulong addr, ulong size);
-/** - * rproc_elf_sanity_check() - Verify if an image is a valid ELF one - * - * Check if a valid ELF image exists at the given memory location. Auto - * detects ELF32/ELF64 and verifies basic ELF64/ELF32 format requirements - * like magic number and sections size. - * - * @addr: address of the image to verify - * @size: size of the image - * @return 0 if the image looks good, else appropriate error value. - */ -int rproc_elf_sanity_check(ulong addr, ulong size); - /** * rproc_elf32_load_image() - load an ELF32 image * @dev: device loading the ELF32 image
On Sat, 12 Sep 2020 at 18:28, Sean Anderson seanga2@gmail.com wrote:
This function is never used anywhere, and it also tries to log with a nonexistant device.
Signed-off-by: Sean Anderson seanga2@gmail.com
drivers/remoteproc/rproc-elf-loader.c | 16 ---------------- include/remoteproc.h | 13 ------------- 2 files changed, 29 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
The name of the device we are working on is `ethdev` and not just `dev`.
Signed-off-by: Sean Anderson seanga2@gmail.com ---
net/mdio-uclass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c index 66ee2e1976..b5e8e46512 100644 --- a/net/mdio-uclass.c +++ b/net/mdio-uclass.c @@ -145,7 +145,7 @@ static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev, break;
if (!ofnode_valid(phandle.node)) { - dev_dbg(dev, "can't find PHY node\n"); + dev_dbg(ethdev, "can't find PHY node\n"); return NULL; }
@@ -161,7 +161,7 @@ static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev, if (uclass_get_device_by_ofnode(UCLASS_MDIO, ofnode_get_parent(phandle.node), &mdiodev)) { - dev_dbg(dev, "can't find MDIO bus for node %s\n", + dev_dbg(ethdev, "can't find MDIO bus for node %s\n", ofnode_get_name(ofnode_get_parent(phandle.node))); return NULL; }
On Sat, 12 Sep 2020 at 18:28, Sean Anderson seanga2@gmail.com wrote:
The name of the device we are working on is `ethdev` and not just `dev`.
Signed-off-by: Sean Anderson seanga2@gmail.com
net/mdio-uclass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
BTW I really like using 'dev' for the struct udevice if there is only one, and other names for other things (e.g. edev for Ethernet, udev for USB).
On 9/16/20 9:09 PM, Simon Glass wrote:
On Sat, 12 Sep 2020 at 18:28, Sean Anderson seanga2@gmail.com wrote:
The name of the device we are working on is `ethdev` and not just `dev`.
Signed-off-by: Sean Anderson seanga2@gmail.com
net/mdio-uclass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
BTW I really like using 'dev' for the struct udevice if there is only one, and other names for other things (e.g. edev for Ethernet, udev for USB).
Yeah, I think it is only really necessary when disambiguating between several different devices. However, I am trying to make these patches as minimal as possible, so I would like to keep the variable names the same where possible.
--Sean
Hi Sean,
On Wed, 16 Sep 2020 at 19:40, Sean Anderson seanga2@gmail.com wrote:
On 9/16/20 9:09 PM, Simon Glass wrote:
On Sat, 12 Sep 2020 at 18:28, Sean Anderson seanga2@gmail.com wrote:
The name of the device we are working on is `ethdev` and not just `dev`.
Signed-off-by: Sean Anderson seanga2@gmail.com
net/mdio-uclass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
BTW I really like using 'dev' for the struct udevice if there is only one, and other names for other things (e.g. edev for Ethernet, udev for USB).
Yeah, I think it is only really necessary when disambiguating between several different devices. However, I am trying to make these patches as minimal as possible, so I would like to keep the variable names the same where possible.
Yes definitely better the way you have the patches now.
This substitutes literal log levels with their symbolic constants.
Signed-off-by: Sean Anderson seanga2@gmail.com ---
include/dm/device_compat.h | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/include/dm/device_compat.h b/include/dm/device_compat.h index 3d8cd09f4c..7c14aa464d 100644 --- a/include/dm/device_compat.h +++ b/include/dm/device_compat.h @@ -10,6 +10,7 @@ #ifndef _DM_DEVICE_COMPAT_H #define _DM_DEVICE_COMPAT_H
+#include <log.h> #include <linux/compat.h>
/* @@ -48,28 +49,28 @@ })
#define dev_emerg(dev, fmt, ...) \ - __dev_printk(0, dev, fmt, ##__VA_ARGS__) + __dev_printk(LOGL_EMERG, dev, fmt, ##__VA_ARGS__) #define dev_alert(dev, fmt, ...) \ - __dev_printk(1, dev, fmt, ##__VA_ARGS__) + __dev_printk(LOGL_ALERT, dev, fmt, ##__VA_ARGS__) #define dev_crit(dev, fmt, ...) \ - __dev_printk(2, dev, fmt, ##__VA_ARGS__) + __dev_printk(LOGL_CRIT, dev, fmt, ##__VA_ARGS__) #define dev_err(dev, fmt, ...) \ - __dev_printk(3, dev, fmt, ##__VA_ARGS__) + __dev_printk(LOGL_ERR, dev, fmt, ##__VA_ARGS__) #define dev_warn(dev, fmt, ...) \ - __dev_printk(4, dev, fmt, ##__VA_ARGS__) + __dev_printk(LOGL_WARNING, dev, fmt, ##__VA_ARGS__) #define dev_notice(dev, fmt, ...) \ - __dev_printk(5, dev, fmt, ##__VA_ARGS__) + __dev_printk(LOGL_NOTICE, dev, fmt, ##__VA_ARGS__) #define dev_info(dev, fmt, ...) \ - __dev_printk(6, dev, fmt, ##__VA_ARGS__) + __dev_printk(LOGL_INFO, dev, fmt, ##__VA_ARGS__)
#ifdef DEBUG #define dev_dbg(dev, fmt, ...) \ - __dev_printk(7, dev, fmt, ##__VA_ARGS__) + __dev_printk(LOGL_DEBUG, dev, fmt, ##__VA_ARGS__) #else #define dev_dbg(dev, fmt, ...) \ ({ \ if (0) \ - __dev_printk(7, dev, fmt, ##__VA_ARGS__); \ + __dev_printk(LOGL_DEBUG, dev, fmt, ##__VA_ARGS__); \ }) #endif
@@ -79,7 +80,7 @@ #define dev_vdbg(dev, fmt, ...) \ ({ \ if (0) \ - __dev_printk(7, dev, fmt, ##__VA_ARGS__); \ + __dev_printk(LOGL_DEBUG, dev, fmt, ##__VA_ARGS__); \ }) #endif
On Sat, 12 Sep 2020 at 18:28, Sean Anderson seanga2@gmail.com wrote:
This substitutes literal log levels with their symbolic constants.
Signed-off-by: Sean Anderson seanga2@gmail.com
include/dm/device_compat.h | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
This adorns messages generated by dev_xxx() with the device and driver names. It also redirects dev_xxx() to log() when it is available. The names of these functions very roughly take inspiration from Linux, but there is no deeper correlation.
Signed-off-by: Sean Anderson seanga2@gmail.com ---
include/dm/device_compat.h | 58 ++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/include/dm/device_compat.h b/include/dm/device_compat.h index 7c14aa464d..5d18eb68fa 100644 --- a/include/dm/device_compat.h +++ b/include/dm/device_compat.h @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0+ */ /* + * Copyright (C) 2020 Sean Anderson seanga2@gmail.com * Copyright (c) 2013 Google, Inc * * (C) Copyright 2012 @@ -33,19 +34,33 @@ #undef dev_warn #endif
-/* - * REVISIT: - * print device name like Linux - */ -#define dev_printk(dev, fmt, ...) \ -({ \ - printk(fmt, ##__VA_ARGS__); \ +#ifdef VERBOSE_DEBUG +#define _VERBOSE_DEBUG 1 +#else +#define _VERBOSE_DEBUG 0 +#endif + +#define dev_printk_emit(cat, level, fmt, ...) \ +({ \ + if ((_DEBUG && level == LOGL_DEBUG) || \ + (_VERBOSE_DEBUG && level == LOGL_DEBUG_CONTENT)) \ + printf(fmt, ##__VA_ARGS__); \ + else if (CONFIG_IS_ENABLED(LOG)) \ + log(cat, level, fmt, ##__VA_ARGS__); \ + else if (level < CONFIG_VAL(LOGLEVEL)) \ + printf(fmt, ##__VA_ARGS__); \ })
-#define __dev_printk(level, dev, fmt, ...) \ -({ \ - if (level < CONFIG_VAL(LOGLEVEL)) \ - dev_printk(dev, fmt, ##__VA_ARGS__); \ +#define __dev_printk(level, dev, fmt, ...) \ +({ \ + struct udevice *__dev = dev; \ + if (__dev) \ + dev_printk_emit(__dev->driver->id, level, "%s %s: " fmt, \ + __dev->name, __dev->driver->name, \ + ##__VA_ARGS__); \ + else \ + dev_printk_emit(LOG_CATEGORY, level, \ + "(NULL udevice *): " fmt, ##__VA_ARGS__); \ })
#define dev_emerg(dev, fmt, ...) \ @@ -62,26 +77,9 @@ __dev_printk(LOGL_NOTICE, dev, fmt, ##__VA_ARGS__) #define dev_info(dev, fmt, ...) \ __dev_printk(LOGL_INFO, dev, fmt, ##__VA_ARGS__) - -#ifdef DEBUG #define dev_dbg(dev, fmt, ...) \ __dev_printk(LOGL_DEBUG, dev, fmt, ##__VA_ARGS__) -#else -#define dev_dbg(dev, fmt, ...) \ -({ \ - if (0) \ - __dev_printk(LOGL_DEBUG, dev, fmt, ##__VA_ARGS__); \ -}) -#endif - -#ifdef VERBOSE_DEBUG -#define dev_vdbg dev_dbg -#else -#define dev_vdbg(dev, fmt, ...) \ -({ \ - if (0) \ - __dev_printk(LOGL_DEBUG, dev, fmt, ##__VA_ARGS__); \ -}) -#endif +#define dev_vdbg(dev, fmt, ...) \ + __dev_printk(LOGL_DEBUG_CONTENT, dev, fmt, ##__VA_ARGS__)
#endif
On 9/12/20 8:28 PM, Sean Anderson wrote:
This series adds some additional information to dev_dbg. With this series, and the following options applied in addition to sandbox_defconfig
CONFIG_LOGLEVEL=8 CONFIG_LOG=n CONFIG_CMD_LOG=n CONFIG_CMD_TPM=n
the output of U-Boot is
U-Boot 2020.10-rc4-00006-g19b641dddd-dirty (Sep 12 2020 - 18:15:00 -0400)
Model: sandbox DRAM: 128 MiB pinctrl-gpio sandbox_pinctrl_gpio: set_state_simple op missing leds gpio_led: set_state_simple op missing default_on gpio_led: set_state_simple op missing base-gpios sandbox_gpio: set_state_simple op missing default_off gpio_led: set_state_simple op missing wdt@0 wdt_sandbox: set_state_simple op missing WDT: Started with servicing (60s timeout) MMC: mmc0 mmc_sandbox: set_state_simple op missing mmc1 mmc_sandbox: set_state_simple op missing mmc2 mmc_sandbox: set_state_simple op missing mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD) lcd sandbox_lcd_sdl: set_state_simple op missing In: serial Out: vidconsole Err: vidconsole Model: sandbox hog_input_active_low gpio_hog: set_state_simple op missing hog_input_active_high gpio_hog: set_state_simple op missing hog_output_low gpio_hog: set_state_simple op missing hog_output_high gpio_hog: set_state_simple op missing cros-ec google_cros_ec_sandbox: set_state_simple op missing SCSI: pci_ep pci_ep_sandbox: set_state_simple op missing Net: eth@10002000 eth_sandbox: set_state_simple op missing eth0: eth@10002000eth@10003000 eth_sandbox: set_state_simple op missing , eth5: eth@10003000sbe5 eth_sandbox: set_state_simple op missing , eth3: sbe5eth@10004000 eth_sandbox: set_state_simple op missing , eth6: eth@10004000 bootcount@0 bootcount-rtc: set_state_simple op missing i2c@0 i2c_sandbox: set_state_simple op missing rtc@61 rtc-sandbox: set_state_simple op missing emul i2c_emul_parent_drv: set_state_simple op missing emull sandbox_i2c_rtc_emul: set_state_simple op missing Hit any key to stop autoboot:
When using CONFIG_LOG the output of U-Boot is too verbose to fit in this email. Note that there is a soft dependency on [1] if you would like to test this patch with CONFIG_LOG and a higher LOG_LEVEL than LOGL_INFO.
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=201343
Sean Anderson (4): remoteproc: Remove unused function rproc_elf_sanity_check net: mdio: Supply appropriate devices for dev_dgb calls dm: Use symbolic constants for log levels in dev_xxx() dm: Print device name in dev_xxx() like Linux
drivers/remoteproc/rproc-elf-loader.c | 16 ------ include/dm/device_compat.h | 75 +++++++++++++-------------- include/remoteproc.h | 13 ----- net/mdio-uclass.c | 4 +- 4 files changed, 39 insertions(+), 69 deletions(-)
In my haste I may have forgotten to run CI and make many more patches like the first two to fix drivers calling a dev_ log function without an actual device. So this series breaks a lot of things at the moment [1]. Consider it an RFC I guess for after I fix all those build failures.
--Sean
[1] https://dev.azure.com/seanga2/u-boot/_build/results?buildId=30&view=logs...
participants (2)
-
Sean Anderson -
Simon Glass