| Message ID | 1489564565-20856-3-git-send-email-eddie.cai.linux@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Eddie, On 15 March 2017 at 01:56, Eddie Cai <eddie.cai.linux@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@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? > > 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? > + > +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? > > 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@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 > + remove extra blank line > + > +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? > + > + 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. > + printf("do_rockusb1\n"); Do you want this? > + 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? > + 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? > + "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? > @@ -0,0 +1,13 @@ > +/* > + * (C) Copyright 2017 > + * > + * Eddie Cai <eddie.cai.linux@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. > + > +#endif /* _ROCKUSB_H_ */ > -- > 2.7.4 > Regards, Simon
Hi Simon 2017-03-20 10:29 GMT+08:00 Simon Glass <sjg@chromium.org>: > Hi Eddie, > > On 15 March 2017 at 01:56, Eddie Cai <eddie.cai.linux@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@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@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@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 >
Hi Eddie, On 20 March 2017 at 02:14, Eddie Cai <eddie.cai.linux@gmail.com> wrote: > Hi Simon > > 2017-03-20 10:29 GMT+08:00 Simon Glass <sjg@chromium.org>: >> >> Hi Eddie, >> >> On 15 March 2017 at 01:56, Eddie Cai <eddie.cai.linux@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@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 >> [..] >> >> > + "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 ? No I was thinking of arch/arm/include/asm/arch-rockchip. Because this is a rockchip-specific file, isn't it? Regards, Simon
Hi Eddie, > 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@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 > > 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. > + > +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 > > 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@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> > + > + Please remove the extra blank here. (also run your patches through ./scripts/checkpatch.pl before submission. > +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]; > + } I would prefer to force the whole command syntax support: "rockchip 0 mmc 0" to comply with other commands (like thor, dfu). Instead of "rockchip 0 0" and imply the mmc. > + dev_index = simple_strtoul(devnum, NULL, 0); > + rockusb_dev_init(devtype, dev_index); > + > + 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; > + printf("do_rockusb1\n"); ^^^^^^^^^^^^^^^^^^^^^^^^ this should be either removed or rewritten as "debug(...)". > + 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"); > + 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", > + "rockusb <USB_controller> [<devtype>] <devnum> e.g. rockusb > 0 mmc 0\n" > + " devtype defaults to mmc" [<devtype>] should be mandatory. > +); > diff --git a/include/rockusb.h b/include/rockusb.h > new file mode 100644 > index 0000000..cdea63d > --- /dev/null > +++ b/include/rockusb.h > @@ -0,0 +1,13 @@ > +/* > + * (C) Copyright 2017 > + * > + * Eddie Cai <eddie.cai.linux@gmail.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > +#ifndef _ROCKUSB_H_ > +#define _ROCKUSB_H_ > + > +void rockusb_dev_init(char *dev_type, int dev_index); > + > +#endif /* _ROCKUSB_H_ */ Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
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. + +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 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@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> + + +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); + + 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; + printf("do_rockusb1\n"); + 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"); + 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", + "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 @@ -0,0 +1,13 @@ +/* + * (C) Copyright 2017 + * + * Eddie Cai <eddie.cai.linux@gmail.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ +#ifndef _ROCKUSB_H_ +#define _ROCKUSB_H_ + +void rockusb_dev_init(char *dev_type, int dev_index); + +#endif /* _ROCKUSB_H_ */
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@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