| Message ID | 20190418001356.124334-2-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron | expand |
Hi, On 4/18/2019 04:15, Douglas Anderson wrote: > This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus > suspend/resume for dwc2") on ToT. That commit was reverted in commit > b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") > because apparently it broke the Altera SOCFPGA. > > With all the changes that have happened to dwc2 in the meantime, it's > possible that the Altera SOCFPGA will just magically work with this > change now. ...and it would be good to get bus suspend/resume > implemented. > > This change is a forward port of one that's been living in the Chrome > OS 3.14 kernel tree. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > This patch was last posted at: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e= > > ...and appears to have died the death of silence. Maybe it could get > some bake time in linuxnext if we can't find any proactive testing? > > I will also freely admit that I don't know tons about the theory > behind this patch. I'm mostly just re-hashing the original commit > from Kever that was reverted since: > * Turning on partial power down on rk3288 doesn't "just work". I > don't get hotplug events. This is despite dwc2 auto-detecting that > we are power optimized. What do you mean by doesn't "just work" ? It seem to me that even after adding this patch you don't get issues fixed. You mention that you don't get the hotplug events. Please provide dwc2 debug logs and register dumps on this issue. > * If we don't do something like this commit we don't get into as low > of a power mode. > > Changes in v2: None > > drivers/usb/dwc2/hcd.c | 84 ++++++++++++++++++++++++++---------------- > 1 file changed, 53 insertions(+), 31 deletions(-) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index e272d020012e..978232a9e4a8 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -4482,6 +4482,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > unsigned long flags; > int ret = 0; > u32 hprt0; > + u32 pcgctl; > > spin_lock_irqsave(&hsotg->lock, flags); > > @@ -4497,7 +4498,7 @@ 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) > goto skip_power_saving; > "hsotg->params.power_down" is assigned to "DWC2_POWER_DOWN_PARAM_NONE = 0" if there is no hibernation or partial power down supported by the core or power saving features are disabled by "hsotg->params.power_saving = false" , "DWC2_POWER_DOWN_PARAM_PARTIAL" if core supports partial power down, "DWC2_POWER_DOWN_PARAM_HIBERNATION " if the core supports hibernation When you check "if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL)" you are saying that "skip_power_saving" only in that case when core supports Hibernation. But what if core doesn't support both hibernation and partial power down and the "hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" which is 0. With this implementation driver will program entering to suspend when core doesn't support both hibernation and partial power down. > /* > @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > */ > 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); > + if (hprt0 & HPRT0_CONNSTS) { > + hprt0 |= HPRT0_SUSP; Here you set "HPRT0_SUSP" bit but what if core doesn't support both hibernation and Partial Power down assuming that hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" which is 0. > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) You make one checking of hsotg->params.power_down mode here. > + hprt0 &= ~HPRT0_PWR; > + dwc2_writel(hsotg, hprt0, HPRT0); > + } > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { another checking of power_down mode here. > + spin_unlock_irqrestore(&hsotg->lock, flags); > + dwc2_vbus_supply_exit(hsotg); > + spin_lock_irqsave(&hsotg->lock, flags); > + } else { > + pcgctl = readl(hsotg->regs + PCGCTL); > + pcgctl |= PCGCTL_STOPPCLK; > + writel(pcgctl, hsotg->regs + PCGCTL); "PCGCTL_STOPPCLK" bit is set only when core enters to partial power down. So here if hsotg->params.power_down is not equal to DWC2_POWER_DOWN_PARAM_PARTIAL and is DWC2_POWER_DOWN_PARAM_NONE the the bit will be set. > + } > } > > - /* 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; > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { one more power_down mode checking here. I understand that those checking are to make sure that we got partial power down mode enabled but before this patch it was done with one checking. > + /* 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; > + } > + > + /* After entering partial_power_down, hardware is no more accessible */ > + clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > } > > /* Ask phy to be suspended */ > @@ -4530,9 +4545,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > spin_lock_irqsave(&hsotg->lock, flags); > } > > - /* After entering partial_power_down, hardware is no more accessible */ > - clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > - > skip_power_saving: > hsotg->lx_state = DWC2_L2; > unlock: > @@ -4545,6 +4557,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) > { > struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd); > unsigned long flags; > + u32 pcgctl; > int ret = 0; > > spin_lock_irqsave(&hsotg->lock, flags); > @@ -4555,17 +4568,11 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) > if (hsotg->lx_state != DWC2_L2) > goto unlock; > > - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) { > + if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) { > hsotg->lx_state = DWC2_L0; > goto unlock; > } > > - /* > - * Set HW accessible bit before powering on the controller > - * since an interrupt may rise. > - */ > - set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > - > /* > * Enable power if not already done. > * This must not be spinlocked since duration > @@ -4577,10 +4584,23 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) > spin_lock_irqsave(&hsotg->lock, flags); > } > > - /* Exit partial_power_down */ > - ret = dwc2_exit_partial_power_down(hsotg, true); > - if (ret && (ret != -ENOTSUPP)) > - dev_err(hsotg->dev, "exit partial_power_down failed\n"); > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { > + /* > + * Set HW accessible bit before powering on the controller > + * since an interrupt may rise. > + */ > + set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > + > + you leave an odd blank line here. Please delete it. > + /* Exit partial_power_down */ > + ret = dwc2_exit_partial_power_down(hsotg, true); > + if (ret && (ret != -ENOTSUPP)) > + dev_err(hsotg->dev, "exit partial_power_down failed\n"); > + } else { > + pcgctl = readl(hsotg->regs + PCGCTL); > + pcgctl &= ~PCGCTL_STOPPCLK; > + writel(pcgctl, hsotg->regs + PCGCTL); Here if core doesn't support both hibernation and partial power down and "hsotg->params.power_down" is equal to "DWC2_POWER_DOWN_PARAM_NONE" which is 0 then "PCGCTL_STOPPCLK" bit is unset. > + } > > hsotg->lx_state = DWC2_L0; > > @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) > spin_unlock_irqrestore(&hsotg->lock, flags); > dwc2_port_resume(hsotg); > } else { > - dwc2_vbus_supply_init(hsotg); > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { > + dwc2_vbus_supply_init(hsotg); > > - /* Wait for controller to correctly update D+/D- level */ > - usleep_range(3000, 5000); > + /* Wait for controller to correctly update D+/D- level */ > + usleep_range(3000, 5000); > + } > > /* > * Clear Port Enable and Port Status changes. > I have tested the patch on HAPS-DX. With this patch or without it when I have a device connected core enters to partial power down and doesn't exit from it. So I cannot use the device.
Hi, On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Hi, > > On 4/18/2019 04:15, Douglas Anderson wrote: > > This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus > > suspend/resume for dwc2") on ToT. That commit was reverted in commit > > b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") > > because apparently it broke the Altera SOCFPGA. > > > > With all the changes that have happened to dwc2 in the meantime, it's > > possible that the Altera SOCFPGA will just magically work with this > > change now. ...and it would be good to get bus suspend/resume > > implemented. > > > > This change is a forward port of one that's been living in the Chrome > > OS 3.14 kernel tree. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > This patch was last posted at: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e= > > > > ...and appears to have died the death of silence. Maybe it could get > > some bake time in linuxnext if we can't find any proactive testing? > > > > I will also freely admit that I don't know tons about the theory > > behind this patch. I'm mostly just re-hashing the original commit > > from Kever that was reverted since: > > * Turning on partial power down on rk3288 doesn't "just work". I > > don't get hotplug events. This is despite dwc2 auto-detecting that > > we are power optimized. > What do you mean by doesn't "just work" ? It seem to me that even after > adding this patch you don't get issues fixed. > You mention that you don't get the hotplug events. Please provide dwc2 > debug logs and register dumps on this issue. I mean that partial power down in the currently upstream driver doesn't work. AKA: if I turn on partial power down in the upstream driver then hotplug events break. I can try to provide some logs. On what exact version of the code do you want logs? Just your series? Just my series? Mainline? Some attempt at combining both series? As I said things seem to sorta work with the combined series. I can try to clarify if that's the series you want me to test with. ...or I can wait for your next version? > > @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > > */ > > 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); > > + if (hprt0 & HPRT0_CONNSTS) { > + hprt0 |= HPRT0_SUSP; > Here you set "HPRT0_SUSP" bit but what if core doesn't support both > hibernation and Partial Power down assuming that > hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" > which is 0. I am by no means an expert on dwc2, but an assumption made in my patch is that even cores that can't support partial power down can still save some amount of power when hcd_suspend is called. Some evidence that this should be possible: looking at mainline Linux and at dwc2_port_suspend(), I see: * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE * It currently sets HPRT0_SUSP * It currently sets PCGCTL_STOPPCLK specifically in the case where power down is DWC2_POWER_DOWN_PARAM_NONE. ...I believe that the net effect of my patch ends up doing both those same two things in hcd_suspend. That is: when power_down is DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the same thing that dwc2_port_suspend() would do in the same case. Is that not OK? > > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) > You make one checking of hsotg->params.power_down mode here. > > + hprt0 &= ~HPRT0_PWR; > > + dwc2_writel(hsotg, hprt0, HPRT0); > > + } > > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { > another checking of power_down mode here. Yeah, we can debate about how to best share/split code. I'm not in love with the current structure either. When I rebased your patches atop mine I changed this to more fully split them and I agree that was better. > > @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) > > spin_unlock_irqrestore(&hsotg->lock, flags); > > dwc2_port_resume(hsotg); > > } else { > > - dwc2_vbus_supply_init(hsotg); > > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { > > + dwc2_vbus_supply_init(hsotg); > > > > - /* Wait for controller to correctly update D+/D- level */ > > - usleep_range(3000, 5000); > > + /* Wait for controller to correctly update D+/D- level */ > > + usleep_range(3000, 5000); > > + } > > > > /* > > * Clear Port Enable and Port Status changes. > > > > I have tested the patch on HAPS-DX. With this patch or without it when I > have a device connected core enters to partial power down and doesn't > exit from it. So I cannot use the device. Can you explain what HAPS-DX is? -Doug
Hi, On 4/29/2019 21:34, Doug Anderson wrote: > Hi, > > On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> Hi, >> >> On 4/18/2019 04:15, Douglas Anderson wrote: >>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus >>> suspend/resume for dwc2") on ToT. That commit was reverted in commit >>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") >>> because apparently it broke the Altera SOCFPGA. >>> >>> With all the changes that have happened to dwc2 in the meantime, it's >>> possible that the Altera SOCFPGA will just magically work with this >>> change now. ...and it would be good to get bus suspend/resume >>> implemented. >>> >>> This change is a forward port of one that's been living in the Chrome >>> OS 3.14 kernel tree. >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>> --- >>> This patch was last posted at: >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e= >>> >>> ...and appears to have died the death of silence. Maybe it could get >>> some bake time in linuxnext if we can't find any proactive testing? >>> >>> I will also freely admit that I don't know tons about the theory >>> behind this patch. I'm mostly just re-hashing the original commit >>> from Kever that was reverted since: >>> * Turning on partial power down on rk3288 doesn't "just work". I >>> don't get hotplug events. This is despite dwc2 auto-detecting that >>> we are power optimized. >> What do you mean by doesn't "just work" ? It seem to me that even after >> adding this patch you don't get issues fixed. >> You mention that you don't get the hotplug events. Please provide dwc2 >> debug logs and register dumps on this issue. > > I mean that partial power down in the currently upstream driver > doesn't work. AKA: if I turn on partial power down in the upstream > driver then hotplug events break. I can try to provide some logs. On > what exact version of the code do you want logs? Just your series? > Just my series? Mainline? Some attempt at combining both series? As > I said things seem to sorta work with the combined series. I can try > to clarify if that's the series you want me to test with. ...or I can > wait for your next version? As I said this patch doesn't fix the issue with hotplug. With this patch or without the hotplug behaves as it was. I have tested it on our setup. Have you debugged your patch? Does it make any difference on your setup ? Does it fix the issue with hotplug? Try to debug with the following steps. 1. Debug code with just your patch. Capture the logs and provide. So that I can see what is difference with your patch. 2. Debug only with my series and see if those issues with hotplug are still there. > > >>> @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) >>> */ >>> 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); >>> + if (hprt0 & HPRT0_CONNSTS) { > + hprt0 |= HPRT0_SUSP; >> Here you set "HPRT0_SUSP" bit but what if core doesn't support both >> hibernation and Partial Power down assuming that >> hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" >> which is 0. > > I am by no means an expert on dwc2, but an assumption made in my patch > is that even cores that can't support partial power down can still > save some amount of power when hcd_suspend is called. Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ? > > Some evidence that this should be possible: looking at mainline Linux > and at dwc2_port_suspend(), I see: > > * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE Currently (without your and my patches) (looking at mainline Linux) the function dwc2_port_suspend() is called anyway because its call is issued by the system. But it performs entering to suspend only in case of DWC2_POWER_DOWN_PARAM_PARTIAL. This is not an assumption. What I am pointing out is based on debugging and before making assumptions without debugging for me seems not ok. Currently without your patch and without my patches. In the dwc2_port_suspend() it will enter to suspend only in case that power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the code more carefully you will see if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) goto skip_power_saving; This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip power saving. So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it tries to suspend. > > * It currently sets HPRT0_SUSP > > * It currently sets PCGCTL_STOPPCLK specifically in the case where > power down is DWC2_POWER_DOWN_PARAM_NONE. It currently (without my and your patches) doesn't set PCGCTL_STOPPCLK when power down is DWC2_POWER_DOWN_PARAM_NONE. Because again and again when power down is DWC2_POWER_DOWN_PARAM_NONE power saving is skipped. > > ...I believe that the net effect of my patch ends up doing both those > same two things in hcd_suspend. That is: when power_down is > DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the > same thing that dwc2_port_suspend() would do in the same case. Is > that not OK? No if your patch is doing the same thing as it was doing before what is the purpose of the patch ? My testes show that your patch doesn't fix the issue related partial power down. > > > >>> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) >> You make one checking of hsotg->params.power_down mode here. >>> + hprt0 &= ~HPRT0_PWR; >>> + dwc2_writel(hsotg, hprt0, HPRT0); >>> + } >>> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { >> another checking of power_down mode here. > > Yeah, we can debate about how to best share/split code. I'm not in > love with the current structure either. When I rebased your patches > atop mine I changed this to more fully split them and I agree that was > better. > > >>> @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) >>> spin_unlock_irqrestore(&hsotg->lock, flags); >>> dwc2_port_resume(hsotg); >>> } else { >>> - dwc2_vbus_supply_init(hsotg); >>> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { >>> + dwc2_vbus_supply_init(hsotg); >>> >>> - /* Wait for controller to correctly update D+/D- level */ >>> - usleep_range(3000, 5000); >>> + /* Wait for controller to correctly update D+/D- level */ >>> + usleep_range(3000, 5000); >>> + } >>> >>> /* >>> * Clear Port Enable and Port Status changes. >>> >> >> I have tested the patch on HAPS-DX. With this patch or without it when I >> have a device connected core enters to partial power down and doesn't >> exit from it. So I cannot use the device. > > Can you explain what HAPS-DX is? It is the general setup to perform our use case testes. For more information about the details you can google about it. > > > -Doug >
Hi, On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Hi, > > On 4/29/2019 21:34, Doug Anderson wrote: > > Hi, > > > > On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan > > <Arthur.Petrosyan@synopsys.com> wrote: > >> > >> Hi, > >> > >> On 4/18/2019 04:15, Douglas Anderson wrote: > >>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus > >>> suspend/resume for dwc2") on ToT. That commit was reverted in commit > >>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") > >>> because apparently it broke the Altera SOCFPGA. > >>> > >>> With all the changes that have happened to dwc2 in the meantime, it's > >>> possible that the Altera SOCFPGA will just magically work with this > >>> change now. ...and it would be good to get bus suspend/resume > >>> implemented. > >>> > >>> This change is a forward port of one that's been living in the Chrome > >>> OS 3.14 kernel tree. > >>> > >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> > >>> --- > >>> This patch was last posted at: > >>> > >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e= > >>> > >>> ...and appears to have died the death of silence. Maybe it could get > >>> some bake time in linuxnext if we can't find any proactive testing? > >>> > >>> I will also freely admit that I don't know tons about the theory > >>> behind this patch. I'm mostly just re-hashing the original commit > >>> from Kever that was reverted since: > >>> * Turning on partial power down on rk3288 doesn't "just work". I > >>> don't get hotplug events. This is despite dwc2 auto-detecting that > >>> we are power optimized. > >> What do you mean by doesn't "just work" ? It seem to me that even after > >> adding this patch you don't get issues fixed. > >> You mention that you don't get the hotplug events. Please provide dwc2 > >> debug logs and register dumps on this issue. > > > > I mean that partial power down in the currently upstream driver > > doesn't work. AKA: if I turn on partial power down in the upstream > > driver then hotplug events break. I can try to provide some logs. On > > what exact version of the code do you want logs? Just your series? > > Just my series? Mainline? Some attempt at combining both series? As > > I said things seem to sorta work with the combined series. I can try > > to clarify if that's the series you want me to test with. ...or I can > > wait for your next version? > As I said this patch doesn't fix the issue with hotplug. With this patch > or without the hotplug behaves as it was. I have tested it on our setup. > > Have you debugged your patch? Does it make any difference on your setup > ? Does it fix the issue with hotplug? I think we're still not taking on the same page. My patch makes no attempt to make partial power down mode work. My patch attempts to make things work a little better when using DWC2_POWER_DOWN_PARAM_NONE. There is no use testing my patch with partial power down as it shouldn't have any impact there. > > I am by no means an expert on dwc2, but an assumption made in my patch > > is that even cores that can't support partial power down can still > > save some amount of power when hcd_suspend is called. > Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ? > > > > Some evidence that this should be possible: looking at mainline Linux > > and at dwc2_port_suspend(), I see: > > > > * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE > Currently (without your and my patches) (looking at mainline Linux) the > function dwc2_port_suspend() is called anyway because its call is issued > by the system. But it performs entering to suspend only in case of > DWC2_POWER_DOWN_PARAM_PARTIAL. > > This is not an assumption. What I am pointing out is based on debugging > and before making assumptions without debugging for me seems not ok. > > Currently without your patch and without my patches. In the > dwc2_port_suspend() it will enter to suspend only in case that > power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the > code more carefully you will see > > if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) > goto skip_power_saving; > > This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip > power saving. > > So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it > tries to suspend. We must be looking at different code. I'm looking at Linux's tree, AKA: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n3488 I took a mainline kernel ("v5.1-rc7-5-g83a50840e72a") and added printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and PCGCTL_STOPPCLK in dwc2_port_suspend(). [ 454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP [ 454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK ...and just to confirm: # grep '^power' /sys/kernel/debug/*.usb/params /sys/kernel/debug/ff540000.usb/params:power_down : 0 /sys/kernel/debug/ff580000.usb/params:power_down : 0 So I'm really quite convinced that on mainline Linux with DWC2_POWER_DOWN_PARAM_NONE that dwc2_port_suspend() sets HPRT0_SUSP and PCGCTL_STOPPCLK. > > ...I believe that the net effect of my patch ends up doing both those > > same two things in hcd_suspend. That is: when power_down is > > DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the > > same thing that dwc2_port_suspend() would do in the same case. Is > > that not OK? > No if your patch is doing the same thing as it was doing before what is > the purpose of the patch ? The purpose is to make _dwc2_hcd_suspend() work more correctly in the case where power_down is DWC2_POWER_DOWN_PARAM_NONE > My testes show that your patch doesn't fix the issue related partial > power down. Right. I have been trying to say that my patch doesn't do anything at all for partial power down. I am simply trying to make DWC2_POWER_DOWN_PARAM_NONE work more correctly. I haven't run all the power consumption tests in quite a long time and I'll try to get it hooked up tomorrow to confirm that my patch really truly is still needed to help with power consumption. I did confirm that at least there are cases where _dwc2_hcd_suspend() is called and my patch is what sets the important bits. -Doug
Hi, On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote: > > This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus > suspend/resume for dwc2") on ToT. That commit was reverted in commit > b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") > because apparently it broke the Altera SOCFPGA. > > With all the changes that have happened to dwc2 in the meantime, it's > possible that the Altera SOCFPGA will just magically work with this > change now. ...and it would be good to get bus suspend/resume > implemented. > > This change is a forward port of one that's been living in the Chrome > OS 3.14 kernel tree. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > This patch was last posted at: > > https://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org > > ...and appears to have died the death of silence. Maybe it could get > some bake time in linuxnext if we can't find any proactive testing? > > I will also freely admit that I don't know tons about the theory > behind this patch. I'm mostly just re-hashing the original commit > from Kever that was reverted since: > * Turning on partial power down on rk3288 doesn't "just work". I > don't get hotplug events. This is despite dwc2 auto-detecting that > we are power optimized. > * If we don't do something like this commit we don't get into as low > of a power mode. OK, I spent the day digging more into this patch to confirm that it's really the right thing to do. ...and it still seems to be. First off: I'm pretty sure the above sentence "If we don't do something like this commit we don't get into as low of a power mode." is totally wrong. Luckily it's "after the cut" and not part of the commit message. Specifically I did a bunch of power testing and I couldn't find any instance saving power after this patch. ...but, then I looked more carefully at all the history of this commit. I ended up at: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306265/ ...where I said that this fixes a resume speed regression. More details could be found at the linked bug, AKA: https://bugs.chromium.org/p/chromium/issues/detail?id=548336 ...but, sadly, I wasn't as verbose as I usually am and didn't describe my exact testing setup. So I tried to reproduce. ...and I was able to. I tested on an rk3288-veyron-jerry with an empty USB hub plugged into the left port (the host port) and my "servo 2" debug board hooked up to the right port. The "power_Resume" test in Chrome OS certainly showed a regression in 3.14 when doing a suspend/resume cycle. Digging into the logs in 3.14, before this patch I saw this in the logs: usb 3-1: reset high-speed USB device number 2 using dwc2 usb 3-1.7: reset high-speed USB device number 3 using dwc2 ...after this patch: usb 3-1: USB disconnect, device number 2 usb 3-1.7: USB disconnect, device number 3 usb 3-1: new high-speed USB device number 4 using dwc2 usb 3-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00 usb 3-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0 usb 3-1: Product: USB 2.0 Hub [MTT] usb 3-1.7: new high-speed USB device number 5 using dwc2 usb 3-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11 usb 3-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0 usb 3-1.7: Product: USB 2.0 Hub ...so basically my belief is that without this patch we're just sorta leaving the device hanging and it get confused on resume. After this patch we behave slightly better. I tested on 4.19 and found much the same. There: usb 2-1: reset high-speed USB device number 2 using dwc2 usb 2-1.7: reset high-speed USB device number 3 using dwc2 vs. usb 2-1.7: USB disconnect, device number 3 usb 2-1: USB disconnect, device number 2 usb 2-1: new high-speed USB device number 4 using dwc2 usb 2-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00 usb 2-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0 usb 2-1: Product: USB 2.0 Hub [MTT] usb 2-1.7: new high-speed USB device number 5 using dwc2 usb 2-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11 usb 2-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0 usb 2-1.7: Product: USB 2.0 Hub On 4.19 I didn't actually notice a the same resume time regression, presumably because things are happening more asynchronously there (I didn't confirm this). ...but in any case it seems like the right thing to do to actually do the suspend. I'll also re-iterate once more that I'm not claiming that my patch helps with "partial power down". It merely makes the "power savings disabled" case work more properly. I'll also note that my patch is already in Felipe's "testing/next" branch which I continue to believe is correct and good. -Doug
On 5/1/2019 05:57, Doug Anderson wrote: > Hi, > > On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> Hi, >> >> On 4/29/2019 21:34, Doug Anderson wrote: >>> Hi, >>> >>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan >>> <Arthur.Petrosyan@synopsys.com> wrote: >>>> >>>> Hi, >>>> >>>> On 4/18/2019 04:15, Douglas Anderson wrote: >>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus >>>>> suspend/resume for dwc2") on ToT. That commit was reverted in commit >>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") >>>>> because apparently it broke the Altera SOCFPGA. >>>>> >>>>> With all the changes that have happened to dwc2 in the meantime, it's >>>>> possible that the Altera SOCFPGA will just magically work with this >>>>> change now. ...and it would be good to get bus suspend/resume >>>>> implemented. >>>>> >>>>> This change is a forward port of one that's been living in the Chrome >>>>> OS 3.14 kernel tree. >>>>> >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>>> --- >>>>> This patch was last posted at: >>>>> >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e= >>>>> >>>>> ...and appears to have died the death of silence. Maybe it could get >>>>> some bake time in linuxnext if we can't find any proactive testing? >>>>> >>>>> I will also freely admit that I don't know tons about the theory >>>>> behind this patch. I'm mostly just re-hashing the original commit >>>>> from Kever that was reverted since: >>>>> * Turning on partial power down on rk3288 doesn't "just work". I >>>>> don't get hotplug events. This is despite dwc2 auto-detecting that >>>>> we are power optimized. >>>> What do you mean by doesn't "just work" ? It seem to me that even after >>>> adding this patch you don't get issues fixed. >>>> You mention that you don't get the hotplug events. Please provide dwc2 >>>> debug logs and register dumps on this issue. >>> >>> I mean that partial power down in the currently upstream driver >>> doesn't work. AKA: if I turn on partial power down in the upstream >>> driver then hotplug events break. I can try to provide some logs. On >>> what exact version of the code do you want logs? Just your series? >>> Just my series? Mainline? Some attempt at combining both series? As >>> I said things seem to sorta work with the combined series. I can try >>> to clarify if that's the series you want me to test with. ...or I can >>> wait for your next version? >> As I said this patch doesn't fix the issue with hotplug. With this patch >> or without the hotplug behaves as it was. I have tested it on our setup. >> >> Have you debugged your patch? Does it make any difference on your setup >> ? Does it fix the issue with hotplug? > > I think we're still not taking on the same page. > > My patch makes no attempt to make partial power down mode work. My > patch attempts to make things work a little better when using > DWC2_POWER_DOWN_PARAM_NONE. There is no use testing my patch with > partial power down as it shouldn't have any impact there. > > >>> I am by no means an expert on dwc2, but an assumption made in my patch >>> is that even cores that can't support partial power down can still >>> save some amount of power when hcd_suspend is called. >> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ? >>> >>> Some evidence that this should be possible: looking at mainline Linux >>> and at dwc2_port_suspend(), I see: >>> >>> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE >> Currently (without your and my patches) (looking at mainline Linux) the >> function dwc2_port_suspend() is called anyway because its call is issued >> by the system. But it performs entering to suspend only in case of >> DWC2_POWER_DOWN_PARAM_PARTIAL. >> >> This is not an assumption. What I am pointing out is based on debugging >> and before making assumptions without debugging for me seems not ok. >> >> Currently without your patch and without my patches. In the >> dwc2_port_suspend() it will enter to suspend only in case that >> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the >> code more carefully you will see >> >> if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) >> goto skip_power_saving; >> >> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip >> power saving. >> >> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it >> tries to suspend. > > We must be looking at different code. I'm looking at Linux's tree, AKA: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3488&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=IWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=AHu2iOKkybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e= Here you are looking at the old code. After that there are several of changes related to suspend/resume functions. This is the link to the code with changes. Latest version of those functions. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n4489 Your changes are sitting on that latest version of code. Not the old version of it. > > I took a mainline kernel ("v5.1-rc7-5-g83a50840e72a") and added > printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and > PCGCTL_STOPPCLK in dwc2_port_suspend(). I think you did this tests on the old version of the code I have tested the flow myself with the mainline Kernel on "torvalds/master" and not HPRT0_SUSP nor PCGCTL_STOPPCLK are not being set. So here you need to review those things again. > > [ 454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP > [ 454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK > > ...and just to confirm: > > # grep '^power' /sys/kernel/debug/*.usb/params > /sys/kernel/debug/ff540000.usb/params:power_down : 0 > /sys/kernel/debug/ff580000.usb/params:power_down : 0 > > So I'm really quite convinced that on mainline Linux with > DWC2_POWER_DOWN_PARAM_NONE that dwc2_port_suspend() sets HPRT0_SUSP > and PCGCTL_STOPPCLK. > > >>> ...I believe that the net effect of my patch ends up doing both those >>> same two things in hcd_suspend. That is: when power_down is >>> DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the >>> same thing that dwc2_port_suspend() would do in the same case. Is >>> that not OK? >> No if your patch is doing the same thing as it was doing before what is >> the purpose of the patch ? > > The purpose is to make _dwc2_hcd_suspend() work more correctly in the > case where power_down is DWC2_POWER_DOWN_PARAM_NONE > > >> My testes show that your patch doesn't fix the issue related partial >> power down. > > Right. I have been trying to say that my patch doesn't do anything at > all for partial power down. I am simply trying to make > DWC2_POWER_DOWN_PARAM_NONE work more correctly. > > I haven't run all the power consumption tests in quite a long time and > I'll try to get it hooked up tomorrow to confirm that my patch really > truly is still needed to help with power consumption. I did confirm > that at least there are cases where _dwc2_hcd_suspend() is called and > my patch is what sets the important bits. > > -Doug >
On 5/2/2019 03:58, Doug Anderson wrote: > Hi, > > > On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote: >> >> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus >> suspend/resume for dwc2") on ToT. That commit was reverted in commit >> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") >> because apparently it broke the Altera SOCFPGA. >> >> With all the changes that have happened to dwc2 in the meantime, it's >> possible that the Altera SOCFPGA will just magically work with this >> change now. ...and it would be good to get bus suspend/resume >> implemented. >> >> This change is a forward port of one that's been living in the Chrome >> OS 3.14 kernel tree. >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- >> This patch was last posted at: >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e= >> >> ...and appears to have died the death of silence. Maybe it could get >> some bake time in linuxnext if we can't find any proactive testing? >> >> I will also freely admit that I don't know tons about the theory >> behind this patch. I'm mostly just re-hashing the original commit >> from Kever that was reverted since: >> * Turning on partial power down on rk3288 doesn't "just work". I >> don't get hotplug events. This is despite dwc2 auto-detecting that >> we are power optimized. >> * If we don't do something like this commit we don't get into as low >> of a power mode. > > OK, I spent the day digging more into this patch to confirm that it's > really the right thing to do. ...and it still seems to be. > > First off: I'm pretty sure the above sentence "If we don't do > something like this commit we don't get into as low of a power mode." > is totally wrong. Luckily it's "after the cut" and not part of the > commit message. Specifically I did a bunch of power testing and I > couldn't find any instance saving power after this patch. > > ...but, then I looked more carefully at all the history of this > commit. I ended up at: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e= Looking at this code review I see that this patch fixes whatever issues you have on Chrome OS 3.14. But your patch has landed on the top of latest Kernel version. With the latest version I think you would not have the regression issue. So you are fixing Chrome OS 3.14. > > ...where I said that this fixes a resume speed regression. More > details could be found at the linked bug, AKA: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.chromium.org_p_chromium_issues_detail-3Fid-3D548336&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=7gK8ZGX2zZPqC98CDMhqxEY3Acm_TbYa3fpQjWtvexM&e= > > ...but, sadly, I wasn't as verbose as I usually am and didn't describe > my exact testing setup. So I tried to reproduce. ...and I was able > to. I tested on an rk3288-veyron-jerry with an empty USB hub plugged > into the left port (the host port) and my "servo 2" debug board hooked > up to the right port. The "power_Resume" test in Chrome OS certainly > showed a regression in 3.14 when doing a suspend/resume cycle. > > > Digging into the logs in 3.14, before this patch I saw this in the logs: > > usb 3-1: reset high-speed USB device number 2 using dwc2 > usb 3-1.7: reset high-speed USB device number 3 using dwc2 > > ...after this patch: > > usb 3-1: USB disconnect, device number 2 > usb 3-1.7: USB disconnect, device number 3 > usb 3-1: new high-speed USB device number 4 using dwc2 > usb 3-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00 > usb 3-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0 > usb 3-1: Product: USB 2.0 Hub [MTT] > usb 3-1.7: new high-speed USB device number 5 using dwc2 > usb 3-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11 > usb 3-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0 > usb 3-1.7: Product: USB 2.0 Hub > > ...so basically my belief is that without this patch we're just sorta > leaving the device hanging and it get confused on resume. After this > patch we behave slightly better. > > I tested on 4.19 and found much the same. There: > > usb 2-1: reset high-speed USB device number 2 using dwc2 > usb 2-1.7: reset high-speed USB device number 3 using dwc2 > > vs. > > usb 2-1.7: USB disconnect, device number 3 > usb 2-1: USB disconnect, device number 2 > usb 2-1: new high-speed USB device number 4 using dwc2 > usb 2-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00 > usb 2-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0 > usb 2-1: Product: USB 2.0 Hub [MTT] > usb 2-1.7: new high-speed USB device number 5 using dwc2 > usb 2-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11 > usb 2-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0 > usb 2-1.7: Product: USB 2.0 Hub > > > On 4.19 I didn't actually notice a the same resume time regression, > presumably because things are happening more asynchronously there (I > didn't confirm this). ...but in any case it seems like the right > thing to do to actually do the suspend. > > > I'll also re-iterate once more that I'm not claiming that my patch > helps with "partial power down". It merely makes the "power savings > disabled" case work more properly. > > > I'll also note that my patch is already in Felipe's "testing/next" > branch which I continue to believe is correct and good. > > -Doug >
Hi, On Fri, May 3, 2019 at 1:20 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > On 5/1/2019 05:57, Doug Anderson wrote: > > Hi, > > > > On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan > > <Arthur.Petrosyan@synopsys.com> wrote: > >> > >> Hi, > >> > >> On 4/29/2019 21:34, Doug Anderson wrote: > >>> Hi, > >>> > >>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan > >>> <Arthur.Petrosyan@synopsys.com> wrote: > >>>> > >>>> Hi, > >>>> > >>>> On 4/18/2019 04:15, Douglas Anderson wrote: > >>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus > >>>>> suspend/resume for dwc2") on ToT. That commit was reverted in commit > >>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") > >>>>> because apparently it broke the Altera SOCFPGA. > >>>>> > >>>>> With all the changes that have happened to dwc2 in the meantime, it's > >>>>> possible that the Altera SOCFPGA will just magically work with this > >>>>> change now. ...and it would be good to get bus suspend/resume > >>>>> implemented. > >>>>> > >>>>> This change is a forward port of one that's been living in the Chrome > >>>>> OS 3.14 kernel tree. > >>>>> > >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> > >>>>> --- > >>>>> This patch was last posted at: > >>>>> > >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e= > >>>>> > >>>>> ...and appears to have died the death of silence. Maybe it could get > >>>>> some bake time in linuxnext if we can't find any proactive testing? > >>>>> > >>>>> I will also freely admit that I don't know tons about the theory > >>>>> behind this patch. I'm mostly just re-hashing the original commit > >>>>> from Kever that was reverted since: > >>>>> * Turning on partial power down on rk3288 doesn't "just work". I > >>>>> don't get hotplug events. This is despite dwc2 auto-detecting that > >>>>> we are power optimized. > >>>> What do you mean by doesn't "just work" ? It seem to me that even after > >>>> adding this patch you don't get issues fixed. > >>>> You mention that you don't get the hotplug events. Please provide dwc2 > >>>> debug logs and register dumps on this issue. > >>> > >>> I mean that partial power down in the currently upstream driver > >>> doesn't work. AKA: if I turn on partial power down in the upstream > >>> driver then hotplug events break. I can try to provide some logs. On > >>> what exact version of the code do you want logs? Just your series? > >>> Just my series? Mainline? Some attempt at combining both series? As > >>> I said things seem to sorta work with the combined series. I can try > >>> to clarify if that's the series you want me to test with. ...or I can > >>> wait for your next version? > >> As I said this patch doesn't fix the issue with hotplug. With this patch > >> or without the hotplug behaves as it was. I have tested it on our setup. > >> > >> Have you debugged your patch? Does it make any difference on your setup > >> ? Does it fix the issue with hotplug? > > > > I think we're still not taking on the same page. > > > > My patch makes no attempt to make partial power down mode work. My > > patch attempts to make things work a little better when using > > DWC2_POWER_DOWN_PARAM_NONE. There is no use testing my patch with > > partial power down as it shouldn't have any impact there. > > > > > >>> I am by no means an expert on dwc2, but an assumption made in my patch > >>> is that even cores that can't support partial power down can still > >>> save some amount of power when hcd_suspend is called. > >> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ? > >>> > >>> Some evidence that this should be possible: looking at mainline Linux > >>> and at dwc2_port_suspend(), I see: > >>> > >>> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE > >> Currently (without your and my patches) (looking at mainline Linux) the > >> function dwc2_port_suspend() is called anyway because its call is issued > >> by the system. But it performs entering to suspend only in case of > >> DWC2_POWER_DOWN_PARAM_PARTIAL. > >> > >> This is not an assumption. What I am pointing out is based on debugging > >> and before making assumptions without debugging for me seems not ok. > >> > >> Currently without your patch and without my patches. In the > >> dwc2_port_suspend() it will enter to suspend only in case that > >> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the > >> code more carefully you will see > >> > >> if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) > >> goto skip_power_saving; > >> > >> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip > >> power saving. > >> > >> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it > >> tries to suspend. > > > > We must be looking at different code. I'm looking at Linux's tree, AKA: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3488&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=IWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=AHu2iOKkybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e= > Here you are looking at the old code. After that there are several of > changes related to suspend/resume functions. In my email, see that I said I actually checked out mainline kernel (and I gave you the exact version: "v5.1-rc7-5-g83a50840e72a") and added printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and PCGCTL_STOPPCLK in dwc2_port_suspend(). [ 454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP [ 454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK The version "v5.1-rc7-5-g83a50840e72a" is not old code. > This is the link to the code with changes. Latest version of those > functions. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n4489 > > Your changes are sitting on that latest version of code. Not the old > version of it. You are pointing me at _dwc2_hcd_suspend() whereas I pointed at dwc2_port_suspend(). Why? I am saying that dwc2_port_suspend() _does_ set "HPRT0_SUSP" and "PCGCTL_STOPPCLK" even with DWC2_POWER_DOWN_PARAM_NONE. Do you disagree? I completely agree that on mainline _dwc2_hcd_suspend() _does not_ set these bits with DWC2_POWER_DOWN_PARAM_NONE. That is what my patch fixes. -Doug
Hi, On Fri, May 3, 2019 at 1:25 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > On 5/2/2019 03:58, Doug Anderson wrote: > > Hi, > > > > > > On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote: > >> > >> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus > >> suspend/resume for dwc2") on ToT. That commit was reverted in commit > >> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") > >> because apparently it broke the Altera SOCFPGA. > >> > >> With all the changes that have happened to dwc2 in the meantime, it's > >> possible that the Altera SOCFPGA will just magically work with this > >> change now. ...and it would be good to get bus suspend/resume > >> implemented. > >> > >> This change is a forward port of one that's been living in the Chrome > >> OS 3.14 kernel tree. > >> > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > >> --- > >> This patch was last posted at: > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e= > >> > >> ...and appears to have died the death of silence. Maybe it could get > >> some bake time in linuxnext if we can't find any proactive testing? > >> > >> I will also freely admit that I don't know tons about the theory > >> behind this patch. I'm mostly just re-hashing the original commit > >> from Kever that was reverted since: > >> * Turning on partial power down on rk3288 doesn't "just work". I > >> don't get hotplug events. This is despite dwc2 auto-detecting that > >> we are power optimized. > >> * If we don't do something like this commit we don't get into as low > >> of a power mode. > > > > OK, I spent the day digging more into this patch to confirm that it's > > really the right thing to do. ...and it still seems to be. > > > > First off: I'm pretty sure the above sentence "If we don't do > > something like this commit we don't get into as low of a power mode." > > is totally wrong. Luckily it's "after the cut" and not part of the > > commit message. Specifically I did a bunch of power testing and I > > couldn't find any instance saving power after this patch. > > > > ...but, then I looked more carefully at all the history of this > > commit. I ended up at: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e= > Looking at this code review I see that this patch fixes whatever issues > you have on Chrome OS 3.14. But your patch has landed on the top of > latest Kernel version. With the latest version I think you would not > have the regression issue. > So you are fixing Chrome OS 3.14. I'm confused why you ignored the rest of my email where I said I also ported it to 4.19 (which, from a dwc2 host point of view, is pretty much mainline) and saw that the patch was still needed. -Doug
Hi, On 5/3/2019 19:08, Doug Anderson wrote: > Hi, > > On Fri, May 3, 2019 at 1:25 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> On 5/2/2019 03:58, Doug Anderson wrote: >>> Hi, >>> >>> >>> On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote: >>>> >>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus >>>> suspend/resume for dwc2") on ToT. That commit was reverted in commit >>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") >>>> because apparently it broke the Altera SOCFPGA. >>>> >>>> With all the changes that have happened to dwc2 in the meantime, it's >>>> possible that the Altera SOCFPGA will just magically work with this >>>> change now. ...and it would be good to get bus suspend/resume >>>> implemented. >>>> >>>> This change is a forward port of one that's been living in the Chrome >>>> OS 3.14 kernel tree. >>>> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>> --- >>>> This patch was last posted at: >>>> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e= >>>> >>>> ...and appears to have died the death of silence. Maybe it could get >>>> some bake time in linuxnext if we can't find any proactive testing? >>>> >>>> I will also freely admit that I don't know tons about the theory >>>> behind this patch. I'm mostly just re-hashing the original commit >>>> from Kever that was reverted since: >>>> * Turning on partial power down on rk3288 doesn't "just work". I >>>> don't get hotplug events. This is despite dwc2 auto-detecting that >>>> we are power optimized. >>>> * If we don't do something like this commit we don't get into as low >>>> of a power mode. >>> >>> OK, I spent the day digging more into this patch to confirm that it's >>> really the right thing to do. ...and it still seems to be. >>> >>> First off: I'm pretty sure the above sentence "If we don't do >>> something like this commit we don't get into as low of a power mode." >>> is totally wrong. Luckily it's "after the cut" and not part of the >>> commit message. Specifically I did a bunch of power testing and I >>> couldn't find any instance saving power after this patch. >>> >>> ...but, then I looked more carefully at all the history of this >>> commit. I ended up at: >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e= >> Looking at this code review I see that this patch fixes whatever issues >> you have on Chrome OS 3.14. But your patch has landed on the top of >> latest Kernel version. With the latest version I think you would not >> have the regression issue. >> So you are fixing Chrome OS 3.14. > > I'm confused why you ignored the rest of my email where I said I also > ported it to 4.19 (which, from a dwc2 host point of view, is pretty > much mainline) and saw that the patch was still needed. I didn't ignore it just I had to perform testes and reply to it with another email. > > -Doug > I spent yesterday debugging and performing testes with Linux Mainline. So when we don't have any of power saving modes supported and the power_down is DWC2_POWER_DOWN_PARAM_NONE. We can set "PCGCTL_STOPPCLK" bit whenever there is suspend ( Checked the programming guide and data book). I have not seen any case that this affects the flow. I have not been able to see if after setting "PCGCTL_STOPPCLK" bit there is any power saved or driver behaved differently. Maybe it is platform depended . However, there is a possibility that this might save power. So as this is not breaking anything. I am ok with this patch.
On 5/3/2019 19:04, Doug Anderson wrote: > Hi, > > On Fri, May 3, 2019 at 1:20 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> On 5/1/2019 05:57, Doug Anderson wrote: >>> Hi, >>> >>> On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan >>> <Arthur.Petrosyan@synopsys.com> wrote: >>>> >>>> Hi, >>>> >>>> On 4/29/2019 21:34, Doug Anderson wrote: >>>>> Hi, >>>>> >>>>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan >>>>> <Arthur.Petrosyan@synopsys.com> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 4/18/2019 04:15, Douglas Anderson wrote: >>>>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus >>>>>>> suspend/resume for dwc2") on ToT. That commit was reverted in commit >>>>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") >>>>>>> because apparently it broke the Altera SOCFPGA. >>>>>>> >>>>>>> With all the changes that have happened to dwc2 in the meantime, it's >>>>>>> possible that the Altera SOCFPGA will just magically work with this >>>>>>> change now. ...and it would be good to get bus suspend/resume >>>>>>> implemented. >>>>>>> >>>>>>> This change is a forward port of one that's been living in the Chrome >>>>>>> OS 3.14 kernel tree. >>>>>>> >>>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>>>>> --- >>>>>>> This patch was last posted at: >>>>>>> >>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e= >>>>>>> >>>>>>> ...and appears to have died the death of silence. Maybe it could get >>>>>>> some bake time in linuxnext if we can't find any proactive testing? >>>>>>> >>>>>>> I will also freely admit that I don't know tons about the theory >>>>>>> behind this patch. I'm mostly just re-hashing the original commit >>>>>>> from Kever that was reverted since: >>>>>>> * Turning on partial power down on rk3288 doesn't "just work". I >>>>>>> don't get hotplug events. This is despite dwc2 auto-detecting that >>>>>>> we are power optimized. >>>>>> What do you mean by doesn't "just work" ? It seem to me that even after >>>>>> adding this patch you don't get issues fixed. >>>>>> You mention that you don't get the hotplug events. Please provide dwc2 >>>>>> debug logs and register dumps on this issue. >>>>> >>>>> I mean that partial power down in the currently upstream driver >>>>> doesn't work. AKA: if I turn on partial power down in the upstream >>>>> driver then hotplug events break. I can try to provide some logs. On >>>>> what exact version of the code do you want logs? Just your series? >>>>> Just my series? Mainline? Some attempt at combining both series? As >>>>> I said things seem to sorta work with the combined series. I can try >>>>> to clarify if that's the series you want me to test with. ...or I can >>>>> wait for your next version? >>>> As I said this patch doesn't fix the issue with hotplug. With this patch >>>> or without the hotplug behaves as it was. I have tested it on our setup. >>>> >>>> Have you debugged your patch? Does it make any difference on your setup >>>> ? Does it fix the issue with hotplug? >>> >>> I think we're still not taking on the same page. >>> >>> My patch makes no attempt to make partial power down mode work. My >>> patch attempts to make things work a little better when using >>> DWC2_POWER_DOWN_PARAM_NONE. There is no use testing my patch with >>> partial power down as it shouldn't have any impact there. >>> >>> >>>>> I am by no means an expert on dwc2, but an assumption made in my patch >>>>> is that even cores that can't support partial power down can still >>>>> save some amount of power when hcd_suspend is called. >>>> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ? >>>>> >>>>> Some evidence that this should be possible: looking at mainline Linux >>>>> and at dwc2_port_suspend(), I see: >>>>> >>>>> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE >>>> Currently (without your and my patches) (looking at mainline Linux) the >>>> function dwc2_port_suspend() is called anyway because its call is issued >>>> by the system. But it performs entering to suspend only in case of >>>> DWC2_POWER_DOWN_PARAM_PARTIAL. >>>> >>>> This is not an assumption. What I am pointing out is based on debugging >>>> and before making assumptions without debugging for me seems not ok. >>>> >>>> Currently without your patch and without my patches. In the >>>> dwc2_port_suspend() it will enter to suspend only in case that >>>> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the >>>> code more carefully you will see >>>> >>>> if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) >>>> goto skip_power_saving; >>>> >>>> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip >>>> power saving. >>>> >>>> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it >>>> tries to suspend. >>> >>> We must be looking at different code. I'm looking at Linux's tree, AKA: >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3488&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=IWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=AHu2iOKkybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e= >> Here you are looking at the old code. After that there are several of >> changes related to suspend/resume functions. > > In my email, see that I said I actually checked out mainline kernel > (and I gave you the exact version: "v5.1-rc7-5-g83a50840e72a") and > added printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP > and PCGCTL_STOPPCLK in dwc2_port_suspend(). I was talking about your patch which edits _dwc2_hcd_suspend() function. What dwc2_port_suspend() implements for the hibernation or partial power down I have not tested. It is a different implementation. > > [ 454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP > [ 454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK > > The version "v5.1-rc7-5-g83a50840e72a" is not old code. > > >> This is the link to the code with changes. Latest version of those >> functions. >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n4489&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=4lskGlSdN5lp0MXVy1SP0zxzFtbPMSUoQKQzLLmfoOg&s=A-ikUNQklzouItTOQB078WrOiMcfCqgw8sBgLFwtyTU&e= >> >> Your changes are sitting on that latest version of code. Not the old >> version of it. > > You are pointing me at _dwc2_hcd_suspend() whereas I pointed at > dwc2_port_suspend(). Why? Because your patch is editing _dwc2_hcd_suspend(). > > I am saying that dwc2_port_suspend() _does_ set "HPRT0_SUSP" and > "PCGCTL_STOPPCLK" even with DWC2_POWER_DOWN_PARAM_NONE. Do you > disagree? Yes dwc2_port_suspend() does but that is a different implementation > > I completely agree that on mainline _dwc2_hcd_suspend() _does not_ set > these bits with DWC2_POWER_DOWN_PARAM_NONE. That is what my patch > fixes. Yes so I was pointing to that. > > > -Doug > I have performed testes and made sure that the patch is ok. It is just that I am not sure if it does really save power. I have not been able to see if any power is saved. So I am ok with this patch.
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index e272d020012e..978232a9e4a8 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -4482,6 +4482,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) unsigned long flags; int ret = 0; u32 hprt0; + u32 pcgctl; spin_lock_irqsave(&hsotg->lock, flags); @@ -4497,7 +4498,7 @@ 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) goto skip_power_saving; /* @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) */ 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); + if (hprt0 & HPRT0_CONNSTS) { + hprt0 |= HPRT0_SUSP; + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) + hprt0 &= ~HPRT0_PWR; + dwc2_writel(hsotg, hprt0, HPRT0); + } + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { + spin_unlock_irqrestore(&hsotg->lock, flags); + dwc2_vbus_supply_exit(hsotg); + spin_lock_irqsave(&hsotg->lock, flags); + } else { + pcgctl = readl(hsotg->regs + PCGCTL); + pcgctl |= PCGCTL_STOPPCLK; + writel(pcgctl, hsotg->regs + PCGCTL); + } } - /* 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; + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { + /* 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; + } + + /* After entering partial_power_down, hardware is no more accessible */ + clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); } /* Ask phy to be suspended */ @@ -4530,9 +4545,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) spin_lock_irqsave(&hsotg->lock, flags); } - /* After entering partial_power_down, hardware is no more accessible */ - clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); - skip_power_saving: hsotg->lx_state = DWC2_L2; unlock: @@ -4545,6 +4557,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) { struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd); unsigned long flags; + u32 pcgctl; int ret = 0; spin_lock_irqsave(&hsotg->lock, flags); @@ -4555,17 +4568,11 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) if (hsotg->lx_state != DWC2_L2) goto unlock; - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) { + if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) { hsotg->lx_state = DWC2_L0; goto unlock; } - /* - * Set HW accessible bit before powering on the controller - * since an interrupt may rise. - */ - set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); - /* * Enable power if not already done. * This must not be spinlocked since duration @@ -4577,10 +4584,23 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) spin_lock_irqsave(&hsotg->lock, flags); } - /* Exit partial_power_down */ - ret = dwc2_exit_partial_power_down(hsotg, true); - if (ret && (ret != -ENOTSUPP)) - dev_err(hsotg->dev, "exit partial_power_down failed\n"); + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { + /* + * Set HW accessible bit before powering on the controller + * since an interrupt may rise. + */ + set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); + + + /* Exit partial_power_down */ + ret = dwc2_exit_partial_power_down(hsotg, true); + if (ret && (ret != -ENOTSUPP)) + dev_err(hsotg->dev, "exit partial_power_down failed\n"); + } else { + pcgctl = readl(hsotg->regs + PCGCTL); + pcgctl &= ~PCGCTL_STOPPCLK; + writel(pcgctl, hsotg->regs + PCGCTL); + } hsotg->lx_state = DWC2_L0; @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) spin_unlock_irqrestore(&hsotg->lock, flags); dwc2_port_resume(hsotg); } else { - dwc2_vbus_supply_init(hsotg); + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) { + dwc2_vbus_supply_init(hsotg); - /* Wait for controller to correctly update D+/D- level */ - usleep_range(3000, 5000); + /* Wait for controller to correctly update D+/D- level */ + usleep_range(3000, 5000); + } /* * Clear Port Enable and Port Status changes.
This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus suspend/resume for dwc2") on ToT. That commit was reverted in commit b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"") because apparently it broke the Altera SOCFPGA. With all the changes that have happened to dwc2 in the meantime, it's possible that the Altera SOCFPGA will just magically work with this change now. ...and it would be good to get bus suspend/resume implemented. This change is a forward port of one that's been living in the Chrome OS 3.14 kernel tree. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- This patch was last posted at: https://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org ...and appears to have died the death of silence. Maybe it could get some bake time in linuxnext if we can't find any proactive testing? I will also freely admit that I don't know tons about the theory behind this patch. I'm mostly just re-hashing the original commit from Kever that was reverted since: * Turning on partial power down on rk3288 doesn't "just work". I don't get hotplug events. This is despite dwc2 auto-detecting that we are power optimized. * If we don't do something like this commit we don't get into as low of a power mode. Changes in v2: None drivers/usb/dwc2/hcd.c | 84 ++++++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 31 deletions(-)