Re: [PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Fri, Sep 27, 2024, Roger Quadros wrote:
> 
> 
> On 27/09/2024 00:51, Thinh Nguyen wrote:
> > 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@vger.kernel.org
> >>> Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
> >>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> >>
> >> 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?
> 
> I'm sure about suspend. It is not possible to toggle those bits while in system
> suspend so we can't really say if it is required exclusively for system resume or not.
> 
> > 
> >>
> >> 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@baylibre.com/__;!!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?
> 
> dwc3_core_exit() is not always called in the system suspend path so it
> may not be sufficient.
> 
> Any issues if we set this these bits at the end of dwc3_suspend_common()
> irrespective of runtime suspend or system suspend and operating role?

There should be no issue at this point. The problem occurs during
initialization that involves initializing the usb role.

> And should we restore these bits in dwc3_resume_common() to the state they
> were before dwc3_suspend_common()?
> 

Sounds good to me! Would you mind send a fix patch?

Thanks,
Thinh