Hi Dan, On Fri, 17 Jul 2020 at 14:31, Anand Moon <linux.amoon@xxxxxxxxx> wrote: > > 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 After confirming that the suspend resume feature is working correctly I found the solution to the reset warning on 5.8.x kernel Please can you try this following patch. $ cat resetwarn.patch diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c index 1f7f4d88ed9d..60a6f49139fd 100644 --- a/drivers/usb/dwc3/dwc3-meson-g12a.c +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c @@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) platform_set_drvdata(pdev, priv); - priv->reset = devm_reset_control_get_shared(dev, NULL); + priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson"); if (IS_ERR(priv->reset)) { ret = PTR_ERR(priv->reset); dev_err(dev, "failed to get device reset, err=%d\n", ret); -Anand
