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

[Roger Quadros <rogerq@kernel.org>]

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/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 for your suggestions. Yes, I will send a fix soon.

-- 
cheers,
-roger