Am 14.08.24 um 14:01 schrieb Ulf Hansson:
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...
Yes, this was more a cry for help, because i didn't get much feedback
yet here [1] [2]. So i searched for the most elegant solution via trial
& error and this patch is the outcome. One reason why this is still WIP,
is that i didn't test the gadget role path yet.
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?
Yes, on Raspberry Pi we needed to implement the power domain driver to
enable USB at the first place.
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...
Yes, the DT property "snps,need-phy-for-wake" is such a case. But the
PHY on Raspberry Pi is currently modeled as a no-op.
...but I guess you can't use the existing ways to say that registers
are lost because you're trying to be dynamic.
Yes, before this patch the DWC2 doesn't know if the USB domain is
powered down during suspend. So the notifier seems the most elegant
solution to me.
You're saying that your
registers get saved _unless_ the power domain gets turned off, right?
On BCM2835 there is no need to store the registers because there is no
power management supported by USB core except of the power domain. So
DWC2 don't expect a register loss.
...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 don't have enough knowledge about DWC2 and i also don't have the
databook to figure out if there is a magic register which could be used
for the canary approach. But all these different platforms, host vs
gadget role, different low modes let me think the resulting solution
would be also fragile and ugly.
I guess those would be my main thoughts on the topic. Is that roughly
the feedback you were looking for?
Yes, thank you. This was very helpful.
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.
I tried the latter one before without success. Because the DWC2 is aware
that the BCM2835 IP doesn't support any power saving mode, the USB core
stays fully functional in suspend and register restoring on resume
tramples newer registers values.
Best regards
[1] -
https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@xxxxxxx/
[2] -
https://lore.kernel.org/linux-usb/e4532055-c5d6-4ac9-8bbb-b9df18fa178b@xxxxxxx/
Kind regards
Uffe
