On Mon, 3 Jul 2023 at 16:53, Abdellatif El Khlifi
<abdellatif.elkhl...@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
