| Message ID | 0dc725c7e9956eedaf03bb5d68a7d5e856d8cb5a.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: > > - In dwc2_port_suspend() function added waiting for the > HPRT0.PrtSusp register field to be set. > > - In _dwc2_hcd_suspend() function added checking of > "hsotg->flags.b.port_connect_status" port connection > status if port connection status is 0 then skipping > power saving (entering partial power down mode). > Because if there is no device connected there would > be no need to enter partial power down mode. > > - Added "hsotg->bus_suspended = true" beceuse after s/beceuse/because > entering partial power down in host mode the > bus_suspended flag must be set. The above statement sounds to me like trying to win an argument by saying "I'm right because I'm right." Can you give more details about why "bus_suspended" must be set? See below also. > Signed-off-by: Artur Petrosyan <arturp@synopsys.com> > --- > drivers/usb/dwc2/hcd.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index dd82fa516f3f..1d18258b5ff8 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex) > hprt0 |= HPRT0_SUSP; > dwc2_writel(hsotg, hprt0, HPRT0); > > + /* Wait for the HPRT0.PrtSusp register field to be set */ > + if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000)) > + dev_warn(hsotg->dev, "Suspend wasn't generated\n"); > + > hsotg->bus_suspended = true; > > /* > @@ -4488,7 +4492,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > if (hsotg->op_state == OTG_STATE_B_PERIPHERAL) > goto unlock; > > - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) > + if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL || > + hsotg->flags.b.port_connect_status == 0) > goto skip_power_saving; > > /* > @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > goto skip_power_saving; > } > > + hsotg->bus_suspended = true; > + I'm a bit unsure about this. Specifically I note that _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)". Does that now become dead code? ...I talk about this a bit more in my review of ("usb: dwc2: Add enter/exit hibernation from system issued suspend/resume") -Doug
Hi, On 4/27/2019 00:45, Doug Anderson wrote: > Hi, > > On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> - In dwc2_port_suspend() function added waiting for the >> HPRT0.PrtSusp register field to be set. >> >> - In _dwc2_hcd_suspend() function added checking of >> "hsotg->flags.b.port_connect_status" port connection >> status if port connection status is 0 then skipping >> power saving (entering partial power down mode). >> Because if there is no device connected there would >> be no need to enter partial power down mode. >> >> - Added "hsotg->bus_suspended = true" beceuse after > > s/beceuse/because > >> entering partial power down in host mode the >> bus_suspended flag must be set. > > The above statement sounds to me like trying to win an argument by > saying "I'm right because I'm right." Can you give more details about > why "bus_suspended" must be set? See below also. > There is no intention of winning any argument. Are you familiar with "bus_suspended" flag ? have you looked at what is it used for ? * @bus_suspended: True if bus is suspended So when entering to hibernation is performed bus is suspended. To indicate that "hsotg->bus_suspended" is assigned to true. > >> Signed-off-by: Artur Petrosyan <arturp@synopsys.com> >> --- >> drivers/usb/dwc2/hcd.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >> index dd82fa516f3f..1d18258b5ff8 100644 >> --- a/drivers/usb/dwc2/hcd.c >> +++ b/drivers/usb/dwc2/hcd.c >> @@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex) >> hprt0 |= HPRT0_SUSP; >> dwc2_writel(hsotg, hprt0, HPRT0); >> >> + /* Wait for the HPRT0.PrtSusp register field to be set */ >> + if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000)) >> + dev_warn(hsotg->dev, "Suspend wasn't generated\n"); >> + >> hsotg->bus_suspended = true; >> >> /* >> @@ -4488,7 +4492,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) >> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL) >> goto unlock; >> >> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) >> + if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL || >> + hsotg->flags.b.port_connect_status == 0) >> goto skip_power_saving; >> >> /* >> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) >> goto skip_power_saving; >> } >> >> + hsotg->bus_suspended = true; >> + > > I'm a bit unsure about this. Specifically I note that > _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)". > Does that now become dead code? No it doesn't became a dead code. > > ...I talk about this a bit more in my review of ("usb: dwc2: Add > enter/exit hibernation from system issued suspend/resume") > > > -Doug >
Hi, On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Hi, > > On 4/27/2019 00:45, Doug Anderson wrote: > > Hi, > > > > On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan > > <Arthur.Petrosyan@synopsys.com> wrote: > >> > >> - In dwc2_port_suspend() function added waiting for the > >> HPRT0.PrtSusp register field to be set. > >> > >> - In _dwc2_hcd_suspend() function added checking of > >> "hsotg->flags.b.port_connect_status" port connection > >> status if port connection status is 0 then skipping > >> power saving (entering partial power down mode). > >> Because if there is no device connected there would > >> be no need to enter partial power down mode. > >> > >> - Added "hsotg->bus_suspended = true" beceuse after > > > > s/beceuse/because > > > >> entering partial power down in host mode the > >> bus_suspended flag must be set. > > > > The above statement sounds to me like trying to win an argument by > > saying "I'm right because I'm right." Can you give more details about > > why "bus_suspended" must be set? See below also. > > > There is no intention of winning any argument. I was trying to say that your statement does not convey any information about the "why". It just says: "I now set this variable because it needs to be set". This tells me nothing useful because presumably if it did't need to be set then you wouldn't have set it. I want to know more information about how the code was broken before you did this. What specifically requires this variable to be set? > Are you familiar with "bus_suspended" flag ? have you looked at what is > it used for ? > > * @bus_suspended: True if bus is suspended > > So when entering to hibernation is performed bus is suspended. To > indicate that "hsotg->bus_suspended" is assigned to true. Perhaps you can help me understand what the difference is between "port suspend" and "bus suspend" on dwc2. I think this is where my confusion lies since there are functions for both and they do subtly different things. ...but both functions set bus_suspended. > >> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > >> goto skip_power_saving; > >> } > >> > >> + hsotg->bus_suspended = true; > >> + > > > > I'm a bit unsure about this. Specifically I note that > > _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)". > > Does that now become dead code? > No it doesn't became a dead code. Can you explain when it gets triggered, then? -Doug
Hi, On 4/29/2019 21:35, Doug Anderson wrote: > Hi, > > On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> Hi, >> >> On 4/27/2019 00:45, Doug Anderson wrote: >>> Hi, >>> >>> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan >>> <Arthur.Petrosyan@synopsys.com> wrote: >>>> >>>> - In dwc2_port_suspend() function added waiting for the >>>> HPRT0.PrtSusp register field to be set. >>>> >>>> - In _dwc2_hcd_suspend() function added checking of >>>> "hsotg->flags.b.port_connect_status" port connection >>>> status if port connection status is 0 then skipping >>>> power saving (entering partial power down mode). >>>> Because if there is no device connected there would >>>> be no need to enter partial power down mode. >>>> >>>> - Added "hsotg->bus_suspended = true" beceuse after >>> >>> s/beceuse/because >>> >>>> entering partial power down in host mode the >>>> bus_suspended flag must be set. >>> >>> The above statement sounds to me like trying to win an argument by >>> saying "I'm right because I'm right." Can you give more details about >>> why "bus_suspended" must be set? See below also. >>> >> There is no intention of winning any argument. > > I was trying to say that your statement does not convey any > information about the "why". It just says: "I now set this variable > because it needs to be set". This tells me nothing useful because > presumably if it did't need to be set then you wouldn't have set it. > I want to know more information about how the code was broken before > you did this. What specifically requires this variable to be set? Specifically that variable is set when core enters to hibernation or partial power down. > > >> Are you familiar with "bus_suspended" flag ? have you looked at what is >> it used for ? >> >> * @bus_suspended: True if bus is suspended >> >> So when entering to hibernation is performed bus is suspended. To >> indicate that "hsotg->bus_suspended" is assigned to true. > > Perhaps you can help me understand what the difference is between > "port suspend" and "bus suspend" on dwc2. I think this is where my > confusion lies since there are functions for both and they do subtly > different things. ...but both functions set bus_suspended. dwc2_port_suspend() is a function which is called when set port feature "USB_PORT_FEAT_SUSPEND" is asserted. Yet, bus_suspended is a variable. That variable should be set any time that host enters to hibernation or partial power down. > > >>>> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) >>>> goto skip_power_saving; >>>> } >>>> >>>> + hsotg->bus_suspended = true; >>>> + >>> >>> I'm a bit unsure about this. Specifically I note that >>> _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)". >>> Does that now become dead code? >> No it doesn't became a dead code. > > Can you explain when it gets triggered, then? _dwc2_hcd_resume() is triggered by the system. As an example lets assume you have hibernated the PC and then you turn the PC on. When you turn the PC back on in that case _dwc2_hcd_resume() function is called to resume from suspended state (Hibernation/partial power down) > > > -Doug >
Hi, On Tue, Apr 30, 2019 at 12:11 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Hi, > > On 4/29/2019 21:35, Doug Anderson wrote: > > Hi, > > > > On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan > > <Arthur.Petrosyan@synopsys.com> wrote: > >> > >> Hi, > >> > >> On 4/27/2019 00:45, Doug Anderson wrote: > >>> Hi, > >>> > >>> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan > >>> <Arthur.Petrosyan@synopsys.com> wrote: > >>>> > >>>> - In dwc2_port_suspend() function added waiting for the > >>>> HPRT0.PrtSusp register field to be set. > >>>> > >>>> - In _dwc2_hcd_suspend() function added checking of > >>>> "hsotg->flags.b.port_connect_status" port connection > >>>> status if port connection status is 0 then skipping > >>>> power saving (entering partial power down mode). > >>>> Because if there is no device connected there would > >>>> be no need to enter partial power down mode. > >>>> > >>>> - Added "hsotg->bus_suspended = true" beceuse after > >>> > >>> s/beceuse/because > >>> > >>>> entering partial power down in host mode the > >>>> bus_suspended flag must be set. > >>> > >>> The above statement sounds to me like trying to win an argument by > >>> saying "I'm right because I'm right." Can you give more details about > >>> why "bus_suspended" must be set? See below also. > >>> > >> There is no intention of winning any argument. > > > > I was trying to say that your statement does not convey any > > information about the "why". It just says: "I now set this variable > > because it needs to be set". This tells me nothing useful because > > presumably if it did't need to be set then you wouldn't have set it. > > I want to know more information about how the code was broken before > > you did this. What specifically requires this variable to be set? > Specifically that variable is set when core enters to hibernation or > partial power down. > > > > > >> Are you familiar with "bus_suspended" flag ? have you looked at what is > >> it used for ? > >> > >> * @bus_suspended: True if bus is suspended > >> > >> So when entering to hibernation is performed bus is suspended. To > >> indicate that "hsotg->bus_suspended" is assigned to true. > > > > Perhaps you can help me understand what the difference is between > > "port suspend" and "bus suspend" on dwc2. I think this is where my > > confusion lies since there are functions for both and they do subtly > > different things. ...but both functions set bus_suspended. > dwc2_port_suspend() is a function which is called when set port feature > "USB_PORT_FEAT_SUSPEND" is asserted. Yet, bus_suspended is a variable. > That variable should be set any time that host enters to hibernation or > partial power down. > > > > > > >>>> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > >>>> goto skip_power_saving; > >>>> } > >>>> > >>>> + hsotg->bus_suspended = true; > >>>> + > >>> > >>> I'm a bit unsure about this. Specifically I note that > >>> _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)". > >>> Does that now become dead code? > >> No it doesn't became a dead code. > > > > Can you explain when it gets triggered, then? > _dwc2_hcd_resume() is triggered by the system. > As an example lets assume you have hibernated the PC and then you turn > the PC on. When you turn the PC back on in that case _dwc2_hcd_resume() > function is called to resume from suspended state (Hibernation/partial > power down) I think you are still not understanding my question here. Please re-read it again. -Doug
On 5/1/2019 05:55, Doug Anderson wrote: > Hi, > > On Tue, Apr 30, 2019 at 12:11 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> Hi, >> >> On 4/29/2019 21:35, Doug Anderson wrote: >>> Hi, >>> >>> On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan >>> <Arthur.Petrosyan@synopsys.com> wrote: >>>> >>>> Hi, >>>> >>>> On 4/27/2019 00:45, Doug Anderson wrote: >>>>> Hi, >>>>> >>>>> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan >>>>> <Arthur.Petrosyan@synopsys.com> wrote: >>>>>> >>>>>> - In dwc2_port_suspend() function added waiting for the >>>>>> HPRT0.PrtSusp register field to be set. >>>>>> >>>>>> - In _dwc2_hcd_suspend() function added checking of >>>>>> "hsotg->flags.b.port_connect_status" port connection >>>>>> status if port connection status is 0 then skipping >>>>>> power saving (entering partial power down mode). >>>>>> Because if there is no device connected there would >>>>>> be no need to enter partial power down mode. >>>>>> >>>>>> - Added "hsotg->bus_suspended = true" beceuse after >>>>> >>>>> s/beceuse/because >>>>> >>>>>> entering partial power down in host mode the >>>>>> bus_suspended flag must be set. >>>>> >>>>> The above statement sounds to me like trying to win an argument by >>>>> saying "I'm right because I'm right." Can you give more details about >>>>> why "bus_suspended" must be set? See below also. >>>>> >>>> There is no intention of winning any argument. >>> >>> I was trying to say that your statement does not convey any >>> information about the "why". It just says: "I now set this variable >>> because it needs to be set". This tells me nothing useful because >>> presumably if it did't need to be set then you wouldn't have set it. >>> I want to know more information about how the code was broken before >>> you did this. What specifically requires this variable to be set? >> Specifically that variable is set when core enters to hibernation or >> partial power down. >>> >>> >>>> Are you familiar with "bus_suspended" flag ? have you looked at what is >>>> it used for ? >>>> >>>> * @bus_suspended: True if bus is suspended >>>> >>>> So when entering to hibernation is performed bus is suspended. To >>>> indicate that "hsotg->bus_suspended" is assigned to true. >>> >>> Perhaps you can help me understand what the difference is between >>> "port suspend" and "bus suspend" on dwc2. I think this is where my >>> confusion lies since there are functions for both and they do subtly >>> different things. ...but both functions set bus_suspended. >> dwc2_port_suspend() is a function which is called when set port feature >> "USB_PORT_FEAT_SUSPEND" is asserted. Yet, bus_suspended is a variable. >> That variable should be set any time that host enters to hibernation or >> partial power down. >> >>> >>> >>>>>> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) >>>>>> goto skip_power_saving; >>>>>> } >>>>>> >>>>>> + hsotg->bus_suspended = true; >>>>>> + >>>>> >>>>> I'm a bit unsure about this. Specifically I note that >>>>> _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)". >>>>> Does that now become dead code? >>>> No it doesn't became a dead code. >>> >>> Can you explain when it gets triggered, then? >> _dwc2_hcd_resume() is triggered by the system. >> As an example lets assume you have hibernated the PC and then you turn >> the PC on. When you turn the PC back on in that case _dwc2_hcd_resume() >> function is called to resume from suspended state (Hibernation/partial >> power down) > > I think you are still not understanding my question here. Please > re-read it again. Your question is "Can you explain when it gets triggered, then?" so I have explained one case when it is triggered. > > > -Doug >
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index dd82fa516f3f..1d18258b5ff8 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex) hprt0 |= HPRT0_SUSP; dwc2_writel(hsotg, hprt0, HPRT0); + /* Wait for the HPRT0.PrtSusp register field to be set */ + if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000)) + dev_warn(hsotg->dev, "Suspend wasn't generated\n"); + hsotg->bus_suspended = true; /* @@ -4488,7 +4492,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) if (hsotg->op_state == OTG_STATE_B_PERIPHERAL) goto unlock; - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) + if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL || + hsotg->flags.b.port_connect_status == 0) goto skip_power_saving; /* @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) goto skip_power_saving; } + hsotg->bus_suspended = true; + /* Ask phy to be suspended */ if (!IS_ERR_OR_NULL(hsotg->uphy)) { spin_unlock_irqrestore(&hsotg->lock, flags);
- In dwc2_port_suspend() function added waiting for the HPRT0.PrtSusp register field to be set. - In _dwc2_hcd_suspend() function added checking of "hsotg->flags.b.port_connect_status" port connection status if port connection status is 0 then skipping power saving (entering partial power down mode). Because if there is no device connected there would be no need to enter partial power down mode. - Added "hsotg->bus_suspended = true" beceuse after entering partial power down in host mode the bus_suspended flag must be set. Signed-off-by: Artur Petrosyan <arturp@synopsys.com> --- drivers/usb/dwc2/hcd.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)