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: <095d3acb-b04f-3d24-3f00-b8974bdc2648@quicinc.com>
Date:   Thu, 1 Sep 2022 11:16:10 +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 v2 1/2] clk: qcom: gcc-sc7180: Keep the USB GDSC always on
Content-Language: en-US
To:     Stephen Boyd <sboyd@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Matthias Kaehlcke <mka@chromium.org>
CC:     Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Johan Hovold <johan+linaro@kernel.org>,
        <linux-clk@vger.kernel.org>,
        "Krishna Kurapati" <quic_kriskura@quicinc.com>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>
References: <20220825182152.v2.1.I45235b7c40997bc2abf813e4722b4dcdd6aecf6b@changeid>
 <20220826024003.qpqtdmdohdmpcskt@baldur>
 <5ff21b1e-3af9-36ef-e13e-fa33f526d0e3@quicinc.com>
 <20220830213533.7C4FCC433C1@smtp.kernel.org>
From:   Rajendra Nayak <quic_rjendra@quicinc.com>
In-Reply-To: <20220830213533.7C4FCC433C1@smtp.kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 8/31/2022 3:05 AM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2022-08-29 01:12:02)
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index d3244006c661..0fe017ba901b 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -368,6 +368,10 @@ static int _gdsc_disable(struct gdsc *sc)
>>           if (sc->pwrsts & PWRSTS_OFF)
>>                   gdsc_clear_mem_on(sc);
>>
>> +       /* If the GDSC supports RET, do not explicitly power it off */
>> +       if (sc->pwrsts & PWRSTS_RET)
>> +               return 0;
>> +
>>           ret = gdsc_toggle_logic(sc, GDSC_OFF);
>>           if (ret)
>>                   return ret;
>>
>>
>> So with that change, we would then not need the ALWAYS_ON flag set for usb gdsc,
>> instead we would update the .pwrsts to PWRSTS_RET_ON instead of PWRSTS_OFF_ON,
>> and that should make both usb wake-ups to work and we can still have the usb_gdsc as
>> a subdomain of CX for performance state voting.
> 
> To clarify, usb_gdsc is not setup as a subdomain of CX so far, right?
> Just that eventually we'll make usb_gdsc a subdomain of CX so that
> dev_pm_opp_set_rate() can target the CX domain instead of the usb one?

Thats right,

> 
>> Does that sounds like a reasonable solution?
> 
> It sounds good to me. What about the existing users of PWRSTS_RET

hmm, I thought no ones been actually using PWRSTS_RET but looks like
there is *one* user on msm8974, I'll need to dig up how this change would
affect it,

> though? If I understand correctly this flag means the domain will never
> be turned off, instead it will hit retention during low power modes.

right,

> 
> While you're crafting this patch can you also document the PWRSTS_*
> defines so that we know what they mean? I can guess that PWRSTS_RET
> means "retention" but I don't know what it really means. I guess it
> means "Deepest power off state is retention of memory cells".

Sure I will update the PWRSTS_RET description. The definition of 'retention'
here I think remains the same, it means memory is retained (if RETAIN_MEM is set)
or memory *and* some part of logic is retained (if both RETAIN_MEM and RETAIN_PERIPH
are set) in Deepest power off state, however there is a disconnected in terms of
'how to enter the retention state' for a given domain.
There is no SW control to enter the retention state and this happens in HW when the
parent domain/rail transitions to low power. Either the parents lowest power state
is also retention (like is the case with sc7180/sc7280 for usb with cx as parent) or the
parent makes sure it hands the domain over to another parent (CX to MX in some cases)
to support retention.