Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Dan Robertson <dan@dlrobertson.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3277 bytes --]

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@dlrobertson.com> 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():

> * 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?
> 
> Best Regards
> -Anand

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.

Cheers,

 - Dan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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