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=-Wform...;;] 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