On Mon, 3 Jul 2023 at 12:55, Abdellatif El Khlifi
<abdellatif.elkhl...@arm.com> wrote:
>
> Hi Ilias,
>
> On Tue, Jun 20, 2023 at 05:25:51PM +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?
>
> > Does FF-A have any limits regarding the partition id? If yes, it would
> > be saner to check against that.
>
> The only value that would be invalid is 0. I'll add a check for that in v14.
>
> >
> > > + log_err("Invalid partition ID\n");
> > > + return CMD_RET_USAGE;
> > > + }
> > > +
> > > + ret = ffa_get_dev(&dev);
> > > + if (ret)
> > > + return CMD_RET_FAILURE;
> > > +
> > > + ret = ffa_sync_send_receive(dev, part_id, &msg, 1);
> > > + if (!ret) {
> > > + u8 cnt;
> > > +
> > > + log_info("SP response:\n[LSB]\n");
> > > + for (cnt = 0;
> > > + cnt < sizeof(struct ffa_send_direct_data) /
> > > sizeof(u64);
> > > + cnt++)
> > > + log_info("%llx\n", ((u64 *)&msg)[cnt]);
> >
> > I am not sure I understand why we print it like this.
>
> We would like to show the data received from secure world and in which order.
>
> example:
>
> corstone1000# armffa ping 0x8003
> SP response:
> [LSB]
> fffffffe
> 0
> 0
> 0
> 0
>
> >
> > > + return CMD_RET_SUCCESS;
> > > + }
> > > +
> > > + log_err("Sending direct request error (%d)\n", ret);
> > > + return CMD_RET_FAILURE;
> > > +}
> > > +
> > > +/**
> > > + *do_ffa_devlist() - implementation of the devlist subcommand
> > > + * @cmdtp: [in] Command Table
> > > + * @flag: flags
> > > + * @argc: number of arguments
> > > + * @argv: arguments
> > > + *
> > > + * Query the device belonging to the UCLASS_FFA
> > > + * class.
> > > + *
> > > + * Return:
> > > + *
> > > + * CMD_RET_SUCCESS: on success, otherwise failure
> > > + */
> > > +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?
Thanks
/Ilias
>
> Cheers,
> Abdellatif
