Clarification of set_sel handling when dwc3 is a device (gadget)

Clarification of set_sel handling when dwc3 is a device (gadget)

[claus.stovgaard]

Hi

I have earlier worked with the possibility for disabling U1/U2 to solve
an issue where the dwc3 acting as a gadget device sometimes failing to
leave U2.

Analyzing the situation when the dwc3 failing to leave U2, it happens
when the link is in U2 after timeout from U1, and when the device wants
to leave U2. Not when the host wants to leave U2.

Going back the BOS descriptor from the device reports

bU1DevExitLat 1us
bU2DevExitLat 500us

And the SetSystemExitLatency control transfer ends with

U1 System Exit Latency 86 us
U1 Device to Host Exit Latency 1 us
U2 System Exit Latency 585 us
U2 Device to Host Exit Latency 500 us

Looking at the length of LFPS etc. it seems that the U2 exit is just
above 80 us,and it start link training just after.

So I was wandering how the exit latency of the system is communicated
to dwc3 core, and found the following code from ep0.c
dwc3_ep0_set_sel_cmpl

----
reg = dwc3_readl(dwc->regs, DWC3_DCTL);
if (reg & DWC3_DCTL_INITU2ENA)
	param = dwc->u2pel;
if (reg & DWC3_DCTL_INITU1ENA)
	param = dwc->u1pel;

/*
 * According to Synopsys Databook, if parameter is
 * greater than 125, a value of zero should be
 * programmed in the register.
 */
if (param > 125)
	param = 0;

/* now that we have the time, issue DGCMD Set Sel */
ret = dwc3_send_gadget_generic_command(dwc,
		DWC3_DGCMD_SET_PERIODIC_PAR, param);
WARN_ON(ret < 0);
----

I don't have access to Synopsys Databook, so I am puzzled about
the DWC3_DGCMD_SET_PERIODIC_PAR command. The code favor to use the
device to host exit latency, and use the u1 if present over the u2.

So the dwc3 core never get the system exit latency times, and they just
disapeer.

I hope that someone have access to the Databook, and is able to share
some details about the set periodic command and the parameter.

Regards
Claus

Re: Clarification of set_sel handling when dwc3 is a device (gadget)

[Thinh Nguyen]

Hi,

claus.stovgaard@gmail.com wrote:
> Hi
>
> I have earlier worked with the possibility for disabling U1/U2 to solve
> an issue where the dwc3 acting as a gadget device sometimes failing to
> leave U2.
>
> Analyzing the situation when the dwc3 failing to leave U2, it happens
> when the link is in U2 after timeout from U1, and when the device wants
> to leave U2. Not when the host wants to leave U2.
>
> Going back the BOS descriptor from the device reports
>
> bU1DevExitLat 1us
> bU2DevExitLat 500us
>
> And the SetSystemExitLatency control transfer ends with
>
> U1 System Exit Latency 86 us
> U1 Device to Host Exit Latency 1 us
> U2 System Exit Latency 585 us
> U2 Device to Host Exit Latency 500 us
>
> Looking at the length of LFPS etc. it seems that the U2 exit is just
> above 80 us,and it start link training just after.
>
> So I was wandering how the exit latency of the system is communicated
> to dwc3 core, and found the following code from ep0.c
> dwc3_ep0_set_sel_cmpl
>
> ----
> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> if (reg & DWC3_DCTL_INITU2ENA)
> 	param = dwc->u2pel;
> if (reg & DWC3_DCTL_INITU1ENA)
> 	param = dwc->u1pel;

This is incorrect. If the controller is enabled for both U1 and U2, then
periodic param is U2PEL. If the controller is only enabled for U1, then
U1PEL is used. Probably the original author intended but missed the
"else" on the second if case.

> /*
>  * According to Synopsys Databook, if parameter is
>  * greater than 125, a value of zero should be
>  * programmed in the register.
>  */
> if (param > 125)
> 	param = 0;
>
> /* now that we have the time, issue DGCMD Set Sel */
> ret = dwc3_send_gadget_generic_command(dwc,
> 		DWC3_DGCMD_SET_PERIODIC_PAR, param);
> WARN_ON(ret < 0);
> ----
>
> I don't have access to Synopsys Databook, so I am puzzled about
> the DWC3_DGCMD_SET_PERIODIC_PAR command. The code favor to use the
> device to host exit latency, and use the u1 if present over the u2.
>
> So the dwc3 core never get the system exit latency times, and they just
> disapeer.
>
> I hope that someone have access to the Databook, and is able to share
> some details about the set periodic command and the parameter.
>

According to the databook, currently the controller doesn't use these
programmed values. It uses the value from CoreConsultant configuration
setting.

BR,
Thinh

Re: Clarification of set_sel handling when dwc3 is a device (gadget)

[claus.stovgaard]

On man, 2019-07-01 at 20:48 +0000, Thinh Nguyen wrote:
> Hi,
> 
> 

> > ----
> > reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> > if (reg & DWC3_DCTL_INITU2ENA)
> > 	param = dwc->u2pel;
> > if (reg & DWC3_DCTL_INITU1ENA)
> > 	param = dwc->u1pel;
> 
> This is incorrect. If the controller is enabled for both U1 and U2,
> then
> periodic param is U2PEL. If the controller is only enabled for U1,
> then
> U1PEL is used. Probably the original author intended but missed the
> "else" on the second if case.
> 
> 
> According to the databook, currently the controller doesn't use these
> programmed values. It uses the value from CoreConsultant
> configuration
> setting.
> 

Thanks for your quick reply.

So my understanding from your message is that the code is wrong, and I
was correct in being puzzled. Though the core currently does not use
the values, so currently the code is unimportant as it is unused by the
hardware.

Regards
Claus


> BR,
> Thinh

Re: Clarification of set_sel handling when dwc3 is a device (gadget)

[Thinh Nguyen]

Hi,

claus.stovgaard@gmail.com wrote:
> On man, 2019-07-01 at 20:48 +0000, Thinh Nguyen wrote:
>> Hi,
>>
>>
>>> ----
>>> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>> if (reg & DWC3_DCTL_INITU2ENA)
>>> 	param = dwc->u2pel;
>>> if (reg & DWC3_DCTL_INITU1ENA)
>>> 	param = dwc->u1pel;
>> This is incorrect. If the controller is enabled for both U1 and U2,
>> then
>> periodic param is U2PEL. If the controller is only enabled for U1,
>> then
>> U1PEL is used. Probably the original author intended but missed the
>> "else" on the second if case.
>>
>>
>> According to the databook, currently the controller doesn't use these
>> programmed values. It uses the value from CoreConsultant
>> configuration
>> setting.
>>
> Thanks for your quick reply.
>
> So my understanding from your message is that the code is wrong, and I
> was correct in being puzzled. Though the core currently does not use
> the values, so currently the code is unimportant as it is unused by the
> hardware.
>

Right. Even though currently it does not affect the controller, the code
needs to be fixed along with some note that the controller currently
doesn't use these values.

BR,
Thinh