Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels
On Tue, Oct 01, 2013 at 10:05:17AM +0300, Dan Carpenter wrote:
> On Tue, Oct 01, 2013 at 01:21:28AM +, Paul Zimmerman wrote:
> > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> > > Sent: Monday, September 30, 2013 6:09 PM
> > >
> > > Yeah. I guess it's fine... I was going to suggest adding the + 1 in a
> > > different place but actually it doesn't matter.
> > >
> > > The key to understanding dwc2_set_param_host_channels() is that the
> > > "val" parameter is always -1. That means it always returns -EINVAL and
> > > the caller jumbles the error code in with some other error codes and
> > > then ignores any errors. :/
> >
> > The intent of this was that the value can be overridden by the platform
> > code if required, in which case "val" would not be -1. However, to my
> > knowledge, no in-tree platforms do that, so I guess it would be fine to
> > redo this as you suggest below. We can always add it back if needed.
If we redo the dwc2_set_param_host_channels function, we should probably
redo _all_ of the dwc2_set_param_* functions, since they all do this.
> Why are we even adding one to the number of channels that the hardware
> reports?
Because that's how the hardware register is defined. I presume it never
makes sense to have 0 host channels, this allows a range of 1-16 instead
of 0-15.
In any case, for this particular problem, I would think that simply
making the host_channels field in dcw2_hw_params bigger is the better solution
here. E.g., something like:
struct dwc2_hw_params {
...
-unsigned host_channels:4;
+unsigned host_channels:5;
}
Moving the +1 calculation reverts the code back to what it was before one of my
patches. I moved the +1 to this place, so that the off-by-one detail of
the hardware register is only known at the place where hardware
registers are read. All other code can simply assume that the
"host_channels" variable contains just that: the number of host channels
present.
However, in that patch I apparently chose the wrong size for the
host_channels field, which I think should be problem to fix here.
Gr.
Matthijs
signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels
Hi Dinh, > >Somehow I assumed that was fixed by the hardware, but I see now that you > >are right. Yes, making the definition larger is better than moving the > >+ 1. > This was my original fix to the problem, but I thought that it would > be confusing when reading the code. I also thought about the "+1" > for host_channels was strange too. For debug outputs, it would be > more accurate to display 16 channels, in code-wise, I see that > host_channels is used in 2 for loops. Does it make sense to just fix > the for loops to include channels 0-15? I think that fixing this in the places where the value is used is moving the complexity the wrong way. Not sure if I'm understanding you correctly, thoguh. > >Dinh, do you want to do that? The other option is that Matthijs could > >fix it and give you the Reported-by credit. > I'm fine with that, if Matthijs wants to submit the fix. I can test > it on my hardware too. I'll prepare a patch in a few hours. Would be great if you could test, since my hardware only has a meagre 4 host channels ;-) Gr. Matthijs signature.asc Description: Digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels
Hey Dan, Dinh, > > [resend]: previous reply didn't include Matthijs > > > > He sets his Mail-Followup-To: so that we don't CC him on replies. I > assume it's deliberate because he only wants the copy from the mailing > list? Exactly, I just set that for whatever mailing list I subscribe to. However, since it's customary to explicitely CC involved people on the kernel mailing lists, I've just disabled this behaviour for the kernel lists, to prevent further confusion :-) Gr. Matthijs signature.asc Description: Digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: dwc2: Make dwc2_hw_params.host_channels large enough
The hardware offers a 4-bit register containing the number of host
channels. However, the values of these register mean 1-16 host channels,
not 0-15. Since the dwc2_hw_params struct stores the actual number of
host channels supported instead of the raw register value, it should be
5 bits wide instead of 4.
Before this commit, hardware with 16 host channels would overflow the
field, making it appear as 0 channels.
This bug was introduced in commit 9badec2 (staging: dwc2: interpret all
hwcfg and related register at init time).
Reported-by: Dinh Nguyen
Signed-off-by: Matthijs Kooijman
---
drivers/staging/dwc2/core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
index f7ba34b..fab718d 100644
--- a/drivers/staging/dwc2/core.h
+++ b/drivers/staging/dwc2/core.h
@@ -294,7 +294,7 @@ struct dwc2_hw_params {
unsigned dev_token_q_depth:5;
unsigned max_transfer_size:26;
unsigned max_packet_count:11;
- unsigned host_channels:4;
+ unsigned host_channels:5;
unsigned hs_phy_type:2;
unsigned fs_phy_type:2;
unsigned i2c_enable:1;
--
1.8.4.rc1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: dwc2: Make dwc2_hw_params.host_channels large enough
Hey Dinh, > >Reported-by: Dinh Nguyen > Can you please use: > Dinh Nguyen Sorry, I just used your sender address, should have checked your patch instead. Paul, if you can ack this patch, I'll resend it with the proper tag and include your acked-by as well. Gr. Matthijs signature.asc Description: Digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: dwc2: Make dwc2_hw_params.host_channels large enough
Hi Paul, > By the way, it looks like 'num_dev_ep' would have the same problem, I don't think so, since the hardware doesn't do the off-by-one trick there (presumably because having 0 endpoints make sense, but 0 host channels doesn't): hw->num_dev_ep = (hwcfg2 & GHWCFG2_NUM_DEV_EP_MASK) >> GHWCFG2_NUM_DEV_EP_SHIFT; > except it is not used because we don't support device mode yet. > That one and also 'num_dev_perio_in_ep' and 'dev_token_q_depth' should > be removed, I think. Doesn't hurt to keep them around, I think? When device mode is implemented, I guess this code will need to present exactly like it is now (unlike other device mode code, which likely needs to be adapted to whatever s3-hsotg is doing now)? Gr. Matthijs signature.asc Description: Digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: dwc2: Make dwc2_hw_params.host_channels large enough
The hardware offers a 4-bit register containing the number of host
channels. However, the values of these register mean 1-16 host channels,
not 0-15. Since the dwc2_hw_params struct stores the actual number of
host channels supported instead of the raw register value, it should be
5 bits wide instead of 4.
Before this commit, hardware with 16 host channels would overflow the
field, making it appear as 0 channels.
This bug was introduced in commit 9badec2 (staging: dwc2: interpret all
hwcfg and related register at init time).
Reported-by: Dinh Nguyen
Signed-off-by: Matthijs Kooijman
Acked-by: Paul Zimmerman
---
This is exactly the same patch as before, but with updated tags in the
commit message.
drivers/staging/dwc2/core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
index f7ba34b..fab718d 100644
--- a/drivers/staging/dwc2/core.h
+++ b/drivers/staging/dwc2/core.h
@@ -294,7 +294,7 @@ struct dwc2_hw_params {
unsigned dev_token_q_depth:5;
unsigned max_transfer_size:26;
unsigned max_packet_count:11;
- unsigned host_channels:4;
+ unsigned host_channels:5;
unsigned hs_phy_type:2;
unsigned fs_phy_type:2;
unsigned i2c_enable:1;
--
1.8.4.rc1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: dwc2: add driver parameter to set AHB config register value
Hi Paul,
On Tue, Jul 16, 2013 at 12:22:12PM -0700, Paul Zimmerman wrote:
> The dwc2 driver sets the value of the DWC2 GAHBCFG register to 0x6,
> which is GAHBCFG_HBSTLEN_INCR4. But different platforms may require
> different values. In particular, the Broadcom 2835 SOC used in the
> Raspberry Pi needs a value of 0x10, otherwise the DWC2 controller
> stops working after a short period of heavy USB traffic.
>
> So this patch adds another driver parameter named 'ahbcfg'. The
> default value is 0x6. Any platform needing a different value should
> add a DT attribute to set it.
>
> This patch also removes the 'ahb_single' driver parameter, since
> that bit can now be set using 'ahbcfg'.
>
> This patch does not add DT support to platform.c, I will leave that
> to whoever owns the first platform that needs a non-default value.
> (Stephen?)
I previously submitted a patch to load everything from DT, which might
serve as inspiration: http://article.gmane.org/gmane.linux.usb.general/85086
(but note this reply: http://article.gmane.org/gmane.linux.usb.general/85104)
> Signed-off-by: Paul Zimmerman
Reviewed-by: Matthijs Kooijman
However, I do wonder if it makes sense to let (almost) the entire
GAHBCFG be set from one variable. IMHO it would be more elegant to split
out the separate fields that can be set and have separate config
variables for them (I especially have the feeling that a literal
register value does not belong in a DT attribute, though I'm not quite
sure what's the policy there). Not sure if it's worth the extra effort,
though.
> -int dwc2_set_param_ahb_single(struct dwc2_hsotg *hsotg, int val)
> +int dwc2_set_param_ahbcfg(struct dwc2_hsotg *hsotg, int val)
> {
> - int valid = 1;
> - int retval = 0;
> -
> - if (DWC2_PARAM_TEST(val, 0, 1)) {
> - if (val >= 0) {
> - dev_err(hsotg->dev,
> - "'%d' invalid for parameter ahb_single\n", val);
> - dev_err(hsotg->dev, "ahb_single must be 0 or 1\n");
> - }
> - valid = 0;
> - }
> -
> - if (val > 0 && hsotg->snpsid < DWC2_CORE_REV_2_94a)
> - valid = 0;
> -
> - if (!valid) {
> - if (val >= 0)
> - dev_err(hsotg->dev,
> - "%d invalid for parameter ahb_single. Check HW
> configuration.\n",
> - val);
> - val = 0;
> - dev_dbg(hsotg->dev, "Setting ahb_single to %d\n", val);
> - retval = -EINVAL;
> - }
> -
> - hsotg->core_params->ahb_single = val;
> - return retval;
> + if (val != -1)
> + hsotg->core_params->ahbcfg = val;
> + else
> + hsotg->core_params->ahbcfg = GAHBCFG_HBSTLEN_INCR4;
> + return 0;
> }
Does it make sense to remove all the validity checking here? I could
imagine adding a check like:
if (val & GAHBCFG_CTRL_MASK) {
// Bits in GAHBCFG_CTRL_MASK cannot be set this way
...
valid = 0
}
to prevent silently dropping bits from an invalid value?
Also, it seems that there used to be a requirement of having at least
core revision 2.94a for types other than single. Is this no longer
relevant?
> diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
> index fc075a7..e771e40 100644
> --- a/drivers/staging/dwc2/core.h
> +++ b/drivers/staging/dwc2/core.h
> @@ -150,10 +150,11 @@ enum dwc2_lx_state {
> * are enabled
> * @reload_ctl: True to allow dynamic reloading of HFIR register
> during
> * runtime
> - * @ahb_single: This bit enables SINGLE transfers for remainder data
> in
> - * a transfer for DMA mode of operation.
> - * 0 - remainder data will be sent using INCR burst
> size
> - * 1 - remainder data will be sent using SINGLE burst
> size
> + * @ahbcfg: This field allows the default value of the GAHBCFG
> + * register to be overridden
> + * -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
Shouldn't this mention that a few of the bits in the register cannot be
set like this?
Gr.
Matthijs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: dwc2: add microframe scheduler from downstream Pi kernel
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
> ---
> 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.
Re: [PATCH] staging: dwc2: add driver parameter to set AHB config register value
Hey Paul,
> > > diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
> > > index fc075a7..e771e40 100644
> > > --- a/drivers/staging/dwc2/core.h
> > > +++ b/drivers/staging/dwc2/core.h
> > > @@ -150,10 +150,11 @@ enum dwc2_lx_state {
> > > * are enabled
> > > * @reload_ctl: True to allow dynamic reloading of HFIR register
> > > during
> > > * runtime
> > > - * @ahb_single: This bit enables SINGLE transfers for remainder
> > > data in
> > > - * a transfer for DMA mode of operation.
> > > - * 0 - remainder data will be sent using INCR
> > > burst size
> > > - * 1 - remainder data will be sent using SINGLE
> > > burst size
> > > + * @ahbcfg: This field allows the default value of the
> > > GAHBCFG
> > > + * register to be overridden
> > > + * -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
> > Shouldn't this mention that a few of the bits in the register cannot be
> > set like this?
>
> I could have added a comment along the lines of "see the HSOTG databook
> for valid values for this register". But e.g. the value used by the
> Broadcom SOC is not documented in the Synopsys databook. I'm guessing
> they made some enhancement to our core. So I don't even know what bits
> cannot be set in this register.
With "cannot be set", I meant the bits that are masked out by the driver
code (e.g., GAHBCFG_CTRL_MASK). I'll add a remark along those lines to
my "improve comments" patch I have queued, since your patch is already
picked up by Greg.
Gr.
Matthijs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: dwc2: add driver parameter to set AHB config register value
Hey Paul, one more thing: > > > > + * -1 - GAHBCFG value will not be > > > > overridden This seems incorrect: If it is set to -1, GAHBCFG will be set to 0x06 (INCR4), it is not left unchanged. I'll also include this in my documentation patch. Gr. Matthijs ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: dwc2: add microframe scheduler from downstream Pi kernel
Hi Paul,
> > 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).
It occurred to me that there is in fact more merit to this scheduler
than I originally thought. Before, I thought its only actual purpose was
to detect if there was room or not in the schedule for a new periodic
transfer. (and if I'm not mistaken, it does that badly when intervals of
< 8 uframes are involved).
However, what this scheduler also does is (obviously...) scheduling :-)
It makes sure that the various periodic transfers actually get sent to
the hardware spaced over different uframes instead of all at the first
or last uframe in a frame as before. This allows the driver to actually
accept more than 1 uframe worth of periodic transfers.
> > Overall, I don't think this patch is not even nearly ready to be
> > merged...
>
> Well, I disagree :)
I would argue that at least any questions about what this patch does and
why need to be resolved. Even if you did not write the code, you're the
one submitting it. Also, regardless of who wrote the code, the quality
of it should be high enough to accept in mainline. And frankly, I do not
think "But it works" is sufficient indication of quality by itself.
What I'm afraid of, is that if we introduce some code now that isn't
really needed or solves an unknown problem in the wrong way and we'll
end up carrying that code around without properly understanding what
it's for forever.
Of course this is just how I _think_ things should work, I'm not really
in a position to decide anything. So I'll just stick to commenting on
the code now, as far as I understand it, and try to understand better so
I can offer suggestions for improvements.
> > > - 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.
>
> Sure it can. If hsotg->free_hc_list is empty on entry to this
> function, for example.
Ah, you're right.
> > 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 0x
> > when no NAK was received.
> > * @frame_usecs:Internal variable used by the microframe scheduler
>
> No, its not noise. I took the opportunity while adding the new
> members to reorder the existing ones from smallest to largest, to
> reduce the amount of wasted space due to structure padding. I think
> that's a legitimate thing to do in this patch.
I'm not saying it's a useless change, I meant it is noise for _this_
patch and should go into a separate one.
Gr.
Matthijs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] staging: dwc2: add NAK holdoff patch from downstream Pi kernel
Hi Paul,
> Add the NAK holdoff patch from the downstream Raspberry Pi kernel.
> This allows the transfer scheduler to better handle "cheeky devices
> that just hold off using NAKs".
> @@ -365,6 +366,7 @@ struct dwc2_hsotg {
> u8 otg_port;
> u32 *frame_list;
> dma_addr_t frame_list_dma;
> + int next_sched_frame;
This variable is still not really used, I think. Most of the mentions in
the patch are assignments, except for these two:
> + 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;
> +
...
> + if (!dwc2_frame_num_le(hsotg->next_sched_frame,
> +qh->sched_frame))
> + hsotg->next_sched_frame =
> + qh->sched_frame;
However, these two "uses" of the variable have only the single purpose
of updating the variable itself, no other behaviour is influenced by it.
In effect, the variable does not influence the code at all and can be
dropped, significantly simplifying this patch.
Gr.
Matthijs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] staging: dwc2: add microframe scheduler from downstream Pi kernel
Hi Paul & Dom,
I haven't got time to look closely right now, but at first glance most
of my comments have been resolved. One thing that is still in there, is
this piece of code for which I'm not sure if it is really related to the
topic of the patch:
> @@ -780,6 +784,10 @@ static void dwc2_assign_and_init_hc(struct dwc2_hsotg
> *hsotg,
> chan->data_pid_start = qh->data_toggle;
> chan->multi_count = 1;
>
> + if ((urb->actual_length < 0 || urb->actual_length > urb->length) &&
> + !dwc2_hcd_is_pipe_in(&urb->pipe_info))
> + urb->actual_length = urb->length;
> +
> if (hsotg->core_params->dma_enable > 0) {
> chan->xfer_dma = urb->dma + urb->actual_length;
>
Paul, did you try the patch without this hunk?
Dom, can you tell use why this hunk is needed?
Gr.
Matthijs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/3] staging: dwc2: 2nd try at uframe scheduler patch
Hi Paul, > Matthijs' concern about periodic endpoints with bInterval=1 seems > to be unfounded. I tried a webcam, which uses a bInterval=1 isoc > endpoint, on my PCI-based dev board, and it still works fine with > this patch applied. For the record, I still think this concern actually exists, but you will need to completely fill up the available bandwidth to notice the problem, since the scheduler is now too optimistic about how much bandwidth is availble when devices with bInterval < 8 are present (but pessimistic for devices with bInterval > 8). However, this is something I think we can easily fix later, so getting this series in is a step forward anyway. I haven't got time to properly review and test these patches now, since I'm about to leave for a two-week vacation though. I did a quick scan just now and replied with some comments that occured to me. Gr. Matthijs ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] staging: dwc2: add NAK holdoff patch from downstream Pi kernel
Hi Paul,
> > > @@ -365,6 +366,7 @@ struct dwc2_hsotg {
> > > u8 otg_port;
> > > u32 *frame_list;
> > > dma_addr_t frame_list_dma;
> > > + int next_sched_frame;
> >
> > This variable is still not really used, I think. Most of the mentions in
> > the patch are assignments, except for these two:
> >
> > > + 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;
> > > +
> > ...
> > > + if (!dwc2_frame_num_le(hsotg->next_sched_frame,
> > > +qh->sched_frame))
> > > + hsotg->next_sched_frame =
> > > + qh->sched_frame;
> >
> > However, these two "uses" of the variable have only the single purpose
> > of updating the variable itself, no other behaviour is influenced by it.
> > In effect, the variable does not influence the code at all and can be
> > dropped, significantly simplifying this patch.
>
> Yep, that's kind of embarrassing. I will have to go back and read the
> downstream driver code again to figure out how this code is supposed to
> work. Thanks for the review.
Not sure if you figured this out already, but I seem to remember that
this next_sched_frame variable code was used by the driver to disable
the SOF interrupt when it wasn't needed and/or for this priority
interrupt thing on ARM (FIQ?). Not entirely sure, though.
Gr.
Matthijs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
