Re: [PATCH RESEND v3 1/5] PM: domains: Allow devices attached to genpd to be managed by HW

From: Bjorn Andersson <andersson@kernel.org>
To: Abel Vesa <abel.vesa@linaro.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Kevin Hilman <khilman@kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>, Pavel Machek <pavel@ucw.cz>,
	Len Brown <len.brown@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andy Gross <agross@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Stanimir Varbanov <stanimir.k.varbanov@gmail.com>,
	Vikash Garodia <quic_vgarodia@quicinc.com>,
	Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Taniya Das <tdas@qti.qualcomm.com>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH RESEND v3 1/5] PM: domains: Allow devices attached to genpd to be managed by HW
Date: Fri, 3 Nov 2023 12:46:33 -0700	[thread overview]
Message-ID: <f2tufov2amxapubo4r3wqnzuf3qgm2bt2hc6g5zszj4r5w47xf@vmw3on4axns4> (raw)
In-Reply-To: <20231101-gdsc-hwctrl-v3-1-0740ae6b2b04@linaro.org>

On Wed, Nov 01, 2023 at 11:04:07AM +0200, 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.

Per patch 5, this description isn't correct.

Linux enables the power-domain, which in turn enables parent domains
etc. Once the firmware is booted the hardware-control mechanism is
engaged, but until then, and in order to keep the supplies to the domain
voted for, Linux needs a proper genpd.

Regards,
Bjorn

> 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. Similarly, add the
> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and
> its corresponding optional genpd callback, ->get_hwmode_dev(), which the
> genpd provider can also implement for reading back the mode from the
> hardware.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/base/power/domain.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   | 17 +++++++++++
>  2 files changed, 86 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index da1777e39eaa..3fb1a234c7f2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -550,6 +550,75 @@ 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;
> +
> +	if (genpd->get_hwmode_dev)
> +		return genpd->get_hwmode_dev(genpd, dev);
> +
> +	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;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 34663d0d5c55..0e7d177fdc3e 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -151,6 +151,10 @@ 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);
> +	bool (*get_hwmode_dev)(struct generic_pm_domain *domain,
> +			      struct device *dev);
>  	int (*attach_dev)(struct generic_pm_domain *domain,
>  			  struct device *dev);
>  	void (*detach_dev)(struct generic_pm_domain *domain,
> @@ -213,6 +217,7 @@ struct generic_pm_domain_data {
>  	unsigned int performance_state;
>  	unsigned int default_pstate;
>  	unsigned int rpm_pstate;
> +	bool hw_mode;
>  	void *data;
>  };
>  
> @@ -242,6 +247,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;
> @@ -310,6 +317,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
> 
> -- 
> 2.34.1
> 