Hi Dan,

On Wed, 15 Jul 2020 at 08:48, Dan Robertson [off-list ref] wrote:

On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote:

quoted

Hi Dan,

On Tue, 14 Jul 2020 at 19:00, Dan Robertson [off-list ref] wrote:

quoted

Hi Anand!

On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote:

quoted

hi Dan,

On Mon, 13 Jul 2020 at 21:37, Dan Robertson [off-list ref] wrote:

quoted

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.

quoted

* 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!

quoted

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.

quoted

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

quoted

quoted

* 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?

quoted

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel