[PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off

[Ulf Hansson]

On Tue, 13 Aug 2024 at 21:57, Doug Anderson <dianders at chromium.org> wrote:
>
> Hi,
>
> On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren <wahrenst at gmx.net> 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