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

[Simon Glass]

On Mon, 3 Jul 2023 at 16:53, Abdellatif El Khlifi
<abdellatif.elkhlifi at arm.com> wrote:
>
> 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));

LGTM thanks

>
> Cheers
> Abdellatif