Re: [PATCH v3 6/8] usb: dwc2: host: Assume all devices are on one single_tt hub

[John Youn <John.Youn@synopsys.com>]

On 11/19/2015 8:27 AM, Doug Anderson wrote:
> Felipe,
> 
> On Thu, Nov 19, 2015 at 7:34 AM, Felipe Balbi <balbi@ti.com> wrote:
>>
>> Hi,
>>
>> Douglas Anderson <dianders@chromium.org> writes:
>>> Until we have logic to determine which devices share the same TT let's
>>> add logic to assume that all devices on a given dwc2 controller are on
>>> one single_tt hub.  This is better than the previous code that assumed
>>> that all devices were on one multi_tt hub, since single_tt hubs
>>> appear (in my experience) to be much more common and any schedule that
>>> would work on a single_tt hub will also work on a multi_tt hub.  This
>>> will prevent more than 8 total low/full speed devices to be on the bus
>>> at one time, but that's a reasonable restriction until we've made things
>>> smarter.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>> Changes in v3:
>>> - Assuming single_tt is new for v3; not terribly well tested (yet).
>>>
>>> Changes in v2: None
>>>
>>>  drivers/usb/dwc2/core.h      |  1 +
>>>  drivers/usb/dwc2/hcd_queue.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index 567ee2c9e69f..09aa2b5ae29e 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -782,6 +782,7 @@ struct dwc2_hsotg {
>>>       u16 periodic_usecs;
>>>       unsigned long periodic_bitmap[DIV_ROUND_UP(TOTAL_PERIODIC_USEC,
>>>                                                  BITS_PER_LONG)];
>>> +     bool has_split[8];
>>
>> why don't you use a u8 instead then just set each bit for each
>> "has_split" you need to take care of. This array is kinda ugly.
> 
> Let's actually drop this patch completely.  Julius and I sat down and
> he talked me through things, and with my current understanding the
> current microframe scheduler in dwc2 is broken enough that small
> little band-aids like this will do little more than just push the
> problems around.
> 
> I'm a good portion of the way through a better microframe scheduler.
> I have no doubt that it won't be perfect, but hopefully it will at
> least be based in reality...
> 
> My latest thinking on the patches in this series:
> 
> 1. usb: dwc2: rockchip: Make the max_transfer_size automatic
> 
> I'll probably separate this out into its own patch so I can stop
> sending it as part of this series.  ...or if someone wanted to land it
> then I won't bother.
> 
> 
> 2. usb: dwc2: host: Get aligned DMA in a more supported way
> 
> Still can land any time and has good benefits.  I believe that I can't
> separate this because it will cause conflicts with scheduler patches.
> 
> 
> 3. usb: dwc2: host: Add scheduler tracing
> 
> Would be nice to land.
> 
> 
> 4. usb: dwc2: host: Rewrite the microframe scheduler
> 5. usb: dwc2: host: Keep track of and use our scheduled microframe
> 6. usb: dwc2: host: Assume all devices are on one single_tt hub
> 
> Please drop patches 4-6 right now.
> 
> 
> 7. usb: dwc2: host: Add a delay before releasing periodic bandwidth
> 8. usb: dwc2: host: Giveback URB in tasklet context
> 
> I'll probably move these back up in the series (like in v2) and put
> microframe rewrite atop them.  With my current understanding the
> scheduling is so broken today that the concerns Alan brought up can
> wait until we have a proper scheduler to be addressed.  In the
> meantime getting the huge boost in interrupt speed will help with
> dwc2's correctness (and performance) because it means we're much less
> likely to miss SOF interrupts.
> 
> If anyone has any review time, giving a review to v2 of these patches
> would be helpful.  Otherwise I'll double check that v2 still looks
> good with my current understanding and eventually post them again.
> 
> -Doug
> 

Hi Doug,

Patches 1-3:
Acked-by: John Youn <johnyoun@synopsys.com>

Patch 2:
Tested-by: John Youn <johnyoun@synopsys.com>

Tested on core version 3.20 using internal TE for un-aligned
buffers.

I haven't had time to look into the scheduling patches yet. But I
agree with you that there are fundamental problems. I'll await
your rewrite.

Regards,
John