Hi, On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan <Arthur.Petrosyan@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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
