usb: dwc3: core: Fix system suspend on TI AM62 platforms

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@vger.kernel.org # v6.9+
Fixes: 6d735722063a ("usb: dwc3: core: Prevent phy suspend during init")
Link: https://lore.kernel.org/all/1519dbe7-73b6-4afc-bfe3-23f4f75d772f@kernel.org/
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/usb/dwc3/core.c | 16 ++++++++++++++++
 drivers/usb/dwc3/core.h |  2 ++
 2 files changed, 18 insertions(+)


---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20240923-am62-lpm-usb-f420917bd707

Best regards,

Hi,

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@vger.kernel.org # 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@kernel.org/__;!!A4F2R9G_pg!ahChm4MaKd6VGYqbnM4X1_pY_jqavYDv5HvPFbmicKuhvFsBwlEFi1xO5itGuHmfjbRuUSzReJISf5-1gXpr$ 
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  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:
> 
> /*
>  * xxxxx
>  * xxxxx
>  */

OK.

> 
> 
>> +		if (dwc->susphy_state)
> 
> Shouldn't we check for if (!dwc->susphy_state) and clear the susphy
> bits?

In that case it would have already been cleared so no need to check
and clear again.

> 
>> +			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.

As  dwc3_enable_susphy() sets and clears both GUSB3PIPECTL_SUSPHY and
GUSB2PHYCFG_SUSPHY together it doesn't really help to track both
separately, but just complicates things.

> 
>> +	}
>> +
>>  	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@kernel.org>
>>
> 
> <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>

No it is not conditional. if dis_u2_susphy_quirk or dis_u3_susphy_quirk
is set then we never enable the respective U2/U3 SUSPHY bit.

> 
> Thanks for the patch!
> 
> BR,
> Thinh

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@vger.kernel.org # 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@kernel.org/__;!!A4F2R9G_pg!ahChm4MaKd6VGYqbnM4X1_pY_jqavYDv5HvPFbmicKuhvFsBwlEFi1xO5itGuHmfjbRuUSzReJISf5-1gXpr$ 
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  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@kernel.org>
>>
> 
> <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

Hi Roger,

On Tue, Oct 08, 2024, Roger Quadros wrote:
> 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@vger.kernel.org # 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@kernel.org/__;!!A4F2R9G_pg!ahChm4MaKd6VGYqbnM4X1_pY_jqavYDv5HvPFbmicKuhvFsBwlEFi1xO5itGuHmfjbRuUSzReJISf5-1gXpr$ 
> >> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> >> ---
> >>  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.
> 

It's true that we have a bug where the SUSPHY bits remain disabled after
suspend. However, the SUSPHY bits needing to be set during suspend is
unique to AM62. Let's add this note in the dwc3_suspend_common() check.

Thanks,
Thinh

Hi,

On Mon, Oct 07, 2024, Roger Quadros wrote:
> Hi,
> 
> 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@vger.kernel.org # 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@kernel.org/__;!!A4F2R9G_pg!ahChm4MaKd6VGYqbnM4X1_pY_jqavYDv5HvPFbmicKuhvFsBwlEFi1xO5itGuHmfjbRuUSzReJISf5-1gXpr$ 
> >> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> >> ---
> >>  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:
> > 
> > /*
> >  * xxxxx
> >  * xxxxx
> >  */
> 
> OK.
> 
> > 
> > 
> >> +		if (dwc->susphy_state)
> > 
> > Shouldn't we check for if (!dwc->susphy_state) and clear the susphy
> > bits?
> 
> In that case it would have already been cleared so no need to check
> and clear again.
> 
> > 
> >> +			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.
> 
> As  dwc3_enable_susphy() sets and clears both GUSB3PIPECTL_SUSPHY and
> GUSB2PHYCFG_SUSPHY together it doesn't really help to track both
> separately, but just complicates things.

Then we should check if either GUSB2PHYCFG_SUSPHY or GUSB3PIPECTL_SUSPHY
is set, then apply this.

> 
> > 
> >> +	}
> >> +
> >>  	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@kernel.org>
> >>
> > 
> > <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>
> 
> No it is not conditional. if dis_u2_susphy_quirk or dis_u3_susphy_quirk
> is set then we never enable the respective U2/U3 SUSPHY bit.
> 

I'm not referring to your change. I was referring to this in particular:
bcb128777af5 ("usb: dwc3: core: Suspend PHYs on runtime suspend in host mode")

BR,
Thinh

On 08/10/2024 23:57, Thinh Nguyen wrote:
> Hi,
> 
> On Mon, Oct 07, 2024, Roger Quadros wrote:
>> Hi,
>>
>> 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@vger.kernel.org # 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@kernel.org/__;!!A4F2R9G_pg!ahChm4MaKd6VGYqbnM4X1_pY_jqavYDv5HvPFbmicKuhvFsBwlEFi1xO5itGuHmfjbRuUSzReJISf5-1gXpr$ 
>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>>  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:
>>>
>>> /*
>>>  * xxxxx
>>>  * xxxxx
>>>  */
>>
>> OK.
>>
>>>
>>>
>>>> +		if (dwc->susphy_state)
>>>
>>> Shouldn't we check for if (!dwc->susphy_state) and clear the susphy
>>> bits?
>>
>> In that case it would have already been cleared so no need to check
>> and clear again.
>>
>>>
>>>> +			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.
>>
>> As  dwc3_enable_susphy() sets and clears both GUSB3PIPECTL_SUSPHY and
>> GUSB2PHYCFG_SUSPHY together it doesn't really help to track both
>> separately, but just complicates things.
> 
> Then we should check if either GUSB2PHYCFG_SUSPHY or GUSB3PIPECTL_SUSPHY
> is set, then apply this.

Yes. I will do this.
> 
>>
>>>
>>>> +	}
>>>> +
>>>>  	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@kernel.org>
>>>>
>>>
>>> <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>
>>
>> No it is not conditional. if dis_u2_susphy_quirk or dis_u3_susphy_quirk
>> is set then we never enable the respective U2/U3 SUSPHY bit.
>>
> 
> I'm not referring to your change. I was referring to this in particular:
> bcb128777af5 ("usb: dwc3: core: Suspend PHYs on runtime suspend in host mode")

I get it now.

On 08/10/2024 23:53, Thinh Nguyen wrote:
> Hi Roger,
> 
> On Tue, Oct 08, 2024, Roger Quadros wrote:
>> 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@vger.kernel.org # 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@kernel.org/__;!!A4F2R9G_pg!ahChm4MaKd6VGYqbnM4X1_pY_jqavYDv5HvPFbmicKuhvFsBwlEFi1xO5itGuHmfjbRuUSzReJISf5-1gXpr$ 
>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>>  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.
>>
> 
> It's true that we have a bug where the SUSPHY bits remain disabled after
> suspend. However, the SUSPHY bits needing to be set during suspend is
> unique to AM62. Let's add this note in the dwc3_suspend_common() check.

Yes I will do that. Thanks!