linux-kernel - Re: [PATCH 2/9] clk: qcom: gdsc: Add configurable poll timeout

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y3XrQrnT0dxTvc5S@linaro.org>
Date:   Thu, 17 Nov 2022 10:05:22 +0200
From:   Abel Vesa <abel.vesa@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Mike Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-clk@...r.kernel.org
Subject: Re: [PATCH 2/9] clk: qcom: gdsc: Add configurable poll timeout

On 22-11-16 12:19:09, Konrad Dybcio wrote:
> 
> 
> On 16/11/2022 11:47, Abel Vesa wrote:
> > Depending on the platform, the poll timeout delay might be different,
> > so allow the platform specific drivers to specify their own values.
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> > ---
> >   drivers/clk/qcom/gdsc.c | 5 ++++-
> >   drivers/clk/qcom/gdsc.h | 1 +
> >   2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 0f21a8a767ac..3753f3ef7241 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -107,7 +107,7 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
> >   	do {
> >   		if (gdsc_check_status(sc, status))
> >   			return 0;
> > -	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> > +	} while (ktime_us_delta(ktime_get(), start) < sc->poll_timeout);
> What about the second usage of TIMEOUT_US (in gdsc_toggle_logic)? Is it fine
> for that to be the default value?

The usleep you mention is not really for polling the state.
So I think it should stay as is. Who knows, maybe in the future we will
need to have the configurable as well, but as a toggle delay rather than
a status poll timeout.

I added this configurable poll timeout just because I saw that
downstream, each driver has different values. And it kind of makes sense,
because the state machine inside the GDSC might be different between
platforms, and so, it might take different time to reach a certain on/off
state.

Thanks,
Abel

> 
> 
> Konrad
> >   	if (gdsc_check_status(sc, status))
> >   		return 0;
> > @@ -454,6 +454,9 @@ static int gdsc_init(struct gdsc *sc)
> >   	if (ret)
> >   		goto err_disable_supply;
> > +	if (!sc->poll_timeout)
> > +		sc->poll_timeout = 500;
> > +
> >   	return 0;
> >   err_disable_supply:
> > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> > index 803512688336..9a1e1fb3d12f 100644
> > --- a/drivers/clk/qcom/gdsc.h
> > +++ b/drivers/clk/qcom/gdsc.h
> > @@ -36,6 +36,7 @@ struct gdsc {
> >   	struct generic_pm_domain	*parent;
> >   	struct regmap			*regmap;
> >   	unsigned int			gdscr;
> > +	unsigned int			poll_timeout;
> >   	unsigned int			collapse_ctrl;
> >   	unsigned int			collapse_mask;
> >   	unsigned int			gds_hw_ctrl;