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

[Abdellatif El Khlifi]

Hi Ilias,

On Mon, Jul 03, 2023 at 12:59:58PM +0300, Ilias Apalodimas wrote:
> > > [...]
> > > > +int do_ffa_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > > > +{
> > > > +       struct ffa_send_direct_data msg = {
> > > > +                       .data0 = 0xaaaaaaaa,
> > > > +                       .data1 = 0xbbbbbbbb,
> > > > +                       .data2 = 0xcccccccc,
> > > > +                       .data3 = 0xdddddddd,
> > > > +                       .data4 = 0xeeeeeeee,
> > > > +       };
> > > > +       u16 part_id;
> > > > +       int ret;
> > > > +       struct udevice *dev;
> > > > +
> > > > +       if (argc != 2) {
> > > > +               log_err("Missing argument\n");
> > > > +               return CMD_RET_USAGE;
> > > > +       }
> > > > +
> > > > +       errno = 0;
> > > > +       part_id = strtoul(argv[1], NULL, 16);
> > > > +
> > > > +       if (errno) {
> > >
> > > Is errno used in strtoul?
> >
> > Yes, please refer to [1].
> >
> > [1]: https://man7.org/linux/man-pages/man3/strtoul.3.html
> 
> 
> that's what the libc version does.  Can you check the u-boot version?
> 

Short answer: errno not used in strtoul() an I'm gonna remove the errno check.

More details:

strtoul() is defined as simple_strtoul() in strto.c
errno variable is not set by simple_strtoul() or its callees.
errno.h is included by strto.c to access the error codes.

> > >
...
> > > > +int do_ffa_devlist(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > > > +{
> > > > +       struct udevice *dev;
> > > > +       int ret;
> > > > +
> > > > +       ret = ffa_get_dev(&dev);
> > > > +       if (ret)
> > > > +               return CMD_RET_FAILURE;
> > > > +
> > > > +       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}
      

Cheers,
Abdellatif