| Message ID | 20240619141413.7983-4-quic_jkona@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add control for switching back and forth to HW control | expand |
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@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.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. > + */ > + 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;
On 6/19/2024 7:44 PM, 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> > Reviewed-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org> > --- > drivers/clk/qcom/gdsc.c | 42 +++++++++++++++++++++++++++++++++++++++++ > drivers/clk/qcom/gdsc.h | 1 + > 2 files changed, 43 insertions(+) Reviewed-by: Taniya Das <quic_tdas@quicinc.com>
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@quicinc.com> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.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; >
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. + */ + udelay(1); + + /* + * 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; +} + +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;