Hi,
John Youn <John.Youn@xxxxxxxxxxxx> writes:
>> 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.
Oh, I see what you mean now :-)
>>> 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
makes sense to me :-)
> 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 we declare a local const struct usb_endpoint_descriptor *desc here...
>
> 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);
BTW, we can remove descriptor argument to set_ep_config() as well. Can
you send this as a proper patch? I'll test it out.
> @@ -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);
... we don't need to change this line
> 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))
or this
> 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)) {
or this
> 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
--
balbi
Attachment:
signature.asc
Description: PGP signature
