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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2993 bytes --]


Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
> It seems the yesterday's reply from nxp email account is blocked by ML.
> Send it again.
>
>>
>> Peter Chen <peter.chen@nxp.com> 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.
>>
>
> 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?

>> 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.

> 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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]