Hi,
Bryan O'Donoghue <pure.logic@xxxxxxxxxxxxxxxxx> writes:
> - DWC_USB3_NUM indicates the number of Device mode single directional
> endpoints, including OUT and IN endpoint 0.
>
> - DWC_USB3_NUM_IN_EPS indicates the maximum number of Device mode IN
> endpoints active at any time, including control endpoint 0.
>
> It's possible to configure RTL such that DWC_USB3_NUM_EPS is equal to
> DWC_USB3_NUM_IN_EPS.
>
> dwc3-core calculates the number of OUT endpoints as DWC_USB3_NUM minus
> DWC_USB3_NUM_IN_EPS. If RTL has been configured with DWC_USB3_NUM_IN_EPS
> equal to DWC_USB3_NUM then dwc3-core will calculate the number of OUT
> endpoints as zero.
>
> For example a from dwc3_core_num_eps() shows:
> [ 1.565000] /usb0@f01d0000: found 8 IN and 0 OUT endpoints
>
> This patch refactors the endpoint calculation down to one variable
> dwc->num_eps taking care to maintain the current mapping of endpoints for
> fixed FPGA configurations as described in Table 4-7 of version 2.60a of the
> DWC USB3 databook.
>
> The endpoint mapping will then be EP-OUT, EP-IN etc, up to DWC_USB3_NUM.
> If DWC_USB3_NUM is odd then OUT will take the extra endpoint.
>
> Suggested-by: Felipe Balbi <balbi@xxxxxxxxxx>
> Signed-off-by: Bryan O'Donoghue <pure.logic@xxxxxxxxxxxxxxxxx>
it turns out that this is regressing dwc3 :-s
After testing, it became clear, actually :-)
> ---
> drivers/usb/dwc3/core.c | 3 +--
> drivers/usb/dwc3/core.h | 6 ++----
> drivers/usb/dwc3/debugfs.c | 15 ++-------------
> drivers/usb/dwc3/gadget.c | 36 +++++++++++-------------------------
> 4 files changed, 16 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 369bab1..68c9c84 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -397,8 +397,7 @@ static void dwc3_core_num_eps(struct dwc3 *dwc)
> {
> struct dwc3_hwparams *parms = &dwc->hwparams;
>
> - dwc->num_in_eps = DWC3_NUM_IN_EPS(parms);
> - dwc->num_out_eps = DWC3_NUM_EPS(parms) - dwc->num_in_eps;
> + dwc->num_eps = DWC3_NUM_EPS(parms);
> }
>
> static void dwc3_cache_hwparams(struct dwc3 *dwc)
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2b9e4ca..d496ddc 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -799,8 +799,7 @@ struct dwc3_scratchpad_array {
> * @u2pel: parameter from Set SEL request.
> * @u1sel: parameter from Set SEL request.
> * @u1pel: parameter from Set SEL request.
> - * @num_out_eps: number of out endpoints
> - * @num_in_eps: number of in endpoints
> + * @num_eps: number of endpoints
> * @ep0_next_event: hold the next expected event
> * @ep0state: state of endpoint zero
> * @link_state: link state
> @@ -960,8 +959,7 @@ struct dwc3 {
>
> u8 speed;
>
> - u8 num_out_eps;
> - u8 num_in_eps;
> + u8 num_eps;
>
> struct dwc3_hwparams hwparams;
> struct dentry *root;
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index 31926dd..7df4541 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -822,19 +822,8 @@ static void dwc3_debugfs_create_endpoint_dirs(struct dwc3 *dwc,
> {
> int i;
>
> - for (i = 0; i < dwc->num_in_eps; i++) {
> - u8 epnum = (i << 1) | 1;
> - struct dwc3_ep *dep = dwc->eps[epnum];
> -
> - if (!dep)
> - continue;
> -
> - dwc3_debugfs_create_endpoint_dir(dep, parent);
> - }
> -
> - for (i = 0; i < dwc->num_out_eps; i++) {
> - u8 epnum = (i << 1);
> - struct dwc3_ep *dep = dwc->eps[epnum];
> + for (i = 0; i < dwc->num_eps; i++) {
> + struct dwc3_ep *dep = dwc->eps[i];
>
> if (!dep)
> continue;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4db97ec..ef86ab6 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1930,14 +1930,13 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
>
> /* -------------------------------------------------------------------------- */
>
> -static int dwc3_gadget_init_hw_endpoints(struct dwc3 *dwc,
> - u8 num, u32 direction)
> +static int dwc3_gadget_init_hw_endpoints(struct dwc3 *dwc, u8 num)
> {
> struct dwc3_ep *dep;
> - u8 i;
> + u8 epnum;
> + bool direction = false;
>
> - for (i = 0; i < num; i++) {
> - u8 epnum = (i << 1) | (direction ? 1 : 0);
> + for (epnum = 0; epnum < num; epnum++) {
>
> dep = kzalloc(sizeof(*dep), GFP_KERNEL);
> if (!dep)
> @@ -1945,12 +1944,12 @@ static int dwc3_gadget_init_hw_endpoints(struct dwc3 *dwc,
>
> dep->dwc = dwc;
> dep->number = epnum;
> - dep->direction = !!direction;
> + dep->direction = direction;
this is not how you should calculate direction anymore. direction is the
lowest bit of epnum. So you could declare and initialize direction
inside the for loop:
bool direction = epnum & 1;
> dep->regs = dwc->regs + DWC3_DEP_BASE(epnum);
> dwc->eps[epnum] = dep;
>
> - snprintf(dep->name, sizeof(dep->name), "ep%d%s", epnum >> 1,
> - (epnum & 1) ? "in" : "out");
> + snprintf(dep->name, sizeof(dep->name), "ep%d%s", epnum / 2,
> + direction ? "in" : "out");
this doesn't need to be changed at all. Well, the change from (epnum &
1) to direction is unnecessary, but welcome.
> dep->endpoint.name = dep->name;
>
> @@ -1977,7 +1976,7 @@ static int dwc3_gadget_init_hw_endpoints(struct dwc3 *dwc,
> /* MDWIDTH is represented in bits, we need it in bytes */
> mdwidth /= 8;
>
> - size = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(i));
> + size = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(epnum));
and here's a BIG regression. You're reading registers that don't
exist. The number to be used is without the direction bit so (epnum >>
1).
> @@ -2027,11 +2026,12 @@ static int dwc3_gadget_init_hw_endpoints(struct dwc3 *dwc,
> dep->endpoint.caps.type_int = true;
> }
>
> - dep->endpoint.caps.dir_in = !!direction;
> + dep->endpoint.caps.dir_in = direction;
unnecessary (but welcome) change.
> dep->endpoint.caps.dir_out = !direction;
>
> INIT_LIST_HEAD(&dep->pending_list);
> INIT_LIST_HEAD(&dep->started_list);
> + direction = !direction;
unnecessary (and NOT welcome) change :-)
With the changes below, I have this working:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 19181f62a479..04e4f39ce90d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2000,11 +2000,11 @@ static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 num)
{
struct dwc3_ep *dep;
u8 epnum;
- bool direction = false;
INIT_LIST_HEAD(&dwc->gadget.ep_list);
for (epnum = 0; epnum < num; epnum++) {
+ bool direction = epnum & 1;
dep = kzalloc(sizeof(*dep), GFP_KERNEL);
if (!dep)
@@ -2016,8 +2016,8 @@ static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 num)
dep->regs = dwc->regs + DWC3_DEP_BASE(epnum);
dwc->eps[epnum] = dep;
- snprintf(dep->name, sizeof(dep->name), "ep%d%s", epnum / 2,
- direction ? "in" : "out");
+ snprintf(dep->name, sizeof(dep->name), "ep%d%s", epnum >> 1,
+ direction ? "in" : "out");
dep->endpoint.name = dep->name;
@@ -2044,7 +2044,7 @@ static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 num)
/* MDWIDTH is represented in bits, we need it in bytes */
mdwidth /= 8;
- size = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(epnum));
+ size = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(epnum >> 1));
size = DWC3_GTXFIFOSIZ_TXFDEF(size);
/* FIFO Depth is in MDWDITH bytes. Multiply */
@@ -2117,7 +2117,6 @@ static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 num)
INIT_LIST_HEAD(&dep->pending_list);
INIT_LIST_HEAD(&dep->started_list);
- direction = !direction;
}
return 0;
I can change your patch locally, if you want. I can also drop your
patches and wait for replacements. No strong feelings.
--
balbi
Attachment:
signature.asc
Description: PGP signature
