Messages in this thread | | | Date | Wed, 24 Apr 2024 00:48:50 +0100 | Subject | Re: [PATCH V5 RESEND 3/5] clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode | From | Bryan O'Donoghue <> |
| |
On 13/04/2024 16:20, 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@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/clk/qcom/gdsc.c | 37 +++++++++++++++++++++++++++++++++++++ > drivers/clk/qcom/gdsc.h | 1 + > 2 files changed, 38 insertions(+) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index df9618ab7eea..c5f6be8181d8 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -363,6 +363,39 @@ 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 1usec for mode transition to properly complete */ > + udelay(1);
A delay I suspect you don't need - if the HW spec says "takes 1 usec for this to take effect" that's 1 usec from io write completion from APSS to another system agent.
You poll for the state transition down below anyway.
I'd be pretty certain that's a redundant delay.
> + > + /* > + * When GDSC is switched to HW mode, HW can disable the GDSC. > + * When GDSC is switched back to SW mode, the GDSC will be enabled > + * again, hence need to poll for GDSC to complete the power up. > + */ > + if (!mode) > + return gdsc_poll_status(sc, GDSC_ON); > + > + return 0; > +}
Other than that, seems fine.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
| |