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

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

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