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? -Doug
