[PATCH RESEND v3 0/5] PM: domains: Add control for switching back and forth to HW control

[PATCH RESEND v3 0/5] PM: domains: Add control for switching back and forth to HW control

[Abel Vesa]

This is just a resend of v3:
https://lore.kernel.org/lkml/20230923115008.1698384-1-abel.vesa@linaro.org/

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Abel Vesa (1):
      PM: domains: Add the domain HW-managed mode to the summary

Jagadeesh Kona (3):
      clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode
      clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode
      venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode

Ulf Hansson (1):
      PM: domains: Allow devices attached to genpd to be managed by HW

 drivers/base/power/domain.c                    | 84 +++++++++++++++++++++++++-
 drivers/clk/qcom/gdsc.c                        | 32 ++++++++++
 drivers/clk/qcom/gdsc.h                        |  1 +
 drivers/clk/qcom/videocc-sc7180.c              |  2 +-
 drivers/clk/qcom/videocc-sc7280.c              |  2 +-
 drivers/clk/qcom/videocc-sdm845.c              |  4 +-
 drivers/clk/qcom/videocc-sm8250.c              |  4 +-
 drivers/clk/qcom/videocc-sm8550.c              |  4 +-
 drivers/media/platform/qcom/venus/core.c       |  4 ++
 drivers/media/platform/qcom/venus/core.h       |  1 +
 drivers/media/platform/qcom/venus/pm_helpers.c | 47 ++++++--------
 include/linux/pm_domain.h                      | 17 ++++++
 12 files changed, 165 insertions(+), 37 deletions(-)
---
base-commit: fa7d0c17278644aa691f96d08bef8001b762f9cd
change-id: 20231101-gdsc-hwctrl-13f01ea60cbd

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>

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

[Abel Vesa]

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. 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

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

[Dmitry Baryshkov]

On Wed, 1 Nov 2023 at 11:08, Abel Vesa <abel.vesa@linaro.org> 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. 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>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

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

[Bjorn Andersson]

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
> 

[PATCH RESEND v3 2/5] PM: domains: Add the domain HW-managed mode to the summary

[Abel Vesa]

Now that genpd supports dynamically switching the control for an
attached device between hardware- and software-mode,  let's add this
information to the genpd summary in debugfs.

Suggested-by: Taniya Das <quic_tdas@quicinc.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 3fb1a234c7f2..7044271ec93b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -3181,6 +3181,15 @@ static void rtpm_status_str(struct seq_file *s, struct device *dev)
 	seq_printf(s, "%-25s  ", p);
 }
 
+static void mode_status_str(struct seq_file *s, struct device *dev)
+{
+	struct generic_pm_domain_data *gpd_data;
+
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+
+	seq_printf(s, "%20s", gpd_data->hw_mode ? "HW" : "SW");
+}
+
 static void perf_status_str(struct seq_file *s, struct device *dev)
 {
 	struct generic_pm_domain_data *gpd_data;
@@ -3239,6 +3248,7 @@ static int genpd_summary_one(struct seq_file *s,
 		seq_printf(s, "\n    %-50s  ", kobj_path);
 		rtpm_status_str(s, pm_data->dev);
 		perf_status_str(s, pm_data->dev);
+		mode_status_str(s, pm_data->dev);
 		kfree(kobj_path);
 	}
 
@@ -3255,8 +3265,9 @@ static int summary_show(struct seq_file *s, void *data)
 	int ret = 0;
 
 	seq_puts(s, "domain                          status          children                           performance\n");
-	seq_puts(s, "    /device                                             runtime status\n");
-	seq_puts(s, "----------------------------------------------------------------------------------------------\n");
+	seq_puts(s, "    /device                                             runtime status                           managed by\n");
+	seq_puts(s, "------------------------------------------------------------------------------------------------------------\n");
+
 
 	ret = mutex_lock_interruptible(&gpd_list_lock);
 	if (ret)

-- 
2.34.1

Re: [PATCH RESEND v3 2/5] PM: domains: Add the domain HW-managed mode to the summary

[Dmitry Baryshkov]

On Wed, 1 Nov 2023 at 11:06, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> Now that genpd supports dynamically switching the control for an
> attached device between hardware- and software-mode,  let's add this
> information to the genpd summary in debugfs.
>
> Suggested-by: Taniya Das <quic_tdas@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/domain.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

--
With best wishes
Dmitry

Re: [PATCH RESEND v3 2/5] PM: domains: Add the domain HW-managed mode to the summary

[Bjorn Andersson]

On Wed, Nov 01, 2023 at 11:04:08AM +0200, Abel Vesa wrote:
> Now that genpd supports dynamically switching the control for an
> attached device between hardware- and software-mode,  let's add this
> information to the genpd summary in debugfs.
> 
> Suggested-by: Taniya Das <quic_tdas@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

> ---
>  drivers/base/power/domain.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 3fb1a234c7f2..7044271ec93b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -3181,6 +3181,15 @@ static void rtpm_status_str(struct seq_file *s, struct device *dev)
>  	seq_printf(s, "%-25s  ", p);
>  }
>  
> +static void mode_status_str(struct seq_file *s, struct device *dev)
> +{
> +	struct generic_pm_domain_data *gpd_data;
> +
> +	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> +
> +	seq_printf(s, "%20s", gpd_data->hw_mode ? "HW" : "SW");
> +}
> +
>  static void perf_status_str(struct seq_file *s, struct device *dev)
>  {
>  	struct generic_pm_domain_data *gpd_data;
> @@ -3239,6 +3248,7 @@ static int genpd_summary_one(struct seq_file *s,
>  		seq_printf(s, "\n    %-50s  ", kobj_path);
>  		rtpm_status_str(s, pm_data->dev);
>  		perf_status_str(s, pm_data->dev);
> +		mode_status_str(s, pm_data->dev);
>  		kfree(kobj_path);
>  	}
>  
> @@ -3255,8 +3265,9 @@ static int summary_show(struct seq_file *s, void *data)
>  	int ret = 0;
>  
>  	seq_puts(s, "domain                          status          children                           performance\n");
> -	seq_puts(s, "    /device                                             runtime status\n");
> -	seq_puts(s, "----------------------------------------------------------------------------------------------\n");
> +	seq_puts(s, "    /device                                             runtime status                           managed by\n");
> +	seq_puts(s, "------------------------------------------------------------------------------------------------------------\n");
> +
>  
>  	ret = mutex_lock_interruptible(&gpd_list_lock);
>  	if (ret)
> 
> -- 
> 2.34.1
> 

[PATCH RESEND v3 3/5] clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode

[Abel Vesa]

From: Jagadeesh Kona <quic_jkona@quicinc.com>

Add support for set and get hwmode callbacks to switch the GDSC between
SW and HW modes. Currently, the GDSC is moved to HW control mode
using HW_CTRL flag and if this flag is present, GDSC is moved to HW
mode as part of GDSC enable itself. The intention is to keep the
HW_CTRL flag functionality as is, since many older chipsets still use
this flag.

Introduce a new HW_CTRL_TRIGGER flag to switch the GDSC back and forth
between HW/SW modes dynamically at runtime. If HW_CTRL_TRIGGER flag is
present, register set_hwmode_dev callback to switch the GDSC mode which
can be invoked from consumer drivers using dev_pm_genpd_set_hwmode
function. Unlike HW_CTRL flag, HW_CTRL_TRIGGER won't move the GDSC to HW
control mode as part of GDSC enable itself, GDSC will be moved to HW
control mode only when consumer driver explicity calls
dev_pm_genpd_set_hwmode to switch to HW mode. Also add the
dev_pm_genpd_get_hwmode to allow the consumers to read the actual
HW/SW mode from hardware.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 32 ++++++++++++++++++++++++++++++++
 drivers/clk/qcom/gdsc.h |  1 +
 2 files changed, 33 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 5358e28122ab..c763524cd5da 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -363,6 +363,34 @@ 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);
+
+	if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) {
+		pr_err("Cannot set mode while parent is disabled\n");
+		return -EIO;
+	}
+
+	return gdsc_hwctrl(sc, mode);
+}
+
+static bool gdsc_get_hwmode(struct generic_pm_domain *domain, struct device *dev)
+{
+	struct gdsc *sc = domain_to_gdsc(domain);
+	u32 val;
+	int ret;
+
+	ret = regmap_read(sc->regmap, sc->gdscr, &val);
+	if (ret)
+		return ret;
+
+	if (val & HW_CONTROL_MASK)
+		return true;
+
+	return false;
+}
+
 static int gdsc_init(struct gdsc *sc)
 {
 	u32 mask, val;
@@ -451,6 +479,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;

-- 
2.34.1

Re: [PATCH RESEND v3 3/5] clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode

[Dmitry Baryshkov]

On Wed, 1 Nov 2023 at 11:06, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>
> Add support for set and get hwmode callbacks to switch the GDSC between
> SW and HW modes. Currently, the GDSC is moved to HW control mode
> using HW_CTRL flag and if this flag is present, GDSC is moved to HW
> mode as part of GDSC enable itself. The intention is to keep the
> HW_CTRL flag functionality as is, since many older chipsets still use
> this flag.
>
> Introduce a new HW_CTRL_TRIGGER flag to switch the GDSC back and forth
> between HW/SW modes dynamically at runtime. If HW_CTRL_TRIGGER flag is
> present, register set_hwmode_dev callback to switch the GDSC mode which
> can be invoked from consumer drivers using dev_pm_genpd_set_hwmode
> function. Unlike HW_CTRL flag, HW_CTRL_TRIGGER won't move the GDSC to HW
> control mode as part of GDSC enable itself, GDSC will be moved to HW
> control mode only when consumer driver explicity calls
> dev_pm_genpd_set_hwmode to switch to HW mode. Also add the
> dev_pm_genpd_get_hwmode to allow the consumers to read the actual
> HW/SW mode from hardware.

Can we add two new flags:
- HW_CTRL_TRIGGER
- DEFAULT_HW_TRIGGER

And then define HW_CTRL as HW_CTRL_TRIGGER | DEFAULT_HW_TRIGGER ?

This way older platforms will keep existing behaviour, but can
gradually migrate to the new callbacks?

>
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/clk/qcom/gdsc.c | 32 ++++++++++++++++++++++++++++++++
>  drivers/clk/qcom/gdsc.h |  1 +
>  2 files changed, 33 insertions(+)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 5358e28122ab..c763524cd5da 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -363,6 +363,34 @@ 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);
> +
> +       if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) {
> +               pr_err("Cannot set mode while parent is disabled\n");
> +               return -EIO;
> +       }
> +
> +       return gdsc_hwctrl(sc, mode);
> +}
> +
> +static bool gdsc_get_hwmode(struct generic_pm_domain *domain, struct device *dev)
> +{
> +       struct gdsc *sc = domain_to_gdsc(domain);
> +       u32 val;
> +       int ret;
> +
> +       ret = regmap_read(sc->regmap, sc->gdscr, &val);
> +       if (ret)
> +               return ret;
> +
> +       if (val & HW_CONTROL_MASK)
> +               return true;
> +
> +       return false;
> +}
> +
>  static int gdsc_init(struct gdsc *sc)
>  {
>         u32 mask, val;
> @@ -451,6 +479,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;
>
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH RESEND v3 3/5] clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode

[Jagadeesh Kona]

On 11/2/2023 2:58 AM, Dmitry Baryshkov wrote:
> On Wed, 1 Nov 2023 at 11:06, Abel Vesa <abel.vesa@linaro.org> wrote:
>>
>> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>>
>> Add support for set and get hwmode callbacks to switch the GDSC between
>> SW and HW modes. Currently, the GDSC is moved to HW control mode
>> using HW_CTRL flag and if this flag is present, GDSC is moved to HW
>> mode as part of GDSC enable itself. The intention is to keep the
>> HW_CTRL flag functionality as is, since many older chipsets still use
>> this flag.
>>
>> Introduce a new HW_CTRL_TRIGGER flag to switch the GDSC back and forth
>> between HW/SW modes dynamically at runtime. If HW_CTRL_TRIGGER flag is
>> present, register set_hwmode_dev callback to switch the GDSC mode which
>> can be invoked from consumer drivers using dev_pm_genpd_set_hwmode
>> function. Unlike HW_CTRL flag, HW_CTRL_TRIGGER won't move the GDSC to HW
>> control mode as part of GDSC enable itself, GDSC will be moved to HW
>> control mode only when consumer driver explicity calls
>> dev_pm_genpd_set_hwmode to switch to HW mode. Also add the
>> dev_pm_genpd_get_hwmode to allow the consumers to read the actual
>> HW/SW mode from hardware.
> 
> Can we add two new flags:
> - HW_CTRL_TRIGGER
> - DEFAULT_HW_TRIGGER
> 
> And then define HW_CTRL as HW_CTRL_TRIGGER | DEFAULT_HW_TRIGGER ?
> 
> This way older platforms will keep existing behaviour, but can
> gradually migrate to the new callbacks?
> 

Thanks Dmitry for your review,

The current usecases we have for GDSC's are either use the existing 
HW_CTRL flag to switch the GDSC to HW mode in gdsc_enable() and back to 
SW mode in gdsc_disable(). The second usecase is don't switch GDSC mode 
in gdsc_enable() & gdsc_disable() at all, and switch the GDSC mode only 
in new callbacks when consumers explicitly request it using 
dev_pm_genpd_set_hwmode(), this can be achieved using the new 
HW_CTRL_TRIGGER flag.

By defining HW_CTRL as HW_CTRL_TRIGGER | DEFAULT_HW_TRIGGER, it switches 
the GDSC mode both in enable & disable callbacks and in new callbacks as 
well. But we currently don't have any usecase that requires this behaviour.

Thanks,
Jagadeesh

>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>> ---
>>   drivers/clk/qcom/gdsc.c | 32 ++++++++++++++++++++++++++++++++
>>   drivers/clk/qcom/gdsc.h |  1 +
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index 5358e28122ab..c763524cd5da 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -363,6 +363,34 @@ 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);
>> +
>> +       if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) {
>> +               pr_err("Cannot set mode while parent is disabled\n");
>> +               return -EIO;
>> +       }
>> +
>> +       return gdsc_hwctrl(sc, mode);
>> +}
>> +
>> +static bool gdsc_get_hwmode(struct generic_pm_domain *domain, struct device *dev)
>> +{
>> +       struct gdsc *sc = domain_to_gdsc(domain);
>> +       u32 val;
>> +       int ret;
>> +
>> +       ret = regmap_read(sc->regmap, sc->gdscr, &val);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (val & HW_CONTROL_MASK)
>> +               return true;
>> +
>> +       return false;
>> +}
>> +
>>   static int gdsc_init(struct gdsc *sc)
>>   {
>>          u32 mask, val;
>> @@ -451,6 +479,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;
>>
>> --
>> 2.34.1
>>
> 
> 

[PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Abel Vesa]

From: Jagadeesh Kona <quic_jkona@quicinc.com>

The current HW_CTRL flag switches the video GDSC to HW control mode as
part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to
give consumer drivers more control and switch the GDSC mode as and when
required.

HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to
HW/SW control modes 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>
---
 drivers/clk/qcom/videocc-sc7180.c | 2 +-
 drivers/clk/qcom/videocc-sc7280.c | 2 +-
 drivers/clk/qcom/videocc-sdm845.c | 4 ++--
 drivers/clk/qcom/videocc-sm8250.c | 4 ++--
 drivers/clk/qcom/videocc-sm8550.c | 4 ++--
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
index 5b9b54f616b8..51439f7ba70c 100644
--- a/drivers/clk/qcom/videocc-sc7180.c
+++ b/drivers/clk/qcom/videocc-sc7180.c
@@ -166,7 +166,7 @@ static struct gdsc vcodec0_gdsc = {
 	.pd = {
 		.name = "vcodec0_gdsc",
 	},
-	.flags = HW_CTRL,
+	.flags = HW_CTRL_TRIGGER,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c
index 615695d82319..3d07b1e95986 100644
--- a/drivers/clk/qcom/videocc-sc7280.c
+++ b/drivers/clk/qcom/videocc-sc7280.c
@@ -236,7 +236,7 @@ static struct gdsc mvs0_gdsc = {
 		.name = "mvs0_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = HW_CTRL | RETAIN_FF_ENABLE,
+	.flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE,
 };
 
 static struct gdsc mvsc_gdsc = {
diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c
index c77a4dd5d39c..dad011c48973 100644
--- a/drivers/clk/qcom/videocc-sdm845.c
+++ b/drivers/clk/qcom/videocc-sdm845.c
@@ -260,7 +260,7 @@ static struct gdsc vcodec0_gdsc = {
 	},
 	.cxcs = (unsigned int []){ 0x890, 0x930 },
 	.cxc_count = 2,
-	.flags = HW_CTRL | POLL_CFG_GDSCR,
+	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -271,7 +271,7 @@ static struct gdsc vcodec1_gdsc = {
 	},
 	.cxcs = (unsigned int []){ 0x8d0, 0x950 },
 	.cxc_count = 2,
-	.flags = HW_CTRL | POLL_CFG_GDSCR,
+	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
index ad46c4014a40..c1b73d852f1c 100644
--- a/drivers/clk/qcom/videocc-sm8250.c
+++ b/drivers/clk/qcom/videocc-sm8250.c
@@ -293,7 +293,7 @@ static struct gdsc mvs0_gdsc = {
 	.pd = {
 		.name = "mvs0_gdsc",
 	},
-	.flags = HW_CTRL,
+	.flags = HW_CTRL_TRIGGER,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -302,7 +302,7 @@ static struct gdsc mvs1_gdsc = {
 	.pd = {
 		.name = "mvs1_gdsc",
 	},
-	.flags = HW_CTRL,
+	.flags = HW_CTRL_TRIGGER,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
index f3c9dfaee968..404c6600edae 100644
--- a/drivers/clk/qcom/videocc-sm8550.c
+++ b/drivers/clk/qcom/videocc-sm8550.c
@@ -322,7 +322,7 @@ static struct gdsc video_cc_mvs0_gdsc = {
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.parent = &video_cc_mvs0c_gdsc.pd,
-	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL,
+	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER,
 };
 
 static struct gdsc video_cc_mvs1c_gdsc = {
@@ -347,7 +347,7 @@ static struct gdsc video_cc_mvs1_gdsc = {
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.parent = &video_cc_mvs1c_gdsc.pd,
-	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL,
+	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER,
 };
 
 static struct clk_regmap *video_cc_sm8550_clocks[] = {

-- 
2.34.1

Re: [PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Dmitry Baryshkov]

On Wed, 1 Nov 2023 at 11:06, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>
> The current HW_CTRL flag switches the video GDSC to HW control mode as
> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to
> give consumer drivers more control and switch the GDSC mode as and when
> required.
>
> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to
> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API.

Will existing venus driver continue to work with this patch being applied?

>
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/clk/qcom/videocc-sc7180.c | 2 +-
>  drivers/clk/qcom/videocc-sc7280.c | 2 +-
>  drivers/clk/qcom/videocc-sdm845.c | 4 ++--
>  drivers/clk/qcom/videocc-sm8250.c | 4 ++--
>  drivers/clk/qcom/videocc-sm8550.c | 4 ++--
>  5 files changed, 8 insertions(+), 8 deletions(-)


-- 
With best wishes
Dmitry

Re: [PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Bjorn Andersson]

On Wed, Nov 01, 2023 at 11:04:10AM +0200, Abel Vesa wrote:
> From: Jagadeesh Kona <quic_jkona@quicinc.com>
> 
> The current HW_CTRL flag switches the video GDSC to HW control mode as
> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to
> give consumer drivers more control and switch the GDSC mode as and when
> required.
> 
> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to
> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API.
> 

This states what the code currently does, and what the new code will do.
But I don't find that it adequately describes _why_ this is done.


In the current implementation, the hardware is might collapse the GDSC
anytime between gdsc_enable() and gdsc_disable(). By giving "drivers
more control" the time spent in this state is reduced to some fraction
of that span, which to me implies higher power consumption.

Under the assumption that we don't want to consume more power without
reason, I'm forced to guess that there might be some scenarios that we
want this feature to keep the GDSC non-collapsed against the indication
of the hardware - to avoid some instability somewhere, perhaps?

Regards,
Bjorn

> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/clk/qcom/videocc-sc7180.c | 2 +-
>  drivers/clk/qcom/videocc-sc7280.c | 2 +-
>  drivers/clk/qcom/videocc-sdm845.c | 4 ++--
>  drivers/clk/qcom/videocc-sm8250.c | 4 ++--
>  drivers/clk/qcom/videocc-sm8550.c | 4 ++--
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
> index 5b9b54f616b8..51439f7ba70c 100644
> --- a/drivers/clk/qcom/videocc-sc7180.c
> +++ b/drivers/clk/qcom/videocc-sc7180.c
> @@ -166,7 +166,7 @@ static struct gdsc vcodec0_gdsc = {
>  	.pd = {
>  		.name = "vcodec0_gdsc",
>  	},
> -	.flags = HW_CTRL,
> +	.flags = HW_CTRL_TRIGGER,
>  	.pwrsts = PWRSTS_OFF_ON,
>  };
>  
> diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c
> index 615695d82319..3d07b1e95986 100644
> --- a/drivers/clk/qcom/videocc-sc7280.c
> +++ b/drivers/clk/qcom/videocc-sc7280.c
> @@ -236,7 +236,7 @@ static struct gdsc mvs0_gdsc = {
>  		.name = "mvs0_gdsc",
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = HW_CTRL | RETAIN_FF_ENABLE,
> +	.flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE,
>  };
>  
>  static struct gdsc mvsc_gdsc = {
> diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c
> index c77a4dd5d39c..dad011c48973 100644
> --- a/drivers/clk/qcom/videocc-sdm845.c
> +++ b/drivers/clk/qcom/videocc-sdm845.c
> @@ -260,7 +260,7 @@ static struct gdsc vcodec0_gdsc = {
>  	},
>  	.cxcs = (unsigned int []){ 0x890, 0x930 },
>  	.cxc_count = 2,
> -	.flags = HW_CTRL | POLL_CFG_GDSCR,
> +	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
>  	.pwrsts = PWRSTS_OFF_ON,
>  };
>  
> @@ -271,7 +271,7 @@ static struct gdsc vcodec1_gdsc = {
>  	},
>  	.cxcs = (unsigned int []){ 0x8d0, 0x950 },
>  	.cxc_count = 2,
> -	.flags = HW_CTRL | POLL_CFG_GDSCR,
> +	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
>  	.pwrsts = PWRSTS_OFF_ON,
>  };
>  
> diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
> index ad46c4014a40..c1b73d852f1c 100644
> --- a/drivers/clk/qcom/videocc-sm8250.c
> +++ b/drivers/clk/qcom/videocc-sm8250.c
> @@ -293,7 +293,7 @@ static struct gdsc mvs0_gdsc = {
>  	.pd = {
>  		.name = "mvs0_gdsc",
>  	},
> -	.flags = HW_CTRL,
> +	.flags = HW_CTRL_TRIGGER,
>  	.pwrsts = PWRSTS_OFF_ON,
>  };
>  
> @@ -302,7 +302,7 @@ static struct gdsc mvs1_gdsc = {
>  	.pd = {
>  		.name = "mvs1_gdsc",
>  	},
> -	.flags = HW_CTRL,
> +	.flags = HW_CTRL_TRIGGER,
>  	.pwrsts = PWRSTS_OFF_ON,
>  };
>  
> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
> index f3c9dfaee968..404c6600edae 100644
> --- a/drivers/clk/qcom/videocc-sm8550.c
> +++ b/drivers/clk/qcom/videocc-sm8550.c
> @@ -322,7 +322,7 @@ static struct gdsc video_cc_mvs0_gdsc = {
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
>  	.parent = &video_cc_mvs0c_gdsc.pd,
> -	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL,
> +	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER,
>  };
>  
>  static struct gdsc video_cc_mvs1c_gdsc = {
> @@ -347,7 +347,7 @@ static struct gdsc video_cc_mvs1_gdsc = {
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
>  	.parent = &video_cc_mvs1c_gdsc.pd,
> -	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL,
> +	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER,
>  };
>  
>  static struct clk_regmap *video_cc_sm8550_clocks[] = {
> 
> -- 
> 2.34.1
> 

Re: [PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Jagadeesh Kona]

On 11/4/2023 1:45 AM, Bjorn Andersson wrote:
> On Wed, Nov 01, 2023 at 11:04:10AM +0200, Abel Vesa wrote:
>> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>>
>> The current HW_CTRL flag switches the video GDSC to HW control mode as
>> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to
>> give consumer drivers more control and switch the GDSC mode as and when
>> required.
>>
>> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to
>> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API.
>>
> 
> This states what the code currently does, and what the new code will do.
> But I don't find that it adequately describes _why_ this is done.
> 
> 
> In the current implementation, the hardware is might collapse the GDSC
> anytime between gdsc_enable() and gdsc_disable(). By giving "drivers
> more control" the time spent in this state is reduced to some fraction
> of that span, which to me implies higher power consumption.
> 
> Under the assumption that we don't want to consume more power without
> reason, I'm forced to guess that there might be some scenarios that we
> want this feature to keep the GDSC non-collapsed against the indication
> of the hardware - to avoid some instability somewhere, perhaps?
> 

Thanks Bjorn for your review. Sure, will update commit text with details 
in next series.

Normally, consumers will enable the GDSC and then the required clocks. 
If GDSC is moved to HW mode in gdsc_enable() itself, the subsequent 
clocks enablement that are dependent on GDSC might fail since GDSC could 
be turned off by HW. The consumers can still switch the GDSC to HW mode 
with new API right after the clocks are enabled and the control will be 
taken back to SW mode just before disabling the GDSC, so even with the 
newer implementation, HW can collapse the GDSC anytime for most of the 
duration between gdsc_enable() and gdsc_disable(). This API adds more 
flexibility for consumer drivers to control the GDSC mode as per their
requirements.

Thanks,
Jagadeesh

> Regards,
> Bjorn
> 
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>> ---
>>   drivers/clk/qcom/videocc-sc7180.c | 2 +-
>>   drivers/clk/qcom/videocc-sc7280.c | 2 +-
>>   drivers/clk/qcom/videocc-sdm845.c | 4 ++--
>>   drivers/clk/qcom/videocc-sm8250.c | 4 ++--
>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++--
>>   5 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
>> index 5b9b54f616b8..51439f7ba70c 100644
>> --- a/drivers/clk/qcom/videocc-sc7180.c
>> +++ b/drivers/clk/qcom/videocc-sc7180.c
>> @@ -166,7 +166,7 @@ static struct gdsc vcodec0_gdsc = {
>>   	.pd = {
>>   		.name = "vcodec0_gdsc",
>>   	},
>> -	.flags = HW_CTRL,
>> +	.flags = HW_CTRL_TRIGGER,
>>   	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>   
>> diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c
>> index 615695d82319..3d07b1e95986 100644
>> --- a/drivers/clk/qcom/videocc-sc7280.c
>> +++ b/drivers/clk/qcom/videocc-sc7280.c
>> @@ -236,7 +236,7 @@ static struct gdsc mvs0_gdsc = {
>>   		.name = "mvs0_gdsc",
>>   	},
>>   	.pwrsts = PWRSTS_OFF_ON,
>> -	.flags = HW_CTRL | RETAIN_FF_ENABLE,
>> +	.flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE,
>>   };
>>   
>>   static struct gdsc mvsc_gdsc = {
>> diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c
>> index c77a4dd5d39c..dad011c48973 100644
>> --- a/drivers/clk/qcom/videocc-sdm845.c
>> +++ b/drivers/clk/qcom/videocc-sdm845.c
>> @@ -260,7 +260,7 @@ static struct gdsc vcodec0_gdsc = {
>>   	},
>>   	.cxcs = (unsigned int []){ 0x890, 0x930 },
>>   	.cxc_count = 2,
>> -	.flags = HW_CTRL | POLL_CFG_GDSCR,
>> +	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
>>   	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>   
>> @@ -271,7 +271,7 @@ static struct gdsc vcodec1_gdsc = {
>>   	},
>>   	.cxcs = (unsigned int []){ 0x8d0, 0x950 },
>>   	.cxc_count = 2,
>> -	.flags = HW_CTRL | POLL_CFG_GDSCR,
>> +	.flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
>>   	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>   
>> diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
>> index ad46c4014a40..c1b73d852f1c 100644
>> --- a/drivers/clk/qcom/videocc-sm8250.c
>> +++ b/drivers/clk/qcom/videocc-sm8250.c
>> @@ -293,7 +293,7 @@ static struct gdsc mvs0_gdsc = {
>>   	.pd = {
>>   		.name = "mvs0_gdsc",
>>   	},
>> -	.flags = HW_CTRL,
>> +	.flags = HW_CTRL_TRIGGER,
>>   	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>   
>> @@ -302,7 +302,7 @@ static struct gdsc mvs1_gdsc = {
>>   	.pd = {
>>   		.name = "mvs1_gdsc",
>>   	},
>> -	.flags = HW_CTRL,
>> +	.flags = HW_CTRL_TRIGGER,
>>   	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>   
>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>> index f3c9dfaee968..404c6600edae 100644
>> --- a/drivers/clk/qcom/videocc-sm8550.c
>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>> @@ -322,7 +322,7 @@ static struct gdsc video_cc_mvs0_gdsc = {
>>   	},
>>   	.pwrsts = PWRSTS_OFF_ON,
>>   	.parent = &video_cc_mvs0c_gdsc.pd,
>> -	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL,
>> +	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER,
>>   };
>>   
>>   static struct gdsc video_cc_mvs1c_gdsc = {
>> @@ -347,7 +347,7 @@ static struct gdsc video_cc_mvs1_gdsc = {
>>   	},
>>   	.pwrsts = PWRSTS_OFF_ON,
>>   	.parent = &video_cc_mvs1c_gdsc.pd,
>> -	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL,
>> +	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER,
>>   };
>>   
>>   static struct clk_regmap *video_cc_sm8550_clocks[] = {
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Vikash Garodia]

On 11/10/2023 2:02 PM, Jagadeesh Kona wrote:
> 
> 
> On 11/4/2023 1:45 AM, Bjorn Andersson wrote:
>> On Wed, Nov 01, 2023 at 11:04:10AM +0200, Abel Vesa wrote:
>>> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>
>>> The current HW_CTRL flag switches the video GDSC to HW control mode as
>>> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to
>>> give consumer drivers more control and switch the GDSC mode as and when
>>> required.
>>>
>>> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to
>>> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API.
>>>
>>
>> This states what the code currently does, and what the new code will do.
>> But I don't find that it adequately describes _why_ this is done.
>>
>>
>> In the current implementation, the hardware is might collapse the GDSC
>> anytime between gdsc_enable() and gdsc_disable(). By giving "drivers
>> more control" the time spent in this state is reduced to some fraction
>> of that span, which to me implies higher power consumption.
>>
>> Under the assumption that we don't want to consume more power without
>> reason, I'm forced to guess that there might be some scenarios that we
>> want this feature to keep the GDSC non-collapsed against the indication
>> of the hardware - to avoid some instability somewhere, perhaps?
>>
> 
> Thanks Bjorn for your review. Sure, will update commit text with details in next
> series.
> 
> Normally, consumers will enable the GDSC and then the required clocks. If GDSC
> is moved to HW mode in gdsc_enable() itself, the subsequent clocks enablement
> that are dependent on GDSC might fail since GDSC could be turned off by HW. The
> consumers can still switch the GDSC to HW mode with new API right after the
> clocks are enabled and the control will be taken back to SW mode just before
> disabling the GDSC, so even with the newer implementation, HW can collapse the
> GDSC anytime for most of the duration between gdsc_enable() and gdsc_disable().
> This API adds more flexibility for consumer drivers to control the GDSC mode as
> per their
> requirements.
There is one more scenario where the driver would like GDSC in driver
controlled. Let say video hardware, which is under vcodec0_gdsc, have registers
to be programmed by TZ. In such scenario, the GDSC should be non collapsed,
while TZ programs those registers precisely while loading the firmware and
bringing hardware out of reset.

Regards,
Vikash

Re: [PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Konrad Dybcio]

On 11/7/23 14:05, Bryan O'Donoghue wrote:
> On 01/11/2023 09:04, Abel Vesa wrote:
>> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>>
>> The current HW_CTRL flag switches the video GDSC to HW control mode as
>> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to
>> give consumer drivers more control and switch the GDSC mode as and when
>> required.
>>
>> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to
>> HW/SW control modes 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>
>> ---
>>   drivers/clk/qcom/videocc-sc7180.c | 2 +-
>>   drivers/clk/qcom/videocc-sc7280.c | 2 +-
>>   drivers/clk/qcom/videocc-sdm845.c | 4 ++--
>>   drivers/clk/qcom/videocc-sm8250.c | 4 ++--
>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++--
>>   5 files changed, 8 insertions(+), 8 deletions(-)
> 
> So.
> 
> I'm assuming the rest of this series works however for sc8250 at least this is a NAK, breaks venus on rb5.
Are you saying that applying this patch alone causes the attached crash?

Konrad

Re: [PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Bryan O'Donoghue]

On 07/11/2023 21:21, Konrad Dybcio wrote:
>>
>> So.
>>
>> I'm assuming the rest of this series works however for sc8250 at least 
>> this is a NAK, breaks venus on rb5.
> Are you saying that applying this patch alone causes the attached crash?
> 
> Konrad

I tried to revert this patch in isolation and got

->

[  157.083287] qcom-venus aa00000.video-codec: Failed to switch 
power-domain:1 to SW mode
[  162.004630] qcom-venus aa00000.video-codec: Failed to switch 
power-domain:1 to SW mode

I stopped debugging there.

---
bod

Re: [PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Jagadeesh Kona]

On 11/7/2023 6:35 PM, Bryan O'Donoghue wrote:
> On 01/11/2023 09:04, Abel Vesa wrote:
>> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>>
>> The current HW_CTRL flag switches the video GDSC to HW control mode as
>> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to
>> give consumer drivers more control and switch the GDSC mode as and when
>> required.
>>
>> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to
>> HW/SW control modes 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>
>> ---
>>   drivers/clk/qcom/videocc-sc7180.c | 2 +-
>>   drivers/clk/qcom/videocc-sc7280.c | 2 +-
>>   drivers/clk/qcom/videocc-sdm845.c | 4 ++--
>>   drivers/clk/qcom/videocc-sm8250.c | 4 ++--
>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++--
>>   5 files changed, 8 insertions(+), 8 deletions(-)
> 
> So.
> 
> I'm assuming the rest of this series works however for sc8250 at least 
> this is a NAK, breaks venus on rb5.
> 
> Reproduction:
> 
> - Debian trixie rb5
> - Linux 6.6-rc3 + this patch
> - Attached defconfig
> - This file:
>    https://download.samplelib.com/mp4/sample-30s.mp4
> - This command:
>    ffplay -loglevel debug -code:video h264_v4l2m2m -i sample-30s.mp4
> 
> Second play of file shows stuck gdsc as below
> 
> I didn't try on rb3, I'd expect similar results. Does this work on 8550 ?
> 
> [ 1601.581204] ------------[ cut here ]------------
> [ 1601.585983] mvs0_gdsc status stuck at 'off'
> [ 1601.586015] WARNING: CPU: 1 PID: 13372 at drivers/clk/qcom/gdsc.c:178 
> gdsc_toggle_logic+0x16c/0x174
> [ 1601.599627] Modules linked in: nf_tables libcrc32c nfnetlink 
> snd_soc_hdmi_codec q6asm_dai q6routing q6afe_dai q6asm q6adm 
> q6afe_clocks snd_q6dsp_common q6afe q6core apr pdr_interface venus_enc 
> venus_dec qcom_camss videobuf2_dma_contig mcp251xfd imx412 
> videobuf2_dma_sg venus_core xhci_plat_hcd v4l2_fwnode fastrpc xhci_hcd 
> can_dev qrtr_smd lontium_lt9611uxc msm v4l2_async v4l2_mem2mem 
> qcom_spmi_adc_tm5 rtc_pm8xxx qcom_spmi_adc5 qcom_pon videobuf2_memops 
> crct10dif_ce qcom_spmi_temp_alarm videobuf2_v4l2 qcom_vadc_common 
> gpu_sched drm_dp_aux_bus videodev snd_soc_sm8250 drm_display_helper 
> snd_soc_qcom_sdw videobuf2_common snd_soc_qcom_common qrtr i2c_qcom_cci 
> soundwire_qcom mc i2c_qcom_geni spi_geni_qcom phy_qcom_qmp_combo 
> qcom_q6v5_pas qcom_rng soundwire_bus qcom_pil_info 
> snd_soc_lpass_va_macro qcom_q6v5 slimbus snd_soc_lpass_macro_common 
> qcom_sysmon snd_soc_lpass_wsa_macro display_connector qcom_common 
> socinfo qcom_glink_smem qmi_helpers drm_kms_helper mdt_loader qcom_wdt 
> icc_osm_l3 qnoc_sm8250 fuse drm backlight dm_mod
> [ 1601.599859]  ip_tables x_tables
> [ 1601.695314] CPU: 1 PID: 13372 Comm: video_decoder Not tainted 
> 6.6.0-rc3-00396-gdbc0d9fa7641-dirty #1
> [ 1601.704694] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
> [ 1601.711582] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS 
> BTYPE=--)
> [ 1601.718740] pc : gdsc_toggle_logic+0x16c/0x174
> [ 1601.723317] lr : gdsc_toggle_logic+0x16c/0x174
> [ 1601.727888] sp : ffff80008adab800
> [ 1601.731296] x29: ffff80008adab800 x28: ffffb661e8596210 x27: 
> ffffb661e855ad88
> [ 1601.738629] x26: 0000000000000000 x25: ffff100c0b5a0d28 x24: 
> ffffb6620fd92118
> [ 1601.745960] x23: ffffb6620fe1d3d8 x22: 0000000000000000 x21: 
> 0000000000000001
> [ 1601.753292] x20: 00000000ffffff92 x19: ffffb6620fd92118 x18: 
> ffffffffffc0d3e8
> [ 1601.760631] x17: 0000000000000000 x16: ffffb6620e269e14 x15: 
> 0000000000000028
> [ 1601.767973] x14: 0000000000000000 x13: ffff100d75c00000 x12: 
> 0000000000000894
> [ 1601.775304] x11: 00000000000002dc x10: ffff100d767044a0 x9 : 
> ffff100d75c00000
> [ 1601.782636] x8 : 00000000fffdffff x7 : ffff100d76700000 x6 : 
> 00000000000002dc
> [ 1601.789976] x5 : ffff100d7ef40d48 x4 : 40000000fffe02dc x3 : 
> ffff59ab6fa21000
> [ 1601.797306] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
> ffff100cdacdec80
> [ 1601.804638] Call trace:
> [ 1601.807161]  gdsc_toggle_logic+0x16c/0x174
> [ 1601.811383]  gdsc_enable+0x60/0x27c
> [ 1601.814982]  _genpd_power_on+0x94/0x184
> [ 1601.818931]  genpd_power_on+0xa8/0x16c
> [ 1601.822791]  genpd_runtime_resume+0xd4/0x2a4
> [ 1601.827184]  __rpm_callback+0x48/0x1dc
> [ 1601.831045]  rpm_callback+0x6c/0x78
> [ 1601.834638]  rpm_resume+0x45c/0x6c8
> [ 1601.838231]  __pm_runtime_resume+0x4c/0x90
> [ 1601.842443]  coreid_power_v4+0x378/0x58c [venus_core]
> [ 1601.847695]  vdec_start_streaming+0xc0/0x4e8 [venus_dec]
> [ 1601.853169]  vb2_start_streaming+0x68/0x15c [videobuf2_common]
> [ 1601.859199]  vb2_core_streamon+0xf8/0x1bc [videobuf2_common]
> [ 1601.865032]  vb2_streamon+0x18/0x64 [videobuf2_v4l2]
> [ 1601.870174]  v4l2_m2m_ioctl_streamon+0x38/0x98 [v4l2_mem2mem]
> [ 1601.876134]  v4l_streamon+0x24/0x30 [videodev]
> [ 1601.880759]  __video_do_ioctl+0x15c/0x3c0 [videodev]
> [ 1601.885905]  video_usercopy+0x1f0/0x658 [videodev]
> [ 1601.890868]  video_ioctl2+0x18/0x28 [videodev]
> [ 1601.895481]  v4l2_ioctl+0x40/0x60 [videodev]
> [ 1601.899911]  __arm64_sys_ioctl+0xac/0xf0
> [ 1601.903958]  invoke_syscall+0x48/0x114
> [ 1601.907829]  el0_svc_common.constprop.0+0x40/0xe0
> [ 1601.912672]  do_el0_svc+0x1c/0x28
> [ 1601.916085]  el0_svc+0x40/0xe8
> [ 1601.919243]  el0t_64_sync_handler+0x100/0x12c
> [ 1601.923730]  el0t_64_sync+0x190/0x194
> [ 1601.927505] ---[ end trace 0000000000000000 ]---
> [ 1608.121533] ------------[ cut here ]------------
> 
> And just reverting the one patch - yields
> 
> [  157.083287] qcom-venus aa00000.video-codec: Failed to switch 
> power-domain:1 to SW mode
> [  162.004630] qcom-venus aa00000.video-codec: Failed to switch 
> power-domain:1 to SW mode
> 
> I'll leave the testing here. Please fix !
> 

Thanks Bryan for reporting this, this could be happening since GDSC 
might be left in HW control mode during power off sequence, so the 
subsequent GDSC enable is failing since GDSC is still in HW mode. I am 
checking internally on this and will get back.

Thanks,
Jagadeesh

> ---
> bod

Re: [PATCH RESEND v3 4/5] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Bryan O'Donoghue]

On 10/11/2023 08:29, Jagadeesh Kona wrote:
> 
> 
> On 11/7/2023 6:35 PM, Bryan O'Donoghue wrote:
>> On 01/11/2023 09:04, Abel Vesa wrote:
>>> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>
>>> The current HW_CTRL flag switches the video GDSC to HW control mode as
>>> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to
>>> give consumer drivers more control and switch the GDSC mode as and when
>>> required.
>>>
>>> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to
>>> HW/SW control modes 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>
>>> ---
>>>   drivers/clk/qcom/videocc-sc7180.c | 2 +-
>>>   drivers/clk/qcom/videocc-sc7280.c | 2 +-
>>>   drivers/clk/qcom/videocc-sdm845.c | 4 ++--
>>>   drivers/clk/qcom/videocc-sm8250.c | 4 ++--
>>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++--
>>>   5 files changed, 8 insertions(+), 8 deletions(-)
>>
>> So.
>>
>> I'm assuming the rest of this series works however for sc8250 at least 
>> this is a NAK, breaks venus on rb5.
>>
>> Reproduction:
>>
>> - Debian trixie rb5
>> - Linux 6.6-rc3 + this patch
>> - Attached defconfig
>> - This file:
>>    https://download.samplelib.com/mp4/sample-30s.mp4
>> - This command:
>>    ffplay -loglevel debug -code:video h264_v4l2m2m -i sample-30s.mp4
>>
>> Second play of file shows stuck gdsc as below
>>
>> I didn't try on rb3, I'd expect similar results. Does this work on 8550 ?
>>
>> [ 1601.581204] ------------[ cut here ]------------
>> [ 1601.585983] mvs0_gdsc status stuck at 'off'
>> [ 1601.586015] WARNING: CPU: 1 PID: 13372 at 
>> drivers/clk/qcom/gdsc.c:178 gdsc_toggle_logic+0x16c/0x174
>> [ 1601.599627] Modules linked in: nf_tables libcrc32c nfnetlink 
>> snd_soc_hdmi_codec q6asm_dai q6routing q6afe_dai q6asm q6adm 
>> q6afe_clocks snd_q6dsp_common q6afe q6core apr pdr_interface venus_enc 
>> venus_dec qcom_camss videobuf2_dma_contig mcp251xfd imx412 
>> videobuf2_dma_sg venus_core xhci_plat_hcd v4l2_fwnode fastrpc xhci_hcd 
>> can_dev qrtr_smd lontium_lt9611uxc msm v4l2_async v4l2_mem2mem 
>> qcom_spmi_adc_tm5 rtc_pm8xxx qcom_spmi_adc5 qcom_pon videobuf2_memops 
>> crct10dif_ce qcom_spmi_temp_alarm videobuf2_v4l2 qcom_vadc_common 
>> gpu_sched drm_dp_aux_bus videodev snd_soc_sm8250 drm_display_helper 
>> snd_soc_qcom_sdw videobuf2_common snd_soc_qcom_common qrtr 
>> i2c_qcom_cci soundwire_qcom mc i2c_qcom_geni spi_geni_qcom 
>> phy_qcom_qmp_combo qcom_q6v5_pas qcom_rng soundwire_bus qcom_pil_info 
>> snd_soc_lpass_va_macro qcom_q6v5 slimbus snd_soc_lpass_macro_common 
>> qcom_sysmon snd_soc_lpass_wsa_macro display_connector qcom_common 
>> socinfo qcom_glink_smem qmi_helpers drm_kms_helper mdt_loader qcom_wdt 
>> icc_osm_l3 qnoc_sm8250 fuse drm backlight dm_mod
>> [ 1601.599859]  ip_tables x_tables
>> [ 1601.695314] CPU: 1 PID: 13372 Comm: video_decoder Not tainted 
>> 6.6.0-rc3-00396-gdbc0d9fa7641-dirty #1
>> [ 1601.704694] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 
>> (DT)
>> [ 1601.711582] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS 
>> BTYPE=--)
>> [ 1601.718740] pc : gdsc_toggle_logic+0x16c/0x174
>> [ 1601.723317] lr : gdsc_toggle_logic+0x16c/0x174
>> [ 1601.727888] sp : ffff80008adab800
>> [ 1601.731296] x29: ffff80008adab800 x28: ffffb661e8596210 x27: 
>> ffffb661e855ad88
>> [ 1601.738629] x26: 0000000000000000 x25: ffff100c0b5a0d28 x24: 
>> ffffb6620fd92118
>> [ 1601.745960] x23: ffffb6620fe1d3d8 x22: 0000000000000000 x21: 
>> 0000000000000001
>> [ 1601.753292] x20: 00000000ffffff92 x19: ffffb6620fd92118 x18: 
>> ffffffffffc0d3e8
>> [ 1601.760631] x17: 0000000000000000 x16: ffffb6620e269e14 x15: 
>> 0000000000000028
>> [ 1601.767973] x14: 0000000000000000 x13: ffff100d75c00000 x12: 
>> 0000000000000894
>> [ 1601.775304] x11: 00000000000002dc x10: ffff100d767044a0 x9 : 
>> ffff100d75c00000
>> [ 1601.782636] x8 : 00000000fffdffff x7 : ffff100d76700000 x6 : 
>> 00000000000002dc
>> [ 1601.789976] x5 : ffff100d7ef40d48 x4 : 40000000fffe02dc x3 : 
>> ffff59ab6fa21000
>> [ 1601.797306] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
>> ffff100cdacdec80
>> [ 1601.804638] Call trace:
>> [ 1601.807161]  gdsc_toggle_logic+0x16c/0x174
>> [ 1601.811383]  gdsc_enable+0x60/0x27c
>> [ 1601.814982]  _genpd_power_on+0x94/0x184
>> [ 1601.818931]  genpd_power_on+0xa8/0x16c
>> [ 1601.822791]  genpd_runtime_resume+0xd4/0x2a4
>> [ 1601.827184]  __rpm_callback+0x48/0x1dc
>> [ 1601.831045]  rpm_callback+0x6c/0x78
>> [ 1601.834638]  rpm_resume+0x45c/0x6c8
>> [ 1601.838231]  __pm_runtime_resume+0x4c/0x90
>> [ 1601.842443]  coreid_power_v4+0x378/0x58c [venus_core]
>> [ 1601.847695]  vdec_start_streaming+0xc0/0x4e8 [venus_dec]
>> [ 1601.853169]  vb2_start_streaming+0x68/0x15c [videobuf2_common]
>> [ 1601.859199]  vb2_core_streamon+0xf8/0x1bc [videobuf2_common]
>> [ 1601.865032]  vb2_streamon+0x18/0x64 [videobuf2_v4l2]
>> [ 1601.870174]  v4l2_m2m_ioctl_streamon+0x38/0x98 [v4l2_mem2mem]
>> [ 1601.876134]  v4l_streamon+0x24/0x30 [videodev]
>> [ 1601.880759]  __video_do_ioctl+0x15c/0x3c0 [videodev]
>> [ 1601.885905]  video_usercopy+0x1f0/0x658 [videodev]
>> [ 1601.890868]  video_ioctl2+0x18/0x28 [videodev]
>> [ 1601.895481]  v4l2_ioctl+0x40/0x60 [videodev]
>> [ 1601.899911]  __arm64_sys_ioctl+0xac/0xf0
>> [ 1601.903958]  invoke_syscall+0x48/0x114
>> [ 1601.907829]  el0_svc_common.constprop.0+0x40/0xe0
>> [ 1601.912672]  do_el0_svc+0x1c/0x28
>> [ 1601.916085]  el0_svc+0x40/0xe8
>> [ 1601.919243]  el0t_64_sync_handler+0x100/0x12c
>> [ 1601.923730]  el0t_64_sync+0x190/0x194
>> [ 1601.927505] ---[ end trace 0000000000000000 ]---
>> [ 1608.121533] ------------[ cut here ]------------
>>
>> And just reverting the one patch - yields
>>
>> [  157.083287] qcom-venus aa00000.video-codec: Failed to switch 
>> power-domain:1 to SW mode
>> [  162.004630] qcom-venus aa00000.video-codec: Failed to switch 
>> power-domain:1 to SW mode
>>
>> I'll leave the testing here. Please fix !
>>
> 
> Thanks Bryan for reporting this, this could be happening since GDSC 
> might be left in HW control mode during power off sequence, so the 
> subsequent GDSC enable is failing since GDSC is still in HW mode. I am 
> checking internally on this and will get back.

Great,

Please remember to check for rb3 / sdm845 too.

---
bod

[PATCH RESEND v3 5/5] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode

[Abel Vesa]

From: Jagadeesh Kona <quic_jkona@quicinc.com>

This change demonstrates the use of dev_pm_genpd_set_hwmode API from
video driver to switch the video mvs0 gdsc to SW/HW modes at runtime
based on requirement.

This change adds a new boolean array member vcodec_pmdomains_hwctrl in
venus_resources structure to indicate if GDSC's have HW control support
or not. This data is used in vcodec_control_v4() to check if GDSC has
support to switch to HW control mode and then call dev_pm_genpd_set_hwmode
to switch the GDSC mode.

Before the GDSC HWCTL was available to the consumer, the venus driver
needed to somehow keep the power from collapsing while under the driver
control. The only way to do that was to clear the CORE_PWR_DISABLE bit
(in wrapper POWER_CONTROL register) and, respectively, set it back after
the driver control was completed.

Now, that there is a way to switch the GDSC HW/SW control back and
forth, the CORE_PWR_DISABLE toggling can be dropped.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       |  4 +++
 drivers/media/platform/qcom/venus/core.h       |  1 +
 drivers/media/platform/qcom/venus/pm_helpers.c | 47 +++++++++++---------------
 3 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 054b8e74ba4f..8145062ab6f7 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -706,6 +706,7 @@ static const struct venus_resources sdm845_res_v2 = {
 	.vcodec1_clks = { "vcodec1_core", "vcodec1_bus" },
 	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = { "venus", "vcodec0", "vcodec1" },
+	.vcodec_pmdomains_hwctrl = { false, true, true },
 	.vcodec_pmdomains_num = 3,
 	.opp_pmdomain = (const char *[]) { "cx", NULL },
 	.vcodec_num = 2,
@@ -755,6 +756,7 @@ static const struct venus_resources sc7180_res = {
 	.vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
 	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = { "venus", "vcodec0" },
+	.vcodec_pmdomains_hwctrl = { false, true },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = (const char *[]) { "cx", NULL },
 	.vcodec_num = 1,
@@ -812,6 +814,7 @@ static const struct venus_resources sm8250_res = {
 	.vcodec0_clks = { "vcodec0_core" },
 	.vcodec_clks_num = 1,
 	.vcodec_pmdomains = { "venus", "vcodec0" },
+	.vcodec_pmdomains_hwctrl = { false, true },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = (const char *[]) { "mx", NULL },
 	.vcodec_num = 1,
@@ -871,6 +874,7 @@ static const struct venus_resources sc7280_res = {
 	.vcodec0_clks = {"vcodec_core", "vcodec_bus"},
 	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = { "venus", "vcodec0" },
+	.vcodec_pmdomains_hwctrl = { false, true },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = (const char *[]) { "cx", NULL },
 	.vcodec_num = 1,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 4a633261ece4..6d591ecad482 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -73,6 +73,7 @@ struct venus_resources {
 	const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
 	unsigned int vcodec_clks_num;
 	const char * const vcodec_pmdomains[VIDC_PMDOMAINS_NUM_MAX];
+	bool vcodec_pmdomains_hwctrl[VIDC_PMDOMAINS_NUM_MAX];
 	unsigned int vcodec_pmdomains_num;
 	const char **opp_pmdomain;
 	unsigned int vcodec_num;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index a1b127caa90a..9d1dc8366ab0 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -408,35 +408,28 @@ static const struct venus_pm_ops pm_ops_v3 = {
 
 static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
 {
-	void __iomem *ctrl, *stat;
-	u32 val;
-	int ret;
-
-	if (IS_V6(core)) {
-		ctrl = core->wrapper_base + WRAPPER_CORE_POWER_CONTROL_V6;
-		stat = core->wrapper_base + WRAPPER_CORE_POWER_STATUS_V6;
-	} else if (coreid == VIDC_CORE_ID_1) {
-		ctrl = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_CONTROL;
-		stat = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_STATUS;
-	} else {
-		ctrl = core->wrapper_base + WRAPPER_VCODEC1_MMCC_POWER_CONTROL;
-		stat = core->wrapper_base + WRAPPER_VCODEC1_MMCC_POWER_STATUS;
-	}
-
-	if (enable) {
-		writel(0, ctrl);
-
-		ret = readl_poll_timeout(stat, val, val & BIT(1), 1, 100);
-		if (ret)
-			return ret;
-	} else {
-		writel(1, ctrl);
+	int i, ret = 0;
+	struct device *dev = core->dev;
+	const struct venus_resources *res = core->res;
 
-		ret = readl_poll_timeout(stat, val, !(val & BIT(1)), 1, 100);
-		if (ret)
-			return ret;
+	for (i = 0; i < res->vcodec_pmdomains_num; i++) {
+		if (res->vcodec_pmdomains_hwctrl[i]) {
+
+			if (!core->pmdomains[i])
+				return -ENODEV;
+
+			/*
+			 * enable( true ), switch the gdsc to SW mode
+			 * enable( false), switch the gdsc to HW mode
+			 */
+			ret = dev_pm_genpd_set_hwmode(core->pmdomains[i], !enable);
+			if (ret) {
+				dev_err(dev, "Failed to switch power-domain:%d to %s mode\n",
+					i, enable ? "SW" : "HW");
+				return ret;
+			}
+		}
 	}
-
 	return 0;
 }
 

-- 
2.34.1

Re: [PATCH RESEND v3 5/5] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode

[Bryan O'Donoghue]

On 01/11/2023 09:04, Abel Vesa wrote:
> From: Jagadeesh Kona <quic_jkona@quicinc.com>
> 
> This change demonstrates the use of dev_pm_genpd_set_hwmode API from
> video driver to switch the video mvs0 gdsc to SW/HW modes at runtime
> based on requirement.
> 
> This change adds a new boolean array member vcodec_pmdomains_hwctrl in
> venus_resources structure to indicate if GDSC's have HW control support
> or not. This data is used in vcodec_control_v4() to check if GDSC has
> support to switch to HW control mode and then call dev_pm_genpd_set_hwmode
> to switch the GDSC mode.
> 
> Before the GDSC HWCTL was available to the consumer, the venus driver
> needed to somehow keep the power from collapsing while under the driver
> control. The only way to do that was to clear the CORE_PWR_DISABLE bit
> (in wrapper POWER_CONTROL register) and, respectively, set it back after
> the driver control was completed.
> 
> Now, that there is a way to switch the GDSC HW/SW control back and
> forth, the CORE_PWR_DISABLE toggling can be dropped.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

So I plan to give this a test on rb5 and db410c

My q here though is - has anybody on the submission list tested this 
through suspend/resume and vdd min ?

---
bod

Re: [PATCH RESEND v3 5/5] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode

[Konrad Dybcio]

On 11/3/23 11:44, Bryan O'Donoghue wrote:
> On 01/11/2023 09:04, Abel Vesa wrote:
>> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>>
>> This change demonstrates the use of dev_pm_genpd_set_hwmode API from
>> video driver to switch the video mvs0 gdsc to SW/HW modes at runtime
>> based on requirement.
>>
>> This change adds a new boolean array member vcodec_pmdomains_hwctrl in
>> venus_resources structure to indicate if GDSC's have HW control support
>> or not. This data is used in vcodec_control_v4() to check if GDSC has
>> support to switch to HW control mode and then call dev_pm_genpd_set_hwmode
>> to switch the GDSC mode.
>>
>> Before the GDSC HWCTL was available to the consumer, the venus driver
>> needed to somehow keep the power from collapsing while under the driver
>> control. The only way to do that was to clear the CORE_PWR_DISABLE bit
>> (in wrapper POWER_CONTROL register) and, respectively, set it back after
>> the driver control was completed.
>>
>> Now, that there is a way to switch the GDSC HW/SW control back and
>> forth, the CORE_PWR_DISABLE toggling can be dropped.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> 
> So I plan to give this a test on rb5 and db410c
> 
> My q here though is - has anybody on the submission list tested this through suspend/resume and vdd min ?
I think that only chromebooks (sc7[12]80) and sc7180-acer-aspire1.dts
are able to hit vddmin upstream as of today

Konrad

Re: [PATCH RESEND v3 0/5] PM: domains: Add control for switching back and forth to HW control

[Greg Kroah-Hartman]

On Wed, Nov 01, 2023 at 11:04:06AM +0200, Abel Vesa wrote:
> This is just a resend of v3:
> https://lore.kernel.org/lkml/20230923115008.1698384-1-abel.vesa@linaro.org/

Why?

It's the middle of the merge window, not much we can do until -rc1 is
out...

thanks,

greg k-h