| Message ID | 1477304297-5248-2-git-send-email-sricharan@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
Hi Sricharan, On 10/24/2016 01:18 PM, Sricharan R wrote: > From: Rajendra Nayak <rnayak@codeaurora.org> > > Some GDSCs might support a HW control mode, where in the power > domain (gdsc) is brought in and out of low power state (while > unsued) without any SW assistance, saving power. > Such GDSCs can be configured in a HW control mode when powered on > until they are explicitly requested to be powered off by software. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > --- > drivers/clk/qcom/gdsc.c | 15 +++++++++++++++ > drivers/clk/qcom/gdsc.h | 1 + > 2 files changed, 16 insertions(+) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index f12d7b2..a5e1c8c 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg) > return !!(val & PWR_ON_MASK); > } > > +static int gdsc_hwctrl(struct gdsc *sc, bool en) > +{ > + u32 val = en ? HW_CONTROL_MASK : 0; > + > + return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val); > +} > + > static int gdsc_toggle_logic(struct gdsc *sc, bool en) > { > int ret; > @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain) > */ > udelay(1); > > + /* Turn on HW trigger mode if supported */ > + if (sc->flags & HW_CTRL) > + gdsc_hwctrl(sc, true); Could you check gdsc_hwctrl() for an error. <cut>
Hi Stan, >Hi Sricharan, > >On 10/24/2016 01:18 PM, Sricharan R wrote: >> From: Rajendra Nayak <rnayak@codeaurora.org> >> >> Some GDSCs might support a HW control mode, where in the power >> domain (gdsc) is brought in and out of low power state (while >> unsued) without any SW assistance, saving power. >> Such GDSCs can be configured in a HW control mode when powered on >> until they are explicitly requested to be powered off by software. >> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> --- >> drivers/clk/qcom/gdsc.c | 15 +++++++++++++++ >> drivers/clk/qcom/gdsc.h | 1 + >> 2 files changed, 16 insertions(+) >> >> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >> index f12d7b2..a5e1c8c 100644 >> --- a/drivers/clk/qcom/gdsc.c >> +++ b/drivers/clk/qcom/gdsc.c >> @@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg) >> return !!(val & PWR_ON_MASK); >> } >> >> +static int gdsc_hwctrl(struct gdsc *sc, bool en) >> +{ >> + u32 val = en ? HW_CONTROL_MASK : 0; >> + >> + return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val); >> +} >> + >> static int gdsc_toggle_logic(struct gdsc *sc, bool en) >> { >> int ret; >> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain) >> */ >> udelay(1); >> >> + /* Turn on HW trigger mode if supported */ >> + if (sc->flags & HW_CTRL) >> + gdsc_hwctrl(sc, true); > Sure, will add the check. Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/24, Sricharan R wrote: > @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain) > */ > udelay(1); > > + /* Turn on HW trigger mode if supported */ > + if (sc->flags & HW_CTRL) > + gdsc_hwctrl(sc, true); > + It sounds like this will cause glitches if the hardware isn't asserting their hw control bit by default? This has me concerned that we can't just throw the hw control enable part into here, because that bit doesn't live in the clock controller, instead it lives in the hw block that is powered by the power domain? Or does the power on reset value of that hw control signal asserted? If that's true then we should be ok to force it into hw control mode by default.
Hi Stephen, >On 10/24, Sricharan R wrote: >> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain) >> */ >> udelay(1); >> >> + /* Turn on HW trigger mode if supported */ >> + if (sc->flags & HW_CTRL) >> + gdsc_hwctrl(sc, true); >> + > >It sounds like this will cause glitches if the hardware isn't >asserting their hw control bit by default? This has me concerned >that we can't just throw the hw control enable part into here, >because that bit doesn't live in the clock controller, instead it >lives in the hw block that is powered by the power domain? > >Or does the power on reset value of that hw control signal >asserted? If that's true then we should be ok to force it into hw >control mode by default. > The hw control bit is set by default. Instead its turned 'off' with the reset value. So it has to not be turned 'on' at some point to put the gdsc in hw control if required. This bit is part of the gdscr register. So i did not quite understand the reason for the glitch here ? Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, >-----Original Message----- >From: linux-arm-msm-owner@vger.kernel.org [mailto:linux-arm-msm-owner@vger.kernel.org] On Behalf Of Sricharan >Sent: Wednesday, November 02, 2016 12:21 PM >To: 'Stephen Boyd' <sboyd@codeaurora.org> >Cc: mturquette@baylibre.com; linux-clk@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; >rnayak@codeaurora.org; stanimir.varbanov@linaro.org >Subject: RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control > >Hi Stephen, > >>On 10/24, Sricharan R wrote: >>> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain) >>> */ >>> udelay(1); >>> >>> + /* Turn on HW trigger mode if supported */ >>> + if (sc->flags & HW_CTRL) >>> + gdsc_hwctrl(sc, true); >>> + >> >>It sounds like this will cause glitches if the hardware isn't >>asserting their hw control bit by default? This has me concerned >>that we can't just throw the hw control enable part into here, >>because that bit doesn't live in the clock controller, instead it >>lives in the hw block that is powered by the power domain? >> >>Or does the power on reset value of that hw control signal >>asserted? If that's true then we should be ok to force it into hw >>control mode by default. >> > >The hw control bit is set by default. Instead its turned 'off' >with the reset value. So it has to not >be turned 'on' at some point >to put the gdsc in hw control if required. This bit is part of the >gdscr register. So i did not quite understand the reason for the >glitch here ? > typo above, i meant it has to be turned 'on' at some point if required. Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/02, Sricharan wrote: > Hi Stephen, > > >On 10/24, Sricharan R wrote: > >> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain) > >> */ > >> udelay(1); > >> > >> + /* Turn on HW trigger mode if supported */ > >> + if (sc->flags & HW_CTRL) > >> + gdsc_hwctrl(sc, true); > >> + > > > >It sounds like this will cause glitches if the hardware isn't > >asserting their hw control bit by default? This has me concerned > >that we can't just throw the hw control enable part into here, > >because that bit doesn't live in the clock controller, instead it > >lives in the hw block that is powered by the power domain? > > > >Or does the power on reset value of that hw control signal > >asserted? If that's true then we should be ok to force it into hw > >control mode by default. > > > > The hw control bit is set by default. Instead its turned 'off' > with the reset value. So it has to not > be turned 'on' at some point > to put the gdsc in hw control if required. This bit is part of the > gdscr register. So i did not quite understand the reason for the > glitch here ? I mean the reset value of the hw control signal inside the device that is inside the GDSC power domain. For example, the hw control bit inside the video core.
Hi Stephen, >> >On 10/24, Sricharan R wrote: >> >> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain) >> >> */ >> >> udelay(1); >> >> >> >> + /* Turn on HW trigger mode if supported */ >> >> + if (sc->flags & HW_CTRL) >> >> + gdsc_hwctrl(sc, true); >> >> + >> > >> >It sounds like this will cause glitches if the hardware isn't >> >asserting their hw control bit by default? This has me concerned >> >that we can't just throw the hw control enable part into here, >> >because that bit doesn't live in the clock controller, instead it >> >lives in the hw block that is powered by the power domain? >> > >> >Or does the power on reset value of that hw control signal >> >asserted? If that's true then we should be ok to force it into hw >> >control mode by default. >> > >> >> The hw control bit is set by default. Instead its turned 'off' >> with the reset value. So it has to not >> be turned 'on' at some point >> to put the gdsc in hw control if required. This bit is part of the >> gdscr register. So i did not quite understand the reason for the >> glitch here ? > >I mean the reset value of the hw control signal inside the device >that is inside the GDSC power domain. For example, the hw control >bit inside the video core. > Ok, so the video ip core, has a hw control signal/bit. I checked this by dumping this out that, the moment the gdsc is put to hw control, the video ip's hw control bit also gets asserted/set. so this means that video ip's bit get aligned with the gdsc setting. so this should avoid the glitches, right ? Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/03, Sricharan wrote: > Ok, so the video ip core, has a hw control signal/bit. > I checked this by dumping this out that, the moment the > gdsc is put to hw control, the video ip's hw control bit also > gets asserted/set. so this means that video ip's bit get > aligned with the gdsc setting. so this should avoid the > glitches, right ? > Yes that matches my understanding. Thanks for confirming.
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index f12d7b2..a5e1c8c 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg) return !!(val & PWR_ON_MASK); } +static int gdsc_hwctrl(struct gdsc *sc, bool en) +{ + u32 val = en ? HW_CONTROL_MASK : 0; + + return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val); +} + static int gdsc_toggle_logic(struct gdsc *sc, bool en) { int ret; @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain) */ udelay(1); + /* Turn on HW trigger mode if supported */ + if (sc->flags & HW_CTRL) + gdsc_hwctrl(sc, true); + return 0; } @@ -174,6 +185,10 @@ static int gdsc_disable(struct generic_pm_domain *domain) if (sc->pwrsts == PWRSTS_ON) return gdsc_assert_reset(sc); + /* Turn off HW trigger mode if supported */ + if (sc->flags & HW_CTRL) + gdsc_hwctrl(sc, false); + if (sc->pwrsts & PWRSTS_OFF) gdsc_clear_mem_on(sc); diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h index 3bf497c..b1f30f8 100644 --- a/drivers/clk/qcom/gdsc.h +++ b/drivers/clk/qcom/gdsc.h @@ -50,6 +50,7 @@ struct gdsc { #define PWRSTS_RET_ON (PWRSTS_RET | PWRSTS_ON) const u8 flags; #define VOTABLE BIT(0) +#define HW_CTRL BIT(1) struct reset_controller_dev *rcdev; unsigned int *resets; unsigned int reset_count;