> >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@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); > >> > > > >Would you please send a patch to fix it? > > > >Peter > > > >-- > > > >Thanks, > >Peter Chen > > Pawel
