Commit fb616e3f authored by Douglas Anderson's avatar Douglas Anderson Committed by Felipe Balbi

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.
Acked-by: default avatarJohn Youn <johnyoun@synopsys.com>
Signed-off-by: default avatarDouglas Anderson <dianders@chromium.org>
Tested-by: default avatarHeiko Stuebner <heiko@sntech.de>
Tested-by: default avatarStefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: default avatarFelipe Balbi <balbi@kernel.org>
parent 483bb254
......@@ -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: \
......
This diff is collapsed.
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment