On Fri, Mar 08, 2013 at 11:54:41AM +0200, Felipe Balbi wrote:
> On Fri, Mar 08, 2013 at 05:38:53PM +0800, Peter Chen wrote:
> > On Thu, Mar 07, 2013 at 09:58:52AM +0100, Peter Bestler wrote:
> > > Hi,
> > >
> > > We try to get our device (based on p2020rdb) usb 2.0 compliant. We ran the usb30cv test suite (version 1.0.1.2, chapter 9 tests for usb 2.0 devices) on win7 with g_zero and g_serial. We access the device via an usb 3.0 hcd from intel. Our device runs the 3.2.35-rt52 kernel. I spotted the following problem with ch9setaddress in fsl_udc_core.c.
> > >
> > > All tests passed on single execution. At running it in batch mode the first test after switching from default to adressed state failed. The subsequent tests passed. It doesn't depend on the selected tests, only on the switch over from default to adressed. It fails with our custom gadgetfs driver too. Another device with Intel PXA25x and the same setup passes all tests.
> > >
> > > With the total phase usbsniffer i spotted the following behavior:
> > > The test issues a setAddress and receives an ACK, 125 us afterwards the host issues a getDescriptor (setuptx) request, which fails at setuptx. The USB sniffer reports invalid PID sequence.
> > >
> > > For debugging purpose I delayed the dma_map_single in ep0_prime_status (which does the ACK, right?) by 2 milliseconds. And all tests are passing in batch mode. It's quite the same sequence on the bus, but between setAdress and its ACK is a delay of 3 ms.
> > >
> > > I think delaying the ACK in set request isn't the way to go. I think we set the address too early; we have to wait until the status phase of the set address has finished. My understanding is that the device has to respond to address 0 until the complete status phase of setAddress is passed (is this correct).
> > >
> > > Has anybody ran the usb30cv on fsl_udc recently?
> > >
> > > After applying the patch f79a60b8785 none of the tests run anymore. Did I miss anything here? How can we fix this issue?
> > >
> > > Best regards
> > >
> > > Peter
> > >
> > > --- Patch for debugging ---
> > > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> > > index 55abfb6..fdbfd25 100644
> > > --- a/drivers/usb/gadget/fsl_udc_core.c
> > > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > > @@ -1285,6 +1285,7 @@ static int ep0_prime_status(struct fsl_udc *udc, int direction)
> > > req->req.complete = NULL;
> > > req->dtd_count = 0;
> > >
> > > + udelay(2000);
> > > req->req.dma = dma_map_single(ep->udc->gadget.dev.parent,
> > > req->req.buf, req->req.length,
> > > ep_is_in(ep) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
> > >
> > >
> > >
> > >
> >
> > Please check your datasheet if your controller has USBADRA bit
> > at DEVICEADDR, if it exists, it means your controller supports
> > advance store device address function. Please have a try for
> > below change, if it fixes your problem, please give a tested-by,
> > then, I can make change for chipidea and fsl-core driver.
> >
> > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> > index 04d5fef..a75c884 100644
> > --- a/drivers/usb/gadget/fsl_udc_core.c
> > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > @@ -1335,10 +1335,14 @@ static void udc_reset_ep_queue(struct fsl_udc *udc, u8 pipe)
> > */
> > static void ch9setaddress(struct fsl_udc *udc, u16 value, u16 index, u16 length)
> > {
> > - /* Save the new address to device struct */
> > udc->device_address = (u8) value;
> > + /* Set the new address */
> > + fsl_writel(((u32)value << USB_DEVICE_ADDRESS_BIT_POS)|
> > + (1 << USB_DEVICE_ADDRESS_BIT_POS),
>
> do you mean:
>
> (u32) ((value << USB_DEVICE_ADDRESS_BIT_POS) |
> (1 << USB_DEVICE_ADDRESS_BIT_POS))
>
> ??
>
> Also, why do you need the extra (1 << USB_DEVICE_ADDRESS_BIT_POS) ?
>
> You'd be making all addresses odd, no ?
Sorry, my wrong, should be below:
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index 04d5fef..b0e78e6 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -1335,10 +1335,14 @@ static void udc_reset_ep_queue(struct fsl_udc *udc, u8 pipe)
*/
static void ch9setaddress(struct fsl_udc *udc, u16 value, u16 index, u16 length)
{
- /* Save the new address to device struct */
udc->device_address = (u8) value;
+ /* Set the new address */
+ fsl_writel(((u32)value << USB_DEVICE_ADDRESS_BIT_POS)|
+ (1 << USB_DEVICE_ADDRESS_ADV_BIT_POS),
+ &dr_regs->deviceaddr);
/* Update usb state */
udc->usb_state = USB_STATE_ADDRESS;
+
/* Status phase */
if (ep0_prime_status(udc, EP_DIR_IN))
ep0stall(udc);
@@ -1539,13 +1543,6 @@ static void setup_received_irq(struct fsl_udc *udc,
static void ep0_req_complete(struct fsl_udc *udc, struct fsl_ep *ep0,
struct fsl_req *req)
{
- if (udc->usb_state == USB_STATE_ADDRESS) {
- /* Set the new address */
- u32 new_address = (u32) udc->device_address;
- fsl_writel(new_address << USB_DEVICE_ADDRESS_BIT_POS,
- &dr_regs->deviceaddr);
- }
-
done(ep0, req, 0);
switch (udc->ep0_state) {
diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
index c6703bb..bf515d1 100644
--- a/drivers/usb/gadget/fsl_usb2_udc.h
+++ b/drivers/usb/gadget/fsl_usb2_udc.h
@@ -183,6 +183,7 @@ struct usb_sys_interface {
/* Device Address bit masks */
#define USB_DEVICE_ADDRESS_MASK 0xFE000000
+#define USB_DEVICE_ADDRESS_ADV_BIT_POS 24
#define USB_DEVICE_ADDRESS_BIT_POS 25
/* endpoint list address bit masks */
>
> --
> balbi
--
Best Regards,
Peter Chen
Hi,
I tested your patch, Peter and it works fine. All tests of the usbcv30
are passing with my setup. Thank you so far.
So you get a tested-by: peter.bestler@xxxxxxxxxx
In my opinion we do it in the wrong order; we set the address before the status stage
completes. But isn't the setAddress request specified that we should
set it after the status stage completes, like it was before
(http://www.beyondlogic.org/usbnutshell/usb6.shtml)
But we should return a nack as long the address isn't written to the silicon
and the complete setAddress request is processed.
In my opinion the usb 3 cv behaves correct after getting the ack it continues
with next request (ack indicates that device is ready to get next request).
> > > After applying the patch f79a60b8785 none of the tests run anymore.
Currently I have no clue why it does not work, maybe I should do further investigations
about it.
Best Regards.
Peter
--
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
