| Message ID | 20230628105652.1670316-3-abel.vesa@linaro.org (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | PM: domains: Add control for switching back and forth to HW control | expand |
On 28.06.2023 12:56, Abel Vesa wrote: > Implement the GDSC specific genpd set_hwmode_dev callback in order to > switch the HW control on or off. For any GDSC that supports HW control > set this callback in order to allow its consumers to control it. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- This still does nothing to prevent the HW_CTRL state being changed in init, enable and disable functions. Konrad > drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index 5358e28122ab..9a04bf2e4379 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -314,6 +314,26 @@ static int gdsc_enable(struct generic_pm_domain *domain) > return 0; > } > > +static int gdsc_set_hwmode_dev(struct generic_pm_domain *domain, > + struct device *dev, bool enable) > +{ > + int ret = gdsc_hwctrl(domain_to_gdsc(domain), enable); > + > + if (ret) > + goto out; > + > + /* > + * Wait for the GDSC to go through a power down and > + * up cycle. In case there is a status polling going on > + * before the power cycle is completed it might read an > + * wrong status value. > + */ > + udelay(1); > + > +out: > + return ret; > +} > + > static int gdsc_disable(struct generic_pm_domain *domain) > { > struct gdsc *sc = domain_to_gdsc(domain); > @@ -451,6 +471,8 @@ 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) > + sc->pd.set_hwmode_dev = gdsc_set_hwmode_dev; > > ret = pm_genpd_init(&sc->pd, NULL, !on); > if (ret)
Hi Abel, Thanks for the patch. On 6/28/2023 10:48 PM, Konrad Dybcio wrote: > On 28.06.2023 12:56, Abel Vesa wrote: >> Implement the GDSC specific genpd set_hwmode_dev callback in order to >> switch the HW control on or off. For any GDSC that supports HW control >> set this callback in order to allow its consumers to control it. >> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >> --- > This still does nothing to prevent the HW_CTRL state being changed in > init, enable and disable functions. > > Konrad >> drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >> index 5358e28122ab..9a04bf2e4379 100644 >> --- a/drivers/clk/qcom/gdsc.c >> +++ b/drivers/clk/qcom/gdsc.c >> @@ -314,6 +314,26 @@ static int gdsc_enable(struct generic_pm_domain *domain) >> return 0; >> } >> >> +static int gdsc_set_hwmode_dev(struct generic_pm_domain *domain, >> + struct device *dev, bool enable) >> +{ >> + int ret = gdsc_hwctrl(domain_to_gdsc(domain), enable); >> + >> + if (ret) >> + goto out; >> + >> + /* >> + * Wait for the GDSC to go through a power down and >> + * up cycle. In case there is a status polling going on >> + * before the power cycle is completed it might read an >> + * wrong status value. >> + */ >> + udelay(1); >> + >> +out: >> + return ret; >> +} >> + >> static int gdsc_disable(struct generic_pm_domain *domain) >> { >> struct gdsc *sc = domain_to_gdsc(domain); >> @@ -451,6 +471,8 @@ 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) >> + sc->pd.set_hwmode_dev = gdsc_set_hwmode_dev; >> We do not want to move to SW mode without consumers wanting to move to this mode. We want a new flag for the consumers wanting to move to this mode. The mode in which the GDSC would be enabled would be in SW mode only. + if (sc->flags & HW_CTRL_TRIGGER) { + sc->pd.set_hwmode_dev = gdsc_set_mode; + } + >> ret = pm_genpd_init(&sc->pd, NULL, !on); >> if (ret)
On 6/28/2023 4:26 PM, Abel Vesa wrote: > Implement the GDSC specific genpd set_hwmode_dev callback in order to > switch the HW control on or off. For any GDSC that supports HW control > set this callback in order to allow its consumers to control it. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index 5358e28122ab..9a04bf2e4379 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -314,6 +314,26 @@ static int gdsc_enable(struct generic_pm_domain *domain) > return 0; > } > > +static int gdsc_set_hwmode_dev(struct generic_pm_domain *domain, > + struct device *dev, bool enable) > +{ > + int ret = gdsc_hwctrl(domain_to_gdsc(domain), enable); > + > + if (ret) > + goto out; > + > + /* > + * Wait for the GDSC to go through a power down and > + * up cycle. In case there is a status polling going on > + * before the power cycle is completed it might read an > + * wrong status value. > + */ > + udelay(1); > + > +out: > + return ret; > +} > + > static int gdsc_disable(struct generic_pm_domain *domain) > { > struct gdsc *sc = domain_to_gdsc(domain); > @@ -451,6 +471,8 @@ 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) > + sc->pd.set_hwmode_dev = gdsc_set_hwmode_dev; > Forgot to add the get_mode. + if (sc->flags & HW_CTRL_TRIGGER) { + sc->pd.set_hwmode_dev = gdsc_set_mode; + sc->pd.get_hwmode_dev = gdsc_get_mode; + } + > ret = pm_genpd_init(&sc->pd, NULL, !on); > if (ret)
On 23-07-10 09:40:14, Taniya Das wrote: > Hi Abel, > > Thanks for the patch. > > On 6/28/2023 10:48 PM, Konrad Dybcio wrote: > > On 28.06.2023 12:56, Abel Vesa wrote: > > > Implement the GDSC specific genpd set_hwmode_dev callback in order to > > > switch the HW control on or off. For any GDSC that supports HW control > > > set this callback in order to allow its consumers to control it. > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > --- > > This still does nothing to prevent the HW_CTRL state being changed in > > init, enable and disable functions. > > > > Konrad > > > drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++++ > > > 1 file changed, 22 insertions(+) > > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > > > index 5358e28122ab..9a04bf2e4379 100644 > > > --- a/drivers/clk/qcom/gdsc.c > > > +++ b/drivers/clk/qcom/gdsc.c > > > @@ -314,6 +314,26 @@ static int gdsc_enable(struct generic_pm_domain *domain) > > > return 0; > > > } > > > +static int gdsc_set_hwmode_dev(struct generic_pm_domain *domain, > > > + struct device *dev, bool enable) > > > +{ > > > + int ret = gdsc_hwctrl(domain_to_gdsc(domain), enable); > > > + > > > + if (ret) > > > + goto out; > > > + > > > + /* > > > + * Wait for the GDSC to go through a power down and > > > + * up cycle. In case there is a status polling going on > > > + * before the power cycle is completed it might read an > > > + * wrong status value. > > > + */ > > > + udelay(1); > > > + > > > +out: > > > + return ret; > > > +} > > > + > > > static int gdsc_disable(struct generic_pm_domain *domain) > > > { > > > struct gdsc *sc = domain_to_gdsc(domain); > > > @@ -451,6 +471,8 @@ 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) > > > + sc->pd.set_hwmode_dev = gdsc_set_hwmode_dev; > We do not want to move to SW mode without consumers wanting to move to this > mode. > > We want a new flag for the consumers wanting to move to this mode. The mode > in which the GDSC would be enabled would be in SW mode only. > + if (sc->flags & HW_CTRL_TRIGGER) { > + sc->pd.set_hwmode_dev = gdsc_set_mode; > + } > + OK, maybe I'm missing something here. Do you suggest we have GDSCs that, even though they support HW ctrl, should not be controllable by the consumer? Why isn't dev_pm_genpd_set_hwmode good enough? If a consumer doesn't want to control it then the consumer can just skip calling the mentioned function. Or maybe you want this all hidden into the genpd provider? > > > > ret = pm_genpd_init(&sc->pd, NULL, !on); > > > if (ret) > > -- > Thanks & Regards, > Taniya Das.
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index 5358e28122ab..9a04bf2e4379 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -314,6 +314,26 @@ static int gdsc_enable(struct generic_pm_domain *domain) return 0; } +static int gdsc_set_hwmode_dev(struct generic_pm_domain *domain, + struct device *dev, bool enable) +{ + int ret = gdsc_hwctrl(domain_to_gdsc(domain), enable); + + if (ret) + goto out; + + /* + * Wait for the GDSC to go through a power down and + * up cycle. In case there is a status polling going on + * before the power cycle is completed it might read an + * wrong status value. + */ + udelay(1); + +out: + return ret; +} + static int gdsc_disable(struct generic_pm_domain *domain) { struct gdsc *sc = domain_to_gdsc(domain); @@ -451,6 +471,8 @@ 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) + sc->pd.set_hwmode_dev = gdsc_set_hwmode_dev; ret = pm_genpd_init(&sc->pd, NULL, !on); if (ret)
Implement the GDSC specific genpd set_hwmode_dev callback in order to switch the HW control on or off. For any GDSC that supports HW control set this callback in order to allow its consumers to control it. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)