Hi Roger,
On Wed, Sep 25, 2024, Roger Quadros wrote:
> Hello Thinh,
>
> On 17/04/2024 02:41, Thinh Nguyen wrote:
> > GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY should be cleared
> > during initialization. Suspend during initialization can result in
> > undefined behavior due to clock synchronization failure, which often
> > seen as core soft reset timeout.
> >
> > The programming guide recommended these bits to be cleared during
> > initialization for DWC_usb3.0 version 1.94 and above (along with
> > DWC_usb31 and DWC_usb32). The current check in the driver does not
> > account if it's set by default setting from coreConsultant.
> >
> > This is especially the case for DRD when switching mode to ensure the
> > phy clocks are available to change mode. Depending on the
> > platforms/design, some may be affected more than others. This is noted
> > in the DWC_usb3x programming guide under the above registers.
> >
> > Let's just disable them during driver load and mode switching. Restore
> > them when the controller initialization completes.
> >
> > Note that some platforms workaround this issue by disabling phy suspend
> > through "snps,dis_u3_susphy_quirk" and "snps,dis_u2_susphy_quirk" when
> > they should not need to.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
>
> This patch is causing system suspend failures on TI AM62 platforms [1]
>
> I will try to explain why.
> Before this patch, both DWC3_GUSB3PIPECTL_SUSPHY and DWC3_GUSB2PHYCFG_SUSPHY
> bits (hence forth called 2 SUSPHY bits) were being set during initialization
> and even during re-initialization after a system suspend/resume.
>
> These bits are required to be set for system suspend/resume to work correctly
> on AM62 platforms.
Is it only for suspend or both suspend and resume?
>
> After this patch, the bits are only set when Host controller starts or
> when Gadget driver starts.
>
> On AM62 platform we have 2 USB controllers, one in Host and one in Dual role.
> Just after boot, for the Host controller we have the 2 SUSPHY bits set but
> for the Dual-Role controller, as no role has started the 2 SUSPHY bits are
> not set. Thus system suspend resume will fail.
>
> On the other hand, if we load a gadget driver just after boot then both
> controllers have the 2 SUSPHY bits set and system suspend resume works for
> the first time.
> However, after system resume, the core is re-initialized so the 2 SUSPHY bits
> are cleared for both controllers. For host controller it is never set again.
> For gadget controller as gadget start is called, the 2 SUSPHY bits are set
> again. The second system suspend resume will still fail as one controller
> (Host) doesn't have the 2 SUSPHY bits set.
>
> To summarize, the existing solution is not sufficient for us to have a
> reliable behavior. We need the 2 SUSPHY bits to be set regardless of what
> role we are in or whether the role has started or not.
>
> My suggestion is to move back the SUSPHY enable to end of dwc3_core_init().
> Then if SUSPHY needs to be disabled for DRD role switching, it should be
> disabled and enabled exactly there.
>
> What do you suggest?
>
> [1] - https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-kernel/20240904194229.109886-1-msp@xxxxxxxxxxxx/__;!!A4F2R9G_pg!Y10q3gwCzryOoiXpk6DMGn74iFQIg6GloY10J16kWCbqwgS1Algo5HRg05vm38dMw8n47qmKpqJlyXt9Kqlm$
>
Thanks for reporting the issue.
This is quite an interesting behavior. As you said, we will need to
isolate this change to only during DRD role switch.
We may not necessarily just enable at the end of dwc3_core_init() since
that would keep the SUSPHY bits on during the DRD role switch. If this
issue only occurs before suspend, perhaps we can check and set these
bits during suspend or dwc3_core_exit() instead?
BR,
Thinh
