Hi Paul,
> The transfer scheduler in the dwc2 driver is pretty basic, not to
> mention buggy. It works fairly well with just a couple of devices
> plugged in, but if you add, say, multiple devices with periodic
> endpoints, the scheduler breaks down and can't even enumerate all
> the devices.
This seems related to a patch I made last year for the dwc_otg driver.
On RT3052, we only have 4 host channels, so it was easy to run out of
them using a 3G stick and a hub. The 3G sticks hog up 2 host channels
because they keep NAKing their bulk in transfers and 2 for intr
endpoints, leaving none for any other transfers. I created a patch to
allow canceling a NAKing host channel, but I suspect that this
microframe scheduler might help in this case as well, because it allows
sharing a single host channel for all periodic transfers, I think,
leaving the other three for bulk transfers. It might still be useful to
forward port my patch at some point, since that would in theory allow
multiple blocking endpoints to be used at the same time.
> To fix this, import the "microframe scheduler" patch from the
> driver in the downstream Raspberry Pi kernel, which is based on
> the Synopsys vendor driver. That patch was done by the guys from
> raspberrypi.org, including at least Gordon Hollingsworth and
> "popcornmix".
>
> I have added a driver parameter for this, enabled by default, in
> case anyone has problems with it and needs to disable it. I don't
> think we should add a DT binding for that, though, since I plan to
> remove the option once any bugs are fixed.
Some general remarks:
It seems that this patch doesn't include just the microframe scheduling,
but also the NAK holdoff patch (which was a separate patch in the
downstream kernel IIRC and seems like a separate feature in any case)
and other unrelated and unused bits.
Furthermore, I wonder about how this scheduler works exactly. What I see
is:
- the number usecs needed for a single transfer in a periodic qh is
calculated
- When the qh is scheduled, the first of the 8 microframes with enough
usecs available is picked for this qh (disregarding FS transfers that
don't fit in 1 microframe for now)
However, this seems correct only for endpoints with an interval of 8
microframes? If an isoc endpoint has an interval of 1, it should be
scheduled in _every_ microframe, right?
The old code was pessimistic (the dwc2_check_periodic_bandwidth
essentially assumes an interval of 1 for everything) but the new code is
actually too optimistic (as it essentially assumes an interval of 8 for
everything).
This leads me to believe that this code works because it makes the
scheduler way to optimistic and because it removes the "one channel per
periodic endpoint" policy (which is way too optimistic), but it would
break when adding too much (periodic) devices (in nasty ways, no nice
"not enough bandwidth" messages).
Overall, I don't think this patch is not even nearly ready to be
merged...
There's a lot more detailed comments inline in the code below.
> Signed-off-by: Paul Zimmerman <pa...@synopsys.com>
> ---
> Gordon, I would like to add a copyright notice for the work you
> guys did on this (thanks!). Can you send me the names and dates I
> should add?
>
> This patch should be applied on top of "staging: dwc2: add driver
> parameter to set AHB config register value" or else it won't apply
> cleanly.
>
> drivers/staging/dwc2/core.c | 21 ++++
> drivers/staging/dwc2/core.h | 47 +++++---
> drivers/staging/dwc2/hcd.c | 94 +++++++++++++---
> drivers/staging/dwc2/hcd.h | 27 +++--
> drivers/staging/dwc2/hcd_ddma.c | 13 ++-
> drivers/staging/dwc2/hcd_intr.c | 59 +++++++---
> drivers/staging/dwc2/hcd_queue.c | 236
> ++++++++++++++++++++++++++++++++++++---
> drivers/staging/dwc2/pci.c | 1 +
> 8 files changed, 417 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
> index a090f79..16afc06 100644
> --- a/drivers/staging/dwc2/core.c
> +++ b/drivers/staging/dwc2/core.c
> @@ -2612,6 +2612,26 @@ int dwc2_set_param_otg_ver(struct dwc2_hsotg *hsotg,
> int val)
> return retval;
> }
>
> +int dwc2_set_param_uframe_sched(struct dwc2_hsotg *hsotg, int val)
> +{
> + int retval = 0;
> +
> + if (DWC2_PARAM_TEST(val, 0, 1)) {
> + if (val >= 0) {
> + dev_err(hsotg->dev,
> + "'%d' invalid for parameter uframe_sched\n",
> + val);
> + dev_err(hsotg->dev, "uframe_sched must be 0 or 1\n");
> + }
> + val = 1;
> + dev_dbg(hsotg->dev, "Setting uframe_sched to %d\n", val);
> + retval = -EINVAL;
> + }
> +
> + hsotg->core_params->uframe_sched = val;
> + return retval;
> +}
> +
> /*
> * This function is called during module intialization to pass module
> parameters
> * for the DWC_otg core. It returns non-0 if any parameters are invalid.
> @@ -2658,6 +2678,7 @@ int dwc2_set_parameters(struct dwc2_hsotg *hsotg,
> retval |= dwc2_set_param_reload_ctl(hsotg, params->reload_ctl);
> retval |= dwc2_set_param_ahbcfg(hsotg, params->ahbcfg);
> retval |= dwc2_set_param_otg_ver(hsotg, params->otg_ver);
> + retval |= dwc2_set_param_uframe_sched(hsotg, params->uframe_sched);
>
> return retval;
> }
> diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
> index e771e40..3107a4f 100644
> --- a/drivers/staging/dwc2/core.h
> +++ b/drivers/staging/dwc2/core.h
> @@ -74,6 +74,9 @@ enum dwc2_lx_state {
> * 0 - HNP and SRP capable (default)
> * 1 - SRP Only capable
> * 2 - No HNP/SRP capable
> + * @otg_ver: OTG version supported
> + * 0 - 1.3
> + * 1 - 2.0
> * @dma_enable: Specifies whether to use slave or DMA mode for
> accessing
> * the data FIFOs. The driver will automatically detect
> the
> * value for this parameter if none is specified.
> @@ -90,20 +93,10 @@ enum dwc2_lx_state {
> * the attached device and the value of phy_type.
> * 0 - High Speed (default)
> * 1 - Full Speed
> - * @host_support_fs_ls_low_power: Specifies whether low power mode is
> supported
> - * when attached to a Full Speed or Low Speed device in
> - * host mode.
> - * 0 - Don't support low power mode (default)
> - * 1 - Support low power mode
> - * @host_ls_low_power_phy_clk: Specifies the PHY clock rate in low power mode
> - * when connected to a Low Speed device in host mode.
> This
> - * parameter is applicable only if
> - * host_support_fs_ls_low_power is enabled. If phy_type
> is
> - * set to FS then defaults to 6 MHZ otherwise 48 MHZ.
> - * 0 - 48 MHz
> - * 1 - 6 MHz
> * @enable_dynamic_fifo: 0 - Use coreConsultant-specified FIFO size
> parameters
> * 1 - Allow dynamic FIFO sizing (default)
> + * @en_multiple_tx_fifo: Specifies whether dedicated per-endpoint transmit
> FIFOs
> + * are enabled
> * @host_rx_fifo_size: Number of 4-byte words in the Rx FIFO in host mode
> when
> * dynamic FIFO sizing is enabled
> * 16 to 32768 (default 1024)
> @@ -145,9 +138,19 @@ enum dwc2_lx_state {
> * 0 - No (default)
> * 1 - Yes
> * @ulpi_fs_ls: True to make ULPI phy operate in FS/LS mode only
> + * @host_support_fs_ls_low_power: Specifies whether low power mode is
> supported
> + * when attached to a Full Speed or Low Speed device in
> + * host mode.
> + * 0 - Don't support low power mode (default)
> + * 1 - Support low power mode
> + * @host_ls_low_power_phy_clk: Specifies the PHY clock rate in low power mode
> + * when connected to a Low Speed device in host mode.
> This
> + * parameter is applicable only if
> + * host_support_fs_ls_low_power is enabled. If phy_type
> is
> + * set to FS then defaults to 6 MHZ otherwise 48 MHZ.
> + * 0 - 48 MHz
> + * 1 - 6 MHz
> * @ts_dline: True to enable Term Select Dline pulsing
> - * @en_multiple_tx_fifo: Specifies whether dedicated per-endpoint transmit
> FIFOs
> - * are enabled
> * @reload_ctl: True to allow dynamic reloading of HFIR register
> during
> * runtime
> * @ahbcfg: This field allows the default value of the GAHBCFG
> @@ -155,9 +158,7 @@ enum dwc2_lx_state {
> * -1 - GAHBCFG value will not be overridden
> * all others - GAHBCFG value will be overridden with
> * this value
> - * @otg_ver: OTG version supported
> - * 0 - 1.3
> - * 1 - 2.0
The above changes look unrelated to the subject of this patch?
> + * @uframe_sched: True to enable the microframe scheduler
> *
> * The following parameters may be specified when starting the module. These
> * parameters define how the DWC_otg controller should be configured.
> @@ -191,6 +192,7 @@ struct dwc2_core_params {
> int ts_dline;
> int reload_ctl;
> int ahbcfg;
> + int uframe_sched;
> };
>
> /**
> @@ -266,6 +268,7 @@ struct dwc2_core_params {
> * This value is in microseconds per (micro)frame. The
> * assumption is that all periodic transfers may occur
> in
> * the same (micro)frame.
> + * @frame_usecs: Internal variable used by the microframe scheduler
This could use a real explanation, like:
* @frame_usecs: Number of usecs per microframe that are still
available to schedule periodic transfers in.
> * @frame_number: Frame number read from the core at SOF. The value
> ranges
> * from 0 to HFNUM_MAX_FRNUM.
> * @periodic_qh_count: Count of periodic QHs, if using several eps. Used for
> @@ -278,6 +281,8 @@ struct dwc2_core_params {
> * host channel is available for non-periodic
> transactions.
> * @non_periodic_channels: Number of host channels assigned to non-periodic
> * transfers
> + * @available_host_channels Number of host channels available for the
> microframe
> + * scheduler to use
This description looks weird. The scheduler doesn't use host channels,
it uses frames and usecs. It seems that this really is the number of
host channels not in use, but I wonder if one can't just check the
number of channels in the free_hc_list (given the only checks done are <
1 and <= 1, checking against the list shouldn't be so hard?)
> * @hc_ptr_array: Array of pointers to the host channel descriptors.
> * Allows accessing a host channel descriptor given the
> * host channel number. This is useful in interrupt
> @@ -292,6 +297,9 @@ struct dwc2_core_params {
> * @otg_port: OTG port number
> * @frame_list: Frame list
> * @frame_list_dma: Frame list DMA address
> + * @next_sched_frame: Internal variable used by the microframe scheduler
> + * @np_count: Internal variable used by the microframe scheduler
> + * @np_sent: Internal variable used by the microframe scheduler
The next_sched_frame, np_count and np_sent seem to be unused. There is a
lot of code to set them and keep them correct, but they are never
actually used for anything. I remember there was some other patch (or
part of this patch?) which did something with higher priority interrupts
on ARM, perhaps that's what this was for? Alternatively, I think these
could be used to disable the SOF interrupt when nothing needs to be
scheduled soon, but I think that should be a separate patch from this
one?
> */
> struct dwc2_hsotg {
> struct device *dev;
> @@ -338,6 +346,7 @@ struct dwc2_hsotg {
> struct list_head periodic_sched_assigned;
> struct list_head periodic_sched_queued;
> u16 periodic_usecs;
> + u16 frame_usecs[8];
> u16 frame_number;
> u16 periodic_qh_count;
>
> @@ -353,6 +362,7 @@ struct dwc2_hsotg {
> struct list_head free_hc_list;
> int periodic_channels;
> int non_periodic_channels;
> + int available_host_channels;
> struct dwc2_host_chan *hc_ptr_array[MAX_EPS_CHANNELS];
> u8 *status_buf;
> dma_addr_t status_buf_dma;
> @@ -365,6 +375,9 @@ struct dwc2_hsotg {
> u8 otg_port;
> u32 *frame_list;
> dma_addr_t frame_list_dma;
> + int next_sched_frame;
> + int np_count;
> + int np_sent;
>
> /* DWC OTG HW Release versions */
> #define DWC2_CORE_REV_2_71a 0x4f54271a
> diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
> index 962468f..51598bc 100644
> --- a/drivers/staging/dwc2/hcd.c
> +++ b/drivers/staging/dwc2/hcd.c
> @@ -537,10 +537,15 @@ static void dwc2_hcd_reinit(struct dwc2_hsotg *hsotg)
> int i;
>
> hsotg->flags.d32 = 0;
> -
> hsotg->non_periodic_qh_ptr = &hsotg->non_periodic_sched_active;
> - hsotg->non_periodic_channels = 0;
> - hsotg->periodic_channels = 0;
> +
> + if (hsotg->core_params->uframe_sched > 0) {
> + hsotg->available_host_channels =
> + hsotg->core_params->host_channels;
> + } else {
> + hsotg->non_periodic_channels = 0;
> + hsotg->periodic_channels = 0;
> + }
>
> /*
> * Put all channels in the free channel list and clean up channel
> @@ -716,12 +721,13 @@ static int dwc2_hc_setup_align_buf(struct dwc2_hsotg
> *hsotg, struct dwc2_qh *qh,
> * @qh: Transactions from the first QTD for this QH are selected and
> assigned
> * to a free host channel
> */
> -static void dwc2_assign_and_init_hc(struct dwc2_hsotg *hsotg,
> - struct dwc2_qh *qh)
> +static int dwc2_assign_and_init_hc(struct dwc2_hsotg *hsotg, struct dwc2_qh
> *qh)
> {
> - struct dwc2_host_chan *chan;
> + struct list_head *hc_item, *hc_tmp;
> + struct dwc2_host_chan *chan = NULL;
> struct dwc2_hcd_urb *urb;
> struct dwc2_qtd *qtd;
> + u32 hcchar;
> void *bufptr = NULL;
>
> if (dbg_qh(qh))
> @@ -729,22 +735,36 @@ static void dwc2_assign_and_init_hc(struct dwc2_hsotg
> *hsotg,
>
> if (list_empty(&qh->qtd_list)) {
> dev_dbg(hsotg->dev, "No QTDs in QH list\n");
> - return;
> + return -ENOMEM;
> }
>
> - if (list_empty(&hsotg->free_hc_list)) {
> - dev_dbg(hsotg->dev, "No free channel to assign\n");
> - return;
> + /* Find a free host channel with both CHENA and CHDIS clear */
> + list_for_each_safe(hc_item, hc_tmp, &hsotg->free_hc_list) {
> + chan = list_entry(hc_item, struct dwc2_host_chan,
> + hc_list_entry);
> + hcchar = readl(hsotg->regs + HCCHAR(chan->hc_num));
> + if ((hcchar & (HCCHAR_CHENA | HCCHAR_CHDIS)) == 0)
> + break;
> + /* Move channel to end of free list */
> + list_del(&chan->hc_list_entry);
> + list_add_tail(&chan->hc_list_entry, &hsotg->free_hc_list);
> + chan = NULL;
> }
I think this list could end up in an infinite loop if the free_hc_list
contains 2 or more channels and all of those have CHENA or CHDIS set.
Not sure if that is ever possible, but perhaps some kind of guard
(saving the pointer to the first channel moved to the end of the list
and breaking on that?) might be useful just in case?
Could you expand on what it means for a channel to have CHENA or CHDIS
set and why it could even happen for a channel that is in the free list?
> - chan = list_first_entry(&hsotg->free_hc_list, struct dwc2_host_chan,
> - hc_list_entry);
> + if (!chan) {
> + dev_dbg(hsotg->dev, "No free channel to assign\n");
> + return -ENOMEM;
> + }
As above, I don't think this can ever happen.
> - /* Remove the host channel from the free list */
> + /* Remove host channel from free list */
This looks like a useless change, but ok :-)
> list_del_init(&chan->hc_list_entry);
>
> qtd = list_first_entry(&qh->qtd_list, struct dwc2_qtd, qtd_list_entry);
> urb = qtd->urb;
> + if ((urb->actual_length < 0 || urb->actual_length > urb->length) &&
> + !dwc2_hcd_is_pipe_in(&urb->pipe_info))
> + urb->actual_length = urb->length;
> +
Hmm, in what cases can this happen? Is this change really related to the
scheduler?
> qh->channel = chan;
> qtd->in_process = 1;
>
> @@ -817,7 +837,7 @@ static void dwc2_assign_and_init_hc(struct dwc2_hsotg
> *hsotg,
> &hsotg->free_hc_list);
> qtd->in_process = 0;
> qh->channel = NULL;
> - return;
> + return -ENOMEM;
> }
> } else {
> chan->align_buf = 0;
> @@ -836,6 +856,8 @@ static void dwc2_assign_and_init_hc(struct dwc2_hsotg
> *hsotg,
>
> dwc2_hc_init(hsotg, chan);
> chan->qh = qh;
> +
> + return 0;
> }
>
> /**
> @@ -864,8 +886,14 @@ enum dwc2_transaction_type dwc2_hcd_select_transactions(
> while (qh_ptr != &hsotg->periodic_sched_ready) {
> if (list_empty(&hsotg->free_hc_list))
> break;
> + if (hsotg->core_params->uframe_sched > 0) {
> + if (hsotg->available_host_channels <= 1)
> + break;
> + hsotg->available_host_channels--;
> + }
> qh = list_entry(qh_ptr, struct dwc2_qh, qh_list_entry);
> - dwc2_assign_and_init_hc(hsotg, qh);
> + if (dwc2_assign_and_init_hc(hsotg, qh))
> + break;
>
> /*
> * Move the QH from the periodic ready schedule to the
> @@ -884,13 +912,38 @@ enum dwc2_transaction_type dwc2_hcd_select_transactions(
> num_channels = hsotg->core_params->host_channels;
> qh_ptr = hsotg->non_periodic_sched_inactive.next;
> while (qh_ptr != &hsotg->non_periodic_sched_inactive) {
> - if (hsotg->non_periodic_channels >= num_channels -
> + if (hsotg->core_params->uframe_sched <= 0 &&
> + hsotg->non_periodic_channels >= num_channels -
> hsotg->periodic_channels)
> break;
> if (list_empty(&hsotg->free_hc_list))
> break;
> qh = list_entry(qh_ptr, struct dwc2_qh, qh_list_entry);
> - dwc2_assign_and_init_hc(hsotg, qh);
> +
> + /*
> + * Check to see if this is a NAK'd retransmit, in which case
> + * ignore for retransmission. We hold off on bulk
> + * retransmissions to reduce NAK interrupt overhead for
> + * cheeky devices that just hold off using NAKs.
> + */
> + if (dwc2_full_frame_num(qh->nak_frame) ==
> + dwc2_full_frame_num(dwc2_hcd_get_frame_number(hsotg))) {
> + /* Make interrupt run on next frame (i.e. 8 uframes) */
> + hsotg->next_sched_frame = ((qh->nak_frame + 8) & ~7) &
> + HFNUM_MAX_FRNUM;
TODO: Does this have any effect?
> + qh_ptr = qh_ptr->next;
> + continue;
> + } else {
> + qh->nak_frame = 0xffff;
> + }
Shouldn't the if above handle 0xffff specially? It seems that
dwc2_full_frame_num doesn't and happily returns the maximum full frame
number possible, which could accidentally match the current frame number
AFAICS. I guess normally this doesn't matter, since nak_frame is only
set to 0xffff when the qh is scheduled, but that might not happen if
dwc2_assign_and_init_hc below fails.
> +
> + if (hsotg->core_params->uframe_sched > 0) {
> + if (hsotg->available_host_channels < 1)
> + break;
> + hsotg->available_host_channels--;
> + }
> + if (dwc2_assign_and_init_hc(hsotg, qh))
> + break;
Doesn't it make sense to move the update of
available_host_channels inside dwc2_assign_and_init_hc? I think that
with the current code, the value of available_host_channels might become
out of sync with reality if dwc2_assign_and_init_hc fails?
Also, looking closely at the above, it seems to me that it is possible
for nonperiodic transfers to use up all host channels. In the old code, every
periodic transfer would reserve one host channel, but now this no longer
happens. The checks above make sure that the last host channel is never
used for a periodic transfer, but it can be used for a nonperiodic
transfer. If the nonperiodic transfer keeps sending NAKs it could (in
DMA mode) keep the host channel blocked indefinately. If this happens
for all available host channels, there would be non host channel
available for periodic transfers...
I wonder if the solution to this is to make sure that neither periodic
nor non-periodic transfers can eat up all host channels? Or perhaps
change the checks above so that non-periodic transfers can never use the
last channel? Downside of that is that you might leave a channel unused
which you could better have used. Perhaps only do this when none of the
channels are in use for a periodic transfer right now?
Also, I wonder if using just a single host channel periodic transfers is
enough? If there are multiple periodic transfers scheduled for the same
microframe, you can then only send one of them to the hardware and have
to wait for the transfer complete interrupt before you can send the next
one. I suspect that the delay this introduces could cause the second
periodic transfer to not fit into the microframe anymore?
> /*
> * Move the QH from the non-periodic inactive schedule to the
> @@ -899,13 +952,15 @@ enum dwc2_transaction_type dwc2_hcd_select_transactions(
> qh_ptr = qh_ptr->next;
> list_move(&qh->qh_list_entry,
> &hsotg->non_periodic_sched_active);
> + hsotg->np_sent++;
>
> if (ret_val == DWC2_TRANSACTION_NONE)
> ret_val = DWC2_TRANSACTION_NON_PERIODIC;
> else
> ret_val = DWC2_TRANSACTION_ALL;
>
> - hsotg->non_periodic_channels++;
> + if (hsotg->core_params->uframe_sched <= 0)
> + hsotg->non_periodic_channels++;
> }
>
> return ret_val;
> @@ -2905,6 +2960,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
> hsotg->hc_ptr_array[i] = channel;
> }
>
> + if (hsotg->core_params->uframe_sched > 0)
> + dwc2_hcd_init_usecs(hsotg);
> +
> /* Initialize hsotg start work */
> INIT_DELAYED_WORK(&hsotg->start_work, dwc2_hcd_start_func);
>
> diff --git a/drivers/staging/dwc2/hcd.h b/drivers/staging/dwc2/hcd.h
> index 933e8d1..8558b0f 100644
> --- a/drivers/staging/dwc2/hcd.h
> +++ b/drivers/staging/dwc2/hcd.h
> @@ -232,16 +232,21 @@ enum dwc2_transaction_type {
> * - DWC2_HC_PID_DATA1
> * @ping_state: Ping state
> * @do_split: Full/low speed endpoint on high-speed hub requires
> split
> - * @qtd_list: List of QTDs for this QH
> - * @channel: Host channel currently processing transfers for this
> QH
> + * @td_first: Index of first activated isochronous transfer
> descriptor
> + * @td_last: Index of last activated isochronous transfer
> descriptor
> * @usecs: Bandwidth in microseconds per (micro)frame
> * @interval: Interval between transfers in (micro)frames
> - * @sched_frame: (micro)frame to initialize a periodic transfer.
> + * @sched_frame: (Micro)frame to initialize a periodic transfer.
> * The transfer executes in the following (micro)frame.
> + * @nak_frame: Internal variable used by the microframe scheduler
> + * @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
> * @dw_align_buf: Used instead of original buffer if its physical
> address
> * is not dword-aligned
> * @dw_align_buf_dma: DMA address for align_buf
> + * @qtd_list: List of QTDs for this QH
> + * @channel: Host channel currently processing transfers for this
> QH
> * @qh_list_entry: Entry for QH in either the periodic or non-periodic
> * schedule
> * @desc_list: List of transfer descriptors
> @@ -249,9 +254,6 @@ enum dwc2_transaction_type {
> * @n_bytes: Xfer Bytes array. Each element corresponds to a
> transfer
> * descriptor and indicates original XferSize value for
> the
> * descriptor
> - * @ntd: Actual number of transfer descriptors in a list
> - * @td_first: Index of first activated isochronous transfer
> descriptor
> - * @td_last: Index of last activated isochronous transfer
> descriptor
> * @tt_buffer_dirty True if clear_tt_buffer_complete is pending
> *
> * A Queue Head (QH) holds the static characteristics of an endpoint and
> @@ -266,21 +268,23 @@ struct dwc2_qh {
> u8 data_toggle;
> u8 ping_state;
> u8 do_split;
> - struct list_head qtd_list;
> - struct dwc2_host_chan *channel;
> + u8 td_first;
> + u8 td_last;
> u16 usecs;
> u16 interval;
> u16 sched_frame;
> + u16 nak_frame;
> + u16 frame_usecs[8];
> u16 start_split_frame;
> + u16 ntd;
> u8 *dw_align_buf;
> dma_addr_t dw_align_buf_dma;
> + struct list_head qtd_list;
> + struct dwc2_host_chan *channel;
> struct list_head qh_list_entry;
> struct dwc2_hcd_dma_desc *desc_list;
> dma_addr_t desc_list_dma;
> u32 *n_bytes;
> - u16 ntd;
> - u8 td_first;
> - u8 td_last;
> unsigned tt_buffer_dirty:1;
> };
Only the addition of nak_frame and frame_usecs seem relevant to this
patch, the rest seems noise. And those variables could use some actual
documentation. I propose:
* @nak_frame: (Micro)frame number in which this qh was
re-queued because a NAK was received. Is 0xffff
when no NAK was received.
* @frame_usecs: Internal variable used by the microframe scheduler
>
> @@ -462,6 +466,7 @@ extern void dwc2_hcd_queue_transactions(struct dwc2_hsotg
> *hsotg,
>
> /* Schedule Queue Functions */
> /* Implemented in hcd_queue.c */
> +extern void dwc2_hcd_init_usecs(struct dwc2_hsotg *hsotg);
> extern void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh);
> extern int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh);
> extern void dwc2_hcd_qh_unlink(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh);
> diff --git a/drivers/staging/dwc2/hcd_ddma.c b/drivers/staging/dwc2/hcd_ddma.c
> index de5af1b..ef2b42b 100644
> --- a/drivers/staging/dwc2/hcd_ddma.c
> +++ b/drivers/staging/dwc2/hcd_ddma.c
> @@ -271,10 +271,14 @@ static void dwc2_release_channel_ddma(struct dwc2_hsotg
> *hsotg,
> {
> struct dwc2_host_chan *chan = qh->channel;
>
> - if (dwc2_qh_is_non_per(qh))
> - hsotg->non_periodic_channels--;
> - else
> + if (dwc2_qh_is_non_per(qh)) {
> + if (hsotg->core_params->uframe_sched > 0)
> + hsotg->available_host_channels++;
> + else
> + hsotg->non_periodic_channels--;
> + } else {
> dwc2_update_frame_list(hsotg, qh, 0);
> + }
Shouldn't this update available_host_channels for periodic qh's as well?
Looking at dwc2_hcd_select_transactions above, available_host_channels
is decremented for those as well?
> /*
> * The condition is added to prevent double cleanup try in case of
> @@ -370,7 +374,8 @@ void dwc2_hcd_qh_free_ddma(struct dwc2_hsotg *hsotg,
> struct dwc2_qh *qh)
>
> if ((qh->ep_type == USB_ENDPOINT_XFER_ISOC ||
> qh->ep_type == USB_ENDPOINT_XFER_INT) &&
> - !hsotg->periodic_channels && hsotg->frame_list) {
> + (hsotg->core_params->uframe_sched > 0 ||
> + !hsotg->periodic_channels) && hsotg->frame_list) {
> dwc2_per_sched_disable(hsotg);
> dwc2_frame_list_free(hsotg);
> }
This seems weird to me: effectively you'll always disable the periodic
schedule when just one periodic qh is freed. Or is it so that only a
single periodic qh can be active with uframe_sched enabled? Doesn't look
like this to me.
> diff --git a/drivers/staging/dwc2/hcd_intr.c b/drivers/staging/dwc2/hcd_intr.c
> index 9e68ef1..3ec9d38 100644
> --- a/drivers/staging/dwc2/hcd_intr.c
> +++ b/drivers/staging/dwc2/hcd_intr.c
> @@ -118,9 +118,11 @@ static void dwc2_hc_handle_tt_clear(struct dwc2_hsotg
> *hsotg,
> */
> static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
> {
> + enum dwc2_transaction_type tr_type;
> struct list_head *qh_entry;
> struct dwc2_qh *qh;
> - enum dwc2_transaction_type tr_type;
> + int next_sched_frame = -1;
> + int did_something = 0;
This variable seems unused?
>
> #ifdef DEBUG_SOF
> dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n");
> @@ -135,17 +137,29 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
> while (qh_entry != &hsotg->periodic_sched_inactive) {
> qh = list_entry(qh_entry, struct dwc2_qh, qh_list_entry);
> qh_entry = qh_entry->next;
> - if (dwc2_frame_num_le(qh->sched_frame, hsotg->frame_number))
> + if (dwc2_frame_num_le(qh->sched_frame, hsotg->frame_number)) {
> /*
> * Move QH to the ready list to be executed next
> * (micro)frame
> */
> list_move(&qh->qh_list_entry,
> &hsotg->periodic_sched_ready);
> + did_something = 1;
> + } else {
> + if (next_sched_frame < 0 ||
> + dwc2_frame_num_le(qh->sched_frame,
> + next_sched_frame))
> + next_sched_frame = qh->sched_frame;
> + }
> }
> +
> + hsotg->next_sched_frame = next_sched_frame;
> +
> tr_type = dwc2_hcd_select_transactions(hsotg);
> - if (tr_type != DWC2_TRANSACTION_NONE)
> + if (tr_type != DWC2_TRANSACTION_NONE) {
> dwc2_hcd_queue_transactions(hsotg, tr_type);
> + did_something = 1;
> + }
>
> /* Clear interrupt */
> writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
> @@ -752,18 +766,23 @@ cleanup:
> dwc2_hc_cleanup(hsotg, chan);
> list_add_tail(&chan->hc_list_entry, &hsotg->free_hc_list);
>
> - switch (chan->ep_type) {
> - case USB_ENDPOINT_XFER_CONTROL:
> - case USB_ENDPOINT_XFER_BULK:
> - hsotg->non_periodic_channels--;
> - break;
> - default:
> - /*
> - * Don't release reservations for periodic channels here.
> - * That's done when a periodic transfer is descheduled (i.e.
> - * when the QH is removed from the periodic schedule).
> - */
> - break;
> + if (hsotg->core_params->uframe_sched > 0) {
> + hsotg->available_host_channels++;
> + } else {
> + switch (chan->ep_type) {
> + case USB_ENDPOINT_XFER_CONTROL:
> + case USB_ENDPOINT_XFER_BULK:
> + hsotg->non_periodic_channels--;
> + break;
> + default:
> + /*
> + * Don't release reservations for periodic channels
> + * here. That's done when a periodic transfer is
> + * descheduled (i.e. when the QH is removed from the
> + * periodic schedule).
> + */
> + break;
> + }
> }
>
> haintmsk = readl(hsotg->regs + HAINTMSK);
> @@ -1205,6 +1224,16 @@ static void dwc2_hc_nak_intr(struct dwc2_hsotg *hsotg,
> chnum);
>
> /*
> + * When we get bulk NAKs then remember this so we holdoff on this qh
> + * until the beginning of the next frame
> + */
> + switch (dwc2_hcd_get_pipe_type(&qtd->urb->pipe_info)) {
> + case USB_ENDPOINT_XFER_BULK:
> + chan->qh->nak_frame = dwc2_hcd_get_frame_number(hsotg);
> + break;
> + }
> +
> + /*
> * Handle NAK for IN/OUT SSPLIT/CSPLIT transfers, bulk, control, and
> * interrupt. Re-start the SSPLIT transfer.
> */
> diff --git a/drivers/staging/dwc2/hcd_queue.c
> b/drivers/staging/dwc2/hcd_queue.c
> index 5461e3b..2748cb9 100644
> --- a/drivers/staging/dwc2/hcd_queue.c
> +++ b/drivers/staging/dwc2/hcd_queue.c
> @@ -83,6 +83,7 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct
> dwc2_qh *qh,
> dev_speed = dwc2_host_get_speed(hsotg, urb->priv);
>
> dwc2_host_hub_info(hsotg, urb->priv, &hub_addr, &hub_port);
> + qh->nak_frame = 0xffff;
>
> if ((dev_speed == USB_SPEED_LOW || dev_speed == USB_SPEED_FULL) &&
> hub_addr != 0 && hub_addr != 1) {
> @@ -324,6 +325,147 @@ static int dwc2_check_periodic_bandwidth(struct
> dwc2_hsotg *hsotg,
> }
>
> /**
> + * Microframe scheduler
> + * track the total use in hsotg->frame_usecs
> + * keep each qh use in qh->frame_usecs
> + * when surrendering the qh then donate the time back
> + */
> +static const unsigned short max_uframe_usecs[] = {
> + 100, 100, 100, 100, 100, 100, 30, 0
> +};
What's the point of not scheduling anything in the last microframe and
not so much in the next-to-last one? To keep some time available for
non-periodic transfers?
> +
> +void dwc2_hcd_init_usecs(struct dwc2_hsotg *hsotg)
> +{
> + int i;
> +
> + hsotg->next_sched_frame = hsotg->np_count = hsotg->np_sent = 0;
> + for (i = 0; i < 8; i++)
Perhaps that 8 should be ARRAY_SIZE(hsotg->frame_usecs) from
<linux/kernel.h>? A few more of those below. Or is "8" universal enough
that using the literal is ok (I guess it's not a big problem, of
course).
> + hsotg->frame_usecs[i] = max_uframe_usecs[i];
> +}
> +
> +static int dwc2_find_single_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh
> *qh)
> +{
> + unsigned short utime = qh->usecs;
> + int done = 0;
> + int i = 0;
> + int ret = -1;
> +
> + while (!done) {
> + /* At the start hsotg->frame_usecs[i] = max_uframe_usecs[i] */
> + if (utime <= hsotg->frame_usecs[i]) {
> + hsotg->frame_usecs[i] -= utime;
> + qh->frame_usecs[i] += utime;
> + ret = i;
> + done = 1;
> + } else {
> + i++;
> + if (i == 8)
> + done = 1;
> + }
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * use this for FS apps that can span multiple uframes
> + */
> +static int dwc2_find_multi_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh
> *qh)
> +{
> + unsigned short utime = qh->usecs;
> + unsigned short xtime;
> + int t_left = utime;
> + int done = 0;
> + int i = 0;
> + int j;
> + int ret = -1;
> +
> + while (!done) {
> + if (hsotg->frame_usecs[i] <= 0) {
> + i++;
> + if (i == 8) {
> + ret = -1;
> + done = 1;
> + }
> + continue;
> + }
> +
> + /*
> + * we need n consecutive slots so use j as a start slot
> + * j plus j+1 must be enough time (for now)
This comment doesn't make sense, it suggests that 2 slots is always
enough?
What the below code does (I _think_) is start at uframe i, use any time
left in that frame, and then add completely unused subsequent frames,
optionally ending with another partial frame.
> + */
> + xtime = hsotg->frame_usecs[i];
> + for (j = i + 1; j < 8; j++) {
> + /*
> + * if we add this frame remaining time to xtime we may
> + * be OK, if not we need to test j for a complete frame
> + */
> + if (xtime + hsotg->frame_usecs[j] < utime) {
> + if (hsotg->frame_usecs[j] <
> + max_uframe_usecs[j]) {
> + ret = -1;
> + break;
> + }
> + }
> + if (xtime >= utime) {
> + ret = i;
> + break;
> + }
> + /* add the frame time to x time */
> + xtime += hsotg->frame_usecs[j];
> + /* we must have a fully available next frame or break */
> + if (xtime < utime &&
> + hsotg->frame_usecs[j] == max_uframe_usecs[j]) {
> + ret = -1;
> + break;
> + }
This last if seems redundant with the first one, it expresses exactly the
same expressions AFAICS. In other words, this last if can never be true,
since if it would, the first if would have been true and broke out of
the loop.
I also wonder if this loop cannot be written a bit more clearly.
> + }
> + if (ret >= 0) {
> + t_left = utime;
> + for (j = i; t_left > 0 && j < 8; j++) {
> + t_left -= hsotg->frame_usecs[j];
> + if (t_left <= 0) {
It seems to me that doing "if (t_left < hsotg->frame_usecs[j])" here
before changing t_left might give code that's easier to read, not having
to deal with a negative t_left. Might be a matter of taste, though.
> + qh->frame_usecs[j] +=
> + hsotg->frame_usecs[j] + t_left;
> + hsotg->frame_usecs[j] = -t_left;
> + ret = i;
> + done = 1;
> + } else {
> + qh->frame_usecs[j] +=
> + hsotg->frame_usecs[j];
> + hsotg->frame_usecs[j] = 0;
> + }
> + }
> + } else {
> + i++;
> + if (i == 8) {
> + ret = -1;
> + done = 1;
> + }
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int dwc2_find_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
> +{
> + int ret;
> +
> + if (qh->dev_speed == USB_SPEED_HIGH) {
> + /* if this is a hs transaction we need a full frame */
I guess this comment should so "no more than one microframe" ? Or should
it say "cannot span across a SOF"?
> + ret = dwc2_find_single_uframe(hsotg, qh);
> + } else {
> + /*
> + * if this is a fs transaction we may need a sequence
> + * of frames
> + */
> + ret = dwc2_find_multi_uframe(hsotg, qh);
> + }
> + return ret;
> +}
> +
> +/**
> * dwc2_check_max_xfer_size() - Checks that the max transfer size allowed in
> a
> * host channel is large enough to handle the maximum data transfer in a
> single
> * (micro)frame for a periodic transfer
> @@ -367,15 +509,35 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg
> *hsotg, struct dwc2_qh *qh)
> {
> int status;
>
> - status = dwc2_periodic_channel_available(hsotg);
> - if (status) {
> - dev_dbg(hsotg->dev,
> - "%s: No host channel available for periodic transfer\n",
> - __func__);
> - return 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;
This seems to schedule the qh for one frame earlier than what
find_uframe decided. Why is this? Especially if it happens for all
qh's, does it even matter?
> +
> + /* Set the new frame up */
> + if (frame > -1) {
> + qh->sched_frame &= ~0x7;
> + qh->sched_frame |= (frame & 7);
> + }
> +
> + if (status != -1)
> + 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);
> }
>
> - status = dwc2_check_periodic_bandwidth(hsotg, qh);
> if (status) {
> dev_dbg(hsotg->dev,
> "%s: Insufficient periodic bandwidth for periodic
> transfer\n",
Below here, dwc2_check_max_xfer_size is called, which could cause the
scheduling to fail and this function to return. If this happens, I think
the microframes claimed by the scheduler in dwc2_find_uframe above will
never be returned.
> @@ -391,16 +553,22 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg
> *hsotg, struct dwc2_qh *qh)
> return status;
> }
>
> - if (hsotg->core_params->dma_desc_enable > 0)
> + if (hsotg->core_params->dma_desc_enable > 0) {
> /* Don't rely on SOF and start in ready schedule */
> list_add_tail(&qh->qh_list_entry, &hsotg->periodic_sched_ready);
> - else
> + } else {
> + if (list_empty(&hsotg->periodic_sched_inactive) ||
> + dwc2_frame_num_le(qh->sched_frame, hsotg->next_sched_frame))
> + hsotg->next_sched_frame = qh->sched_frame;
> +
> /* Always start in inactive schedule */
> list_add_tail(&qh->qh_list_entry,
> &hsotg->periodic_sched_inactive);
> + }
>
> - /* Reserve periodic channel */
> - hsotg->periodic_channels++;
> + if (hsotg->core_params->uframe_sched <= 0)
> + /* Reserve periodic channel */
> + hsotg->periodic_channels++;
>
> /* Update claimed usecs per (micro)frame */
> hsotg->periodic_usecs += qh->usecs;
This last line could also be moved inside the if above it, since it is
only used by dwc2_check_periodic_bandwidth which is not used with
uframe_sched > 0.
> @@ -418,13 +586,22 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg
> *hsotg, struct dwc2_qh *qh)
> static void dwc2_deschedule_periodic(struct dwc2_hsotg *hsotg,
> struct dwc2_qh *qh)
> {
> - list_del_init(&qh->qh_list_entry);
> + int i;
>
> - /* Release periodic channel reservation */
> - hsotg->periodic_channels--;
> + list_del_init(&qh->qh_list_entry);
>
> /* Update claimed usecs per (micro)frame */
> hsotg->periodic_usecs -= qh->usecs;
> +
> + if (hsotg->core_params->uframe_sched > 0) {
> + for (i = 0; i < 8; i++) {
> + hsotg->frame_usecs[i] += qh->frame_usecs[i];
> + qh->frame_usecs[i] = 0;
> + }
> + } else {
> + /* Release periodic channel reservation */
> + hsotg->periodic_channels--;
> + }
> }
>
> /**
> @@ -454,6 +631,7 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct
> dwc2_qh *qh)
> /* Always start in inactive schedule */
> list_add_tail(&qh->qh_list_entry,
> &hsotg->non_periodic_sched_inactive);
> + hsotg->np_count++;
> } else {
> status = dwc2_schedule_periodic(hsotg, qh);
> if (status == 0) {
> @@ -557,6 +735,23 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg,
> struct dwc2_qh *qh,
> dev_vdbg(hsotg->dev, "%s()\n", __func__);
>
> if (dwc2_qh_is_non_per(qh)) {
> + struct dwc2_qh *qh_tmp;
> + struct list_head *qh_list;
> +
> + list_for_each(qh_list, &hsotg->non_periodic_sched_inactive) {
> + qh_tmp = list_entry(qh_list, struct dwc2_qh,
> + qh_list_entry);
> + if (qh_tmp == qh) {
> + /*
> + * IRQ is being disabled because this one never
> + * gets a np_count increment. This is still not
> + * absolutely correct, but it should fix itself
> + * with just an unnecessary extra interrupt.
> + */
> + hsotg->np_sent = hsotg->np_count;
> + }
> + }
> +
> dwc2_hcd_qh_unlink(hsotg, qh);
> if (!list_empty(&qh->qtd_list))
> /* Add back to inactive non-periodic schedule */
> @@ -581,12 +776,21 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg,
> struct dwc2_qh *qh,
> * Remove from periodic_sched_queued and move to
> * appropriate queue
> */
> - if (qh->sched_frame == frame_number)
> + if ((hsotg->core_params->uframe_sched > 0 &&
> + dwc2_frame_num_le(qh->sched_frame, frame_number))
> + || (hsotg->core_params->uframe_sched <= 0 &&
> + qh->sched_frame == frame_number)) {
This change seems unneeded to me. A bit further up, there is code (both
in dwc2_hcd_qh_deactivate itself and in the called
dwc2_sched_periodic_split) that makes sure that sched_frame is never
smaller than frame_number, so the old code is equivalent to the new
code in all possible cases I can see.
> list_move(&qh->qh_list_entry,
> &hsotg->periodic_sched_ready);
> - else
> + } else {
> + if (hsotg->core_params->uframe_sched > 0 &&
> + !dwc2_frame_num_le(hsotg->next_sched_frame,
> + qh->sched_frame))
> + hsotg->next_sched_frame =
> + qh->sched_frame;
> list_move(&qh->qh_list_entry,
> &hsotg->periodic_sched_inactive);
> + }
> }
> }
> }
> diff --git a/drivers/staging/dwc2/pci.c b/drivers/staging/dwc2/pci.c
> index fdfbc71..ace8682 100644
> --- a/drivers/staging/dwc2/pci.c
> +++ b/drivers/staging/dwc2/pci.c
> @@ -84,6 +84,7 @@ static const struct dwc2_core_params dwc2_module_params = {
> .ts_dline = -1,
> .reload_ctl = -1,
> .ahbcfg = -1,
> + .uframe_sched = -1,
> };
>
> /**
> --
> 1.8.2.rc0.16.g20a599e
>
Gr.
Matthijs
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
