[U-Boot] [PATCH 1/3] vsprintf: Support phys_addr_t specifier unconditionally

[Thierry Reding]

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 at gmail.com> wrote:
> >
> > From: Thierry Reding <treding at 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 at 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", &reg);
    drivers/pinctrl/pinctrl-single.c:69:            dev_dbg(dev, "  reg/val 0x%pa/0x%08x\n", &reg, 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190401/7c7e9d52/attachment.sig>