linux-kernel - Re: [PATCH V6 3/5] clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 21 Jun 2024 15:24:21 +0530
From: Jagadeesh Kona <quic_jkona@...cinc.com>
To: Caleb Connolly <caleb.connolly@...aro.org>,
        Bjorn Andersson
	<andersson@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        "Stephen
 Boyd" <sboyd@...nel.org>,
        Stanimir Varbanov <stanimir.k.varbanov@...il.com>,
        Vikash Garodia <quic_vgarodia@...cinc.com>,
        Bryan O'Donoghue
	<bryan.odonoghue@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "Ulf Hansson" <ulf.hansson@...aro.org>,
        "Rafael J . Wysocki"
	<rafael@...nel.org>,
        Kevin Hilman <khilman@...nel.org>, Pavel Machek
	<pavel@....cz>,
        Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Andy
 Gross <agross@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Abel Vesa <abel.vesa@...aro.org>
CC: <linux-pm@...r.kernel.org>, <linux-media@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Taniya Das <quic_tdas@...cinc.com>,
        "Satya
 Priya Kakitapalli" <quic_skakitap@...cinc.com>,
        Imran Shaik
	<quic_imrashai@...cinc.com>,
        Ajit Pandey <quic_ajipan@...cinc.com>
Subject: Re: [PATCH V6 3/5] clk: qcom: gdsc: Add set and get hwmode callbacks
 to switch GDSC mode



On 6/20/2024 3:40 AM, Caleb Connolly wrote:
> Hi Jagadeesh,
> 
> Sorry, some grammar nitpicks.
> 
> On 19/06/2024 16:14, Jagadeesh Kona wrote:
>> Some GDSC client drivers require the GDSC mode to be switched dynamically
>> to HW mode at runtime to gain the power benefits. Typically such client
>> drivers require the GDSC to be brought up in SW mode initially to enable
>> the required dependent clocks and configure the hardware to proper state.
>> Once initial hardware set up is done, they switch the GDSC to HW mode to
>> save power. At the end of usecase, they switch the GDSC back to SW mode
>> and disable the GDSC.
>>
>> Introduce HW_CTRL_TRIGGER flag to register the set_hwmode_dev and
>> get_hwmode_dev callbacks for GDSC's whose respective client drivers
>> require the GDSC mode to be switched dynamically at runtime using
>> dev_pm_genpd_set_hwmode() API.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@...cinc.com>
>> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>> ---
>>   drivers/clk/qcom/gdsc.c | 42 +++++++++++++++++++++++++++++++++++++++++
>>   drivers/clk/qcom/gdsc.h |  1 +
>>   2 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index df9618ab7eea..6acc7af82255 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -363,6 +363,44 @@ static int gdsc_disable(struct generic_pm_domain 
>> *domain)
>>       return 0;
>>   }
>> +static int gdsc_set_hwmode(struct generic_pm_domain *domain, struct 
>> device *dev, bool mode)
>> +{
>> +    struct gdsc *sc = domain_to_gdsc(domain);
>> +    int ret;
>> +
>> +    ret = gdsc_hwctrl(sc, mode);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /*
>> +     * Wait for the GDSC to go through a power down and
>> +     * up cycle. In case SW/FW end up polling status
>> +     * bits for the gdsc before the power cycle is completed
>> +     * it might read the status wrongly.
> 
> If we poll the status register before the power cycle is finished we 
> might read incorrect values.

Thanks Caleb for your review. Sure, will take care of these comments in
next series.

Thanks,
Jagadeesh

>> +     */
>> +    udelay(1);
>> +
>> +    /*
>> +     * When GDSC is switched to HW mode, HW can disable the GDSC.
> The GDSC
>> +     * When GDSC is switched back to SW mode, the GDSC will be enabled
> The GDSC
>> +     * again, hence need to poll for GDSC to complete the power 
>> uphence we need to poll
> 
> Kind regards,
>> +     */
>> +    if (!mode)
>> +        return gdsc_poll_status(sc, GDSC_ON);
>> +
>> +    return 0;
>> +}
>> +
>> +static bool gdsc_get_hwmode(struct generic_pm_domain *domain, struct 
>> device *dev)
>> +{
>> +    struct gdsc *sc = domain_to_gdsc(domain);
>> +    u32 val;
>> +
>> +    regmap_read(sc->regmap, sc->gdscr, &val);
>> +
>> +    return !!(val & HW_CONTROL_MASK);
>> +}
>> +
>>   static int gdsc_init(struct gdsc *sc)
>>   {
>>       u32 mask, val;
>> @@ -451,6 +489,10 @@ static int gdsc_init(struct gdsc *sc)
>>           sc->pd.power_off = gdsc_disable;
>>       if (!sc->pd.power_on)
>>           sc->pd.power_on = gdsc_enable;
>> +    if (sc->flags & HW_CTRL_TRIGGER) {
>> +        sc->pd.set_hwmode_dev = gdsc_set_hwmode;
>> +        sc->pd.get_hwmode_dev = gdsc_get_hwmode;
>> +    }
>>       ret = pm_genpd_init(&sc->pd, NULL, !on);
>>       if (ret)
>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>> index 803512688336..1e2779b823d1 100644
>> --- a/drivers/clk/qcom/gdsc.h
>> +++ b/drivers/clk/qcom/gdsc.h
>> @@ -67,6 +67,7 @@ struct gdsc {
>>   #define ALWAYS_ON    BIT(6)
>>   #define RETAIN_FF_ENABLE    BIT(7)
>>   #define NO_RET_PERIPH    BIT(8)
>> +#define HW_CTRL_TRIGGER    BIT(9)
>>       struct reset_controller_dev    *rcdev;
>>       unsigned int            *resets;
>>       unsigned int            reset_count;
>