Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1b0ffb68-d437-1148-4ee9-47b6dd51db7f@quicinc.com>
Date:   Wed, 14 Sep 2022 17:12:39 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb
 gdsc
Content-Language: en-US
To:     Johan Hovold <johan@kernel.org>,
        Rajendra Nayak <quic_rjendra@quicinc.com>
CC:     <andersson@kernel.org>, <agross@kernel.org>,
        <konrad.dybcio@somainline.org>, <mturquette@baylibre.com>,
        <sboyd@kernel.org>, <mka@chromium.org>, <johan+linaro@kernel.org>,
        <dianders@chromium.org>, <linux-clk@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20220901101756.28164-1-quic_rjendra@quicinc.com>
 <20220901101756.28164-3-quic_rjendra@quicinc.com>
 <YyF+IuoDjBZzEQxO@hovoldconsulting.com>
 <33af27a0-85b9-4301-62d9-24132989e26e@quicinc.com>
 <32211aef-6b87-ab5b-637b-7cf9610f6926@quicinc.com>
 <YyGZ2D9LYYyF31Ug@hovoldconsulting.com>
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <YyGZ2D9LYYyF31Ug@hovoldconsulting.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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,