| Message ID | c62b99a6e4ad6de2982de988e9f9bcd0c6ec4daa.1555672441.git.arturp@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc2: Fix and improve power saving modes. | expand |
Hi, On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Added a new flow of entering and exiting hibernation when PC is > hibernated or suspended. > > Signed-off-by: Artur Petrosyan <arturp@synopsys.com> > --- > 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)) > + if (!hsotg->bus_suspended) As per my comments above I don't have a good grasp on what "bus_suspended" is for. ...that being said, if your change here is actually correct then you probably (?) want to remove the "if (hsotg->bus_suspended)" check later in this same function. Said another way, you've now got code that looks like: if (!hsotg->bus_suspended) goto exit; /* code that I think doesn't touch bus_suspended */ if (hsotg->bus_suspended) { /* do something */ } else { /* do something else */ } Presumably the "do something" is now dead code? > goto unlock; > > if (hsotg->lx_state != DWC2_L2) > goto unlock; > > - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL || > + hprt0 = dwc2_read_hprt0(hsotg); > + > + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE || > hprt0 & HPRT0_CONNSTS) { > hsotg->lx_state = DWC2_L0; > goto unlock; > @@ -4597,36 +4616,51 @@ 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"); > + switch (hsotg->params.power_down) { > + case DWC2_POWER_DOWN_PARAM_PARTIAL: > > - hsotg->lx_state = DWC2_L0; > + /* 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"); > > - spin_unlock_irqrestore(&hsotg->lock, flags); > + hsotg->lx_state = DWC2_L0; > > - if (hsotg->bus_suspended) { > - spin_lock_irqsave(&hsotg->lock, flags); > - hsotg->flags.b.port_suspend_change = 1; > spin_unlock_irqrestore(&hsotg->lock, flags); > - dwc2_port_resume(hsotg); > - } else { > - dwc2_vbus_supply_init(hsotg); > > - /* Wait for controller to correctly update D+/D- level */ > - usleep_range(3000, 5000); > + if (hsotg->bus_suspended) { > + spin_lock_irqsave(&hsotg->lock, flags); > + hsotg->flags.b.port_suspend_change = 1; > + spin_unlock_irqrestore(&hsotg->lock, flags); > + dwc2_port_resume(hsotg); > + } else { > + dwc2_vbus_supply_init(hsotg); > > - /* > - * Clear Port Enable and Port Status changes. > - * Enable Port Power. > - */ > - dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET | > + /* Wait for controller to correctly update D+/D- level */ > + usleep_range(3000, 5000); > + > + /* > + * Clear Port Enable and Port Status changes. > + * Enable Port Power. > + */ > + dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET | > HPRT0_ENACHG, HPRT0); > - /* Wait for controller to detect Port Connect */ > - usleep_range(5000, 7000); > + /* Wait for controller to detect Port Connect */ > + usleep_range(5000, 7000); > + } > + break; > + case DWC2_POWER_DOWN_PARAM_HIBERNATION: > + > + /* Exit host hibernation. */ > + if (hsotg->hibernated) > + dwc2_exit_hibernation(hsotg, 0, 0, 1); > + break; > + default: > + goto unlock; > } > > + spin_unlock_irqrestore(&hsotg->lock, flags); > + I'm pretty curious if you tested DWC2_POWER_DOWN_PARAM_PARTIAL after applying your patch series. As far as I can tell your switch statement for DWC2_POWER_DOWN_PARAM_PARTIAL will "break" with the spinlock already unlocked. ...so you'll run spin_unlock_irqrestore twice. Is that really legit? NOTE: in case it helps you, I've attempted to rebase this atop my series. Please double-check that I didn't mess anything up, though. https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/sandbox/dianders/190426-dwc2-stuff ...with that a quick test seems to show that partial power down is sorta working on rk3288 now. I _think_ I saw one case where hotplug failed but I've seen several where it works. ...unfortunately it seems to break when I do hotplug on the port where I have "snps,reset-phy-on-wake" set. -Doug
Hi, On 4/27/2019 01:01, Doug Anderson wrote: > Hi, > > On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> Added a new flow of entering and exiting hibernation when PC is >> hibernated or suspended. >> >> Signed-off-by: Artur Petrosyan <arturp@synopsys.com> >> --- >> 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(). > > >> + /* 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 *". This code comes form previous implementations I have not touched it not to back anything. > > 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? When there is an error power_saving should be skipped. > > >> + } 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...Enter hibernation implements setting bus_suspend so I don't touch this. Actually this patch set fixes issues it doesn't clean up everything related to hibernation or partial power down. > > >> /* 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)) >> + if (!hsotg->bus_suspended) > > As per my comments above I don't have a good grasp on what > "bus_suspended" is for. ...that being said, if your change here is > actually correct then you probably (?) want to remove the "if > (hsotg->bus_suspended)" check later in this same function. > > Said another way, you've now got code that looks like: > > if (!hsotg->bus_suspended) > goto exit; > > /* code that I think doesn't touch bus_suspended */ > > if (hsotg->bus_suspended) { > /* do something */ > } else { > /* do something else */ > } > > Presumably the "do something" is now dead code? > That part is not dad because if hsotg->bus_suspended is true resuming/exiting from suspend/partial power down/hibernation should be performed. On the other hand if hsotg->bus_suspended is false there is no need to resume. So of course if core is not suspended the code responsible for resuming should not be called. In that sense the code can be called dead. > >> goto unlock; >> >> if (hsotg->lx_state != DWC2_L2) >> goto unlock; >> >> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL || >> + hprt0 = dwc2_read_hprt0(hsotg); >> + >> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE || >> hprt0 & HPRT0_CONNSTS) { >> hsotg->lx_state = DWC2_L0; >> goto unlock; >> @@ -4597,36 +4616,51 @@ 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"); >> + switch (hsotg->params.power_down) { >> + case DWC2_POWER_DOWN_PARAM_PARTIAL: >> >> - hsotg->lx_state = DWC2_L0; >> + /* 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"); >> >> - spin_unlock_irqrestore(&hsotg->lock, flags); >> + hsotg->lx_state = DWC2_L0; >> >> - if (hsotg->bus_suspended) { >> - spin_lock_irqsave(&hsotg->lock, flags); >> - hsotg->flags.b.port_suspend_change = 1; >> spin_unlock_irqrestore(&hsotg->lock, flags); >> - dwc2_port_resume(hsotg); >> - } else { >> - dwc2_vbus_supply_init(hsotg); >> >> - /* Wait for controller to correctly update D+/D- level */ >> - usleep_range(3000, 5000); >> + if (hsotg->bus_suspended) { >> + spin_lock_irqsave(&hsotg->lock, flags); >> + hsotg->flags.b.port_suspend_change = 1; >> + spin_unlock_irqrestore(&hsotg->lock, flags); >> + dwc2_port_resume(hsotg); >> + } else { >> + dwc2_vbus_supply_init(hsotg); >> >> - /* >> - * Clear Port Enable and Port Status changes. >> - * Enable Port Power. >> - */ >> - dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET | >> + /* Wait for controller to correctly update D+/D- level */ >> + usleep_range(3000, 5000); >> + >> + /* >> + * Clear Port Enable and Port Status changes. >> + * Enable Port Power. >> + */ >> + dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET | >> HPRT0_ENACHG, HPRT0); >> - /* Wait for controller to detect Port Connect */ >> - usleep_range(5000, 7000); >> + /* Wait for controller to detect Port Connect */ >> + usleep_range(5000, 7000); >> + } >> + break; >> + case DWC2_POWER_DOWN_PARAM_HIBERNATION: >> + >> + /* Exit host hibernation. */ >> + if (hsotg->hibernated) >> + dwc2_exit_hibernation(hsotg, 0, 0, 1); >> + break; >> + default: >> + goto unlock; >> } >> >> + spin_unlock_irqrestore(&hsotg->lock, flags); >> + > > I'm pretty curious if you tested DWC2_POWER_DOWN_PARAM_PARTIAL after > applying your patch series. As far as I can tell your switch > statement for DWC2_POWER_DOWN_PARAM_PARTIAL will "break" with the > spinlock already unlocked. ...so you'll run spin_unlock_irqrestore > twice. Is that really legit? I have tested the patches on HAPS-DX and Linaro HiKey 960 boards. > > > > NOTE: in case it helps you, I've attempted to rebase this atop my > series. Please double-check that I didn't mess anything up, though > > https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2Blog_refs_sandbox_dianders_190426-2Ddwc2-2Dstuff&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=2xJPvFyQhKsjBCMYd6_Kpb7_r7HPUAoeJHcQmBH80Gc&s=wEgbdpKgC9t9CfXpI8awXLos9GuRcsNEkpW09dgMxiw&e= > > > ...with that a quick test seems to show that partial power down is > sorta working on rk3288 now. I _think_ I saw one case where hotplug > failed but I've seen several where it works. ...unfortunately it > seems to break when I do hotplug on the port where I have > "snps,reset-phy-on-wake" set. You can provide debug logs for that scenario I will try to help you fix issues with that. > > -Doug >
Hi, On Mon, Apr 29, 2019 at 5:01 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Hi, > > On 4/27/2019 01:01, Doug Anderson wrote: > > Hi, > > > > On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan > > <Arthur.Petrosyan@synopsys.com> wrote: > >> > >> Added a new flow of entering and exiting hibernation when PC is > >> hibernated or suspended. > >> > >> Signed-off-by: Artur Petrosyan <arturp@synopsys.com> > >> --- > >> 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 */ > >> + 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 *". > This code comes form previous implementations I have not touched it not > to back anything. Please fix. Even if you had internal code that did this it still needs to be fixed when going upstream. It is highly unlikely you'll break something when removing something like this. > > 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? > When there is an error power_saving should be skipped. OK, so you agree? > >> + } 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... > Enter hibernation implements setting bus_suspend so I don't touch this. > Actually this patch set fixes issues it doesn't clean up everything > related to hibernation or partial power down. Yet more asymmetry. > >> /* 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)) > >> + if (!hsotg->bus_suspended) > > > > As per my comments above I don't have a good grasp on what > > "bus_suspended" is for. ...that being said, if your change here is > > actually correct then you probably (?) want to remove the "if > > (hsotg->bus_suspended)" check later in this same function. > > > > Said another way, you've now got code that looks like: > > > > if (!hsotg->bus_suspended) > > goto exit; > > > > /* code that I think doesn't touch bus_suspended */ > > > > if (hsotg->bus_suspended) { > > /* do something */ > > } else { > > /* do something else */ > > } > > > > Presumably the "do something" is now dead code? > > > That part is not dad because if hsotg->bus_suspended is true > resuming/exiting from suspend/partial power down/hibernation should be > performed. > On the other hand if hsotg->bus_suspended is false there is no need to > resume. > > So of course if core is not suspended the code responsible for resuming > should not be called. In that sense the code can be called dead. I think I didn't explain it well. Does this patch help you? https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4e84efdbeb74bcb8b24e2b1fea24153981acc185%5E%21/ > >> + spin_unlock_irqrestore(&hsotg->lock, flags); > >> + > > > > I'm pretty curious if you tested DWC2_POWER_DOWN_PARAM_PARTIAL after > > applying your patch series. As far as I can tell your switch > > statement for DWC2_POWER_DOWN_PARAM_PARTIAL will "break" with the > > spinlock already unlocked. ...so you'll run spin_unlock_irqrestore > > twice. Is that really legit? > I have tested the patches on HAPS-DX and Linaro HiKey 960 boards. How is it possible that you don't end up spin unlocking more than once? This seems like a pretty serious problem. My guess is that whatever tests you ran didn't actually exercise this function. Personally I could only get it to exercise by doing suspend/resume. ...and when I did, sure enough I saw: BUG: spinlock already unlocked on CPU#2, kworker/u8:32/5812 > > ...with that a quick test seems to show that partial power down is > > sorta working on rk3288 now. I _think_ I saw one case where hotplug > > failed but I've seen several where it works. ...unfortunately it > > seems to break when I do hotplug on the port where I have > > "snps,reset-phy-on-wake" set. > You can provide debug logs for that scenario I will try to help you fix > issues with that. All you need to do is make sure you run the function _dwc2_hcd_suspend() with power down mode set to "DWC2_POWER_DOWN_PARAM_PARTIAL". ...or walk through the code and see that spin_unlock_irqrestore() will certainly be called twice. -Doug
On 4/29/2019 21:43, Doug Anderson wrote: > Hi, > > On Mon, Apr 29, 2019 at 5:01 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> Hi, >> >> On 4/27/2019 01:01, Doug Anderson wrote: >>> Hi, >>> >>> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan >>> <Arthur.Petrosyan@synopsys.com> wrote: >>>> >>>> Added a new flow of entering and exiting hibernation when PC is >>>> hibernated or suspended. >>>> >>>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com> >>>> --- >>>> 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() So what? why should dwc2_enter_partial_power_down() be called when we have power_down == DWC2_POWER_DOWN_PARAM_HIBERNATION ? So what the switch statement basically does is that it checks if core supports partial power down then it calls dwc2_enter_partial_power_down() if core supports hibernation it calls dwc2_enter_hibernation(); > > > This is all part of the whole asymmetry between PARTIAL and > HIBERNATION that makes it hard to understand. See partial power down and hibernation are actually two separate implementations. 1. When core supports partial power down we enter to partial power down. 2. When core supports Hibernation we enter to Hibernation 3. If core doesn't support none of them we just skip the power saving. So the switch statement checks the availability of one of the power saving modes and then calls either dwc2_enter_hibernation() or dwc2_enter_partial_power_down(). > > >>>> + /* 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 *". >> This code comes form previous implementations I have not touched it not >> to back anything. > > Please fix. Even if you had internal code that did this it still > needs to be fixed when going upstream. It is highly unlikely you'll > break something when removing something like this. > > > >>> 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? >> When there is an error power_saving should be skipped. > > OK, so you agree? Agree that if there is an error en entering to hibernation then power saving should be skipped. > > >>>> + } 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... >> Enter hibernation implements setting bus_suspend so I don't touch this. >> Actually this patch set fixes issues it doesn't clean up everything >> related to hibernation or partial power down. > > Yet more asymmetry. > > >>>> /* 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)) >>>> + if (!hsotg->bus_suspended) >>> >>> As per my comments above I don't have a good grasp on what >>> "bus_suspended" is for. ...that being said, if your change here is >>> actually correct then you probably (?) want to remove the "if >>> (hsotg->bus_suspended)" check later in this same function. >>> >>> Said another way, you've now got code that looks like: >>> >>> if (!hsotg->bus_suspended) >>> goto exit; >>> >>> /* code that I think doesn't touch bus_suspended */ >>> >>> if (hsotg->bus_suspended) { >>> /* do something */ >>> } else { >>> /* do something else */ >>> } >>> >>> Presumably the "do something" is now dead code? >>> >> That part is not dad because if hsotg->bus_suspended is true >> resuming/exiting from suspend/partial power down/hibernation should be >> performed. >> On the other hand if hsotg->bus_suspended is false there is no need to >> resume. >> >> So of course if core is not suspended the code responsible for resuming >> should not be called. In that sense the code can be called dead. > > I think I didn't explain it well. Does this patch help you? > > https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2B_4e84efdbeb74bcb8b24e2b1fea24153981acc185-255E-2521_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=nZWkYoZKaqSi_evQfEH0uCUKIXb5UZydmgRSXsVwSOs&s=h-ZqBu_CidHWrCH6sAbTi1ddgSfHkYjwSfCOGjpHbRM&e= No it doesn't. This is diff from your suggestion. hsotg->lx_state = DWC2_L0; + hsotg->flags.b.port_suspend_change = 1; Why are you setting this ? spin_unlock_irqrestore(&hsotg->lock, flags); - - if (hsotg->bus_suspended) { - spin_lock_irqsave(&hsotg->lock, flags); - hsotg->flags.b.port_suspend_change = 1; - spin_unlock_irqrestore(&hsotg->lock, flags); - dwc2_port_resume(hsotg); You have removed this but it should be called when we exit from partial power down. Have you performed testes on this? I am not sure that this is a working code. - } else { - dwc2_vbus_supply_init(hsotg); - - /* Wait for controller to correctly update D+/D- level */ - usleep_range(3000, 5000); - - /* - * Clear Port Enable and Port Status changes. - * Enable Port Power. - */ - dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET | - HPRT0_ENACHG, HPRT0); - /* Wait for controller to detect Port Connect */ - usleep_range(5000, 7000); - } + dwc2_port_resume(hsotg); break; > > >>>> + spin_unlock_irqrestore(&hsotg->lock, flags); >>>> + >>> >>> I'm pretty curious if you tested DWC2_POWER_DOWN_PARAM_PARTIAL after >>> applying your patch series. As far as I can tell your switch >>> statement for DWC2_POWER_DOWN_PARAM_PARTIAL will "break" with the >>> spinlock already unlocked. ...so you'll run spin_unlock_irqrestore >>> twice. Is that really legit? >> I have tested the patches on HAPS-DX and Linaro HiKey 960 boards. > > How is it possible that you don't end up spin unlocking more than > once? This seems like a pretty serious problem. > > My guess is that whatever tests you ran didn't actually exercise this > function. Personally I could only get it to exercise by doing > suspend/resume. > > ...and when I did, sure enough I saw: > > BUG: spinlock already unlocked on CPU#2, kworker/u8:32/5812 I will debug it once more. > > >>> ...with that a quick test seems to show that partial power down is >>> sorta working on rk3288 now. I _think_ I saw one case where hotplug >>> failed but I've seen several where it works. ...unfortunately it >>> seems to break when I do hotplug on the port where I have >>> "snps,reset-phy-on-wake" set. >> You can provide debug logs for that scenario I will try to help you fix >> issues with that. > > All you need to do is make sure you run the function > _dwc2_hcd_suspend() with power down mode set to > "DWC2_POWER_DOWN_PARAM_PARTIAL". ...or walk through the code and see > that spin_unlock_irqrestore() will certainly be called twice. I hibernate the PC it enters to partial power down then I turn PC on everything works fine. But I will check this one more time. > > > > -Doug >
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) { + /* 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__); + } else { + goto skip_power_saving; + } + break; + default: goto skip_power_saving; } - hsotg->bus_suspended = true; - /* 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)) + if (!hsotg->bus_suspended) goto unlock; if (hsotg->lx_state != DWC2_L2) goto unlock; - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL || + hprt0 = dwc2_read_hprt0(hsotg); + + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE || hprt0 & HPRT0_CONNSTS) { hsotg->lx_state = DWC2_L0; goto unlock; @@ -4597,36 +4616,51 @@ 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"); + switch (hsotg->params.power_down) { + case DWC2_POWER_DOWN_PARAM_PARTIAL: - hsotg->lx_state = DWC2_L0; + /* 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"); - spin_unlock_irqrestore(&hsotg->lock, flags); + hsotg->lx_state = DWC2_L0; - if (hsotg->bus_suspended) { - spin_lock_irqsave(&hsotg->lock, flags); - hsotg->flags.b.port_suspend_change = 1; spin_unlock_irqrestore(&hsotg->lock, flags); - dwc2_port_resume(hsotg); - } else { - dwc2_vbus_supply_init(hsotg); - /* Wait for controller to correctly update D+/D- level */ - usleep_range(3000, 5000); + if (hsotg->bus_suspended) { + spin_lock_irqsave(&hsotg->lock, flags); + hsotg->flags.b.port_suspend_change = 1; + spin_unlock_irqrestore(&hsotg->lock, flags); + dwc2_port_resume(hsotg); + } else { + dwc2_vbus_supply_init(hsotg); - /* - * Clear Port Enable and Port Status changes. - * Enable Port Power. - */ - dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET | + /* Wait for controller to correctly update D+/D- level */ + usleep_range(3000, 5000); + + /* + * Clear Port Enable and Port Status changes. + * Enable Port Power. + */ + dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET | HPRT0_ENACHG, HPRT0); - /* Wait for controller to detect Port Connect */ - usleep_range(5000, 7000); + /* Wait for controller to detect Port Connect */ + usleep_range(5000, 7000); + } + break; + case DWC2_POWER_DOWN_PARAM_HIBERNATION: + + /* Exit host hibernation. */ + if (hsotg->hibernated) + dwc2_exit_hibernation(hsotg, 0, 0, 1); + break; + default: + goto unlock; } + spin_unlock_irqrestore(&hsotg->lock, flags); + return ret; unlock: spin_unlock_irqrestore(&hsotg->lock, flags);
Added a new flow of entering and exiting hibernation when PC is hibernated or suspended. Signed-off-by: Artur Petrosyan <arturp@synopsys.com> --- drivers/usb/dwc2/hcd.c | 128 +++++++++++++++++++++++++++++++------------------ 1 file changed, 81 insertions(+), 47 deletions(-)