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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
