Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1315206 > unrolled thread

[PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    [PATCH v5 18/21] usb: dwc2: host: Add dwc2_hcd_get_future_frame_number() call Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066 Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
      Re: [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528  for rk3066 Kever Yang <kever.yang@rock-chips.com> - 2016-01-27 11:20 +0100
        Re: [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528  for rk3066 Doug Anderson <dianders@chromium.org> - 2016-01-27 20:50 +0100
    [PATCH v5 14/21] usb: dwc2: host: Split code out to make dwc2_do_reserve() Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    [PATCH v5 16/21] usb: dwc2: host: Manage frame nums better in scheduler Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
      Re: [PATCH v5 16/21] usb: dwc2: host: Manage frame nums better in scheduler Doug Anderson <dianders@chromium.org> - 2016-01-27 21:50 +0100
    [PATCH v5 11/21] usb: dwc2: host: Use periodic interrupt even with DMA Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    [PATCH v5 05/21] usb: dwc2: host: Avoid use of chan->qh after qh freed Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    [PATCH v5 15/21] usb: dwc2: host: Add scheduler logging for missed SOFs Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    [PATCH v5 21/21] usb: dwc2: host: If using uframe scheduler, end splits better Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    Re: [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits Heiko Stuebner <heiko@sntech.de> - 2016-01-23 19:00 +0100
      Re: [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff,  especially with splits Doug Anderson <dianders@chromium.org> - 2016-01-24 00:20 +0100
        Re: [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff,  especially with splits Doug Anderson <dianders@chromium.org> - 2016-01-24 06:40 +0100
    Re: [PATCH v5 20/21] usb: dwc2: host: Totally redo the microframe scheduler Doug Anderson <dianders@chromium.org> - 2016-01-24 06:50 +0100

#1315206 — [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits

This is a bit of catchall series for all the bug fix and performance
patches I've been working on over the last few months.  Note that for
dwc2 we need to do LOTS in software and need super low interrupt
latency, so most performance improvements actually fix real bugs.

Patches are structured to start with no-brainer stuff that could be
applied ASAP, especially things I've already gotten Acks for.  Things
get slightly more RFC / RFT like as we get farther down the series.
Anything that can be landed sooner rather than later (especially those
Acked long ago) would help in re-posts (I'm not biased, of course).

It's been a few months since my last post of this series.  In the
meantime I've added a bunch of small bugfixes to the start of it and
also TOTALLY REWROTE the microframe scheduler.  I'll say up front: I
know nothing about USB.  I haven't read the whole spec.  I'm not
terribly familiar with the OHCI, EHCI, and XHCI drivers in the kernel.
...and I'm pretty clueless overall.  Nevertheless, I've attempted to
write up a fancy scheduler based on the portion of the spec talking
about microframe scheduling requirements.  This rewritten scheduler does
seem to help when I start jamming lots of USB things into a hub, so
presumably the code is a reasonably starting point.  Given my current
understanding of USB the old code was fairly insane, so presumably even
if my new patch isn't perfect it's better than what we had.

Anyway, on to the patches:

1. usb: dwc2: rockchip: Make the max_transfer_size automatic

   No brainer.  Can land any time.

2. usb: dwc2: host: Get aligned DMA in a more supported way

   Although this touches a lot of code, it's mostly just deleting
   stuff.  The way this is working is nearly the same as tegra.  Biggest
   objection I expect is that it has too much duplication with tegra and
   musb.  I'd personally prefer to land it now and remove duplication
   later, but up to others.  Speeding up interrupt handler helps with
   SOF scheduling, so this is not just a dumb optimization.

3. usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066
4. usb: dwc2: host: Set host_perio_tx_fifo_size to 304 for rk3066

   Seems like a good idea and small impact, but if someone hates it or
   it breaks on some Rockchip SoC, just drop it.  I've only tested on
   rk3288 so it would be nice if someone with access to more Rockchip
   SoCs can give a tested by.

5. usb: dwc2: host: Avoid use of chan->qh after qh freed

   Simple bugfix.  Unrelated to the series but thrown in here.

6. usb: dwc2: host: Always add to the tail of queues

   Big functionality improvement.  Small patch.  Suggest applying ASAP.

7. usb: dwc2: hcd: fix split transfer schedule sequence

   Unless I'm misunderstanding, this should be a no-brainer to fix.
   Could be some bikeshedding on how to fix this.  Let me know if/how
   you want me to spin.  Otherwise I'd say land it and it will fix a
   bunch of stuff.

8. usb: dwc2: host: Add scheduler tracing

   Shouldn't hurt anything.  If you have bikesheds, let me know.

9.  usb: dwc2: host: Add a delay before releasing periodic bandwidth
10. usb: dwc2: host: Giveback URB in tasklet context

   I think we should take these.  They improve things a bunch and I have
   found no regressions due to them.  Additional testing appreciated, of
   course.

11. usb: dwc2: host: Use periodic interrupt even with DMA

   Just came up with this one recently so it's had slightly less
   testing.  ...but it certainly fixed a bunch of stuff.  Could probably
   be moved around in the series to be pretty much anywhere.  I don't
   think this has a huge impact until we fix the scheduler (below) but
   at the same time I'm pretty sure it's something that's been wrong for
   a long time.

12. usb: dwc2: host: Rename some fields in struct dwc2_qh
13. usb: dwc2: host: Reorder things in hcd_queue.c
14. usb: dwc2: host: Split code out to make dwc2_do_reserve()

   Cleanups to make future patches easier to understand.  Bikeshed away.
   All no-op changes.

15. usb: dwc2: host: Add scheduler logging for missed SOFs

   I found this to be quite helpful.  If you hate it, drop it from the
   series.

16. usb: dwc2: host: Manage frame nums better in scheduler

   Doesn't totally make sense on its own, but a good halfway point to
   the microframe scheduler.  ...and shouldn't regress anything.  Allows
   us to do the "Properly set even/odd frame" patch below which
   definitely improves things.

17. usb: dwc2: host: Schedule periodic right away if it's time

   Yet another small change to make scheduling tighter.

18. usb: dwc2: host: Add dwc2_hcd_get_future_frame_number() call

   Prep for ("usb: dwc2: host: Properly set even/odd frame")

19. usb: dwc2: host: Properly set even/odd frame

   Helps quite a bit.  Helps even more after the redone microframe
   scheduler.  Feel free to tidy up if you see easy ways to do this.
   Maybe someone has a better way to estimate time on the wire?

20. usb: dwc2: host: Totally redo the microframe scheduler

   Eyeballs please!  I think I've stared at this too much and now my
   eyes are glazing over.  This definitely helps but also probably needs
   a few more spins?  Of course, if nobody wants to review it, IMHO
   checking it in as-is is WAAAAY better than what we had before.

21. usb: dwc2: host: If using uframe scheduler, end splits better

   Low confidence in this one.  Worry that it will end something too
   soon, but haven't seen it yet.

===

Below is discussion of some of the speedup stuff (mostly relevant to the
first few patches).

===

The dwc2 interrupt handler is quite slow.  On rk3288 with a few things
plugged into the ports and with cpufreq locked at 696MHz (to simulate
real world idle system), I can easily observe dwc2_handle_hcd_intr()
taking > 120 us, sometimes > 150 us.  Note that SOF interrupts come
every 125 us with high speed USB, so taking > 120 us in the interrupt
handler is a big deal.

The patches here will speed up the interrupt controller significantly.
After this series, I have a hard time seeing the interrupt controller
taking > 20 us and I don't ever see it taking > 30 us ever in my tests
unless I bring the cpufreq back down.  With the cpufreq at 126 MHz I can
still see the interrupt handler take > 50 us, so I'm sure we could
improve this further.  ...but hey, it's a start.

This series also shows big speed improvements when testing with a USB
Gigabit Ethernet adapter.  Previously the tested adapter would top out
at about 15MB/s.  After these changes it gets about 23MB/s.

In addition to the speedup, this series also has the advantage of
simplifying dwc2 and making it more like everyone else (introducing the
possibility of future simplifications).  Picking this series up will
help your diffstat and likely win you friends.  ;)

===

Steps for gathering data with ftrace (for some reason I have to run
twice):

cd /sys/devices/system/cpu/cpu0/cpufreq/
echo userspace > scaling_governor
echo 696000 > scaling_setspeed

cd /sys/kernel/debug/tracing
echo 0 > tracing_on
echo "" > trace
echo nop > current_tracer
echo function_graph > current_tracer
echo dwc2_handle_hcd_intr > set_graph_function
echo dwc2_handle_common_intr >> set_graph_function
echo dwc2_handle_hcd_intr > set_ftrace_filter
echo dwc2_handle_common_intr >> set_ftrace_filter
echo funcgraph-abstime > trace_options
echo 70 > tracing_thresh
echo 1 > /sys/kernel/debug/tracing/tracing_on

sleep 2
cat trace

Changes in v5:
- Move list maintenance to hcd.c to avoid gadget-only compile error
- Moved defines outside of ifdef to avoid gadget-only compile error.

Changes in v4:
- Add John's Acks from <https://patchwork.kernel.org/patch/7631551>
- Set host_rx_fifo_size to 528 for rk3066 new for v4.
- Set host_perio_tx_fifo_size to 304 for rk3066 new for v4.
- Avoid use of chan->qh after qh freed new for v4.
- Always add to the tail of queues new for v4.
- fix split transfer schedule sequence new for v4.
- Retooled scheduler tracing a bit, so left off John's Ack from v3.
- Moved periodic bandwidth release delay patch earlier again.
- A bit earlier in the list of patches than in v3.
- Use periodic interrupt even with DMA new for v4.
- Rename some fields in struct dwc2_qh new for v4.
- Reorder things in hcd_queue.c new for v4.
- Split code out to make dwc2_do_reserve() new for v4.
- Add scheduler logging for missed SOFs new for v4.
- Manage frame nums better in scheduler new for v4.
- Schedule periodic right away if it's time new for v4.
- Add dwc2_hcd_get_future_frame_number() call new for v4.
- Properly set even/odd frame new for v4.
- Figured out what the microframe scheduler was supposed to do.
- Microframe rewrite is totally different from v3, hopefully more right.
- Microframe rewrite is later in the series now.
- If using uframe scheduler, end splits better new for v4.

Changes in v3:
- Moved periodic bandwidth release delay patch later in the series.
- The uframe scheduler patch is folded into optimization series.
- Optimize uframe scheduler "single uframe" case a little.
- uframe scheduler now atop logging patches.
- uframe scheduler now before delayed bandwidth release patches.
- Add defines like EARLY_FRAME_USEC
- Reorder dwc2_deschedule_periodic() in prep for future patches.
- uframe scheduler now shows real usefulness w/ future patches!
- Assuming single_tt is new for v3; not terribly well tested (yet).
- Keep track and use our uframe new for v3.

Changes in v2:
- Add a warn if setup_dma is not aligned (Julius Werner).
- Periodic bandwidth release delay new for V2
- Commit message now says that URB giveback change needs delay change.
- Totally rewrote uframe scheduler again after writing test code.
- uframe scheduler atop delayed bandwidth release patches.

Douglas Anderson (21):
  usb: dwc2: rockchip: Make the max_transfer_size automatic
  usb: dwc2: host: Get aligned DMA in a more supported way
  usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066
  usb: dwc2: host: Set host_perio_tx_fifo_size to 304 for rk3066
  usb: dwc2: host: Avoid use of chan->qh after qh freed
  usb: dwc2: host: Always add to the tail of queues
  usb: dwc2: hcd: fix split transfer schedule sequence
  usb: dwc2: host: Add scheduler tracing
  usb: dwc2: host: Add a delay before releasing periodic bandwidth
  usb: dwc2: host: Giveback URB in tasklet context
  usb: dwc2: host: Use periodic interrupt even with DMA
  usb: dwc2: host: Rename some fields in struct dwc2_qh
  usb: dwc2: host: Reorder things in hcd_queue.c
  usb: dwc2: host: Split code out to make dwc2_do_reserve()
  usb: dwc2: host: Add scheduler logging for missed SOFs
  usb: dwc2: host: Manage frame nums better in scheduler
  usb: dwc2: host: Schedule periodic right away if it's time
  usb: dwc2: host: Add dwc2_hcd_get_future_frame_number() call
  usb: dwc2: host: Properly set even/odd frame
  usb: dwc2: host: Totally redo the microframe scheduler
  usb: dwc2: host: If using uframe scheduler, end splits better

 drivers/usb/dwc2/core.c      |  115 ++-
 drivers/usb/dwc2/core.h      |  114 ++-
 drivers/usb/dwc2/hcd.c       |  390 ++++++---
 drivers/usb/dwc2/hcd.h       |  126 ++-
 drivers/usb/dwc2/hcd_ddma.c  |   41 +-
 drivers/usb/dwc2/hcd_intr.c  |  164 ++--
 drivers/usb/dwc2/hcd_queue.c | 1995 ++++++++++++++++++++++++++++++++++--------
 drivers/usb/dwc2/platform.c  |    6 +-
 8 files changed, 2279 insertions(+), 672 deletions(-)

-- 
2.7.0.rc3.207.g0ac5344

[toc] | [next] | [standalone]

#1315207 — [PATCH v5 18/21] usb: dwc2: host: Add dwc2_hcd_get_future_frame_number() call

As we start getting more exact about our scheduling it's becoming more
and more important to know exactly how far through the current frame we
are.  This lets us make decisions about whether there's still time left
to start a new transaction in the current frame.

We'll add dwc2_hcd_get_future_frame_number() which will tell you what
the frame number will be a certain number of microseconds (us) from
now.  We can use this information to help decide if there's enough time
left in the frame for a transaction that will take a certain duration.

This is expected to be used by a future change ("usb: dwc2: host:
Properly set even/odd frame").

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4:
- Add dwc2_hcd_get_future_frame_number() call new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/core.h |  4 ++++
 drivers/usb/dwc2/hcd.c  | 29 +++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 64d45a2053bb..52cbea28d0e9 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1235,12 +1235,16 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg,
 
 #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
 extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
+extern int dwc2_hcd_get_future_frame_number(struct dwc2_hsotg *hsotg, int us);
 extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
 extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
 extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
 #else
 static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
 { return 0; }
+static inline int dwc2_hcd_get_future_frame_number(struct dwc2_hsotg *hsotg,
+						   int us)
+{ return 0; }
 static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
 static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
 static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 03072d1fd44d..83dc92da10a0 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1947,6 +1947,35 @@ int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
 	return (hfnum & HFNUM_FRNUM_MASK) >> HFNUM_FRNUM_SHIFT;
 }
 
+int dwc2_hcd_get_future_frame_number(struct dwc2_hsotg *hsotg, int us)
+{
+	u32 hprt = dwc2_readl(hsotg->regs + HPRT0);
+	u32 hfir = dwc2_readl(hsotg->regs + HFIR);
+	u32 hfnum = dwc2_readl(hsotg->regs + HFNUM);
+	unsigned int us_per_frame;
+	unsigned int frame_number;
+	unsigned int remaining;
+	unsigned int interval;
+	unsigned int phy_clks;
+
+	/* High speed has 125 us per (micro) frame; others are 1 ms per */
+	us_per_frame = (hprt & HPRT0_SPD_MASK) ? 1000 : 125;
+
+	/* Extract fields */
+	frame_number = (hfnum & HFNUM_FRNUM_MASK) >> HFNUM_FRNUM_SHIFT;
+	remaining = (hfnum & HFNUM_FRREM_MASK) >> HFNUM_FRREM_SHIFT;
+	interval = (hfir & HFIR_FRINT_MASK) >> HFIR_FRINT_SHIFT;
+
+	/*
+	 * Number of phy clocks since the last tick of the frame number after
+	 * "us" has passed.
+	 */
+	phy_clks = (interval - remaining) +
+		   DIV_ROUND_UP(interval * us, us_per_frame);
+
+	return dwc2_frame_num_inc(frame_number, phy_clks / interval);
+}
+
 int dwc2_hcd_is_b_host(struct dwc2_hsotg *hsotg)
 {
 	return hsotg->op_state == OTG_STATE_B_HOST;
-- 
2.7.0.rc3.207.g0ac5344

[toc] | [prev] | [next] | [standalone]

#1315208 — [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066

As documented in dwc2_calculate_dynamic_fifo(), host_rx_fifo_size should
really be:
 2 * ((Largest Packet size / 4) + 1 + 1) + n
 with n = number of host channel.

We have 9 host channels, so
 2 * ((1024/4) + 2) + 9 = 516 + 9 = 525

We've got 960 / 972 total_fifo_size on rk3288 (and presumably on
rk3066) and 525 + 128 + 256 = 909 so we're still under on both ports
even when we increment by 5.

Since we have space, Kever Yang suggests bumping by 8.  He says this
will meet INCR16 access and next fifo type can start with a aligned
address.

...so let's bump up by 8.  In the future, it would be nice if
dwc2_calculate_dynamic_fifo() could handle the "too small" FIFO case and
come up with something more dynamically.  When we do that we can figure
out how to allocate the extra 48 / 60 bytes of FIFO that we're currently
wasting.

NOTE: no known bugs are fixed by this patch, but it seems like a simple
fix and ought to fix someone.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4:
- Set host_rx_fifo_size to 528 for rk3066 new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 5008a467ce06..b6d7666e715c 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -126,7 +126,7 @@ static const struct dwc2_core_params params_rk3066 = {
 	.speed				= -1,
 	.enable_dynamic_fifo		= 1,
 	.en_multiple_tx_fifo		= -1,
-	.host_rx_fifo_size		= 520,	/* 520 DWORDs */
+	.host_rx_fifo_size		= 528,	/* 528 DWORDs */
 	.host_nperio_tx_fifo_size	= 128,	/* 128 DWORDs */
 	.host_perio_tx_fifo_size	= 256,	/* 256 DWORDs */
 	.max_transfer_size		= -1,
-- 
2.7.0.rc3.207.g0ac5344

[toc] | [prev] | [next] | [standalone]

#1318820 — Re: [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066

Hi Doug,

On 01/23/2016 02:18 AM, Douglas Anderson wrote:
> As documented in dwc2_calculate_dynamic_fifo(), host_rx_fifo_size should
> really be:
>   2 * ((Largest Packet size / 4) + 1 + 1) + n
>   with n = number of host channel.
>
> We have 9 host channels, so
>   2 * ((1024/4) + 2) + 9 = 516 + 9 = 525
>
> We've got 960 / 972 total_fifo_size on rk3288 (and presumably on
> rk3066) and 525 + 128 + 256 = 909 so we're still under on both ports
> even when we increment by 5.
>
> Since we have space, Kever Yang suggests bumping by 8.  He says this
> will meet INCR16 access and next fifo type can start with a aligned
> address.
I have double check this feature, the INCR16 is actually a burst 16
on to the bus, the aligned problem only happen on the DRAM
access, and it's OK for internal RAM access via internal DMA.

Bump to 8 looks much better when we check the FIFO setting if we
have enough memory, right? :)

rk3066/rk3188/rk3288 have the same hw config for dwc2 controller,
so this patch should works on those files.

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
>
> ...so let's bump up by 8.  In the future, it would be nice if
> dwc2_calculate_dynamic_fifo() could handle the "too small" FIFO case and
> come up with something more dynamically.  When we do that we can figure
> out how to allocate the extra 48 / 60 bytes of FIFO that we're currently
> wasting.
>
> NOTE: no known bugs are fixed by this patch, but it seems like a simple
> fix and ought to fix someone.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v5: None
> Changes in v4:
> - Set host_rx_fifo_size to 528 for rk3066 new for v4.
>
> Changes in v3: None
> Changes in v2: None
>
>   drivers/usb/dwc2/platform.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 5008a467ce06..b6d7666e715c 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -126,7 +126,7 @@ static const struct dwc2_core_params params_rk3066 = {
>   	.speed				= -1,
>   	.enable_dynamic_fifo		= 1,
>   	.en_multiple_tx_fifo		= -1,
> -	.host_rx_fifo_size		= 520,	/* 520 DWORDs */
> +	.host_rx_fifo_size		= 528,	/* 528 DWORDs */
>   	.host_nperio_tx_fifo_size	= 128,	/* 128 DWORDs */
>   	.host_perio_tx_fifo_size	= 256,	/* 256 DWORDs */
>   	.max_transfer_size		= -1,

[toc] | [prev] | [next] | [standalone]

#1319537 — Re: [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066

Kever,

On Wed, Jan 27, 2016 at 2:13 AM, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Doug,
>
> On 01/23/2016 02:18 AM, Douglas Anderson wrote:
>>
>> As documented in dwc2_calculate_dynamic_fifo(), host_rx_fifo_size should
>> really be:
>>   2 * ((Largest Packet size / 4) + 1 + 1) + n
>>   with n = number of host channel.
>>
>> We have 9 host channels, so
>>   2 * ((1024/4) + 2) + 9 = 516 + 9 = 525
>>
>> We've got 960 / 972 total_fifo_size on rk3288 (and presumably on
>> rk3066) and 525 + 128 + 256 = 909 so we're still under on both ports
>> even when we increment by 5.
>>
>> Since we have space, Kever Yang suggests bumping by 8.  He says this
>> will meet INCR16 access and next fifo type can start with a aligned
>> address.
>
> I have double check this feature, the INCR16 is actually a burst 16
> on to the bus, the aligned problem only happen on the DRAM
> access, and it's OK for internal RAM access via internal DMA.
>
> Bump to 8 looks much better when we check the FIFO setting if we
> have enough memory, right? :)
>
> rk3066/rk3188/rk3288 have the same hw config for dwc2 controller,
> so this patch should works on those files.
>
> Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks for double-checking and thanks for the review!

If it's all the same to you, I'll probably change it back to 525 and
then increase the periodic FIFO size by 3 DWORDS in the next patch.
12 bytes may not be much, but might as well make use of them to
improve performance / compatibility?

Presumably you're also OK with the next patch in the series: ("usb:
dwc2: host: Set host_perio_tx_fifo_size to 304 for rk3066")?


Note to anyone who may be thinking of applying patches in this series:

* You can skip both this and the next patch if you wish and any other
patches in the series are unaffected.

* You can request that I spin just these two patches and I will do it
ASAP.  Otherwise I'll add that fix to the next spin of the whole
series.

* You can make the changes yourself to this patch and the next one
(this patch should be 525 and the next should increase
host_perio_tx_fifo_size to 307).

I'm OK with any of those 3 things.  ;)


-Doug

[toc] | [prev] | [next] | [standalone]

#1315209 — [PATCH v5 14/21] usb: dwc2: host: Split code out to make dwc2_do_reserve()

This no-op change splits code out of dwc2_schedule_periodic() into a
dwc2_do_reserve() function.  This makes it a little easier to follow the
logic.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4:
- Split code out to make dwc2_do_reserve() new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd_queue.c | 112 ++++++++++++++++++++++++++-----------------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 8a2067bc1e62..9ce407e5017d 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -245,6 +245,70 @@ static int dwc2_find_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 }
 
 /**
+ * dwc2_do_reserve() - Make a periodic reservation
+ *
+ * Try to allocate space in the periodic schedule.  Depending on parameters
+ * this might use the microframe scheduler or the dumb scheduler.
+ *
+ * @hsotg: The HCD state structure for the DWC OTG controller
+ * @qh:    QH for the periodic transfer.
+ *
+ * Returns: 0 upon success; error upon failure.
+ */
+static int dwc2_do_reserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
+{
+	int status;
+
+	if (hsotg->core_params->uframe_sched > 0) {
+		int frame = -1;
+
+		status = dwc2_find_uframe(hsotg, qh);
+		if (status == 0)
+			frame = 7;
+		else if (status > 0)
+			frame = status - 1;
+
+		/* Set the new frame up */
+		if (frame >= 0) {
+			qh->next_active_frame &= ~0x7;
+			qh->next_active_frame |= (frame & 7);
+			dwc2_sch_dbg(hsotg,
+				     "QH=%p sched_p nxt=%04x, uf=%d\n",
+				     qh, qh->next_active_frame, frame);
+		}
+
+		if (status > 0)
+			status = 0;
+	} else {
+		status = dwc2_periodic_channel_available(hsotg);
+		if (status) {
+			dev_info(hsotg->dev,
+				 "%s: No host channel available for periodic transfer\n",
+				 __func__);
+			return status;
+		}
+
+		status = dwc2_check_periodic_bandwidth(hsotg, qh);
+	}
+
+	if (status) {
+		dev_dbg(hsotg->dev,
+			"%s: Insufficient periodic bandwidth for periodic transfer\n",
+			__func__);
+		return status;
+	}
+
+	if (hsotg->core_params->uframe_sched <= 0)
+		/* Reserve periodic channel */
+		hsotg->periodic_channels++;
+
+	/* Update claimed usecs per (micro)frame */
+	hsotg->periodic_usecs += qh->host_us;
+
+	return 0;
+}
+
+/**
  * dwc2_do_unreserve() - Actually release the periodic reservation
  *
  * This function actually releases the periodic bandwidth that was reserved
@@ -393,51 +457,9 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 	 * that case.
 	 */
 	if (!qh->unreserve_pending) {
-		if (hsotg->core_params->uframe_sched > 0) {
-			int frame = -1;
-
-			status = dwc2_find_uframe(hsotg, qh);
-			if (status == 0)
-				frame = 7;
-			else if (status > 0)
-				frame = status - 1;
-
-			/* Set the new frame up */
-			if (frame >= 0) {
-				qh->next_active_frame &= ~0x7;
-				qh->next_active_frame |= (frame & 7);
-				dwc2_sch_dbg(hsotg,
-					     "QH=%p sched_p nxt=%04x, uf=%d\n",
-					     qh, qh->next_active_frame, frame);
-			}
-
-			if (status > 0)
-				status = 0;
-		} else {
-			status = dwc2_periodic_channel_available(hsotg);
-			if (status) {
-				dev_info(hsotg->dev,
-					"%s: No host channel available for periodic transfer\n",
-					__func__);
-				return status;
-			}
-
-			status = dwc2_check_periodic_bandwidth(hsotg, qh);
-		}
-
-		if (status) {
-			dev_dbg(hsotg->dev,
-				"%s: Insufficient periodic bandwidth for periodic transfer\n",
-				__func__);
+		status = dwc2_do_reserve(hsotg, qh);
+		if (status)
 			return status;
-		}
-
-		if (hsotg->core_params->uframe_sched <= 0)
-			/* Reserve periodic channel */
-			hsotg->periodic_channels++;
-
-		/* Update claimed usecs per (micro)frame */
-		hsotg->periodic_usecs += qh->host_us;
 	}
 
 	qh->unreserve_pending = 0;
@@ -450,7 +472,7 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 		list_add_tail(&qh->qh_list_entry,
 			      &hsotg->periodic_sched_inactive);
 
-	return status;
+	return 0;
 }
 
 /**
-- 
2.7.0.rc3.207.g0ac5344

[toc] | [prev] | [next] | [standalone]

#1315210 — [PATCH v5 16/21] usb: dwc2: host: Manage frame nums better in scheduler

The dwc2 scheduler (contained in hcd_queue.c) was a bit confusing in the
way it initted / kept track of which frames a QH was going to be active
in.  Let's clean things up a little bit in preparation for a rewrite of
the microframe scheduler.

Specifically:
* Old code would pick a frame number in dwc2_qh_init() and would try to
  pick it "in a slightly future (micro)frame".  As far as I can tell the
  reason for this was that there was a delay between dwc2_qh_init() and
  when we actually wanted to dwc2_hcd_qh_add().  ...but apparently this
  attempt to be slightly in the future wasn't enough because
  dwc2_hcd_qh_add() then had code to reset things if the frame _wasn't_
  in the future.  There's no reason not to just pick the frame later.
  For non-periodic QH we now pick the frame in dwc2_hcd_qh_add().  For
  periodic QH we pick the frame at dwc2_schedule_periodic() time.
* The old "dwc2_qh_init() actually assigned to "hsotg->frame_number".
  This doesn't seem like a great idea since that variable is supposed to
  be used to keep track of which SOF the interrupt handler has seen.
  Let's be clean: anyone who wants the current frame number (instead of
  the one as of the last interrupt) should ask for it.
* The old code wasn't terribly consistent about trying to use the frame
  that the microframe scheduler assigned to it.  In
  dwc2_sched_periodic_split() when it was scheduling the first frame it
  always "ORed" in 0x7 (!).  Since the frame goes on the wire 1 uFrame
  after next_active_frame it meant that the SSPLIT would always try for
  uFrame 0 and the transaction would happen on the low speed bus during
  uFrame 1.  This is irregardless of what the microframe scheduler
  said.
* The old code assumed it would get called to schedule the next in a
  periodic split very quickly.  That is if next_active_frame was
  0 (transfer on wire in uFrame 1) it assumed it was getting called to
  schedule the next uFrame during uFrame 1 too (so it could queue
  something up for uFrame 2).  It should be possible to actually queue
  something up for uFrame 2 while in uFrame 2 (AKA queue up ASAP).  To
  do this, code needs to look at the previously scheduled frame when
  deciding when to next be active, not look at the current frame number.
* If there was no microframe scheduler, the old code would check for
  whether we should be active using "qh->next_active_frame ==
  frame_number".  This seemed like a race waiting to happen.  ...plus
  there's no way that you wouldn't want to schedule if next_active_frame
  was actually less than frame number.

Note that this change doesn't make 100% sense on its own since it's
expecting some sanity in the frame numbers assigned by the microframe
scheduler and (as per the future patch which rewries it) I think that
the current microframe scheduler is quite insane.  However, it seems
like splitting this up from the microframe scheduler patch makes things
into smaller chunks and hopefully adds to clarity rather than reduces
it.  The two patches could certainly be squashed.  Not that in the very
least, I don't see any obvious bad behavior introduced with just this
patch.

I've attempted to keep the config parameter to disable the microframe
scheduler in tact in this change, though I'm not sure it's worth it.
Obviously the code is touched a lot so it's possible I regressed
something when the microframe scheduler is disabled, though I did some
basic testing and it seemed to work OK.  I'm still not 100% sure why you
wouldn't want the microframe scheduler (presuming it works), so maybe a
future patch (or a future version of this patch?) could remove that
parameter.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4:
- Manage frame nums better in scheduler new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd.h       |  10 +-
 drivers/usb/dwc2/hcd_queue.c | 355 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 273 insertions(+), 92 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 10c35585a2bd..fd266ac53a28 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -244,8 +244,11 @@ enum dwc2_transaction_type {
  *                      the bus.  We'll move the qh to active here.  If the
  *                      host is in high speed mode this will be a uframe.  If
  *                      the host is in low speed mode this will be a full frame.
+ * @start_active_frame: If we are partway through a split transfer, this will be
+ *			what next_active_frame was when we started.  Otherwise
+ *			it should always be the same as next_active_frame.
+ * @assigned_uframe:    The uframe (0 -7) assigned by dwc2_find_uframe().
  * @frame_usecs:        Internal variable used by the microframe scheduler
- * @start_split_frame:  (Micro)frame at which last start split was initialized
  * @ntd:                Actual number of transfer descriptors in a list
  * @qtd_list:           List of QTDs for this QH
  * @channel:            Host channel currently processing transfers for this QH
@@ -279,8 +282,9 @@ struct dwc2_qh {
 	u16 host_us;
 	u16 host_interval;
 	u16 next_active_frame;
+	u16 start_active_frame;
+	u16 assigned_uframe;
 	u16 frame_usecs[8];
-	u16 start_split_frame;
 	u16 ntd;
 	struct list_head qtd_list;
 	struct dwc2_host_chan *channel;
@@ -746,7 +750,7 @@ do {									\
 	_qtd_ = list_entry((_qh_)->qtd_list.next, struct dwc2_qtd,	\
 			   qtd_list_entry);				\
 	if (usb_pipeint(_qtd_->urb->pipe) &&				\
-	    (_qh_)->start_split_frame != 0 && !_qtd_->complete_split) {	\
+	    (_qh_)->start_active_frame != 0 && !_qtd_->complete_split) { \
 		_hfnum_.d32 = dwc2_readl((_hcd_)->regs + HFNUM);	\
 		switch (_hfnum_.b.frnum & 0x7) {			\
 		case 7:							\
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 9ce407e5017d..f235402dfc5e 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -38,6 +38,7 @@
  * This file contains the functions to manage Queue Heads and Queue
  * Transfer Descriptors for Host mode
  */
+#include <linux/gcd.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
@@ -245,6 +246,96 @@ static int dwc2_find_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 }
 
 /**
+ * dwc2_pick_first_frame() - Choose 1st frame for qh that's already scheduled
+ *
+ * Takes a qh that has already been scheduled (which means we know we have the
+ * bandwdith reserved for us) and set the next_active_frame and the
+ * start_active_frame.
+ *
+ * This is expected to be called on qh's that weren't previously actively
+ * running.  It just picks the next frame that we can fit into without any
+ * thought about the past.
+ *
+ * @hsotg: The HCD state structure for the DWC OTG controller
+ * @qh:    QH for a periodic endpoint
+ *
+ */
+static void dwc2_pick_first_frame(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
+{
+	u16 frame_number;
+	u16 earliest_frame;
+	u16 next_active_frame;
+	u16 interval;
+
+	/*
+	 * Use the real frame number rather than the cached value as of the
+	 * last SOF to give us a little extra slop.
+	 */
+	frame_number = dwc2_hcd_get_frame_number(hsotg);
+
+	/*
+	 * We wouldn't want to start any earlier than the next frame just in
+	 * case the frame number ticks as we're doing this calculation.
+	 *
+	 * NOTE: if we could quantify how long till we actually get scheduled
+	 * we might be able to avoid the "+ 1" by looking at the upper part of
+	 * HFNUM (the FRREM field).  For now we'll just use the + 1 though.
+	 */
+	earliest_frame = dwc2_frame_num_inc(frame_number, 1);
+	next_active_frame = earliest_frame;
+
+	/* Get the "no microframe schduler" out of the way... */
+	if (hsotg->core_params->uframe_sched <= 0) {
+		if (qh->do_split)
+			/* Splits are active at microframe 0 minus 1 */
+			next_active_frame |= 0x7;
+		goto exit;
+	}
+
+	/* Adjust interval as per high speed schedule which has 8 uFrame */
+	interval = gcd(qh->host_interval, 8);
+
+	/*
+	 * We know interval must divide (HFNUM_MAX_FRNUM + 1) now that we've
+	 * done the gcd(), so it's safe to move to the beginning of the current
+	 * interval like this.
+	 *
+	 * After this we might be before earliest_frame, but don't worry,
+	 * we'll fix it...
+	 */
+	next_active_frame = (next_active_frame / interval) * interval;
+
+	/*
+	 * Actually choose to start at the frame number we've been
+	 * scheduled for.
+	 */
+	next_active_frame = dwc2_frame_num_inc(next_active_frame,
+					       qh->assigned_uframe);
+
+	/*
+	 * We actually need 1 frame before since the next_active_frame is
+	 * the frame number we'll be put on the ready list and we won't be on
+	 * the bus until 1 frame later.
+	 */
+	next_active_frame = dwc2_frame_num_dec(next_active_frame, 1);
+
+	/*
+	 * By now we might actually be before the earliest_frame.  Let's move
+	 * up intervals until we're not.
+	 */
+	while (dwc2_frame_num_gt(earliest_frame, next_active_frame))
+		next_active_frame = dwc2_frame_num_inc(next_active_frame,
+						       interval);
+
+exit:
+	qh->next_active_frame = next_active_frame;
+	qh->start_active_frame = next_active_frame;
+
+	dwc2_sch_vdbg(hsotg, "QH=%p First fn=%04x nxt=%04x\n",
+		     qh, frame_number, qh->next_active_frame);
+}
+
+/**
  * dwc2_do_reserve() - Make a periodic reservation
  *
  * Try to allocate space in the periodic schedule.  Depending on parameters
@@ -260,25 +351,9 @@ static int dwc2_do_reserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 	int status;
 
 	if (hsotg->core_params->uframe_sched > 0) {
-		int frame = -1;
-
 		status = dwc2_find_uframe(hsotg, qh);
-		if (status == 0)
-			frame = 7;
-		else if (status > 0)
-			frame = status - 1;
-
-		/* Set the new frame up */
-		if (frame >= 0) {
-			qh->next_active_frame &= ~0x7;
-			qh->next_active_frame |= (frame & 7);
-			dwc2_sch_dbg(hsotg,
-				     "QH=%p sched_p nxt=%04x, uf=%d\n",
-				     qh, qh->next_active_frame, frame);
-		}
-
-		if (status > 0)
-			status = 0;
+		if (status >= 0)
+			qh->assigned_uframe = status;
 	} else {
 		status = dwc2_periodic_channel_available(hsotg);
 		if (status) {
@@ -305,6 +380,8 @@ static int dwc2_do_reserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 	/* Update claimed usecs per (micro)frame */
 	hsotg->periodic_usecs += qh->host_us;
 
+	dwc2_pick_first_frame(hsotg, qh);
+
 	return 0;
 }
 
@@ -460,6 +537,14 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 		status = dwc2_do_reserve(hsotg, qh);
 		if (status)
 			return status;
+	} else {
+		/*
+		 * It might have been a while, so make sure that frame_number
+		 * is still good.  Note: we could also try to use the similar
+		 * dwc2_next_periodic_start() but that schedules much more
+		 * tightly and we might need to hurry and queue things up.
+		 */
+		dwc2_pick_first_frame(hsotg, qh);
 	}
 
 	qh->unreserve_pending = 0;
@@ -520,7 +605,6 @@ static void dwc2_deschedule_periodic(struct dwc2_hsotg *hsotg,
  * @urb:   Holds the information about the device/endpoint needed to initialize
  *         the QH
  */
-#define SCHEDULE_SLOP 10
 static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
 			 struct dwc2_hcd_urb *urb)
 {
@@ -569,11 +653,6 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
 			      qh->ep_type == USB_ENDPOINT_XFER_ISOC,
 			      bytecount));
 
-		/* Ensure frame_number corresponds to the reality */
-		hsotg->frame_number = dwc2_hcd_get_frame_number(hsotg);
-		/* Start in a slightly future (micro)frame */
-		qh->next_active_frame = dwc2_frame_num_inc(hsotg->frame_number,
-						     SCHEDULE_SLOP);
 		qh->host_interval = urb->interval;
 		dwc2_sch_dbg(hsotg, "QH=%p init nxt=%04x, fn=%04x, int=%#x\n",
 			     qh, qh->next_active_frame, hsotg->frame_number,
@@ -589,8 +668,6 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
 		    (dev_speed == USB_SPEED_LOW ||
 		     dev_speed == USB_SPEED_FULL)) {
 			qh->host_interval *= 8;
-			qh->next_active_frame |= 0x7;
-			qh->start_split_frame = qh->next_active_frame;
 			dwc2_sch_dbg(hsotg,
 				     "QH=%p init*8 nxt=%04x, fn=%04x, int=%#x\n",
 				     qh, qh->next_active_frame,
@@ -738,22 +815,12 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 		/* QH already in a schedule */
 		return 0;
 
-	if (!dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number) &&
-			!hsotg->frame_number) {
-		u16 new_frame;
-
-		dev_dbg(hsotg->dev,
-				"reset frame number counter\n");
-		new_frame = dwc2_frame_num_inc(hsotg->frame_number,
-				SCHEDULE_SLOP);
-
-		dwc2_sch_vdbg(hsotg, "QH=%p reset nxt=%04x=>%04x\n",
-			      qh, qh->next_active_frame, new_frame);
-		qh->next_active_frame = new_frame;
-	}
-
 	/* Add the new QH to the appropriate schedule */
 	if (dwc2_qh_is_non_per(qh)) {
+		/* Schedule right away */
+		qh->start_active_frame = hsotg->frame_number;
+		qh->next_active_frame = qh->start_active_frame;
+
 		/* Always start in inactive schedule */
 		list_add_tail(&qh->qh_list_entry,
 			      &hsotg->non_periodic_sched_inactive);
@@ -807,46 +874,145 @@ void dwc2_hcd_qh_unlink(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 	}
 }
 
-/*
- * Schedule the next continuing periodic split transfer
+/**
+ * dwc2_next_for_periodic_split() - Set next_active_frame midway thru a split.
+ *
+ * This is called for setting next_active_frame for periodic splits for all but
+ * the first packet of the split.  Confusing?  I thought so...
+ *
+ * Periodic splits are single low/full speed transfers that we end up splitting
+ * up into several high speed transfers.  They always fit into one full (1 ms)
+ * frame but might be split over several microframes (125 us each).  We to put
+ * each of the parts on a very specific high speed frame.
+ *
+ * This function figures out where the next active uFrame needs to be.
+ *
+ * @hsotg:        The HCD state structure
+ * @qh:           QH for the periodic transfer.
+ * @frame_number: The current frame number.
+ *
+ * Return: number missed by (or 0 if we didn't miss).
  */
-static void dwc2_sched_periodic_split(struct dwc2_hsotg *hsotg,
-				      struct dwc2_qh *qh, u16 frame_number,
-				      int sched_next_periodic_split)
+static int dwc2_next_for_periodic_split(struct dwc2_hsotg *hsotg,
+					 struct dwc2_qh *qh, u16 frame_number)
 {
-	u16 incr;
 	u16 old_frame = qh->next_active_frame;
+	u16 prev_frame_number = dwc2_frame_num_dec(frame_number, 1);
+	int missed = 0;
+	u16 incr;
+
+	/*
+	 * Basically: increment 1 normally, but 2 right after the start split
+	 * (except for ISOC out).
+	 */
+	if (old_frame == qh->start_active_frame &&
+	    !(qh->ep_type == USB_ENDPOINT_XFER_ISOC && !qh->ep_is_in))
+		incr = 2;
+	else
+		incr = 1;
+
+	qh->next_active_frame = dwc2_frame_num_inc(old_frame, incr);
 
-	if (sched_next_periodic_split) {
+	/*
+	 * Note that it's OK for frame_number to be 1 frame past
+	 * next_active_frame.  Remember that next_active_frame is supposed to
+	 * be 1 frame _before_ when we want to be scheduled.  If we're 1 frame
+	 * past it just means schedule ASAP.
+	 *
+	 * It's _not_ OK, however, if we're more than one frame past.
+	 */
+	if (dwc2_frame_num_gt(prev_frame_number, qh->next_active_frame)) {
+		/*
+		 * OOPS, we missed.  That's actually pretty bad since
+		 * the hub will be unhappy; try ASAP I guess.
+		 */
+		missed = dwc2_frame_num_dec(prev_frame_number,
+					    qh->next_active_frame);
 		qh->next_active_frame = frame_number;
-		incr = dwc2_frame_num_inc(qh->start_split_frame, 1);
-		if (dwc2_frame_num_le(frame_number, incr)) {
-			/*
-			 * Allow one frame to elapse after start split
-			 * microframe before scheduling complete split, but
-			 * DON'T if we are doing the next start split in the
-			 * same frame for an ISOC out
-			 */
-			if (qh->ep_type != USB_ENDPOINT_XFER_ISOC ||
-			    qh->ep_is_in != 0) {
-				qh->next_active_frame = dwc2_frame_num_inc(
-					qh->next_active_frame, 1);
-			}
-		}
-	} else {
-		qh->next_active_frame =
-			dwc2_frame_num_inc(qh->start_split_frame,
-					   qh->host_interval);
-		if (dwc2_frame_num_le(qh->next_active_frame, frame_number))
-			qh->next_active_frame = frame_number;
-		qh->next_active_frame |= 0x7;
-		qh->start_split_frame = qh->next_active_frame;
 	}
 
-	dwc2_sch_vdbg(hsotg, "QH=%p next(%d) fn=%04x, nxt=%04x=>%04x (%+d)\n",
-		      qh, sched_next_periodic_split, frame_number, old_frame,
-		      qh->next_active_frame,
-		      dwc2_frame_num_dec(qh->next_active_frame, old_frame));
+	return missed;
+}
+
+/**
+ * dwc2_next_periodic_start() - Set next_active_frame for next transfer start
+ *
+ * This is called for setting next_active_frame for a periodic transfer for
+ * all cases other than midway through a periodic split.  This will also update
+ * start_active_frame.
+ *
+ * Since we _always_ keep start_active_frame as the start of the previous
+ * transfer this is normally pretty easy: we just add our interval to
+ * start_active_frame and we've got our answer.
+ *
+ * The tricks come into play if we miss.  In that case we'll look for the next
+ * slot we can fit into.
+ *
+ * @hsotg:        The HCD state structure
+ * @qh:           QH for the periodic transfer.
+ * @frame_number: The current frame number.
+ *
+ * Return: number missed by (or 0 if we didn't miss).
+ */
+static int dwc2_next_periodic_start(struct dwc2_hsotg *hsotg,
+				     struct dwc2_qh *qh, u16 frame_number)
+{
+	int missed = 0;
+	u16 interval = qh->host_interval;
+	u16 prev_frame_number = dwc2_frame_num_dec(frame_number, 1);
+
+	qh->start_active_frame = dwc2_frame_num_inc(qh->start_active_frame,
+						    interval);
+
+	/*
+	 * The dwc2_frame_num_gt() function used below won't work terribly well
+	 * with if we just incremented by a really large intervals since the
+	 * frame counter only goes to 0x3fff.  It's terribly unlikely that we
+	 * will have missed in this case anyway.  Just go to exit.  If we want
+	 * to try to do better we'll need to keep track of a bigger counter
+	 * somewhere in the driver and handle overflows.
+	 */
+	if (interval >= 0x1000)
+		goto exit;
+
+	/*
+	 * Test for misses, which is when it's too late to schedule.
+	 *
+	 * A few things to note:
+	 * - We compare against prev_frame_number since start_active_frame
+	 *   and next_active_frame are always 1 frame before we want things
+	 *   to be active and we assume we can still get scheduled in the
+	 *   current frame number.
+	 * - Some misses are expected.  Specifically, in order to work
+	 *   perfectly dwc2 really needs quite spectacular interrupt latency
+	 *   requirements.  It needs to be able to handle its interrupts
+	 *   completely within 125 us of them being asserted. That not only
+	 *   means that the dwc2 interrupt handler needs to be fast but it
+	 *   means that nothing else in the system has to block dwc2 for a long
+	 *   time.  We can help with the dwc2 parts of this, but it's hard to
+	 *   guarantee that a system will have interrupt latency < 125 us, so
+	 *   we have to be robust to some misses.
+	 */
+	if (dwc2_frame_num_gt(prev_frame_number, qh->start_active_frame)) {
+		u16 ideal_start = qh->start_active_frame;
+
+		/* Adjust interval as per gcd with plan length. */
+		interval = gcd(interval, 8);
+
+		do {
+			qh->start_active_frame = dwc2_frame_num_inc(
+				qh->start_active_frame, interval);
+		} while (dwc2_frame_num_gt(prev_frame_number,
+					   qh->start_active_frame));
+
+		missed = dwc2_frame_num_dec(qh->start_active_frame,
+					    ideal_start);
+	}
+
+exit:
+	qh->next_active_frame = qh->start_active_frame;
+
+	return missed;
 }
 
 /*
@@ -865,7 +1031,9 @@ static void dwc2_sched_periodic_split(struct dwc2_hsotg *hsotg,
 void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
 			    int sched_next_periodic_split)
 {
+	u16 old_frame = qh->next_active_frame;
 	u16 frame_number;
+	int missed;
 
 	if (dbg_qh(qh))
 		dev_vdbg(hsotg->dev, "%s()\n", __func__);
@@ -878,30 +1046,39 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
 		return;
 	}
 
-	frame_number = dwc2_hcd_get_frame_number(hsotg);
-
-	if (qh->do_split) {
-		dwc2_sched_periodic_split(hsotg, qh, frame_number,
-					  sched_next_periodic_split);
-	} else {
-		qh->next_active_frame = dwc2_frame_num_inc(
-			qh->next_active_frame, qh->host_interval);
-		if (dwc2_frame_num_le(qh->next_active_frame, frame_number))
-			qh->next_active_frame = frame_number;
-	}
-
 	if (list_empty(&qh->qtd_list)) {
 		dwc2_hcd_qh_unlink(hsotg, qh);
 		return;
 	}
+
+	/*
+	 * Use the real frame number rather than the cached value as of the
+	 * last SOF just to get us a little closer to reality.  Note that
+	 * means we don't actually know if we've already handled the SOF
+	 * interrupt for this frame.
+	 */
+	frame_number = dwc2_hcd_get_frame_number(hsotg);
+
+	if (sched_next_periodic_split)
+		missed = dwc2_next_for_periodic_split(hsotg, qh, frame_number);
+	else
+		missed = dwc2_next_periodic_start(hsotg, qh, frame_number);
+
+	dwc2_sch_vdbg(hsotg,
+		     "QH=%p next(%d) fn=%04x, sch=%04x=>%04x (%+d) miss=%d %s\n",
+		     qh, sched_next_periodic_split, frame_number, old_frame,
+		     qh->next_active_frame,
+		     dwc2_frame_num_dec(qh->next_active_frame, old_frame),
+		missed, missed ? "MISS" : "");
+
 	/*
 	 * Remove from periodic_sched_queued and move to
 	 * appropriate queue
+	 *
+	 * Note: we purposely use the frame_number from the "hsotg" structure
+	 * since we know SOF interrupt will handle future frames.
 	 */
-	if ((hsotg->core_params->uframe_sched > 0 &&
-	     dwc2_frame_num_le(qh->next_active_frame, frame_number)) ||
-	    (hsotg->core_params->uframe_sched <= 0 &&
-	     qh->next_active_frame == frame_number))
+	if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
 		list_move_tail(&qh->qh_list_entry,
 			       &hsotg->periodic_sched_ready);
 	else
-- 
2.7.0.rc3.207.g0ac5344

[toc] | [prev] | [next] | [standalone]

#1319777 — Re: [PATCH v5 16/21] usb: dwc2: host: Manage frame nums better in scheduler

Hi,

On Fri, Jan 22, 2016 at 10:18 AM, Douglas Anderson
<dianders@chromium.org> wrote:
> The dwc2 scheduler (contained in hcd_queue.c) was a bit confusing in the
> way it initted / kept track of which frames a QH was going to be active
> in.  Let's clean things up a little bit in preparation for a rewrite of
> the microframe scheduler.
>
> Specifically:
> * Old code would pick a frame number in dwc2_qh_init() and would try to
>   pick it "in a slightly future (micro)frame".  As far as I can tell the
>   reason for this was that there was a delay between dwc2_qh_init() and
>   when we actually wanted to dwc2_hcd_qh_add().  ...but apparently this
>   attempt to be slightly in the future wasn't enough because
>   dwc2_hcd_qh_add() then had code to reset things if the frame _wasn't_
>   in the future.  There's no reason not to just pick the frame later.
>   For non-periodic QH we now pick the frame in dwc2_hcd_qh_add().  For
>   periodic QH we pick the frame at dwc2_schedule_periodic() time.
> * The old "dwc2_qh_init() actually assigned to "hsotg->frame_number".
>   This doesn't seem like a great idea since that variable is supposed to
>   be used to keep track of which SOF the interrupt handler has seen.
>   Let's be clean: anyone who wants the current frame number (instead of
>   the one as of the last interrupt) should ask for it.
> * The old code wasn't terribly consistent about trying to use the frame
>   that the microframe scheduler assigned to it.  In
>   dwc2_sched_periodic_split() when it was scheduling the first frame it
>   always "ORed" in 0x7 (!).  Since the frame goes on the wire 1 uFrame
>   after next_active_frame it meant that the SSPLIT would always try for
>   uFrame 0 and the transaction would happen on the low speed bus during
>   uFrame 1.  This is irregardless of what the microframe scheduler
>   said.
> * The old code assumed it would get called to schedule the next in a
>   periodic split very quickly.  That is if next_active_frame was
>   0 (transfer on wire in uFrame 1) it assumed it was getting called to
>   schedule the next uFrame during uFrame 1 too (so it could queue
>   something up for uFrame 2).  It should be possible to actually queue
>   something up for uFrame 2 while in uFrame 2 (AKA queue up ASAP).  To
>   do this, code needs to look at the previously scheduled frame when
>   deciding when to next be active, not look at the current frame number.
> * If there was no microframe scheduler, the old code would check for
>   whether we should be active using "qh->next_active_frame ==
>   frame_number".  This seemed like a race waiting to happen.  ...plus
>   there's no way that you wouldn't want to schedule if next_active_frame
>   was actually less than frame number.
>
> Note that this change doesn't make 100% sense on its own since it's
> expecting some sanity in the frame numbers assigned by the microframe
> scheduler and (as per the future patch which rewries it) I think that
> the current microframe scheduler is quite insane.  However, it seems
> like splitting this up from the microframe scheduler patch makes things
> into smaller chunks and hopefully adds to clarity rather than reduces
> it.  The two patches could certainly be squashed.  Not that in the very
> least, I don't see any obvious bad behavior introduced with just this
> patch.
>
> I've attempted to keep the config parameter to disable the microframe
> scheduler in tact in this change, though I'm not sure it's worth it.
> Obviously the code is touched a lot so it's possible I regressed
> something when the microframe scheduler is disabled, though I did some
> basic testing and it seemed to work OK.  I'm still not 100% sure why you
> wouldn't want the microframe scheduler (presuming it works), so maybe a
> future patch (or a future version of this patch?) could remove that
> parameter.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v5: None
> Changes in v4:
> - Manage frame nums better in scheduler new for v4.
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/usb/dwc2/hcd.h       |  10 +-
>  drivers/usb/dwc2/hcd_queue.c | 355 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 273 insertions(+), 92 deletions(-)

As an FYI to anyone considering reviewing or applying this particular patch.

It's proposed to squash
<https://chromium-review.googlesource.com/#/c/324185/> as a fixup to
this patch for the next version.  This is intended to fix problems
that Alan Stern reported at
<http://www.spinics.net/lists/linux-usb/msg135678.html>

[toc] | [prev] | [next] | [standalone]

#1315211 — [PATCH v5 11/21] usb: dwc2: host: Use periodic interrupt even with DMA

The old code in dwc2_process_periodic_channels() would only enable the
"periodic empty" interrupt if we weren't using DMA.  That wasn't right
since we can still get into cases where we have small FIFOs even on
systems that have DMA (the rk3288 is a prime example).

Let's always enable/disable the "periodic empty" when appropriate.  As
part of this:

* Always call dwc2_process_periodic_channels() even if there's nothing
  in periodic_sched_assigned (we move the queue empty check so we still
  avoid the extra work).  That will make extra certain that we will
  properly disable the "periodic empty" interrupt even if there's
  nothing queued up.

* Move the enable of "periodic empty" due to non-empty
  periodic_sched_assigned to be for slave mode (non-DMA mode) only.
  Presumably this was the original intention of the check for DMA since
  it seems to match the comments above where in slave mode we leave
  things on the assigned queue.

Note that even before this change slave mode didn't work for me, so I
can't say for sure that my understanding of slave mode is correct.
However, this shouldn't change anything for slave mode so if slave mode
worked for someone in the past it ought to still work.

With this change, I no longer get constant misses reported by my other
debugging code (and with future patches) when I've got:
* Rockchip rk3288 Chromebook, using port ff540000
  -> Pluggable 7-port Hub with Charging (powered)
     -> Microsoft Wireless Keyboard 2000 in port 1.
     -> Das Keyboard in port 2.
     -> Jabra Speaker in port 3
     -> Logitech, Inc. Webcam C600 in port 4
     -> Microsoft Sidewinder X6 Keyboard in port 5

...and I'm playing music on the USB speaker and capturing video from the
webcam.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4:
- Use periodic interrupt even with DMA new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd.c | 71 +++++++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 9fd14b74b49c..584c64114a82 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1109,10 +1109,14 @@ static void dwc2_process_periodic_channels(struct dwc2_hsotg *hsotg)
 	u32 fspcavail;
 	u32 gintmsk;
 	int status;
-	int no_queue_space = 0;
-	int no_fifo_space = 0;
+	bool no_queue_space = false;
+	bool no_fifo_space = false;
 	u32 qspcavail;
 
+	/* If empty list then just adjust interrupt enables */
+	if (list_empty(&hsotg->periodic_sched_assigned))
+		goto exit;
+
 	if (dbg_perio())
 		dev_vdbg(hsotg->dev, "Queue periodic transactions\n");
 
@@ -1190,42 +1194,32 @@ static void dwc2_process_periodic_channels(struct dwc2_hsotg *hsotg)
 		}
 	}
 
-	if (hsotg->core_params->dma_enable <= 0) {
-		tx_status = dwc2_readl(hsotg->regs + HPTXSTS);
-		qspcavail = (tx_status & TXSTS_QSPCAVAIL_MASK) >>
-			    TXSTS_QSPCAVAIL_SHIFT;
-		fspcavail = (tx_status & TXSTS_FSPCAVAIL_MASK) >>
-			    TXSTS_FSPCAVAIL_SHIFT;
-		if (dbg_perio()) {
-			dev_vdbg(hsotg->dev,
-				 "  P Tx Req Queue Space Avail (after queue): %d\n",
-				 qspcavail);
-			dev_vdbg(hsotg->dev,
-				 "  P Tx FIFO Space Avail (after queue): %d\n",
-				 fspcavail);
-		}
-
-		if (!list_empty(&hsotg->periodic_sched_assigned) ||
-		    no_queue_space || no_fifo_space) {
-			/*
-			 * May need to queue more transactions as the request
-			 * queue or Tx FIFO empties. Enable the periodic Tx
-			 * FIFO empty interrupt. (Always use the half-empty
-			 * level to ensure that new requests are loaded as
-			 * soon as possible.)
-			 */
-			gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+exit:
+	if (no_queue_space || no_fifo_space ||
+	    (hsotg->core_params->dma_enable <= 0 &&
+	     !list_empty(&hsotg->periodic_sched_assigned))) {
+		/*
+		 * May need to queue more transactions as the request
+		 * queue or Tx FIFO empties. Enable the periodic Tx
+		 * FIFO empty interrupt. (Always use the half-empty
+		 * level to ensure that new requests are loaded as
+		 * soon as possible.)
+		 */
+		gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+		if (!(gintmsk & GINTSTS_PTXFEMP)) {
 			gintmsk |= GINTSTS_PTXFEMP;
 			dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
-		} else {
-			/*
-			 * Disable the Tx FIFO empty interrupt since there are
-			 * no more transactions that need to be queued right
-			 * now. This function is called from interrupt
-			 * handlers to queue more transactions as transfer
-			 * states change.
-			 */
-			gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+		}
+	} else {
+		/*
+		 * Disable the Tx FIFO empty interrupt since there are
+		 * no more transactions that need to be queued right
+		 * now. This function is called from interrupt
+		 * handlers to queue more transactions as transfer
+		 * states change.
+		*/
+		gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+		if (gintmsk & GINTSTS_PTXFEMP) {
 			gintmsk &= ~GINTSTS_PTXFEMP;
 			dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
 		}
@@ -1372,9 +1366,8 @@ void dwc2_hcd_queue_transactions(struct dwc2_hsotg *hsotg,
 	dev_vdbg(hsotg->dev, "Queue Transactions\n");
 #endif
 	/* Process host channels associated with periodic transfers */
-	if ((tr_type == DWC2_TRANSACTION_PERIODIC ||
-	     tr_type == DWC2_TRANSACTION_ALL) &&
-	    !list_empty(&hsotg->periodic_sched_assigned))
+	if (tr_type == DWC2_TRANSACTION_PERIODIC ||
+	    tr_type == DWC2_TRANSACTION_ALL)
 		dwc2_process_periodic_channels(hsotg);
 
 	/* Process host channels associated with non-periodic transfers */
-- 
2.7.0.rc3.207.g0ac5344

[toc] | [prev] | [next] | [standalone]

#1315212 — [PATCH v5 05/21] usb: dwc2: host: Avoid use of chan->qh after qh freed

When poking around with USB devices with slub_debug enabled, I found
another obvious use after free.  Turns out that in dwc2_hc_n_intr() I
was in a state when the contents of chan->qh was filled with 0x6b,
indicating that chan->qh was freed but chan still had a reference to
it.

Let's make sure that whenever we free qh we also make sure we remove a
reference from its channel.

The bug fixed here doesn't appear to be new--I believe I just got lucky
and happened to see it while stress testing.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4:
- Avoid use of chan->qh after qh freed new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd.c      |  8 ++++++++
 drivers/usb/dwc2/hcd_intr.c | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index bc4bdbc1534e..7783c8ba0173 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -164,6 +164,9 @@ static void dwc2_qh_list_free(struct dwc2_hsotg *hsotg,
 					 qtd_list_entry)
 			dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
 
+		if (qh->channel && qh->channel->qh == qh)
+			qh->channel->qh = NULL;
+
 		spin_unlock_irqrestore(&hsotg->lock, flags);
 		dwc2_hcd_qh_free(hsotg, qh);
 		spin_lock_irqsave(&hsotg->lock, flags);
@@ -554,7 +557,12 @@ static int dwc2_hcd_endpoint_disable(struct dwc2_hsotg *hsotg,
 		dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
 
 	ep->hcpriv = NULL;
+
+	if (qh->channel && qh->channel->qh == qh)
+		qh->channel->qh = NULL;
+
 	spin_unlock_irqrestore(&hsotg->lock, flags);
+
 	dwc2_hcd_qh_free(hsotg, qh);
 
 	return 0;
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 352c98364317..99efc2bd1617 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1935,6 +1935,16 @@ static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int chnum)
 	}
 
 	dwc2_writel(hcint, hsotg->regs + HCINT(chnum));
+
+	/*
+	 * If we got an interrupt after someone called
+	 * dwc2_hcd_endpoint_disable() we don't want to crash below
+	 */
+	if (!chan->qh) {
+		dev_warn(hsotg->dev, "Interrupt on disabled channel\n");
+		return;
+	}
+
 	chan->hcint = hcint;
 	hcint &= hcintmsk;
 
-- 
2.7.0.rc3.207.g0ac5344

[toc] | [prev] | [next] | [standalone]

#1315213 — [PATCH v5 15/21] usb: dwc2: host: Add scheduler logging for missed SOFs

We'll use the new "scheduler verbose debugging" macro to log missed
SOFs.  This is fast enough (assuming you configure it to use the ftrace
buffer) that we can do it without worrying about the speed hit.  The
overhead hit if the scheduler tracing is set to "no_printk" should be
near zero.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4:
- Add scheduler logging for missed SOFs new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/core.h     |  3 ++-
 drivers/usb/dwc2/hcd.c      |  2 +-
 drivers/usb/dwc2/hcd_intr.c | 12 ++++++++----
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 18f9e4045643..64d45a2053bb 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -809,9 +809,10 @@ struct dwc2_hsotg {
 	bool bus_suspended;
 	bool new_connection;
 
+	u16 last_frame_num;
+
 #ifdef CONFIG_USB_DWC2_TRACK_MISSED_SOFS
 #define FRAME_NUM_ARRAY_SIZE 1000
-	u16 last_frame_num;
 	u16 *frame_num_array;
 	u16 *last_frame_num_array;
 	int frame_num_idx;
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 584c64114a82..03072d1fd44d 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3082,8 +3082,8 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
 			FRAME_NUM_ARRAY_SIZE, GFP_KERNEL);
 	if (!hsotg->last_frame_num_array)
 		goto error1;
-	hsotg->last_frame_num = HFNUM_MAX_FRNUM;
 #endif
+	hsotg->last_frame_num = HFNUM_MAX_FRNUM;
 
 	/* Check if the bus driver or platform code has setup a dma_mask */
 	if (hsotg->core_params->dma_enable > 0 &&
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index b54a0c41d34f..1875e4590a3c 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -55,12 +55,16 @@
 /* This function is for debug only */
 static void dwc2_track_missed_sofs(struct dwc2_hsotg *hsotg)
 {
-#ifdef CONFIG_USB_DWC2_TRACK_MISSED_SOFS
 	u16 curr_frame_number = hsotg->frame_number;
+	u16 expected = dwc2_frame_num_inc(hsotg->last_frame_num, 1);
+
+	if (expected != curr_frame_number)
+		dwc2_sch_vdbg(hsotg, "MISSED SOF %04x != %04x\n",
+			expected, curr_frame_number);
 
+#ifdef CONFIG_USB_DWC2_TRACK_MISSED_SOFS
 	if (hsotg->frame_num_idx < FRAME_NUM_ARRAY_SIZE) {
-		if (((hsotg->last_frame_num + 1) & HFNUM_MAX_FRNUM) !=
-		    curr_frame_number) {
+		if (expected != curr_frame_number) {
 			hsotg->frame_num_array[hsotg->frame_num_idx] =
 					curr_frame_number;
 			hsotg->last_frame_num_array[hsotg->frame_num_idx] =
@@ -79,8 +83,8 @@ static void dwc2_track_missed_sofs(struct dwc2_hsotg *hsotg)
 		}
 		hsotg->dumped_frame_num_array = 1;
 	}
-	hsotg->last_frame_num = curr_frame_number;
 #endif
+	hsotg->last_frame_num = curr_frame_number;
 }
 
 static void dwc2_hc_handle_tt_clear(struct dwc2_hsotg *hsotg,
-- 
2.7.0.rc3.207.g0ac5344

[toc] | [prev] | [next] | [standalone]

#1315215 — [PATCH v5 21/21] usb: dwc2: host: If using uframe scheduler, end splits better

The microframe scheduler figured out exactly how many transfers we need
for a split transaction.  Let's use this knowledge to know when to end
things.

Without this I found that certain devices would just keep responding
with tons of NYET resonses on their INT_IN endpoint.  These would just
keep going and going and eventually we'd decide to terminate the
transfer (because the whole frame changed), but by that time the
scheduler would decide that we "missed" the start of the next transfer.
I can also imagine that if we blow past the end of our scheduled time we
may mess up other things that were scheduled to happen.

No known test cases are improved by this patch except that the scheduler
code doesn't yell about MISSES constantly anymore.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4:
- If using uframe scheduler, end splits better new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd_intr.c | 48 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 1875e4590a3c..2519e1bdad1a 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1353,14 +1353,50 @@ static void dwc2_hc_nyet_intr(struct dwc2_hsotg *hsotg,
 
 		if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
 		    chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
-			int frnum = dwc2_hcd_get_frame_number(hsotg);
+			struct dwc2_qh *qh = chan->qh;
+			bool past_end;
+
+			if (hsotg->core_params->uframe_sched <= 0) {
+				int frnum = dwc2_hcd_get_frame_number(hsotg);
+
+				/* Don't have num_hs_transfers; simple logic */
+				past_end = dwc2_full_frame_num(frnum) !=
+				     dwc2_full_frame_num(qh->next_active_frame);
+			} else {
+				int end_frnum;
 
-			if (dwc2_full_frame_num(frnum) !=
-			    dwc2_full_frame_num(chan->qh->next_active_frame)) {
 				/*
-				 * No longer in the same full speed frame.
-				 * Treat this as a transaction error.
-				 */
+				* Figure out the end frame based on schedule.
+				*
+				* We don't want to go on trying again and again
+				* forever.  Let's stop when we've done all the
+				* transfers that were scheduled.
+				*
+				* We're going to be comparing start_active_frame
+				* and next_active_frame, both of which are 1
+				* before the time the packet goes on the wire,
+				* so that cancels out.  Basically if had 1
+				* transfer and we saw 1 NYET then we're done.
+				* We're getting a NYET here so if next >=
+				* (start + num_transfers) we're done. The
+				* complexity is that for all but ISOC_OUT we
+				* skip one slot.
+				*/
+				end_frnum = dwc2_frame_num_inc(
+					qh->start_active_frame,
+					qh->num_hs_transfers);
+
+				if (qh->ep_type != USB_ENDPOINT_XFER_ISOC ||
+				    qh->ep_is_in)
+					end_frnum =
+					       dwc2_frame_num_inc(end_frnum, 1);
+
+				past_end = dwc2_frame_num_le(
+					end_frnum, qh->next_active_frame);
+			}
+
+			if (past_end) {
+				/* Treat this as a transaction error. */
 #if 0
 				/*
 				 * Todo: Fix system performance so this can
-- 
2.7.0.rc3.207.g0ac5344

[toc] | [prev] | [next] | [standalone]

#1315709

Hi,

Am Freitag, 22. Januar 2016, 10:18:35 schrieb Douglas Anderson:
> This is a bit of catchall series for all the bug fix and performance
> patches I've been working on over the last few months.  Note that for
> dwc2 we need to do LOTS in software and need super low interrupt
> latency, so most performance improvements actually fix real bugs.
> 
> Patches are structured to start with no-brainer stuff that could be
> applied ASAP, especially things I've already gotten Acks for.  Things
> get slightly more RFC / RFT like as we get farther down the series.
> Anything that can be landed sooner rather than later (especially those
> Acked long ago) would help in re-posts (I'm not biased, of course).
> 
> It's been a few months since my last post of this series.  In the
> meantime I've added a bunch of small bugfixes to the start of it and
> also TOTALLY REWROTE the microframe scheduler.  I'll say up front: I
> know nothing about USB.  I haven't read the whole spec.  I'm not
> terribly familiar with the OHCI, EHCI, and XHCI drivers in the kernel.
> ...and I'm pretty clueless overall.  Nevertheless, I've attempted to
> write up a fancy scheduler based on the portion of the spec talking
> about microframe scheduling requirements.  This rewritten scheduler does
> seem to help when I start jamming lots of USB things into a hub, so
> presumably the code is a reasonably starting point.  Given my current
> understanding of USB the old code was fairly insane, so presumably even
> if my new patch isn't perfect it's better than what we had.

I've tested this series (+the low-speed fix from today) on the following
usb-tree on a rk3288-veyron-jerry:

/:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=ehci-platform/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Video, Driver=, 480M
    |__ Port 1: Dev 2, If 1, Class=Video, Driver=, 480M
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Vendor Specific Class, Driver=zd1211rw, 480M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/5p, 480M
        |__ Port 1: Dev 3, If 0, Class=Vendor Specific Class, Driver=smsc95xx, 480M
        |__ Port 3: Dev 4, If 0, Class=Hub, Driver=hub/4p, 480M
            |__ Port 1: Dev 6, If 0, Class=Vendor Specific Class, Driver=, 12M
            |__ Port 3: Dev 8, If 0, Class=Hub, Driver=hub/4p, 480M
                |__ Port 1: Dev 10, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
                |__ Port 1: Dev 10, If 1, Class=Human Interface Device, Driver=usbhid, 1.5M
                |__ Port 2: Dev 11, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
                |__ Port 2: Dev 11, If 1, Class=Human Interface Device, Driver=usbhid, 1.5M
                |__ Port 3: Dev 12, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
                |__ Port 4: Dev 13, If 0, Class=Mass Storage, Driver=usb-storage, 480M
            |__ Port 4: Dev 9, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
        |__ Port 4: Dev 5, If 0, Class=Mass Storage, Driver=usb-storage, 480M
        |__ Port 5: Dev 7, If 0, Class=Mass Storage, Driver=usb-storage, 480M


At first we were maxing out the 9 channels of the otg port, which the
driver didn't seem to care about in 4.4-rc6, but after realizing that and
switching over to the 16ch host-port it ran really nicely, especially wrt
faster handling of all the keyboard input.

When looking over to the other desk and the person using that veyron-
jerry not cursing anymore, it seem to be running fine in a real-world use
for 2 hours now ;-)

So,
Tested-by: Heiko Stuebner <heiko@sntech.de>


> The patches here will speed up the interrupt controller significantly.
> After this series, I have a hard time seeing the interrupt controller
> taking > 20 us and I don't ever see it taking > 30 us ever in my tests
> unless I bring the cpufreq back down.  With the cpufreq at 126 MHz I can
> still see the interrupt handler take > 50 us, so I'm sure we could
> improve this further.  ...but hey, it's a start.

In the configuration described above, we were still able to run the
interrupt handlers capabilities when the cpufreq is at 126MHz, but
once set to a sane minimum it is running really nice now.


Heiko

[toc] | [prev] | [next] | [standalone]

#1315779 — Re: [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits

Heiko,

On Sat, Jan 23, 2016 at 9:52 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> Hi,
>
> Am Freitag, 22. Januar 2016, 10:18:35 schrieb Douglas Anderson:
>> This is a bit of catchall series for all the bug fix and performance
>> patches I've been working on over the last few months.  Note that for
>> dwc2 we need to do LOTS in software and need super low interrupt
>> latency, so most performance improvements actually fix real bugs.
>>
>> Patches are structured to start with no-brainer stuff that could be
>> applied ASAP, especially things I've already gotten Acks for.  Things
>> get slightly more RFC / RFT like as we get farther down the series.
>> Anything that can be landed sooner rather than later (especially those
>> Acked long ago) would help in re-posts (I'm not biased, of course).
>>
>> It's been a few months since my last post of this series.  In the
>> meantime I've added a bunch of small bugfixes to the start of it and
>> also TOTALLY REWROTE the microframe scheduler.  I'll say up front: I
>> know nothing about USB.  I haven't read the whole spec.  I'm not
>> terribly familiar with the OHCI, EHCI, and XHCI drivers in the kernel.
>> ...and I'm pretty clueless overall.  Nevertheless, I've attempted to
>> write up a fancy scheduler based on the portion of the spec talking
>> about microframe scheduling requirements.  This rewritten scheduler does
>> seem to help when I start jamming lots of USB things into a hub, so
>> presumably the code is a reasonably starting point.  Given my current
>> understanding of USB the old code was fairly insane, so presumably even
>> if my new patch isn't perfect it's better than what we had.
>
> I've tested this series (+the low-speed fix from today) on the following
> usb-tree on a rk3288-veyron-jerry:
>
> /:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=ehci-platform/1p, 480M
>     |__ Port 1: Dev 2, If 0, Class=Video, Driver=, 480M
>     |__ Port 1: Dev 2, If 1, Class=Video, Driver=, 480M
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
>     |__ Port 1: Dev 2, If 0, Class=Vendor Specific Class, Driver=zd1211rw, 480M
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/5p, 480M
>         |__ Port 1: Dev 3, If 0, Class=Vendor Specific Class, Driver=smsc95xx, 480M
>         |__ Port 3: Dev 4, If 0, Class=Hub, Driver=hub/4p, 480M
>             |__ Port 1: Dev 6, If 0, Class=Vendor Specific Class, Driver=, 12M
>             |__ Port 3: Dev 8, If 0, Class=Hub, Driver=hub/4p, 480M
>                 |__ Port 1: Dev 10, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
>                 |__ Port 1: Dev 10, If 1, Class=Human Interface Device, Driver=usbhid, 1.5M
>                 |__ Port 2: Dev 11, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
>                 |__ Port 2: Dev 11, If 1, Class=Human Interface Device, Driver=usbhid, 1.5M
>                 |__ Port 3: Dev 12, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
>                 |__ Port 4: Dev 13, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>             |__ Port 4: Dev 9, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
>         |__ Port 4: Dev 5, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>         |__ Port 5: Dev 7, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>
>
> At first we were maxing out the 9 channels of the otg port, which the
> driver didn't seem to care about in 4.4-rc6, but after realizing that and
> switching over to the 16ch host-port it ran really nicely, especially wrt
> faster handling of all the keyboard input.

Actually, I should probably fix this in the patch ("usb: dwc2: host:
Totally redo the microframe scheduler").  While trying to make things
work originally I had moved the "dwc2_periodic_channel_available()" to
always happen even with the microframe scheduler, but I don't think
there's any strong reason for that.  I think we could just undo it and
everything would work just as well or better.

I'll try to do some testing with that fix next week and then try to
send up a spun series.  Note that the reason why ("usb: dwc2: host:
Totally redo the microframe scheduler") was near the end of the series
was that I'd expected that things earlier than it were in a pretty
good shape to land while I was expecting that the microframe rewrite
might need a spin or two.


> When looking over to the other desk and the person using that veyron-
> jerry not cursing anymore, it seem to be running fine in a real-world use
> for 2 hours now ;-)

Great news!  Thank you for testing!  :)

-Doug

[toc] | [prev] | [next] | [standalone]

#1315827 — Re: [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits

Hi,

On Sat, Jan 23, 2016 at 3:09 PM, Doug Anderson <dianders@chromium.org> wrote:
> Heiko,
>
> On Sat, Jan 23, 2016 at 9:52 AM, Heiko Stuebner <heiko@sntech.de> wrote:
>> Hi,
>>
>> Am Freitag, 22. Januar 2016, 10:18:35 schrieb Douglas Anderson:
>>> This is a bit of catchall series for all the bug fix and performance
>>> patches I've been working on over the last few months.  Note that for
>>> dwc2 we need to do LOTS in software and need super low interrupt
>>> latency, so most performance improvements actually fix real bugs.
>>>
>>> Patches are structured to start with no-brainer stuff that could be
>>> applied ASAP, especially things I've already gotten Acks for.  Things
>>> get slightly more RFC / RFT like as we get farther down the series.
>>> Anything that can be landed sooner rather than later (especially those
>>> Acked long ago) would help in re-posts (I'm not biased, of course).
>>>
>>> It's been a few months since my last post of this series.  In the
>>> meantime I've added a bunch of small bugfixes to the start of it and
>>> also TOTALLY REWROTE the microframe scheduler.  I'll say up front: I
>>> know nothing about USB.  I haven't read the whole spec.  I'm not
>>> terribly familiar with the OHCI, EHCI, and XHCI drivers in the kernel.
>>> ...and I'm pretty clueless overall.  Nevertheless, I've attempted to
>>> write up a fancy scheduler based on the portion of the spec talking
>>> about microframe scheduling requirements.  This rewritten scheduler does
>>> seem to help when I start jamming lots of USB things into a hub, so
>>> presumably the code is a reasonably starting point.  Given my current
>>> understanding of USB the old code was fairly insane, so presumably even
>>> if my new patch isn't perfect it's better than what we had.
>>
>> I've tested this series (+the low-speed fix from today) on the following
>> usb-tree on a rk3288-veyron-jerry:
>>
>> /:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=ehci-platform/1p, 480M
>>     |__ Port 1: Dev 2, If 0, Class=Video, Driver=, 480M
>>     |__ Port 1: Dev 2, If 1, Class=Video, Driver=, 480M
>> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
>>     |__ Port 1: Dev 2, If 0, Class=Vendor Specific Class, Driver=zd1211rw, 480M
>> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
>>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/5p, 480M
>>         |__ Port 1: Dev 3, If 0, Class=Vendor Specific Class, Driver=smsc95xx, 480M
>>         |__ Port 3: Dev 4, If 0, Class=Hub, Driver=hub/4p, 480M
>>             |__ Port 1: Dev 6, If 0, Class=Vendor Specific Class, Driver=, 12M
>>             |__ Port 3: Dev 8, If 0, Class=Hub, Driver=hub/4p, 480M
>>                 |__ Port 1: Dev 10, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
>>                 |__ Port 1: Dev 10, If 1, Class=Human Interface Device, Driver=usbhid, 1.5M
>>                 |__ Port 2: Dev 11, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
>>                 |__ Port 2: Dev 11, If 1, Class=Human Interface Device, Driver=usbhid, 1.5M
>>                 |__ Port 3: Dev 12, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
>>                 |__ Port 4: Dev 13, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>>             |__ Port 4: Dev 9, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
>>         |__ Port 4: Dev 5, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>>         |__ Port 5: Dev 7, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>>
>>
>> At first we were maxing out the 9 channels of the otg port, which the
>> driver didn't seem to care about in 4.4-rc6, but after realizing that and
>> switching over to the 16ch host-port it ran really nicely, especially wrt
>> faster handling of all the keyboard input.
>
> Actually, I should probably fix this in the patch ("usb: dwc2: host:
> Totally redo the microframe scheduler").  While trying to make things
> work originally I had moved the "dwc2_periodic_channel_available()" to
> always happen even with the microframe scheduler, but I don't think
> there's any strong reason for that.  I think we could just undo it and
> everything would work just as well or better.
>
> I'll try to do some testing with that fix next week and then try to
> send up a spun series.  Note that the reason why ("usb: dwc2: host:
> Totally redo the microframe scheduler") was near the end of the series
> was that I'd expected that things earlier than it were in a pretty
> good shape to land while I was expecting that the microframe rewrite
> might need a spin or two.

I can't do more than cursory testing without being at work and able to
plug / unplug peripherals, but I'd expect that this should fix the
problem:

https://chromium-review.googlesource.com/#/c/323326/

I won't plan on spinning the series with that patch until at least a
week has passed.  I believe that John is out right now and I don't
want to spam a 21-part series too many times.

-Doug

[toc] | [prev] | [next] | [standalone]

#1315829 — Re: [PATCH v5 20/21] usb: dwc2: host: Totally redo the microframe scheduler

Hi,

On Fri, Jan 22, 2016 at 10:18 AM, Douglas Anderson
<dianders@chromium.org> wrote:
> This totally reimplements the microframe scheduler in dwc2 to attempt to
> handle periodic splits properly.  The old code didn't even try, so this
> was a significant effort since periodic splits are one of the most
> complicated things in USB.
>
> I've attempted to keep the old "don't use the microframe" schduler
> around for now, but not sure it's needed.  It has also only been lightly
> tested.
>
> I think it's pretty certain that this scheduler isn't perfect and might
> have some bugs, but it seems much better than what was there before.
> With this change my stressful USB test (USB webcam + USB audio + some
> keyboards) crackles less.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v5:
> - Moved defines outside of ifdef to avoid gadget-only compile error.
>
> Changes in v4:
> - Figured out what the microframe scheduler was supposed to do.
> - Microframe rewrite is totally different from v3, hopefully more right.
> - Microframe rewrite is later in the series now.
>
> Changes in v3:
> - The uframe scheduler patch is folded into optimization series.
> - Optimize uframe scheduler "single uframe" case a little.
> - uframe scheduler now atop logging patches.
> - uframe scheduler now before delayed bandwidth release patches.
> - Add defines like EARLY_FRAME_USEC
> - Reorder dwc2_deschedule_periodic() in prep for future patches.
> - uframe scheduler now shows real usefulness w/ future patches!
> - Assuming single_tt is new for v3; not terribly well tested (yet).
> - Keep track and use our uframe new for v3.
>
> Changes in v2:
> - Totally rewrote uframe scheduler again after writing test code.
> - uframe scheduler atop delayed bandwidth release patches.
>
>  drivers/usb/dwc2/core.h      |   85 ++-
>  drivers/usb/dwc2/hcd.c       |   87 ++-
>  drivers/usb/dwc2/hcd.h       |   79 ++-
>  drivers/usb/dwc2/hcd_queue.c | 1208 +++++++++++++++++++++++++++++++++++-------
>  4 files changed, 1271 insertions(+), 188 deletions(-)

Needs more testing, but possibly the next version of this patch will
include <https://chromium-review.googlesource.com/#/c/323326/>.  If
you happen to be testing / reviewing this patch, please consider it
with the proposed fixup.  I'm happy to send out a squashed version if
that makes someone's review easier.

-Doug

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web