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

[Lukasz Majewski <l.majewski@samsung.com>]

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_ */



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group