Hi Dan, On Wed, 15 Jul 2020 at 21:53, Anand Moon <linux.amoon@xxxxxxxxx> wrote: > > Hi Dan, > > On Wed, 15 Jul 2020 at 08:48, Dan Robertson <dan@xxxxxxxxxxxxxxx> wrote: > > > > On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote: > > > Hi Dan, > > > > > > On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@xxxxxxxxxxxxxxx> wrote: > > > > > > > > Hi Anand! > > > > > > > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote: > > > > > hi Dan, > > > > > > > > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@xxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > When testing suspend for another driver I noticed the following warning: > > > > > > > > > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > > > > > > Hardware name: Hardkernel ODROID-N2 (DT) > > > > > > [..] > > > > > > pc : reset_control_assert+0x184/0x19c > > > > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c > > > > > > [..] > > > > > > Call trace: > > > > > > reset_control_assert+0x184/0x19c > > > > > > dwc3_meson_g12a_suspend+0x68/0x7c > > > > > > platform_pm_suspend+0x28/0x54 > > > > > > __device_suspend+0x590/0xabc > > > > > > dpm_suspend+0x104/0x404 > > > > > > dpm_suspend_start+0x84/0x1bc > > > > > > suspend_devices_and_enter+0xc4/0x4fc > > > > > > > > > > > > In my limited experience and knowlege it appears that we hit this > > > > > > because the reset control was switched to shared and the the use > > > > > > of the reset control was not changed. > > > > > > > > > > > > > * Calling reset_control_assert without first calling reset_control_deassert > > > > > > > * is not allowed on a shared reset control. Calling reset_control_reset is > > > > > > > * also not allowed on a shared reset control. > > > > > > > > > > > > The above snippet from reset_control_get_shared() seems to indicate that > > > > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe() > > > > > > and reset_control_deassert is not guaranteed to have been called > > > > > > before dwc3_meson_g12a_suspend() and reset_control_assert(). > > > > > > > > > > > > After some basic tests with the following patch I no longer hit the > > > > > > warning. Comments and critiques on the patch are welcome. If there is a > > > > > > reason for the current use of the reset control, I'd love to learn why! > > > > > > Like I said before, I have not really looked at this driver before and > > > > > > have verify limited experience with reset controls... Was working on > > > > > > another driver, hit the warning, and thought I'd take a shot at the > > > > > > fix :-) > > > > > > > > > > > Thanks, I was also looking into this issue > > > > > > > > Awesome! > > > > > > > > > So best Fix to this issue is to drop the call of reset_control_assert > > > > > from dwc3_meson_g12a_suspend > > > > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume > > > > > With these changes we do not see the warning on suspend and resume > > > > > > > > We definitely would avoid hitting the warning without the > > > > reset_control_(de)assert calls in suspend and resume. That is a valid use of > > > > the reset control, but why would that be best? > > > > > > > > From reset_control_reset(): > > > > > > Before entering the suspend state the code tries to do following > > > clk_bulk_disable_unprepare > > > regulator_disable > > > phy_power_off > > > phy_exit > > > > > > After this operation it's needless to call *reset_control_assert* > > > I tried to move this call before all the above operations > > > but still no success with this. > > > > How so? Once the reset() is removed prope() and deassert() is guaranteed > > to have been called before suspend, like what is in the patch and similar > > to other uses of shared reset controllers, this is possible. > > > > > Similarly on resume we should avoid calling resume > > > *reset_control_deassert* earlier > > > as the core is just reinitializing the devices. > > > clk_bulk_prepare_enable > > > usb_init > > > phy_init > > > phy_power_on > > > regulator_enable > > > > > > > > > * Consumers must not use reset_control_(de)assert on shared reset lines when > > > > > * reset_control_reset has been used. > > > > > > > > If we use reset_control_reset() then we can not (de)assert the reset line > > > > on suspend/resume or any other time. Wouldn't we want to be able to > > > > (de)assert the reset line? And why do we no longer want to (de)assert the > > > > reset line on suspend/resume? > > > > > > > > > > Can you try this attached patch? > > > > > > > > > I think I had already tested something similar. Removing the (de)assert calls > > > > but keeping reset will definitely remove the warning, but it means we can not > > > > (de)assert the line. My guess is that this is not what we want, but I could be > > > > wrong. Thoughts, input, or references to documentation on this would be > > > > appreciated. > > > > > > > > > > As per my knowledge suspend to mem will do complete power down of the > > > devices with support suspend and resume feature sequentially and then it tries > > > to bring the device up one by one. > > > So it should also take care of reset lines as well. > > > > So do we only _actually_ care about the reset line in the probe? Seems like we > > should remove the reset controller from the structure if that is the case. > > > > Cheers, > > > > - Dan > > Sorry I am not the _expert_ in suspend/resume feature but I can debug this, > Certainly we need to look into whether reset controller calls are needed > to suspend or resume for this driver. > > First thing we need to get the RTC driver to work on Odroid N2 for > suspend wakeup > to work properly. > For this I have created the following patches. > > [0] https://patchwork.kernel.org/cover/11665865/ > > With these patches the RTC feature for wake up is broken right now in > my testing. > Once I get something to work further I will update you. > > --Anand Sorry for the _noise_ :‑( This feature seems to be working fine with VRTC drivers. I have tested this with a pre-compiled image of Archlinux distro. [root@alarm alarm]# uname -a Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux [root@alarm alarm]# rtcwake -s 30 -m mem rtcwake: assuming RTC uses UTC ... rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan 1 00:10:14 1970 [ 583.591477] PM: suspend entry (deep) [ 583.593737] Filesystems sync: 0.002 seconds [ 583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done. [ 583.825802] OOM killer disabled. [ 583.828966] Freezing remaining freezable tasks ... (elapsed 0.003 seconds) done. [ 583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache [ 584.020094] PM: suspend devices took 0.190 seconds [ 584.070586] Disabling non-boot CPUs ... [ 584.075037] CPU1: shutdown [ 584.075223] psci: CPU1 killed (polled 0 ms) [ 584.097199] CPU2: shutdown [ 584.098546] psci: CPU2 killed (polled 0 ms) [ 584.115370] CPU3: shutdown [ 584.116500] psci: CPU3 killed (polled 0 ms) [ 584.128116] CPU4: shutdown [ 584.129235] psci: CPU4 killed (polled 10 ms) [ 584.140122] CPU5: shutdown [ 584.147289] psci: CPU5 killed (polled 0 ms) bl30 get wakeup sources! process command 00000006 bl30 enter suspend! Little core clk suspend rate 1896000000 Big core clk suspend rate 24000000 store restore gp0 pll suspend_counter: 3 Enter ddr suspend ddr suspend time: 16us alarm=31S process command 00000001 GPIOA_11/13 off cec ver:2018/04/19 CEC cfg:0x0000 WAKEUP GPIO cfg:0x00000000 use vddee new table! use vddee new table! exit_reason:0x03 Enter ddr resume DMC_DRAM_STAT3: 0x544 ddr resume time: 3188us store restore gp0 pll cfg15 33b00000 cfg15 33b00000 Li[ 584.148720] Enabling non-boot CPUs ... [ 584.149124] Detected VIPT I-cache on CPU1 [ 584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034] [ 584.149594] CPU1 is up [ 584.160687] Detected VIPT I-cache on CPU2 [ 584.160730] arch_timer: CPU2: Trapping CNTVCT access [ 584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092] [ 584.161327] CPU2 is up [ 584.177645] Detected VIPT I-cache on CPU3 [ 584.177668] arch_timer: CPU3: Trapping CNTVCT access [ 584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092] [ 584.178036] CPU3 is up [ 584.195338] Detected VIPT I-cache on CPU4 [ 584.195361] arch_timer: CPU4: Trapping CNTVCT access [ 584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092] [ 584.195762] CPU4 is up [ 584.213002] Detected VIPT I-cache on CPU5 [ 584.213024] arch_timer: CPU5: Trapping CNTVCT access [ 584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092] [ 584.213450] CPU5 is up ttle core clk resume rate 1896000000 Big core clk resume rate 50000000 [ 584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety Features support found [ 584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for phy/rgmii link mode [ 584.401216] usb usb1: root hub lost power or was reset [ 584.401470] usb usb2: root hub lost power or was reset [ 584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode [ 584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 using xhci-hcd [ 584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd [ 585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3 using xhci-hcd [ 585.333303] PM: resume devices took 1.100 seconds [ 585.333507] OOM killer enabled. [ 585.335549] Restarting tasks ... done. [ 585.378044] PM: suspend exit rtcwake: read rtc alarm failed: Invalid argument [root@alarm alarm]# -Anand
