Re: [PATCH] usb: dwc3: core: Fix system suspend on TI AM62 platforms

[Roger Quadros]

Hi Thinh,

On 05/10/2024 04:04, Thinh Nguyen wrote:
> Hi,
> 
> On Tue, Oct 01, 2024, Roger Quadros wrote:
>> Since commit 6d735722063a ("usb: dwc3: core: Prevent phy suspend during init"),
>> system suspend is broken on AM62 TI platforms.
>>
>> Before that commit, both DWC3_GUSB3PIPECTL_SUSPHY and DWC3_GUSB2PHYCFG_SUSPHY
>> bits (hence forth called 2 SUSPHY bits) were being set during core
>> initialization and even during core re-initialization after a system
>> suspend/resume.
>>
>> These bits are required to be set for system suspend/resume to work correctly
>> on AM62 platforms.
>>
>> Since that commit, the 2 SUSPHY bits are not set for DEVICE/OTG mode if gadget
>> driver is not loaded and started.
>> For Host mode, the 2 SUSPHY bits are set before the first system suspend but
>> get cleared at system resume during core re-init and are never set again.
>>
>> This patch resovles these two issues by ensuring the 2 SUSPHY bits are set
>> before system suspend and restored to the original state during system resume.
>>
>> Cc: stable@xxxxxxxxxxxxxxx # v6.9+
>> Fixes: 6d735722063a ("usb: dwc3: core: Prevent phy suspend during init")
>> Link: https://urldefense.com/v3/__https://lore.kernel.org/all/1519dbe7-73b6-4afc-bfe3-23f4f75d772f@xxxxxxxxxx/__;!!A4F2R9G_pg!ahChm4MaKd6VGYqbnM4X1_pY_jqavYDv5HvPFbmicKuhvFsBwlEFi1xO5itGuHmfjbRuUSzReJISf5-1gXpr$ 
>> Signed-off-by: Roger Quadros <rogerq@xxxxxxxxxx>
>> ---
>>  drivers/usb/dwc3/core.c | 16 ++++++++++++++++
>>  drivers/usb/dwc3/core.h |  2 ++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 9eb085f359ce..1233922d4d54 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -2336,6 +2336,9 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>  	u32 reg;
>>  	int i;
>>  
>> +	dwc->susphy_state = !!(dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)) &
>> +			    DWC3_GUSB2PHYCFG_SUSPHY);
>> +
>>  	switch (dwc->current_dr_role) {
>>  	case DWC3_GCTL_PRTCAP_DEVICE:
>>  		if (pm_runtime_suspended(dwc->dev))
>> @@ -2387,6 +2390,11 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>  		break;
>>  	}
>>  
>> +	if (!PMSG_IS_AUTO(msg)) {
>> +		if (!dwc->susphy_state)
>> +			dwc3_enable_susphy(dwc, true);
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -2454,6 +2462,14 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>  		break;
>>  	}
>>  
>> +	if (!PMSG_IS_AUTO(msg)) {
>> +		/* dwc3_core_init_for_resume() disables SUSPHY so just handle
>> +		 * the enable case
>> +		 */
> 
> Can we note that this is a particular behavior needed for AM62 here?
> And can we use this comment style:

Looking at this again, this fix is not specific to AM62 but for all platforms.
e.g. if Host Role was already started when going to system suspend, SUSPHY bits
were enabled, then after system resume SUSPHY bits are cleared at dwc3_core_init_for_resume().

Host stop/start was not called so SUSPHY bits remain cleared. So here
we deal with enabling SUSPHY.

> 
> /*
>  * xxxxx
>  * xxxxx
>  */
> 
> 
>> +		if (dwc->susphy_state)
> 
> Shouldn't we check for if (!dwc->susphy_state) and clear the susphy
> bits?
> 
>> +			dwc3_enable_susphy(dwc, true);
> 
> The dwc3_enable_susphy() set and clear both GUSB3PIPECTL_SUSPHY and
> GUSB2PHYCFG_SUSPHY, perhaps we should split that function out so we can
> only need to set for GUSB2PHYCFG_SUSPHY since you only track for that.
> 
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index c71240e8f7c7..b2ed5aba4c72 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1150,6 +1150,7 @@ struct dwc3_scratchpad_array {
>>   * @sys_wakeup: set if the device may do system wakeup.
>>   * @wakeup_configured: set if the device is configured for remote wakeup.
>>   * @suspended: set to track suspend event due to U3/L2.
>> + * @susphy_state: state of DWC3_GUSB2PHYCFG_SUSPHY before PM suspend.
>>   * @imod_interval: set the interrupt moderation interval in 250ns
>>   *			increments or 0 to disable.
>>   * @max_cfg_eps: current max number of IN eps used across all USB configs.
>> @@ -1382,6 +1383,7 @@ struct dwc3 {
>>  	unsigned		sys_wakeup:1;
>>  	unsigned		wakeup_configured:1;
>>  	unsigned		suspended:1;
>> +	unsigned		susphy_state:1;
>>  
>>  	u16			imod_interval;
>>  
>>
>> ---
>> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
>> change-id: 20240923-am62-lpm-usb-f420917bd707
>>
>> Best regards,
>> -- 
>> Roger Quadros <rogerq@xxxxxxxxxx>
>>
> 
> <rant/>
> While reviewing your change, I see that we misuse the
> dis_u2_susphy_quirk to make this property a conditional thing during
> suspend and resume for certain platform. That bugs me because we can't
> easily change it without the reported hardware to test.
> </rant>
> 
> Thanks for the patch!
> 
> BR,
> Thinh

-- 
cheers,
-roger