[U-Boot] [U-Boot 2/3] cmd: add rockusb command

[Eddie Cai]

Hi Simon

2017-03-20 10:29 GMT+08:00 Simon Glass <sjg at chromium.org>:

> Hi Eddie,
>
> On 15 March 2017 at 01:56, Eddie Cai <eddie.cai.linux at gmail.com> wrote:
> >
> > this patch add rockusb command. the usage is
> >         rockusb <USB_controller> [<devtype>] <devnum>
> > e.g. rockusb 0 mmc 0
> >
> > Signed-off-by: Eddie Cai <eddie.cai.linux at gmail.com>
> > ---
> >  cmd/Kconfig       | 12 +++++++++
> >  cmd/Makefile      |  1 +
> >  cmd/rockusb.c     | 79 ++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++
> >  include/rockusb.h | 13 +++++++++
> >  4 files changed, 105 insertions(+)
> >  create mode 100644 cmd/rockusb.c
> >  create mode 100644 include/rockusb.h
>
> Can you please add some documentation on how to use this command and
> what host tools you need?
>
Sure, I will add  README.rockusb in next version

>
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index ef53156..dac2cc0 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -475,6 +475,18 @@ config CMD_DFU
> >           Enables the command "dfu" which is used to have U-Boot create
> a DFU
> >           class device via USB.
> >
> > +config CMD_ROCKUSB
> > +        bool "rockusb"
> > +        select USB_FUNCTION_ROCKUSB
> > +        help
> > +          Enables the command "rockusb" which is used to have U-Boot
> create a
> > +          Rockusb class device via USB.
>
> Does this mean Rockchip-class device?
>
 Yes, This is vendor specific class.

>
> > +
> > +config USB_FUNCTION_ROCKUSB
> > +        bool "Enable USB rockusb gadget"
> > +        help
> > +          This enables the USB part of the rockusb gadget.
> > +
> >  config CMD_USB_MASS_STORAGE
> >         bool "UMS usb mass storage"
> >         help
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index f13bb8c..f5f1663 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -141,6 +141,7 @@ endif
> >
> >  obj-$(CONFIG_CMD_USB) += usb.o disk.o
> >  obj-$(CONFIG_CMD_FASTBOOT) += fastboot.o
> > +obj-$(CONFIG_CMD_ROCKUSB) += rockusb.o
> >  obj-$(CONFIG_CMD_FS_UUID) += fs_uuid.o
>
> Can you put it in alphabetical order if possible?
>
Sure, I will try to put it in  alphabetical order in next version

>
> >
> >  obj-$(CONFIG_CMD_USB_MASS_STORAGE) += usb_mass_storage.o
> > diff --git a/cmd/rockusb.c b/cmd/rockusb.c
> > new file mode 100644
> > index 0000000..f5bd86e
> > --- /dev/null
> > +++ b/cmd/rockusb.c
> > @@ -0,0 +1,79 @@
> > +/*
> > + * Copyright (C) 2017 Eddie Cai <eddie.cai.linux at gmail.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <console.h>
> > +#include <g_dnl.h>
> > +#include <usb.h>
> > +#include <rockusb.h>
>
> Should go above usb.h
>
done

>
> > +
>
> remove extra blank line
>
done

>
> > +
> > +static int do_rockusb(cmd_tbl_t *cmdtp, int flag, int argc, char *const
> argv[])
> > +{
> > +       int controller_index, dev_index;
> > +       char *usb_controller;
> > +       char *devtype;
> > +       char *devnum;
> > +       int ret;
> > +
> > +       if (argc < 2)
> > +               return CMD_RET_USAGE;
> > +
> > +       usb_controller = argv[1];
> > +       controller_index = simple_strtoul(usb_controller, NULL, 0);
> > +
> > +       if (argc >= 4) {
> > +               devtype = argv[2];
> > +               devnum  = argv[3];
> > +       } else {
> > +               devtype = "mmc";
> > +               devnum  = argv[2];
> > +       }
> > +       dev_index = simple_strtoul(devnum, NULL, 0);
> > +       rockusb_dev_init(devtype, dev_index);
>
> Can this fail, e.g. if index is out of range?
>
I will add a return value in next version

>
> > +
> > +       ret = board_usb_init(controller_index, USB_INIT_DEVICE);
> > +       if (ret) {
> > +               error("USB init failed: %d", ret);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       g_dnl_clear_detach();
> > +       ret = g_dnl_register("usb_dnl_rockusb");
> > +       if (ret)
> > +               return ret;
>
> Commands should return either 0 or CMD_RET_FAILURE.
>
OK, i will fix it in next version.

>
> > +       printf("do_rockusb1\n");
>
> Do you want this?
>
Oops, i will remove it in next version.

>
> > +       if (!g_dnl_board_usb_cable_connected()) {
> > +               puts("\rUSB cable not detected.\n" \
> > +                    "Command exit.\n");
> > +               ret = CMD_RET_FAILURE;
> > +               goto exit;
> > +       }
> > +
> > +       while (1) {
> > +               if (g_dnl_detach())
> > +                       break;
> > +               if (ctrlc())
> > +                       break;
> > +               usb_gadget_handle_interrupts(controller_index);
> > +       }
> > +       printf("do_rockusb2\n");
>
> Do you want this?
>
i will remove it in next version.

>
> > +       ret = CMD_RET_SUCCESS;
> > +
> > +exit:
> > +       g_dnl_unregister();
> > +       g_dnl_clear_detach();
> > +       board_usb_cleanup(controller_index, USB_INIT_DEVICE);
> > +
> > +       return ret;
> > +}
> > +
> > +U_BOOT_CMD(rockusb, 4, 1, do_rockusb,
> > +       "Use the ROCKUSB",
>
> Can you add a little more here?
>
sure

>
> > +       "rockusb <USB_controller> [<devtype>] <devnum>  e.g. rockusb 0
> mmc 0\n"
> > +       "    devtype defaults to mmc"
> > +);
> > diff --git a/include/rockusb.h b/include/rockusb.h
> > new file mode 100644
> > index 0000000..cdea63d
> > --- /dev/null
> > +++ b/include/rockusb.h
>
> Can this go in include/asm instead?
>
do you mean include/asm-generic ? may i know why ?

>
> > @@ -0,0 +1,13 @@
> > +/*
> > + * (C) Copyright 2017
> > + *
> > + * Eddie Cai <eddie.cai.linux at gmail.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +#ifndef _ROCKUSB_H_
> > +#define _ROCKUSB_H_
> > +
> > +void rockusb_dev_init(char *dev_type, int dev_index);
>
> Please add a function comment.
>
OK, i will add a function comment in next version .

>
> > +
> > +#endif /* _ROCKUSB_H_ */
> > --
> > 2.7.4
> >
>
> Regards,
> Simon
>