From: Christoph Niedermaier Sent: Thursday, April 10, 2025 2:17 PM > From: Michael Walle <mwa...@kernel.org> > Sent: Thursday, April 10, 2025 12:44 PM > > On Wed Apr 9, 2025 at 5:22 PM CEST, Tom Rini wrote: > > > On Wed, Apr 09, 2025 at 02:33:08PM +0200, Michael Walle wrote: > > > > Hi, > > > > > > > > > >> The formatting with %pa / %pap behaves like %x, which results in an > > > > > >> incorrect value being output. To improve this, a new fine-tuning > > > > > >> Kconfig XPL_USE_TINY_PRINTF_POINTER_SUPPORT for pointer formatting > > > > > >> has been added. If it is enabled, the output of %pa / %pap should > > > > > >> be correct, and if it is disabled, the pointer formatting is > > > > > >> completely unsupported. In addition to indicate unsupported > > > > > >> formatting, > > > > > >> '?' will be output. This allows enabling pointer formatting only > > > > > >> when needed. For SPL_NET and NET_LWIP it is selected by default. > > > > > >> Then it also supports the formatting with %pm, %pM and %pI4. > > > > > >> > > > > > >> Signed-off-by: Christoph Niedermaier > > > > > >> <cniederma...@dh-electronics.com> > > > > > >> --- > > > > > >> Cc: Tom Rini <tr...@konsulko.com> > > > > > >> Cc: Simon Glass <s...@chromium.org> > > > > > >> Cc: Michael Walle <mwa...@kernel.org> > > > > > >> Cc: Quentin Schulz <quentin.sch...@cherry.de> > > > > > >> Cc: Marek Vasut <ma...@denx.de> > > > > > >> Cc: Benedikt Spranger <b.spran...@linutronix.de> > > > > > >> Cc: Jerome Forissier <jerome.foriss...@linaro.org> > > > > > >> Cc: John Ogness <john.ogn...@linutronix.de> > > > > > >> Cc: Ilias Apalodimas <ilias.apalodi...@linaro.org> > > > > > >> --- > > > > > >> Kconfig | 1 + > > > > > >> common/spl/Kconfig | 1 + > > > > > >> lib/Kconfig | 8 ++++++++ > > > > > >> lib/tiny-printf.c | 45 > > > > > >> +++++++++++++++++++-------------------------- > > > > > >> 4 files changed, 29 insertions(+), 26 deletions(-) > > > > > >> > > > > > >> diff --git a/Kconfig b/Kconfig > > > > > >> index 6379a454166..4d13717294c 100644 > > > > > >> --- a/Kconfig > > > > > >> +++ b/Kconfig > > > > > >> @@ -757,6 +757,7 @@ config NET > > > > > >> > > > > > >> config NET_LWIP > > > > > >> bool "Use lwIP for networking stack" > > > > > >> + select XPL_USE_TINY_PRINTF_POINTER_SUPPORT if > > > > > >> SPL_USE_TINY_PRINTF || > > TPL_USE_TINY_PRINTF || VPL_USE_TINY_PRINTF > > > > > >> imply NETDEVICES > > > > > >> help > > > > > >> Include networking support based on the lwIP (lightweight IP) > > > > > >> diff --git a/common/spl/Kconfig b/common/spl/Kconfig > > > > > >> index 94e118f8465..72736dbecf5 100644 > > > > > >> --- a/common/spl/Kconfig > > > > > >> +++ b/common/spl/Kconfig > > > > > >> @@ -1096,6 +1096,7 @@ config SPL_DM_SPI_FLASH > > > > > >> config SPL_NET > > > > > >> bool "Support networking" > > > > > >> depends on !NET_LWIP > > > > > >> + select XPL_USE_TINY_PRINTF_POINTER_SUPPORT if > > > > > >> SPL_USE_TINY_PRINTF || > > TPL_USE_TINY_PRINTF || VPL_USE_TINY_PRINTF > > > > > >> help > > > > > >> Enable support for network devices (such as Ethernet) in SPL. > > > > > >> This permits SPL to load U-Boot over a network link rather > > > > > >> than > > > > > >> diff --git a/lib/Kconfig b/lib/Kconfig > > > > > >> index 1a683dea670..62e28d4a1f3 100644 > > > > > >> --- a/lib/Kconfig > > > > > >> +++ b/lib/Kconfig > > > > > >> @@ -253,6 +253,14 @@ config VPL_USE_TINY_PRINTF > > > > > >> > > > > > >> The supported format specifiers are %c, %s, %u/%d and %x. > > > > > >> > > > > > >> +config XPL_USE_TINY_PRINTF_POINTER_SUPPORT > > > > > >> + bool "Extend tiny printf with the pointer formatting %p" > > > > > >> + depends on SPL_USE_TINY_PRINTF || TPL_USE_TINY_PRINTF || > VPL_USE_TINY_PRINTF > > > > > >> + help > > > > > >> + This option enables the formatting of pointers %p. It supports > > > > > >> + %p and %pa / %pap. If this option is selected by SPL_NET or > > > > > >> NET_LWIP > > > > > >> + it also supports the formatting with %pm, %pM and %pI4. > > > > > > > > > > > > This isn't quite what I'd like to see. I don't want to start using > > > > > > the > > > > > > literal XPL namespace as that will lead to confusion down the line. > > > > > > > > > > OK, in V2 I will only support SPL. > > > > > > > > > > > Since we only have SPL_NET, I think we should name this symbol > > > > > > SPL_USE_TINY_PRINTF_POINTER_SUPPORT, not ask about it (so bool > > > > > > without > > > > > > "prompt text" following), and select from SPL_NET if > > > > > > SPL_USE_TINY_PRINTF. > > > > > > > > IIRC, the old one also enabled the pointer support if DEBUG is > > > > enabled. I don't think this will work with Kconfig. > > > > > > I was looking around for, but didn't quite see, a good existing option > > > to "if .." around the prompt text for. > > > > What I meant was that you normally do -DDEBUG on a file (or > > equally a #define DEBUG). Thus you cannot add it to Kconfig. > > > > > > > Now you will get the output '?' when using formatting with %p or %pa. > > > > > If someone wants to use the pointer support e.g. %pa in > > > > > pinctrl-single.c > > > > > and is restricted to use tiny printf, then it would be good to have > > > > > the option to enable it manually and not be forced to enable SPL_NET > > > > > or > > > > > NET_LWIP to have the pointer support enabled. In this case, it makes > > > > > sense to allow switching it on in menuconfig. > > > > > > > > FWIW, I'm also fine with enabling full printf support as long as the > > > > tiny one doesn't print misleading values. > > > > > > I'm not sure if the one non-debug %pa print in pinctrl-single.c is > > > really triggered within SPL, and I do hope that the way this patch is > > > otherwise done will make it easier if someone needs %pa to work when > > > debugging a problem in SPL, and can't enable full printf due to space. > > > > Yes, but the dev_dbg() triggered it because of the same reason as > > above, DEBUG isn't defined if you do it on a per-file basis. Or I'm > > getting that logic wrong, as I don't understand why _DEBUG (commit > > c091f65234cf introduced it but doesn't tell the reason). > > Should I replace _DEBUG with defined(DEBUG) in the next version? > > I also found something else: > [...] > @@ -307,6 +299,7 @@ static int _vprintf(struct printf_info *info, const char > *fmt, va_list > va) > case '%': > out(info, '%'); > default: > + out(info, '?'); > break; > } > [...] > If I add "out(info, '?');" in the default case for case '%' a break is > missing. > So for "%%" the output will be "%?".
I will send a version 2 as a new basis. Regards Christoph
