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

[Felipe Balbi <balbi@kernel.org>]

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