[U-Boot] [PATCH 1/3] vsprintf: Support phys_addr_t specifier unconditionally
From: Thierry Reding treding@nvidia.com
When phys_addr_t printf specifier support was first introduced in commit 1eebd14b7902 ("vsprintf: Add modifier for phys_addr_t"), it was enabled only if CONFIG_CMD_NET was selected. Since physical addresses are not unique to networking support it doesn't make sense to conditionally add it in those cases only. Move support for it outside of the CMD_NET guard so that the specifier is always supported.
Signed-off-by: Thierry Reding treding@nvidia.com --- lib/vsprintf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 1b6c154d8d72..2403825dc98d 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -457,7 +457,6 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, return device_path_string(buf, end, ptr, field_width, precision, flags); #endif -#ifdef CONFIG_CMD_NET case 'a': flags |= SPECIAL | ZEROPAD;
@@ -469,6 +468,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, break; } break; +#ifdef CONFIG_CMD_NET case 'm': flags |= SPECIAL; /* Fallthrough */
From: Thierry Reding treding@nvidia.com
If 64-bit physical addresses support is enabled, make sure the sandox defines the correct types for phys_addr_t and phys_size_t.
Signed-off-by: Thierry Reding treding@nvidia.com --- arch/sandbox/include/asm/types.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h index 7cd56b41474f..c1a5d2af8285 100644 --- a/arch/sandbox/include/asm/types.h +++ b/arch/sandbox/include/asm/types.h @@ -18,21 +18,21 @@ typedef unsigned short umode_t; /* * Number of bits in a C 'long' on this architecture. */ -#ifdef CONFIG_PHYS64 +#ifdef CONFIG_PHYS_64BIT #define BITS_PER_LONG 64 -#else /* CONFIG_PHYS64 */ +#else /* CONFIG_PHYS_64BIT */ #define BITS_PER_LONG 32 -#endif /* CONFIG_PHYS64 */ +#endif /* CONFIG_PHYS_64BIT */
-#ifdef CONFIG_PHYS64 +#ifdef CONFIG_PHYS_64BIT typedef unsigned long long dma_addr_t; typedef u64 phys_addr_t; typedef u64 phys_size_t; -#else /* CONFIG_PHYS64 */ +#else /* CONFIG_PHYS_64BIT */ typedef unsigned long dma_addr_t; typedef u32 phys_addr_t; typedef u32 phys_size_t; -#endif /* CONFIG_PHYS64 */ +#endif /* CONFIG_PHYS_64BIT */
#endif /* __KERNEL__ */
On Tue, 12 Mar 2019 at 04:38, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
If 64-bit physical addresses support is enabled, make sure the sandox defines the correct types for phys_addr_t and phys_size_t.
Signed-off-by: Thierry Reding treding@nvidia.com
arch/sandbox/include/asm/types.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Applied to u-boot-dm, thanks!
From: Thierry Reding treding@nvidia.com
Use the %pap printf specifier to print physical addresses. The physical address is passed by reference and hence avoids the need to play tricks with the preprocessor to use the correct specifier.
Signed-off-by: Thierry Reding treding@nvidia.com --- arch/sandbox/lib/pci_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/sandbox/lib/pci_io.c b/arch/sandbox/lib/pci_io.c index 5039973cd7af..01822c606956 100644 --- a/arch/sandbox/lib/pci_io.c +++ b/arch/sandbox/lib/pci_io.c @@ -34,7 +34,7 @@ int pci_map_physmem(phys_addr_t paddr, unsigned long *lenp, return 0; }
- debug("%s: failed: addr=%x\n", __func__, paddr); + debug("%s: failed: addr=%pap\n", __func__, &paddr); return -ENOSYS; }
On Tue, 12 Mar 2019 at 04:38, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
When phys_addr_t printf specifier support was first introduced in commit 1eebd14b7902 ("vsprintf: Add modifier for phys_addr_t"), it was enabled only if CONFIG_CMD_NET was selected. Since physical addresses are not unique to networking support it doesn't make sense to conditionally add it in those cases only. Move support for it outside of the CMD_NET guard so that the specifier is always supported.
Signed-off-by: Thierry Reding treding@nvidia.com
lib/vsprintf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Is there a code-size impact here? Perhaps instead it should be a new Kconfig option, implied by CMD_NET?
Reviewed-by: Simon Glass sjg@chromium.org
Regards, Simon
On Sat, Mar 30, 2019 at 03:19:27PM -0600, Simon Glass wrote:
On Tue, 12 Mar 2019 at 04:38, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
When phys_addr_t printf specifier support was first introduced in commit 1eebd14b7902 ("vsprintf: Add modifier for phys_addr_t"), it was enabled only if CONFIG_CMD_NET was selected. Since physical addresses are not unique to networking support it doesn't make sense to conditionally add it in those cases only. Move support for it outside of the CMD_NET guard so that the specifier is always supported.
Signed-off-by: Thierry Reding treding@nvidia.com
lib/vsprintf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Is there a code-size impact here? Perhaps instead it should be a new Kconfig option, implied by CMD_NET?
There's a tiny code-size impact needed for the extra "case" statement, etc. But it shouldn't be more than maybe a handful of instructions. I don't think we can reasonable guard this by a Kconfig option because this specifier is used to print physical addresses, which can happen pretty much anywhere. A quick grep on a recent master shows that this is used in a number of different drivers:
$ git grep -n '".*%pa.*"' arch/arm/lib/cache-cp15.c:66: debug("%s: start=%pa, size=%zu, option=%llx\n", __func__, &start, size, arch/arm/lib/cache-cp15.c:69: debug("%s: start=%pa, size=%zu, option=0x%x\n", __func__, &start, size, arch/arm/lib/cache.c:84: debug("mapping memory %pa-%pa non-cached\n", &start, &end); arch/arm/lib/cache.c:102: debug("allocated %zu bytes of uncached memory @%pa\n", size, &next); drivers/pinctrl/pinctrl-single.c:52: dev_dbg(dev, " invalid register offset 0x%pa\n", ®); drivers/pinctrl/pinctrl-single.c:69: dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val); drivers/spi/fsl_dspi.c:653: debug("DSPI: regs=%pa, max-frequency=%d, endianess=%s, num-cs=%d\n", drivers/video/tegra.c:330: debug("LCD frame buffer at %pa, size %x\n", &priv->frame_buffer,
Guarding the specifier with a Kconfig symbol would require extending the list of dependencies everytime someone else starts using it, which means that people (i.e. reviewers) actually have to know about this, remember it and very closely look at the printf format string to detect whether a %pa is used or not.
Note also that while the above seems like only a small number of users, there are potentially a lot more that actually print out a physical address but use some other specifier to do so. You only really need the %pa if you need to support code across platforms where phys_addr_t has a different size.
Thierry
participants (2)
-
Simon Glass -
Thierry Reding