diff options
author | Douglas Anderson <dianders@chromium.org> | 2016-01-28 18:20:08 -0800 |
---|---|---|
committer | Felipe Balbi <balbi@kernel.org> | 2016-03-04 15:14:44 +0200 |
commit | fb616e3f837eee20cc0c6d5866983f7d2730d5a3 (patch) | |
tree | 1586db4c44fecbe53ea19c54d965de0f1ea793d6 /drivers/usb/dwc2/hcd.h | |
parent | 483bb2544c470aebdcc41199c0b145d05058a3bd (diff) | |
download | talos-obmc-linux-fb616e3f837eee20cc0c6d5866983f7d2730d5a3.tar.gz talos-obmc-linux-fb616e3f837eee20cc0c6d5866983f7d2730d5a3.zip |
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: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Diffstat (limited to 'drivers/usb/dwc2/hcd.h')
-rw-r--r-- | drivers/usb/dwc2/hcd.h | 10 |
1 files changed, 7 insertions, 3 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: \ |