[PATCH v2 0/6] PM: domains: Add control for switching back and forth to HW control

[PATCH v2 0/6] PM: domains: Add control for switching back and forth to HW control

[Abel Vesa]

The v1 (and the back story) can be found here:
https://lore.kernel.org/all/20230628105652.1670316-1-abel.vesa@linaro.org/

Changes since v1:
 * patch for printing domain HW-managed mode in the summary
 * patch that adds one consumer (venus)
 * patch for gdsc with new (different) flag
 * patch for videocc GDSC provider to update flags

Abel Vesa (1):
  PM: domains: Add the domain HW-managed mode to the summary

Jagadeesh Kona (4):
  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
  clk: qcom: videocc-sm8550: Use HW_CTRL_TRIGGER instead of HW_CTRL for
    GDSC
  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 +
 .../media/platform/qcom/venus/pm_helpers.c    | 47 +++++------
 include/linux/pm_domain.h                     | 17 ++++
 12 files changed, 165 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] 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>
---

Changes since v1:
 * reword the commit message to mention the dev_pm_genpd_get_hwmode
 * add the get_hwmode_dev callback to the generic_pm_domain
 * call the genpd provider implementation of the get_hwmode_dev from the
   generic API

 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 5cb2023581d4..dfb4f1de540d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -541,6 +541,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 f776fb93eaa0..36d308ba40b0 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -146,6 +146,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,
@@ -208,6 +212,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 +242,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 +312,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 v2 2/6] PM: domains: Add the domain HW-managed mode to the summary

[Abel Vesa]

Now that domains support being managed by the HW, lets add that
information to the genpd summary.

Suggested-by: Taniya Das <quic_tdas@quicinc.com>
Signed-off-by: Abel Vesa <abel.vesa@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 dfb4f1de540d..053b7b510825 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -3171,6 +3171,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_Mode" : "SW_Mode");
+}
+
 static void perf_status_str(struct seq_file *s, struct device *dev)
 {
 	struct generic_pm_domain_data *gpd_data;
@@ -3229,6 +3238,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);
 	}
 
@@ -3245,8 +3255,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                           mode\n");
+	seq_puts(s, "------------------------------------------------------------------------------------------------------------\n");
+
 
 	ret = mutex_lock_interruptible(&gpd_list_lock);
 	if (ret)
-- 
2.34.1

Re: [PATCH v2 2/6] PM: domains: Add the domain HW-managed mode to the summary

[Konrad Dybcio]

On 16.08.2023 16:57, Abel Vesa wrote:
> Now that domains support being managed by the HW, lets add that
> information to the genpd summary.
> 
> Suggested-by: Taniya Das <quic_tdas@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@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 dfb4f1de540d..053b7b510825 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -3171,6 +3171,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_Mode" : "SW_Mode");
That's a very personal opinion and take it for what it is, but I think
"_Mode" is excessive given the column is named "mode"

Konrad

Re: [PATCH v2 2/6] PM: domains: Add the domain HW-managed mode to the summary

[Ulf Hansson]

On Wed, 16 Aug 2023 at 16:57, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> Now that domains support being managed by the HW, lets add that
> information to the genpd summary.

To avoid confusion, would you mind re-phrasing the above to something
along the lines of:

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>

Kind regards
Uffe

> ---
>  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 dfb4f1de540d..053b7b510825 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -3171,6 +3171,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_Mode" : "SW_Mode");
> +}
> +
>  static void perf_status_str(struct seq_file *s, struct device *dev)
>  {
>         struct generic_pm_domain_data *gpd_data;
> @@ -3229,6 +3238,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);
>         }
>
> @@ -3245,8 +3255,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                           mode\n");
> +       seq_puts(s, "------------------------------------------------------------------------------------------------------------\n");
> +
>
>         ret = mutex_lock_interruptible(&gpd_list_lock);
>         if (ret)
> --
> 2.34.1
>

[PATCH v2 3/6] 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..3e4a721f1605 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_mode(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_mode(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_mode;
+		sc->pd.get_hwmode_dev = gdsc_get_mode;
+	}
 
 	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 v2 3/6] clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode

[Konrad Dybcio]

On 16.08.2023 16:57, Abel Vesa 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.
> 
> 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..3e4a721f1605 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_mode(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_mode(struct generic_pm_domain *domain, struct device *dev)
s/mode/hwmode would probably be more descriptive for returning true/false

Konrad

[PATCH v2 4/6] 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 ++--
 4 files changed, 6 insertions(+), 6 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,
 };
 
-- 
2.34.1

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Konrad Dybcio]

On 16.08.2023 16:57, 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>
> ---
Do we have any use for the HW_CTRL flag?

Perhaps it should be renamed to HW_CTRL_ALWAYS?

Or even better, *if and only if* that is necessary, add a common
property like "always_hw_managed" to the genpd code?

Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Abel Vesa]

On 23-08-16 19:56:46, Konrad Dybcio wrote:
> On 16.08.2023 16:57, 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>
> > ---
> Do we have any use for the HW_CTRL flag?
> 
> Perhaps it should be renamed to HW_CTRL_ALWAYS?
> 
> Or even better, *if and only if* that is necessary, add a common
> property like "always_hw_managed" to the genpd code?

The HW_CTRL flag is still needed for the consumers that expect the GDSC
to be have the HW control bit set right after it gets enabled.

AFAIU, for the HW_CTRL_TRIGGER, the switch to HW control will only be
done by the consumer (not on enable).

> 
> Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Konrad Dybcio]

On 23.08.2023 12:41, Abel Vesa wrote:
> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>> On 16.08.2023 16:57, 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>
>>> ---
>> Do we have any use for the HW_CTRL flag?
>>
>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>
>> Or even better, *if and only if* that is necessary, add a common
>> property like "always_hw_managed" to the genpd code?
> 
> The HW_CTRL flag is still needed for the consumers that expect the GDSC
> to be have the HW control bit set right after it gets enabled.
Guess the correct question here would be.. Are there any?

Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Jagadeesh Kona]

On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
> On 23.08.2023 12:41, Abel Vesa wrote:
>> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>>> On 16.08.2023 16:57, 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>
>>>> ---
>>> Do we have any use for the HW_CTRL flag?
>>>
>>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>>
>>> Or even better, *if and only if* that is necessary, add a common
>>> property like "always_hw_managed" to the genpd code?
>>
>> The HW_CTRL flag is still needed for the consumers that expect the GDSC
>> to be have the HW control bit set right after it gets enabled.
> Guess the correct question here would be.. Are there any?
> 

Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW 
control mode when it is enabled.

Thanks,
Jagadeesh

> Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Abel Vesa]

On 23-08-28 12:18:30, Jagadeesh Kona wrote:
> 
> 
> On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
> > On 23.08.2023 12:41, Abel Vesa wrote:
> > > On 23-08-16 19:56:46, Konrad Dybcio wrote:
> > > > On 16.08.2023 16:57, 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>
> > > > > ---
> > > > Do we have any use for the HW_CTRL flag?
> > > > 
> > > > Perhaps it should be renamed to HW_CTRL_ALWAYS?
> > > > 
> > > > Or even better, *if and only if* that is necessary, add a common
> > > > property like "always_hw_managed" to the genpd code?
> > > 
> > > The HW_CTRL flag is still needed for the consumers that expect the GDSC
> > > to be have the HW control bit set right after it gets enabled.
> > Guess the correct question here would be.. Are there any?
> > 
> 
> Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW
> control mode when it is enabled.
> 

Actually, since all the GDSCs that support HW control are by default
switched to HW mode after they are enabled, we can't make any changes
with respect to that since we risk breaking consumers. Therefore, the
new flag makes perfect sense since we can switch GDSCs from HW_CTRL to
HW_CTRL_TRIGGER per platform/consumer.


> Thanks,
> Jagadeesh
> 
> > Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Konrad Dybcio]

On 28.08.2023 09:30, Abel Vesa wrote:
> On 23-08-28 12:18:30, Jagadeesh Kona wrote:
>>
>>
>> On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
>>> On 23.08.2023 12:41, Abel Vesa wrote:
>>>> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>>>>> On 16.08.2023 16:57, 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>
>>>>>> ---
>>>>> Do we have any use for the HW_CTRL flag?
>>>>>
>>>>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>>>>
>>>>> Or even better, *if and only if* that is necessary, add a common
>>>>> property like "always_hw_managed" to the genpd code?
>>>>
>>>> The HW_CTRL flag is still needed for the consumers that expect the GDSC
>>>> to be have the HW control bit set right after it gets enabled.
>>> Guess the correct question here would be.. Are there any?
>>>
>>
>> Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW
>> control mode when it is enabled.
>>
> 
> Actually, since all the GDSCs that support HW control are by default
> switched to HW mode after they are enabled, we can't make any changes
> with respect to that since we risk breaking consumers. Therefore, the
> new flag makes perfect sense since we can switch GDSCs from HW_CTRL to
> HW_CTRL_TRIGGER per platform/consumer.
Ok, I can get behind this reasoning.

The flag name gives me a little 'eeh' feeling, but I can't
think of anything much better either..

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Konrad Dybcio]

On 28.08.2023 08:48, Jagadeesh Kona wrote:
> 
> 
> On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
>> On 23.08.2023 12:41, Abel Vesa wrote:
>>> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>>>> On 16.08.2023 16:57, 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>
>>>>> ---
>>>> Do we have any use for the HW_CTRL flag?
>>>>
>>>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>>>
>>>> Or even better, *if and only if* that is necessary, add a common
>>>> property like "always_hw_managed" to the genpd code?
>>>
>>> The HW_CTRL flag is still needed for the consumers that expect the GDSC
>>> to be have the HW control bit set right after it gets enabled.
>> Guess the correct question here would be.. Are there any?
>>
> 
> Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW control mode when it is enabled.
Oh really?

Looking at msm-5.10 techpack, only the SDE RSC driver seems to
trigger regulator fast mode (so, enabling gdsc hw_ctrl on downstream).

Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Jagadeesh Kona]

On 9/2/2023 5:33 PM, Konrad Dybcio wrote:
> On 28.08.2023 08:48, Jagadeesh Kona wrote:
>>
>>
>> On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
>>> On 23.08.2023 12:41, Abel Vesa wrote:
>>>> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>>>>> On 16.08.2023 16:57, 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>
>>>>>> ---
>>>>> Do we have any use for the HW_CTRL flag?
>>>>>
>>>>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>>>>
>>>>> Or even better, *if and only if* that is necessary, add a common
>>>>> property like "always_hw_managed" to the genpd code?
>>>>
>>>> The HW_CTRL flag is still needed for the consumers that expect the GDSC
>>>> to be have the HW control bit set right after it gets enabled.
>>> Guess the correct question here would be.. Are there any?
>>>
>>
>> Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW control mode when it is enabled.
> Oh really?
> 
> Looking at msm-5.10 techpack, only the SDE RSC driver seems to
> trigger regulator fast mode (so, enabling gdsc hw_ctrl on downstream).
> 

Yes, on downstream, display GDSC has only one consumer(SDE RSC driver) 
and there are no other consumers. SDE RSC driver switches the GDSC to hw 
control mode once GDSC is enabled and leaves it in hw control mode. Thanks!

Regards,
Jagadeesh

> Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Konrad Dybcio]

On 4.09.2023 11:27, Jagadeesh Kona wrote:
> 
> 
> On 9/2/2023 5:33 PM, Konrad Dybcio wrote:
>> On 28.08.2023 08:48, Jagadeesh Kona wrote:
>>>
>>>
>>> On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
>>>> On 23.08.2023 12:41, Abel Vesa wrote:
>>>>> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>>>>>> On 16.08.2023 16:57, 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>
>>>>>>> ---
>>>>>> Do we have any use for the HW_CTRL flag?
>>>>>>
>>>>>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>>>>>
>>>>>> Or even better, *if and only if* that is necessary, add a common
>>>>>> property like "always_hw_managed" to the genpd code?
>>>>>
>>>>> The HW_CTRL flag is still needed for the consumers that expect the GDSC
>>>>> to be have the HW control bit set right after it gets enabled.
>>>> Guess the correct question here would be.. Are there any?
>>>>
>>>
>>> Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW control mode when it is enabled.
>> Oh really?
>>
>> Looking at msm-5.10 techpack, only the SDE RSC driver seems to
>> trigger regulator fast mode (so, enabling gdsc hw_ctrl on downstream).
>>
> 
> Yes, on downstream, display GDSC has only one consumer(SDE RSC driver) and there are no other consumers. SDE RSC driver switches the GDSC to hw control mode once GDSC is enabled and leaves it in hw control mode. Thanks!
Sorry for pulling your tongue here a bit, but would it only concern
RPMh SoCs? Designs like SM6115 don't implement RSCs, should they not
have HW_CTRL enabled at all times?

Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Jagadeesh Kona]

On 9/4/2023 9:32 PM, Konrad Dybcio wrote:
> On 4.09.2023 11:27, Jagadeesh Kona wrote:
>>
>>
>> On 9/2/2023 5:33 PM, Konrad Dybcio wrote:
>>> On 28.08.2023 08:48, Jagadeesh Kona wrote:
>>>>
>>>>
>>>> On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
>>>>> On 23.08.2023 12:41, Abel Vesa wrote:
>>>>>> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>>>>>>> On 16.08.2023 16:57, 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>
>>>>>>>> ---
>>>>>>> Do we have any use for the HW_CTRL flag?
>>>>>>>
>>>>>>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>>>>>>
>>>>>>> Or even better, *if and only if* that is necessary, add a common
>>>>>>> property like "always_hw_managed" to the genpd code?
>>>>>>
>>>>>> The HW_CTRL flag is still needed for the consumers that expect the GDSC
>>>>>> to be have the HW control bit set right after it gets enabled.
>>>>> Guess the correct question here would be.. Are there any?
>>>>>
>>>>
>>>> Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW control mode when it is enabled.
>>> Oh really?
>>>
>>> Looking at msm-5.10 techpack, only the SDE RSC driver seems to
>>> trigger regulator fast mode (so, enabling gdsc hw_ctrl on downstream).
>>>
>>
>> Yes, on downstream, display GDSC has only one consumer(SDE RSC driver) and there are no other consumers. SDE RSC driver switches the GDSC to hw control mode once GDSC is enabled and leaves it in hw control mode. Thanks!
> Sorry for pulling your tongue here a bit, but would it only concern
> RPMh SoCs? Designs like SM6115 don't implement RSCs, should they not
> have HW_CTRL enabled at all times?
> 

Yes, for RPMh SoCs which have display RSC block, GDSC is switched to HW 
control mode. For SoCs which doesn't have display RSC block, display 
driver controls the GDSC in SW mode on downstream. Thanks!

Regards,
Jagadeesh

> Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Konrad Dybcio]

On 7.09.2023 07:55, Jagadeesh Kona wrote:
> 
> 
> On 9/4/2023 9:32 PM, Konrad Dybcio wrote:
>> On 4.09.2023 11:27, Jagadeesh Kona wrote:
>>>
>>>
>>> On 9/2/2023 5:33 PM, Konrad Dybcio wrote:
>>>> On 28.08.2023 08:48, Jagadeesh Kona wrote:
>>>>>
>>>>>
>>>>> On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
>>>>>> On 23.08.2023 12:41, Abel Vesa wrote:
>>>>>>> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>>>>>>>> On 16.08.2023 16:57, 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>
>>>>>>>>> ---
>>>>>>>> Do we have any use for the HW_CTRL flag?
>>>>>>>>
>>>>>>>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>>>>>>>
>>>>>>>> Or even better, *if and only if* that is necessary, add a common
>>>>>>>> property like "always_hw_managed" to the genpd code?
>>>>>>>
>>>>>>> The HW_CTRL flag is still needed for the consumers that expect the GDSC
>>>>>>> to be have the HW control bit set right after it gets enabled.
>>>>>> Guess the correct question here would be.. Are there any?
>>>>>>
>>>>>
>>>>> Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW control mode when it is enabled.
>>>> Oh really?
>>>>
>>>> Looking at msm-5.10 techpack, only the SDE RSC driver seems to
>>>> trigger regulator fast mode (so, enabling gdsc hw_ctrl on downstream).
>>>>
>>>
>>> Yes, on downstream, display GDSC has only one consumer(SDE RSC driver) and there are no other consumers. SDE RSC driver switches the GDSC to hw control mode once GDSC is enabled and leaves it in hw control mode. Thanks!
>> Sorry for pulling your tongue here a bit, but would it only concern
>> RPMh SoCs? Designs like SM6115 don't implement RSCs, should they not
>> have HW_CTRL enabled at all times?
>>
> 
> Yes, for RPMh SoCs which have display RSC block, GDSC is switched to HW control mode. For SoCs which doesn't have display RSC block, display driver controls the GDSC in SW mode on downstream. Thanks!
Thanks for explaining!

One last question, I promise.. Should we switch the MDSS GDSC to
HW_CTRL mode only after we start controlling the DISP RSC from Linux,
or should it be done regardless (because of the RPMh solving algos)?

Konrad

Re: [PATCH v2 4/6] clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode

[Jagadeesh Kona]

On 9/7/2023 1:06 PM, Konrad Dybcio wrote:
> On 7.09.2023 07:55, Jagadeesh Kona wrote:
>>
>>
>> On 9/4/2023 9:32 PM, Konrad Dybcio wrote:
>>> On 4.09.2023 11:27, Jagadeesh Kona wrote:
>>>>
>>>>
>>>> On 9/2/2023 5:33 PM, Konrad Dybcio wrote:
>>>>> On 28.08.2023 08:48, Jagadeesh Kona wrote:
>>>>>>
>>>>>>
>>>>>> On 8/26/2023 4:17 PM, Konrad Dybcio wrote:
>>>>>>> On 23.08.2023 12:41, Abel Vesa wrote:
>>>>>>>> On 23-08-16 19:56:46, Konrad Dybcio wrote:
>>>>>>>>> On 16.08.2023 16:57, 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>
>>>>>>>>>> ---
>>>>>>>>> Do we have any use for the HW_CTRL flag?
>>>>>>>>>
>>>>>>>>> Perhaps it should be renamed to HW_CTRL_ALWAYS?
>>>>>>>>>
>>>>>>>>> Or even better, *if and only if* that is necessary, add a common
>>>>>>>>> property like "always_hw_managed" to the genpd code?
>>>>>>>>
>>>>>>>> The HW_CTRL flag is still needed for the consumers that expect the GDSC
>>>>>>>> to be have the HW control bit set right after it gets enabled.
>>>>>>> Guess the correct question here would be.. Are there any?
>>>>>>>
>>>>>>
>>>>>> Yes, Display GDSC(mdss_gdsc) is required to be controlled always in HW control mode when it is enabled.
>>>>> Oh really?
>>>>>
>>>>> Looking at msm-5.10 techpack, only the SDE RSC driver seems to
>>>>> trigger regulator fast mode (so, enabling gdsc hw_ctrl on downstream).
>>>>>
>>>>
>>>> Yes, on downstream, display GDSC has only one consumer(SDE RSC driver) and there are no other consumers. SDE RSC driver switches the GDSC to hw control mode once GDSC is enabled and leaves it in hw control mode. Thanks!
>>> Sorry for pulling your tongue here a bit, but would it only concern
>>> RPMh SoCs? Designs like SM6115 don't implement RSCs, should they not
>>> have HW_CTRL enabled at all times?
>>>
>>
>> Yes, for RPMh SoCs which have display RSC block, GDSC is switched to HW control mode. For SoCs which doesn't have display RSC block, display driver controls the GDSC in SW mode on downstream. Thanks!
> Thanks for explaining!
> 
> One last question, I promise.. Should we switch the MDSS GDSC to
> HW_CTRL mode only after we start controlling the DISP RSC from Linux,
> or should it be done regardless (because of the RPMh solving algos)?
> 

 From GDSC driver, MDSS GDSC can be switched to HW_CTRL mode regardless. 
Thanks!

Regards,
Jagadeesh

> Konrad

[PATCH v2 5/6] clk: qcom: videocc-sm8550: Use HW_CTRL_TRIGGER instead of HW_CTRL for GDSC

[Abel Vesa]

From: Jagadeesh Kona <quic_jkona@quicinc.com>

HW_CTRL moves the GDSC to HW control mode as part of GDSC enable itself.
Use HW_CTRL_TRIGGER flag instead of HW_CTRL flag for video GDSC's to
switch the GDSC to HW/SW control modes only when consumer requested to
switch GDSC mode using dev_pm_genpd_set_hwmode.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 drivers/clk/qcom/videocc-sm8550.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
index 3bb136ec31b1..504b2ef264eb 100644
--- a/drivers/clk/qcom/videocc-sm8550.c
+++ b/drivers/clk/qcom/videocc-sm8550.c
@@ -324,7 +324,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 = {
@@ -349,7 +349,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 v2 5/6] clk: qcom: videocc-sm8550: Use HW_CTRL_TRIGGER instead of HW_CTRL for GDSC

[Konrad Dybcio]

On 16.08.2023 16:57, Abel Vesa wrote:
> From: Jagadeesh Kona <quic_jkona@quicinc.com>
> 
> HW_CTRL moves the GDSC to HW control mode as part of GDSC enable itself.
> Use HW_CTRL_TRIGGER flag instead of HW_CTRL flag for video GDSC's to
> switch the GDSC to HW/SW control modes only when consumer requested to
> switch GDSC mode using dev_pm_genpd_set_hwmode.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
Any reason for it to be a separate patch?

I'd say either keep all changes separate (for easier bisecting) or keep
them all in a single commit.

Konrad

[PATCH v2 6/6] 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.

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 +
 .../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 48c9084bb4db..c53eef23c793 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 v2 6/6] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode

[Konrad Dybcio]

On 16.08.2023 16:57, 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.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
[...]

>  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);
This removal cries for better explanation.

Has the venus hw been setting some registers that alter the GDSC's state?
Or the hardware's expectations of the GDSC state?

Konrad

Re: [PATCH v2 6/6] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode

[Konrad Dybcio]

On 16.08.2023 19:58, Konrad Dybcio wrote:
> On 16.08.2023 16:57, 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.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>> ---
> [...]
> 
>>  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);
> This removal cries for better explanation.
> 
> Has the venus hw been setting some registers that alter the GDSC's state?
> Or the hardware's expectations of the GDSC state?
Clearly can't read the commit message

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v2 0/6] PM: domains: Add control for switching back and forth to HW control

[Rafael J. Wysocki]

On Wed, Aug 16, 2023 at 4:57 PM Abel Vesa <abel.vesa@linaro.org> wrote:
>
> The v1 (and the back story) can be found here:
> https://lore.kernel.org/all/20230628105652.1670316-1-abel.vesa@linaro.org/
>
> Changes since v1:
>  * patch for printing domain HW-managed mode in the summary
>  * patch that adds one consumer (venus)
>  * patch for gdsc with new (different) flag
>  * patch for videocc GDSC provider to update flags
>
> Abel Vesa (1):
>   PM: domains: Add the domain HW-managed mode to the summary
>
> Jagadeesh Kona (4):
>   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
>   clk: qcom: videocc-sm8550: Use HW_CTRL_TRIGGER instead of HW_CTRL for
>     GDSC
>   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 +
>  .../media/platform/qcom/venus/pm_helpers.c    | 47 +++++------
>  include/linux/pm_domain.h                     | 17 ++++
>  12 files changed, 165 insertions(+), 37 deletions(-)
>
> --

The second patch requires an ACK from Ulf anyway and with his ACK the
whole series can be routed via arm-soc as far as I'm concerned.

I have nothing to add here.

Thanks!