| Message ID | 20240728130029.78279-6-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: bcm2835: Implement initial S2Idle for Raspberry Pi | expand |
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. 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. Best regards > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > > Any feedback is appreciated. > > drivers/usb/dwc2/core.c | 16 ++++++++++++ > drivers/usb/dwc2/core.h | 17 +++++++++++++ > drivers/usb/dwc2/gadget.c | 49 +++++++++++++++++++++++++++++++++++++ > drivers/usb/dwc2/hcd.c | 49 +++++++++++++++++++++++++++++++++++++ > drivers/usb/dwc2/platform.c | 32 ++++++++++++++++++++++++ > 5 files changed, 163 insertions(+) > > diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c > index 9919ab725d54..a3263cfdedac 100644 > --- a/drivers/usb/dwc2/core.c > +++ b/drivers/usb/dwc2/core.c > @@ -391,6 +391,22 @@ int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup, > return dwc2_gadget_exit_hibernation(hsotg, rem_wakeup, reset); > } > > +int dwc2_enter_poweroff(struct dwc2_hsotg *hsotg) > +{ > + if (dwc2_is_host_mode(hsotg)) > + return dwc2_host_enter_poweroff(hsotg); > + else > + return dwc2_gadget_enter_poweroff(hsotg); > +} > + > +int dwc2_exit_poweroff(struct dwc2_hsotg *hsotg) > +{ > + if (dwc2_is_host_mode(hsotg)) > + return dwc2_host_exit_poweroff(hsotg); > + else > + return dwc2_gadget_exit_poweroff(hsotg); > +} > + > /* > * Do core a soft reset of the core. Be careful with this because it > * resets all the internal state machines of the core. > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > index 2bd74f3033ed..9ab755cc3081 100644 > --- a/drivers/usb/dwc2/core.h > +++ b/drivers/usb/dwc2/core.h > @@ -9,6 +9,7 @@ > #define __DWC2_CORE_H__ > > #include <linux/acpi.h> > +#include <linux/notifier.h> > #include <linux/phy/phy.h> > #include <linux/regulator/consumer.h> > #include <linux/usb/gadget.h> > @@ -1080,6 +1081,8 @@ struct dwc2_hsotg { > struct regulator *vbus_supply; > struct regulator *usb33d; > > + struct notifier_block genpd_nb; > + > spinlock_t lock; > void *priv; > int irq; > @@ -1316,6 +1319,8 @@ int dwc2_exit_partial_power_down(struct dwc2_hsotg *hsotg, int rem_wakeup, > int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg, int is_host); > int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup, > int reset, int is_host); > +int dwc2_enter_poweroff(struct dwc2_hsotg *hsotg); > +int dwc2_exit_poweroff(struct dwc2_hsotg *hsotg); > void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg); > int dwc2_phy_init(struct dwc2_hsotg *hsotg, bool select_phy); > > @@ -1435,6 +1440,8 @@ int dwc2_hsotg_tx_fifo_total_depth(struct dwc2_hsotg *hsotg); > int dwc2_hsotg_tx_fifo_average_depth(struct dwc2_hsotg *hsotg); > void dwc2_gadget_init_lpm(struct dwc2_hsotg *hsotg); > void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg); > +int dwc2_gadget_enter_poweroff(struct dwc2_hsotg *hsotg); > +int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg); > static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) > { hsotg->fifo_map = 0; } > #else > @@ -1482,6 +1489,10 @@ static inline int dwc2_hsotg_tx_fifo_average_depth(struct dwc2_hsotg *hsotg) > { return 0; } > static inline void dwc2_gadget_init_lpm(struct dwc2_hsotg *hsotg) {} > static inline void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg) {} > +static inline int dwc2_gadget_enter_poweroff(struct dwc2_hsotg *hsotg) > +{ return 0; } > +static inline int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg) > +{ return 0; } > static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) {} > #endif > > @@ -1505,6 +1516,8 @@ int dwc2_host_exit_partial_power_down(struct dwc2_hsotg *hsotg, > void dwc2_host_enter_clock_gating(struct dwc2_hsotg *hsotg); > void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup); > bool dwc2_host_can_poweroff_phy(struct dwc2_hsotg *dwc2); > +int dwc2_host_enter_poweroff(struct dwc2_hsotg *hsotg); > +int dwc2_host_exit_poweroff(struct dwc2_hsotg *hsotg); > static inline void dwc2_host_schedule_phy_reset(struct dwc2_hsotg *hsotg) > { schedule_work(&hsotg->phy_reset_work); } > #else > @@ -1544,6 +1557,10 @@ static inline void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, > int rem_wakeup) {} > static inline bool dwc2_host_can_poweroff_phy(struct dwc2_hsotg *dwc2) > { return false; } > +static inline int dwc2_host_enter_poweroff(struct dwc2_hsotg *hsotg) > +{ return 0; } > +static inline int dwc2_host_exit_poweroff(struct dwc2_hsotg *hsotg) > +{ return 0; } > static inline void dwc2_host_schedule_phy_reset(struct dwc2_hsotg *hsotg) {} > > #endif > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index e7bf9cc635be..38f0112970fe 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -5710,3 +5710,52 @@ void dwc2_gadget_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup) > hsotg->lx_state = DWC2_L0; > hsotg->bus_suspended = false; > } > + > +int dwc2_gadget_enter_poweroff(struct dwc2_hsotg *hsotg) > +{ > + int ret; > + > + dev_dbg(hsotg->dev, "Entering device power off.\n"); > + > + /* Backup all registers */ > + ret = dwc2_backup_global_registers(hsotg); > + if (ret) { > + dev_err(hsotg->dev, "%s: failed to backup global registers\n", > + __func__); > + return ret; > + } > + > + ret = dwc2_backup_device_registers(hsotg); > + if (ret) { > + dev_err(hsotg->dev, "%s: failed to backup device registers\n", > + __func__); > + return ret; > + } > + > + dev_dbg(hsotg->dev, "Entering device power off completed.\n"); > + return 0; > +} > + > +int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg) > +{ > + int ret; > + > + dev_dbg(hsotg->dev, "Exiting device power off.\n"); > + > + ret = dwc2_restore_global_registers(hsotg); > + if (ret) { > + dev_err(hsotg->dev, "%s: failed to restore registers\n", > + __func__); > + return ret; > + } > + > + ret = dwc2_restore_device_registers(hsotg, 0); > + if (ret) { > + dev_err(hsotg->dev, "%s: failed to restore device registers\n", > + __func__); > + return ret; > + } > + > + dev_dbg(hsotg->dev, "Exiting device power off completed.\n"); > + return 0; > +} > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index cb54390e7de4..22afdafb474e 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -5993,3 +5993,52 @@ void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup) > jiffies + msecs_to_jiffies(71)); > } > } > + > +int dwc2_host_enter_poweroff(struct dwc2_hsotg *hsotg) > +{ > + int ret; > + > + dev_dbg(hsotg->dev, "Entering host power off.\n"); > + > + /* Backup all registers */ > + ret = dwc2_backup_global_registers(hsotg); > + if (ret) { > + dev_err(hsotg->dev, "%s: failed to backup global registers\n", > + __func__); > + return ret; > + } > + > + ret = dwc2_backup_host_registers(hsotg); > + if (ret) { > + dev_err(hsotg->dev, "%s: failed to backup host registers\n", > + __func__); > + return ret; > + } > + > + dev_dbg(hsotg->dev, "Entering host power off completed.\n"); > + return 0; > +} > + > +int dwc2_host_exit_poweroff(struct dwc2_hsotg *hsotg) > +{ > + int ret; > + > + dev_dbg(hsotg->dev, "Exiting host power off.\n"); > + > + ret = dwc2_restore_global_registers(hsotg); > + if (ret) { > + dev_err(hsotg->dev, "%s: failed to restore registers\n", > + __func__); > + return ret; > + } > + > + ret = dwc2_restore_host_registers(hsotg); > + if (ret) { > + dev_err(hsotg->dev, "%s: failed to restore host registers\n", > + __func__); > + return ret; > + } > + > + dev_dbg(hsotg->dev, "Exiting host power off completed.\n"); > + return 0; > +} > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index 7b84416dfc2b..b97eefc18a6b 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -16,6 +16,7 @@ > #include <linux/platform_device.h> > #include <linux/phy/phy.h> > #include <linux/platform_data/s3c-hsotg.h> > +#include <linux/pm_domain.h> > #include <linux/reset.h> > > #include <linux/usb/of.h> > @@ -307,6 +308,8 @@ static void dwc2_driver_remove(struct platform_device *dev) > struct dwc2_gregs_backup *gr; > int ret = 0; > > + dev_pm_genpd_remove_notifier(&dev->dev); > + > gr = &hsotg->gr_backup; > > /* Exit Hibernation when driver is removed. */ > @@ -421,6 +424,31 @@ int dwc2_check_core_version(struct dwc2_hsotg *hsotg) > return 0; > } > > +static int dwc2_power_notifier(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct dwc2_hsotg *hsotg = container_of(nb, struct dwc2_hsotg, > + genpd_nb); > + int ret; > + > + switch (action) { > + case GENPD_NOTIFY_ON: > + ret = dwc2_exit_poweroff(hsotg); > + if (ret) > + dev_err(hsotg->dev, "exit poweroff failed\n"); > + break; > + case GENPD_NOTIFY_PRE_OFF: > + ret = dwc2_enter_poweroff(hsotg); > + if (ret) > + dev_err(hsotg->dev, "enter poweroff failed\n"); > + break; > + default: > + break; > + } > + > + return NOTIFY_OK; > +} > + > /** > * dwc2_driver_probe() - Called when the DWC_otg core is bound to the DWC_otg > * driver > @@ -620,6 +648,10 @@ static int dwc2_driver_probe(struct platform_device *dev) > } > } > #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */ > + > + hsotg->genpd_nb.notifier_call = dwc2_power_notifier; > + dev_pm_genpd_add_notifier(&dev->dev, &hsotg->genpd_nb); > + > return 0; > > #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \ > -- > 2.34.1 >
Hi, On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren <wahrenst@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? -Doug
On Tue, 13 Aug 2024 at 21:57, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren <wahrenst@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
Am 14.08.24 um 14:01 schrieb Ulf Hansson: > On Tue, 13 Aug 2024 at 21:57, Doug Anderson <dianders@chromium.org> wrote: >> Hi, >> >> On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren <wahrenst@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
Hi, On Wed, Aug 14, 2024 at 2:48 PM Stefan Wahren <wahrenst@gmx.net> wrote: > > >> 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 won't admit to having a DWC2 databook. ;-) ...but don't think it's too hard to find a good canary. What about "GAHBCFG_GLBL_INTR_EN" ? From a quick glance it looks like the driver seems to set that bit during driver startup and then it stays on until driver shutdown. The databook that I definitely won't admit to having almost certainly says that this register resets to 0 on all hardware and it's applicable to both host and device. I think you could say that if the register is 0 at resume time that registers must have been lost and you can restore them. I guess if that doesn't work then "GUSBCFG_TOUTCAL" could be used (I think that resets to 0 but must be initted to non-0 by the driver). Yet another register that could probably work as a canary would be "GINTMSK". I believe that inits to all 0 (everything is masked) and obviously to use the device we've got to unmask _some_ interrupts. I can look for more, if need be. -Doug
Hi Doug, Am 15.08.24 um 21:37 schrieb Doug Anderson: > Hi, > > On Wed, Aug 14, 2024 at 2:48 PM Stefan Wahren <wahrenst@gmx.net> wrote: >>>> 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 won't admit to having a DWC2 databook. ;-) you convinced me of the canary approach. I missed one critical point the whole time. The used power domain notifier can/will be called while the USB clock is disabled. So this worked for the Raspberry Pi because the clock is fixed. > ...but don't think it's too hard to find a good canary. What about > "GAHBCFG_GLBL_INTR_EN" ? From a quick glance it looks like the driver > seems to set that bit during driver startup and then it stays on until > driver shutdown. The databook that I definitely won't admit to having > almost certainly says that this register resets to 0 on all hardware > and it's applicable to both host and device. I think you could say > that if the register is 0 at resume time that registers must have been > lost and you can restore them. There are several reason to not use the GAHBCFG_GLBL_INTR_EN. One is the fact that the driver disabled this flag at several places, not just on shutdown. Another reason is that the register layout of GAHBCFG on BCM2835 is customized ( see last page of datasheet [1]). I dumped the relevant registers with a unmodified dwc2 driver and the outcome is a little bit unexpected (0 = PRE_POWER_OFF, 3 = POWER_ON): [ 169.101071] dwc2 20980000.usb: dwc2_suspend enter GAHBCFG = 00000031 [ 169.101143] dwc2 20980000.usb: dwc2_suspend enter GUSBCFG = 20001707 [ 169.101172] dwc2 20980000.usb: dwc2_suspend enter GINTMSK = f3000806 [ 169.105888] dwc2 20980000.usb: dwc2_power_notifier: 0 GAHBCFG = 00000031 [ 169.105962] dwc2 20980000.usb: dwc2_power_notifier: 0 GUSBCFG = 20001707 [ 169.105994] dwc2 20980000.usb: dwc2_power_notifier: 0 GINTMSK = f3000806 [ 174.248046] dwc2 20980000.usb: dwc2_power_notifier: 3 GAHBCFG = 0000001f [ 174.248118] dwc2 20980000.usb: dwc2_power_notifier: 3 GUSBCFG = 20402700 [ 174.248148] dwc2 20980000.usb: dwc2_power_notifier: 3 GINTMSK = f3000806 [ 174.253086] dwc2 20980000.usb: dwc2_resume enter: GAHBCFG = 0000001f [ 174.253162] dwc2 20980000.usb: dwc2_resume enter: GUSBCFG = 20402700 [ 174.253190] dwc2 20980000.usb: dwc2_resume enter: GINTMSK = f3000806 > I guess if that doesn't work then "GUSBCFG_TOUTCAL" could be used (I > think that resets to 0 but must be initted to non-0 by the driver). Yes this looks good and match with the trace above. The driver seems to initialize this once and a quick test seems to work so far. I will stick to this. > Yet another register that could probably work as a canary would be > "GINTMSK". I believe that inits to all 0 (everything is masked) and > obviously to use the device we've got to unmask _some_ interrupts. I don't know why but this didn't worked according to trace, but i also didn't noticed a interrupt after enabling of the power domain.Thanks [1] - https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf > I can look for more, if need be. > > -Doug >
diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index 9919ab725d54..a3263cfdedac 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -391,6 +391,22 @@ int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup, return dwc2_gadget_exit_hibernation(hsotg, rem_wakeup, reset); } +int dwc2_enter_poweroff(struct dwc2_hsotg *hsotg) +{ + if (dwc2_is_host_mode(hsotg)) + return dwc2_host_enter_poweroff(hsotg); + else + return dwc2_gadget_enter_poweroff(hsotg); +} + +int dwc2_exit_poweroff(struct dwc2_hsotg *hsotg) +{ + if (dwc2_is_host_mode(hsotg)) + return dwc2_host_exit_poweroff(hsotg); + else + return dwc2_gadget_exit_poweroff(hsotg); +} + /* * Do core a soft reset of the core. Be careful with this because it * resets all the internal state machines of the core. diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 2bd74f3033ed..9ab755cc3081 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -9,6 +9,7 @@ #define __DWC2_CORE_H__ #include <linux/acpi.h> +#include <linux/notifier.h> #include <linux/phy/phy.h> #include <linux/regulator/consumer.h> #include <linux/usb/gadget.h> @@ -1080,6 +1081,8 @@ struct dwc2_hsotg { struct regulator *vbus_supply; struct regulator *usb33d; + struct notifier_block genpd_nb; + spinlock_t lock; void *priv; int irq; @@ -1316,6 +1319,8 @@ int dwc2_exit_partial_power_down(struct dwc2_hsotg *hsotg, int rem_wakeup, int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg, int is_host); int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup, int reset, int is_host); +int dwc2_enter_poweroff(struct dwc2_hsotg *hsotg); +int dwc2_exit_poweroff(struct dwc2_hsotg *hsotg); void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg); int dwc2_phy_init(struct dwc2_hsotg *hsotg, bool select_phy); @@ -1435,6 +1440,8 @@ int dwc2_hsotg_tx_fifo_total_depth(struct dwc2_hsotg *hsotg); int dwc2_hsotg_tx_fifo_average_depth(struct dwc2_hsotg *hsotg); void dwc2_gadget_init_lpm(struct dwc2_hsotg *hsotg); void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg); +int dwc2_gadget_enter_poweroff(struct dwc2_hsotg *hsotg); +int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg); static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) { hsotg->fifo_map = 0; } #else @@ -1482,6 +1489,10 @@ static inline int dwc2_hsotg_tx_fifo_average_depth(struct dwc2_hsotg *hsotg) { return 0; } static inline void dwc2_gadget_init_lpm(struct dwc2_hsotg *hsotg) {} static inline void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg) {} +static inline int dwc2_gadget_enter_poweroff(struct dwc2_hsotg *hsotg) +{ return 0; } +static inline int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg) +{ return 0; } static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) {} #endif @@ -1505,6 +1516,8 @@ int dwc2_host_exit_partial_power_down(struct dwc2_hsotg *hsotg, void dwc2_host_enter_clock_gating(struct dwc2_hsotg *hsotg); void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup); bool dwc2_host_can_poweroff_phy(struct dwc2_hsotg *dwc2); +int dwc2_host_enter_poweroff(struct dwc2_hsotg *hsotg); +int dwc2_host_exit_poweroff(struct dwc2_hsotg *hsotg); static inline void dwc2_host_schedule_phy_reset(struct dwc2_hsotg *hsotg) { schedule_work(&hsotg->phy_reset_work); } #else @@ -1544,6 +1557,10 @@ static inline void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup) {} static inline bool dwc2_host_can_poweroff_phy(struct dwc2_hsotg *dwc2) { return false; } +static inline int dwc2_host_enter_poweroff(struct dwc2_hsotg *hsotg) +{ return 0; } +static inline int dwc2_host_exit_poweroff(struct dwc2_hsotg *hsotg) +{ return 0; } static inline void dwc2_host_schedule_phy_reset(struct dwc2_hsotg *hsotg) {} #endif diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index e7bf9cc635be..38f0112970fe 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -5710,3 +5710,52 @@ void dwc2_gadget_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup) hsotg->lx_state = DWC2_L0; hsotg->bus_suspended = false; } + +int dwc2_gadget_enter_poweroff(struct dwc2_hsotg *hsotg) +{ + int ret; + + dev_dbg(hsotg->dev, "Entering device power off.\n"); + + /* Backup all registers */ + ret = dwc2_backup_global_registers(hsotg); + if (ret) { + dev_err(hsotg->dev, "%s: failed to backup global registers\n", + __func__); + return ret; + } + + ret = dwc2_backup_device_registers(hsotg); + if (ret) { + dev_err(hsotg->dev, "%s: failed to backup device registers\n", + __func__); + return ret; + } + + dev_dbg(hsotg->dev, "Entering device power off completed.\n"); + return 0; +} + +int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg) +{ + int ret; + + dev_dbg(hsotg->dev, "Exiting device power off.\n"); + + ret = dwc2_restore_global_registers(hsotg); + if (ret) { + dev_err(hsotg->dev, "%s: failed to restore registers\n", + __func__); + return ret; + } + + ret = dwc2_restore_device_registers(hsotg, 0); + if (ret) { + dev_err(hsotg->dev, "%s: failed to restore device registers\n", + __func__); + return ret; + } + + dev_dbg(hsotg->dev, "Exiting device power off completed.\n"); + return 0; +} diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index cb54390e7de4..22afdafb474e 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -5993,3 +5993,52 @@ void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup) jiffies + msecs_to_jiffies(71)); } } + +int dwc2_host_enter_poweroff(struct dwc2_hsotg *hsotg) +{ + int ret; + + dev_dbg(hsotg->dev, "Entering host power off.\n"); + + /* Backup all registers */ + ret = dwc2_backup_global_registers(hsotg); + if (ret) { + dev_err(hsotg->dev, "%s: failed to backup global registers\n", + __func__); + return ret; + } + + ret = dwc2_backup_host_registers(hsotg); + if (ret) { + dev_err(hsotg->dev, "%s: failed to backup host registers\n", + __func__); + return ret; + } + + dev_dbg(hsotg->dev, "Entering host power off completed.\n"); + return 0; +} + +int dwc2_host_exit_poweroff(struct dwc2_hsotg *hsotg) +{ + int ret; + + dev_dbg(hsotg->dev, "Exiting host power off.\n"); + + ret = dwc2_restore_global_registers(hsotg); + if (ret) { + dev_err(hsotg->dev, "%s: failed to restore registers\n", + __func__); + return ret; + } + + ret = dwc2_restore_host_registers(hsotg); + if (ret) { + dev_err(hsotg->dev, "%s: failed to restore host registers\n", + __func__); + return ret; + } + + dev_dbg(hsotg->dev, "Exiting host power off completed.\n"); + return 0; +} diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index 7b84416dfc2b..b97eefc18a6b 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -16,6 +16,7 @@ #include <linux/platform_device.h> #include <linux/phy/phy.h> #include <linux/platform_data/s3c-hsotg.h> +#include <linux/pm_domain.h> #include <linux/reset.h> #include <linux/usb/of.h> @@ -307,6 +308,8 @@ static void dwc2_driver_remove(struct platform_device *dev) struct dwc2_gregs_backup *gr; int ret = 0; + dev_pm_genpd_remove_notifier(&dev->dev); + gr = &hsotg->gr_backup; /* Exit Hibernation when driver is removed. */ @@ -421,6 +424,31 @@ int dwc2_check_core_version(struct dwc2_hsotg *hsotg) return 0; } +static int dwc2_power_notifier(struct notifier_block *nb, + unsigned long action, void *data) +{ + struct dwc2_hsotg *hsotg = container_of(nb, struct dwc2_hsotg, + genpd_nb); + int ret; + + switch (action) { + case GENPD_NOTIFY_ON: + ret = dwc2_exit_poweroff(hsotg); + if (ret) + dev_err(hsotg->dev, "exit poweroff failed\n"); + break; + case GENPD_NOTIFY_PRE_OFF: + ret = dwc2_enter_poweroff(hsotg); + if (ret) + dev_err(hsotg->dev, "enter poweroff failed\n"); + break; + default: + break; + } + + return NOTIFY_OK; +} + /** * dwc2_driver_probe() - Called when the DWC_otg core is bound to the DWC_otg * driver @@ -620,6 +648,10 @@ static int dwc2_driver_probe(struct platform_device *dev) } } #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */ + + hsotg->genpd_nb.notifier_call = dwc2_power_notifier; + dev_pm_genpd_add_notifier(&dev->dev, &hsotg->genpd_nb); + return 0; #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
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. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- Any feedback is appreciated. drivers/usb/dwc2/core.c | 16 ++++++++++++ drivers/usb/dwc2/core.h | 17 +++++++++++++ drivers/usb/dwc2/gadget.c | 49 +++++++++++++++++++++++++++++++++++++ drivers/usb/dwc2/hcd.c | 49 +++++++++++++++++++++++++++++++++++++ drivers/usb/dwc2/platform.c | 32 ++++++++++++++++++++++++ 5 files changed, 163 insertions(+) -- 2.34.1