| 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
| ||
|
Message-ID: <35907177-eecd-4b58-a5da-7186b0f60193@quicinc.com>
Date: Tue, 13 Feb 2024 18:34:55 +0530
From: Jagadeesh Kona <quic_jkona@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>, Abel Vesa <abel.vesa@...aro.org>
CC: "Rafael J. Wysocki" <rafael@...nel.org>,
Kevin Hilman
<khilman@...nel.org>,
Ulf Hansson <ulf.hansson@...aro.org>, Pavel Machek
<pavel@....cz>,
Len Brown <len.brown@...el.com>,
Greg Kroah-Hartman
<gregkh@...uxfoundation.org>,
Andy Gross <agross@...nel.org>,
Konrad Dybcio
<konrad.dybcio@...aro.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>,
Taniya Das <quic_tdas@...cinc.com>,
Dmitry Baryshkov
<dmitry.baryshkov@...aro.org>,
<linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
<linux-media@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] clk: qcom: gdsc: Add set and get hwmode callbacks
to switch GDSC mode
On 1/31/2024 4:30 AM, Bjorn Andersson wrote:
> On Mon, Jan 22, 2024 at 10:47:03AM +0200, Abel Vesa wrote:
>> From: Jagadeesh Kona <quic_jkona@...cinc.com>
>>
>> Add support for set and get hwmode callbacks to switch the GDSC between
>> SW and HW modes. Currently, the GDSC is moved to HW control mode
>> using HW_CTRL flag and if this flag is present, GDSC is moved to HW
>> mode as part of GDSC enable itself. The intention is to keep the
>> HW_CTRL flag functionality as is, since many older chipsets still use
>> this flag.
>>
>
> This provides insight into why we end up with both HW_CTRL and
> HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but
> rather just an implementation detail.
>
>> But consumer drivers also require the GDSC mode to be switched dynamically
>> at runtime based on requirement for certain usecases. Some of these
>> usecases are switching the GDSC to SW mode to keep it ON during the
>> enablement of clocks that are dependent on GDSC and while programming
>> certain configurations that require GDSC to be ON. Introduce a new
>> HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev
>> callbacks which allows the consumer drivers to switch the GDSC back and
>> forth between HW/SW modes dynamically at runtime using new
>> dev_pm_genpd_set_hwmode API.
>>
>
> This still expresses the need for HW_CTRL_TRIGGER in terms of "some
> drivers need for some use case". We don't need these many words to say:
> "Introduce HW_CTRL_TRIGGER for client drivers that need it."
>
Thanks Bjorn for your review.
Sure will update the commit text to be more precise in next series.
>
> I find that it would be useful to document that every time a GDSC is
> turned on the mode will be switched to SW...
>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@...cinc.com>
>> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
>> ---
>> drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/clk/qcom/gdsc.h | 1 +
>> 2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index 5358e28122ab..71626eb20101 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -363,6 +363,56 @@ 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);
>> + u32 val;
>> + int ret;
>> +
>> + if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) {
>
> Why is this a restriction only for GDSCs supplied by regulators? I don't
> find anything preventing this API from being called on GDSCs supplied by
> other genpd instances.
>
> Also note that regulator_is_enabled() is racy, in that it tells us if
> the regulator is currently turned on, not if we're the one holding that
> vote. As such this might change at any moment - and hence shouldn't be
> significant here.
>
Below is the consumer's sequence that switch the GDSC's b/w HW & SW modes:-
1) Enable the GDSC in SW mode
2) Enable required clocks
3) Switch the GDSC to HW mode using dev_pm_genpd_set_hwmode(true)
4) Usecase start
5) Usecase end
6) Switch the GDSC back to SW mode using dev_pm_genpd_set_hwmode(false)
7) Disable clocks
8) Disable GDSC
Hence the new API dev_pm_genpd_set_hwmode() will always be called
between gdsc_enable() and gdsc_disable(), which ensures GDSC's parent
power domain/regulator is ON when this callback is being called. Also,
we can remove the above regulator_is_enabled() check as well.
>> + pr_err("Cannot set mode while parent is disabled\n");
>> + return -EIO;
>> + }
>> +
>> + ret = gdsc_hwctrl(sc, mode);
>> + if (ret)
>> + return ret;
>> +
>> + /* Wait for 1usec for mode transition to properly complete */
>> + udelay(1);
>> +
>> + if (!mode) {
>> + ret = regmap_read(sc->regmap, sc->gdscr, &val);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * While switching from HW to SW mode, if GDSC is in enabled
>> + * state, poll for GDSC to complete the power up.
>> + */
>
> I had to give this some thought, to conclude that this is relevant if HW
> has the GDSC disabled and we're switching to SW - which would then
> enable it. I think this comment can be improved slightly, to save the
> reader the need for figuring out this on their own.
>
Sure, I will improvise the comment in next series.
>> + if (!(val & SW_COLLAPSE_MASK))
>
> This not being true, would imply that gdsc_disable() has been called
> already, in which case there's no guarantee that the parent still
> supplies power.
>
> In the introduced API power on and hw control are orthogonal states, but
> not so in this implementation. This need to made clear, to reduce future
> surprises.
>
Yes, above SW_COLLAPSE_MASK check is also not required and will remove
it in next series.
>> + 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;
>> + int ret;
>> +
>> + ret = regmap_read(sc->regmap, sc->gdscr, &val);
>> + if (ret)
>> + return ret;
>> +
>> + if (val & HW_CONTROL_MASK)
>> + return true;
>> +
>> + return false;
>
> return !!(val & HW_CONTROL_MASK);
>
Sure, will update this in the next series.
> Regards,
> Bjorn
>
>> +}
>> +
>> static int gdsc_init(struct gdsc *sc)
>> {
>> u32 mask, val;
>> @@ -451,6 +501,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;
>>
>> --
>> 2.34.1
>>
>
Powered by blists - more mailing lists