On 01/10/2024 04:00, Thinh Nguyen wrote:
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/2024090...
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 for your suggestions. Yes, I will send a fix soon.