Messages in this thread | | | Date | Wed, 14 Sep 2022 17:12:39 +0530 | Subject | Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc | From | Krishna Kurapati PSSNV <> |
| |
On 9/14/2022 2:37 PM, Johan Hovold wrote: > [ Please try to wrap your replies at 72 columns or so. ] > > On Wed, Sep 14, 2022 at 02:28:31PM +0530, Rajendra Nayak wrote: >> >> On 9/14/2022 2:07 PM, 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). >>>> >>> Hi Johan, >>> >>> 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 >>> >>> 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. >> >> ok, so perhaps the commit msg should be updated to something like >> >> 'USB on sc7280 needs to prevent the GDSC from being turned OFF for a couple of reasons >> 1. To prevent re-init and re-enumeration of all connected devices resulting in additional delay coming out of low power states >> 2. To support wakeups from connected devices from low power states' > > The fundamental issue here is that state is lost during suspend which > the driver doesn't currently (or can not) restore. Doesn't hurt to > mention the specific consequences you list above as well. > >>>> 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. >> >> It perhaps makes sense to update that as well, and given this is a compute specific chipset and we dont have to worry about >> USB in device mode, its safe to assume if and when that controller is used (in future designs) it would only support host mode? > > What would be the problem when using the controller in peripheral mode? > Don't you want to retain the controller state when suspended also in > that case? > Hi Johan. You're right. When peripheral bus suspend lands in, we might need to keep GDSC ON in device mode as well.
Regards, Krishna,
| |