[RFC,1/2] PM: domains: Allow devices attached to genpd to be managed by HW
Commit Message
From: Ulf Hansson <ulf.hansson@linaro.org>
Some power-domains may be capable of relying on the HW to control the power
for a device that's hooked up to it. Typically, for these kinds of
configurations the device doesn't really need to be attached to a PM domain
(genpd), from Linux point of view. However, in some cases the behaviour of
the power-domain and its device can be changed in runtime.
To allow a consumer driver to change the behaviour of the PM domain for its
device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover,
let's add a corresponding optional genpd callback, ->set_hwmode_dev(),
which the genpd provider should implement if it can support switching
between HW controlled mode and SW controlled mode.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/domain.c | 66 +++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 15 +++++++++
2 files changed, 81 insertions(+)
Comments
On Tue, Jun 27, 2023 at 01:40:32PM +0300, Abel Vesa wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> Some power-domains may be capable of relying on the HW to control the power
> for a device that's hooked up to it. Typically, for these kinds of
> configurations the device doesn't really need to be attached to a PM domain
> (genpd), from Linux point of view. However, in some cases the behaviour of
> the power-domain and its device can be changed in runtime.
>
> To allow a consumer driver to change the behaviour of the PM domain for its
> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover,
> let's add a corresponding optional genpd callback, ->set_hwmode_dev(),
> which the genpd provider should implement if it can support switching
> between HW controlled mode and SW controlled mode.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
You can't forward on a patch from someone else without also adding your
signed-off-by on it, right?
Also, why is this a RFC series? What is left to do with it to get it
into a state which you feel comfortable having us review it "for real"?
thanks,
greg k-h
On 27.06.2023 12:40, 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>
> ---
Currently all GDSCs with flags & HW_CTRL enable hw ctrl mode implicilty.
I didn't get any cover letter with these patches.. are you planning on
retiring that behavior? Presumably after adding a matching pair of set_hwmode
in venus!
fwiw this patch lgtm
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)
On 23-06-27 12:46:28, Greg Kroah-Hartman wrote:
> On Tue, Jun 27, 2023 at 01:40:32PM +0300, Abel Vesa wrote:
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> >
> > Some power-domains may be capable of relying on the HW to control the power
> > for a device that's hooked up to it. Typically, for these kinds of
> > configurations the device doesn't really need to be attached to a PM domain
> > (genpd), from Linux point of view. However, in some cases the behaviour of
> > the power-domain and its device can be changed in runtime.
> >
> > To allow a consumer driver to change the behaviour of the PM domain for its
> > device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover,
> > let's add a corresponding optional genpd callback, ->set_hwmode_dev(),
> > which the genpd provider should implement if it can support switching
> > between HW controlled mode and SW controlled mode.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> You can't forward on a patch from someone else without also adding your
> signed-off-by on it, right?
Oups, forgot to add it. Will do in the next version.
>
> Also, why is this a RFC series? What is left to do with it to get it
> into a state which you feel comfortable having us review it "for real"?
There is a bit of back story here. This HW control support is something
that Qualcomm platforms support for some of the PDs. Sent this as RFC
as I thought it might open up a discussion of such a generic need at
first. But now that I think of it, it might've been a non-RFC patch as
well.
>
> thanks,
>
> greg k-h
On Tue, Jun 27, 2023 at 01:54:06PM +0300, Abel Vesa wrote:
> On 23-06-27 12:46:28, Greg Kroah-Hartman wrote:
> > Also, why is this a RFC series? What is left to do with it to get it
> > into a state which you feel comfortable having us review it "for real"?
>
> There is a bit of back story here. This HW control support is something
> that Qualcomm platforms support for some of the PDs. Sent this as RFC
> as I thought it might open up a discussion of such a generic need at
> first. But now that I think of it, it might've been a non-RFC patch as
> well.
You need to provide that information, otherwise what are we supposed to
do with this patch series? What would you do if you got it in your
inbox?
thanks,
greg k-h
On 23-06-27 12:47:24, Konrad Dybcio wrote:
> On 27.06.2023 12:40, 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>
> > ---
> Currently all GDSCs with flags & HW_CTRL enable hw ctrl mode implicilty.
> I didn't get any cover letter with these patches.. are you planning on
> retiring that behavior? Presumably after adding a matching pair of set_hwmode
> in venus!
I didn't think a cover letter was needed here. After a chat offline with
Taniya about this and it seems there is at least one consumer driver
that needs to switch back and forth the HW control bit. For the rest of
the consumers, the safest way is to assume that they expect their GDSC
to be in HW control mode from the moment it is enabled until it gets
disabled. One example of this is venus.
>
> fwiw this patch lgtm
>
> 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)
On 23-06-27 13:02:50, Greg Kroah-Hartman wrote:
> On Tue, Jun 27, 2023 at 01:54:06PM +0300, Abel Vesa wrote:
> > On 23-06-27 12:46:28, Greg Kroah-Hartman wrote:
> > > Also, why is this a RFC series? What is left to do with it to get it
> > > into a state which you feel comfortable having us review it "for real"?
> >
> > There is a bit of back story here. This HW control support is something
> > that Qualcomm platforms support for some of the PDs. Sent this as RFC
> > as I thought it might open up a discussion of such a generic need at
> > first. But now that I think of it, it might've been a non-RFC patch as
> > well.
>
> You need to provide that information, otherwise what are we supposed to
> do with this patch series? What would you do if you got it in your
> inbox?
Will do in the next version.
Thanks.
>
> thanks,
>
> greg k-h
@@ -541,6 +541,72 @@ void dev_pm_genpd_synced_poweroff(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
+/**
+ * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain.
+ *
+ * @dev: Device for which the HW-mode should be changed.
+ * @enable: Value to set or unset the HW-mode.
+ *
+ * Some PM domains can rely on HW signals to control the power for a device. To
+ * allow a consumer driver to switch the behaviour for its device in runtime,
+ * which may be beneficial from a latency or energy point of view, this function
+ * may be called.
+ *
+ * It is assumed that the users guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ *
+ * Returns 0 on success and negative error values on failures.
+ */
+int dev_pm_genpd_set_hwmode(struct device *dev, bool enable)
+{
+ struct generic_pm_domain *genpd;
+ int ret = 0;
+
+ genpd = dev_to_genpd_safe(dev);
+ if (!genpd)
+ return -ENODEV;
+
+ if (!genpd->set_hwmode_dev)
+ return -EOPNOTSUPP;
+
+ genpd_lock(genpd);
+
+ if (dev_gpd_data(dev)->hw_mode == enable)
+ goto out;
+
+ ret = genpd->set_hwmode_dev(genpd, dev, enable);
+ if (!ret)
+ dev_gpd_data(dev)->hw_mode = enable;
+
+out:
+ genpd_unlock(genpd);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_set_hwmode);
+
+/**
+ * dev_pm_genpd_get_hwmode - Get the HW mode setting for the device.
+ *
+ * @dev: Device for which the current HW-mode setting should be fetched.
+ *
+ * This helper function allows consumer drivers to fetch the current HW mode
+ * setting of its the device.
+ *
+ * It is assumed that the users guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ */
+bool dev_pm_genpd_get_hwmode(struct device *dev)
+{
+ struct generic_pm_domain *genpd;
+
+ genpd = dev_to_genpd_safe(dev);
+ if (!genpd)
+ return false;
+
+ return dev_gpd_data(dev)->hw_mode;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_get_hwmode);
+
static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
{
unsigned int state_idx = genpd->state_idx;
@@ -146,6 +146,8 @@ struct generic_pm_domain {
int (*set_performance_state)(struct generic_pm_domain *genpd,
unsigned int state);
struct gpd_dev_ops dev_ops;
+ int (*set_hwmode_dev)(struct generic_pm_domain *domain,
+ struct device *dev, bool enable);
int (*attach_dev)(struct generic_pm_domain *domain,
struct device *dev);
void (*detach_dev)(struct generic_pm_domain *domain,
@@ -208,6 +210,7 @@ struct generic_pm_domain_data {
unsigned int performance_state;
unsigned int default_pstate;
unsigned int rpm_pstate;
+ bool hw_mode;
void *data;
};
@@ -237,6 +240,8 @@ int dev_pm_genpd_remove_notifier(struct device *dev);
void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
void dev_pm_genpd_synced_poweroff(struct device *dev);
+int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
+bool dev_pm_genpd_get_hwmode(struct device *dev);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
@@ -305,6 +310,16 @@ static inline ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
{ }
+static inline int dev_pm_genpd_set_hwmode(struct device *dev, bool enable)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline bool dev_pm_genpd_get_hwmode(struct device *dev)
+{
+ return false;
+}
+
#define simple_qos_governor (*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
#endif