On 06/25/2013 09:45 AM, Alan Stern wrote:
On Mon, 24 Jun 2013, Daniel Santos wrote:
I submit an out URB (to rx my response)
No, an OUT URB transfers data _out_ from the computer to the device.
The response goes from the device _in_ to the computer.
I sure do appreciate this because I was initially confused about this
and am now re-confused. I presumed that "out" meant "out of the
computer and into the USB device".
That is exactly what it means.
However, I indeed have the variable
labeled "out" that I am using to receive responses and visa versa. I
have this little struct I use for both the IN and OUT URBs in my device
struct:
struct mcp2210_endpoint_proxy {
const struct usb_host_endpoint *ep;
struct urb *urb;
struct mcp2210_msg *buffer;
/* FIXME: inefficient */
u8 submitted:1;
u8 completed:1;
u8 kill:1;
u8 retry_count:4;
u8 is_dir_in:1;
};
/* then in my device struct: */
struct mcp2210_device {
struct usb_device *udev;
struct usb_interface *intf;
struct spi_master *spi_master;
struct gpio_chip *gpio_chip;
...
struct mcp2210_endpoint_proxy in;
struct mcp2210_endpoint_proxy out;
...
};
I initialize them both with a single function and it works! This is
what caused me to believe that the URB directions were described from
the USB device standpoint. (please pardon my mailer's mangling of
whitespace)
static __always_inline int init_endpoint(struct mcp2210_device *dev,
const struct usb_host_endpoint *ep)
{
int is_dir_in = !!usb_endpoint_dir_in(&ep->desc);
struct mcp2210_endpoint_proxy *dest;
unsigned int pipe;
usb_complete_t complete_fn;
struct urb *urb;
if (is_dir_in) {
dest = &dev->in;
pipe = usb_sndintpipe(dev->udev, ep->desc.bEndpointAddress);
complete_fn = urb_complete_in;
} else {
dest = &dev->out;
pipe = usb_rcvintpipe(dev->udev, ep->desc.bEndpointAddress);
complete_fn = urb_complete_out;
}
dest->is_dir_in = is_dir_in;
So you're assigning an OUT pipe value to the "in" proxy and vice versa.
But the OUT pipe uses the endpoint number of the IN endpoint, and vice
versa.
This might work if the two endpoints have the same number (for example,
if the OUT endpoint's address is 0x02 and the IN endpoint's address is
0x82). However, it certainly isn't what you should be doing!
I guess we should add a check to the debugging code, to make sure the
pipe's direction matches the endpoint's direction.
Very interesting, since the inverse doesn't work -- well at least on the
Pi. That's making me suspect that it wasn't working because I simply
encountered another bug in the USB host controller driver -- that's
very, very, very, weird. I'll re-test this on my workstation.
if (unlikely(dest->ep)) {
mcp2210_warn("Unexpected: more than one interrupt %s endpoint "
"discovered, ingoring them\n",
urb_dir_str[is_dir_in]);
return 0;
}
urb = usb_alloc_urb(0, GFP_KERNEL);
if (unlikely(!urb)) {
mcp2210_err("Failed to alloc %s URB\n", urb_dir_str[is_dir_in]);
return -ENOMEM;
}
dest->buffer = usb_alloc_coherent(dev->udev, 64, GFP_KERNEL,
&urb->transfer_dma);
Most likely you don't need to use a coherent buffer.
I've never messed with DMA before, so I was just following some
examples. If the USB host controller can use that to be faster, it's
definitely the preference. The MCP2210 can potentially transfer 20
Mbit/sec to an SPI device (in theory at least -- I'ld like to see how
that works with a 64 byte buffer :), and there's a little overhead in
it's protocol as well.
if (unlikely(!dest->buffer)) {
usb_free_urb(urb);
mcp2210_err("Failed to alloc %s URB DMA buffer\n",
urb_dir_str[is_dir_in]);
return -ENOMEM;
}
dest->ep = ep;
dest->urb = urb;
usb_fill_int_urb(urb,
dev->udev,
pipe,
dest->buffer,
64,
complete_fn,
dev,
ep->desc.bInterval);
urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
return 0;
}
This is how I use it in my probe function:
int mcp2210_probe(struct usb_interface *intf, const struct usb_device_id
*id)
{
struct usb_device *udev = interface_to_usbdev(intf);
struct usb_host_interface *intf_desc = intf->cur_altsetting;
struct mcp2210_device *dev;
struct usb_host_endpoint *ep, *ep_end;
int ret = -ENODEV;
int i;
printk("mcp2210_probe\n");
...
/* Set up interrupt endpoint information. */
ep_end = &intf_desc->endpoint[intf_desc->desc.bNumEndpoints];
for (ep = intf_desc->endpoint; ep != ep_end; ++ep) {
if (!usb_endpoint_xfer_int(&ep->desc))
continue;
if (init_endpoint(dev, ep))
goto error0;
}
if (unlikely(!dev->in.ep || !dev->out.ep)) {
mcp2210_err("could not find in and/or out interrupt endpoints");
goto error0;
}
...
}
I don't understand why this works and the previous didn't in light of
your comment (as well as my original assumptions). Can you see any
flaws in this? I've used !!usb_endpoint_dir_in(&ep->desc) to get a one
or zero for rather or not I have the IN endpoint since I store this in a
u8:1 bitfield.
usb_endpoint_dir_in() always returns a 0 or 1 value, so the !! isn't
needed.
Well, at least usb_endpoint_dir_in() is an inline, so my superfluous !!
will get compiled-out. At least I've had somebody look at that part of
the code and verify that I'm sniffing IN and OUT correctly.
Gotcha here! My thought on submitting them both at the same time is to
(hopefully, theoretically, etc.) speed up the transaction, but also to
give me more opportunities to allocate memory from the GFP_KERNEL pool
(I'm not really sure how much usb_submit_urb needs to allocate).
Not much memory is needed. Anyway, submitting them both should not
cause any problems.
Oh, good. I had that on "tweak list" of things to improve once I had
actually got it working correctly. I wont worry about that too much then.
Daniel
--
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
