[U-Boot] [PATCH v2 6/7] cmd: Add axi command

[Mario Six]

Hi Simon,

On Fri, May 25, 2018 at 4:41 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Mario,
>
> On 23 May 2018 at 06:10, Mario Six <mario.six at gdsys.cc> wrote:
>> Add a command to debug the AXI bus.
>>
>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>>
>> ---
>>
>> v1 -> v2:
>> No changes
>>
>> ---
>>  cmd/Kconfig  |   8 ++
>>  cmd/Makefile |   1 +
>>  cmd/axi.c    | 310 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 319 insertions(+)
>>  create mode 100644 cmd/axi.c
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Comments below.
>
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 38406fcfdac..0f730f7a397 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -995,6 +995,14 @@ config CMD_USB_MASS_STORAGE
>>         help
>>           USB mass storage support
>>
>> +config CMD_AXI
>> +       bool "axi"
>> +       depends on AXI
>> +       help
>> +         Enable the command "axi" for accessing AXI (Advanced eXtensible
>> +         Interface) busses, a on-chip interconnect specification for managing
>> +         functional blocks in SoC designs, which is also often used in designs
>> +         involving FPGAs (e.g.  communication with IP cores in Xilinx FPGAs).
>>  endmenu
>>
>>
>> diff --git a/cmd/Makefile b/cmd/Makefile
>> index 0d7322ee0a4..cfca11499ec 100644
>> --- a/cmd/Makefile
>> +++ b/cmd/Makefile
>> @@ -145,6 +145,7 @@ obj-$(CONFIG_CMD_ZFS) += zfs.o
>>  obj-$(CONFIG_CMD_DFU) += dfu.o
>>  obj-$(CONFIG_CMD_GPT) += gpt.o
>>  obj-$(CONFIG_CMD_ETHSW) += ethsw.o
>> +obj-$(CONFIG_CMD_AXI) += axi.o
>>
>>  # Power
>>  obj-$(CONFIG_CMD_PMIC) += pmic.o
>> diff --git a/cmd/axi.c b/cmd/axi.c
>> new file mode 100644
>> index 00000000000..d8001875e2c
>> --- /dev/null
>> +++ b/cmd/axi.c
>> @@ -0,0 +1,310 @@
>> +/*
>> + * (C) Copyright 2016
>> + * Dirk Eibach,  Guntermann & Drunck GmbH, dirk.eibach at gdsys.cc
>> + *
>> + * (C) Copyright 2017
>> + * Mario Six,  Guntermann & Drunck GmbH, mario.six at gdsys.cc
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <command.h>
>> +#include <console.h>
>> +#include <axi.h>
>> +
>> +static struct udevice *axi_cur_bus;
>> +static uint dp_last_size;
>> +static uint dp_last_addr;
>> +static uint dp_last_length = 0x40;
>> +
>> +static void show_bus(struct udevice *bus)
>> +{
>> +       struct udevice *dev;
>> +
>> +       printf("Bus %d:\t%s", bus->req_seq, bus->name);
>> +       if (device_active(bus))
>> +               printf("  (active %d)", bus->seq);
>> +       printf("\n");
>> +       for (device_find_first_child(bus, &dev);
>> +            dev;
>> +            device_find_next_child(&dev))
>> +               printf("  %s\n", dev->name);
>> +}
>> +
>> +static int do_axi_show_bus(cmd_tbl_t *cmdtp, int flag, int argc,
>> +                          char * const argv[])
>> +{
>> +       if (argc == 1) {
>> +               /* show all busses */
>> +               struct udevice *bus;
>> +
>> +               for (uclass_first_device(UCLASS_AXI, &bus);
>> +                    bus;
>> +                    uclass_next_device(&bus))
>> +                       show_bus(bus);
>> +       } else {
>> +               int i;
>> +
>> +               /* show specific bus */
>> +               i = simple_strtoul(argv[1], NULL, 10);
>> +
>> +               struct udevice *bus;
>> +               int ret;
>> +
>> +               ret = uclass_get_device_by_seq(UCLASS_AXI, i, &bus);
>
> Here you probe the bus, whereas above you don't. Is that intended?
>

Right, I should probably make sure that the busses are probed in either case,
good point.

Will be fixed in v3.

>> +               if (ret) {
>> +                       printf("Invalid bus %d: err=%d\n", i, ret);
>> +                       return CMD_RET_FAILURE;
>> +               }
>> +               show_bus(bus);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int cmd_axi_set_bus_num(unsigned int busnum)
>> +{
>> +       struct udevice *bus;
>> +       struct udevice *dummy;
>> +       int ret;
>> +
>> +       /* Make sure that all sequence numbers are initialized */
>> +       for (uclass_first_device(UCLASS_AXI, &dummy);
>> +            dummy;
>> +            uclass_next_device(&dummy))
>> +               ;
>> +
>> +       ret = uclass_get_device_by_seq(UCLASS_AXI, busnum, &bus);
>
> Here you probe the bus which seems right to me.
>

I'll use this same approach in the other function as well.

>> +       if (ret) {
>> +               debug("%s: No bus %d\n", __func__, busnum);
>> +               return ret;
>> +       }
>> +       axi_cur_bus = bus;
>> +
>> +       return 0;
>> +}
>> +
> [..]
>
> Regards,
> Simon

Best regards,
Mario