On Saturday 07 March 2009, tom.leiming@xxxxxxxxx wrote: > From: Ming Lei <tom.leiming@xxxxxxxxx> > > This patch introduces isochronous source sink configuration driver > into zero composite driver. The driver is based on David Brownell's > f_sourcesink.c I like the idea, but this needs a few updates. So ... NAK on this for now. The biggest behavioral change would be to always use at least double buffering ... there's no general way to ensure that the FIFO gets reloaded quickly enough for the next ISO packet, so it's best to avoid that issue. The way you have it now might work if the hardware has working double-buffering support, and the peripheral isn't too busy; but for high bandwidth, it will get fairly dicy. There should also be a minor structural change to the config: ISO endpoints may not reside in altsetting zero. So you've got to define another altsetting ... zero with no endpoints, and the ones you have here would be alt #1, with endpoints. > It can be used to evalute isochronous transfer performance of usb > host controller or usb host controller driver, also can be used to > troubleshoot usb host controller driver(usually some embedded hcd) > handily. > > I have verified the isochronous configuration driver in omap3-based > beagle board. Which is an exceptionally functional bit of hardware. ;) This should work on a few more devices before it gets into mainline. Both high speed and full speed. I can tell this won't work on some common hardware already. > > Signed-off-by: Ming Lei <tom.leiming@xxxxxxxxx> > --- > drivers/usb/gadget/f_iso.c | 479 ++++++++++++++++++++++++++++++++++++++++++++ > drivers/usb/gadget/zero.c | 25 ++- > 2 files changed, 501 insertions(+), 3 deletions(-) > create mode 100644 drivers/usb/gadget/f_iso.c > > diff --git a/drivers/usb/gadget/f_iso.c b/drivers/usb/gadget/f_iso.c > new file mode 100644 > index 0000000..18e7429 > --- /dev/null > +++ b/drivers/usb/gadget/f_iso.c > @@ -0,0 +1,479 @@ > +/* > + * f_iso.c - USB peripheral isochronous source/sink configuration driver > + * This driver is based on David Brownell's f_sourcesink.c. You should add a "Copyright (C) 2009 Tom Leiming" here. Without a copyright, the license can't apply, because it's a grant from the copyright holder. :) > + * > + * 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 > + */ > + > +/* #define VERBOSE_DEBUG */ > + > +#include <linux/kernel.h> > +#include <linux/utsname.h> > +#include <linux/device.h> > + > +#include "g_zero.h" > +#include "gadget_chips.h" > + > + > +#define MULTI (0<<11) Please make the packet parameters be module parameters. For high speed, a 3KB "packet" would imply MULTI=2 (3 packets per uframe) and maxpacket=(3KB/3)=1024. Not all high speed UDCs support high bandwidth like that though. For full speed, lots of hardware has odd restrictions. Like maxpacket of 256 bytes (possibly configurable) or lack of ISO support... To some extent this will be a replacement for the gadgetfs based test program ... which supports both packet size controls and transfer rate parameters, sufficient to drive 24 MByte/sec for high bandwidth transfers. ISTR it also supports the same for interrupt transfers... > + > +/* > + * ISOCHRONOUS SOURCE/SINK FUNCTION ... a primary testing vehicle for > + * USB peripheral controller drivers, it can also be used to evalute > + * performance of usb host controller or usb host controller diver, > + * and can be used to troubleshoot usb host controller driver handily. > + * > + * This just sinks isochronous packets OUT to the peripheral and sources > + * them IN to the host. As such it supports basic functionality and > + * load tests. > + * > + * In terms of control messaging, this supports all the standard requests > + * > + * This is currently packaged as a configuration driver, which can't be > + * combined with other functions to make composite devices. However, it > + * can be combined with other independent configurations. > + */ > +struct f_iso_sourcesink { > + struct usb_function function; > + > + struct usb_ep *in_ep; > + struct usb_ep *out_ep; > +}; > + > +static inline struct f_iso_sourcesink *func_to_iso_ss(struct usb_function *f) > +{ > + return container_of(f, struct f_iso_sourcesink, function); > +} > + > +/*-------------------------------------------------------------------------*/ > + > +static struct usb_interface_descriptor iso_source_sink_intf = { > + .bLength = sizeof iso_source_sink_intf, > + .bDescriptorType = USB_DT_INTERFACE, > + > + .bNumEndpoints = 2, No ... altsetting zero must not have any ISO endpoints. You need to have two altsettings: #0 has no endpoints, and #1 would have the endpoints you list here. You could have *more* altsettings, at different data rates, but for now I'd say "don't bother". > + .bInterfaceClass = USB_CLASS_VENDOR_SPEC, > + /* .iInterface = DYNAMIC */ > +}; > + > +/* full speed support: */ > + > +static struct usb_endpoint_descriptor fs_iso_source_desc = { > + .bLength = USB_DT_ENDPOINT_SIZE, > + .bDescriptorType = USB_DT_ENDPOINT, > + > + .bEndpointAddress = USB_DIR_IN, > + .bmAttributes = USB_ENDPOINT_XFER_ISOC, > + .wMaxPacketSize = __constant_cpu_to_le16(1023) , > + .bInterval = 1, Full speed packet size (1..1023) and interval should be module parameters. For full speed I'd use the same values for both IN and OUT transfers, but not high speed (see below). > +}; > + > +static struct usb_endpoint_descriptor fs_iso_sink_desc = { > + .bLength = USB_DT_ENDPOINT_SIZE, > + .bDescriptorType = USB_DT_ENDPOINT, > + > + .bEndpointAddress = USB_DIR_OUT, > + .bmAttributes = USB_ENDPOINT_XFER_ISOC, > + .wMaxPacketSize = __constant_cpu_to_le16(1023) , > + .bInterval = 1, > +}; > + > +static struct usb_descriptor_header *fs_iso_source_sink_descs[] = { > + (struct usb_descriptor_header *) &iso_source_sink_intf, Two different altsettings required ... > + (struct usb_descriptor_header *) &fs_iso_sink_desc, > + (struct usb_descriptor_header *) &fs_iso_source_desc, > + NULL, > +}; > + > +/* high speed support: */ > +static struct usb_endpoint_descriptor hs_iso_source_desc = { > + .bLength = USB_DT_ENDPOINT_SIZE, > + .bDescriptorType = USB_DT_ENDPOINT, > + .bEndpointAddress = USB_DIR_IN, > + > + .bmAttributes = USB_ENDPOINT_XFER_ISOC, > + .wMaxPacketSize = cpu_to_le16(1024|MULTI) , > + .bInterval = 1, Likewise highspeed packet size (1..3KB) and interval should be module parameters. In this case it'd be important to let them work at different rates. For example, on the Beagle you used to test with, the patch supporting high bandwidth ISO only configures one endpoint with enough buffer space (4KB) to handle a 24 MByte/sec data rate. > +}; > + > +static struct usb_endpoint_descriptor hs_iso_sink_desc = { > + .bLength = USB_DT_ENDPOINT_SIZE, > + .bDescriptorType = USB_DT_ENDPOINT, > + .bEndpointAddress = USB_DIR_OUT, > + > + .bmAttributes = USB_ENDPOINT_XFER_ISOC, > + .wMaxPacketSize = cpu_to_le16(1024|MULTI), > + .bInterval = 1, > +}; > + > +static struct usb_descriptor_header *hs_iso_source_sink_descs[] = { > + (struct usb_descriptor_header *) &iso_source_sink_intf, Two different altsettings required ... > + (struct usb_descriptor_header *) &hs_iso_source_desc, > + (struct usb_descriptor_header *) &hs_iso_sink_desc, > + NULL, > +}; > + > +/* function-specific strings: */ > +static struct usb_string strings_iso_sourcesink[] = { > + [0].s = "iso source and sink data", > + { } /* end of list */ > +}; > + > +static struct usb_gadget_strings stringtab_iso_sourcesink = { > + .language = 0x0409, /* en-us */ > + .strings = strings_iso_sourcesink, > +}; > + > +static struct usb_gadget_strings *iso_sourcesink_strings[] = { > + &stringtab_iso_sourcesink, > + NULL, > +}; > + > +/*-------------------------------------------------------------------------*/ > +static int __init > +iso_sourcesink_bind(struct usb_configuration *c, struct usb_function *f) > +{ > + struct usb_composite_dev *cdev = c->cdev; > + struct f_iso_sourcesink *ss = func_to_iso_ss(f); > + int id; > + > + /* allocate interface ID(s) */ > + id = usb_interface_id(c, f); > + if (id < 0) > + return id; > + iso_source_sink_intf.bInterfaceNumber = id; You'll have to set that for both interfaces. > + > + /* allocate endpoints */ > + ss->in_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_source_desc); Hmm, I seem to recall expecting that high speed endpoints would get configured first here. For ISO, it can matter a lot ... needing 2KB or 3KB of buffer space (for high bandwidth) will be awkward. > + if (!ss->in_ep) { > +autoconf_fail: > + ERROR(cdev, "%s: can't autoconfigure on %s\n", > + f->name, cdev->gadget->name); > + return -ENODEV; > + } > + ss->in_ep->driver_data = cdev; /* claim */ > + > + ss->out_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_sink_desc); > + if (!ss->out_ep) > + goto autoconf_fail; > + ss->out_ep->driver_data = cdev; /* claim */ > + > + /* support high speed hardware */ > + if (gadget_is_dualspeed(c->cdev->gadget)) { > + hs_iso_source_desc.bEndpointAddress = > + fs_iso_source_desc.bEndpointAddress; > + hs_iso_sink_desc.bEndpointAddress = > + fs_iso_sink_desc.bEndpointAddress; > + f->hs_descriptors = hs_iso_source_sink_descs; > + } > + > + DBG(cdev, "%s speed %s: IN/%s, OUT/%s\n", > + gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full", > + f->name, ss->in_ep->name, ss->out_ep->name); For ISO I'd also dump the data rate info: interval and data size for each supported speed and direction. > + return 0; > +} > + > +static void > +iso_sourcesink_unbind(struct usb_configuration *c, struct usb_function *f) > +{ > + kfree(func_to_iso_ss(f)); > +} > + > +static unsigned char iso_count; > +static void iso_reinit_write_data(struct usb_ep *ep, struct usb_request *req) > +{ > + memset(req->buf, iso_count++, req->length); > +} > + > +static void iso_source_sink_complete(struct usb_ep *ep, struct usb_request *req) > +{ > + struct f_iso_sourcesink *ss = ep->driver_data; > + struct usb_composite_dev *cdev = ss->function.config->cdev; > + int status = req->status; > + > + switch (status) { > + > + case 0: /* normal completion? */ You're not looking at the RX data here. So there's no point in the memset. And the TX data isn't going to change either; so there's no point in re-initializing it. Likewise "usbtest" cases 15 (ISO read) and 16 (ISO write) don't touch the ISO data (not entirely a feature) ... so I'd just drop this if/else entirely. > + if (ep == ss->out_ep) > + memset(req->buf, 0x55, req->length); > + else > + iso_reinit_write_data(ep, req); > + break; > + > + /* this endpoint is normally active while we're configured */ > + case -ECONNABORTED: /* hardware forced ep reset */ > + case -ECONNRESET: /* request dequeued */ > + case -ESHUTDOWN: /* disconnect from host */ > + VDBG(cdev, "%s gone (%d), %d/%d\n", ep->name, status, > + req->actual, req->length); > + free_ep_req(ep, req); > + return; > + > + case -EOVERFLOW: /* buffer overrun on read means that > + * we didn't provide a big enough > + * buffer. > + */ > + default: > +#if 1 > + DBG(cdev, "%s complete --> %d, %d/%d\n", ep->name, > + status, req->actual, req->length); > +#endif > + case -EREMOTEIO: /* short read */ > + break; > + } > + > + status = usb_ep_queue(ep, req, GFP_ATOMIC); > + if (status) { > + ERROR(cdev, "kill %s: resubmit %d bytes --> %d\n", > + ep->name, req->length, status); > + usb_ep_set_halt(ep); ISO endpoints can't halt. :) > + /* FIXME recover later ... somehow */ > + } > +} > + > +static int iso_source_sink_start_ep(struct f_iso_sourcesink *ss, bool is_in) > +{ > + struct usb_ep *ep; > + struct usb_request *req; > + int status; > + > + ep = is_in ? ss->in_ep : ss->out_ep; This should loop twice ... hardware typically will support double buffering (ping/pong) but not much more. > + req = alloc_ep_req(ep); > + if (!req) > + return -ENOMEM; > + > + req->complete = iso_source_sink_complete; > + if (is_in) > + iso_reinit_write_data(ep, req); > + else > + memset(req->buf, 0x55, req->length); See above. No point in that if/else for now. > + > + status = usb_ep_queue(ep, req, GFP_ATOMIC); > + if (status) { > + struct usb_composite_dev *cdev; > + > + cdev = ss->function.config->cdev; > + ERROR(cdev, "start %s %s --> %d\n", > + is_in ? "IN" : "OUT", > + ep->name, status); > + free_ep_req(ep, req); > + } > + > + return status; > +} > + > +static void disable_iso_source_sink(struct f_iso_sourcesink *ss) > +{ > + struct usb_composite_dev *cdev; > + > + cdev = ss->function.config->cdev; > + disable_endpoints(cdev, ss->in_ep, ss->out_ep); > + VDBG(cdev, "%s disabled\n", ss->function.name); > +} > + > +static int > +enable_iso_source_sink(struct usb_composite_dev *cdev, \ No need for that backslash; it's just normal whitespace. > + struct f_iso_sourcesink *ss) > +{ > + int result = 0; > + const struct usb_endpoint_descriptor *src, *sink; > + struct usb_ep *ep; > + > + src = ep_choose(cdev->gadget, &hs_iso_source_desc, &fs_iso_source_desc); > + sink = ep_choose(cdev->gadget, &hs_iso_sink_desc, &fs_iso_sink_desc); > + > + /* one endpoint writes (sources) zeroes IN (to the host) */ > + ep = ss->in_ep; > + result = usb_ep_enable(ep, src); > + if (result < 0) > + return result; > + > + ep->driver_data = ss; > + > + result = iso_source_sink_start_ep(ss, true); > + if (result < 0) { > +fail: > + ep = ss->in_ep; > + usb_ep_disable(ep); > + ep->driver_data = NULL; > + return result; > + } > + > + /* one endpoint reads (sinks) anything OUT (from the host) */ > + ep = ss->out_ep; > + result = usb_ep_enable(ep, sink); > + if (result < 0) > + goto fail; > + ep->driver_data = ss; > + > + result = iso_source_sink_start_ep(ss, false); > + if (result < 0) { > + usb_ep_disable(ep); > + ep->driver_data = NULL; > + goto fail; > + } > + > + DBG(cdev, "%s enabled\n", ss->function.name); > + return result; > +} > + > +static int iso_sourcesink_set_alt(struct usb_function *f, > + unsigned intf, unsigned alt) > +{ > + struct f_iso_sourcesink *ss = func_to_iso_ss(f); > + struct usb_composite_dev *cdev = f->config->cdev; > + > + /* we know alt is zero */ > + if (ss->in_ep->driver_data) > + disable_iso_source_sink(ss); > + return enable_iso_source_sink(cdev, ss); This is wrong. See above ... as described in the USB 2.0 spec, ISO endpoints don't belong in altsetting zero. So instead: switch (alt) { case 0: return 0; case 1: return enable(...); default: return -EINVAL; } > +} > + > +static void iso_sourcesink_disable(struct usb_function *f) > +{ > + struct f_iso_sourcesink *ss = func_to_iso_ss(f); > + > + disable_iso_source_sink(ss); > +} > + > + > +/*-------------------------------------------------------------------------*/ > + > +static int __init iso_sourcesink_bind_config(struct usb_configuration *c) > +{ > + struct f_iso_sourcesink *ss; > + int status; > + > + ss = kzalloc(sizeof *ss, GFP_KERNEL); > + if (!ss) > + return -ENOMEM; > + > + ss->function.name = "iso source/sink"; > + ss->function.descriptors = fs_iso_source_sink_descs; > + ss->function.bind = iso_sourcesink_bind; > + ss->function.unbind = iso_sourcesink_unbind; > + ss->function.set_alt = iso_sourcesink_set_alt; > + ss->function.disable = iso_sourcesink_disable; > + > + status = usb_add_function(c, &ss->function); > + if (status) > + kfree(ss); > + return status; > +} > + > +static int iso_sourcesink_setup(struct usb_configuration *c, > + const struct usb_ctrlrequest *ctrl) > +{ > + struct usb_request *req = c->cdev->req; > + int value = -EOPNOTSUPP; > + u16 w_index = le16_to_cpu(ctrl->wIndex); > + u16 w_value = le16_to_cpu(ctrl->wValue); > + u16 w_length = le16_to_cpu(ctrl->wLength); > + > + /* composite driver infrastructure handles everything except > + * the two control test requests. > + */ I'd just strike this entire setup() routine ... it's enough to have it in the bulk source/sink configuration. No point in cloning that code here, and possibly letting bugs grow. > + switch (ctrl->bRequest) { > + > + /* > + * These are the same vendor-specific requests supported by > + * Intel's USB 2.0 compliance test devices. We exceed that > + * device spec by allowing multiple-packet requests. > + * > + * NOTE: the Control-OUT data stays in req->buf ... better > + * would be copying it into a scratch buffer, so that other > + * requests may safely intervene. > + */ > + case 0x5b: /* control WRITE test -- fill the buffer */ > + if (ctrl->bRequestType != (USB_DIR_OUT|USB_TYPE_VENDOR)) > + goto unknown; > + if (w_value || w_index) > + break; > + /* just read that many bytes into the buffer */ > + if (w_length > req->length) > + break; > + value = w_length; > + break; > + case 0x5c: /* control READ test -- return the buffer */ > + if (ctrl->bRequestType != (USB_DIR_IN|USB_TYPE_VENDOR)) > + goto unknown; > + if (w_value || w_index) > + break; > + /* expect those bytes are still in the buffer; send back */ > + if (w_length > req->length) > + break; > + value = w_length; > + break; > + > + default: > +unknown: > + VDBG(c->cdev, > + "unknown control req%02x.%02x v%04x i%04x l%d\n", > + ctrl->bRequestType, ctrl->bRequest, > + w_value, w_index, w_length); > + } > + > + /* respond with data transfer or status phase? */ > + if (value >= 0) { > + VDBG(c->cdev, "source/sink req%02x.%02x v%04x i%04x l%d\n", > + ctrl->bRequestType, ctrl->bRequest, > + w_value, w_index, w_length); > + req->zero = 0; > + req->length = value; > + value = usb_ep_queue(c->cdev->gadget->ep0, req, GFP_ATOMIC); > + if (value < 0) > + ERROR(c->cdev, "source/sinkc response, err %d\n", > + value); > + } > + > + /* device either stalls (value < 0) or reports success */ > + return value; > +} > + > +static struct usb_configuration iso_sourcesink_driver = { > + .label = "iso source/sink", > + .strings = iso_sourcesink_strings, > + .bind = iso_sourcesink_bind_config, > + .setup = iso_sourcesink_setup, > + .bConfigurationValue = 4, > + .bmAttributes = USB_CONFIG_ATT_SELFPOWER, > + /* .iConfiguration = DYNAMIC */ > +}; > + > +/** > + * sourcesink_add - add a source/sink testing configuration to a device > + * @cdev: the device to support the configuration > + */ > +int __init iso_sourcesink_add(struct usb_composite_dev *cdev) > +{ > + int id; > + > + /* allocate string ID(s) */ > + id = usb_string_id(cdev); > + if (id < 0) > + return id; > + strings_iso_sourcesink[0].id = id; > + > + iso_source_sink_intf.iInterface = id; > + iso_sourcesink_driver.iConfiguration = id; > + > + /* support OTG systems */ > + if (gadget_is_otg(cdev->gadget)) { > + iso_sourcesink_driver.descriptors = otg_desc; > + iso_sourcesink_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP; > + } > + > + return usb_add_config(cdev, &iso_sourcesink_driver); > +} > diff --git a/drivers/usb/gadget/zero.c b/drivers/usb/gadget/zero.c > index 20614dc..db2a7b5 100644 > --- a/drivers/usb/gadget/zero.c > +++ b/drivers/usb/gadget/zero.c > @@ -71,6 +71,7 @@ > #include "config.c" > #include "epautoconf.c" > > +#include "f_iso.c" > #include "f_sourcesink.c" > #include "f_loopback.c" > > @@ -92,6 +93,14 @@ module_param(buflen, uint, 0); > static int loopdefault = 0; > module_param(loopdefault, bool, S_IRUGO|S_IWUSR); > > +/* > + * Normally the "iso" configuration is third (index 2) so > + * it's not the default. Here's where to change that order, to > + * work better with hosts where config changes are problematic or > + * controllers (like original superh) that only support one config. > + */ > +static int isodefault; > +module_param(isodefault, bool, S_IRUGO|S_IWUSR); Please preserve the blank line here... > /*-------------------------------------------------------------------------*/ > > /* Thanks to NetChip Technologies for donating this product ID. > @@ -118,7 +127,7 @@ static struct usb_device_descriptor device_desc = { > > .idVendor = cpu_to_le16(DRIVER_VENDOR_NUM), > .idProduct = cpu_to_le16(DRIVER_PRODUCT_NUM), > - .bNumConfigurations = 2, > + .bNumConfigurations = 3, > }; > > #ifdef CONFIG_USB_OTG > @@ -244,12 +253,22 @@ static int __init zero_bind(struct usb_composite_dev *cdev) > */ > if (loopdefault) { > loopback_add(cdev); > - if (!gadget_is_sh(gadget)) > + if (!gadget_is_sh(gadget)) { > sourcesink_add(cdev); > + iso_sourcesink_add(cdev); You shouldn't assume all hardware can support ISO properly In particular, if gadget_is_pxa() don't try to enable it, and report an error if someone tries to set isodefault ... it doesn't support altsettings other than zero (sigh). And may not support the configuration number you use... For other controllers, the lack of support for ISO should become apparent politely when iso_sourcesink_add() fails. Although ... none of these calls actually check the return values from those add() calls; sigh. > + } > + } else if (isodefault) { > + iso_sourcesink_add(cdev); > + if (!gadget_is_sh(gadget)) { > + sourcesink_add(cdev); > + loopback_add(cdev); > + } > } else { > sourcesink_add(cdev); > - if (!gadget_is_sh(gadget)) > + if (!gadget_is_sh(gadget)) { > loopback_add(cdev); > + iso_sourcesink_add(cdev); > + } > } > > gcnum = usb_gadget_controller_number(gadget); > -- > 1.6.0.GIT > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html