Re: [PATCH v1 04/14] usb: dwc2: Fix suspend state in host mode for partial power down.

[Doug Anderson]

Hi,

On Tue, Apr 30, 2019 at 12:11 AM Artur Petrosyan
 wrote:
>
> Hi,
>
> On 4/29/2019 21:35, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan
> >  wrote:
> >>
> >> Hi,
> >>
> >> On 4/27/2019 00:45, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> >>>  wrote:
> >>>>
> >>>> - In dwc2_port_suspend() function added waiting for the
> >>>> HPRT0.PrtSusp register field to be set.
> >>>>
> >>>> - In _dwc2_hcd_suspend() function added checking of
> >>>> "hsotg->flags.b.port_connect_status" port connection
> >>>> status if port connection status is 0 then skipping
> >>>> power saving (entering partial power down mode).
> >>>> Because if there is no device connected there would
> >>>> be no need to enter partial power down mode.
> >>>>
> >>>> - Added "hsotg->bus_suspended = true" beceuse after
> >>>
> >>> s/beceuse/because
> >>>
> >>>> entering partial power down in host mode the
> >>>> bus_suspended flag must be set.
> >>>
> >>> The above statement sounds to me like trying to win an argument by
> >>> saying "I'm right because I'm right."  Can you give more details about
> >>> why "bus_suspended" must be set?  See below also.
> >>>
> >> There is no intention of winning any argument.
> >
> > I was trying to say that your statement does not convey any
> > information about the "why".  It just says: "I now set this variable
> > because it needs to be set".  This tells me nothing useful because
> > presumably if it did't need to be set then you wouldn't have set it.
> > I want to know more information about how the code was broken before
> > you did this.  What specifically requires this variable to be set?
> Specifically that variable is set when core enters to hibernation or
> partial power down.
> >
> >
> >> Are you familiar with "bus_suspended" flag ? have you looked at what is
> >> it used for ?
> >>
> >>* @bus_suspended: True if bus is suspended
> >>
> >> So when entering to hibernation is performed bus is suspended. To
> >> indicate that "hsotg->bus_suspended" is assigned to true.
> >
> > Perhaps you can help me understand what the difference is between
> > "port suspend" and "bus suspend" on dwc2.  I think this is where my
> > confusion lies since there are functions for both and they do subtly
> > different things.  ...but both functions set bus_suspended.
> dwc2_port_suspend() is a function which is called when set port feature
> "USB_PORT_FEAT_SUSPEND" is asserted. Yet, bus_suspended is a variable.
> That variable should be set any time that host enters to hibernation or
> partial power down.
>
> >
> >
> >>>> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> >>>>   goto skip_power_saving;
> >>>>   }
> >>>>
> >>>> +   hsotg->bus_suspended = true;
> >>>> +
> >>>
> >>> I'm a bit unsure about this.  Specifically I note that
> >>> _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)".
> >>> Does that now become dead code?
> >> No it doesn't became a dead code.
> >
> > Can you explain when it gets triggered, then?
> _dwc2_hcd_resume() is triggered by the system.
> As an example lets assume you have hibernated the PC and then you turn
> the PC on. When you turn the PC back on in that case _dwc2_hcd_resume()
> function is called to resume from suspended state (Hibernation/partial
> power down)

I think you are still not understanding my question here.  Please
re-read it again.


-Doug

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 11:59 PM Artur Petrosyan
 wrote:
>
> On 4/29/2019 21:34, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
> >  wrote:
> >>
> >> On 4/27/2019 00:43, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> >>>  wrote:
> >>>>
> >>>> - Added backup of DCFG register.
> >>>> - Added Set the Power-On Programming done bit.
> >>>>
> >>>> Signed-off-by: Artur Petrosyan 
> >>>> ---
> >>>>drivers/usb/dwc2/gadget.c | 10 ++
> >>>>1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> >>>> index 6812a8a3a98b..dcb0fbb8bc42 100644
> >>>> --- a/drivers/usb/dwc2/gadget.c
> >>>> +++ b/drivers/usb/dwc2/gadget.c
> >>>> @@ -5004,6 +5004,7 @@ int dwc2_restore_device_registers(struct 
> >>>> dwc2_hsotg *hsotg, int remote_wakeup)
> >>>>{
> >>>>   struct dwc2_dregs_backup *dr;
> >>>>   int i;
> >>>> +   u32 dctl;
> >>>>
> >>>>   dev_dbg(hsotg->dev, "%s\n", __func__);
> >>>>
> >>>> @@ -5019,6 +5020,15 @@ int dwc2_restore_device_registers(struct 
> >>>> dwc2_hsotg *hsotg, int remote_wakeup)
> >>>>   if (!remote_wakeup)
> >>>>   dwc2_writel(hsotg, dr->dctl, DCTL);
> >>>>
> >>>> +   if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> >>>> +   dwc2_writel(hsotg, dr->dcfg, DCFG);
> >>>> +
> >>>> +   /* Set the Power-On Programming done bit */
> >>>> +   dctl = dwc2_readl(hsotg, DCTL);
> >>>> +   dctl |= DCTL_PWRONPRGDONE;
> >>>> +   dwc2_writel(hsotg, dctl, DCTL);
> >>>> +   }
> >>>
> >>> I can't vouch one way or the other for the correctness of this change,
> >>> but I will say that it's frustrating how asymmetric hibernate and
> >>> partial power down are.  It makes things really hard to reason about.
> >>> Is there any way you could restructure this so it happens in the same
> >>> way between hibernate and partial power down?
> >>>
> >>
> >>> Specifically looking at the similar sequence in
> >>> dwc2_gadget_exit_hibernation() (which calls this function), I see:
> >>>
> >>> 1. There are some extra delays.  Are those needed for partial power down?
> >> Do you mean delays in dwc2_gadget_exit_hibernation() ? If yes they are
> >> needed for hibernation flow. What relates to delays in hibernation
> >> needed for partial power down. Anything that is implemented in the
> >> hibernation delays or other things are part of hibernation and are not
> >> used in partial power down because they are two different flows of power
> >> saving.
> >
> > OK, if they aren't needed for partial power down then that's fine.
> > When I see two functions doing nearly the same sets of writes but one
> > has delays and the other doesn't it makes me wonder if that was on
> > purpose or not.
> >
> >
> >>> 2. For exiting hibernation the setting of "DCTL_PWRONPRGDONE" only
> >>> happens if "not remote wakeup".  Is it truly on purpose that you don't
> >>> do that?
> >> Currently partial power down doesn't support remote wakeup flow.
> >
> > Oh.  What happens if you have partial power down enabled and try to
> > enable remote wakeup?  Does it give an error?
> As far as I have been debugging I have not seen error in that case.
>
> Do you mean like it should print a message saying that current partial
> power down code doesn't support remote wakeup?

Not sure.  ...why don't we just forget about this question?  I don't
have enough gadget knowledge nor any way to test.

-Doug

Re: [PATCH v1 08/14] usb: dwc2: Add default param to control power optimization.

[Doug Anderson]

Hi,

On Tue, Apr 30, 2019 at 5:45 AM Artur Petrosyan
 wrote:
>
> > If setting "power_down = 0" is wrong then please update your patch to
> > remove all the mainline code that sets power_down to 0.  Presumably
> > this means you'd want to convert that code over to using "power_saving
> > = False".  Perhaps then I can see your vision of how this works more
> > clearly.
> Yes this is a good idea.

Actually, I have a feeling it won't work, at least not without more
changes.  ...and that's part of the problem with your patch.

Specifically dwc2 works by first filling in the "default" parameters.
Then the per-platform config function runs and overrides the defaults.
If the per-platform config function overrides the "power_saving"
parameter then it will be too late.


> > NOTE: I'm curious how you envision what someone would do if they had a
> > core that supported hibernation but they only wanted to enable partial
> > power down.  I guess then they'd have to set "power_saving = True" and
> > then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"?  I guess your
> > vision of the world is:
> I have implemented everything based on programming guide and data book.
> Core can only support hibernation or partial power down based on the
> configuration parameters. There cannot be two modes simultaneously of
> power saving only one of them.

Ah, this is new information to me.  I assumed they were supersets of
each other, so if you supported hibernation you also supported partial
power down.  Thanks for clearing that up!


> The power_down flag is set to DWC2_POWER_DOWN_PARAM_PARTIAL ,
> DWC2_POWER_DOWN_PARAM_HIBERNATION or DWC2_POWER_DOWN_PARAM_NONE while
> checking the hw parameters. So it just indicates which power down mode
> is supporting the core.

I don't think this is what the params are about.  The params are about
the currently configured parameters, not about what the core supports.
This is why all the other code checks the actual value of the params
to decide whether to do power savings.

-Doug

Re: [PATCH v1 14/14] usb: dwc2: Add enter/exit hibernation from system issued suspend/resume

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 5:01 AM Artur Petrosyan
 wrote:
>
> Hi,
>
> On 4/27/2019 01:01, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> >  wrote:
> >>
> >> Added a new flow of entering and exiting hibernation when PC is
> >> hibernated or suspended.
> >>
> >> Signed-off-by: Artur Petrosyan 
> >> ---
> >>   drivers/usb/dwc2/hcd.c | 128 
> >> +++--
> >>   1 file changed, 81 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> >> index 45d4a3e1ebd2..f1e92a287cb1 100644
> >> --- a/drivers/usb/dwc2/hcd.c
> >> +++ b/drivers/usb/dwc2/hcd.c
> >> @@ -4510,35 +4510,54 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> >>  if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
> >>  goto unlock;
> >>
> >> -   if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
> >> +   if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE ||
> >>  hsotg->flags.b.port_connect_status == 0)
> >>  goto skip_power_saving;
> >>
> >> -   /*
> >> -* Drive USB suspend and disable port Power
> >> -* if usb bus is not suspended.
> >> -*/
> >> -   if (!hsotg->bus_suspended) {
> >> -   hprt0 = dwc2_read_hprt0(hsotg);
> >> -   hprt0 |= HPRT0_SUSP;
> >> -   hprt0 &= ~HPRT0_PWR;
> >> -   dwc2_writel(hsotg, hprt0, HPRT0);
> >> -   spin_unlock_irqrestore(&hsotg->lock, flags);
> >> -   dwc2_vbus_supply_exit(hsotg);
> >> -   spin_lock_irqsave(&hsotg->lock, flags);
> >> -   }
> >> +   switch (hsotg->params.power_down) {
> >> +   case DWC2_POWER_DOWN_PARAM_PARTIAL:
> >> +   /*
> >> +* Drive USB suspend and disable port Power
> >> +* if usb bus is not suspended.
> >> +*/
> >> +   if (!hsotg->bus_suspended) {
> >> +   hprt0 = dwc2_read_hprt0(hsotg);
> >> +   hprt0 |= HPRT0_SUSP;
> >> +   hprt0 &= ~HPRT0_PWR;
> >> +   dwc2_writel(hsotg, hprt0, HPRT0);
> >> +   spin_unlock_irqrestore(&hsotg->lock, flags);
> >> +   dwc2_vbus_supply_exit(hsotg);
> >> +   spin_lock_irqsave(&hsotg->lock, flags);
> >> +   }
> >>
> >> -   /* Enter partial_power_down */
> >> -   ret = dwc2_enter_partial_power_down(hsotg);
> >> -   if (ret) {
> >> -   if (ret != -ENOTSUPP)
> >> -   dev_err(hsotg->dev,
> >> -   "enter partial_power_down failed\n");
> >> +   /* Enter partial_power_down */
> >> +   ret = dwc2_enter_partial_power_down(hsotg);
> >> +   if (ret) {
> >> +   if (ret != -ENOTSUPP)
> >> +   dev_err(hsotg->dev,
> >> +   "enter partial_power_down 
> >> failed\n");
> >> +   goto skip_power_saving;
> >> +   }
> >> +   hsotg->bus_suspended = true;
> >> +   break;
> >> +   case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> >> +   if (!hsotg->bus_suspended) {
> >
> > Do you have any idea why for DWC2_POWER_DOWN_PARAM_PARTIAL we still
> > call dwc2_enter_partial_power_down() even if bus_suspended is true,
> > but for hibernate you don't call dwc2_enter_hibernation()?
> For Hibernation I do call dwc2_enter_hibernation().

Maybe you didn't understand the question.  I'll be clearer.

Imagine _dwc2_hcd_suspend() is called but "bus_suspended" is already
true at the start of the function.

If we're in DWC2_POWER_DOWN_PARAM_PARTIAL, _dwc2_hcd_suspend() _will_
call dwc2_enter_partial_power_down()

If we're in DWC2_POWER_DOWN_PARAM_HIBERNATION, _dwc2_hcd_suspend()
_will NOT_ call dwc2_enter_partial_power_down()


This is all part of the whole asymmetry between PARTIAL and
HIBERNATION that makes it hard to understand.


> >> +   /* Enter hibernation

Re: [PATCH v1 08/14] usb: dwc2: Add default param to control power optimization.

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 4:30 AM Artur Petrosyan
 wrote:
>
> Hi,
>
> On 4/27/2019 00:46, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
> >  wrote:
> >>
> >> - Added a default param "power_saving" to enable or
> >>disable hibernation or partial power down features.
> >>
> >> - Printed hibernation param in hw_params_show and
> >>power_saving param in params_show.
> >>
> >> Signed-off-by: Artur Petrosyan 
> >> Signed-off-by: Minas Harutyunyan 
> >> ---
> >>   drivers/usb/dwc2/core.h|  3 +++
> >>   drivers/usb/dwc2/debugfs.c |  2 ++
> >>   drivers/usb/dwc2/params.c  | 19 +--
> >>   3 files changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> >> index 30bab8463c96..9221933ab64e 100644
> >> --- a/drivers/usb/dwc2/core.h
> >> +++ b/drivers/usb/dwc2/core.h
> >> @@ -373,6 +373,8 @@ enum dwc2_ep0_state {
> >>*  case.
> >>*  0 - No (default)
> >>*  1 - Yes
> >> + * @power_saving:  Specifies if power saving is enabled or not. If it 
> >> is
> >> + * enabled power_down functionality will be enabled.
> >>* @power_down: Specifies whether the controller support 
> >> power_down.
> >>* If power_down is enabled, the controller will 
> >> enter
> >>* power_down in both peripheral and host mode when
> >
> > Why are you adding a new parameter?  power_saving should be exactly
> > the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE".  Just use that
> > anywhere you need it.
> Customers should have a parameter using which they will disable entire
> power saving hibernation and Partial Power Down support.
>
> power_down is used to see which power saving mode we got
> (hibernation/partial power down).
>
> >
> > Having two parameters like you're doing is just asking for them to get
> > out of sync.  ...and, in fact, I think they will get out of sync.  On
> > rk3288, for instance:
> >
> > -> dwc2_set_default_params()
> > ---> power_saving = true
> > ---> dwc2_set_param_power_down()
> > -> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
> > -> set_params(), which is actually dwc2_set_rk_params()
> > ---> power_down = 0
> Setting power_down = 0  is a wrong and old option of disabling power
> saving feature because if we set power_down = 0 then it shows that there
> is no support for any power saving mode. That is why this patch is
> introduced to provide an easier way of disabling power saving modes.

If setting "power_down = 0" is wrong then please update your patch to
remove all the mainline code that sets power_down to 0.  Presumably
this means you'd want to convert that code over to using "power_saving
= False".  Perhaps then I can see your vision of how this works more
clearly.

NOTE: I'm curious how you envision what someone would do if they had a
core that supported hibernation but they only wanted to enable partial
power down.  I guess then they'd have to set "power_saving = True" and
then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"?  I guess your
vision of the world is:


// Example 1: Core supports power savings but we want disabled
// (no code since this is the default)

// Example 2: Pick the best power saving available
params->power_saving = True

// Example 3: Supports hibernation, but we only want partial:
params->power_saving = True
params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL


My vision of the world is:

// Example 1: Core supports power savings but we want disabled
params->power_down = DWC2_POWER_DOWN_PARAM_NONE

// Example 2: Pick the best power saving available
// (no code since this is the default)

// Example 3: Supports hibernation, but we only want partial:
params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL


I like that in my vision of the world "pick the best" is the default
(though I suppose we need to fix the driver so it actually works) and
that there's only one variable so you don't have extra confusion.


> > ...so at the end of dwc2_init_params() you will have power_saving =
> > true but power_down set to DWC2_POWER_DOWN_PARAM_NONE.  That seems
> > bad.  ...and, in fact:
> >
> > # grep '^power' /sys/kernel/debug/*.usb/params
> > /sys/kernel/debug/ff54.usb/params:power_saving  : 1

Re: [PATCH v1 04/14] usb: dwc2: Fix suspend state in host mode for partial power down.

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan
 wrote:
>
> Hi,
>
> On 4/27/2019 00:45, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> >  wrote:
> >>
> >> - In dwc2_port_suspend() function added waiting for the
> >>HPRT0.PrtSusp register field to be set.
> >>
> >> - In _dwc2_hcd_suspend() function added checking of
> >>"hsotg->flags.b.port_connect_status" port connection
> >>status if port connection status is 0 then skipping
> >>power saving (entering partial power down mode).
> >>Because if there is no device connected there would
> >>be no need to enter partial power down mode.
> >>
> >> - Added "hsotg->bus_suspended = true" beceuse after
> >
> > s/beceuse/because
> >
> >>entering partial power down in host mode the
> >>bus_suspended flag must be set.
> >
> > The above statement sounds to me like trying to win an argument by
> > saying "I'm right because I'm right."  Can you give more details about
> > why "bus_suspended" must be set?  See below also.
> >
> There is no intention of winning any argument.

I was trying to say that your statement does not convey any
information about the "why".  It just says: "I now set this variable
because it needs to be set".  This tells me nothing useful because
presumably if it did't need to be set then you wouldn't have set it.
I want to know more information about how the code was broken before
you did this.  What specifically requires this variable to be set?


> Are you familiar with "bus_suspended" flag ? have you looked at what is
> it used for ?
>
>   * @bus_suspended: True if bus is suspended
>
> So when entering to hibernation is performed bus is suspended. To
> indicate that "hsotg->bus_suspended" is assigned to true.

Perhaps you can help me understand what the difference is between
"port suspend" and "bus suspend" on dwc2.  I think this is where my
confusion lies since there are functions for both and they do subtly
different things.  ...but both functions set bus_suspended.


> >> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> >>  goto skip_power_saving;
> >>  }
> >>
> >> +   hsotg->bus_suspended = true;
> >> +
> >
> > I'm a bit unsure about this.  Specifically I note that
> > _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)".
> > Does that now become dead code?
> No it doesn't became a dead code.

Can you explain when it gets triggered, then?


-Doug

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
 wrote:
>
> On 4/27/2019 00:43, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> >  wrote:
> >>
> >> - Added backup of DCFG register.
> >> - Added Set the Power-On Programming done bit.
> >>
> >> Signed-off-by: Artur Petrosyan 
> >> ---
> >>   drivers/usb/dwc2/gadget.c | 10 ++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> >> index 6812a8a3a98b..dcb0fbb8bc42 100644
> >> --- a/drivers/usb/dwc2/gadget.c
> >> +++ b/drivers/usb/dwc2/gadget.c
> >> @@ -5004,6 +5004,7 @@ int dwc2_restore_device_registers(struct dwc2_hsotg 
> >> *hsotg, int remote_wakeup)
> >>   {
> >>  struct dwc2_dregs_backup *dr;
> >>  int i;
> >> +   u32 dctl;
> >>
> >>  dev_dbg(hsotg->dev, "%s\n", __func__);
> >>
> >> @@ -5019,6 +5020,15 @@ int dwc2_restore_device_registers(struct dwc2_hsotg 
> >> *hsotg, int remote_wakeup)
> >>  if (!remote_wakeup)
> >>  dwc2_writel(hsotg, dr->dctl, DCTL);
> >>
> >> +   if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> >> +   dwc2_writel(hsotg, dr->dcfg, DCFG);
> >> +
> >> +   /* Set the Power-On Programming done bit */
> >> +   dctl = dwc2_readl(hsotg, DCTL);
> >> +   dctl |= DCTL_PWRONPRGDONE;
> >> +   dwc2_writel(hsotg, dctl, DCTL);
> >> +   }
> >
> > I can't vouch one way or the other for the correctness of this change,
> > but I will say that it's frustrating how asymmetric hibernate and
> > partial power down are.  It makes things really hard to reason about.
> > Is there any way you could restructure this so it happens in the same
> > way between hibernate and partial power down?
> >
>
> > Specifically looking at the similar sequence in
> > dwc2_gadget_exit_hibernation() (which calls this function), I see:
> >
> > 1. There are some extra delays.  Are those needed for partial power down?
> Do you mean delays in dwc2_gadget_exit_hibernation() ? If yes they are
> needed for hibernation flow. What relates to delays in hibernation
> needed for partial power down. Anything that is implemented in the
> hibernation delays or other things are part of hibernation and are not
> used in partial power down because they are two different flows of power
> saving.

OK, if they aren't needed for partial power down then that's fine.
When I see two functions doing nearly the same sets of writes but one
has delays and the other doesn't it makes me wonder if that was on
purpose or not.


> > 2. For exiting hibernation the setting of "DCTL_PWRONPRGDONE" only
> > happens if "not remote wakeup".  Is it truly on purpose that you don't
> > do that?
> Currently partial power down doesn't support remote wakeup flow.

Oh.  What happens if you have partial power down enabled and try to
enable remote wakeup?  Does it give an error?


> > 3. I see that dctl gets "DCTL_PWRONPRGDONE" set as part of the
> > sequence in the "not remote wakeup" case before calling this function.
> > ...but then part of the function (that you didn't change) clobbers it,
> > I think.
> >
> Setting device programming done bit in dwc2_gadget_exit_hibernation()
> comes from older code and from debugging I noticed that if it is not
> done at that point then the flow brakes.
>
> So in partial power down flow we need to set that bit wile restoring
> device registers. That is why the implementation is such.
>
> >
> > I have no idea if any of that is a problem but the fact that the
> > hibernate and partial power down code runs through such different
> > paths makes it really hard to know / reason about.  Many of those
> > differences exist before your patch, but you're adding a new
> > difference rather than trying to unify and that worries me.
> >
> >
>
> So to make it easy to reason about I think we should debug it. Please
> point out where it fails. Have you tested this flow and did you see any
> wrong behavior of hibernation or partial power down? if yes please
> provide the debug logs so that they can be investigated.
>
> All of these patches have been tested on HAPS-DX and and Linaro HiKey
> 960 board. These patches fix Hibernation and Partial Power down issues.

I have no real way to test this code.  I'm only setup to use dwc2 as a
USB host since my target device is a laptop with type A ports on it.
Although one of the ports could be made a gadget and I could force the
mode and use an A-to-A cable, I don't have any use cases here nor do I
really have any experience using dwc2 as a gadget.

...so if you and others are happy with the code I won't stand in the
way--I was just reviewing the rest of the series so I figured I'd do a
cursory pass on this patch too.


-Doug

Re: [PATCH v1 14/14] usb: dwc2: Add enter/exit hibernation from system issued suspend/resume

[Doug Anderson]

Hi,

On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
 wrote:
>
> Added a new flow of entering and exiting hibernation when PC is
> hibernated or suspended.
>
> Signed-off-by: Artur Petrosyan 
> ---
>  drivers/usb/dwc2/hcd.c | 128 
> +++--
>  1 file changed, 81 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 45d4a3e1ebd2..f1e92a287cb1 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4510,35 +4510,54 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
> goto unlock;
>
> -   if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
> +   if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE ||
> hsotg->flags.b.port_connect_status == 0)
> goto skip_power_saving;
>
> -   /*
> -* Drive USB suspend and disable port Power
> -* if usb bus is not suspended.
> -*/
> -   if (!hsotg->bus_suspended) {
> -   hprt0 = dwc2_read_hprt0(hsotg);
> -   hprt0 |= HPRT0_SUSP;
> -   hprt0 &= ~HPRT0_PWR;
> -   dwc2_writel(hsotg, hprt0, HPRT0);
> -   spin_unlock_irqrestore(&hsotg->lock, flags);
> -   dwc2_vbus_supply_exit(hsotg);
> -   spin_lock_irqsave(&hsotg->lock, flags);
> -   }
> +   switch (hsotg->params.power_down) {
> +   case DWC2_POWER_DOWN_PARAM_PARTIAL:
> +   /*
> +* Drive USB suspend and disable port Power
> +* if usb bus is not suspended.
> +*/
> +   if (!hsotg->bus_suspended) {
> +   hprt0 = dwc2_read_hprt0(hsotg);
> +   hprt0 |= HPRT0_SUSP;
> +   hprt0 &= ~HPRT0_PWR;
> +   dwc2_writel(hsotg, hprt0, HPRT0);
> +   spin_unlock_irqrestore(&hsotg->lock, flags);
> +   dwc2_vbus_supply_exit(hsotg);
> +   spin_lock_irqsave(&hsotg->lock, flags);
> +   }
>
> -   /* Enter partial_power_down */
> -   ret = dwc2_enter_partial_power_down(hsotg);
> -   if (ret) {
> -   if (ret != -ENOTSUPP)
> -   dev_err(hsotg->dev,
> -   "enter partial_power_down failed\n");
> +   /* Enter partial_power_down */
> +   ret = dwc2_enter_partial_power_down(hsotg);
> +   if (ret) {
> +   if (ret != -ENOTSUPP)
> +   dev_err(hsotg->dev,
> +   "enter partial_power_down failed\n");
> +   goto skip_power_saving;
> +   }
> +   hsotg->bus_suspended = true;
> +   break;
> +   case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> +   if (!hsotg->bus_suspended) {

Do you have any idea why for DWC2_POWER_DOWN_PARAM_PARTIAL we still
call dwc2_enter_partial_power_down() even if bus_suspended is true,
but for hibernate you don't call dwc2_enter_hibernation()?


> +   /* Enter hibernation */
> +   spin_unlock_irqrestore(&hsotg->lock, flags);
> +   ret = dwc2_enter_hibernation(hsotg, 1);
> +   spin_lock_irqsave(&hsotg->lock, flags);
> +   if (ret && ret != -ENOTSUPP)
> +   dev_err(hsotg->dev,
> +   "%s: enter hibernation failed\n",
> +   __func__);

nit: no __func__ in dev_xxx() error messages.  The device plus the
message should be enough.  Only resort to __func__ if you're forced to
do an error message without a "struct device *".

nit: as per my comments in an earlier patch, remove special case for -ENOTSUPP

Also, presumably you want to match the error handling in
DWC2_POWER_DOWN_PARAM_PARTIAL and do a "goto skip_power_saving" when
you see an error?


> +   } else {
> +   goto skip_power_saving;
> +   }
> +   break;
> +   default:
> goto skip_power_saving;
> }
>
> -   hsotg->bus_suspended = true;
> -

It's a bit weird to remove this, but I guess it just got moved to the
partial power down case?  ...and in the hibernate case you're relying
on the hibernate function to set this?  Yet another frustratingly
asymmetric code structure...


> /* Ask phy to be suspended */
> if (!IS_ERR_OR_NULL(hsotg->uphy)) {
> spin_unlock_irqrestore(&hsotg->lock, flags);
> @@ -4564,17 +4583,17 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> int ret = 0;
> u32 hprt0;
>
> -   hprt0 = dwc2_read_hprt0(hsotg);
> -
> spin_lock_irqsave(&hsotg->lock, flags);
>
> -   if (dwc2_is_device_mode(hsotg))
> +   i

Re: [PATCH v1 12/14] usb: dwc2: Clear fifo_map when resetting core.

[Doug Anderson]

Hi,

On Fri, Apr 19, 2019 at 1:06 PM Artur Petrosyan
 wrote:
>
> If core enters host hibernation when switching from
> device mode to host mode disconnection of host cable
> asserts a WARNING in dwc2_hsotg_init_fifo() that
> fifo_map is not cleared.
>
> To avoid the WARNING, fifo_map should be cleared
> in dwc2_core_reset() function.
>
> - Cleared fifo_map when resetting the core.
>
> Signed-off-by: Artur Petrosyan 
> ---
>  drivers/usb/dwc2/core.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index fb471d18a3de..fbbd6a2f10ad 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -532,6 +532,12 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg, bool 
> skip_wait)
> greset |= GRSTCTL_CSFTRST;
> dwc2_writel(hsotg, greset, GRSTCTL);
>
> +   /* Clear fifo_map */
> +   #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
> +   IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
> +   hsotg->fifo_map = 0;
> +   #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */

Please please no sprinkling "#if" statements in code like this.  Here
lies the way to madness.  The right way to do this in the style of the
Linux kernel is to add a function that is a "static inline" no-op when
the options are compiled out.

As one example you can see my recent patch:

https://lkml.kernel.org/r/20190425154021.4465-1-diand...@chromium.org



-Doug

Re: [PATCH v1 09/14] usb: dwc2: Update dwc2_handle_usb_suspend_intr function.

[Doug Anderson]

Hi,

On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
 wrote:
>
> To avoid working in two modes (partial power down
> and hibernation) changed conditions for entering
> partial power down or hibernation.
>
> Instead of checking hw_params.power_optimized and
> hw_params.hibernation now checking power_down
> param which already set to one of the options
> (Hibernation or Partial Power Down) based on
> OTG_EN_PWROPT.
>
> Signed-off-by: Artur Petrosyan 
> Signed-off-by: Minas Harutyunyan 
> ---
>  drivers/usb/dwc2/core_intr.c | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)

In general I'm in support of this patch--it's cleaner and gets rid of
a needless goto \o/

...but you don't go far enough.  You can fully get rid of all of the
"-ENOTSUPP" stuff.  I've actually picked my patches and yours atop
Felipe's "testing/next" tree and you can find it here:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/sandbox/dianders/190426-dwc2-stuff

...as part of that I've included a patch ("usb: dwc2: Get rid of
useless error checks for hibernation/partial power down"), AKA:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0c924f736e2f7c1bb02531aa33c04a3ae5f4fc4c

Feel free to squash that into your patch or add it to your series if
you like it.  Note that patch points out that there's are still some
instances where calling dwc2_exit_partial_power_down() might still
happen in a case where it's not obvious if we were in partial power
down mode and made me wonder if there might be some bugs there.

-Doug

Re: [PATCH v1 08/14] usb: dwc2: Add default param to control power optimization.

[Doug Anderson]

Hi,

On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
 wrote:
>
> - Added a default param "power_saving" to enable or
>   disable hibernation or partial power down features.
>
> - Printed hibernation param in hw_params_show and
>   power_saving param in params_show.
>
> Signed-off-by: Artur Petrosyan 
> Signed-off-by: Minas Harutyunyan 
> ---
>  drivers/usb/dwc2/core.h|  3 +++
>  drivers/usb/dwc2/debugfs.c |  2 ++
>  drivers/usb/dwc2/params.c  | 19 +--
>  3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 30bab8463c96..9221933ab64e 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -373,6 +373,8 @@ enum dwc2_ep0_state {
>   *  case.
>   *  0 - No (default)
>   *  1 - Yes
> + * @power_saving:  Specifies if power saving is enabled or not. If it is
> + * enabled power_down functionality will be enabled.
>   * @power_down: Specifies whether the controller support power_down.
>   * If power_down is enabled, the controller will enter
>   * power_down in both peripheral and host mode when

Why are you adding a new parameter?  power_saving should be exactly
the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE".  Just use that
anywhere you need it.

Having two parameters like you're doing is just asking for them to get
out of sync.  ...and, in fact, I think they will get out of sync.  On
rk3288, for instance:

-> dwc2_set_default_params()
---> power_saving = true
---> dwc2_set_param_power_down()
-> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
-> set_params(), which is actually dwc2_set_rk_params()
---> power_down = 0


...so at the end of dwc2_init_params() you will have power_saving =
true but power_down set to DWC2_POWER_DOWN_PARAM_NONE.  That seems
bad.  ...and, in fact:

# grep '^power' /sys/kernel/debug/*.usb/params
/sys/kernel/debug/ff54.usb/params:power_saving  : 1
/sys/kernel/debug/ff54.usb/params:power_down: 0
/sys/kernel/debug/ff58.usb/params:power_saving  : 1
/sys/kernel/debug/ff58.usb/params:power_down: 0


...so while you could fix all of the various set_params() functions,
it seems better to just drop this patch since I don't think it buys
anything.

-Doug

Re: [PATCH v1 06/14] usb: dwc2: Add part. power down exit from dwc2_conn_id_status_change().

[Doug Anderson]

Hi,

On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
 wrote:
>
> Before changing to connector B exiting from Partial
> Power Down is required.
>
> - Added exiting from Partial Power Down mode when
>   connector ID status changes to "connId B".
>   Because if connector ID status changed to B connector
>   while core was in partial power down mode, HANG would
>   accrue from a soft reset.
>
> Signed-off-by: Artur Petrosyan 
> ---
>  drivers/usb/dwc2/hcd.c | 18 ++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 8367902a47eb..54450fa352cf 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3348,6 +3348,7 @@ static void dwc2_conn_id_status_change(struct 
> work_struct *work)
> wf_otg);
> u32 count = 0;
> u32 gotgctl;
> +   u32 pcgcctl;
> unsigned long flags;
>
> dev_dbg(hsotg->dev, "%s()\n", __func__);
> @@ -3387,6 +3388,23 @@ static void dwc2_conn_id_status_change(struct 
> work_struct *work)
> if (count > 250)
> dev_err(hsotg->dev,
> "Connection id status change timed out\n");
> +
> +   if (hsotg->params.power_down ==
> +   DWC2_POWER_DOWN_PARAM_PARTIAL &&
> +   hsotg->lx_state == DWC2_L2) {
> +   pcgcctl = dwc2_readl(hsotg, PCGCTL);
> +   pcgcctl &= ~PCGCTL_STOPPCLK;
> +   dwc2_writel(hsotg, pcgcctl, PCGCTL);
> +
> +   pcgcctl = dwc2_readl(hsotg, PCGCTL);
> +   pcgcctl &= ~PCGCTL_PWRCLMP;
> +   dwc2_writel(hsotg, pcgcctl, PCGCTL);
> +
> +   pcgcctl = dwc2_readl(hsotg, PCGCTL);
> +   pcgcctl &= ~PCGCTL_RSTPDWNMODULE;
> +   dwc2_writel(hsotg, pcgcctl, PCGCTL);

You really need to read it over and over?  ...and you really can't
just write this all at once?

...if this really needs to be separate steps, do you need a delay
between them or can they just happen as fast as possible?

As far as I can tell from the programming guide I've seen, you:

1. Don't need to read it over and over again.

2. Should delay 10 PHY clock cycles after the writes


-Doug

Re: [PATCH v1 05/14] usb: dwc2: Add port conn. sts. checking in _dwc2_hcd_resume() function.

[Doug Anderson]

Hi,

On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
 wrote:
>
> Added port connection status checking which prevents exiting from
> Partial Power Down mode from _dwc2_hcd_resume() when no entering
> to Partial Power Down mode happened.
>
> Signed-off-by: Artur Petrosyan 
> ---
>  drivers/usb/dwc2/hcd.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 1d18258b5ff8..8367902a47eb 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4544,6 +4544,9 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> unsigned long flags;
> int ret = 0;
> +   u32 hprt0;
> +
> +   hprt0 = dwc2_read_hprt0(hsotg);
>
> spin_lock_irqsave(&hsotg->lock, flags);

In this patch you put the reading of hprt0 before the spinlock (which
seems wrong) and then you move it after the spinlock in the last patch
in the series, AKA ("usb: dwc2: Add enter/exit hibernation from system
issued suspend/resume").  Can you just put it in the right place here
so you don't need to move it in a later patch?


-Doug

Re: [PATCH v1 04/14] usb: dwc2: Fix suspend state in host mode for partial power down.

[Doug Anderson]

Hi,

On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
 wrote:
>
> - In dwc2_port_suspend() function added waiting for the
>   HPRT0.PrtSusp register field to be set.
>
> - In _dwc2_hcd_suspend() function added checking of
>   "hsotg->flags.b.port_connect_status" port connection
>   status if port connection status is 0 then skipping
>   power saving (entering partial power down mode).
>   Because if there is no device connected there would
>   be no need to enter partial power down mode.
>
> - Added "hsotg->bus_suspended = true" beceuse after

s/beceuse/because

>   entering partial power down in host mode the
>   bus_suspended flag must be set.

The above statement sounds to me like trying to win an argument by
saying "I'm right because I'm right."  Can you give more details about
why "bus_suspended" must be set?  See below also.


> Signed-off-by: Artur Petrosyan 
> ---
>  drivers/usb/dwc2/hcd.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index dd82fa516f3f..1d18258b5ff8 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg 
> *hsotg, u16 windex)
> hprt0 |= HPRT0_SUSP;
> dwc2_writel(hsotg, hprt0, HPRT0);
>
> +   /* Wait for the HPRT0.PrtSusp register field to be set */
> +   if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000))
> +   dev_warn(hsotg->dev, "Suspend wasn't generated\n");
> +
> hsotg->bus_suspended = true;
>
> /*
> @@ -4488,7 +4492,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
> goto unlock;
>
> -   if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
> +   if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
> +   hsotg->flags.b.port_connect_status == 0)
> goto skip_power_saving;
>
> /*
> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> goto skip_power_saving;
> }
>
> +   hsotg->bus_suspended = true;
> +

I'm a bit unsure about this.  Specifically I note that
_dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)".
Does that now become dead code?

...I talk about this a bit more in my review of ("usb: dwc2: Add
enter/exit hibernation from system issued suspend/resume")


-Doug

Re: [PATCH v1 03/14] usb: dwc2: Fix wakeup detected and session request interrupt handlers.

[Doug Anderson]

Hi,

On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
 wrote:
>
> @@ -426,8 +438,6 @@ static void dwc2_handle_wakeup_detected_intr(struct 
> dwc2_hsotg *hsotg)
> /* Change to L0 state */
> hsotg->lx_state = DWC2_L0;
> } else {
> -   if (hsotg->params.power_down)
> -   return;
>

nit: you leave an odd blank line here.  Please delete it.

-Doug

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
 wrote:
>
> - Added backup of DCFG register.
> - Added Set the Power-On Programming done bit.
>
> Signed-off-by: Artur Petrosyan 
> ---
>  drivers/usb/dwc2/gadget.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 6812a8a3a98b..dcb0fbb8bc42 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -5004,6 +5004,7 @@ int dwc2_restore_device_registers(struct dwc2_hsotg 
> *hsotg, int remote_wakeup)
>  {
> struct dwc2_dregs_backup *dr;
> int i;
> +   u32 dctl;
>
> dev_dbg(hsotg->dev, "%s\n", __func__);
>
> @@ -5019,6 +5020,15 @@ int dwc2_restore_device_registers(struct dwc2_hsotg 
> *hsotg, int remote_wakeup)
> if (!remote_wakeup)
> dwc2_writel(hsotg, dr->dctl, DCTL);
>
> +   if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +   dwc2_writel(hsotg, dr->dcfg, DCFG);
> +
> +   /* Set the Power-On Programming done bit */
> +   dctl = dwc2_readl(hsotg, DCTL);
> +   dctl |= DCTL_PWRONPRGDONE;
> +   dwc2_writel(hsotg, dctl, DCTL);
> +   }

I can't vouch one way or the other for the correctness of this change,
but I will say that it's frustrating how asymmetric hibernate and
partial power down are.  It makes things really hard to reason about.
Is there any way you could restructure this so it happens in the same
way between hibernate and partial power down?

Specifically looking at the similar sequence in
dwc2_gadget_exit_hibernation() (which calls this function), I see:

1. There are some extra delays.  Are those needed for partial power down?

2. For exiting hibernation the setting of "DCTL_PWRONPRGDONE" only
happens if "not remote wakeup".  Is it truly on purpose that you don't
do that?

3. I see that dctl gets "DCTL_PWRONPRGDONE" set as part of the
sequence in the "not remote wakeup" case before calling this function.
...but then part of the function (that you didn't change) clobbers it,
I think.


I have no idea if any of that is a problem but the fact that the
hibernate and partial power down code runs through such different
paths makes it really hard to know / reason about.  Many of those
differences exist before your patch, but you're adding a new
difference rather than trying to unify and that worries me.



-Doug

Re: [balbi-usb:testing/next 19/42] drivers/usb/dwc2/core_intr.c:448:25: error: 'struct dwc2_hsotg' has no member named 'phy_reset_work'

[Doug Anderson]

Hi,

On Thu, Apr 25, 2019 at 8:02 AM Doug Anderson  wrote:
>
> Hi,
>
> On Thu, Apr 25, 2019 at 6:14 AM kbuild test robot  wrote:
> >
> >439  /*
> >440   * If we've got this quirk then the PHY is 
> > stuck upon
> >441   * wakeup.  Assert reset.  This will 
> > propagate out and
> >442   * eventually we'll re-enumerate the 
> > device.  Not great
> >443   * but the best we can do.  We can't call 
> > phy_reset()
> >444   * at interrupt time but there's no hurry, 
> > so we'll
> >445   * schedule it for later.
> >446   */
> >447  if (hsotg->reset_phy_on_wake)
> >  > 448  
> > schedule_work(&hsotg->phy_reset_work);
>
> Doh!  I didn't notice that the part of the structure I put this in has:
>
> #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
>
> I'll whip something up real quick that should fix it.  Sorry about that!

https://lkml.kernel.org/r/20190425154021.4465-1-diand...@chromium.org

-Doug

Re: [balbi-usb:testing/next 19/42] drivers/usb/dwc2/core_intr.c:448:25: error: 'struct dwc2_hsotg' has no member named 'phy_reset_work'

[Doug Anderson]

Hi,

On Thu, Apr 25, 2019 at 6:14 AM kbuild test robot  wrote:
>
>439  /*
>440   * If we've got this quirk then the PHY is 
> stuck upon
>441   * wakeup.  Assert reset.  This will 
> propagate out and
>442   * eventually we'll re-enumerate the device.  
> Not great
>443   * but the best we can do.  We can't call 
> phy_reset()
>444   * at interrupt time but there's no hurry, so 
> we'll
>445   * schedule it for later.
>446   */
>447  if (hsotg->reset_phy_on_wake)
>  > 448  schedule_work(&hsotg->phy_reset_work);

Doh!  I didn't notice that the part of the structure I put this in has:

#if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)

I'll whip something up real quick that should fix it.  Sorry about that!

-Doug

Re: lan78xx: About 8000 usb interrupts per second when idle

[Doug Anderson]

Hi,

On Sun, Apr 14, 2019 at 12:46 PM Lukas Wunner  wrote:
>
> On Tue, Apr 09, 2019 at 09:28:16AM +, Minas Harutyunyan wrote:
> > Am 09.04.19 um 08:54 schrieb Jisheng Zhang:
> > > The second one: 8000 usb interrupts per second when idle.
> > > This is abnormal. any idea? Is it due to the lan78xx?
> >
> > dwc2 in host mode enable SOF interrupts if any periodic EP are in use.
> > So, 8000 interrupts per second is expectant behavior.
>
> The dwc_otg driver patched into the Raspberry Pi Foundation's
> kernel seems to make do with much fewer interrupts and much
> lower CPU load.  How does it do that and how could dwc2 be
> made to do the same?  Would it be possible for you to provide
> me with documentation on the chip?  The Synopsis website
> requires registration for downloads and registration requires
> a Synopsis customer ID.
>
> It seems the Foundation's dwc_otg driver was forked from code
> that later begat dwc2.

Your information might be misleading.  The downstream dwc2 driver for
Raspberry PI handles the SoF interrupts at FIQ (fast interrupt) time.
The idea here is that this is to prioritize it above all other things
in the system since FIQ can fire even if we're currently in another
interrupt handler.

IIRC:

* FIQs don't get counted in /proc/interrupts.  So probably you really
are getting 8000 FIQs per second still, you just don't know it.

* FIQs don't count towards CPU load calculations, so it looks like the
CPU is less loaded by this than it really is.  I have no evidence
here, but I seem to remember someone telling me this, so if you
believe this is wrong then ignore it.


That all being said, though the purpose of using the FIQ is to improve
the latency of handling SoF interrupts, it is also plausible that when
it was written it also had the side effect of making the code more
efficient.  I mean, there's theoretically maybe some built-in
efficiency by skipping all the Linux interrupt infrastructure, but I'd
bet that's not a huge deal and a bigger deal is how inefficient the
mainline dwc2 driver is at handling interrupts.  I doubt you'd manage
to get FIQ support for something like this on mainline, but you could
possible spend more time improving the efficiency of the interrupt
handler.  I spent some time on this a while ago but it was just small
things--I didn't gut it and re-think how to make it faster.


Note also that I spent a bit of time a few years ago making the
upstream dwc2 driver more robust despite some of its inefficiencies.
In the end it was fairly robust, though if you wanted to do something
like audio or USB webcams with it you'd still struggle without higher
CPU speeds or patches like .  I'm still of
the belief that, unless the downstream driver has ported over the
uFrame work I did, that the upstream dwc2 driver will be compatible
with more more combinations of devices/hubs than the downstream one.


-Doug

Re: [RESEND PATCH v2] usb: dwc2: disable power_down on rockchip for regression

[Doug Anderson]

Hi,

On Fri, Oct 26, 2018 at 7:38 AM Hal Emmerich  wrote:
>
>
> From 04fbf78e4e569bf872f1ffcb0a6f9b89569dc913 Mon Sep 17 00:00:00 2001
> From: Hal Emmerich 
> Date: Thu, 19 Jul 2018 21:48:08 -0500
> Subject: [PATCH] usb: dwc2: disable power_down on rockchip devices
>
>  The bug would let the usb controller enter partial power down,
>  which was formally known as hibernate, upon boot if nothing was plugged
>  in to the port. Partial power down couldn't be exited properly, so any
>  usb devices plugged in after boot would not be usable.
>
>  Before the name change, params.hibernation was false by default, so
>  _dwc2_hcd_suspend() would skip entering hibernation. With the
>  rename, _dwc2_hcd_suspend() was changed to use  params.power_down
>  to decide whether or not to enter partial power down.
>
>  Since params.power_down is non-zero by default, it needs to be set
>  to 0 for rockchip devices to restore functionality.
>
>  This bug was reported in the linux-usb thread:
>  REGRESSION: usb: dwc2: USB device not seen after boot
>
>  The commit that caused this regression is:
>  6d23ee9caa6790aea047f9aca7f3c03cb8d96eb6
>
> Signed-off-by: Hal Emmerich 
> Acked-by: Minas Harutyunyan 
> ---
>  drivers/usb/dwc2/params.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index bf7052e037d6..09292dc977e4 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -81,6 +81,7 @@ static void dwc2_set_rk_params(struct dwc2_hsotg *hsotg)
> p->host_perio_tx_fifo_size = 256;
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
> GAHBCFG_HBSTLEN_SHIFT;
> +   p->power_down = 0;

This landed as commit c216765d3a1d ("usb: dwc2: disable power_down on
rockchip devices").  Can it please go to stable?  Hotplug of USB on
v4.19 stable, for example, is currently broken due to the lack of this
patch.

Thanks!

-Doug

Re: [PATCH v2] usb: dwc2: host: use hrtimer for NAK retries

[Doug Anderson]

Felipe,

On Tue, Nov 20, 2018 at 11:16 PM Minas Harutyunyan
 wrote:
>
> On 11/14/2018 3:30 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Sun, Sep 9, 2018 at 9:24 PM Terin Stock  wrote:
> >>
> >> Modify the wait delay utilize the high resolution timer API to allow for
> >> more precisely scheduled callbacks.
> >>
> >> A previous commit added a 1ms retry delay after multiple consecutive
> >> NAKed transactions using jiffies. On systems with a low timer interrupt
> >> frequency, this delay may be significantly longer than specified,
> >> resulting in misbehavior with some USB devices.
> >>
> >> This scenario was reached on a Raspberry Pi 3B with a Macally FDD-USB
> >> floppy drive (identified as 0424:0fdc Standard Microsystems Corp.
> >> Floppy, based on the USB97CFDC USB FDC). With the relay delay, the drive
> >> would be unable to mount a disk, replying with NAKs until the device was
> >> reset.
> >>
> >> Using ktime, the delta between starting the timer (in dwc2_hcd_qh_add)
> >> and the callback function can be determined. With the original delay
> >> implementation, this value was consistently approximately 12ms. (output
> >> in us).
> >>
> >>  -0 [000] ..s.  1600.559974: dwc2_wait_timer_fn: wait_timer 
> >> delta: 11976
> >>  -0 [000] ..s.  1600.571974: dwc2_wait_timer_fn: wait_timer 
> >> delta: 11977
> >>  -0 [000] ..s.  1600.583974: dwc2_wait_timer_fn: wait_timer 
> >> delta: 11976
> >>  -0 [000] ..s.  1600.595974: dwc2_wait_timer_fn: wait_timer 
> >> delta: 11977
> >>
> >> After converting the relay delay to using a higher resolution timer, the
> >> delay was much closer to 1ms.
> >>
> >>  -0 [000] d.h.  1956.553017: dwc2_wait_timer_fn: wait_timer 
> >> delta: 1002
> >>  -0 [000] d.h.  1956.554114: dwc2_wait_timer_fn: wait_timer 
> >> delta: 1002
> >>  -0 [000] d.h.  1957.542660: dwc2_wait_timer_fn: wait_timer 
> >> delta: 1004
> >>  -0 [000] d.h.  1957.543701: dwc2_wait_timer_fn: wait_timer 
> >> delta: 1002
> >>
> >> The floppy drive operates properly with delays up to approximately 5ms,
> >> and sends NAKs for any delays that are longer.
> >>
> >> Fixes: 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions 
> >> right away")
> >> Signed-off-by: Terin Stock 
> >> ---
> >>   drivers/usb/dwc2/hcd.h   |2 +-
> >>   drivers/usb/dwc2/hcd_queue.c |   19 ---
> >>   2 files changed, 13 insertions(+), 8 deletions(-)
> >
> > Reviewed-by: Douglas Anderson 
> > Cc: sta...@vger.kernel.org
> >
> Acked-by: Minas Harutyunyan 

It looks like Terin missed CCing you on the original patch.  I assume
you are still picking up dwc2 changes like this?  If so, should he
resend the patch targeting you (and including Minas and my tags) or do
you want to pick it up from one of the mailing lists he CCed?

Thanks!

-Doug

Re: [PATCH v2] usb: dwc2: host: use hrtimer for NAK retries

[Doug Anderson]

Hi,

On Sun, Sep 9, 2018 at 9:24 PM Terin Stock  wrote:
>
> Modify the wait delay utilize the high resolution timer API to allow for
> more precisely scheduled callbacks.
>
> A previous commit added a 1ms retry delay after multiple consecutive
> NAKed transactions using jiffies. On systems with a low timer interrupt
> frequency, this delay may be significantly longer than specified,
> resulting in misbehavior with some USB devices.
>
> This scenario was reached on a Raspberry Pi 3B with a Macally FDD-USB
> floppy drive (identified as 0424:0fdc Standard Microsystems Corp.
> Floppy, based on the USB97CFDC USB FDC). With the relay delay, the drive
> would be unable to mount a disk, replying with NAKs until the device was
> reset.
>
> Using ktime, the delta between starting the timer (in dwc2_hcd_qh_add)
> and the callback function can be determined. With the original delay
> implementation, this value was consistently approximately 12ms. (output
> in us).
>
> -0 [000] ..s.  1600.559974: dwc2_wait_timer_fn: wait_timer 
> delta: 11976
> -0 [000] ..s.  1600.571974: dwc2_wait_timer_fn: wait_timer 
> delta: 11977
> -0 [000] ..s.  1600.583974: dwc2_wait_timer_fn: wait_timer 
> delta: 11976
> -0 [000] ..s.  1600.595974: dwc2_wait_timer_fn: wait_timer 
> delta: 11977
>
> After converting the relay delay to using a higher resolution timer, the
> delay was much closer to 1ms.
>
> -0 [000] d.h.  1956.553017: dwc2_wait_timer_fn: wait_timer 
> delta: 1002
> -0 [000] d.h.  1956.554114: dwc2_wait_timer_fn: wait_timer 
> delta: 1002
> -0 [000] d.h.  1957.542660: dwc2_wait_timer_fn: wait_timer 
> delta: 1004
> -0 [000] d.h.  1957.543701: dwc2_wait_timer_fn: wait_timer 
> delta: 1002
>
> The floppy drive operates properly with delays up to approximately 5ms,
> and sends NAKs for any delays that are longer.
>
> Fixes: 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right 
> away")
> Signed-off-by: Terin Stock 
> ---
>  drivers/usb/dwc2/hcd.h   |2 +-
>  drivers/usb/dwc2/hcd_queue.c |   19 ---
>  2 files changed, 13 insertions(+), 8 deletions(-)

Reviewed-by: Douglas Anderson 
Cc: sta...@vger.kernel.org

Re: [PATCH] usb: dwc2: host: use hrtimer for NAK retries

[Doug Anderson]

Hi,

On Tue, Aug 28, 2018 at 11:04 AM, Terin Stock  wrote:
>> It would also be good to document what device you were plugging in
>> that you were having problems with and what system you were running
>> on.  That would help someone else if they ever wanted to modify the
>> same area of code and re-test.  They'd have a better chance of not
>> re-breaking you.
>
> This is a rather generic USB floppy drive. Is there a preference of
> vid and pid and/or product and manufacturer strings?

Whichever seems better to you is probably fine.  This just gives folks
a clue at trying to replicate your setup.  ;-)


>> Overall nit: please CC LKML on patches.  If nothing else that makes
>> them easier to find in lore.kernel.org/patchwork
>
> To be clear, is this CC on the email envelope?

Don't add "Cc:" of LKML in the patch description itself, just make
sure it's CCed on the message in git-send-email.

I'll put in my usual plug for considering "patman" to help post
patches.  That calls get_maintainer for you which always suggests
CCing LKML.  http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README

Re: [PATCH] usb: dwc2: host: use hrtimer for NAK retries

[Doug Anderson]

Hi,

On Sat, Aug 25, 2018 at 8:45 PM, Terin Stock  wrote:
> Upon upgrading a Raspberry Pi 3B-based project from vanilla 4.14,
> attempts to mount a floppy disk in a generic USB floppy drive would hang
> until the floppy drive was removed from the system.
>
> Tracing shows that during mounting the drive produces a large amount of
> NAKed transactions, but would eventually continue. A previous commit
> added a retry delay on NAKed transactions, using jiffies, that results
> in indefinite NAKs in this scenario.
>
> Modify the wait delay utilize the high resolution timer API to allow for
> more accurately scheduled callbacks.

I think your commit will be more compelling with additional data.  As
Allen says it looks like you're not actually changing the delay.  You
could include things like:

* On systems with 100 HZ in the ideal case we'd end up delaying for 10
ms - 20 ms when we used jiffies.  Now we'll get much closer to 1 ms.

* Timer functions are not very high priority, so if we were running at
a high load then we'd sometimes see much longer delays.  (NOTE: if you
say this then please back it up with data--I think I've heard
anecdotally that the normal timer functions can be quite delayed but I
haven't done the research to back it up).  Presumably you could use
ktime to measure delays before and after your patch and you could
include this in the commit message.


It would also be good to document what device you were plugging in
that you were having problems with and what system you were running
on.  That would help someone else if they ever wanted to modify the
same area of code and re-test.  They'd have a better chance of not
re-breaking you.


NOTE: it's possible that the problem here is just that the USB device
you're plugging in is not very forgiving to the kernel taking a long
time to talk to it again after the NAK.  Having such a long delay here
isn't common and presumably the device you have just doesn't handle
it.  Possibly the device is non-compliant (I'm not enough of an expert
on the spec), but it's still nice to try to support it.


> Fixes: 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right 
> away")
> Signed-off-by: Terin Stock 
> ---
>  drivers/usb/dwc2/hcd.h   |2 +-
>  drivers/usb/dwc2/hcd_queue.c |   17 ++---
>  2 files changed, 11 insertions(+), 8 deletions(-)

Overall nit: please CC LKML on patches.  If nothing else that makes
them easier to find in lore.kernel.org/patchwork


> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
> index 5502a501f516..93483dc37801 100644
> --- a/drivers/usb/dwc2/hcd.h
> +++ b/drivers/usb/dwc2/hcd.h
> @@ -366,7 +366,7 @@ struct dwc2_qh {
> u32 desc_list_sz;
> u32 *n_bytes;
> struct timer_list unreserve_timer;
> -   struct timer_list wait_timer;
> +   struct hrtimer wait_timer;
> struct dwc2_tt *dwc_tt;
> int ttport;
> unsigned tt_buffer_dirty:1;
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index 301ced1618f8..2d0cfd7f2cfe 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -59,7 +59,7 @@
>  #define DWC2_UNRESERVE_DELAY (msecs_to_jiffies(5))
>
>  /* If we get a NAK, wait this long before retrying */
> -#define DWC2_RETRY_WAIT_DELAY (msecs_to_jiffies(1))
> +#define DWC2_RETRY_WAIT_DELAY 1*1E6L
>
>  /**
>   * dwc2_periodic_channel_available() - Checks that a channel is available 
> for a
> @@ -1465,9 +1465,9 @@ static void dwc2_deschedule_periodic(struct dwc2_hsotg 
> *hsotg,
>   *
>   * @t: Pointer to wait_timer in a qh.
>   */
> -static void dwc2_wait_timer_fn(struct timer_list *t)
> +static enum hrtimer_restart dwc2_wait_timer_fn(struct hrtimer *t)

nit: please update function docstring to include a "Return:" clause now.


Other than the above things look pretty good here to me.  Thanks for the patch!

-Doug

Re: [PATCH 2/2] usb: dwc2: Fix inefficient copy of unaligned buffers

[Doug Anderson]

Hi,

On Thu, Jul 5, 2018 at 7:31 AM, Antti Seppälä  wrote:
> Make sure only to copy any actual data rather than the whole buffer,
> when releasing the temporary buffer used for unaligned non-isochronous
> transfers.
>
> Taken directly from commit 0efd937e27d5e ("USB: ehci-tegra: fix inefficient
> copy of unaligned buffers")
>
> Tested with Lantiq xRX200 (MIPS) and RPi Model B Rev 2 (ARM)
>
> Signed-off-by: Antti Seppälä 
> ---
>  drivers/usb/dwc2/hcd.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)

I haven't personally gone down and tracked down the validity of
"actual_length", but I agree that this matches what tegra has done and
seems sane.

Reviewed-by: Douglas Anderson 
--
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

Re: [PATCH 1/2] usb: dwc2: Fix DMA alignment to start at allocated boundary

[Doug Anderson]

Hi,

On Thu, Jul 5, 2018 at 7:31 AM, Antti Seppälä  wrote:
> The commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more
> supported way") introduced a common way to align DMA allocations.
> The code in the commit aligns the struct dma_aligned_buffer but the
> actual DMA address pointed by data[0] gets aligned to an offset from
> the allocated boundary by the kmalloc_ptr and the old_xfer_buffer
> pointers.
>
> This is against the recommendation in Documentation/DMA-API.txt which
> states:
>
>   Therefore, it is recommended that driver writers who don't take
>   special care to determine the cache line size at run time only map
>   virtual regions that begin and end on page boundaries (which are
>   guaranteed also to be cache line boundaries).
>
> The effect of this is that architectures with non-coherent DMA caches
> may run into memory corruption or kernel crashes with Unhandled
> kernel unaligned accesses exceptions.
>
> Fix the alignment by positioning the DMA area in front of the allocation
> and use memory at the end of the area for storing the orginal
> transfer_buffer pointer. This may have the added benefit of increased
> performance as the DMA area is now fully aligned on all architectures.
>
> Tested with Lantiq xRX200 (MIPS) and RPi Model B Rev 2 (ARM).
>
> Fixes: 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more
> supported way")
>
> Signed-off-by: Antti Seppälä 
> ---
>  drivers/usb/dwc2/hcd.c | 44 +++-
>  1 file changed, 23 insertions(+), 21 deletions(-)

Thanks for tracking this down and sorry for the original regression.
Seems like a good fix.  With this fix, I'd be curious of your
observations on how dwc2 performs (both performance and compatibility
under stress) with the newest driver compared to whatever you were
using before.

Also: you're using the dwc2_set_ltq_params() parameters?  Have you
checked if removing the "max_transfer_size" limit boosts your
performance?

Cc: sta...@vger.kernel.org
Reviewed-by: Douglas Anderson 
--
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

Re: usb: dwc2: crash regression in commit 3bc04e28a030 (bisected)

[Doug Anderson]

Hi,

On Sun, Jul 1, 2018 at 11:30 PM, Antti Seppälä  wrote:
> On 30 June 2018 at 02:57, Doug Anderson  wrote:
>> Hi,
>>
>> On Fri, Jun 29, 2018 at 11:29 AM, Antti Seppälä  wrote:
>>> Hi Doug, John and linux-usb.
>>>
>>> I'd like to report a regression in commit 3bc04e28a030 (usb: dwc2:
>>> host: Get aligned DMA in a more supported way)
>>
>> Seems unlikely, but any chance that
>> <https://patchwork.kernel.org/patch/10393775/> helps you?
>>
>
> Thank you for your suggestion but unfortunately the patch does not
> help and the crash remains.

A few more shots in the dark in case they help:

1. For the kmalloc() in dwc2_alloc_dma_aligned_buffer(), change from:

kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);

to:

kmalloc_ptr = kmalloc(kmalloc_size, mem_flags | GFP_DMA);


The old code used to hardcode this, so maybe it somehow makes a difference?

---

2. Change DWC2_USB_DMA_ALIGN to a larger size.  Maybe 32 or 64?

---

3. Undo just the part of the patch that removed:

/*
 * Clip max_transfer_size to 65535. dwc2_hc_setup_align_buf() allocates
 * coherent buffers with this size, and if it's too large we can
 * exhaust the coherent DMA pool.
 */
if (hw->max_transfer_size > 65535)
  hw->max_transfer_size = 65535;

Maybe there's something on your platform where you have a problem with
big transfers?

===

It feels like there's something unique about your setup since I
haven't heard about this crash and that patch is 1.5 years old.
Certainly it could be the MIPS / Big Endian, but I suppose it could
also be the driver for the USB device you're plugging in.  Any chance
the new code is just tickling a problem in that driver?  Can you
reproduce similar crashes with any other USB devices on your platform?


-Doug
--
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

Re: usb: dwc2: crash regression in commit 3bc04e28a030 (bisected)

[Doug Anderson]

Hi,

On Fri, Jun 29, 2018 at 11:29 AM, Antti Seppälä  wrote:
> Hi Doug, John and linux-usb.
>
> I'd like to report a regression in commit 3bc04e28a030 (usb: dwc2:
> host: Get aligned DMA in a more supported way)

Seems unlikely, but any chance that
 helps you?


> Apparently the patch does something nasty that the Lantiq platform
> really does not like as whenever I plug my usb 3g modem into the usb
> port of my BT HomeHub v5 (Lantiq XRX200) I get a kernel
> oops/crash/panic with usually quite a weird content that looks like
> some sort of memory corruption.
>
> I've bisected the crash and reverting 3bc04e28a030 allows the 3g-modem
> to be plugged and the kernel does not crash.
>
> Below is the console log when I plug the modem in. I used stable
> vanilla kernel 4.9.109 from OpenWrt during my tests with dwc2 debug
> prints enabled:
>
> root@lantiq:/# echo -n "module dwc2 +p" >
> /sys/kernel/debug/dynamic_debug/control
> [   92.563454] dwc2 1e101000.ifxhcd: gintsts=0521  gintmsk=f3000806
> [   92.568762] dwc2 1e101000.ifxhcd: DWC OTG HCD HUB STATUS DATA: Root
> port status changed
> [   92.576447] dwc2 1e101000.ifxhcd:   port_connect_status_change: 1
> [   92.582523] dwc2 1e101000.ifxhcd:   port_reset_change: 0
> [   92.587830] dwc2 1e101000.ifxhcd:   port_enable_change: 0
> [   92.593228] dwc2 1e101000.ifxhcd:   port_suspend_change: 0
> [   92.598710] dwc2 1e101000.ifxhcd:   port_over_current_change: 0
> [   92.607242] dwc2 1e101000.ifxhcd: ClearPortFeature 
> USB_PORT_FEAT_C_CONNECTION
> [   92.758240] dwc2 1e101000.ifxhcd: SetPortFeature
> [   92.761535] dwc2 1e101000.ifxhcd: SetPortFeature - USB_PORT_FEAT_RESET
> [   92.768063] dwc2 1e101000.ifxhcd: In host mode, hprt0=00021501
> [   92.841013] dwc2 1e101000.ifxhcd: gintsts=0529  gintmsk=f3000806
> [   92.905329] dwc2 1e101000.ifxhcd: ClearPortFeature USB_PORT_FEAT_C_RESET
> [   92.968536] usb 1-1: new high-speed USB device number 2 using dwc2
> [   92.975029] dwc2 1e101000.ifxhcd: SetPortFeature
> [   92.978358] dwc2 1e101000.ifxhcd: SetPortFeature - USB_PORT_FEAT_RESET
> [   92.984837] dwc2 1e101000.ifxhcd: In host mode, hprt0=1101
> [   92.990674] dwc2 1e101000.ifxhcd: gintsts=0529  gintmsk=f3000806
> [   93.060349] dwc2 1e101000.ifxhcd: DWC OTG HCD HUB STATUS DATA: Root
> port status changed
> [   93.067020] dwc2 1e101000.ifxhcd:   port_connect_status_change: 0
> [   93.073099] dwc2 1e101000.ifxhcd:   port_reset_change: 0
> [   93.078375] dwc2 1e101000.ifxhcd:   port_enable_change: 1
> [   93.083766] dwc2 1e101000.ifxhcd:   port_suspend_change: 0
> [   93.089252] dwc2 1e101000.ifxhcd:   port_over_current_change: 0
> [   93.096284] dwc2 1e101000.ifxhcd: gintsts=0529  gintmsk=f3000806
> [   93.152672] dwc2 1e101000.ifxhcd: ClearPortFeature USB_PORT_FEAT_C_RESET
> [   93.216952] dwc2 1e101000.ifxhcd: DWC OTG HCD EP DISABLE:
> bEndpointAddress=0x00, ep->hcpriv=86e4fa00
> [   93.224792] dwc2 1e101000.ifxhcd: DWC OTG HCD EP DISABLE:
> bEndpointAddress=0x00, ep->hcpriv=  (null)
> [   93.233926] dwc2 1e101000.ifxhcd: DWC OTG HCD EP RESET: 
> bEndpointAddress=0x00
> [   93.268547] dwc2 1e101000.ifxhcd: DWC OTG HCD EP RESET: 
> bEndpointAddress=0x81
> [   93.274399] dwc2 1e101000.ifxhcd: DWC OTG HCD EP RESET: 
> bEndpointAddress=0x01
> [   93.307463] usb-storage 1-1:1.0: USB Mass Storage device detected
> [   93.312238] dwc2 1e101000.ifxhcd: DWC OTG HCD HUB STATUS DATA: Root
> port status changed
> [   93.312256] dwc2 1e101000.ifxhcd:   port_connect_status_change: 0
> [   93.312270] dwc2 1e101000.ifxhcd:   port_reset_change: 0
> [   93.312329] dwc2 1e101000.ifxhcd:   port_enable_change: 1
> [   93.312342] dwc2 1e101000.ifxhcd:   port_suspend_change: 0
> [   93.312356] dwc2 1e101000.ifxhcd:   port_over_current_change: 0
> [   93.408514] scsi host0: usb-storage 1-1:1.0
> [   93.437010] dwc2 1e101000.ifxhcd: ClearPortFeature USB_PORT_FEAT_C_ENABLE
> [   94.152597] dwc2 1e101000.ifxhcd: DWC OTG HCD EP RESET: 
> bEndpointAddress=0x01
> [   94.166421] dwc2 1e101000.ifxhcd: gintsts=2529  gintmsk=f3000806
> [   94.171336] dwc2 1e101000.ifxhcd: ++Disconnect Detected Interrupt++
> (Host) a_host
> [   94.180561] dwc2 1e101000.ifxhcd: DWC OTG HCD EP RESET: 
> bEndpointAddress=0x01
> [   94.186473] dwc2 1e101000.ifxhcd: Not connected
> [   94.300415] dwc2 1e101000.ifxhcd: DWC OTG HCD HUB STATUS DATA: Root
> port status changed
> [   94.307074] dwc2 1e101000.ifxhcd:   port_connect_status_change: 1
> [   94.313203] dwc2 1e101000.ifxhcd:   port_reset_change: 0
> [   94.318467] dwc2 1e101000.ifxhcd:   port_enable_change: 1
> [   94.323858] dwc2 1e101000.ifxhcd:   port_suspend_change: 0
> [   94.329344] dwc2 1e101000.ifxhcd:   port_over_current_change: 0
> [   94.336998] dwc2 1e101000.ifxhcd: ClearPortFeature 
> USB_PORT_FEAT_C_CONNECTION
> [   94.343368] dwc2 1e101000.ifxhcd: ClearPortFeature USB_PORT_FEAT_C_ENABLE
> [   94.350145] usb 1-1: USB disconnect, device number 2
> [   94.365605] dwc2 1e101000.ifxh

Re: [PATCH] arm64: dts: msm8996: Use dwc3-qcom glue driver for USB

[Doug Anderson]

Hi,

On Wed, May 30, 2018 at 4:04 AM, Manu Gautam  wrote:
> Move from dwc3-of-simple to dwc3-qcom glue driver to
> support peripheral mode which requires qscratch wrapper
> programming on VBUS event.

Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")

> Signed-off-by: Manu Gautam 
> ---
>  arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++
>  arch/arm64/boot/dts/qcom/msm8996.dtsi| 6 --
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi 
> b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> index f45a0ab30d30..83bc1b9ff6ef 100644
> --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> @@ -108,6 +108,7 @@
>
> usb@6a0 {
> status = "okay";
> +   extcon = <&usb3_id>;
>
> dwc3@6a0 {
> extcon = <&usb3_id>;
> @@ -124,6 +125,7 @@
>
> usb@760 {
> status = "okay";
> +   extcon = <&usb2_id>;
>
> dwc3@760 {
> extcon = <&usb2_id>;
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi 
> b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 26292027ba9b..d30516c0db87 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -777,7 +777,8 @@
> };
>
> usb2: usb@760 {

Please update the unit address, which should match the new value for the "reg".

> -   compatible = "qcom,dwc3";
> +   compatible = "qcom,msm8996-dwc3", "qcom,dwc3";
> +   reg = <0x76f8800 0x400>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> @@ -805,7 +806,8 @@
> };
>
> usb3: usb@6a0 {
> -   compatible = "qcom,dwc3";
> +   compatible = "qcom,msm8996-dwc3", "qcom,dwc3";
> +   reg = <0x6af8800 0x400>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;

This patch is a little bit unfortunate since it demonstrates that a
patch that already landed (specifically commit a4333c3a6ba9 ("usb:
dwc3: Add Qualcomm DWC3 glue driver")) broke device tree backward
compatibility.  Specifically if you get a kernel with that commit in
it but not this device tree change then you'll end up with broken USB.
Also if anyone happened to have a device tree stored somewhere out of
tree based on the old binding then they will of course be broken.

I will leave it to people more familiar with msm8996 to decide if this
issue is important to them and if we need to change the bindings to
maintain backward compatibility or if we should instead just land this
patch ASAP.

-Doug
--
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

Re: [PATCH] usb: dwc2: fix the incorrect bitmaps for the ports of multi_tt hub

[Doug Anderson]

Hi,

On Mon, May 21, 2018 at 3:12 AM, William Wu  wrote:
> The dwc2_get_ls_map() use ttport to reference into the
> bitmap if we're on a multi_tt hub. But the bitmaps index
> from 0 to (hub->maxchild - 1), while the ttport index from
> 1 to hub->maxchild. This will cause invalid memory access
> when the number of ttport is hub->maxchild.
>
> Without this patch, I can easily meet a Kernel panic issue
> if connect a low-speed USB mouse with the max port of FE2.1
> multi-tt hub (1a40:0201) on rk3288 platform.
>
> Signed-off-by: William Wu 
> ---
>  drivers/usb/dwc2/hcd_queue.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index d7c3d6c..9c55d1a 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -383,7 +383,7 @@ static unsigned long *dwc2_get_ls_map(struct dwc2_hsotg 
> *hsotg,
> /* Get the map and adjust if this is a multi_tt hub */
> map = qh->dwc_tt->periodic_bitmaps;
> if (qh->dwc_tt->usb_tt->multi)
> -   map += DWC2_ELEMENTS_PER_LS_BITMAP * qh->ttport;
> +   map += DWC2_ELEMENTS_PER_LS_BITMAP * (qh->ttport - 1);

Oops, thanks for the fix.

Fixes: 9f9f09b048f5 ("usb: dwc2: host: Totally redo the microframe scheduler")
Cc: sta...@vger.kernel.org
Reviewed-by: Douglas Anderson 

-Doug
--
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

Re: [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Doug Anderson]

Hi,

On Wed, May 9, 2018 at 3:11 AM, William Wu  wrote:
> The commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in
> a more supported way") rips out a lot of code to simply the
> allocation of aligned DMA. However, it also introduces a new
> issue when use isoc split in transfer.
>
> In my test case, I connect the dwc2 controller with an usb hs
> Hub (GL852G-12), and plug an usb fs audio device (Plantronics
> headset) into the downstream port of Hub. Then use the usb mic
> to record, we can find noise when playback.
>
> It's because that the usb Hub uses an MDATA for the first
> transaction and a DATA0 for the second transaction for the isoc
> split in transaction. An typical isoc split in transaction sequence
> like this:
>
> - SSPLIT IN transaction
> - CSPLIT IN transaction
>   - MDATA packet
> - CSPLIT IN transaction
>   - DATA0 packet
>
> The DMA address of MDATA (urb->dma) is always DWORD-aligned, but
> the DMA address of DATA0 (urb->dma + qtd->isoc_split_offset) may
> not be DWORD-aligned, it depends on the qtd->isoc_split_offset (the
> length of MDATA). In my test case, the length of MDATA is usually
> unaligned, this cause DATA0 packet transmission error.
>
> This patch use kmem_cache to allocate aligned DMA buf for isoc
> split in transaction. Note that according to usb 2.0 spec, the
> maximum data payload size is 1023 bytes for each fs isoc ep,
> and the maximum allowable interrupt data payload size is 64 bytes
> or less for fs interrupt ep. So we set the size of object to be
> 1024 bytes in the kmem cache.
>
> Signed-off-by: William Wu 
> ---
> Changes in v4:
> - get rid of "qh->dw_align_buf_size"
> - allocate unaligned_cache for Address DMA mode and Desc DMA mode
> - freeing order opposite of allocation

You only did half of this.  You changed the order under "error4" but
forgot to make dwc2_hcd_remove() match.


> - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE
>
> Changes in v3:
> - Modify the commit message
> - Use Kmem_cache to allocate aligned DMA buf
> - Fix coding style
>
> Changes in v2:
> - None
>
>  drivers/usb/dwc2/core.h  |  3 ++
>  drivers/usb/dwc2/hcd.c   | 87 
> ++--
>  drivers/usb/dwc2/hcd.h   |  8 
>  drivers/usb/dwc2/hcd_intr.c  |  8 
>  drivers/usb/dwc2/hcd_queue.c |  3 ++
>  5 files changed, 105 insertions(+), 4 deletions(-)

My only remaining comment is a really nitty detail.  Unless someone
else feels strongly about it, I'm not sure it's worth spinning the
patch just for that nit unless someone else has review comments.

I believe I've looked at this enough to say:

Reviewed-by: Douglas Anderson 
--
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

Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Doug Anderson]

Hi,

On Wed, May 9, 2018 at 1:55 AM, wlf  wrote:
> +   } else if (hsotg->params.host_dma) {

 Are you sure this is "else if"?  Can't you have descriptor DMA enabled
 in the controller and still need to do a normal DMA transfer if you
 plug in a hub?  Seems like this should be just "if".
>>>
>>> Sorry, I don't understand the case "have descriptor DMA enabled in the
>>> controller and still need to do a normal DMA transfer". But maybe it
>>> still
>>> has another problem if just use "if" here, because it will create kmem
>>> caches for Slave mode which actually doesn't need aligned DMA buf.
>>
>> So right now your code looks like:
>>
>> if (hsotg->params.dma_desc_enable ||
>>  hsotg->params.dma_desc_fs_enable) {
>>  ...
>>  ...
>> } else if (hsotg->params.host_dma) {
>> ...
>> }
>>
>>
>> I've never played much with "descriptor DMA" on dwc2 because every
>> time I enabled it (last I tried was years ago) a whole bunch of
>> peripherals stopped working and I didn't dig (I just left it off).
>> Thus, I'm no expert on descriptor DMA.  ...that being said, my
>> understanding is that if you enable descriptor DMA in the controller
>> then it will enable a smarter DMA mode for some of the transfers.
>> IIRC Descriptor DMA mode specifically can't handle splits.  Is my
>> memory correct there?  Presumably that means that when you enable
>> descriptor DMA in the controller the driver will automatically fall
>> back to normal Address DMA mode if things get too complicated.  When
>> it falls back to Address DMA mode, presumably dwc2_hcd_init() won't
>> get called again.  That means that any data structures that are needed
>> for Address DMA need to be allocated in dwc2_hcd_init() even if
>> descriptor DMA is enabled because we might need to fall back to
>> Address DMA.
>>
>> Thus, I'm suggesting changing the code to:
>>
>> if (hsotg->params.dma_desc_enable ||
>>  hsotg->params.dma_desc_fs_enable) {
>>  ...
>>  ...
>> }
>>
>> if (hsotg->params.host_dma) {
>> ...
>> }
>>
>>
>> ...as I said, I'm no expert and I could just be confused.  If
>> something I say seems wrong please correct me.
>
> Ah, I got it. Thanks for your detailed explanation. Although I don't know if
> it's possible that dwc2 driver automatically fall back to normal Address DMA
> mode when desc DMA work abnormally with split, I agree with your suggestion.
> I'll fix it in V4 patch.

Hmm, I guess you're right.  I did a quick search and I can't find any
evidence of a fallback like this.  Maybe I dreamed that.  I found some
old comment in the commit history that said:

/*
 * Disable descriptor dma mode by default as the HW can support
 * it, but does not support it for SPLIT transactions.
 * Disable it for FS devices as well.
 */

...and it looks there's currently nobody using descriptor DMA anyway
(assuming I read the code correctly).  It does seem like people were
ever going to turn it on then it would have to be dynamic as I was
dreaming it was...

-Doug
--
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

Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Doug Anderson]

Hi,

On Tue, May 8, 2018 at 12:43 AM, wlf  wrote:
> Dear Doug,
>
> 在 2018年05月08日 13:11, Doug Anderson 写道:
>>
>> Hi,
>>
>> On Mon, May 7, 2018 at 8:07 PM, William Wu 
>> wrote:
>>>
>>> +static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
>>> +   struct dwc2_qh *qh,
>>> +   struct dwc2_host_chan *chan)
>>> +{
>>> +   if (!hsotg->unaligned_cache)
>>> +   return -ENOMEM;
>>> +
>>> +   if (!qh->dw_align_buf) {
>>> +   qh->dw_align_buf =
>>> kmem_cache_alloc(hsotg->unaligned_cache,
>>> +   GFP_ATOMIC |
>>> GFP_DMA);
>>> +   if (!qh->dw_align_buf)
>>> +   return -ENOMEM;
>>> +
>>> +   qh->dw_align_buf_size = min_t(u32, chan->max_packet,
>>> +
>>> DWC2_KMEM_UNALIGNED_BUF_SIZE);
>>
>> Rather than using min_t, wouldn't it be better to return -ENOMEM if
>> "max_packet" > DWC2_KMEM_UNALIGNED_BUF_SIZE?  As it is, you might
>> allocate less space than you need, right?  That seems like it would be
>> bad (even though this is probably impossible).
>
> Yes, good idea! So is it good to fix it like this?
>
> if (!qh->dw_align_buf || chan->max_packet >
> DWC2_KMEM_UNALIGNED_BUF_SIZE)
> return -ENOMEM;
>
> qh->dw_align_buf_size = chan->max_packet;

Won't that orphan memory in the case that the allocation is OK but the
size is wrong?

I would have put it all the way at the top:

if (!hsotg->unaligned_cache ||
chan->max_packet > DWC2_KMEM_UNALIGNED_BUF_SIZE)
  return -ENOMEM;


It also feels like you should get rid of "qh->dw_align_buf_size".  The
buffer is always DWC2_KMEM_UNALIGNED_BUF_SIZE.

...if you need to keep track of how many bytes you mapped with
dma_map_single() and somehow can't get back to "chan", rename this to
qh->dw_align_buf_mapped_bytes or something.  I haven't followed
through all the code, but I _think_ you'd want to make sure to set
qh->dw_align_buf_mapped_bytes every time through
dwc2_alloc_split_dma_aligned_buf(), even if dw_align_buf was already
allocated.  Specifically, my worry is in the case where you enter
dwc2_alloc_split_dma_aligned_buf() and qh->dw_align_buf is non-NULL.
Could "max_packet" be different in this case compared to what it was
when dw_align_buf was first allocated?



>>> @@ -2797,6 +2837,32 @@ static int dwc2_assign_and_init_hc(struct
>>> dwc2_hsotg *hsotg, struct dwc2_qh *qh)
>>>  /* Set the transfer attributes */
>>>  dwc2_hc_init_xfer(hsotg, chan, qtd);
>>>
>>> +   /* For non-dword aligned buffers */
>>> +   if (hsotg->params.host_dma && qh->do_split &&
>>> +   chan->ep_is_in && (chan->xfer_dma & 0x3)) {
>>> +   dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
>>> +   if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
>>> +   dev_err(hsotg->dev,
>>> +   "Failed to allocate memory to handle
>>> non-aligned buffer\n");
>>> +   /* Add channel back to free list */
>>> +   chan->align_buf = 0;
>>> +   chan->multi_count = 0;
>>> +   list_add_tail(&chan->hc_list_entry,
>>> + &hsotg->free_hc_list);
>>> +   qtd->in_process = 0;
>>> +   qh->channel = NULL;
>>> +   return -ENOMEM;
>>> +   }
>>> +   } else {
>>> +   /*
>>> +* We assume that DMA is always aligned in non-split
>>> +* case or split out case. Warn if not.
>>> +*/
>>> +   WARN_ON_ONCE(hsotg->params.host_dma &&
>>> +(chan->xfer_dma & 0x3));
>>> +   chan->align_buf = 0;
>>> +   }
>>> +
>>>  if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
>>>  chan->ep_type == USB_ENDPOINT_XFER_ISOC)
>>>  /*
>>> @@ -5241,6 +5307,17 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>>>  hsotg->params.dma_desc_enable = false;
&g

Re: [PATCH v3 2/2] usb: dwc2: fix isoc split in transfer with no data

[Doug Anderson]

Hi,

On Mon, May 7, 2018 at 8:07 PM, William Wu  wrote:
> If isoc split in transfer with no data (the length of DATA0
> packet is zero), we can't simply return immediately. Because
> the DATA0 can be the first transaction or the second transaction
> for the isoc split in transaction. If the DATA0 packet with no
> data is in the first transaction, we can return immediately.
> But if the DATA0 packet with no data is in the second transaction
> of isoc split in transaction sequence, we need to increase the
> qtd->isoc_frame_index and giveback urb to device driver if needed,
> otherwise, the MDATA packet will be lost.
>
> A typical test case is that connect the dwc2 controller with an
> usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics
> headset) into the downstream port of Hub. Then use the usb mic
> to record, we can find noise when playback.
>
> In the case, the isoc split in transaction sequence like this:
>
> - SSPLIT IN transaction
> - CSPLIT IN transaction
>   - MDATA packet (176 bytes)
> - CSPLIT IN transaction
>   - DATA0 packet (0 byte)
>
> This patch use both the length of DATA0 and qtd->isoc_split_offset
> to check if the DATA0 is in the second transaction.
>
> Signed-off-by: William Wu 
> ---
> Changes in v3:
> - Remove "qtd->isoc_split_offset = 0" in the if test
>
> Changes in v2:
> - Modify the commit message
>
>  drivers/usb/dwc2/hcd_intr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index ba6fd852..3003594 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -930,9 +930,8 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg 
> *hsotg,
> frame_desc = &qtd->urb->iso_descs[qtd->isoc_frame_index];
> len = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd,
>   DWC2_HC_XFER_COMPLETE, NULL);
> -   if (!len) {
> +   if (!len && !qtd->isoc_split_offset) {
> qtd->complete_split = 0;
> -   qtd->isoc_split_offset = 0;
> return 0;
> }

This looks fine to me now, but as per my comments on the previous
version I don't think I've dug through this problem enough to add my
Reviewed-by tag.  I'll assume that John or someone with more knowledge
of the USB protocol than I have will Review / Ack.


-Doug
--
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

Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Doug Anderson]

Hi,

On Mon, May 7, 2018 at 8:07 PM, William Wu  wrote:
> +static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
> +   struct dwc2_qh *qh,
> +   struct dwc2_host_chan *chan)
> +{
> +   if (!hsotg->unaligned_cache)
> +   return -ENOMEM;
> +
> +   if (!qh->dw_align_buf) {
> +   qh->dw_align_buf = kmem_cache_alloc(hsotg->unaligned_cache,
> +   GFP_ATOMIC | GFP_DMA);
> +   if (!qh->dw_align_buf)
> +   return -ENOMEM;
> +
> +   qh->dw_align_buf_size = min_t(u32, chan->max_packet,
> + DWC2_KMEM_UNALIGNED_BUF_SIZE);

Rather than using min_t, wouldn't it be better to return -ENOMEM if
"max_packet" > DWC2_KMEM_UNALIGNED_BUF_SIZE?  As it is, you might
allocate less space than you need, right?  That seems like it would be
bad (even though this is probably impossible).


> @@ -2797,6 +2837,32 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg 
> *hsotg, struct dwc2_qh *qh)
> /* Set the transfer attributes */
> dwc2_hc_init_xfer(hsotg, chan, qtd);
>
> +   /* For non-dword aligned buffers */
> +   if (hsotg->params.host_dma && qh->do_split &&
> +   chan->ep_is_in && (chan->xfer_dma & 0x3)) {
> +   dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
> +   if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
> +   dev_err(hsotg->dev,
> +   "Failed to allocate memory to handle 
> non-aligned buffer\n");
> +   /* Add channel back to free list */
> +   chan->align_buf = 0;
> +   chan->multi_count = 0;
> +   list_add_tail(&chan->hc_list_entry,
> + &hsotg->free_hc_list);
> +   qtd->in_process = 0;
> +   qh->channel = NULL;
> +   return -ENOMEM;
> +   }
> +   } else {
> +   /*
> +* We assume that DMA is always aligned in non-split
> +* case or split out case. Warn if not.
> +*/
> +   WARN_ON_ONCE(hsotg->params.host_dma &&
> +(chan->xfer_dma & 0x3));
> +   chan->align_buf = 0;
> +   }
> +
> if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
> chan->ep_type == USB_ENDPOINT_XFER_ISOC)
> /*
> @@ -5241,6 +5307,17 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
> hsotg->params.dma_desc_enable = false;
> hsotg->params.dma_desc_fs_enable = false;
> }
> +   } else if (hsotg->params.host_dma) {

Are you sure this is "else if"?  Can't you have descriptor DMA enabled
in the controller and still need to do a normal DMA transfer if you
plug in a hub?  Seems like this should be just "if".


> +   /*
> +* Create kmem caches to handle non-aligned buffer
> +* in Buffer DMA mode.
> +*/
> +   hsotg->unaligned_cache = 
> kmem_cache_create("dwc2-unaligned-dma",
> +   DWC2_KMEM_UNALIGNED_BUF_SIZE, 
> 4,

Worth using "DWC2_USB_DMA_ALIGN" rather than 4?


> +   SLAB_CACHE_DMA, NULL);
> +   if (!hsotg->unaligned_cache)
> +   dev_err(hsotg->dev,
> +   "unable to create dwc2 unaligned cache\n");
> }
>
> hsotg->otg_port = 1;
> @@ -5279,6 +5356,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>  error4:
> kmem_cache_destroy(hsotg->desc_gen_cache);
> kmem_cache_destroy(hsotg->desc_hsisoc_cache);
> +   kmem_cache_destroy(hsotg->unaligned_cache);

nitty nit: freeing order should be opposite of allocation, so the new
line should be above the other two.
--
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

Re: [PATCH v5 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values

[Doug Anderson]

Rob,

On Mon, May 7, 2018 at 8:53 AM, Rob Herring  wrote:
> On Thu, May 03, 2018 at 02:36:13AM +0530, Manu Gautam wrote:
>> To improve eye diagram for PHYs on different boards of same SOC,
>> some parameters may need to be changed. Provide device tree
>> properties to override these from board specific device tree
>> files. While at it, replace "qcom,qusb2-v2-phy" with compatible
>> string for USB2 PHY on sdm845 which was earlier added for
>> sdm845 only.
>>
>> Signed-off-by: Manu Gautam 
>> ---
>>  .../devicetree/bindings/phy/qcom-qusb2-phy.txt | 23 +-
>>  include/dt-bindings/phy/phy-qcom-qusb2.h   | 37 
>> ++
>>  2 files changed, 59 insertions(+), 1 deletion(-)
>>  create mode 100644 include/dt-bindings/phy/phy-qcom-qusb2.h
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt 
>> b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> index 42c9742..03025d9 100644
>> --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> @@ -6,7 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on 
>> Qualcomm chipsets.
>>  Required properties:
>>   - compatible: compatible list, contains
>>  "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
>> -"qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
>> +"qcom,sdm845-qusb2-phy" for 10nm PHY on sdm845.
>>
>>   - reg: offset and length of the PHY register set.
>>   - #phy-cells: must be 0.
>> @@ -27,6 +27,27 @@ Optional properties:
>>   tuning parameter value for qusb2 phy.
>>
>>   - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
>> + - qcom,imp-res-offset-value: It is a 6 bit value that specifies offset to 
>> be
>> + added to PHY refgen RESCODE via IMP_CTRL1 register. It is a PHY
>> + tuning parameter that may vary for different boards of same 
>> SOC.
>> + This property is applicable to only QUSB2 v2 PHY (sdm845).
>> + - qcom,hstx-trim-value: It is a 4 bit value that specifies tuning for HSTX
>> + output current.
>> + Possible range is - 15mA to 24mA (stepsize of 600 uA).
>> + See dt-bindings/phy/phy-qcom-qusb2.h for applicable values.
>> + This property is applicable to only QUSB2 v2 PHY (sdm845).
>> + Default value is 22.2mA for sdm845.
>> + - qcom,preemphasis-level: It is a 2 bit value that specifies pre-emphasis 
>> level.
>> + Possible range is 0 to 15% (stepsize of 5%).
>> + See dt-bindings/phy/phy-qcom-qusb2.h for applicable values.
>> + This property is applicable to only QUSB2 v2 PHY (sdm845).
>> + Default value is 10% for sdm845.
>> +- qcom,preemphasis-width: It is a 1 bit value that specifies how long the 
>> HSTX
>> + pre-emphasis (specified using qcom,preemphasis-level) must be 
>> in
>> + effect. Duration could be half-bit of full-bit.
>
> s/of/or/
>
> But I'd just make this a boolean instead: qcom,preemphasis-half-bit

I had this same comment in the post of v4.  See
.  Specifically, I said:

> Perhaps just make this a boolean property.  If it exists then you get
> the non-default case.  AKA: if the default is full bit width, then
> you'd allow a boolean property "qcom,preemphasis-half-width" to
> override.  If the default is half bit width then you'd allow
> "qcom,preemphasis-full-width" to override.

Manu replied:

> Default property value for an SOC is specified in driver and could vary from
> soc to soc. Hence, from board devicetree for different SOCs we might need
> to select separate widths overriding default driver values.
> Alternative is to have two bool properties each for half and full-width. Did
> you actually mean that?


IMHO given Manu's argument it seems fine to specify it the way he did.
Please advise if you agree or disagree.

-Doug
--
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

Re: [PATCH v5 1/7] clk: msm8996-gcc: Mark halt check as no-op for USB/PCIE pipe_clk

[Doug Anderson]

Hi,

On Wed, May 2, 2018 at 2:06 PM, Manu Gautam  wrote:
> The USB and PCIE pipe clocks are sourced from external clocks
> inside the QMP USB/PCIE PHYs. Enabling or disabling of PIPE RCG
> clocks is dependent on PHY initialization sequence hence
> update halt_check to BRANCH_HALT_SKIP for these clocks so
> that clock status bit is not polled when enabling or disabling
> the clocks. It allows to simplify PHY client driver code which
> is both user and source of the pipe_clk and avoid error logging
> related status check on clk_disable/enable.
>
> Signed-off-by: Manu Gautam 
> ---
>  drivers/clk/qcom/gcc-msm8996.c | 4 
>  1 file changed, 4 insertions(+)

FWIW this matches my understanding of what Stephen and you agreed upon.  Thus:

Reviewed-by: Douglas Anderson 
--
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

Re: [PATCH v5 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values

[Doug Anderson]

Hi,

On Wed, May 2, 2018 at 2:06 PM, Manu Gautam  wrote:
> To improve eye diagram for PHYs on different boards of same SOC,
> some parameters may need to be changed. Provide device tree
> properties to override these from board specific device tree
> files. While at it, replace "qcom,qusb2-v2-phy" with compatible
> string for USB2 PHY on sdm845 which was earlier added for
> sdm845 only.
>
> Signed-off-by: Manu Gautam 
> ---
>  .../devicetree/bindings/phy/qcom-qusb2-phy.txt | 23 +-
>  include/dt-bindings/phy/phy-qcom-qusb2.h   | 37 
> ++
>  2 files changed, 59 insertions(+), 1 deletion(-)
>  create mode 100644 include/dt-bindings/phy/phy-qcom-qusb2.h

Thanks for adding the #defines, describing the defaults, and including
which SoCs the new properties work on.  This looks great to me now,
thanks!

Reviewed-by: Douglas Anderson 
--
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

Re: [PATCH v5 7/7] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Doug Anderson]

Hi,

On Wed, May 2, 2018 at 2:06 PM, Manu Gautam  wrote:
> There are two QUSB2 PHYs present on sdm845. In order
> to improve eye diagram for both the PHYs some parameters
> need to be changed. Provide device tree properties to
> override these from board specific device tree files.
>
> Signed-off-by: Manu Gautam 
> ---
>  drivers/phy/qualcomm/phy-qcom-qusb2.c | 126 
> +++---
>  1 file changed, 118 insertions(+), 8 deletions(-)

I confirmed that all my previous comments were fixed and this looks
nice to me now.

Reviewed-by: Douglas Anderson 
--
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

Re: [PATCH v5 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Doug Anderson]

Hi,

On Wed, May 2, 2018 at 2:06 PM, Manu Gautam  wrote:
> QMP PHY for USB/PCIE requires pipe_clk for locking of
> retime buffers at the pipe interface. Driver checks for
> PHY_STATUS without enabling pipe_clk due to which
> phy_init() fails with initialization timeout.
> Though pipe_clk is output from PHY (after PLL is programmed
> during initialization sequence) to GCC clock_ctl and then fed
> back to PHY but for PHY_STATUS register to reflect successful
> initialization pipe_clk from GCC must be present.
> Since, clock driver now ignores status_check for pipe_clk on
> clk_enable/disable, driver can safely enable/disable pipe_clk
> from phy_init/exit.
>
> Signed-off-by: Manu Gautam 
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 22 --
>  1 file changed, 8 insertions(+), 14 deletions(-)

Given the agreement with Stephen Boyd that we'll just BRANCH_HALT_SKIP
for now, this all looks good to me.

Reviewed-by: Douglas Anderson 
--
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

Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Doug Anderson]

Hi,

On Wed, May 2, 2018 at 10:14 AM, wlf  wrote:
> It's a good way to allocate an extra 3 bytes in the original bounce buffer
> for this unaligned
> issue， it's similar to the tailroom of sk_buff. However, just as you said,
> we'd better find
> the special cases where we need an oversized bounce buffer, otherwise，we
> need to allocate
> a bounce buffer for all of urbs.
>
> It's hard for me to know the special cases in the
> dwc2_alloc_dma_aligned_buffer(), because
> it's called from usb_submit_urb() in the device class driver, and I hardly
> know the split state
> in this process, much less if the split transaction need aligned buffer. Do
> you have any idea?
>
> I suppose that we can't find the special cases where we need an oversized
> bounce buffer
> in the dwc2_alloc_dma_aligned_buffer(), and if we still want to re-use the
> original bounce
> buffer with extra 3 bytes, then we need to allocate a  bounce buffer for all
> of urbs, and do
> unnecessary  data copy for these urbs  whose transfer_buffer were already
> aligned.  This
> may reduce the transmission rate of USB.
>
> Can we just pre-allocate an additional aligned buffer (the size is 200
> bytes) for split transaction
> in dwc2_map_urb_for_dma for all of urbs. And if we find the split
> transaction is unaligned，
> we can easily use the pre-allocated aligned buffer.

OK, so thinking about this more...

Previously things got really slow at interrupt time because we were
trying to allocate as much as 64K at interrupt time.  That wasn't so
great.  In your case, you're only allocating 200 bytes.  As I
understand things, allocating 200 bytes at interrupt time is probably
not a huge deal.

...so I guess it come down to a tradeoff here: is it worth eating 200
bytes for each URB to save an 200 byte allocation at interrupt time in
this one rare case.

I'd certainly welcome anyone's opinion here, but I'm going to go with
saying it's fine to allocate the 200 bytes at interrupt time (like
your patch does).  ...but, I _think_ you want to use
kmem_cache_create() to create a cache and then kmem_cache_zalloc().
Since all allocations are the same size and you want this to be fast,
I think using kmem_cache is good.



>>> +   /* For non-dword aligned buffers */
>>> +   if (hsotg->params.host_dma > 0 && qh->do_split &&
>>> +   chan->ep_is_in && (chan->xfer_dma & 0x3)) {
>>
>> So what happens if were unaligned (AKA (chan->xfer_dma & 0x3)) but
>> we're not doing split or it's not an IN EP?  Do we just fail then?
>>
>> I guess the rest of this patch only handles the "in" case and maybe
>> you expect that the problems will only come about for do_split, but it
>> still might be wise to at least print a warning in the other cases?
>> >From reading dwc2_hc_init_xfer() it seems like you could run into this
>> same problem in the "out" case?
>
> Actually, I only find non-dword aligned issue in the case of split in
> transaction.
> And I think that if we're not doing split or it's an OUT EP, we can always
> get aligned buffer
> in the current code. For non-split case, the dwc2_alloc_dma_aligned_buffer()
> is enough. And for split out case, if the transaction is subdivided into
> multiple start-splits,
> each with a data payload of 188 bytes or less, so the DMA address is always
> aligned.

Can you at least print an error message if you end up with non-aligned
DMA in one of the other cases?


>>> DMA_FROM_DEVICE);
>>> +   memcpy(qtd->urb->buf + frame_desc->offset +
>>> +  qtd->isoc_split_offset, chan->qh->dw_align_buf,
>>> len);
>>
>> Assuming I'm understanding this patch correctly, I think it would be
>> better to write:
>>
>>memcpy(qtd->xfer_dma, chan->qh->dw_align_buf, len);
>
> Sorry, there's no "xfer_buf" in qtd, do you means the "chan->xfer_dma"? If
> it's, I think we can't
> do memcpy from a transfer buffer to a DMA address. Maybe chan->xfer_buf is
> more suitable,
> but it seems that the dwc2 driver doesn't update the chan->xfer_buf for isoc
> transfer with dma
> enabled in dwc2_hc_init_xfer().

Yes, I meant chan->xfer_dma.  Ah, right.  xfer_dma is a DMA address.

I guess you could in theory you could do:

memcpy(qtd->urb->buf + (chan->xfer_dma - urb->dma),
chan->qh->dw_align_buf);

That at least avoids duplicating the math.  Maybe either do that, or
if you don't like it at least add a comment saying that the math needs
to match the math in dwc2_hc_init_xfer().


>> Then if you ever end up having to align a transfer other than a split
>> you won't be doing the wrong math.  As it is it's very non-obvious
>> that you're hardcoding the same formula that's in dwc2_hc_init_xfer()
>
> Actually, I'm hardcoding the same formula from the old code which has been
> ripped out
> in the commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more
> supported way").

Ah, got it.  Well, I think the old code was just hardcoding the same
formula in two places then.  ;-)


>>> -   if (qh->desc_list)
>>> +   if (qh->desc

Re: [PATCH v2 2/2] usb: dwc2: fix isoc split in transfer with no data

[Doug Anderson]

Hi,

On Tue, May 1, 2018 at 8:04 PM, William Wu  wrote:
> If isoc split in transfer with no data (the length of DATA0
> packet is zero), we can't simply return immediately. Because
> the DATA0 can be the first transaction or the second transaction
> for the isoc split in transaction. If the DATA0 packet with no
> data is in the first transaction, we can return immediately.
> But if the DATA0 packet with no data is in the second transaction
> of isoc split in transaction sequence, we need to increase the
> qtd->isoc_frame_index and giveback urb to device driver if needed,
> otherwise, the MDATA packet will be lost.
>
> A typical test case is that connect the dwc2 controller with an
> usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics
> headset) into the downstream port of Hub. Then use the usb mic
> to record, we can find noise when playback.
>
> In the case, the isoc split in transaction sequence like this:
>
> - SSPLIT IN transaction
> - CSPLIT IN transaction
>   - MDATA packet (176 bytes)
> - CSPLIT IN transaction
>   - DATA0 packet (0 byte)
>
> This patch use both the length of DATA0 and qtd->isoc_split_offset
> to check if the DATA0 is in the second transaction.
>
> Signed-off-by: William Wu 
> ---
> Changes in v2:
> - Modify the commit message
>
>  drivers/usb/dwc2/hcd_intr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index 5e2378f..479f628 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -930,7 +930,7 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg 
> *hsotg,
> frame_desc = &qtd->urb->iso_descs[qtd->isoc_frame_index];
> len = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd,
>   DWC2_HC_XFER_COMPLETE, NULL);
> -   if (!len) {
> +   if (!len && !qtd->isoc_split_offset) {
> qtd->complete_split = 0;
> qtd->isoc_split_offset = 0;
> return 0;

I don't think my USB-fu is strong enough to do a real review of this
patch, but one small nitpick is that you can remove
"qtd->isoc_split_offset = 0" in the if test now.  AKA:

if (!len && !qtd->isoc_split_offset) {
  qtd->complete_split = 0;
  return 0;
}

...since you only enter the "if" test now when isoc_split_offset is
already 0...  Hopefully John Youn will have some time to comment on
this patch with more real USB knowledge...


-Doug
--
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

Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Doug Anderson]

Hi,

On Tue, May 1, 2018 at 8:04 PM, William Wu  wrote:
> The commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in
> a more supported way") rips out a lot of code to simply the
> allocation of aligned DMA. However, it also introduces a new
> issue when use isoc split in transfer.
>
> In my test case, I connect the dwc2 controller with an usb hs
> Hub (GL852G-12), and plug an usb fs audio device (Plantronics
> headset) into the downstream port of Hub. Then use the usb mic
> to record, we can find noise when playback.
>
> It's because that the usb Hub uses an MDATA for the first
> transaction and a DATA0 for the second transaction for the isoc
> split in transaction. An typical isoc split in transaction sequence
> like this:
>
> - SSPLIT IN transaction
> - CSPLIT IN transaction
>   - MDATA packet
> - CSPLIT IN transaction
>   - DATA0 packet
>
> The DMA address of MDATA (urb->dma) is always DWORD-aligned, but
> the DMA address of DATA0 (urb->dma + qtd->isoc_split_offset) may
> not be DWORD-aligned, it depends on the qtd->isoc_split_offset (the
> length of MDATA). In my test case, the length of MDATA is usually
> unaligned, this casue DATA0 packet transmission error.
>
> This patch base on the old way of aligned DMA allocation in the
> dwc2 driver to get aligned DMA for isoc split in.
>
> Signed-off-by: William Wu 
> ---
> Changes in v2:
> - None
>
>  drivers/usb/dwc2/hcd.c   | 63 
> +---
>  drivers/usb/dwc2/hcd.h   | 10 +++
>  drivers/usb/dwc2/hcd_intr.c  |  8 ++
>  drivers/usb/dwc2/hcd_queue.c |  8 +-
>  4 files changed, 85 insertions(+), 4 deletions(-)

A word of warning that I'm pretty rusty on dwc2 and even when I was
making lots of patches I still considered myself a bit clueless.
...so if something seems wrong, please call me on it...


> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 190f959..8c2b35f 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -1562,11 +1562,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg 
> *hsotg,
> }
>
> if (hsotg->params.host_dma) {
> -   dwc2_writel((u32)chan->xfer_dma,
> -   hsotg->regs + HCDMA(chan->hc_num));
> +   dma_addr_t dma_addr;
> +
> +   if (chan->align_buf) {
> +   if (dbg_hc(chan))
> +   dev_vdbg(hsotg->dev, "align_buf\n");
> +   dma_addr = chan->align_buf;
> +   } else {
> +   dma_addr = chan->xfer_dma;
> +   }
> +   dwc2_writel((u32)dma_addr, hsotg->regs + HCDMA(chan->hc_num));
> +
> if (dbg_hc(chan))
> dev_vdbg(hsotg->dev, "Wrote %08lx to HCDMA(%d)\n",
> -(unsigned long)chan->xfer_dma, chan->hc_num);
> +(unsigned long)dma_addr, chan->hc_num);
> }
>
> /* Start the split */
> @@ -2620,6 +2629,33 @@ static void dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
> }
>  }
>
> +static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
> +   struct dwc2_qh *qh,
> +   struct dwc2_host_chan *chan)
> +{
> +   if (!qh->dw_align_buf) {
> +   qh->dw_align_buf = kmalloc(chan->max_packet,

So you're potentially doing a bounce buffer atop a bounce buffer now,
right?  That seems pretty non-optimal.  You're also back to doing a
kmalloc at interrupt time which I found was pretty bad for
performance.

Is there really no way you can do your memory allocation in
dwc2_alloc_dma_aligned_buffer() instead of here?  For input packets,
if you could just allocate an extra 3 bytes in the original bounce
buffer you could just re-use the original bounce buffer together with
a memmove?  AKA:

transfersize = 13 + 64;
buf = alloc(16 + 64);

// Do the first transfer, no problems.
dma_into(buf, 13);

// 2nd transfer isn't aligned, so align.
// we allocated a little extra to account for this
dma_into(buf + 16, 64);

// move back where it belongs.
memmove(buf + 13, buf + 16, 64);


To make the above work you'd need to still allocate a bounce buffer
even if the original "urb->transfer_buffer" was already aligned.
Ideally you'd be able to know when dwc2_alloc_dma_aligned_buffer()
that this is one of the special cases where you need a slightly
oversized bounce buffer.

---

If you somehow need to do something for output, you'd do the opposite.
You'd copy backwards top already transferred data to align.


> +  GFP_ATOMIC | GFP_DMA);
> +   if (!qh->dw_align_buf)
> +   return -ENOMEM;
> +
> +   qh->dw_align_buf_size = chan->max_packet;
> +   }
> +
> +   qh->dw_align_buf_dma = dma_map_single(hsotg->dev, qh->dw_align_buf,
> + qh->dw

Re: [PATCH v5 00/17] Support for Qualcomm QUSBv2 and QMPv3 USB PHYs

[Doug Anderson]

Manu,

On Mon, Feb 19, 2018 at 9:21 PM, Manu Gautam  wrote:
> Hi Kishon,
>
>
> On 2/16/2018 5:19 PM, Kishon Vijay Abraham I wrote:
>>
>> On Thursday 01 February 2018 06:08 PM, Kishon Vijay Abraham I wrote:
>>>
>>> The series looks good. I'll start merging once -rc1 is tagged.
>> merged, thanks!
>>
>> -Kishon
>
> Following git isn't updated with these:
> git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git
>
> Should I be looking at somewhere else?
> I have some follow-up PHY patches to send, hence wanted to make sure they
> apply cleanly in your tree.

Sometimes you've got to give maintainers a day or three before patches
actually show up on git.kernel.org.  ...but I happened to notice that
they all appear to be there now so you should be all set to post your
followup patches.  :)

-Doug
--
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

Re: [PATCH v4] usb: dwc2: host: Don't retry NAKed transactions right away

[Doug Anderson]

Hi,

On Tue, Dec 19, 2017 at 8:56 AM, Stefan Wahren  wrote:
> Hi Doug,
>
>> Doug Anderson  hat am 19. Dezember 2017 um 16:57 
>> geschrieben:
>>
>>
>> Felipe,
>>
>> On Tue, Dec 12, 2017 at 10:30 AM, Douglas Anderson
>>  wrote:
>> ...
>>
>> I don't mean to be a pest, but I'm hoping that we can land this
>> somewhere (if nothing else in your /next tree) just so it doesn't miss
>> another release cycle.  If you're not so keen on collecting dwc2 host
>> patches these days, I can also see if Greg KH is willing to take this
>> directly into his tree.  Please let me know.  Thanks for your time!
>> :)
>>
>> -Doug
>
> it's applied to next:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h=next&id=38d2b5fb75c15923fb89c32134516a623515bce4

Awww, crud.  OK, then _really_ I'm just being a pest.  :(  Thanks for
pointing it out Stefan.  I'm so used to all the emails that fly by
from folks when things land that I didn't think to check directly.
Sorry for the noise Felipe and thanks to both of you guys your all
your help.

-Doug
--
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

Re: [PATCH v4] usb: dwc2: host: Don't retry NAKed transactions right away

[Doug Anderson]

Felipe,

On Tue, Dec 12, 2017 at 10:30 AM, Douglas Anderson
 wrote:
> On rk3288-veyron devices on Chrome OS it was found that plugging in an
> Arduino-based USB device could cause the system to lockup, especially
> if the CPU Frequency was at one of the slower operating points (like
> 100 MHz / 200 MHz).
>
> Upon tracing, I found that the following was happening:
> * The USB device (full speed) was connected to a high speed hub and
>   then to the rk3288.  Thus, we were dealing with split transactions,
>   which is all handled in software on dwc2.
> * Userspace was initiating a BULK IN transfer
> * When we sent the SSPLIT (to start the split transaction), we got an
>   ACK.  Good.  Then we issued the CSPLIT.
> * When we sent the CSPLIT, we got back a NAK.  We immediately (from
>   the interrupt handler) started to retry and sent another SSPLIT.
> * The device kept NAKing our CSPLIT, so we kept ping-ponging between
>   sending a SSPLIT and a CSPLIT, each time sending from the interrupt
>   handler.
> * The handling of the interrupts was (because of the low CPU speed and
>   the inefficiency of the dwc2 interrupt handler) was actually taking
>   _longer_ than it took the other side to send the ACK/NAK.  Thus we
>   were _always_ in the USB interrupt routine.
> * The fact that USB interrupts were always going off was preventing
>   other things from happening in the system.  This included preventing
>   the system from being able to transition to a higher CPU frequency.
>
> As I understand it, there is no requirement to retry super quickly
> after a NAK, we just have to retry sometime in the future.  Thus one
> solution to the above is to just add a delay between getting a NAK and
> retrying the transmission.  If this delay is sufficiently long to get
> out of the interrupt routine then the rest of the system will be able
> to make forward progress.  Even a 25 us delay would probably be
> enough, but we'll be extra conservative and try to delay 1 ms (the
> exact amount depends on HZ and the accuracy of the jiffy and how close
> the current jiffy is to ticking, but could be as much as 20 ms or as
> little as 1 ms).
>
> Presumably adding a delay like this could impact the USB throughput,
> so we only add the delay with repeated NAKs.
>
> NOTE: Upon further testing of a pl2303 serial adapter, I found that
> this fix may help with problems there.  Specifically I found that the
> pl2303 serial adapters tend to respond with a NAK when they have
> nothing to say and thus we end with this same sequence.
>
> Signed-off-by: Douglas Anderson 
> Reviewed-by: Julius Werner 
> Tested-by: Stefan Wahren 
> Acked-by: John Youn 
> ---
>
> Changes in v4:
> - Removed Cc for stable as per Felipe's request in v3
> - Rebased and squashed the two patches since Kees' timer stuff landed
> - Add John Youn's Ack.
>
> Changes in v3:
> - Add tested-by for Stefan Wahren
> - Sent to Felipe Balbi as candiate to land this.
> - Add Cc for stable (it's always been broken so go as far is as easy)
>
> Changes in v2:
> - Address http://crosreview.com/737520 feedback
>
>  drivers/usb/dwc2/core.h  |  1 +
>  drivers/usb/dwc2/hcd.c   |  7 
>  drivers/usb/dwc2/hcd.h   |  9 +
>  drivers/usb/dwc2/hcd_intr.c  | 20 +++
>  drivers/usb/dwc2/hcd_queue.c | 81 
> +---
>  5 files changed, 114 insertions(+), 4 deletions(-)

I don't mean to be a pest, but I'm hoping that we can land this
somewhere (if nothing else in your /next tree) just so it doesn't miss
another release cycle.  If you're not so keen on collecting dwc2 host
patches these days, I can also see if Greg KH is willing to take this
directly into his tree.  Please let me know.  Thanks for your time!
:)

-Doug
--
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

Re: [PATCH] phy: rockchip-typec: Try to turn the PHY on several times

[Doug Anderson]

Hi,

On Wed, Dec 13, 2017 at 4:41 AM, Enric Balletbo Serra
 wrote:
> Hi Doug,
>
> 2017-12-11 22:45 GMT+01:00 Douglas Anderson :
>> Bind / unbind stress testing of the USB controller on rk3399 found
>> that we'd often end up with lots of failures that looked like this:
>>
>>   phy phy-ff80.phy.9: phy poweron failed --> -110
>>   dwc3 fe90.dwc3: failed to initialize core
>>   dwc3: probe of fe90.dwc3 failed with error -110
>>
>> Those errors were sometimes seen at bootup too, in which case USB
>> peripherals wouldn't work until unplugged and re-plugged in.
>>
>> I spent some time trying to figure out why the PHY was failing to
>> power on but I wasn't able to.  Possibly this has to do with the fact
>> that the PHY docs say that the USB controller "needs to be held in
>> reset to hold pipe power state in P2 before initializing the Type C
>> PHY" but that doesn't appear to be easy to do with the dwc3 driver
>> today.  Messing around with the ordering of the reset vs. the PHY
>> initialization in the dwc3 driver didn't seem to fix things.
>>
>> I did, however, find that if I simply retry the power on it seems to
>> have a good chance of working.  So let's add some retries.  I ran a
>> pretty tight bind/unbind loop overnight.  When I did so, I found that
>> I need to retry between 1% and 2% of the time.  Overnight I found only
>> a small handful of times where I needed 2 retries.  I never found a
>> case where I needed 3 retries.
>>
>> I'm completely aware of the fact that this is quite an ugly hack and I
>> wish I didn't have to resort to it, but I have no other real idea how
>> to make this hardware reliable.  If Rockchip in the future can come up
>> with a solution we can always revert this hack.  Until then, let's at
>> least have something that works.
>>
>> This patch is tested atop Enric's latest dwc3 patch series ending at:
>>   https://patchwork.kernel.org/patch/10095527/
>> ...but it could be applied independently of that series without any
>> bad effects.
>>
>> For some more details on this bug, you can refer to:
>>   https://bugs.chromium.org/p/chromium/issues/detail?id=783464
>>
>> Signed-off-by: Douglas Anderson 
>> ---
>>
>>  drivers/phy/rockchip/phy-rockchip-typec.c | 24 ++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-typec.c 
>> b/drivers/phy/rockchip/phy-rockchip-typec.c
>> index ee85fa0ca4b0..5c2157156ce1 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-typec.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-typec.c
>> @@ -349,6 +349,8 @@
>>  #define MODE_DFP_USB   BIT(1)
>>  #define MODE_DFP_DPBIT(2)
>>
>> +#define POWER_ON_TRIES 5
>> +
>
> I did the test of increase the number of tries to 100 because
> unfortunately, even with this patch applied, I can see the problem on
> my kevin with current mainline.
>
> [  244.309094] rockchip-typec-phy ff80.phy: Turn on failed after 100 loops
>
> That's an extra debug print ^
>
> [  244.317019] phy phy-ff80.phy.8: phy poweron failed --> -110
> [  244.323824] dwc3 fe90.dwc3: failed to initialize core
> [  244.330057] dwc3: probe of fe90.dwc3 failed with error -110
>
> So I'm wondering if there is something else that I need to apply to
> really fix this as you didn't reproduce the issue doing lots of tests
> and I can reproduce the issue very easily.

Ah!  I added that message to the top of my upstream series and,
indeed, I sometimes see the PHY fail to turn on.  Doh.

OK, so here's what I've done:

* The place where I ran the overnight loops was actually the Chrome OS
4.4 kernel.  In that kernel I had a message very similar to yours and
I didn't hit it.  I just re-ran this for 20 minutes now and I can
re-confirm.  In the Chrome OS kernel I never see it needing more than
a 1 (or 2) loops and it doesn't ever get into the "totally failed"
case.

* Previously I ran ~10 minutes with the upstream kernel, but at the
time I didn't have your printout.  After 10 minutes I checked my logs
and I definitely saw the "Needed 1 loops to turn on", so I knew my
patch was doing something useful.  It didn't occur to me to re-confirm
that I didn't get the "totally failed" upstream, though now that I say
it out loud it's stupid that I didn't think to do this.

* Previously when playing with patches on the upstream kernel I saw
lots of problems powering on the PHY and I thought my patch was
helping, but that was all very non-scientific.


So to say it shortly:

* For me, my patch makes things a slightly better even on the upstream
kernel (I do sometimes see the "turned on after 1 tries")

* I can confirm that my patch doesn't fix everything upstream, so
there's something different about the Chrome OS tree still.

---

I also picked all the local patches from the Chrome OS kernel to the
PHY driver and now my PHY driver in the upstream and downstream trees
match.  I can still reproduce problems.  So the issue is somewhere 

Re: [PATCH v3 1/2] usb: dwc2: host: Don't retry NAKed transactions right away

[Doug Anderson]

Hi,

On Tue, Dec 12, 2017 at 3:06 AM, Felipe Balbi  wrote:
>
> Hi,
>
> Douglas Anderson  writes:
>> On rk3288-veyron devices on Chrome OS it was found that plugging in an
>> Arduino-based USB device could cause the system to lockup, especially
>> if the CPU Frequency was at one of the slower operating points (like
>> 100 MHz / 200 MHz).
>>
>> Upon tracing, I found that the following was happening:
>> * The USB device (full speed) was connected to a high speed hub and
>>   then to the rk3288.  Thus, we were dealing with split transactions,
>>   which is all handled in software on dwc2.
>> * Userspace was initiating a BULK IN transfer
>> * When we sent the SSPLIT (to start the split transaction), we got an
>>   ACK.  Good.  Then we issued the CSPLIT.
>> * When we sent the CSPLIT, we got back a NAK.  We immediately (from
>>   the interrupt handler) started to retry and sent another SSPLIT.
>> * The device kept NAKing our CSPLIT, so we kept ping-ponging between
>>   sending a SSPLIT and a CSPLIT, each time sending from the interrupt
>>   handler.
>> * The handling of the interrupts was (because of the low CPU speed and
>>   the inefficiency of the dwc2 interrupt handler) was actually taking
>>   _longer_ than it took the other side to send the ACK/NAK.  Thus we
>>   were _always_ in the USB interrupt routine.
>> * The fact that USB interrupts were always going off was preventing
>>   other things from happening in the system.  This included preventing
>>   the system from being able to transition to a higher CPU frequency.
>>
>> As I understand it, there is no requirement to retry super quickly
>> after a NAK, we just have to retry sometime in the future.  Thus one
>> solution to the above is to just add a delay between getting a NAK and
>> retrying the transmission.  If this delay is sufficiently long to get
>> out of the interrupt routine then the rest of the system will be able
>> to make forward progress.  Even a 25 us delay would probably be
>> enough, but we'll be extra conservative and try to delay 1 ms (the
>> exact amount depends on HZ and the accuracy of the jiffy and how close
>> the current jiffy is to ticking, but could be as much as 20 ms or as
>> little as 1 ms).
>>
>> Presumably adding a delay like this could impact the USB throughput,
>> so we only add the delay with repeated NAKs.
>>
>> NOTE: Upon further testing of a pl2303 serial adapter, I found that
>> this fix may help with problems there.  Specifically I found that the
>> pl2303 serial adapters tend to respond with a NAK when they have
>> nothing to say and thus we end with this same sequence.
>>
>> Signed-off-by: Douglas Anderson 
>> Cc: sta...@vger.kernel.org
>> Reviewed-by: Julius Werner 
>> Tested-by: Stefan Wahren 
>
> This seems too big for -rc or -stable inclusion.

I've removed the stable tag at your request.  I originally added it at
your request in response to v2 of this patch.  I'd agree that it's a
pretty big patch and therefore "risky" to pick back to stable.  ...but
it does fix a bug reported by several people on the mailing lists, so
I'll leave it to your discretion.  Previously in relation to the
stable tag, I had mentioned:

   It's a little weird since it doesn't "fix" any specific
   commit, so I guess it will be up to stable folks to decide how far to
   go back.  The dwc2 devices I work with are actually on 3.14, but we
   have some pretty massive backports related to dwc2 there...

> In any case, this
> doesn't apply to my testing/next branch. Care to rebase and collect acks
> you received while doing that?

Sure, no problem.  I've posted v4 with John Youn's Ack.  The reason v3
didn't apply is that you've now got commit e99e88a9d2b0 ("treewide:
setup_timer() -> timer_setup()").  Originally my plan was to beat that
patch into the kernel and then I'd do the timer conversion myself.
That was patch #2 in the v3 series, AKA
.  ...but since I failed
to beat Kees' patch in, I've now squashed patches #1 and #2 together
and resolved the trivial conflict.

If anyone were thinking of trying to backport this patch to older
kernels (where they presumably don't have Kees's timer patch) they can
always use the v3 patch posted here as a reference for how to make
things work.  ;)


-Doug
--
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

Re: [PATCH v2] usb: dwc2: host: Don't retry NAKed transactions right away

[Doug Anderson]

Hi,

On Tue, Nov 28, 2017 at 6:37 AM, Stefan Wahren  wrote:
> Hi Doug,
>
> Doug Anderson  hat am 30. Oktober 2017 um 18:14
> geschrieben:
>
> Hi,
>
> On Mon, Oct 30, 2017 at 1:32 AM, Felipe Balbi  wrote:
>>
>
> Hi,
>
> Doug Anderson  writes:
>
> Hi,
>
> On Sat, Oct 28, 2017 at 8:51 AM, Stefan Wahren 
> wrote:
>
> Hi Doug,
>
> [add Felipe since this should go through his tree]
>
> Ah. Sorry Felipe! I know you've landed some dwc2 stuff in the past
>
> No problems :-)
>
> but for some reason get_maintainer didn't ID you so I thought maybe
> you weren't doing it anymore. Please let me know if you'd like me to
> send this to you again with collected Reviewed-by and Tested-by tags.
>
> Yeah, please resend with all tags collected, however let's wait for a
> week or so and give other people time to catch up. I already sent my
> pull request to Greg, this would, anyway, go into the -rc cycle.
>
> Doh! I just re-read this one more time (after sending v3) and
> realized I had read it incorrectly. I read it as "please send the
> patch with the tags and I'll wait a week before landing", but you
> actually said "please wait a week before re-sending". Sorry for the
> noise. In the very least, you should be on the "To" line now so if
> anyone else has any extra tags it should be very easy for you to see
> them.
>
> Right that there's no massive urgency. It's been broken forever.
>
> since 4.15rc1 is out, how are your plans for the next Patch version?

Yeah, I was going to wait a week or so before pestering Felipe since
some people are still in Thanksgiving holiday mode and rc1 _just_ came
out...  My understanding is that the existing v3 patch is OK as-is and
I wasn't planning to send another one.  patchwork.kernel.org IDs for
the most recent version:

10032939 New  [v3,1/2] usb: dwc2: host: Don't retry NAKed
transactions right away
10032935 New  [v3,2/2] usb: dwc2: host: Convert hcd_queue to timer_setup

-Doug
--
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

Re: [PATCH v2] usb: dwc2: host: Don't retry NAKed transactions right away

[Doug Anderson]

Hi,

On Mon, Oct 30, 2017 at 1:32 AM, Felipe Balbi  wrote:
>
> Hi,
>
> Doug Anderson  writes:
>> Hi,
>>
>> On Sat, Oct 28, 2017 at 8:51 AM, Stefan Wahren  
>> wrote:
>>> Hi Doug,
>>>
>>> [add Felipe since this should go through his tree]
>>
>> Ah.  Sorry Felipe!  I know you've landed some dwc2 stuff in the past
>
> No problems :-)
>
>> but for some reason get_maintainer didn't ID you so I thought maybe
>> you weren't doing it anymore.  Please let me know if you'd like me to
>> send this to you again with collected Reviewed-by and Tested-by tags.
>
> Yeah, please resend with all tags collected, however let's wait for a
> week or so and give other people time to catch up. I already sent my
> pull request to Greg, this would, anyway, go into the -rc cycle.

Doh!  I just re-read this one more time (after sending v3) and
realized I had read it incorrectly.  I read it as "please send the
patch with the tags and I'll wait a week before landing", but you
actually said "please wait a week before re-sending".  Sorry for the
noise.  In the very least, you should be on the "To" line now so if
anyone else has any extra tags it should be very easy for you to see
them.

Right that there's no massive urgency.  It's been broken forever.


> Please add a Cc stable tag too, if necessary.

Good point.  It's a little weird since it doesn't "fix" any specific
commit, so I guess it will be up to stable folks to decide how far to
go back.  The dwc2 devices I work with are actually on 3.14, but we
have some pretty massive backports related to dwc2 there...


-Doug
--
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

Re: [PATCH v2] usb: dwc2: host: Don't retry NAKed transactions right away

[Doug Anderson]

Hi,

On Sat, Oct 28, 2017 at 8:51 AM, Stefan Wahren  wrote:
> Hi Doug,
>
> [add Felipe since this should go through his tree]

Ah.  Sorry Felipe!  I know you've landed some dwc2 stuff in the past
but for some reason get_maintainer didn't ID you so I thought maybe
you weren't doing it anymore.  Please let me know if you'd like me to
send this to you again with collected Reviewed-by and Tested-by tags.


> i'm back from vacation and happy to see this patch.
>
>> Douglas Anderson  hat am 26. Oktober 2017 um 22:22 
>> geschrieben:
>>
>>
>> On rk3288-veyron devices on Chrome OS it was found that plugging in an
>> Arduino-based USB device could cause the system to lockup, especially
>> if the CPU Frequency was at one of the slower operating points (like
>> 100 MHz / 200 MHz).
>>
>> Upon tracing, I found that the following was happening:
>> * The USB device (full speed) was connected to a high speed hub and
>>   then to the rk3288.  Thus, we were dealing with split transactions,
>>   which is all handled in software on dwc2.
>> * Userspace was initiating a BULK IN transfer
>> * When we sent the SSPLIT (to start the split transaction), we got an
>>   ACK.  Good.  Then we issued the CSPLIT.
>> * When we sent the CSPLIT, we got back a NAK.  We immediately (from
>>   the interrupt handler) started to retry and sent another SSPLIT.
>> * The device kept NAKing our CSPLIT, so we kept ping-ponging between
>>   sending a SSPLIT and a CSPLIT, each time sending from the interrupt
>>   handler.
>> * The handling of the interrupts was (because of the low CPU speed and
>>   the inefficiency of the dwc2 interrupt handler) was actually taking
>>   _longer_ than it took the other side to send the ACK/NAK.  Thus we
>>   were _always_ in the USB interrupt routine.
>> * The fact that USB interrupts were always going off was preventing
>>   other things from happening in the system.  This included preventing
>>   the system from being able to transition to a higher CPU frequency.
>>
>> As I understand it, there is no requirement to retry super quickly
>> after a NAK, we just have to retry sometime in the future.  Thus one
>> solution to the above is to just add a delay between getting a NAK and
>> retrying the transmission.  If this delay is sufficiently long to get
>> out of the interrupt routine then the rest of the system will be able
>> to make forward progress.  Even a 25 us delay would probably be
>> enough, but we'll be extra conservative and try to delay 1 ms (the
>> exact amount depends on HZ and the accuracy of the jiffy and how close
>> the current jiffy is to ticking, but could be as much as 20 ms or as
>> little as 1 ms).
>>
>> Presumably adding a delay like this could impact the USB throughput,
>> so we only add the delay with repeated NAKs.
>>
>> NOTE: Upon further testing of a pl2303 serial adapter, I found that
>> this fix may help with problems there.  Specifically I found that the
>> pl2303 serial adapters tend to respond with a NAK when they have
>> nothing to say and thus we end with this same sequence.
>
> This patch fixes "dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 
> 146s" on bcm2835. After applying your patch on linux-next plus "mm: drop 
> migrate type checks from has_unmovable_pages" i'm able to open 3 PL2303 
> serial adapters parallel on a Raspberry Pi 1 B without any issues.

Woohoo!  It only took a year from your report of this issue to getting
a fix.  ;)  It sounds as if Julius found something the other day too
(dealing better with stalls), so any day now we'll have a mainline
kernel that can keep up with "blazingly fast" UART connections.  :-P


> Tested-by: Stefan Wahren 


Thank you so much for the testing!


> Please see my comment below
>
>>
>> Signed-off-by: Douglas Anderson 
>> Reviewed-by: Julius Werner 
>> ---
>>
>> Changes in v2:
>> - Address http://crosreview.com/737520 feedback
>>
>>  drivers/usb/dwc2/core.h  |  1 +
>>  drivers/usb/dwc2/hcd.c   |  7 
>>  drivers/usb/dwc2/hcd.h   |  9 +
>>  drivers/usb/dwc2/hcd_intr.c  | 20 +++
>>  drivers/usb/dwc2/hcd_queue.c | 81 
>> +---
>>  5 files changed, 114 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 8367d4f985c1..c0807e558726 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -962,6 +962,7 @@ struct dwc2_hsotg {
>>   } flags;
>>
>>   struct list_head non_periodic_sched_inactive;
>> + struct list_head non_periodic_sched_waiting;
>>   struct list_head non_periodic_sched_active;
>>   struct list_head *non_periodic_qh_ptr;
>>   struct list_head periodic_sched_inactive;
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index c2631145f404..ed92ae78dcf9 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -653,6 +653,10 @@ static void dwc2_dump_channel_info(struct dwc2_hsotg 
>> *hsotg,
>>   list_for_each_entry(qh, &hso

Re: [PATCH] usb: dwc2: host: Don't retry NAKed transactions right away

[Doug Anderson]

Hi,

On Wed, Oct 25, 2017 at 2:08 PM, Douglas Anderson  wrote:
> On rk3288-veyron devices on Chrome OS it was found that plugging in an
> Arduino-based USB device could cause the system to lockup, especially
> if the CPU Frequency was at one of the slower operating points (like
> 100 MHz / 200 MHz).
>
> Upon tracing, I found that the following was happening:
> * The USB device (full speed) was connected to a high speed hub and
>   then to the rk3288.  Thus, we were dealing with split transactions,
>   which is all handled in software on dwc2.
> * Userspace was initiating a BULK IN transfer
> * When we sent the SSPLIT (to start the split transaction), we got an
>   ACK.  Good.  Then we issued the CSPLIT.
> * When we sent the CSPLIT, we got back a NAK.  We immediately (from
>   the interrupt handler) started to retry and sent another SSPLIT.
> * The device kept NAKing our CSPLIT, so we kept ping-ponging between
>   sending a SSPLIT and a CSPLIT, each time sending from the interrupt
>   handler.
> * The handling of the interrupts was (because of the low CPU speed and
>   the inefficiency of the dwc2 interrupt handler) was actually taking
>   _longer_ than it took the other side to send the ACK/NAK.  Thus we
>   were _always_ in the USB interrupt routine.
> * The fact that USB interrupts were always going off was preventing
>   other things from happening in the system.  This included preventing
>   the system from being able to transition to a higher CPU frequency.
>
> As I understand it, there is no requirement to retry super quickly
> after a NAK, we just have to retry sometime in the future.  Thus one
> solution to the above is to just add a delay between getting a NAK and
> retrying the transmission.  If this delay is sufficiently long to get
> out of the interrupt routine then the rest of the system will be able
> to make forward progress.  Even a 25 us delay would probably be
> enough, but we'll be extra conservative and try to delay 1 ms (the
> exact amount depends on HZ and the accuracy of the jiffy and how close
> the current jiffy is to ticking, but could be as much as 20 ms or as
> little as 1 ms).
>
> Presumably adding a delay like this could impact the USB throughput,
> so we only add the delay with repeated NAKs.
>
> NOTE: Upon further testing of a pl2303 serial adapter, I found that
> this fix may help with problems there.  Specifically I found that the
> pl2303 serial adapters tend to respond with a NAK when they have
> nothing to say and thus we end with this same sequence.
>
> Signed-off-by: Douglas Anderson 
> ---
>
>  drivers/usb/dwc2/core.h  |  1 +
>  drivers/usb/dwc2/hcd.c   |  7 
>  drivers/usb/dwc2/hcd.h   |  9 +
>  drivers/usb/dwc2/hcd_intr.c  | 12 +++
>  drivers/usb/dwc2/hcd_queue.c | 81 
> +---
>  5 files changed, 106 insertions(+), 4 deletions(-)

One note is that I got some review feedback on the Chrome OS gerrit at
.  I'll plan to spin with that feedback
tomorrow unless I hear otherwise.  Thanks!  :)

-Doug
--
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

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Hi,

On Mon, Oct 16, 2017 at 1:49 PM, Julius Werner  wrote:
>> d9a14b00 339317035 C Ii:1:004:1 -32:1 0
>> d9a14b00 339317049 S Ii:1:004:1 -115:1 10 <
>> d9a14b00 339318040 C Ii:1:004:1 -32:1 0
>> d9a14b00 339318057 S Ii:1:004:1 -115:1 10 <
>> d9a14b00 339319042 C Ii:1:004:1 -32:1 0
>> d9a14b00 339319056 S Ii:1:004:1 -115:1 10 <
>> d9a14b00 339329551 C Ii:1:004:1 -32:1 0
>> d9a14b00 339329571 S Ii:1:004:1 -115:1 10 <
>> d9a14b00 339330586 C Ii:1:004:1 -32:1 0
>> d9a14b00 339330601 S Ii:1:004:1 -115:1 10 <
>> d9a14b00 339331035 C Ii:1:004:1 -32:1 0
>
> Sorry for necromancing an old thread, but I just happened to read
> through this and thought someone might care:
>
> If I read that right, the usbmon output shows that the interrupt
> endpoint is stalled (keeps returning -EPIPE). A STALL is a special
> device-side USB condition that tells the host something is wrong and
> will persist until cleared manually. It seems that the driver isn't
> prepared for this (see
> drivers/usb/serial/pl2303.c#pl2303_read_int_callback) and just keeps
> resubmitting the URB, so it will stall again as fast as the endpoint
> allows it to. This may be the reason why you get so many transfers
> that it overwhelms the CPU.
>
> A fix would be to catch -EPIPE in that function and handle it
> explicitly (with either a CLEAR_STALL to the endpoint or a full USB
> reset... would have to look at the documentation for PL2303 to see
> what the stall actually means and how you're supposed to treat it).

To further comment on this old thread, I just posted another patch at
 that could also make
pl2303 less able to bring dwc2-based controllers to a screeching halt.
I added many of the people who had taken part in this thread, but if
you were just lurking here then hopefully you can dig it up and try it
out.

-Doug
--
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

Re: [PATCH V4] r8152: add Linksys USB3GIGV1 id

[Doug Anderson]

Grant,

On Thu, Sep 28, 2017 at 11:35 AM, Grant Grundler  wrote:
> This linksys dongle by default comes up in cdc_ether mode.
> This patch allows r8152 to claim the device:
>Bus 002 Device 002: ID 13b1:0041 Linksys
>
> Signed-off-by: Grant Grundler 
> ---
>  drivers/net/usb/cdc_ether.c | 10 ++
>  drivers/net/usb/r8152.c |  2 ++
>  2 files changed, 12 insertions(+)

This seems nice to me now.  Thanks for all the fixes!  I'm no expert
in this area, but as far as I know this is ready to go now, so FWIW:

Reviewed-by: Douglas Anderson 
--
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

Re: [PATCH V2] r8152: add Linksys USB3GIGV1 id

[Doug Anderson]

Hi,

On Thu, Sep 28, 2017 at 3:28 PM, Rustad, Mark D  wrote:
>
>> On Sep 27, 2017, at 9:39 AM, Grant Grundler  wrote:
>>
>> On Wed, Sep 27, 2017 at 12:15 AM, Oliver Neukum  wrote:
>>> Am Dienstag, den 26.09.2017, 08:19 -0700 schrieb Doug Anderson:
>>>>
>>>> I know that for at least some of the adapters in the CDC Ethernet
>>>> blacklist it was claimed that the CDC Ethernet support in the adapter
>>>> was kinda broken anyway so the blacklist made sense.  ...but for the
>>>> Linksys Gigabit adapter the CDC Ethernet driver seems to work OK, it's
>>>> just not quite as full featured / efficient as the R8152 driver.
>>>>
>>>> Is that not a concern?  I guess you could tell people in this
>>>> situation that they simply need to enable the R8152 driver to get
>>>> continued support for their Ethernet adapter?
>>>
>>> Hi,
>>>
>>> yes, it is a valid concern. An #ifdef will be needed.
>>
>> Good idea - I will post V3 shortly.
>>
>> I'm assuming you mean to add #ifdef CONFIG_USB_RTL8152 around the
>> blacklist entry in cdc_ether driver.
>
> Shouldn't that be an #if IS_ENABLED(...) test, since that seems to be the 
> proper way to check configured drivers.

Yes, I had the same feedback on v3.  See my comments at
<https://patchwork.kernel.org/patch/9974485/>.  Grant has fixed it in
v4.  Please see <https://patchwork.kernel.org/patch/9976657/>.  :)

-Doug
--
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

Re: [PATCH V3] r8152: add Linksys USB3GIGV1 id

[Doug Anderson]

Hi,

On Wed, Sep 27, 2017 at 5:07 PM, Grant Grundler  wrote:
>>>  #define DELL_VENDOR_ID 0x413C
>>>  #define REALTEK_VENDOR_ID  0x0bda
>>>  #define SAMSUNG_VENDOR_ID  0x04e8
>>> +#define LINKSYS_VENDOR_ID  0x13b1
>>>  #define LENOVO_VENDOR_ID   0x17ef
>>
>> Slight nit that "LI" sorts after "LE".  You got it right in the other case...
>
> The list isn't sorted by any rational thing I can see.  I managed to
> check my OCD reaction to sort the list numerically. :)

Whoops, you're right.  It seems to be in a random order.  I just saw
LE, LI, and N sorted properly and jumped to a conclusion.  In any
case, if it's all the same to you it'd be nice if you were consistent
between LENOVO/LINKSYS in the two files.  :-P

-Doug
--
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

Re: [PATCH V3] r8152: add Linksys USB3GIGV1 id

[Doug Anderson]

Hi,

On Wed, Sep 27, 2017 at 10:28 AM, Grant Grundler  wrote:
> This linksys dongle by default comes up in cdc_ether mode.
> This patch allows r8152 to claim the device:
>Bus 002 Device 002: ID 13b1:0041 Linksys
>
> Signed-off-by: Grant Grundler 
> ---
>  drivers/net/usb/cdc_ether.c | 10 ++
>  drivers/net/usb/r8152.c |  2 ++
>  2 files changed, 12 insertions(+)
>
> V3: for backwards compat, add #ifdef CONFIG_USB_RTL8152 around
> the cdc_ether blacklist entry so the cdc_ether driver can
> still claim the device if r8152 driver isn't configured.
>
> V2: add LINKSYS_VENDOR_ID to cdc_ether blacklist
>
> diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
> index 8ab281b478f2..446dcc0f1f70 100644
> --- a/drivers/net/usb/cdc_ether.c
> +++ b/drivers/net/usb/cdc_ether.c
> @@ -546,6 +546,7 @@ static const struct driver_info wwan_info = {
>  #define DELL_VENDOR_ID 0x413C
>  #define REALTEK_VENDOR_ID  0x0bda
>  #define SAMSUNG_VENDOR_ID  0x04e8
> +#define LINKSYS_VENDOR_ID  0x13b1
>  #define LENOVO_VENDOR_ID   0x17ef

Slight nit that "LI" sorts after "LE".  You got it right in the other case...


>  #define NVIDIA_VENDOR_ID   0x0955
>  #define HP_VENDOR_ID   0x03f0
> @@ -737,6 +738,15 @@ static const struct usb_device_id  products[] = {
> .driver_info = 0,
>  },
>
> +#ifdef CONFIG_USB_RTL8152
> +/* Linksys USB3GIGV1 Ethernet Adapter */
> +{
> +   USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, 
> USB_CLASS_COMM,
> +   USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
> +   .driver_info = 0,
> +},
> +#endif

I believe you want to use IS_ENABLED(), don't you?

There's still a weird esoteric side case where kernel modules don't
all need to be included in the filesystem just because they were built
at the same time.  ...but IMHO that seems like enough of a nit that we
can probably ignore it unless someone has a better idea.


-Doug
--
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

Re: [PATCH V2] r8152: add Linksys USB3GIGV1 id

[Doug Anderson]

Hi

On Mon, Sep 25, 2017 at 6:09 PM, Grant Grundler  wrote:
> This linksys dongle by default comes up in cdc_ether mode.
> This patch allows r8152 to claim the device:
>Bus 002 Device 002: ID 13b1:0041 Linksys
>
> Signed-off-by: Grant Grundler 
> ---
>  drivers/net/usb/cdc_ether.c | 8 
>  drivers/net/usb/r8152.c | 2 ++
>  2 files changed, 10 insertions(+)
>
> V2: add LINKSYS_VENDOR_ID to cdc_ether blacklist

I understand that in v1 people pointed out that if we didn't add this
to the cdc_ether blacklist that we might end up picking the wrong
driver.  ...but one thing concerns me: what happens if someone has the
CDC_ETHER driver configured in their system but _not_ the R8152
driver?  All of a sudden this USB Ethernet adapter which used to work
fine with the CDC Ethernet driver will stop working.

I know that for at least some of the adapters in the CDC Ethernet
blacklist it was claimed that the CDC Ethernet support in the adapter
was kinda broken anyway so the blacklist made sense.  ...but for the
Linksys Gigabit adapter the CDC Ethernet driver seems to work OK, it's
just not quite as full featured / efficient as the R8152 driver.

Is that not a concern?  I guess you could tell people in this
situation that they simply need to enable the R8152 driver to get
continued support for their Ethernet adapter?


-Doug
--
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

Re: [RFC PATCH 2/3] usbnet: Avoid potential races in usbnet_deferred_kevent()

[Doug Anderson]

Hi,

On Tue, Sep 19, 2017 at 1:37 PM, Oliver Neukum  wrote:
> Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:
>> In general when you've got a flag communicating that "something needs
>> to be done" you want to clear that flag _before_ doing the task.  If
>> you clear the flag _after_ doing the task you end up with the risk
>> that this will happen:
>>
>> 1. Requester sets flag saying task A needs to be done.
>> 2. Worker comes and stars doing task A.
>> 3. Worker finishes task A but hasn't yet cleared the flag.
>> 4. Requester wants to set flag saying task A needs to be done again.
>> 5. Worker clears the flag without doing anything.
>>
>> Let's make the usbnet codebase consistently clear the flag _before_ it
>> does the requested work.  That way if there's another request to do
>> the work while the work is already in progress it won't be lost.
>>
>> NOTES:
>> - No known bugs are fixed by this; it's just found by code inspection.
>
> Hi,
>
> unfortunately the patch is wrong. The flags must be cleared only
> in case the handler is successful. That is not guaranteed.
>
> Regards
> Oliver
>
> NACK

OK, thanks for reviewing!  I definitely wasn't super confident about
the patch (hence the RFC).

Do you think that the races I identified are possible to hit?  In
other words: should I try to rework the patch somehow or just drop it?
 Originally I had the patch setting the flags back to true in the
failure cases, but then I convinced myself that wasn't needed.  I can
certainly go back and try it that way...

-Doug
--
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

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Hi,

On Wed, May 10, 2017 at 9:31 AM, Johan Hovold  wrote:
> On Mon, May 08, 2017 at 10:22:08PM +0200, Stefan Wahren wrote:
>>
>> > Stefan Wahren  hat am 25. April 2017 um 20:11 
>> > geschrieben:
>> >
>> >
>> > > Stefan Wahren  hat am 22. April 2017 um 22:50 
>> > > geschrieben:
>> > >
>> > >
>> > > Hi,
>> > >
>> > > > Eric Anholt  hat am 20. April 2017 um 20:54 
>> > > > geschrieben:
>> > > >
>> > > >
>> > > > Stefan Wahren  writes:
>> > > >
>> > > > > Hi,
>> > > > >
>> > > > >> Doug Anderson  hat am 18. April 2017 um 
>> > > > >> 22:41 geschrieben:
>> > > > >>
>> > > > >>
>> > > > >> It's hard to know for sure that all of this time is really in
>> > > > >> urb_enqueue().  Possible we could have task switched out and been
>> > > > >> blocked elsewhere.  Using ftrace to get more fine-grained timings
>> > > > >> would be useful.  ktime_get(), ktime_sub(), and ktime_to_us() are 
>> > > > >> your
>> > > > >> friends here if you want to use trace_printk.
>> > > > >
>> > > > > i'm a newbie to ftrace, so i hope this would be helpful.
>> > > > >
>> > > > > # connect PL2303 to the onboard hub
>> > > > > # echo 0 > options/sleep-time
>> > > > > # echo 0 > function_profile_enabled
>> > > > > # echo 1 > function_profile_enabled
>> > > > > # ./usb_test
>> > > > > # Waiting for at least 20 seconds and then disconnect PL2303
>> > > > > # echo 0 > function_profile_enabled
>> > > > > # cat trace_stat/function0
>> > > > >
>> > > > >   Function   HitTimeAvg  
>> > > > >s^2
>> > > > >      ------  
>> > > > >---
>> > > > >   bcm2835_handle_irq  361347219567633 us 
>> > > > > 607.636 us  1485199 us
>> > > > >   __handle_domain_irq1082482212639551 us 
>> > > > > 196.437 us  3642030 us
>> > > > >   generic_handle_irq 1082482100592051 us 
>> > > > > 92.927 us   50511334 us
>> > > > >   irq_exit   108248298197771 us 
>> > > > > 90.715 us   29649040 us
>> > > > >   handle_level_irq   108248295812379 us 
>> > > > > 88.511 us   51910093 us
>> > > >
>> > > > If I'm reading this output right, we're spending half of our interrupt
>> > > > processing time in irq_exit(), so even if dwc2's interrupt was free 
>> > > > (the
>> > > > generic_handle_irq() chain), we'd be eating about half the CPU getting
>> > > > back out of the interrupt handler, right?
>> > > >
>> > > > I don't really know anything about DWC2 or USB, but is there any way we
>> > > > could mitigate the interrupt frequency with this hardware?  If nothing
>> > > > else, could we loop reading gintsts until it reads back 0?
>> > >
>> > > first of all i updated my kernel to 4.11rc7 and the issue still
>> > > occures. Today i had some time to investigate this issue further
>> > > and i made some interesting observations:
>> > >
>> > > 1. The lockup doesn't occure always after starting usb_test. In
>> > > rare cases i was able to run the program without lockup.
>> > > 2. In case the lockup occured we are always able to "rescue" the
>> > > BCM2835 from this state by sending some serial data to the PL2303.
>> >
>> > Based on this scenario i patched the interrupt routine to detect the
>> > interrupt storm and normal condition. So i can dump the global and
>> > host registers in both situations (bad and goodcase).
>> >
>> > Here is the diff between both register dumps, maybe someone see
>> > something interesting:
>> >
>> > --- badcase.txt 2017-04-25 18:02:59.0 +
>> > +++ goodcase.txt2017

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Hi,

On Thu, Apr 20, 2017 at 12:57 PM, Eric Anholt  wrote:
> Doug Anderson  writes:
>
>> Hi,
>>
>> On Thu, Apr 20, 2017 at 11:54 AM, Eric Anholt  wrote:
>>> Stefan Wahren  writes:
>>>
>>>> Hi,
>>>>
>>>>> Doug Anderson  hat am 18. April 2017 um 22:41 
>>>>> geschrieben:
>>>>>
>>>>>
>>>>> It's hard to know for sure that all of this time is really in
>>>>> urb_enqueue().  Possible we could have task switched out and been
>>>>> blocked elsewhere.  Using ftrace to get more fine-grained timings
>>>>> would be useful.  ktime_get(), ktime_sub(), and ktime_to_us() are your
>>>>> friends here if you want to use trace_printk.
>>>>
>>>> i'm a newbie to ftrace, so i hope this would be helpful.
>>>>
>>>> # connect PL2303 to the onboard hub
>>>> # echo 0 > options/sleep-time
>>>> # echo 0 > function_profile_enabled
>>>> # echo 1 > function_profile_enabled
>>>> # ./usb_test
>>>> # Waiting for at least 20 seconds and then disconnect PL2303
>>>> # echo 0 > function_profile_enabled
>>>> # cat trace_stat/function0
>>>>
>>>>   Function   HitTimeAvg
>>>>  s^2
>>>>      ------
>>>>  ---
>>>>   bcm2835_handle_irq  361347219567633 us 607.636 
>>>> us  1485199 us
>>>>   __handle_domain_irq1082482212639551 us 196.437 
>>>> us  3642030 us
>>>>   generic_handle_irq 1082482100592051 us 92.927 us 
>>>>   50511334 us
>>>>   irq_exit   108248298197771 us 90.715 us  
>>>>  29649040 us
>>>>   handle_level_irq   108248295812379 us 88.511 us  
>>>>  51910093 us
>>>
>>> If I'm reading this output right, we're spending half of our interrupt
>>> processing time in irq_exit(), so even if dwc2's interrupt was free (the
>>> generic_handle_irq() chain), we'd be eating about half the CPU getting
>>> back out of the interrupt handler, right?
>>>
>>> I don't really know anything about DWC2 or USB, but is there any way we
>>> could mitigate the interrupt frequency with this hardware?  If nothing
>>> else, could we loop reading gintsts until it reads back 0?
>>
>> Take ftrace with a little bit of a grain of salt, especially on older
>> / slower ARMs (without the arch timer).  Whenever ftrace takes a log
>> it grabs a timestamp.  This can be an expensive (ish) operation.  Even
>> on newer CPUs it's still not free if you call it as much as ftrace,
>> but on older CPUs it's extra expensive.
>
> If per-function timestamp cost was the problem, shouldn't I expect to
> see a bunch of irq_exit()'s children each taking a bit of time?  We have
> a long callchain with the functions each taking a bit of time in the
> dwc2 interrupt handler, but irq_exit() seems to be a monolithic cost.

Maybe.  I remember some of the timestamp code being a might bit odd.
Specifically there might have been cases where the timestamp code
predictable slower in some cases, but that slowness would be blamed on
the wrong function.  I think this might have to do with some of the
the fact that a memory mapped read could block until other outstanding
memory mapped operations finished.

-Doug
--
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

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Hi,

On Thu, Apr 20, 2017 at 11:54 AM, Eric Anholt  wrote:
> Stefan Wahren  writes:
>
>> Hi,
>>
>>> Doug Anderson  hat am 18. April 2017 um 22:41 
>>> geschrieben:
>>>
>>>
>>> It's hard to know for sure that all of this time is really in
>>> urb_enqueue().  Possible we could have task switched out and been
>>> blocked elsewhere.  Using ftrace to get more fine-grained timings
>>> would be useful.  ktime_get(), ktime_sub(), and ktime_to_us() are your
>>> friends here if you want to use trace_printk.
>>
>> i'm a newbie to ftrace, so i hope this would be helpful.
>>
>> # connect PL2303 to the onboard hub
>> # echo 0 > options/sleep-time
>> # echo 0 > function_profile_enabled
>> # echo 1 > function_profile_enabled
>> # ./usb_test
>> # Waiting for at least 20 seconds and then disconnect PL2303
>> # echo 0 > function_profile_enabled
>> # cat trace_stat/function0
>>
>>   Function   HitTimeAvg  
>>s^2
>>      ------  
>>---
>>   bcm2835_handle_irq  361347219567633 us 607.636 us  
>> 1485199 us
>>   __handle_domain_irq1082482212639551 us 196.437 us  
>> 3642030 us
>>   generic_handle_irq 1082482100592051 us 92.927 us   
>> 50511334 us
>>   irq_exit   108248298197771 us 90.715 us
>>29649040 us
>>   handle_level_irq   108248295812379 us 88.511 us
>>51910093 us
>
> If I'm reading this output right, we're spending half of our interrupt
> processing time in irq_exit(), so even if dwc2's interrupt was free (the
> generic_handle_irq() chain), we'd be eating about half the CPU getting
> back out of the interrupt handler, right?
>
> I don't really know anything about DWC2 or USB, but is there any way we
> could mitigate the interrupt frequency with this hardware?  If nothing
> else, could we loop reading gintsts until it reads back 0?

Take ftrace with a little bit of a grain of salt, especially on older
/ slower ARMs (without the arch timer).  Whenever ftrace takes a log
it grabs a timestamp.  This can be an expensive (ish) operation.  Even
on newer CPUs it's still not free if you call it as much as ftrace,
but on older CPUs it's extra expensive.

I spent a chunk of time working on optimizations for that on exynos
since it showed up in profiles as an expensive operation (Chrome asks
for the time a lot during its internal profiling).  Some of that type
of data is in commit 3252a646aa2c ("clocksource: exynos_mct: Only use
32-bits where possible").
--
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

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Hi,

On Thu, Apr 20, 2017 at 12:46 AM, Stefan Wahren  wrote:
> Am 19.04.2017 um 23:47 schrieb Doug Anderson:
>> Hi,
>>
>> On Wed, Apr 19, 2017 at 1:25 PM, Stefan Wahren  
>> wrote:
>>> Hi,
>>>
>>>> Doug Anderson  hat am 18. April 2017 um 22:41 
>>>> geschrieben:
>>>>
>>>>
>>>> It's hard to know for sure that all of this time is really in
>>>> urb_enqueue().  Possible we could have task switched out and been
>>>> blocked elsewhere.  Using ftrace to get more fine-grained timings
>>>> would be useful.  ktime_get(), ktime_sub(), and ktime_to_us() are your
>>>> friends here if you want to use trace_printk.
>>> i'm a newbie to ftrace, so i hope this would be helpful.
>>>
>>> # connect PL2303 to the onboard hub
>>> # echo 0 > options/sleep-time
>>> # echo 0 > function_profile_enabled
>>> # echo 1 > function_profile_enabled
>>> # ./usb_test
>>> # Waiting for at least 20 seconds and then disconnect PL2303
>>> # echo 0 > function_profile_enabled
>>> # cat trace_stat/function0
>>>
>>>   Function   HitTimeAvg 
>>> s^2
>>>      ------ 
>>> ---
>>>   bcm2835_handle_irq  361347219567633 us 607.636 us 
>>>  1485199 us
>>>   __handle_domain_irq1082482212639551 us 196.437 us 
>>>  3642030 us
>>>   generic_handle_irq 1082482100592051 us 92.927 us  
>>>  50511334 us
>>>   irq_exit   108248298197771 us 90.715 us   
>>> 29649040 us
>>>   handle_level_irq   108248295812379 us 88.511 us   
>>> 51910093 us
>>>   do_sys_open   180687612983 us 48512.17 us 
>>> 2198507 us
>>>   SyS_open  160187372331 us 54573.59 us 
>>> 1898996 us
>>>   do_filp_open  186287368058 us 46921.62 us 
>>> 1634982 us
>>>   path_openat   186287314553 us 46892.88 us 
>>> 3357817 us
>>>   __do_softirq  303586266050 us 28423.73 us 
>>> 6449768 us
>>>   vfs_open  151585877012 us 56684.49 us 
>>> 101673.5 us
>>>   do_dentry_open151585861429 us 56674.21 us 
>>> 812420.7 us
>>>   usb_submit_urb 13685760172 us 630589.5 us 
>>> 59532024 us
>>>   usb_hcd_submit_urb 13485756518 us 639974.0 us 
>>> 726298102 us
>>>   _dwc2_hcd_urb_enqueue  13485738333 us 639838.3 us 
>>> 874104371 us
>> The 134 calls to this are taking a ton of time.  It would be nice to
>> know where all the time actually was in here.  Are you on a
>> single-core device, or multi-core?
>
> Raspberry Pi B is a single core device which runs with 700 MHz cpu freq.
>
>> Oh, probably this so slow because we're just getting interrupted
>> constantly.  You can see that during your trace 80686112 us was in
>> handle_irq_event().  Presumably all of that time could have been
>> counted towards whatever code you were in when the interrupt went off.
>> Of that 80 seconds, 60 seconds or so was usb_hcd_irq, and of that 57
>> seconds was in _dwc2_hcd_irq().
>
> Since the FTDI (full speed device) and the PL2303 (also full speed
> device) behaves differently, i will try to compare those ftraces.

The most interesting to know is what the endpoints looked like for
these two devices.  Since they are full speed devices, I'd expect that
any interrupt end points should fire once per "ms" at most.


>> Presumably the big part of your problem is that dwc2_handle_hcd_intr()
>> is just too slow.  From glancing at the numbers below, it seems likely
>> that nothing in particular is slow, it's just running a lot of code
>> and (in total) all of that is slow.  ...but someone with more time on
>> their hands would need to really debug.
>>
>> One thing that would be interesting would be to see if you can
>> increase the bug clock for talking to the dwc2 controller.
>
> Unfortunately the bcm2835 datasheet [1] doesn't provide many information
> about the USB IP core and the Synopsys documents ar

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Hi,

On Wed, Apr 19, 2017 at 1:25 PM, Stefan Wahren  wrote:
> Hi,
>
>> Doug Anderson  hat am 18. April 2017 um 22:41 
>> geschrieben:
>>
>>
>> It's hard to know for sure that all of this time is really in
>> urb_enqueue().  Possible we could have task switched out and been
>> blocked elsewhere.  Using ftrace to get more fine-grained timings
>> would be useful.  ktime_get(), ktime_sub(), and ktime_to_us() are your
>> friends here if you want to use trace_printk.
>
> i'm a newbie to ftrace, so i hope this would be helpful.
>
> # connect PL2303 to the onboard hub
> # echo 0 > options/sleep-time
> # echo 0 > function_profile_enabled
> # echo 1 > function_profile_enabled
> # ./usb_test
> # Waiting for at least 20 seconds and then disconnect PL2303
> # echo 0 > function_profile_enabled
> # cat trace_stat/function0
>
>   Function   HitTimeAvg   
>   s^2
>      ------   
>   ---
>   bcm2835_handle_irq  361347219567633 us 607.636 us   
>1485199 us
>   __handle_domain_irq1082482212639551 us 196.437 us   
>3642030 us
>   generic_handle_irq 1082482100592051 us 92.927 us
>50511334 us
>   irq_exit   108248298197771 us 90.715 us 
>   29649040 us
>   handle_level_irq   108248295812379 us 88.511 us 
>   51910093 us
>   do_sys_open   180687612983 us 48512.17 us   
>   2198507 us
>   SyS_open  160187372331 us 54573.59 us   
>   1898996 us
>   do_filp_open  186287368058 us 46921.62 us   
>   1634982 us
>   path_openat   186287314553 us 46892.88 us   
>   3357817 us
>   __do_softirq  303586266050 us 28423.73 us   
>   6449768 us
>   vfs_open  151585877012 us 56684.49 us   
>   101673.5 us
>   do_dentry_open151585861429 us 56674.21 us   
>   812420.7 us
>   usb_submit_urb 13685760172 us 630589.5 us   
>   59532024 us
>   usb_hcd_submit_urb 13485756518 us 639974.0 us   
>   726298102 us
>   _dwc2_hcd_urb_enqueue  13485738333 us 639838.3 us   
>   874104371 us

The 134 calls to this are taking a ton of time.  It would be nice to
know where all the time actually was in here.  Are you on a
single-core device, or multi-core?

Oh, probably this so slow because we're just getting interrupted
constantly.  You can see that during your trace 80686112 us was in
handle_irq_event().  Presumably all of that time could have been
counted towards whatever code you were in when the interrupt went off.
Of that 80 seconds, 60 seconds or so was usb_hcd_irq, and of that 57
seconds was in _dwc2_hcd_irq().

Presumably the big part of your problem is that dwc2_handle_hcd_intr()
is just too slow.  From glancing at the numbers below, it seems likely
that nothing in particular is slow, it's just running a lot of code
and (in total) all of that is slow.  ...but someone with more time on
their hands would need to really debug.

One thing that would be interesting would be to see if you can
increase the bug clock for talking to the dwc2 controller.  Possibly
some of those memory mapped IOs talking to dwc2 could be making
everything slow?

Other than that, maybe you can find some way to optimize the code in
dwc2 so it runs faster, at least in the cases you care about...



>   chrdev_open 8785716519 us 985247.3 us   
>   1951112835 us
>   tty_open 385714494 us 28571498 us   
>   3743206849 us
>   tty_port_open385712603 us 28570867 us   
>   1968003468 us
>   serial_open  185712472 us 85712472 us   
>   0.000 us
>   serial_port_activate 185709890 us 85709890 us   
>   0.000 us
>   pl2303_open  185709875 us 85709875 us   
>   0.000 us
>   usb_serial_generic_open  185701170 us 85701170 us   
>   0.000 us
>   usb_serial_generic_submit_read   185701166 us 85701166 us   
>   0.000 us
>   usb_serial_generic_submit_read   285701160 us 42850580 us   
>   2252410463 us
>   handle_irq_event   108248280686112 us 74.538 us 
>   36417905 us
>   handle_irq_event_percpu108248278398378 us 72.424 

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Stefan,

On Tue, Apr 18, 2017 at 1:25 PM, Stefan Wahren  wrote:
> Hi,
>
> [add Johan]
>
>> Stefan Wahren  hat am 18. April 2017 um 10:07 
>> geschrieben:
>>
>>
>> Am 18.04.2017 um 00:37 schrieb Doug Anderson:
>> > Hi,
>> >
>> > On Mon, Apr 17, 2017 at 4:05 AM, Stefan Wahren  
>> > wrote:
>> >> Hi,
>> >>
>> >>> Stefan Wahren  hat am 31. Oktober 2016 um 21:34 
>> >>> geschrieben:
>> >>>
>> >>>
>> >>> I inspired by this issue [1] i build up a slightly modified setup with a
>> >>> Raspberry Pi B (mainline kernel 4.9rc3), a powered 7 port USB hub and 5 
>> >>> Prolific
>> >>> PL2303 USB to serial convertors. I modified the usb_test for dwc2 [2], 
>> >>> which
>> >>> only tries to open all ttyUSB devices one after the other.
>> >>>
>> >>> Unfortunately the complete system stuck after opening the first ttyUSB 
>> >>> device (
>> >>> heartbeat LED stop blinking, no reaction to debug UART). The only way to
>> >>> reanimate the system is to powerdown the USB hub with the USB to serial
>> >>> convertors.
>> >>>
>> >>> [1] - https://github.com/raspberrypi/linux/issues/1692
>> >>> [2] - 
>> >>> https://gist.github.com/lategoodbye/dd0d30af27b6f101b03d5923b279dbaa
>> >> since this issue still exists with 4.11 (even without or with microframe 
>> >> scheduler enabled), i want to ask some additional questions:
>> >>
>> >> Is this issue reproducible with other dwc2 platforms than bcm2835?
>> > +Edmund Szeto, who I seem to remember emailing me about similar
>> > questions in the past.
>> >
>> >
>> >> Does the soft lockup also occurs after opening the second serial 
>> >> convertor or later?
>> > I don't have serial converters easily available to me, but back in the
>> > day when I was stressing things out on rk3288 I never saw anything
>> > this bad.  ...of course, on rk3288 we've got 4 A17 cores running
>> > really fast, so possibly just being slower is what causes your
>> > problems here?
>>
>> The downstream kernel of the Raspberry Pi foundation with it's
>> out-of-tree dwc_otg driver is able to handle 8 serial converter on a RPI
>> B. I would be happy to get at least 2 or 3 working on mainline.
>>
>> >
>> > I will make the following observations:
>> >
>> > 1. With dwc2 you often end up in the situation where you need to
>> > service an interrupt every 125 uS.  If servicing that interrupt takes
>> > anywhere near 125 uS in the common case then you'll be in trouble.
>>
>> I will try to measure this with a logic analyzer.
>>
>
> i took GPIO17 to measure _dwc2_hcd_irq and GPIO18 to measure 
> _dwc2_hcd_urb_enqueue (patch against 4.11rc1 below).
>
> So i made my observations for 3 test cases:
>
> 1) no serial converter connected (idle)
> 2) 1 FTDI serial converter connected
> 3) 1 PL2303 serial converter connected
>
> case   | ksoftirq cpu | mean duration | max duration  | max duration | 
> urb_enqueue  |
>|  | hcd_irq   | hcd_irq   | urb_enqueue  | 
> within 10 sec|
> ---+--+---+---+--+--+
> idle   | 0.0% | 2 us  | 16.5 us   | 12 us| 5  
>   |
> FTDI   | 25.0%| 8.5 us| 18.0 us   |  31000 us| ~ 
> 400|
> PL2303 | top doesn't work | 8.5 us| 22.5 us   | 90 us| 4  
>   |

It's hard to know for sure that all of this time is really in
urb_enqueue().  Possible we could have task switched out and been
blocked elsewhere.  Using ftrace to get more fine-grained timings
would be useful.  ktime_get(), ktime_sub(), and ktime_to_us() are your
friends here if you want to use trace_printk.


> So it seems the serial USB driver has also an impact. In the analyzer trace 
> the FTDI triggers many smaller urb_enqueue calls in the opposite to the 
> PL2303 which only has few but huge calls.
>
> Additional notes:
> After closing the serial connection to the FTDI the system is usable as 
> before. In case of PL2303 i need to disconnect the converter in order to get 
> a usable system.
>
> Why do they behave so differently?
> Are these results of a overload?
> Doug, can you point me to your timing patch?

I did in the previous message, but basically take a look at:

https:

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Hi,

On Tue, Apr 18, 2017 at 1:07 AM, Stefan Wahren  wrote:
>> 1. With dwc2 you often end up in the situation where you need to
>> service an interrupt every 125 uS.  If servicing that interrupt takes
>> anywhere near 125 uS in the common case then you'll be in trouble.
>
> I will try to measure this with a logic analyzer.

Why a logic analyzer?  I'd think that "ftrace" would be your friend
here.  If you configure it just right you ought to be able to figure
out exactly what your kernel is doing.

...or, if you don't want to learn ftrace (it's cool and worth it, but
there's some time) you can just use "trace_printk" to get a fairly
low-over head printout to a memory buffer.  You can put that printout
in various places in the code and figure out what's taking so long.
In fact, that's exactly how the patch at

works.

-Doug
--
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

Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Doug Anderson]

Hi,

On Mon, Apr 17, 2017 at 4:05 AM, Stefan Wahren  wrote:
> Hi,
>
>> Stefan Wahren  hat am 31. Oktober 2016 um 21:34 
>> geschrieben:
>>
>>
>> I inspired by this issue [1] i build up a slightly modified setup with a
>> Raspberry Pi B (mainline kernel 4.9rc3), a powered 7 port USB hub and 5 
>> Prolific
>> PL2303 USB to serial convertors. I modified the usb_test for dwc2 [2], which
>> only tries to open all ttyUSB devices one after the other.
>>
>> Unfortunately the complete system stuck after opening the first ttyUSB 
>> device (
>> heartbeat LED stop blinking, no reaction to debug UART). The only way to
>> reanimate the system is to powerdown the USB hub with the USB to serial
>> convertors.
>>
>> [1] - https://github.com/raspberrypi/linux/issues/1692
>> [2] - https://gist.github.com/lategoodbye/dd0d30af27b6f101b03d5923b279dbaa
>
> since this issue still exists with 4.11 (even without or with microframe 
> scheduler enabled), i want to ask some additional questions:
>
> Is this issue reproducible with other dwc2 platforms than bcm2835?

+Edmund Szeto, who I seem to remember emailing me about similar
questions in the past.


> Does the soft lockup also occurs after opening the second serial convertor or 
> later?

I don't have serial converters easily available to me, but back in the
day when I was stressing things out on rk3288 I never saw anything
this bad.  ...of course, on rk3288 we've got 4 A17 cores running
really fast, so possibly just being slower is what causes your
problems here?

I will make the following observations:

1. With dwc2 you often end up in the situation where you need to
service an interrupt every 125 uS.  If servicing that interrupt takes
anywhere near 125 uS in the common case then you'll be in trouble.

===

2. When I was testing on rk3288 (on kernel 3.14) I did see occasions
where uvc_video_complete() could sometimes take > 125 uS.  It's been a
long time now, but if I remember correctly this had to do with the
fact that the URB buffers were allocated in a way where you had to
access them non-cached and this was super duper slow.  In my
particular case I could "fix" it by adjusting UVC_MAX_PACKETS
(crosreview.com/321932).  ...and I had some timing code in
crosreview.com/321980.

Again, it was a long time ago, but elsewhere I have written down:

-

Specifically:
* The USB "complete" functions are called with local interrupts
disabled.  Specifically see __usb_hcd_giveback_urb().
* I see calls to uvc_video_complete() that easily take > 125us.

Unfortunately the interrupts disabled while uvc_video_complete() is
called are always the interrupts for the same CPU that's dealing with
the normal dwc2 USB interrupts.

--

Ugh.  This may be the memcpy() as others have found:

http://www.spinics.net/lists/linux-usb/msg83581.html

...looks like the issue is that the driver is allocating memory that's
supposed to be DMA coherent and copying from this memory is slow.

-

You could probably pick my timing patch and then see if you're
actually hitting cases like this, I guess?

===

3. Are you running CPUFreq by chance?

...back in the day we had a bug on rk3288 where we were temporarily
running the CPU as slow as 8 MHz for a short while during a CPUFreq
transition.  If you happened to get a dwc2 interrupt while at this
speed then you were in trouble.


-Doug
--
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

Re: [PATCH v3] usb: hub: Fix error loop seen after hub communication errors

[Doug Anderson]

Hi,

On Thu, Mar 16, 2017 at 12:24 PM, Guenter Roeck  wrote:
> @@ -1198,7 +1201,7 @@ static void hub_activate(struct usb_hub *hub, enum 
> hub_activation_type type)
>
> /* Scan all ports that need attention */
> kick_hub_wq(hub);
> -
> +abort:

One tiny nit that could be done when applying this patch is to add a
space before "abort".  Other goto labels in this function are preceded
by a space and it's sane to try to match the existing coding
convention in the function rather than trying to mix and match.

Other than that this patch seems sane to me, but I am by no means an
expert on this code.  ;)


-Doug
--
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

Re: [PATCH v8 3/3] arm64: dts: rockchip: add usb2-phy support for rk3399

[Doug Anderson]

Hi,

On Tue, Jul 19, 2016 at 12:28 AM, Frank Wang  wrote:

You need a patch description here, even for simple patches.  All you
have now is a subject.


> Signed-off-by: Frank Wang 
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-evb.dts |   19 +++
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi|   47 
> ++-

Personally I'd prefer to see EVB in a separate patch.


>  2 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-evb.dts 
> b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> index 1a3eb14..31d4828 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> @@ -69,6 +69,15 @@
> regulator-max-microvolt = <330>;
> };
>
> +   vbus_host: vbus-host-regulator {
> +   compatible = "regulator-fixed";
> +   enable-active-high;
> +   gpio = <&gpio4 25 GPIO_ACTIVE_HIGH>;
> +   pinctrl-names = "default";
> +   pinctrl-0 = <&host_vbus_drv>;
> +   regulator-name = "vbus_host";
> +   };
> +

To match my schematics, this would probably be "vcc5v0_host".
Technically there are two regulators but since they are the same
voltage and enabled by the same GPIO it seems like modeling it as one
regulator is fine.

If you really wanted to model things you could also include the input
supply (VCC5V0_SYS).  Not sure how much you care to model in EVB.


> vcc_phy: vcc-phy-regulator {
> compatible = "regulator-fixed";
> regulator-name = "vcc_phy";
> @@ -93,6 +102,16 @@
> status = "okay";
>  };
>
> +&u2phy0_host {
> +   phy-supply = <&vbus_host>;
> +   status = "okay";
> +};
> +
> +&u2phy1_host {
> +   phy-supply = <&vbus_host>;
> +   status = "okay";
> +};
> +

Technically "u2" sorts alphabetically before "uart".


>  &usb_host0_ehci {
> status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index d7f8e06..0383785 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -221,6 +221,8 @@
> interrupts = ;
> clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
> clock-names = "hclk_host0", "hclk_host0_arb";
> +   phys = <&u2phy0_host>;
> +   phy-names = "u2phy0";

This is wrong.  From
"Documentation/devicetree/bindings/usb/usb-ehci.txt" phy-names should
be "usb".

> status = "disabled";
> };
>
> @@ -239,6 +241,8 @@
> interrupts = ;
> clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>;
> clock-names = "hclk_host1", "hclk_host1_arb";
> +   phys = <&u2phy1_host>;
> +   phy-names = "u2phy1";

This is wrong.  From
"Documentation/devicetree/bindings/usb/usb-ehci.txt" phy-names should
be "usb".

> status = "disabled";
> };
>
> @@ -481,8 +485,42 @@
> };
>
> grf: syscon@ff77 {
> -   compatible = "rockchip,rk3399-grf", "syscon";
> +   compatible = "rockchip,rk3399-grf", "syscon", "simple-mfd";
> reg = <0x0 0xff77 0x0 0x1>;
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +
> +   u2phy0: usb2-phy@e450 {
> +   compatible = "rockchip,rk3399-usb2phy";
> +   reg = <0xe450 0x10>;
> +   clocks = <&cru SCLK_USB2PHY0_REF>;
> +   clock-names = "phyclk";
> +   #clock-cells = <0>;
> +   clock-output-names = "clk_usbphy0_480m";

Any reason why there isn't a 'status = "disabled";' here?


> +   u2phy0_host: host-port {
> +   #phy-cells = <0>;
> +   interrupts = ;
> +   interrupt-names = "linestate";
> +   status = "disabled";
> +   };
> +   };
> +
> +   u2phy1: usb2-phy@e460 {
> +   compatible = "rockchip,rk3399-usb2phy";
> +   reg = <0xe460 0x10>;
> +   clocks = <&cru SCLK_USB2PHY1_REF>;
> +   clock-names = "phyclk";
> +   #clock-cells = <0>;
> +   clock-output-names = "clk_usbphy1_480m";
> +
> +   u2phy1_host: host-port {
> +   #phy-cells = <0>;
> +   interrupts = ;
> +   interrupt-names = "linestate";
> +   status = "disabled";
> +   };
> +   };
> };
>
> watchdog@ff84 {
> @@ -1009,5 +1047,12 @@
> <1 14 RK_FUNC

Re: [PATCH 1/4] usb: dwc3: of-simple: add compatible for rockchip

[Doug Anderson]

William,

On Mon, May 9, 2016 at 4:46 AM, William Wu  wrote:
> Signed-off-by: William Wu 
> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
> b/drivers/usb/dwc3/dwc3-of-simple.c
> index 9743353..1f3665b 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -162,6 +162,7 @@ static const struct dev_pm_ops dwc3_of_simple_dev_pm_ops 
> = {
>  static const struct of_device_id of_dwc3_simple_match[] = {
> { .compatible = "qcom,dwc3" },
> { .compatible = "xlnx,zynqmp-dwc3" },
> +   { .compatible = "rockchip,dwc3" },

It is, of course, up to Felipe.  ...but personally I'd prefer that
things here be sorted alphabetically.  Sorting things in a consistent
manner tends to reduce merge conflicts as the list gets longer and
also makes it easier to find things.

-Doug
--
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

Re: [RFT PATCH 2/2] Revert "usb: dwc2: Fix probe problem on bcm2835"

[Doug Anderson]

John,

On Tue, Mar 22, 2016 at 12:26 PM, John Youn  wrote:
> Thanks for the debug logs and everyones help.
>
> After reviewing with our hardware engineers, it seems this is likely
> to do with the IDDIG debounce filtering when switching between
> modes. You can see if this is enabled in your core by checking
> GHWCFG4.IddgFltr.
>
> From the databook:
>
> OTG_EN_IDDIG_FILTER
>
> Specifies whether to add a filter on the "iddig" input from the
> PHY. If your PHY already has a filter on iddig for de-bounce, then
> it is not necessary to enable the one in the DWC_otg.  The filter
> is implemented in the DWC_otg_wpc module as a 20-bit counter that
> works on the PHY clock. In the case of the UTMI+ PHY, this pin is
> from the PHY. In the case of the ULPI PHY, this signal is
> generated by the ULPI Wrapper inside the core.
>
>
> This only affects OTG cores and the delay is 10ms for 30MHz PHY clock
> and 50ms with a 6MHz PHY clock.

Wow, nice to have it so perfectly explained!  :)


> The reason we see this after a reset is that by default the IDDIG
> indicates device mode. But if the id pin is set to host the core will
> immediately detect it after the reset and trigger the filtering delay
> before changing to host mode.
>
> This delay is also triggered by force mode. This is the origin of the
> 25 ms delay specified in the databook. The databook did not take into
> account that there might be a 6MHz clock so this delay could actually
> be up to 50 ms. So we aren't delaying enough time for those cases.
>
> I think this explains all the problems and platform differences we are
> seeing related to this.
>
> I think we can just add an IDDIG delay after a force mode, a clear
> force mode, and any time after reset where we don't do either a force
> or clear force mode immediately afterwards. Maybe set the delay time
> via a core parameter or measure it once on probe. Or we can probably
> just poll for the mode change in all of the above conditions.

The driver seems to be able to figure out the PHY clock in most cases
in dwc2_calc_frame_interval().  It doesn't seem to handle 6MHz there,
though.  I wonder if that's another bug?

Polling seems fine too, though.


> Are you able to continue looking into this? If not, I can take it up.

I'm pretty much out of time at this point and it sounds like you've
though through all of the corner cases.  If you can take it up that
would be wonderful!  :)  I'm happy to give the patches a test, though!
 :)


-Doug
--
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

Re: [RFT PATCH 2/2] Revert "usb: dwc2: Fix probe problem on bcm2835"

[Doug Anderson]

Hi,

On Wed, Mar 16, 2016 at 11:28 AM, John Youn  wrote:
> On 3/10/2016 11:14 AM, John Youn wrote:
>> On 3/9/2016 11:06 AM, Doug Anderson wrote:
>>> Stefan,
>>>
>>> On Wed, Mar 9, 2016 at 11:01 AM, Stefan Wahren  
>>> wrote:
>>>>
>>>>> Doug Anderson  hat am 7. März 2016 um 22:30
>>>>> geschrieben:
>>>>>
>>>>>
>>>>> Stefan,
>>>>>
>>>>> On Mon, Mar 7, 2016 at 10:40 AM, Stefan Wahren  
>>>>> wrote:
>>>>>> Hi Doug,
>>>>>>
>>>>>>> Douglas Anderson  hat am 4. März 2016 um 19:23
>>>>>>> geschrieben:
>>>>>>>
>>>>>>>
>>>>>>> This reverts commit 192cb07f7928 ("usb: dwc2: Fix probe problem on
>>>>>>> bcm2835") now that we've found the root cause. See the change
>>>>>>> titled ("usb: dwc2: Add a 10 ms delay to dwc2_core_reset()").
>>>>>>
>>>>>> adding a delay of 10 ms after a core reset might be a idea, but applying
>>>>>> both
>>>>>> patches breaks USB support on RPi :-(
>>>>>>
>>>>>> I'm getting the wrong register values ...
>>>>>
>>>>> Ugh. :(
>>>>>
>>>>> Just out of curiosity, if you loop and time long it takes for the
>>>>> registers to get to the right state after reset, what do you get?
>>>>> AKA, pick:
>>>>>
>>>>> https://chromium-review.googlesource.com/331260
>>>>>
>>>>> ...and let me know what it prints out.
>>>>
>>>> On my Raspberry Pi B i get the following:
>>>>
>>>> [2.084411] dwc2 2098.usb: mapped PA 2098 to VA cc88
>>>> [2.084461] dwc2 2098.usb: cannot get otg clock
>>>> [2.084549] dwc2 2098.usb: registering common handler for irq33
>>>> [2.084713] dwc2 2098.usb: Configuration mismatch. dr_mode forced 
>>>> to host
>>>> [2.153965] dwc2 2098.usb: Waited 49996 us, 0x00201000 => 
>>>> 0x01001000,
>>>> 0x => 0x02002000
>>>> [2.174930] dwc2 2098.usb: Forcing mode to host
>>>>
>>>> So i changed the delay in patch #1 to msleep(50) and then both patches 
>>>> work like
>>>> a charm.
>>>
>>> Great news!  :-)
>>>
>>> John: it's pretty clear that there's something taking almost exactly
>>> 10ms on my system and almost exactly 50ms on Stefan's system.  Is
>>> there some register we could poll to see when this process is done?
>>> ...or can we look at the dwc2 revision number / feature register and
>>> detect how long to delay?
>>>
>>
>> Hi Doug,
>>
>> I'll have to ask around to see if anyone knows about this. And I'll
>> run some tests on the platforms I have available to me as well.
>>
>
> There's still nothing definitive on our end as to why this is
> happening. Also I don't think there is any other way to poll the
> reset.

One thing I noticed is that the delay was only needed on my OTG port
(not my host-only port).  ...I also noticed that while waiting the
HPTXFSIZ was temporarily 0x while I was delaying.  That seems
to match Stephan.

I wonder if perhaps the delay has to do with how long it took to
detect that it needed to go into host mode?

Ah, interesting.  It looks as if "GOTGCTL" changes during this time
too.  To summarize:

HOST-only (ff54.usb): needs no delay:
  GNPTXFSIZ: 0x04000400
  HPTXFSIZ: 0x02000800
  GOTGCTL: 0x002d

OTG (ff58): needs 10ms delay.  Before delay:
  GNPTXFSIZ: 0x00100400
  HPTXFSIZ: 0x
  GOTGCTL: 0x0001
After delay:
  GNPTXFSIZ: 0x04000400
  HPTXFSIZ: 0x02000800
  GOTGCTL: 0x002c

Could we loop until either BSesVld or ASesVld is set?  Don't know much
about OTG, but seems like that might be something sane?

> Our hardware engineers asked for some more information to look
> into it further. Doug, Stefan, Caesar, and anyone else with a related
> platform, do you know the answers to the following:
>
> 1. What is the AHB Clock frequency? Is the AHB Clock gated during
> Reset?

According to commit 20bde643434d ("usb: dwc2: reduce dwc2 driver probe
time"), our AHB clock is 150MHz.  I'm nearly certain it is not gated.


> 2. Also is the PHY clock stopped during the reset or is the PHY PLL
> lock times high in the order of ms?

I don't th

Re: [RFT PATCH 2/2] Revert "usb: dwc2: Fix probe problem on bcm2835"

[Doug Anderson]

Stefan,

On Wed, Mar 9, 2016 at 11:01 AM, Stefan Wahren  wrote:
>
>> Doug Anderson  hat am 7. März 2016 um 22:30
>> geschrieben:
>>
>>
>> Stefan,
>>
>> On Mon, Mar 7, 2016 at 10:40 AM, Stefan Wahren  
>> wrote:
>> > Hi Doug,
>> >
>> >> Douglas Anderson  hat am 4. März 2016 um 19:23
>> >> geschrieben:
>> >>
>> >>
>> >> This reverts commit 192cb07f7928 ("usb: dwc2: Fix probe problem on
>> >> bcm2835") now that we've found the root cause. See the change
>> >> titled ("usb: dwc2: Add a 10 ms delay to dwc2_core_reset()").
>> >
>> > adding a delay of 10 ms after a core reset might be a idea, but applying
>> > both
>> > patches breaks USB support on RPi :-(
>> >
>> > I'm getting the wrong register values ...
>>
>> Ugh. :(
>>
>> Just out of curiosity, if you loop and time long it takes for the
>> registers to get to the right state after reset, what do you get?
>> AKA, pick:
>>
>> https://chromium-review.googlesource.com/331260
>>
>> ...and let me know what it prints out.
>
> On my Raspberry Pi B i get the following:
>
> [2.084411] dwc2 2098.usb: mapped PA 2098 to VA cc88
> [2.084461] dwc2 2098.usb: cannot get otg clock
> [2.084549] dwc2 2098.usb: registering common handler for irq33
> [2.084713] dwc2 2098.usb: Configuration mismatch. dr_mode forced to 
> host
> [2.153965] dwc2 2098.usb: Waited 49996 us, 0x00201000 => 0x01001000,
> 0x => 0x02002000
> [2.174930] dwc2 2098.usb: Forcing mode to host
>
> So i changed the delay in patch #1 to msleep(50) and then both patches work 
> like
> a charm.

Great news!  :-)

John: it's pretty clear that there's something taking almost exactly
10ms on my system and almost exactly 50ms on Stefan's system.  Is
there some register we could poll to see when this process is done?
...or can we look at the dwc2 revision number / feature register and
detect how long to delay?
--
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

Re: [RFT PATCH 2/2] Revert "usb: dwc2: Fix probe problem on bcm2835"

[Doug Anderson]

Stefan,

On Mon, Mar 7, 2016 at 10:40 AM, Stefan Wahren  wrote:
> Hi Doug,
>
>> Douglas Anderson  hat am 4. März 2016 um 19:23
>> geschrieben:
>>
>>
>> This reverts commit 192cb07f7928 ("usb: dwc2: Fix probe problem on
>> bcm2835") now that we've found the root cause. See the change
>> titled ("usb: dwc2: Add a 10 ms delay to dwc2_core_reset()").
>
> adding a delay of 10 ms after a core reset might be a idea, but applying both
> patches breaks USB support on RPi :-(
>
> I'm getting the wrong register values ...

Ugh.  :(

Just out of curiosity, if you loop and time long it takes for the
registers to get to the right state after reset, what do you get?
AKA, pick:

https://chromium-review.googlesource.com/331260

...and let me know what it prints out.  On my system I see:

[1.990743] dwc2 ff54.usb: Waited 31 us, 0x04000400 =>
0x04000400, 0x02000800 => 0x02000800
[2.119677] dwc2 ff58.usb: Waited 9997 us, 0x00100400 =>
0x04000400, 0x => 0x02000800

I believe the difference in behavior is because of the two different
types of USB controllers (one is OTG and the other is host only).


-Doug
--
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

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Sat, Mar 5, 2016 at 12:41 PM, Michael Niewoehner
 wrote:
> Hi Douglas,
> Hi John,
>
> Am 05.03.2016 um 01:33 schrieb Doug Anderson :
>
>> Michael,
>>
>> On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  
>> wrote:
>>>>> From testing and trying to make sense of the documentation, it appears
>>>>> that a 10 ms delay is needed after resetting the core to make sure that
>>>>> everything is stable and consistent.  Let's add it.
>>>>>
>>>>> In my testing (on rk3288) this allows us to revert commit
>>>>> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
>>>>> could never reproduce the problems on my board, this might also allow us
>>>>> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
>>>>> dr_mode").
>>>>>
>>>>> Signed-off-by: Douglas Anderson 
>>>>
>>>> Tested-by: Michael Niewoehner 
>>
>> Thanks!  That's great news!
>>
>>
>>>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>>>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>>>> not been applied anyways ...
>>>> However, I tested you your two patches with „magically reverted“ 
>>>> bd84f4ae9986 (msleep 50) on rk3188.
>>>> The sdcard keeps being detected and boots just fine.
>>> I meant usb stick of course… too much sdcards in my head today \o/.
>>
>> Odd.  It looks to be there for me...
>>
>> $ git checkout 62718e304aa6
>> HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
>>
>> $ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
>> drivers/usb/dwc2/core.c-}
>> drivers/usb/dwc2/core.c-
>> drivers/usb/dwc2/core.c-/*
>> drivers/usb/dwc2/core.c: * NOTE: This is required for some
>> rockchip soc based
>> drivers/usb/dwc2/core.c- * platforms.
>> drivers/usb/dwc2/core.c- */
>> drivers/usb/dwc2/core.c-msleep(50);
>
> I unfortunately have bad news.
> After some more testing it turned out that usb does NOT work (always) on 
> rk3188 when reverting bd84f4ae9986.
> It looks like that is dependent on which device / vendor is plugged in.
> The usb stick I tested yesterday worked once but today just blinks shortly 
> and then stops working.
> Another usb stick I tested today doesn’t blink or work at all. Maybe I should 
> have tested booting some more times :-(

Just to clarify based on IRC conversation on #linux-rockchip:

* My two patches work fine as per Michael (c0d3z3r0) and another person (mrjay).

* My two patches _don't_ also allow us to revert to "50 ms" commit
bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing dr_mode").
This is Michael's "bad news".

That means we could apply my two patches and the continue to work
separately to figure out how to revert commit bd84f4ae9986 ("usb:
dwc2: Add extra delay when forcing dr_mode").

Thanks!

-Doug
--
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

Re: Odd merge 3b30be3b6487 ("Merge tag 'v4.5-rc6' into next")

[Doug Anderson]

Hi,

On Fri, Mar 4, 2016 at 5:56 PM, John Youn  wrote:
> On 3/4/2016 5:04 PM, Doug Anderson wrote:
>> Felipe,
>>
>> Michael pointed out that there's an odd merge that happened.
>> Specifically it looks like commit bd84f4ae9986 ("usb: dwc2: Add extra
>> delay when forcing dr_mode") got lost somehow in the merge commit
>> 3b30be3b6487 ("Merge tag 'v4.5-rc6' into next")
>>
>> Specifically:
>>
>> $ git blame 3b30be3b6487 -- drivers/usb/dwc2/core.c | grep "This is required"
>> $ git blame 3b30be3b6487^2 -- drivers/usb/dwc2/core.c | grep "This is 
>> required"
>> bd84f4ae9986a drivers/usb/dwc2/core.c (John Youn
>> 2016-02-15 15:30:20 -0800  624)   * NOTE: This is required for some
>> rockchip soc based
>>
>>
>> I think I'm about at the limit of my git-fu, so I'm a bit baffled.
>> Any idea what happened?  Was anything else lost?
>>
>
>
> Felipe recently rebased his next branch without the merge commit
> referenced above. So maybe that's causing some issues.
>
> But everything seems ok to me. I see the commit and correct code on
> 4.5-rc6, Felipe's next branch, and Greg's usb-next.

They are missing from from next-20160304 and next-20160303 (though in
next-20160302) as found by Michael.

-Doug
--
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

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Fri, Mar 4, 2016 at 4:33 PM, Doug Anderson  wrote:
> Michael,
>
> On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  
> wrote:
>>>> From testing and trying to make sense of the documentation, it appears
>>>> that a 10 ms delay is needed after resetting the core to make sure that
>>>> everything is stable and consistent.  Let's add it.
>>>>
>>>> In my testing (on rk3288) this allows us to revert commit
>>>> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
>>>> could never reproduce the problems on my board, this might also allow us
>>>> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
>>>> dr_mode").
>>>>
>>>> Signed-off-by: Douglas Anderson 
>>>
>>> Tested-by: Michael Niewoehner 
>
> Thanks!  That's great news!
>
>
>>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>>> not been applied anyways ...
>>> However, I tested you your two patches with „magically reverted“ 
>>> bd84f4ae9986 (msleep 50) on rk3188.
>>> The sdcard keeps being detected and boots just fine.
>> I meant usb stick of course… too much sdcards in my head today \o/.
>
> Odd.  It looks to be there for me...
>
> $ git checkout 62718e304aa6
> HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
>
> $ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
> drivers/usb/dwc2/core.c-}
> drivers/usb/dwc2/core.c-
> drivers/usb/dwc2/core.c-/*
> drivers/usb/dwc2/core.c: * NOTE: This is required for some
> rockchip soc based
> drivers/usb/dwc2/core.c- * platforms.
> drivers/usb/dwc2/core.c- */
> drivers/usb/dwc2/core.c-msleep(50);

For anyone playing along at home, please see
<http://article.gmane.org/gmane.linux.usb.general/138355>.

-Doug
--
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

Odd merge 3b30be3b6487 ("Merge tag 'v4.5-rc6' into next")

[Doug Anderson]

Felipe,

Michael pointed out that there's an odd merge that happened.
Specifically it looks like commit bd84f4ae9986 ("usb: dwc2: Add extra
delay when forcing dr_mode") got lost somehow in the merge commit
3b30be3b6487 ("Merge tag 'v4.5-rc6' into next")

Specifically:

$ git blame 3b30be3b6487 -- drivers/usb/dwc2/core.c | grep "This is required"
$ git blame 3b30be3b6487^2 -- drivers/usb/dwc2/core.c | grep "This is required"
bd84f4ae9986a drivers/usb/dwc2/core.c (John Youn
2016-02-15 15:30:20 -0800  624)   * NOTE: This is required for some
rockchip soc based


I think I'm about at the limit of my git-fu, so I'm a bit baffled.
Any idea what happened?  Was anything else lost?

-Doug
--
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

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Fri, Mar 4, 2016 at 3:46 PM, Karl Palsson  wrote:
>> + /*
>> +  * Sleep for 10-15 ms after the reset to let it finish.
>> +  *
>> +  * It's been confirmed on at least one version of the controller
>> +  * that this is a requirement that this is a requirement in order for
>
> ^^ duplicate wording here.

Thanks for catching.  I'm happy to re-post with fixed wording or have
a maintainer adjust it to this if/when it is applied:

* It's been confirmed on at least one version of the
* controller that this is a requirement in order for
* everything to settle.  Specifically if you:

Please let me know if you'd like it re-posted.


-Doug
--
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

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Michael,

On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  wrote:
>>> From testing and trying to make sense of the documentation, it appears
>>> that a 10 ms delay is needed after resetting the core to make sure that
>>> everything is stable and consistent.  Let's add it.
>>>
>>> In my testing (on rk3288) this allows us to revert commit
>>> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
>>> could never reproduce the problems on my board, this might also allow us
>>> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
>>> dr_mode").
>>>
>>> Signed-off-by: Douglas Anderson 
>>
>> Tested-by: Michael Niewoehner 

Thanks!  That's great news!


>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>> not been applied anyways ...
>> However, I tested you your two patches with „magically reverted“ 
>> bd84f4ae9986 (msleep 50) on rk3188.
>> The sdcard keeps being detected and boots just fine.
> I meant usb stick of course… too much sdcards in my head today \o/.

Odd.  It looks to be there for me...

$ git checkout 62718e304aa6
HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb

$ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
drivers/usb/dwc2/core.c-}
drivers/usb/dwc2/core.c-
drivers/usb/dwc2/core.c-/*
drivers/usb/dwc2/core.c: * NOTE: This is required for some
rockchip soc based
drivers/usb/dwc2/core.c- * platforms.
drivers/usb/dwc2/core.c- */
drivers/usb/dwc2/core.c-msleep(50);
--
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

Re: [PATCH] usb: move usb_calc_bus_time into common code

[Doug Anderson]

John,

On Fri, Feb 19, 2016 at 6:55 PM, John Youn  wrote:
> On 2/19/2016 2:48 PM, Doug Anderson wrote:
>> Hi,
>>
>> On Fri, Feb 19, 2016 at 1:52 PM, Alan Stern  
>> wrote:
>>> On Fri, 19 Feb 2016, Arnd Bergmann wrote:
>>>
>>>> The dwc2 dual-role USB controller driver has started calling
>>>> usb_calc_bus_time, and does so regardless of whether it is
>>>> being built as a host controller or gadget. In case all
>>>
>>> Out of curiosity...  Why does dwc2 need to calculate bus times when it
>>> is in device mode?
>>
>> Hoo boy.  it doesn't.  Nor does it need to properly set the even/odd
>> frame in device mode.
>>
>> Basically dwc2's "core.c" has a whole bunch of stuff in it that's host
>> only and isn't guarded with any #ifdef.  ...yet that file is included
>> in gadget-only builds.  It's a bit of a mess.  Take a gander at all of
>> the "dwc2_hc_xxx" functions sitting in "drivers/usb/dwc2/core.c".
>>
>> Long term that needs to be cleaned up, but such a cleanup is going to
>> be a bit of churn so we'd need to schedule it for a time when not much
>> else is going on (and presumably it should be done by John or in close
>> coordination with him so it can get Acked / landed quickly to avoid
>> conflicts?).  To do this we'd have to figure out why some things are
>> in "core.c" and some in "hcd.c" and if it makes sense to move them all
>> to "hcd.c" or if we need a new "core_hc.c" or something...  All of
>> that design predates me.
>>
>
> Yeah, that stuff has been bugging me for a while. I have a branch that
> started a lot of clean-ups, but it's never been a great time to merge
> it.
>
>> In any case I guess we need a solution for right now, huh?
>>
>> One terribly lame hack would be to just make a dummy no-op
>> "dwc2_hc_set_even_odd_frame()" if host mode is disabled.  That doesn't
>> really fix the root problem of lots of host mode code being compiled
>> in a gadget-only driver.  It also adds an ugly "#ifdef".  ...but it
>> does get around the current compile error.
>>
>>
>> What do folks think?
>>
>
> I think if we can fix it for -next in dwc2 by moving all host code out
> of core then we should do it that way. I'll see if I can revive my
> patches.

It looks like Felipe has dropped my series out of his testing/next to
avoid this issue (thanks Felipe!).  I'm interested in getting this
resolved as soon as possible to get things back in.  Is there anything
you'd like me to do to help?

-Doug
--
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

Re: [PATCH] usb: move usb_calc_bus_time into common code

[Doug Anderson]

Hi,

On Fri, Feb 19, 2016 at 1:52 PM, Alan Stern  wrote:
> On Fri, 19 Feb 2016, Arnd Bergmann wrote:
>
>> The dwc2 dual-role USB controller driver has started calling
>> usb_calc_bus_time, and does so regardless of whether it is
>> being built as a host controller or gadget. In case all
>
> Out of curiosity...  Why does dwc2 need to calculate bus times when it
> is in device mode?

Hoo boy.  it doesn't.  Nor does it need to properly set the even/odd
frame in device mode.

Basically dwc2's "core.c" has a whole bunch of stuff in it that's host
only and isn't guarded with any #ifdef.  ...yet that file is included
in gadget-only builds.  It's a bit of a mess.  Take a gander at all of
the "dwc2_hc_xxx" functions sitting in "drivers/usb/dwc2/core.c".

Long term that needs to be cleaned up, but such a cleanup is going to
be a bit of churn so we'd need to schedule it for a time when not much
else is going on (and presumably it should be done by John or in close
coordination with him so it can get Acked / landed quickly to avoid
conflicts?).  To do this we'd have to figure out why some things are
in "core.c" and some in "hcd.c" and if it makes sense to move them all
to "hcd.c" or if we need a new "core_hc.c" or something...  All of
that design predates me.

In any case I guess we need a solution for right now, huh?

One terribly lame hack would be to just make a dummy no-op
"dwc2_hc_set_even_odd_frame()" if host mode is disabled.  That doesn't
really fix the root problem of lots of host mode code being compiled
in a gadget-only driver.  It also adds an ugly "#ifdef".  ...but it
does get around the current compile error.


What do folks think?

-Doug
--
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

Re: [PATCH v6 17/22] usb: dwc2: host: Manage frame nums better in scheduler

[Doug Anderson]

Hi,

On Thu, Jan 28, 2016 at 6:20 PM, Douglas Anderson  wrote:
>  static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
>  struct dwc2_hcd_urb *urb)
>  {
> @@ -569,11 +655,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);

In reviewing patches I realized that this is actually a revert of
commit dd81dd7c8178 ("usb: dwc2: host: use correct frame number during
qh init").  IMHO that patch was wrong: hsotg->frame_number is supposed
to be the frame number as of the last start of frame.  If we need to
know a more recent frame number then we should query it ourselves.

Presumably the reason for the original patch was to try to fix some of
the same problems I've addressed in my series, so I'd presume that
this doesn't add any new regressions.  I haven't heard much from
Gregory Herrero about my series, but it would be nice to confirm that
this virtual revert wasn't causing problems.

-Doug
--
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

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

[Doug Anderson]

John,

On Tue, Feb 2, 2016 at 3:57 PM, John Youn  wrote:
> On 1/28/2016 6:20 PM, Douglas Anderson wrote:
>> 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).
>>
>
> Hi Doug,
>
> I've yet to review this, but just wanted to let you know that we've
> started on it and also testing. We'll get back to you with some
> feedback and results soon.
>
> We had also been looking at some of these same and related issues so
> we want to make sure everything we've done is compatible with your
> changes and is still working ok too.

Great, thanks for the reply.  It's very helpful to know that you're
looking at it even if you haven't had time to do a full review yet.

Note: if any of my patches are wrong or redundant to patches that
you've developed and tested, I'm happy to take your patches instead.
;)

Note that patches 1 - 11 have already landed in our tree and thus are
getting additional exposure and testing.  IMHO those are all ready for
prime time.  Assuming there are no huge issues, it would be handy if
those 11 patches landed as-is (or almost as-is), but that's just me
being selfish so I don't need to revert / reland new versions.  :-P


Also note that the period of time where I can devote this much time to
dwc2 is coming soon to an end since I have to go on to work on other
things.  If there are major issues or easy fixes I will certainly be
able to help with those things (I'll still be around), but I won't be
able to devote days to tracking down weird problems or testing
rewrites.  ;-)


-Doug
--
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

Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time

[Doug Anderson]

Kever,

On Mon, Feb 1, 2016 at 11:04 PM, Kever Yang  wrote:
>>> Oh, now I get what you're saying!
>>>
>>> A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() ->
>>> dwc2_hcd_qh_deactivate()
>>> ...and always in that case we'll do a select / queue, so we don't need it
>>> there.
>>>
>>> B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate()
>>>
>>> ...but why don't we need it for dwc2_hcd_urb_dequeue()?  Yes, you're
>>> not continuing a split so timing isn't quite as urgent, but you still
>>> might have an INT or ISOC packet that's scheduled with an interval of
>>> 1.  We still might want to schedule right away if there are remaining
>>> QTDs, right?
>>
>> I ran out of time to fully test today, but I couldn't actually get a
>> case where we needed to schedule right away for B).  ...so given your
>> point about the the select / queue already present in case A, we could
>> probably just drop this patch ("usb: dwc2: host: Schedule periodic
>> right away if it's time") and if we can find a case where it's needed
>> in case B we can add the select / queue there.
>>
>> Sound OK?  I'll try to do more testing tomorrow...
>
> Yes, we don't get a case we need to schedule right away for case B).
>
> For INT or ISOC packet, I can recall I have seen somewhere but I can find
> it now, the synchronous transfer is happen in the next uframe instead of the
> uframe
> when the host channel initialized, so there is no difference of setting the
> host channel register sooner or later inside the same frame.
> Which means the existent code should be OK for case A).
>
> We can drop this patch before we have the exact use case.

I put in some printouts and I finally did manage to find a place where
we needed to queue things up in dwc2_hcd_urb_dequeue().  I saw:

314.587916: QH=d9535340 next(0) fn=2a52, sch=2a51=>2a52 (+1) miss=0
314.588040: QH=d9535340 next(0) fn=2a53, sch=2a52=>2a53 (+1) miss=0
314.588162: QH=d9535340 next(0) fn=2a54, sch=2a53=>2a54 (+1) miss=0
314.588299: QH=d9535340 next(0) fn=2a55, sch=2a54=>2a55 (+1) miss=0
314.588304: QH=d9535340 queue in dwc2_hcd_urb_dequeue
314.588363: QH=d9535340 next(0) fn=2a55, sch=2a55=>2a56 (+1) miss=0
314.588413: dwc2_handle_hcd_intr: ff54.usb: SCH: QH=e5cea380 ready
fn=2a56, nxt=2a56
314.588414: dwc2_handle_hcd_intr: ff54.usb: SCH: QH=e73ccc40 ready
fn=2a56, nxt=2a56
314.588415: dwc2_handle_hcd_intr: ff54.usb: SCH: QH=e5cea8c0 ready
fn=2a56, nxt=2a56

It's not something that's terribly common.  It's fine to just drop
this patch, or I can replace it with
.

-Doug
--
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

Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame

[Doug Anderson]

Kever,

On Mon, Feb 1, 2016 at 11:46 PM, Kever Yang  wrote:
> Doug,
>
>
> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>
>> When setting up ISO and INT transfers dwc2 needs to specify whether the
>> transfer is for an even or an odd frame (or microframe if the controller
>> is running in high speed mode).
>>
>> The controller appears to use this as a simple way to figure out if a
>> transfer should happen right away (in the current microframe) or should
>> happen at the start of the next microframe.  Said another way:
>>
>> - If you set "odd" and the current frame number is odd it appears that
>>the controller will try to transfer right away.  Same thing if you set
>>"even" and the current frame number is even.
>> - If the oddness you set and the oddness of the frame number are
>>_different_, the transfer will be delayed until the frame number
>>changes.
>>
>> As I understand it, the above technique allows you to plan ahead of time
>> where possible by always working on the next frame.  ...but it still
>> allows you to properly respond immediately to things that happened in
>> the previous frame.
>>
>> The old dwc2_hc_set_even_odd_frame() didn't really handle this concept.
>> It always looked at the frame number and setup the transfer to happen in
>> the next frame.  In some cases that meant that certain transactions
>> would be transferred in the wrong frame.
>>
>> We'll try our best to set the even / odd to do the transfer in the
>> scheduled frame.  If that fails then we'll do an ugly "schedule ASAP".
>> We'll also modify the scheduler code to handle this and not try to
>> schedule a second transfer for the same frame.
>>
>> Note that this change relies on the work to redo the microframe
>> scheduler.  It can work atop ("usb: dwc2: host: Manage frame nums better
>> in scheduler") but it works even better after ("usb: dwc2: host: Totally
>> redo the microframe scheduler").
>>
>> With this change my stressful USB test (USB webcam + USB audio +
>> keyboards) has less audio crackling than before.
>
> Seems this really help for your case?

Yes, I believe it does.  Of course my test case is pretty "black box"
for the most part in that I play music on youtube while having a
webcam open and several USB input devices connected.  I then try to
decide whether I hear more static or less static.  ...clearly a less
subjective test would be better...

* I tried with http://crosreview.com/325451 (see below) and I hear
more static with "use_old = true" than with "use_old = "false".

* I tried with this entire patch reverted and I hear about the same
static as with "use_old = true".

Note that counting reported MISS lines from my logging also shows that
the new code is better...


> Do you check if the transfer can happen right in the current frame? I know
> it's
> quite difficult to check it, but this changes what I know for the dwc core
> schedule the transaction.

Yes.  I just tried again, too.  I coded up
 and included it.  I
then opened up a USB webcam.

With things set to the old way:

  115.355370  QH=dc6ba8c0 next(0) fn=10cb, sch=10ca=>10cb (+1) miss=0
  115.355373  QH=dc6ba8c0 IMM ready fn=10cb, nxt=10cb
  115.355518  QH=dc6ba8c0 next(0) fn=10cc, sch=10cb=>10cc (+1) miss=0
  115.355522  QH=dc6ba8c0 IMM ready fn=10cc, nxt=10cc
  115.355637  QH=dc6ba8c0 next(0) fn=10cd, sch=10cc=>10cd (+1) miss=0
  115.355641  QH=dc6ba8c0 IMM ready fn=10cd, nxt=10cd
  115.355857  QH=dc6ba8c0 next(0) fn=10ce, sch=10cd=>10ce (+1) miss=0
  115.355859  QH=dc6ba8c0 IMM ready fn=10ce, nxt=10ce
  115.355867  QH=dc6ba8c0, wire=10cf, old_wire=10d0, EO diff (use OLD)
  115.355870  QH=dc6ba8c0 EO MISS w/ old (10ce != 10cf)
  115.356037  QH=dc6ba8c0 next(0) fn=10d0, sch=10cf=>10d0 (+1) miss=1 MISS
  115.356039  QH=dc6ba8c0 IMM ready fn=10d0, nxt=10d0
  115.356169  QH=dc6ba8c0 next(0) fn=10d1, sch=10d0=>10d1 (+1) miss=0
  115.356170  QH=dc6ba8c0 IMM ready fn=10d1, nxt=10d1
  115.356269  QH=dc6ba8c0 next(0) fn=10d2, sch=10d1=>10d2 (+1) miss=0
  115.356273  QH=dc6ba8c0 IMM ready fn=10d2, nxt=10d2
  115.356404  QH=dc6ba8c0 next(0) fn=10d3, sch=10d2=>10d3 (+1) miss=0
  115.356407  QH=dc6ba8c0 IMM ready fn=10d3, nxt=10d3

With the new way:

   87.814741  QH=e2fd7880 next(0) fn=32e4, sch=32e3=>32e4 (+1) miss=0
   87.814744  QH=e2fd7880 IMM ready fn=32e4, nxt=32e4
   87.814858  QH=e2fd7880 next(0) fn=32e5, sch=32e4=>32e5 (+1) miss=0
   87.814862  QH=e2fd7880 IMM ready fn=32e5, nxt=32e5
   87.815010  QH=e2fd7880 next(0) fn=32e6, sch=32e5=>32e6 (+1) miss=0
   87.815012  QH=e2fd7880 IMM ready fn=32e6, nxt=32e6
   87.815220  QH=e2fd7880 next(0) fn=32e8, sch=32e6=>32e7 (+1) miss=0
   87.815222  QH=e2fd7880 IMM ready fn=32e8, nxt=32e7
   87.815230  QH=e2fd7880, wire=32e8, old_wire=32e9, EO diff (use NEW)
   87.815278  QH=e2fd7880 next(0) fn=32e8, sch=32e7=>32e8 (+1) miss=0
   87.815280  QH=e2fd7880 IMM ready fn=32e8, nxt=32e8
   87.815390  QH=e2fd7880 next(0) fn=32e9, sch=32e8=>32e9 (+1) 

Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time

[Doug Anderson]

Kever,

On Sun, Jan 31, 2016 at 8:36 PM, Doug Anderson  wrote:
> Kever,
>
> On Sun, Jan 31, 2016 at 7:32 PM, Kever Yang  wrote:
>> Doug,
>>
>>
>> On 02/01/2016 06:09 AM, Doug Anderson wrote:
>>>
>>> Kever,
>>>
>>> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang 
>>> wrote:
>>>>
>>>> Doug,
>>>>
>>>>
>>>> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>>>>
>>>>> In dwc2_hcd_qh_deactivate() we will put some things on the
>>>>> periodic_sched_ready list.  These things won't be taken off the ready
>>>>> list until the next SOF, which might be a little late.  Let's put them
>>>>> on right away.
>>>>>
>>>>> Signed-off-by: Douglas Anderson 
>>>>> Tested-by: Heiko Stuebner 
>>>>> Tested-by: Stefan Wahren 
>>>>> ---
>>>>> Changes in v6:
>>>>> - Add Heiko's Tested-by.
>>>>> - Add Stefan's Tested-by.
>>>>>
>>>>> Changes in v5: None
>>>>> Changes in v4:
>>>>> - Schedule periodic right away if it's time new for v4.
>>>>>
>>>>> Changes in v3: None
>>>>> Changes in v2: None
>>>>>
>>>>>drivers/usb/dwc2/hcd_queue.c | 18 --
>>>>>1 file changed, 16 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>>>>> index 9b3c435339ee..3abb34a5fc5b 100644
>>>>> --- a/drivers/usb/dwc2/hcd_queue.c
>>>>> +++ b/drivers/usb/dwc2/hcd_queue.c
>>>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg
>>>>> *hsotg, struct dwc2_qh *qh,
>>>>>   * Note: we purposely use the frame_number from the "hsotg"
>>>>> structure
>>>>>   * since we know SOF interrupt will handle future frames.
>>>>>   */
>>>>> -   if (dwc2_frame_num_le(qh->next_active_frame,
>>>>> hsotg->frame_number))
>>>>> +   if (dwc2_frame_num_le(qh->next_active_frame,
>>>>> hsotg->frame_number))
>>>>> {
>>>>> +   enum dwc2_transaction_type tr_type;
>>>>> +
>>>>> +   /*
>>>>> +* We're bypassing the SOF handler which is normally
>>>>> what
>>>>> puts
>>>>> +* us on the ready list because we're in a hurry and
>>>>> need
>>>>> to
>>>>> +* try to catch up.
>>>>> +*/
>>>>> +   dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x,
>>>>> nxt=%04x\n",
>>>>> + qh, frame_number, qh->next_active_frame);
>>>>>  list_move_tail(&qh->qh_list_entry,
>>>>> &hsotg->periodic_sched_ready);
>>>>> -   else
>>>>> +
>>>>> +   tr_type = dwc2_hcd_select_transactions(hsotg);
>>>>
>>>> Do we need to add select_transactions call here? If we get into this
>>>> function in interrupt
>>>> and once we put the qh in ready queue, the qh can be handled in this
>>>> frame
>>>> again by the
>>>> later function call of dwc_hcd_select_transactions, so what we need to to
>>>> here is put
>>>> it in ready list instead of inactive queue, and wait for the schedule.
>>>
>>> I'm not sure I understand.  Can you restate?
>>>
>>>
>>> I'll try to explain more in the meantime...
>>>
>>> Both before and after my change, this function would place something
>>> on the ready queue if the next_active_frame <= the frame number as of
>>> last SOF interrupt (aka hsotg->frame_number).  Otherwise it goes on
>>> the inactive queue.  Assuming that the previous change ("usb: dwc2:
>>> host: Manage frame nums better in scheduler") worked properly then
>>> next_active_frame shouldn't be less than (hsotg->frame_number - 1).
>>> Remember that next_active_frame is always 1 before the wire frame, so
>>> if "next_active_frame == hsotg->frame_number - 1" it means that we
>>> need to 

Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time

[Doug Anderson]

Kever,

On Sun, Jan 31, 2016 at 7:32 PM, Kever Yang  wrote:
> Doug,
>
>
> On 02/01/2016 06:09 AM, Doug Anderson wrote:
>>
>> Kever,
>>
>> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang 
>> wrote:
>>>
>>> Doug,
>>>
>>>
>>> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>>>
>>>> In dwc2_hcd_qh_deactivate() we will put some things on the
>>>> periodic_sched_ready list.  These things won't be taken off the ready
>>>> list until the next SOF, which might be a little late.  Let's put them
>>>> on right away.
>>>>
>>>> Signed-off-by: Douglas Anderson 
>>>> Tested-by: Heiko Stuebner 
>>>> Tested-by: Stefan Wahren 
>>>> ---
>>>> Changes in v6:
>>>> - Add Heiko's Tested-by.
>>>> - Add Stefan's Tested-by.
>>>>
>>>> Changes in v5: None
>>>> Changes in v4:
>>>> - Schedule periodic right away if it's time new for v4.
>>>>
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>drivers/usb/dwc2/hcd_queue.c | 18 --
>>>>1 file changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>>>> index 9b3c435339ee..3abb34a5fc5b 100644
>>>> --- a/drivers/usb/dwc2/hcd_queue.c
>>>> +++ b/drivers/usb/dwc2/hcd_queue.c
>>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg
>>>> *hsotg, struct dwc2_qh *qh,
>>>>   * Note: we purposely use the frame_number from the "hsotg"
>>>> structure
>>>>   * since we know SOF interrupt will handle future frames.
>>>>   */
>>>> -   if (dwc2_frame_num_le(qh->next_active_frame,
>>>> hsotg->frame_number))
>>>> +   if (dwc2_frame_num_le(qh->next_active_frame,
>>>> hsotg->frame_number))
>>>> {
>>>> +   enum dwc2_transaction_type tr_type;
>>>> +
>>>> +   /*
>>>> +* We're bypassing the SOF handler which is normally
>>>> what
>>>> puts
>>>> +* us on the ready list because we're in a hurry and
>>>> need
>>>> to
>>>> +* try to catch up.
>>>> +*/
>>>> +   dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x,
>>>> nxt=%04x\n",
>>>> + qh, frame_number, qh->next_active_frame);
>>>>  list_move_tail(&qh->qh_list_entry,
>>>> &hsotg->periodic_sched_ready);
>>>> -   else
>>>> +
>>>> +   tr_type = dwc2_hcd_select_transactions(hsotg);
>>>
>>> Do we need to add select_transactions call here? If we get into this
>>> function in interrupt
>>> and once we put the qh in ready queue, the qh can be handled in this
>>> frame
>>> again by the
>>> later function call of dwc_hcd_select_transactions, so what we need to to
>>> here is put
>>> it in ready list instead of inactive queue, and wait for the schedule.
>>
>> I'm not sure I understand.  Can you restate?
>>
>>
>> I'll try to explain more in the meantime...
>>
>> Both before and after my change, this function would place something
>> on the ready queue if the next_active_frame <= the frame number as of
>> last SOF interrupt (aka hsotg->frame_number).  Otherwise it goes on
>> the inactive queue.  Assuming that the previous change ("usb: dwc2:
>> host: Manage frame nums better in scheduler") worked properly then
>> next_active_frame shouldn't be less than (hsotg->frame_number - 1).
>> Remember that next_active_frame is always 1 before the wire frame, so
>> if "next_active_frame == hsotg->frame_number - 1" it means that we
>> need to get the transfer on the wire _right away_.  If
>> "next_active_frame == hsotg->frame_number" the transfer doesn't need
>> to go on the wire right away, but since dwc2 can be prepped one frame
>> in advance it doesn't hurt to give it to the hardware right away if
>> there's space.
>>
>> As I understand it, if we stick something on the ready queue it won't
>> generally get lo

Re: [PATCH v6 10/22] usb: dwc2: host: Properly set the HFIR

[Doug Anderson]

Kever,

On Sun, Jan 31, 2016 at 1:23 AM, Kever Yang  wrote:
> Doug,
>
> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>
>> According to the most up to date version of the dwc2 databook, the FRINT
>> field of the HFIR register should be programmed to:
>> * 125 us * (PHY clock freq for HS) - 1
>> * 1000 us * (PHY clock freq for FS/LS) - 1
>
> I got 3 version of dwc_otg databook, 2.74a, 2.94a and 3.10a,
> all the doc describe the FrInt as:

Can you check to see if you can get 3.30a (October 2015)?


> * 125 us * (PHY clock freq for HS)
> * 1000 us * (PHY clock freq for FS/LS)
>
> Maybe John can help to check the design.

Yes, this really needs John or someone at Synopsys.


> There are some feature different in new and old version, but not sure
> if this is one of then.
>
> The doc says If no value is programmed, the corecalculates the value
> based on the PHY clock specified in the FS/LS PHY Clock select field of
> Host configuration register(HCFG.FLSLPclkSel), does this work?

It seems to.  It looks like that's what makes our firmware work.  I'm
not 100% sure if there are any downsides to that approach...

-Doug
--
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

Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time

[Doug Anderson]

Kever,

On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang  wrote:
> Doug,
>
>
> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>
>> In dwc2_hcd_qh_deactivate() we will put some things on the
>> periodic_sched_ready list.  These things won't be taken off the ready
>> list until the next SOF, which might be a little late.  Let's put them
>> on right away.
>>
>> Signed-off-by: Douglas Anderson 
>> Tested-by: Heiko Stuebner 
>> Tested-by: Stefan Wahren 
>> ---
>> Changes in v6:
>> - Add Heiko's Tested-by.
>> - Add Stefan's Tested-by.
>>
>> Changes in v5: None
>> Changes in v4:
>> - Schedule periodic right away if it's time new for v4.
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/usb/dwc2/hcd_queue.c | 18 --
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>> index 9b3c435339ee..3abb34a5fc5b 100644
>> --- a/drivers/usb/dwc2/hcd_queue.c
>> +++ b/drivers/usb/dwc2/hcd_queue.c
>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg
>> *hsotg, struct dwc2_qh *qh,
>>  * Note: we purposely use the frame_number from the "hsotg"
>> structure
>>  * since we know SOF interrupt will handle future frames.
>>  */
>> -   if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
>> +   if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
>> {
>> +   enum dwc2_transaction_type tr_type;
>> +
>> +   /*
>> +* We're bypassing the SOF handler which is normally what
>> puts
>> +* us on the ready list because we're in a hurry and need
>> to
>> +* try to catch up.
>> +*/
>> +   dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x,
>> nxt=%04x\n",
>> + qh, frame_number, qh->next_active_frame);
>> list_move_tail(&qh->qh_list_entry,
>>&hsotg->periodic_sched_ready);
>> -   else
>> +
>> +   tr_type = dwc2_hcd_select_transactions(hsotg);
>
> Do we need to add select_transactions call here? If we get into this
> function in interrupt
> and once we put the qh in ready queue, the qh can be handled in this frame
> again by the
> later function call of dwc_hcd_select_transactions, so what we need to to
> here is put
> it in ready list instead of inactive queue, and wait for the schedule.

I'm not sure I understand.  Can you restate?


I'll try to explain more in the meantime...

Both before and after my change, this function would place something
on the ready queue if the next_active_frame <= the frame number as of
last SOF interrupt (aka hsotg->frame_number).  Otherwise it goes on
the inactive queue.  Assuming that the previous change ("usb: dwc2:
host: Manage frame nums better in scheduler") worked properly then
next_active_frame shouldn't be less than (hsotg->frame_number - 1).
Remember that next_active_frame is always 1 before the wire frame, so
if "next_active_frame == hsotg->frame_number - 1" it means that we
need to get the transfer on the wire _right away_.  If
"next_active_frame == hsotg->frame_number" the transfer doesn't need
to go on the wire right away, but since dwc2 can be prepped one frame
in advance it doesn't hurt to give it to the hardware right away if
there's space.

As I understand it, if we stick something on the ready queue it won't
generally get looked at until the next SOF interrupt.  That means
we'll be too late if "next_active_frame == hsotg->frame_number - 1"
and we'll possibly be too late (depending on interrupt latency) if
"next_active_frame == hsotg->frame_number"


Note that before my series, there were more places than just the SOF
interrupt that would update hsotg->frame_number (see "usb: dwc2: host:
Manage frame nums better in scheduler" for fix).  Also before my
series (specially "usb: dwc2: host: Manage frame nums better in
scheduler") we used the actual current frame number when doing
comparisons.  Also before my series (specifically "usb: dwc2: host:
Properly set even/odd frame") we didn't really place things in the
frame that they were scheduled in anyway.


Also note that I believe that when dwc2_hcd_qh_deactivate() is called
our spinlock is held which means that the SOF interrupt either ran
before our function or won't run till after it.

>
>> +   if (tr_type != DWC2_TRANSACTION_NONE)
>> +   dwc2_hcd_queue_transactions(hsotg, tr_type);
>> +   } else {
>> list_move_tail(&qh->qh_list_entry,
>>&hsotg->periodic_sched_inactive);
>> +   }
>>   }
>> /**
>
>
>
--
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

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

[Doug Anderson]

Stefan,

On Fri, Jan 29, 2016 at 7:28 AM, Stefan Wahren  wrote:
> Hi Doug,
>
> Am 27.01.2016 um 21:43 schrieb Doug Anderson:
>> Stefan,
>>
>> On Wed, Jan 27, 2016 at 12:36 PM, Stefan Wahren  
>> wrote:
>>> i can only give you feedback from a user perspective. My keyboard and 
>>> C-Media
>>> USB Audio still works as expected.
>> OK, thanks!  So you had no problems before my patches and you still
>> have no problems after my patches.  ...but your USB setup is fairly
>> simple, at least.  Your testing is still valuable (thank you thank you
>> thank you), I just wanted to check.  :)
>
> if you need a result from a different USB setup in combination with your
> Patch V6 please describe.
>
> But i don't have a USB analyzer at home ;-)

Many (but not all) of my bugfixes are around dealing with with split
transactions.  That happens when you've got a high speed hub hooked up
to the port and then you attach a bunch of low speed / full speed
devices.

Things were even buggier if you attached a "single tt" hub.  A "single
tt" hub means that all low speed / full speed devices on the hub are
treated as being on a single full speed hub.  Said another way, if you
have a single tt hub then all of the low speed / full speed devices
attached to that hub need to share 12 MB/s of bandwidth.  If you have
a multi tt hub then each of the low speed / full speed devices
attached to the hub have their own 12 MB/s of bandwidth.  Most hubs
I've randomly obtained are single tt.


In any case, before my patches:

* If you have a USB Audio device that is FULL SPEED (not high speed)
plus a mouse or keyboard, playing audio will tend to cause problems
with the mouse / keyboard.

* Attaching multiple keyboards / mice to a hub would tend to cause
dropped keys on the keyboards / slow mice.  Things were worse with
fancier keyboards in general.  You didn't need to type things on all
keyboards at once, just have them attached.

* In general the more stuff you pile onto a hub the more problems.  My
stressful USB test case was to get a USB webcam + USB audio + a few
mice / keyboards.  That ought to improve significantly with my
patches, though I'm not 100% sure that the Raspberry Pi will really be
able to keep up with all that.  The USB webcam might be especially
stressful due to some craziness with uncached memory and URB
completion happening with interrupts disabled (even after forking
things off to a tasklet).  I can go into this more if someone is
interested but there's a bit of details in
<https://chromium-review.googlesource.com/#/c/321980/> and
<https://chromium-review.googlesource.com/#/c/321932/>.


If you happen to have a FULL SPEED (USB 1.1) hub it would also be
interesting to plug that into your Pi directly and then plug a few
things into it.  One of my patches addresses a problem related to
that.  Of course, it's pretty hard to track down a full speed hub
these days (I've got a box full of old peripherals in my closet).
--
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

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

[Doug Anderson]

Hi,

On Wed, Jan 27, 2016 at 7:25 PM, Kever Yang  wrote:
> Hi Doug,
>
> The NULL pointer bug is one of the most frequent issue we met
> during hot plug stress test, thanks for this bug fix.
>
> Reviewed-by: Kever Yang 
>
> Thanks,
> - Kever

Thanks for your review.  I think I actually found one more place where
I needed to clean up the channel->qh, so I'll include that in my next
version.  I'll plan to keep your reviewed-by.  Please yell if you want
it removed.

-Doug
--
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

Re: [PATCH v5 04/21] usb: dwc2: host: Set host_perio_tx_fifo_size to 304 for rk3066

[Doug Anderson]

Hi,

On Thu, Jan 28, 2016 at 10:16 AM, Doug Anderson  wrote:
> Kever,
>
>
> On Wed, Jan 27, 2016 at 10:41 PM, Kever Yang  
> wrote:
>> Hi Doug,
>>
>> We are in HOST mode, we only need to receive data from USB camera
>> with RX FIFO, and no need to use TX FIFO for USB webcam, right? :)
>>
>> Any way, I think we need to NAK this patch after look into the design
>> of dwc2 controller. Because all the dwc2 controller inside the Rockchip
>> chips don't support the thresholding FIFO mode, in this case, there is
>> no more transaction before a whole packet is send out and the dwc2 only
>> care if the available FIFO is enough for next packet or not.
>>
>> So, the addition 48 words won't help to shorten the latency for data prepare
>> in this case.
>
> Ah ha!  I see where I messed up.  It wasn't lack of FIFO space that I
> was running into, it was lack of queue space.  :-P  I had conflated
> the two of them in my mind.  We still use the TX queue to transmit
> small packets so that we can receive our data...
>
> OK, so it's pretty sane to assume that exhausting the periodic TX FIFO
> isn't terribly common, I think.  Audio won't do it by itself and you
> probably won't have more than one audio device.  I guess if you had a
> video _output_ device hooked over USB then it could possible exhaust
> things.  ...but I think dwc2 still has a ways to do before I'd suggest
> anyone hooking that up.  :-P  At some point in time you could imagine
> the driver being more dynamic.
>
> OK, so the non periodic FIFO it is, then.

Actually, in my simple tests adding it to the non periodic FIFO didn't
help at all.  I'll just drop this patch.

-Doug
--
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

Re: [PATCH v5 04/21] usb: dwc2: host: Set host_perio_tx_fifo_size to 304 for rk3066

[Doug Anderson]

Kever,


On Wed, Jan 27, 2016 at 10:41 PM, Kever Yang  wrote:
> Hi Doug,
>
> We are in HOST mode, we only need to receive data from USB camera
> with RX FIFO, and no need to use TX FIFO for USB webcam, right? :)
>
> Any way, I think we need to NAK this patch after look into the design
> of dwc2 controller. Because all the dwc2 controller inside the Rockchip
> chips don't support the thresholding FIFO mode, in this case, there is
> no more transaction before a whole packet is send out and the dwc2 only
> care if the available FIFO is enough for next packet or not.
>
> So, the addition 48 words won't help to shorten the latency for data prepare
> in this case.

Ah ha!  I see where I messed up.  It wasn't lack of FIFO space that I
was running into, it was lack of queue space.  :-P  I had conflated
the two of them in my mind.  We still use the TX queue to transmit
small packets so that we can receive our data...

OK, so it's pretty sane to assume that exhausting the periodic TX FIFO
isn't terribly common, I think.  Audio won't do it by itself and you
probably won't have more than one audio device.  I guess if you had a
video _output_ device hooked over USB then it could possible exhaust
things.  ...but I think dwc2 still has a ways to do before I'd suggest
anyone hooking that up.  :-P  At some point in time you could imagine
the driver being more dynamic.

OK, so the non periodic FIFO it is, then.

-Doug
--
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

Re: [PATCH v5 04/21] usb: dwc2: host: Set host_perio_tx_fifo_size to 304 for rk3066

[Doug Anderson]

Kever,

On Wed, Jan 27, 2016 at 7:10 PM, Kever Yang  wrote:
> Hi Doug,
>
> We are using the minimum FIFO size mode for TX now, which only
> equal to one max packet size.
>
> The addition FIFO size may help shorten the inter-packet data
> prepare latency when the bus/DRAM is busy.
>
> For the actual usage in TX, we have very little change to use the
> period TX FIFO with more than one max packet size in host mode.
> So far as I know, usb audio use the isochronous tx FIFO, but this
> king of device won't have much data payload and won't, I haven't
> see a usb audio have more data than 1024byte/ms.
>
> So I suggest we assign this 48 words to host_nperio_tx_fifi_size instead
> if we have to do this. Because we are using device base on bulk transaction
> like U-disk very frequently.

Try using a USB webcam.  With that plus a USB audio device it's easy
to overwhelm the periodic TX FIFO.

If we overwhelm the periodic TX FIFO we might actually fail to
transmit ISO or INT packets at the scheduled time.  That seems more
serious of a problem to try to fix than eeking out a tiny bit
performance on a USB disk.  ...but of course, it all depends on what
you consider important.  ;)

We could split the difference, I suppose and put half on each?

-Doug
--
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

Re: Bug in split transactions on Raspberry Pi

[Doug Anderson]

Hi,

On Wed, Jan 27, 2016 at 2:03 PM, Alan Stern  wrote:
> On Wed, 27 Jan 2016, Doug Anderson wrote:
>
>> Alan,
>>
>> On Wed, Jan 27, 2016 at 1:34 PM, Alan Stern  
>> wrote:
>> > On Wed, 27 Jan 2016, Doug Anderson wrote:
>> >
>> >> This patch should fix ya.
>> >>
>> >> FIXUP: FROMLIST: usb: dwc2: host: Manage frame nums better in scheduler
>> >> https://chromium-review.googlesource.com/324185
>> >
>> > Hmmm.  That fixed the problem of the polls occuring too frequently, but
>> > now I see again intervals that are larger than 256 ms.  In the most
>> > recent test there are two intervals of 512 ms and one of 2048 ms.
>>
>> OK, good to know.  Ugh.  I'll have to see if I can reproduce that.  If
>> I had to guess, though, I'd say that you're probably running into high
>> interrupt latency problems.
>
> Quite possibly.  Would that delay the transfers by a full period or
> only by one frame?

Yeah, that's the part that doesn't make sense.  I would not have
expected it to be a multiple.  If we missed something then we should
have picked something sooner to reschedule.  Even if the frame we
picked to reschedule wasn't ideal you still should have seen it on the
bus.

In other words, I'd expect lots of 256 ms with the occasional 257,
258, 259.  I wouldn't expect 512.

Presumably the 512 means that there was an error somewhere and the
packet was dropped.  I'm actually tracking down a weird full speed hub
bug right now and it's possible that it's a more serious / general
problem.

It's also possible that we're somehow overflowing a FIFO or a queue.
I've seen some overflow errors pop by while debugging recently, and
those shouldn't happen unless calculations are off somewhere.


>>  Those problems would be worse on the
>> Raspberry Pi than on my system due to the significantly slower
>> processor.
>>
>> Can you confirm that these problems also were introduced by my series?
>>  AKA: you never saw > 256 ms polls before my series and now you see
>> them?
>
> No, these problems were also present in the kernel without your
> patches.

Whew!  Still good to root cause, but also nice to know that I didn't
introduce this one.  ;)  Of course, had I introduced it it would
probably be easier to fix...


-Doug
--
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

Re: Bug in split transactions on Raspberry Pi

[Doug Anderson]

Alan,

On Wed, Jan 27, 2016 at 1:34 PM, Alan Stern  wrote:
> On Wed, 27 Jan 2016, Doug Anderson wrote:
>
>> This patch should fix ya.
>>
>> FIXUP: FROMLIST: usb: dwc2: host: Manage frame nums better in scheduler
>> https://chromium-review.googlesource.com/324185
>
> Hmmm.  That fixed the problem of the polls occuring too frequently, but
> now I see again intervals that are larger than 256 ms.  In the most
> recent test there are two intervals of 512 ms and one of 2048 ms.

OK, good to know.  Ugh.  I'll have to see if I can reproduce that.  If
I had to guess, though, I'd say that you're probably running into high
interrupt latency problems.  Those problems would be worse on the
Raspberry Pi than on my system due to the significantly slower
processor.

Can you confirm that these problems also were introduced by my series?
 AKA: you never saw > 256 ms polls before my series and now you see
them?


Turning on scheduler tracing would probably be quite helpful at this
point.  You can see
<https://chromium-review.googlesource.com/#/c/314254/>.  If printing
even non-verbose traces to the console is too much, you could make
everything "trace_printk".  You might also want
<https://chromium-review.googlesource.com/#/c/323308/>.

Things printed with trace_printk will end up in
/sys/kernel/debug/tracing/.  See "trace" for a single trace buffer or
trace_pipe.  Using the trace buffer can be a bit of a pain but the
cost of the prints is much lower which is why it works well for
verbose scheduler spew.


I typically like the non-verbose stuff in the console and the verbose
stuff in the trace buffer.  Then I can look in the console for info on
what's scheduled and grep through the trace buffer for it.


> PS: Is the databook for the DWC2 controller available for download?
> I'm going on vacation starting next week, and I might want to have a
> little light reading to take along.  :-)

I don't have a databook that I can give you, unfortunately.
--
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

Re: Bug in split transactions on Raspberry Pi

[Doug Anderson]

Hi,

On Wed, Jan 27, 2016 at 12:59 PM, Alan Stern  wrote:
> On Wed, 27 Jan 2016, Doug Anderson wrote:
>
>> > In the end I just used the contents of the dwc2 directory from Linus's
>> > current tree -- I don't think it has changed since 4.5-rc1.  Your
>> > patches applied on top of that with no issues.
>> >
>> > They seem to work.  Is there anything in particular you would like me
>> > to test?
>>
>> I was sorta wondering if it fixed the original problem you were
>> seeing?  Unless I missed it it sounded like it was still broken on ToT
>> and I was wondering if my series happened to help in any way...
>
> They did not fix the original problem.  I still have no idea what could
> be causing the device's selective behavior.  If you want, I can send
> you the .tdc analyzer files for kernels with and without your patch
> series.  Maybe you'll see something I missed.

Argh.  Sure, go ahead and send the .tdc files.  Caveat emptor that I
know very little about USB (as I indicated in the cover letter of my
series), but I'll do my best.  ;)


> It did.  Without your patches the opposite problem occurred; the status
> polls sometimes didn't get sent when they should have.
>
>> Oh, wait, I think I know what this is.  This is probably ("usb: dwc2:
>> host: Manage frame nums better in scheduler").  I'd bet that if you go
>> before that change then the behavior will be the way it was.
>>
>> In said patch you can see that dwc2_schedule_periodic() will now
>> always pick a new first frame.  As I understand it that will happen if
>> there was ever a short period of time when we weren't scheduled and I
>> was worried about the next slice being in the past.  It wouldn't hurt
>> to change the test to only pick a new first frame if the existing
>> first frame is in the past.  That would probably fix your problem.
>
> Are you perhaps thinking of isochronous transfers?  This is an
> interrupt endpoint, not isochronous.  Individual interrupt transfers
> don't have assigned slices.  The "Add a delay before releasing periodic
> bandwidth" patch seems more relevant.  Or maybe "Schedule periodic
> right away if it's time", although that comes before the tasklet patch.

Please try the past I posted that crossed your reply in the ether.  ;)
 AKA: <https://chromium-review.googlesource.com/#/c/324185/>.  I
reproduced what I think is your problem and this patch fixed it for
me.

In my scheduler all periodic transfers have reservations.  Perhaps
it's not critical to reserve for INT transfers, but that's how things
are setup right now.


>> Question: is the behavior you're seeing illegal / undesirable?  It
>> only happens when there's data available, right?  My current
>> (rewritten) reservation algorithm in dwc2 is pretty crude  From a high
>> speed device perspective it reserves a timeslot for the EP in every
>> microframe even if the EP has a long interval.  It won't typically
>> schedule the EP more often than every "interval" microframes, but the
>> reservation is there.  That means we can't support quite as many
>> devices at the same time, but it has the advantage that if we screw up
>> (like if we miss an SOF) we know we can actually try to make up for it
>> quickly and we won't collide with anyone else.  The whole thing also
>> simplifies the scheduler.  In your particular case, we used our
>> existing reservation to schedule the device a little sooner than
>> requested.
>
> It's not illegal.  Interrupt transfers are guaranteed to be scheduled
> at least as often as bInterval; there's nothing wrong with issuing them
> more frequently.  But it is a waste of bandwidth and CPU time to issue
> transfers more often than necessary, so it is undesirable in that
> sense.

OK.  Well, hopefully fixed with my FIXUP patch.  ;)


> You know, from what you've said it seems that writing a proper periodic
> scheduler would be _easier_ for DWC2 than for EHCI.  The EHCI hardware
> requires special data structures to handle cases where a SPLIT packet
> occurs in microframe 7 or microframe 0 of the following frame; those
> structures are a nuisance to manage and ehci-hcd doesn't bother to
> implement them.  It sounds like DWC2 wouldn't need any of that.
> Furthermore, because coordination between the CPU and the EHCI core is
> rather asynchronous, it's difficult to change the microframe allotment
> for an interrupt endpoint should the schedule need to be rebalanced --
> that hasn't been implemented either.  Again, this should be much
> simpler for DWC2.
>
> I've go

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

[Doug Anderson]

Hi,

On Fri, Jan 22, 2016 at 10:18 AM, Douglas Anderson
 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 
> ---
> 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
 as a fixup to
this patch for the next version.  This is intended to fix problems
that Alan Stern reported at

--
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

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

[Doug Anderson]

Stefan,

On Wed, Jan 27, 2016 at 12:36 PM, Stefan Wahren  wrote:
> i can only give you feedback from a user perspective. My keyboard and C-Media
> USB Audio still works as expected.

OK, thanks!  So you had no problems before my patches and you still
have no problems after my patches.  ...but your USB setup is fairly
simple, at least.  Your testing is still valuable (thank you thank you
thank you), I just wanted to check.  :)

-Doug
--
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