On 11/9/2016 12:02 AM, Felipe Balbi wrote:
>
> 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.
Sure. I'll send out a patch later today.
John
--
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
