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

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

[Peter Chen]

Each setup stage will prepare status stage at cdns3_ep0_setup_phase,
it doesn't need to add extra status stage for test mode handling,
otherwise, the controller can't enter the test mode. Through the Lecroy
bus analyzer log, the controller will always wait status stage
even it is prepared by software later than the test mode is set
by software. If we comment out the status stage at cdns3_ep0_setup_phase,
the controller will not enter test mode even the test mode is set
beforehand.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/ep0.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
index e71240b386b4..82645a2a0f52 100644
--- a/drivers/usb/cdns3/ep0.c
+++ b/drivers/usb/cdns3/ep0.c
@@ -332,13 +332,6 @@ static int cdns3_ep0_feature_handle_device(struct cdns3_device *priv_dev,
 		case TEST_K:
 		case TEST_SE0_NAK:
 		case TEST_PACKET:
-			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));
-- 
2.17.1

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

[Felipe Balbi]

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


Hi,

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?

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.

> it doesn't need to add extra status stage for test mode handling,
> otherwise, the controller can't enter the test mode. Through the Lecroy
> bus analyzer log, the controller will always wait status stage
> even it is prepared by software later than the test mode is set
> by software. If we comment out the status stage at cdns3_ep0_setup_phase,

It seems that what you're trying to say here is that the controller will
enter test mode only after STATUS stage completes even though SW has
already enabled the relevant bits in the register. Is that a correct
read of your sentence?

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?

The way this usually works is that SW must ensure that Test Mode is only
entered after STATUS stage has completed. If this controller works
differently, then we should update the comment (rather than removing it)
and put a reference to documentation section which describes this very
fact.

> the controller will not enter test mode even the test mode is set
> beforehand.

Sorry for being skeptical, but thinking from a HW design perspective,
this would mean that HW would latch Test Mode bits and only, actually,
operate on them after completion of STATUS stage. This means that HW
should have an internal "status_stage_completed" flag which gets ANDed
with another "must_enter_test_mode" internal flag, then only if both
conditions are true, will HW read back Test Mode number from the
register file and enter correct test mode.

Is this working against USBCV? What about LeCroy's compliance suite?

cheers

-- 
balbi

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

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

[Felipe Balbi]

[-- 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 --]

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

[Peter Chen]

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

On 20-05-15 12:35:32, Felipe Balbi wrote:
> 
> 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?
> 

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

[-- Attachment #2: test items.PNG --]
[-- Type: image/png, Size: 10967 bytes --]

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

[Pawel Laszczak]

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

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

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

[Peter Chen]

On 20-05-18 04:36:51, Pawel Laszczak wrote:
> 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.

Pawel, would you please confirm it with your design team? I tried to
comment out cdns3_ep0_complete_setup at cdns3_ep0_setup_phase for test
mode case, the test mode can't work. It means it still needs software
setting to notify controller to prepare status stage.

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

If without cdns3_ep0_complete_setup(priv_dev, 0, 1) before mdelay, how
the controller knows it needs to prepare status stage, the test mode
setting code is behind than it.

The focus is: my board can't enter test mode if status stage is
prepared before set test mode, it is strange. At other controller,
eg, chipidea, we set test mode after receiving the status stage
completion interrupt, but at cdns3, there is no status stage completion
interrupt.

> 
> 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@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.
> 
> 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);
> 

Would you please send a patch to fix it?

Peter

-- 

Thanks,
Peter Chen

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

[Pawel Laszczak]

>
>On 20-05-18 04:36:51, Pawel Laszczak wrote:
>> 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.
>
>Pawel, would you please confirm it with your design team? I tried to
>comment out cdns3_ep0_complete_setup at cdns3_ep0_setup_phase for test
>mode case, the test mode can't work. It means it still needs software
>setting to notify controller to prepare status stage.

Sorry. I got something wrong. You have right.  Driver must prepare Status Stage itself.
It's means that origin code is really incorrect and driver sends the second Status Stage twice.

Hardware team confirm also that setting Test Mode in usb_cmd can be done before 
sending Status Stage. Test Mode should start automatically after sending Status Stage 
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.
>
>If without cdns3_ep0_complete_setup(priv_dev, 0, 1) before mdelay, how
>the controller knows it needs to prepare status stage, the test mode
>setting code is behind than it.
>
>The focus is: my board can't enter test mode if status stage is
>prepared before set test mode, it is strange. At other controller,
>eg, chipidea, we set test mode after receiving the status stage
>completion interrupt, but at cdns3, there is no status stage completion
>interrupt.
>
>>
>> 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@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.
>>
>> 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);
>>
>
>Would you please send a patch to fix it?
>
>Peter
>
>--
>
>Thanks,
>Peter Chen

Pawel

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

[Peter Chen]

 
> >Pawel, would you please confirm it with your design team? I tried to
> >comment out cdns3_ep0_complete_setup at cdns3_ep0_setup_phase for test
> >mode case, the test mode can't work. It means it still needs software
> >setting to notify controller to prepare status stage.
> 
> Sorry. I got something wrong. You have right.  Driver must prepare Status Stage
> itself.
> It's means that origin code is really incorrect and driver sends the second Status
> Stage twice.
> 
> Hardware team confirm also that setting Test Mode in usb_cmd can be done before
> sending Status Stage. Test Mode should start automatically after sending Status
> Stage by controller.
> 
> 

Thanks, Pawel.

If this patch is ok, could you please add your reviewed-by?

Peter

> >
> >> 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.
> >
> >If without cdns3_ep0_complete_setup(priv_dev, 0, 1) before mdelay, how
> >the controller knows it needs to prepare status stage, the test mode
> >setting code is behind than it.
> >
> >The focus is: my board can't enter test mode if status stage is
> >prepared before set test mode, it is strange. At other controller, eg,
> >chipidea, we set test mode after receiving the status stage completion
> >interrupt, but at cdns3, there is no status stage completion interrupt.
> >
> >>
> >> 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@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.
> >>
> >> 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);
> >>
> >
> >Would you please send a patch to fix it?
> >
> >Peter
> >
> >--
> >
> >Thanks,
> >Peter Chen
> 
> Pawel

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

[Felipe Balbi]

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


Hi,

Peter Chen <peter.chen@nxp.com> writes:
>> >> 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.

but it is a standard request, part of chapter 9 ;-)

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

hmm, maybe they dropped it over the years. I remember using their suite
for this very reason. Oh well.

-- 
balbi

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

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

[Pawel Laszczak]

Hi,

>
>Each setup stage will prepare status stage at cdns3_ep0_setup_phase,
>it doesn't need to add extra status stage for test mode handling,
>otherwise, the controller can't enter the test mode. Through the Lecroy
>bus analyzer log, the controller will always wait status stage
>even it is prepared by software later than the test mode is set
>by software. If we comment out the status stage at cdns3_ep0_setup_phase,
>the controller will not enter test mode even the test mode is set
>beforehand.
>
>Signed-off-by: Peter Chen <peter.chen@nxp.com>

Reviewed-by: Pawel Laszczak <pawell@cadence.com>

>---
> drivers/usb/cdns3/ep0.c | 7 -------
> 1 file changed, 7 deletions(-)
>
>diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
>index e71240b386b4..82645a2a0f52 100644
>--- a/drivers/usb/cdns3/ep0.c
>+++ b/drivers/usb/cdns3/ep0.c
>@@ -332,13 +332,6 @@ static int cdns3_ep0_feature_handle_device(struct cdns3_device *priv_dev,
> 		case TEST_K:
> 		case TEST_SE0_NAK:
> 		case TEST_PACKET:
>-			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));
>--
>2.17.1