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

[Doug Anderson <dianders@chromium.org>]

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