On Wed, May 03, 2017 at 01:32:28PM +0300, Laurent Pinchart wrote:
> >
> > There was no one reported this problem before, but from the description,
> > it seems an IC issue which is triggered at high loading memory bus,
> > controller may not get time to visit memory at limited time.
>
> That's my guess too. I was expecting the USB controller's bus master interface
> to get stalled but eventually perform the access (or retry it, I'm not sure
> what kind of bus it sits on), but there might be a hardware bug that messes up
> the controller's state machine. I won't rule out the possibility of a software
> issue yet, it might be possible to detect this condition and retry the
> transfer.
I am not sure if it can be recovered, you can call ->fifo_flush, and
->ep_disable and ->ep_enable if it returns -ETIME, and re-submit this
request.
>
> > At Xilinx Zynq, its tx buffer is small, and less than 512 bytes
> > (84bc70f94d81, "usb: chipidea: add xilinx zynq platform data"), and your
> > throughput may be > (512 * 3) bytes/SoF, you can't use non-stream mode by
> > reducing max packet size.
>
> My throughput is actually 1*1024 bytes / SOF.
>
>From previous discussion, the tx fifo size is 341.33 bytes for xilinx
zynq, you can set max packet size as 341 and mult as 3, then you can
transfer 1023 bytes / SoF for non-stream mode, assumed the non-stream
mode can fix your problem.
diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c
index d162cc0..7229f2d 100644
--- a/drivers/usb/chipidea/ci_hdrc_usb2.c
+++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
@@ -33,6 +33,7 @@ static const struct ci_hdrc_platform_data ci_default_pdata = {
static struct ci_hdrc_platform_data ci_zynq_pdata = {
.capoffset = DEF_CAPOFFSET,
+ .flags = CI_HDRC_DISABLE_STREAMING,
};
> > I think you may observe many under-run at bus analyzer during the test.
>
> I'll try to get this checked (as I don't have a USB analyzer here).
>
> > As a workaround, you may try to do below things:
> >
> > 1. Link more TDs before the UVC run
>
> Do you mean I should have more requests queued to avoid hitting the
> software/hardware race that the loop handles ?
Yes, you can comment out that code if you have more pending dTDs.
>
> > 2. Comment out the code, you are stuck in, it is only useful for protect
> > last td status which is handling or will be handled soon by hardware.
> >
> > /*
> > do {
> > hw_write(ci, OP_USBCMD, USBCMD_ATDTW, USBCMD_ATDTW);
> > tmp_stat = hw_read(ci, OP_ENDPTSTAT, BIT(n));
> > } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW));
> > hw_write(ci, OP_USBCMD, USBCMD_ATDTW, 0);
> > if (tmp_stat)
> > goto done;
> > */
>
> I'm not familiar with this driver, but if I understand things correctly, the
> code (and the "TD tripwire" hardware feature) is here to handle a race
> condition where the hardware could look at the last TD's next pointer at the
> same time it gets updated by the software. If I comment the code out, won't
> the endpoint will be primed unconditionally, even if it's currently executing
> transfers ? Won't that be a problem ?
Try below code, I have tested it using qmult=20 for NCM.
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index d68b125..dbef51a 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -485,13 +485,19 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq)
wmb();
if (hw_read(ci, OP_ENDPTPRIME, BIT(n)))
goto done;
- do {
- hw_write(ci, OP_USBCMD, USBCMD_ATDTW, USBCMD_ATDTW);
- tmp_stat = hw_read(ci, OP_ENDPTSTAT, BIT(n));
- } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW));
- hw_write(ci, OP_USBCMD, USBCMD_ATDTW, 0);
- if (tmp_stat)
- goto done;
+
+ if (ci->ep0out == hwep || ci->ep0in == hwep) {
+ do {
+ hw_write(ci, OP_USBCMD, USBCMD_ATDTW, USBCMD_ATDTW);
+ tmp_stat = hw_read(ci, OP_ENDPTSTAT, BIT(n));
+ } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW));
+ hw_write(ci, OP_USBCMD, USBCMD_ATDTW, 0);
+ if (tmp_stat)
+ goto done;
+ } else {
+ if(hw_read(ci, OP_ENDPTSTAT, BIT(n)))
+ goto done;
+ }
}
/* QH configuration */
>
> > 3. If it can let your test run more time, try change code like below:
> >
> > if (remaining_td_num > 2)
> > don't do hardware check;
> > else
> > do hardware check.
>
> I'll test that, but I assume I'm hitting the problem when the number of
> remaining TDs is exactly one.
Let's not meet this situation, just queue before TDs before running.
>
> > Besides, I can try your test if you could show me the detail test steps.
>
> I need to create an easy test case, at the moment it's a complex application
> and not every part of it can be published. I'll try to provide an open-source
> test case implementation.
>
I only have nxp/fsl board which imx6 series SoC is on it.
--
Best Regards,
Peter Chen
--
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
