On 03/02/2016 08:58 PM, Felipe Balbi wrote:
> Hi,
>
> Mathias Nyman <mathias.ny...@linux.intel.com> writes:
>> [ text/plain ]
>> On 26.01.2016 14:58, Lu Baolu wrote:
>>> In case of endpoint stall, software is able to detect the situation
>>> by reading DCCTRL.HIT or DCCTRL.HOT bits. DbC follows the normal USB
>>> framework to handle endpoint stall. When software detects endpoint
>>> stall situation, it should wait until endpoint is recovered before
>>> read or write oprations.
>>>
>>> Signed-off-by: Lu Baolu <baolu...@linux.intel.com>
>>> ---
>>> drivers/usb/early/xhci-dbc.c | 36 ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
>>> index c81df40..344d93e 100644
>>> --- a/drivers/usb/early/xhci-dbc.c
>>> +++ b/drivers/usb/early/xhci-dbc.c
>>> @@ -1163,6 +1163,37 @@ static int xdbc_wait_until_dbc_configured(void)
>>> return -ETIMEDOUT;
>>> }
>>>
>>> +static int xdbc_wait_until_epstall_cleared(bool read)
>>> +{
>>> + int timeout = 0;
>>> +
>>> + if (read) {
>>> + do {
>>> + if (!(readl(&xdbcp->xdbc_reg->control) & CTRL_HIT)) {
>>> + xdbcp->in_ep_state = EP_RUNNING;
>>> +
>>> + return 0;
>>> + }
>>> +
>>> + xdbcp->in_ep_state = EP_HALTED;
>>> + xdbc_udelay(10);
>>> + } while (timeout++ < XDBC_LOOPS);
>>> + } else {
>>> + do {
>>> + if (!(readl(&xdbcp->xdbc_reg->control) & CTRL_HOT)) {
>>> + xdbcp->out_ep_state = EP_RUNNING;
>>> +
>>> + return 0;
>>> + }
>>> +
>>> + xdbcp->out_ep_state = EP_HALTED;
>>> + xdbc_udelay(10);
>>> + } while (timeout++ < XDBC_LOOPS);
>>> + }
>>> +
>>> + return -ETIMEDOUT;
>>> +}
>>>
>> how about something like:
>>
>> enum xdbc_ep_state *ep_state;
>> u32 halt_bit;
>>
>> if (read) {
>> ep_state = &xdbcp->in_ep_state
>> halt_bit = CTRL_HIT
>> } else {
>> ep_state = &xdbcp->out_ep_state
>> halt_bit = CTRL_HOT
>> }
>> do {
>> if (!(readl(..) & halt_bit)) {
>> *ep_state = EP_RUNNING;
>> return 0;
>> }
>> *ep_state = EP_HALTED;
>> delay()
>> } while (..)
> I'll agree, this looks better. Might also want to refactor the handshake
> loop to its own function:
>
> static int xdbg_ep_state_handshake(enum xdbc_ep_state *ep_state, u32 halt_bit)
> {
> do {
> if (!(readl(...) & halt_bit)) {
> ...
> }
> *ep_state = EP_HALTED;
> delay(...);
> } while (...)
Sure. Thanks.
>
>>> @@ -1182,6 +1213,11 @@ static int xdbc_bulk_transfer(void *data, int size,
>>> int loops, bool read)
>>> return -EPERM;
>>> }
>>>
>>> + if (xdbc_wait_until_epstall_cleared(read)) {
>>> + xdbc_trace("%s: endpoint not ready\n", __func__);
>>> + return -EPERM;
>> Is -EPERM appropriate here?
>>
>> Not sure about what error codes the device side is using, but usually
>> HALT is set due to some Data buffer/transmission/TRB error.
> EIO perhaps ?
Sure. I will replace it with -EIO.
>
>> In this case the failure is that debug host failed to send a
>> clearFeature(EP_HALT) request in time.
> I haven't read the spec, but does it define a maximum time for this to
> happen ?
I took a quick look at chapter 9.4 USB3 spec. It only requires that
"ClearFeature (ENDPOINT_HALT) request results in the endpoint
no longer returning a STALL Transaction Packet". xHCI spec doesn't
define a maximum time for this either.
Thanks,
-Baolu
>
