On 11/8/2016 2:48 AM, Felipe Balbi wrote:
>
> Hi,
>
> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>>>>> @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
>>>>>> * @hsotg: The driver state.
>>>>>> * @ep: The index number of the endpoint
>>>>>> * @mps: The maximum packet size in bytes
>>>>>> + * @mc: The multicount value
>>>>>> *
>>>>>> * Configure the maximum packet size for the given endpoint, updating
>>>>>> * the hardware control registers to reflect this.
>>>>>> */
>>>>>> static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
>>>>>> - unsigned int ep, unsigned int mps, unsigned int dir_in)
>>>>>> + unsigned int ep, unsigned int mps,
>>>>>> + unsigned int mc, unsigned int dir_in)
>>>>>
>>>>> this has an odd set of arguments. You pass the ep index, mps, direction
>>>>> and mult value, when you could just pass hsotg_ep and descriptor instead.
>>>>
>>>> Yes looks like we can do some simplification here. And you probably
>>>> don't need to pass a descriptor either since it must be set in the
>>>> usb_ep before enable.
>>>>
>>>> However this is also called in some contexts where a descriptor is not
>>>> available (initialization and ep0). So we have to think about this a
>>>> bit.
>>>>
>>>> I think dwc3 can make similar simplification on the
>>>> __dwc3_gadget_ep_enable().
>>>
>>> __dwc3_gadget_ep_enable() takes the actual descriptors as arguments:
>>>
>>> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>>> const struct usb_endpoint_descriptor *desc,
>>> const struct usb_ss_ep_comp_descriptor *comp_desc,
>>> bool modify, bool restore)
>>>
>>> I fail to see how much simpler we can make this. Perhaps we can turn
>>> bool and restore into a single argument if we use a bitfield instead of
>>> a bool.
>>>
>>
>> Hi Felipe,
>>
>> I mean that there shouldn't be any need to pass in descriptors with
>> usb_ep since the ones in usb_ep should be set properly from ep enable
>> to ep disable.
>>
>> I think that's the case anyways.
>
> __dwc3_gadget_ep_enable() is the one assigning the descriptor to the ep:
>
> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
> const struct usb_endpoint_descriptor *desc,
> const struct usb_ss_ep_comp_descriptor *comp_desc,
> bool modify, bool restore)
> {
> struct dwc3 *dwc = dep->dwc;
> u32 reg;
> int ret;
>
> if (!(dep->flags & DWC3_EP_ENABLED)) {
> ret = dwc3_gadget_start_config(dwc, dep);
> if (ret)
> return ret;
> }
>
> ret = dwc3_gadget_set_ep_config(dwc, dep, desc, comp_desc, modify,
> restore);
> if (ret)
> return ret;
>
> if (!(dep->flags & DWC3_EP_ENABLED)) {
> struct dwc3_trb *trb_st_hw;
> struct dwc3_trb *trb_link;
>
> dep->endpoint.desc = desc;
> dep->comp_desc = comp_desc;
>
> [...]
>
Right, but that's redundant for non-ep0 case. And the EP0 case can be
handled globally elsewhere.
The usb_ep_enable() API function takes only the usb_ep and requires
that the descriptors are set in the usb_ep beforehand. So 'desc'
argument in the callback function is probably not required either.
I think this is correct to make it consistent. Since we don't pass the
SS comp descriptor, and we don't want to keep passing more descriptors
every time a new standard descriptor is added, such as the SSP ISO EP
comp descriptor for 3.1.
See below for a proposed change to see what I mean (not tested).
Regards,
John
---->8----
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2322863..b9903c6 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -539,7 +539,6 @@ struct dwc3_ep {
struct dwc3_trb *trb_pool;
dma_addr_t trb_pool_dma;
- const struct usb_ss_ep_comp_descriptor *comp_desc;
struct dwc3 *dwc;
u32 saved_state;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e47cba5..0fc55e89 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -576,13 +576,12 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep *dep)
* Caller should take care of locking
*/
static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
- const struct usb_endpoint_descriptor *desc,
- const struct usb_ss_ep_comp_descriptor *comp_desc,
bool modify, bool restore)
{
struct dwc3 *dwc = dep->dwc;
u32 reg;
int ret;
+ struct usb_ep *ep;
if (!(dep->flags & DWC3_EP_ENABLED)) {
ret = dwc3_gadget_start_config(dwc, dep);
@@ -590,8 +589,9 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
return ret;
}
- ret = dwc3_gadget_set_ep_config(dwc, dep, desc, comp_desc, modify,
- restore);
+ ep = &dep->endpoint;
+ ret = dwc3_gadget_set_ep_config(dwc, dep, ep->desc, ep->comp_desc,
+ modify, restore);
if (ret)
return ret;
@@ -599,9 +599,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
struct dwc3_trb *trb_st_hw;
struct dwc3_trb *trb_link;
- dep->endpoint.desc = desc;
- dep->comp_desc = comp_desc;
- dep->type = usb_endpoint_type(desc);
+ dep->type = usb_endpoint_type(ep->desc);
dep->flags |= DWC3_EP_ENABLED;
dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
@@ -611,7 +609,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
init_waitqueue_head(&dep->wait_end_transfer);
- if (usb_endpoint_xfer_control(desc))
+ if (usb_endpoint_xfer_control(ep->desc))
goto out;
/* Initialize the TRB ring */
@@ -634,7 +632,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
* Issue StartTransfer here with no-op TRB so we can always rely on No
* Response Update Transfer command.
*/
- if (usb_endpoint_xfer_bulk(desc)) {
+ if (usb_endpoint_xfer_bulk(ep->desc)) {
struct dwc3_gadget_ep_cmd_params params;
struct dwc3_trb *trb;
dma_addr_t trb_dma;
@@ -714,7 +712,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
dep->stream_capable = false;
dep->endpoint.desc = NULL;
- dep->comp_desc = NULL;
dep->type = 0;
dep->flags &= DWC3_EP_END_TRANSFER_PENDING;
@@ -763,7 +760,7 @@ static int dwc3_gadget_ep_enable(struct usb_ep *ep,
return 0;
spin_lock_irqsave(&dwc->lock, flags);
- ret = __dwc3_gadget_ep_enable(dep, desc, ep->comp_desc, false, false);
+ ret = __dwc3_gadget_ep_enable(dep, false, false);
spin_unlock_irqrestore(&dwc->lock, flags);
return ret;
@@ -1735,7 +1732,8 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
dep = dwc->eps[0];
- ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, false,
+ dep->endpoint.desc = &dwc3_gadget_ep0_desc;
+ ret = __dwc3_gadget_ep_enable(dep, false,
false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
@@ -1743,7 +1741,8 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
}
dep = dwc->eps[1];
- ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, false,
+ dep->endpoint.desc = &dwc3_gadget_ep0_desc;
+ ret = __dwc3_gadget_ep_enable(dep, false,
false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
@@ -2573,7 +2572,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
}
dep = dwc->eps[0];
- ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, true,
+ dep->endpoint.desc = &dwc3_gadget_ep0_desc;
+ ret = __dwc3_gadget_ep_enable(dep, true,
false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
@@ -2581,7 +2581,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
}
dep = dwc->eps[1];
- ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, true,
+ dep->endpoint.desc = &dwc3_gadget_ep0_desc;
+ ret = __dwc3_gadget_ep_enable(dep, true,
false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
