[PATCH 0/4] dm: Print device name in dev_xxx() like Linux

[PATCH 0/4] dm: Print device name in dev_xxx() like Linux

[Sean Anderson]

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 at 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 at 10002000 eth_sandbox: set_state_simple op missing
eth0: eth at 10002000eth@10003000 eth_sandbox: set_state_simple op missing
, eth5: eth at 10003000sbe5 eth_sandbox: set_state_simple op missing
, eth3: sbe5eth at 10004000 eth_sandbox: set_state_simple op missing
, eth6: eth at 10004000
bootcount at 0 bootcount-rtc: set_state_simple op missing
i2c at 0 i2c_sandbox: set_state_simple op missing
rtc at 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(-)

-- 
2.28.0

[PATCH 1/4] remoteproc: Remove unused function rproc_elf_sanity_check

[Sean Anderson]

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
-- 
2.28.0

[PATCH 2/4] net: mdio: Supply appropriate devices for dev_dgb calls

[Sean Anderson]

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;
 	}
-- 
2.28.0

[PATCH 3/4] dm: Use symbolic constants for log levels in dev_xxx()

[Sean Anderson]

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
 
-- 
2.28.0

[PATCH 4/4] dm: Print device name in dev_xxx() like Linux

[Sean Anderson]

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
-- 
2.28.0

[PATCH 0/4] dm: Print device name in dev_xxx() like Linux

[Sean Anderson]

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 at 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 at 10002000 eth_sandbox: set_state_simple op missing
> eth0: eth at 10002000eth@10003000 eth_sandbox: set_state_simple op missing
> , eth5: eth at 10003000sbe5 eth_sandbox: set_state_simple op missing
> , eth3: sbe5eth at 10004000 eth_sandbox: set_state_simple op missing
> , eth6: eth at 10004000
> bootcount at 0 bootcount-rtc: set_state_simple op missing
> i2c at 0 i2c_sandbox: set_state_simple op missing
> rtc at 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&j=7f2f73d6-a009-5260-5fdd-7738e0f76a89&t=97bcf3f1-e64d-5752-c0e9-95a663c62e27

[PATCH 1/4] remoteproc: Remove unused function rproc_elf_sanity_check

[Simon Glass]

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>

[PATCH 2/4] net: mdio: Supply appropriate devices for dev_dgb calls

[Simon Glass]

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).

[PATCH 3/4] dm: Use symbolic constants for log levels in dev_xxx()

[Simon Glass]

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>

[PATCH 2/4] net: mdio: Supply appropriate devices for dev_dgb calls

[Sean Anderson]

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

[PATCH 2/4] net: mdio: Supply appropriate devices for dev_dgb calls

[Simon Glass]

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.