Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Johan Hovold <johan@kernel.org>]

On Wed, Sep 14, 2022 at 02:07:06PM +0530, Krishna Kurapati PSSNV wrote:
> On 9/14/2022 12:39 PM, Johan Hovold wrote:
> > On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
> >> USB on sc7280 cannot support wakeups from low power states
> >> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> >> only transitions to RET in low power.
> > 
> > It seems this isn't just needed for wakeup to work. On both sc7280 and
> > sc8280xp the controller doesn't resume properly if the domain has been
> > powered off (i.e. regardless of whether wakeup is enabled or not).

>    I believe you are referring to the reinit that happens in xhci resume 
> path after wakeup happens:
> 
> [   48.675839] xhci-hcd xhci-hcd.14.auto: xHC error in resume, USBSTS 
> 0x411, Reinit

Right, and on sc8280xp the controller doesn't survive resume at all.
 
> I see that when USB GDSC is not in retention, we don't retain controller 
> state and go for reinit and re-enum of connected devices. We are seeing 
> an additional delay of around ~0.7 sec (in chromebooks running on 
> SC7280) in the wakeup path for re-enumeration of connected USB devices. 
> To avoid this, we wanted to put GDSC in retention during PM suspend.

Thanks for confirming. This needs to be described in the commit message
of this patches, which currently only says that this change is needed
for wakeup.

> > Are you sure there's no state that needs to be retained regardless of
> > the wakeup setting?
> > 
> >> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> >> ---
> >>   drivers/clk/qcom/gcc-sc7280.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
> >> index 7ff64d4d5920..de29a034e725 100644
> >> --- a/drivers/clk/qcom/gcc-sc7280.c
> >> +++ b/drivers/clk/qcom/gcc-sc7280.c
> >> @@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
> >>   	.pd = {
> >>   		.name = "gcc_usb30_prim_gdsc",
> >>   	},
> >> -	.pwrsts = PWRSTS_OFF_ON,
> >> +	.pwrsts = PWRSTS_RET_ON,
> >>   	.flags = VOTABLE,
> >>   };
> > 
> > And what about gcc_usb30_sec_gdsc?
> 
> Currently wakeup is not enabled on secondary controller as its not 
> required for end product platform (herobrine variant). So leaving the 
> usb30_sec_gdsc as it is for now.

This is generic code that can be used in different products and if this
is needed for the first controller it is also needed for the second
(i.e. even if wakeup isn't wired up or used on the products you are
working on).

Johan