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

[Simon Glass]

Hi Abdellatif,

On Mon, 3 Jul 2023 at 13:09, Abdellatif El Khlifi
<abdellatif.elkhlifi at arm.com> wrote:
>
> 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}

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?

Regards,
Simon