RE: [PATCH 1/1] usb: cdns3: ep0: delete the redundant status stage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Your changes looks like:
-			cdns3_ep0_complete_setup(priv_dev, 0, 1);
-			/**
-			 *  Little delay to give the controller some time
-			 * for sending status stage.
-			 * This time should be less then 3ms.
-			 */
-			mdelay(1);
 			cdns3_set_register_bit(&priv_dev->regs->usb_cmd,
 					       USB_CMD_STMODE |
 					       USB_STS_TMODE_SEL(tmode - 1));

I'm not sure whether the first line is necessary in this place  " cdns3_ep0_complete_setup(priv_dev, 0, 1);" . 
Maybe it worked accidentally on my board. 

I will confirm it with someone from RTL team, but as I remember the status stage for Test Mode is 
send automatically by controller. mdelay(1) was added to give controller some time for sending 
Status Stage. Status Stage should be send before driver send appropriate Test Mode in
usb_cmd register. If you remove mdelay it can works accidentally on your board. 

Currently I don't have access for testing board so I can't recreate this test again. 

>
>On 20-05-15 12:35:32, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Peter Chen <hzpeterchen@xxxxxxxxx> writes:
>> > It seems the yesterday's reply from nxp email account is blocked by ML.
>> > Send it again.
>> >
>> >>
>> >> Peter Chen <peter.chen@xxxxxxx> writes:
>> >> > Each setup stage will prepare status stage at cdns3_ep0_setup_phase,
>> >>
>> >> care to explain this a little better? The controller itself will produce
>> >> the status stage?
>> >>
>> >
>> > Unlike the other controllers, the CDNS3 does not need to prepare TD
>> > for status stage,
>> > it only needs to write register bits EP_CMD.REQ_CMPL to tell the
>> > controller the request
>> > service is completed, and the controller itself will send ACK answer
>> > for the Status Stage after that.
>> > The code sequence like below:
>> >
>> > cdns3_ep0_setup_phase -> cdns3_ep0_complete_setup ->
>> >             writel((send_erdy ? EP_CMD_ERDY : 0) | EP_CMD_REQ_CMPL,
>> >                    &priv_dev->regs->ep_cmd);
>>
>> got it.
>>
>> >> Usually, the way this works is that SETUP stage must be *always*
>> >> prepared by the SW while STATUS stage is prepared on-demand, after we
>> >> get an interrupt from the controller.
>> >>
>> >> Also, I see a possible bug in cdns3_ep0_setup_phase():
>> >>
>> >>         if (result < 0)
>> >>                 cdns3_ep0_complete_setup(priv_dev, 1, 1);
>> >>         else if (priv_dev->ep0_stage == CDNS3_STATUS_STAGE)
>> >>                 cdns3_ep0_complete_setup(priv_dev, 0, 1);
>> >>
>> >> What happens here if result is 0 but ep0_state != CNDS3_STATUS_STAGE?
>> >> Seems like you should have a "stall and restart" somewhere here as a
>> >> default fallback.

When this case when happen ?

BTW.  I see one issue in cdns3_ep0_complete_setup. 

The last one in this function is incorrect: 
		cdns3_allow_enable_l1(priv_dev, 1);
should be: 
		/* Resume controller before arming transfer. */
		__cdns3_gadget_wakeup(priv_dev);

Regards,
Pawel,

>> >>
>> >
>> > At cdns3_ep0_setup_phase, the status will be CDNS3_DATA_STAGE
>> > or CDNS3_STATUS_STAGE according to if there is a data stage.
>> > If there is a data stage, it will inform of controller ACKing the status
>> > stage after data stage has finished, it is at: ep0.c,
>> > cdns3_transfer_completed->cdns3_ep0_complete_setup
>> >
>> > But I don't know why it needs to send ERDY for ep0 transfer without
>> > data stage, but do need for ep0 transfer with data stage. Maybe Pawel
>> > could explain it. At spec, it only says below for ERDY:
>>
>> Would be good, indeed. Pawel?
>>
>
>Pawel seems not at ML about 2-3 months.
>
>> >> Is this backed by documentation or is this something that just happens
>> >> to work? Pawell, can you confirm that this is the correct programming
>> >> model?
>> >>
>> >
>> > No documentation, maybe Pawel could confirm with designer.
>>
>> yeah, Pawel?
>>
>> >> Is this working against USBCV? What about LeCroy's compliance suite?
>> >>
>> >
>> > For NXP USB certification flow:
>> >
>> > The test mode is only used for USB2 electrical test (eg, Eye diagram),
>> > The HSETT tool is used for device mode which will send command
>> > from Windows PC to let device enter test mode.
>> >
>> > USBCV is used to test protocol level, like USB CH9, Mass Storage protocol
>> > etc.
>>
>> Entering test modes is part of chapter9 tests, IIRC.
>
>No, test mode is only used for electrical signal test, the communication
>between device and host will be stopped once the controller enters test
>mode.
>
>>
>> > For Lecroy's compliance suite, we usually use it for Link test for
>> > USB3.
>>
>> You need to run the HS compliance suite ;-) USB3 devices must still
>> comply with HS/FS/LS.
>
>No HS compliance suite. Lecroy Compliance Suite are only used for
>link test and PD test. I attached the test items for compliance test.
>
>
>--
>
>Thanks,
>Peter Chen




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux