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

[Peter Chen <peter.chen@nxp.com>]

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