Re: [U-Boot] [PATCH 1/7] dfu:usb: Support for g_dnl composite download gadget.

Hi Marek,

Thanks for review.

Dear Lukasz Majewski,

...

Composite USB download gadget support (g_dnl) for download functions. This code works on top of composite gadget.

Signed-off-by: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com Cc: Marek Vasut marex@denx.de

---

drivers/usb/gadget/Makefile | 1 + drivers/usb/gadget/g_dnl.c | 235 +++++++++++++++++++++++++++++++++++++++++++ include/g_dnl.h | 33 ++++++ 3 files changed, 269 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/gadget/g_dnl.c create mode 100644 include/g_dnl.h

diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile index 87d1918..2c067c8 100644 --- a/drivers/usb/gadget/Makefile +++ b/drivers/usb/gadget/Makefile @@ -29,6 +29,7 @@ LIB := $(obj)libusb_gadget.o ifdef CONFIG_USB_GADGET COBJS-y += epautoconf.o config.o usbstring.o COBJS-$(CONFIG_USB_GADGET_S3C_UDC_OTG) += s3c_udc_otg.o +COBJS-$(CONFIG_USBDOWNLOAD_GADGET) += g_dnl.o endif ifdef CONFIG_USB_ETHER COBJS-y += ether.o epautoconf.o config.o usbstring.o diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c new file mode 100644 index 0000000..7925d0e --- /dev/null +++ b/drivers/usb/gadget/g_dnl.c @@ -0,0 +1,235 @@ +/*

• • g_dnl.c -- USB Downloader Gadget
• • Copyright (C) 2012 Samsung Electronics
• • Lukasz Majewski l.majewski@samsung.com
• • This program is free software; you can redistribute it and/or

modify

• • it under the terms of the GNU General Public License as

published by

• • the Free Software Foundation; either version 2 of the License,

or

• • (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public

License

• • along with this program; if not, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA

02111-1307 USA + */

+#include <errno.h> +#include <common.h> +#include <malloc.h> +#include <linux/usb/ch9.h> +#include <usbdescriptors.h> +#include <linux/usb/gadget.h>

+#include <mmc.h> +#include <part.h>

+#include <g_dnl.h> +#include "f_dfu.h"

+#include "gadget_chips.h" +#include "composite.c"

+/* Samsung's IDs */ +#define DRIVER_VENDOR_NUM 0x04E8 +#define DRIVER_PRODUCT_NUM 0x6601

Is it samsung specific? Or can we use some USB IDs that are not bound to samsung?

I think, that those IDs shall be defined at include/configs/<board>.h In that way one can use any ID.

...

+#define STRING_MANUFACTURER 25 +#define STRING_PRODUCT 2 +#define STRING_USBDOWN 2 +#define CONFIG_USBDOWNLOADER 2

+#define DRIVER_VERSION "usb_dnl 2.0"

+static const char shortname[] = "usb_dnl_"; +static const char product[] = "SLP USB download gadget"; +static const char manufacturer[] = "Samsung";

DITTO?

Manufacturer field can be either omitted or changed to other name.

...

+static struct usb_device_descriptor device_desc = {

• .bLength = sizeof device_desc,
• .bDescriptorType = USB_DT_DEVICE,
• .bcdUSB = __constant_cpu_to_le16(0x0200),
• .bDeviceClass = USB_CLASS_COMM,
• .bDeviceSubClass = 0x02, /*0x02:CDC-modem ,

0x00:CDC-serial*/ +

• .idVendor =

__constant_cpu_to_le16(DRIVER_VENDOR_NUM),

• .idProduct =

__constant_cpu_to_le16(DRIVER_PRODUCT_NUM),

• .iProduct = STRING_PRODUCT,
• .bNumConfigurations = 1,

+};

+static const struct usb_descriptor_header *otg_desc[] = {

• (struct usb_descriptor_header *) &(struct

usb_otg_descriptor){

• .bLength =		sizeof(struct
  

usb_otg_descriptor),

• .bDescriptorType =	USB_DT_OTG,
  

• .bmAttributes =		USB_OTG_SRP,
  

• },
• NULL,

+};

+/* static strings, in UTF-8 */ +static struct usb_string odin_string_defs[] = {

• { 0, manufacturer, },
• { 1, product, },

+};

+static struct usb_gadget_strings odin_string_tab = {

• .language = 0x0409, /* en-us */
• .strings = odin_string_defs,

+};

+static struct usb_gadget_strings *g_dnl_composite_strings[] = {

• &odin_string_tab,
• NULL,

+};

+static int g_dnl_unbind(struct usb_composite_dev *cdev) +{

• debug("%s\n", __func__);
• return 0;

+}

+static int g_dnl_do_config(struct usb_configuration *c) +{

• int ret = -1;
• char *s = (char *) c->cdev->driver->name;
• debug("%s: c: 0x%p cdev: 0x%p\n", __func__, c, c->cdev);

Can you elaborate in that debug message more? It's not really useful ;-)

I will correct that.

...

• if (gadget_is_otg(c->cdev->gadget)) {

• c->descriptors = otg_desc;
  

• c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
  

• }
• printf("GADGET DRIVER: %s\n", s);
• if (!strcmp(s, "usb_dnl_dfu"))

• ret = dfu_add(c);
  

And if this fails, we should set ret= as well, no?

The dynamic variable "ret" is defined with -1 value on the beginning of the function. When comparison fails, the -1 value is returned.

...

• return ret;

+}

+static int g_dnl_config_register(struct usb_composite_dev *cdev) +{

• debug("%s:\n", __func__);
• static struct usb_configuration config = {

• .label = "usb_dnload",
  

• .bmAttributes =	USB_CONFIG_ATT_ONE |
  

USB_CONFIG_ATT_SELFPOWER,

• .bConfigurationValue =	CONFIG_USBDOWNLOADER,
  

• .iConfiguration =	STRING_USBDOWN,
  

• .bind = g_dnl_do_config,
  

• };
• return usb_add_config(cdev, &config);

+}

+static int g_dnl_bind(struct usb_composite_dev *cdev) +{

• int gcnum;
• int id, ret;
• struct usb_gadget *gadget = cdev->gadget;
• debug("%s: gadget: 0x%p cdev: 0x%p\n", __func__, gadget,

cdev); +

• id = usb_string_id(cdev);
• if (id < 0)

• return id;
  

• odin_string_defs[0].id = id;
• device_desc.iManufacturer = id;
• id = usb_string_id(cdev);
• if (id < 0)

• return id;
  

• odin_string_defs[1].id = id;
• device_desc.iProduct = id;
• ret = g_dnl_config_register(cdev);
• if (ret)

• goto error;
  

Two newlines

Will fix.

...

• gcnum = usb_gadget_controller_number(gadget);
• debug("gcnum: %d\n", gcnum);
• if (gcnum >= 0)

• device_desc.bcdDevice = cpu_to_le16(0x0200 +
  

gcnum);

• else {

• debug("%s: controller '%s' not recognized\n",
  

• 	shortname, gadget->name);
  

• device_desc.bcdDevice =
  

__constant_cpu_to_le16(0x9999);

• }
• return 0;
• error:
• g_dnl_unbind(cdev);
• return -ENOMEM;

+}

+static void g_dnl_suspend(struct usb_composite_dev *cdev) +{

• if (cdev->gadget->speed == USB_SPEED_UNKNOWN)

• return;
  

• debug("suspend\n");

+}

+static void g_dnl_resume(struct usb_composite_dev *cdev) +{

• debug("resume\n");

+}

Maybe you should check if suspend/resume are set at all. Then you won't need the above stubs.

I will remove them.

...

+static struct usb_composite_driver g_dnl_driver = {

• .name = NULL,
• .dev = &device_desc,
• .strings = g_dnl_composite_strings,
• .bind = g_dnl_bind,
• .unbind = g_dnl_unbind,
• .suspend = g_dnl_suspend,
• .resume = g_dnl_resume,

+};

+int g_dnl_init(char *s) +{

• int ret;
• static char str[16];

Why use '\0' if you can just use 0 ?

I will change.

...

• memset(str, '\0', sizeof(str));
• strncpy(str, shortname, sizeof(shortname));
• if (!strncmp(s, "dfu", sizeof(s))) {

• strncat(str, s, sizeof(str));
  

• } else {

• printf("%s: unknown command: %s\n", __func__, s);
  

• return -EINVAL;
  

• }
• g_dnl_driver.name = str;
• debug("%s: g_dnl_driver.name: %s\n", __func__,

g_dnl_driver.name);

• ret = usb_composite_register(&g_dnl_driver);
• if (ret) {

• printf("%s: failed!, error:%d ", __func__, ret);
  

Missing newline in printf() (...error:%d ").

I will change that.

...

• return ret;
  

• }
• return 0;

+}

+void g_dnl_cleanup(void) +{

• usb_composite_unregister(&g_dnl_driver);

+} diff --git a/include/g_dnl.h b/include/g_dnl.h new file mode 100644 index 0000000..5d0e9a5 --- /dev/null +++ b/include/g_dnl.h @@ -0,0 +1,33 @@ +/*

• • Copyright (C) 2012 Samsung Electronics
• • Lukasz Majewski l.majewski@samsung.com
• • This program is free software; you can redistribute it and/or
• • modify it under the terms of the GNU General Public License as
• • published by the Free Software Foundation; either version 2 of
• • the License, or (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public

License

• • along with this program; if not, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston,
• • MA 02111-1307 USA
• */

+#ifndef __G_DOWNLOAD_H_ +#define __G_DOWNLOAD_H_

+#include <linux/usb/ch9.h> +#include <usbdescriptors.h> +#include <linux/usb/gadget.h>

+int g_dnl_init(char *s); +void g_dnl_cleanup(void);

+/* USB initialization declaration - board specific*/ +void board_usb_init(void); +#endif /* __G_DOWNLOAD_H_ */