[PATCH v13 05/10] arm_ffa: introduce armffa command

[Abdellatif El Khlifi]

Hi Simon, Ilias,

On Mon, Jul 03, 2023 at 02:30:51PM +0100, Simon Glass wrote:
...
> > > > > > +       log_info("device name %s, dev %p, driver name %s, ops %p\n",
> > > > > > +                dev->name,
> > > > > > +               (void *)map_to_sysmem(dev),
> > > > > > +                dev->driver->name,
> > > > > > +                (void *)map_to_sysmem(dev->driver->ops));
> > > > >
> > > > > Isn't it more useful to print the physical address map_to_sysmem() retuns?
> > > >
> > > > That's what map_to_sysmem() does, it returns a physical address and it's  shown in the log.
> > >
> > > I dont have access to u-boot source right, but why do you need all
> > > these void * casts then?
> >
> > Because map_to_sysmem() returns an 'phys_addr_t' (aka 'long long unsigned int') . However, %p expects 'void *'.
> >
> > Compilation warning:
> >
> > cmd/armffa.c:181:18: warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘phys_addr_t’ {aka ‘long long unsigned int’} [8;
> > ;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat=-Wformat=8;;]
> >   181 |         log_info("device name %s, dev %p, driver name %s, ops %p\n",
> >       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   182 |                  dev->name,
> >   183 |                  map_to_sysmem(dev),
> >       |                  ~~~~~~~~~~~~~~~~~~
> >       |                  |
> >       |                  phys_addr_t {aka long long unsigned int}
> 
> You should be using %lx with map_to_sysmen() since it returns a . Use
> %p for pointers.
> 
> In general in U-Boot we use addresses (ulong) rather than pointers.
> Unfortunately the phys_addr_t type can be larger than the word size. I
> suggest creating a printf prefix for that type. See the top of blk.h
> for an example of how to do it. Perhaps in a follow-up patch?
> 

Sounds good. I'm gonna add it in a generic way under include/log.h if that makes sense:

        #ifdef CONFIG_PHYS_64BIT
        #define PhysAddrLength "ll"
        #else
        #define PhysAddrLength ""
        #endif
        #define PHYS_ADDR_LN "%" PhysAddrLength "x"
        #define PHYS_ADDR_LNU "%" PhysAddrLength "u"
        #endif

Note: In a 32-bit platform phys_addr_t is "unsigned int", in a 64-bit platform it is "long long unsigned int"

Then using it this way:

        log_info("device %s, addr " PHYS_ADDR_LN ", driver %s, ops " PHYS_ADDR_LN "\n",
            dev->name,
            map_to_sysmem(dev),
            dev->driver->name,
            map_to_sysmem(dev->driver->ops));

Cheers
Abdellatif