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

[Stefan Wahren]

Am 14.08.24 um 14:01 schrieb 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...
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@gmx.net/
[2] -
https://lore.kernel.org/linux-usb/e4532055-c5d6-4ac9-8bbb-b9df18fa178b@gmx.net/
>
> Kind regards
> Uffe