Hi, On Tue, Apr 30, 2019 at 5:45 AM Artur Petrosyan <Arthur.Petrosyan@xxxxxxxxxxxx> wrote: > > > If setting "power_down = 0" is wrong then please update your patch to > > remove all the mainline code that sets power_down to 0. Presumably > > this means you'd want to convert that code over to using "power_saving > > = False". Perhaps then I can see your vision of how this works more > > clearly. > Yes this is a good idea. Actually, I have a feeling it won't work, at least not without more changes. ...and that's part of the problem with your patch. Specifically dwc2 works by first filling in the "default" parameters. Then the per-platform config function runs and overrides the defaults. If the per-platform config function overrides the "power_saving" parameter then it will be too late. > > NOTE: I'm curious how you envision what someone would do if they had a > > core that supported hibernation but they only wanted to enable partial > > power down. I guess then they'd have to set "power_saving = True" and > > then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"? I guess your > > vision of the world is: > I have implemented everything based on programming guide and data book. > Core can only support hibernation or partial power down based on the > configuration parameters. There cannot be two modes simultaneously of > power saving only one of them. Ah, this is new information to me. I assumed they were supersets of each other, so if you supported hibernation you also supported partial power down. Thanks for clearing that up! > The power_down flag is set to DWC2_POWER_DOWN_PARAM_PARTIAL , > DWC2_POWER_DOWN_PARAM_HIBERNATION or DWC2_POWER_DOWN_PARAM_NONE while > checking the hw parameters. So it just indicates which power down mode > is supporting the core. I don't think this is what the params are about. The params are about the currently configured parameters, not about what the core supports. This is why all the other code checks the actual value of the params to decide whether to do power savings. -Doug
