On Tue, 13 Aug 2024 at 21:57, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren <wahrenst@xxxxxxx> wrote: > > > > Hi Doug, > > > > Am 28.07.24 um 15:00 schrieb Stefan Wahren: > > > DO NOT MERGE > > > > > > According to the dt-bindings there are some platforms, which have a > > > dedicated USB power domain for DWC2 IP core supply. If the power domain > > > is switched off during system suspend then all USB register will lose > > > their settings. > > > > > > So use the power on/off notifier in order to save & restore the USB > > > registers during system suspend. > > sorry for bothering you with this DWC2 stuff, but it would great if you > > can gave some feedback about this patch. > > Boy, it's been _ages_ since I looked at anything to do with dwc2, but > I still have some fondness in my heart for the crufty old driver :-P I > know I was involved with some of the patches to get > wakeup-from-suspend working on dwc2 host controllers in the past but, > if I remember correctly, I mostly shepherded / fixed patches from > Rockchip. Not sure I can spend the days trawling through the driver > and testing things with printk that really answering properly would > need, but let's see... > > > > I was working a lot to get > > suspend to idle working on Raspberry Pi. And this patch is the most > > complex part of the series. > > > > Would you agree with this approach or did i miss something? > > > > The problem is that the power domain driver acts independent from dwc2, > > so we cannot prevent the USB domain power down except declaring a USB > > device as wakeup source. So i decided to use the notifier approach. This > > has been successful tested on some older Raspberry Pi boards. > > My genpd knowledge is probably not as good as it should be. Don't tell > anyone (aside from all the people and lists CCed here). ;-) > > ...so I guess you're relying on the fact that > dev_pm_genpd_add_notifier() will return an error if a power-domain > wasn't specified for dwc2 in the device tree, then you ignore that > error and your callback will never happen. You assume that the power > domain isn't specified then the dwc2 registers will be saved? > > I guess one thing is that I'd wonder if that's really reliable. Maybe > some dwc2 controllers lose their registers over system suspend but > _don't_ specify a power domain? Maybe the USB controller just gets its > power yanked as part of system suspend. Maybe that's why the functions > for saving / restoring registers are already there? It looks like > there are ways for various platforms to specify that registers are > lost in some cases... > > ...but I guess you can't use the existing ways to say that registers > are lost because you're trying to be dynamic. You're saying that your > registers get saved _unless_ the power domain gets turned off, right? > ...and the device core keeps power domains on for suspended devices if > they are wakeup sources, which makes sense. > > So with that, your patch sounds like a plausible way to do it. I guess > one other way to do it would be some sort of "canary" approach. You > could _always_ save registers and then, at resume time, you could > detect if some "canary" register had reset to its power-on default. If > you see this then you can assume power was lost and re-init all the > registers. This could be pretty much any register that you know won't > be its power on default. In some ways a "canary" approach is uglier > but it also might be more reliable across more configurations? > > I guess those would be my main thoughts on the topic. Is that roughly > the feedback you were looking for? Thanks Doug for sharing your thoughts. For the record, I agree with these suggestions. Using the genpd on/off notifiers is certainly fine, but doing a save/restore unconditionally via some of the PM callbacks is usually preferred - if it works. Kind regards Uffe
