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

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

[Abel Vesa]

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v4:
- Re-worded 1st patch commit message, as per Bjorn's suggestion, and added
  Dmitry's R-b tag
- Added Bjorn's and Dmitry's R-b tags to the 2nd patch
- Re-worded 3rd patch commit message, to better explain the HW_CTRL_TRIGGER flag.
- Added mode transition delay when setting mode for GDSC
- Added status polling if GDSSC is enabled when transitioning from HW to SW
- Re-worded 4th patch commit message to better explain why the
  HW_CTRL_TRIGGER needs to be used instead
- Drop changes to SC7180, SDM845 and SM8550 video CC drivers, as only
  SC7280 and SM8250 have been tested so far. More platforms (with v6 venus)
  will be added eventually.
- Call genpd set_hwmode API only for v6 and dropped the vcodec_pmdomains_hwctrl.
- Re-worded 5th patch commit message accordingly. 
- Link to v3:
  https://lore.kernel.org/r/20231101-gdsc-hwctrl-v3-0-0740ae6b2b04@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/clk/qcom/gdsc.c                        | 54 +++++++++++++++++
 drivers/clk/qcom/gdsc.h                        |  1 +
 drivers/clk/qcom/videocc-sc7280.c              |  2 +-
 drivers/clk/qcom/videocc-sm8250.c              |  4 +-
 drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++----
 drivers/pmdomain/core.c                        | 83 +++++++++++++++++++++++++-
 include/linux/pm_domain.h                      | 17 ++++++
 7 files changed, 169 insertions(+), 15 deletions(-)
---
base-commit: 319fbd8fc6d339e0a1c7b067eed870c518a13a02
change-id: 20231101-gdsc-hwctrl-13f01ea60cbd

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

[PATCH v4 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 consumer driver should be able to change the behavior of
power domain at runtime, control the power domain in SW mode for certain
configurations and handover the control to HW mode for other usecases.

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>
---
 drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h | 17 ++++++++++++
 2 files changed, 86 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index a1f6cba3ae6c..41b6411d0ef5 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -548,6 +548,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 b97c5e9820f9..5a26423a7ee1 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -148,6 +148,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,
@@ -210,6 +214,7 @@ struct generic_pm_domain_data {
 	unsigned int performance_state;
 	unsigned int default_pstate;
 	unsigned int rpm_pstate;
+	bool hw_mode;
 	void *data;
 };
 
@@ -239,6 +244,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;
@@ -307,6 +314,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 v4 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>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Bjorn Andersson <andersson@kernel.org>
---
 drivers/pmdomain/core.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 41b6411d0ef5..841fde9e9f87 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -3147,6 +3147,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;
@@ -3205,6 +3214,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);
 	}
 
@@ -3221,8 +3231,8 @@ 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 v4 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.

But consumer drivers also require the GDSC mode to be switched dynamically
at runtime based on requirement for certain usecases. Some of these
usecases are switching the GDSC to SW mode to keep it ON during the
enablement of clocks that are dependent on GDSC and while programming
certain configurations that require GDSC to be ON. Introduce a new
HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev
callbacks which allows the consumer drivers to switch the GDSC back and
forth between HW/SW modes dynamically at runtime using new
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/gdsc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/gdsc.h |  1 +
 2 files changed, 55 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 5358e28122ab..71626eb20101 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -363,6 +363,56 @@ 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);
+	u32 val;
+	int ret;
+
+	if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) {
+		pr_err("Cannot set mode while parent is disabled\n");
+		return -EIO;
+	}
+
+	ret = gdsc_hwctrl(sc, mode);
+	if (ret)
+		return ret;
+
+	/* Wait for 1usec for mode transition to properly complete */
+	udelay(1);
+
+	if (!mode) {
+		ret = regmap_read(sc->regmap, sc->gdscr, &val);
+		if (ret)
+			return ret;
+
+		/*
+		 * While switching from HW to SW mode, if GDSC is in enabled
+		 * state, poll for GDSC to complete the power up.
+		 */
+		if (!(val & SW_COLLAPSE_MASK))
+			return gdsc_poll_status(sc, GDSC_ON);
+	}
+
+	return 0;
+}
+
+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 +501,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 v4 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, and HW can disable the GDSC right after the
GDSC is moved to HW mode. This can lead to subsequent video clock enable
failures that are dependent on the GDSC if GDSC is collapsed by HW. Also,
while few video hardware registers are being programmed by TZ, GDSC
should be in non collapsed state which can be achieved by keeping
it in SW mode.

Use HW_CTRL_TRIGGER flag instead which allows consumer drivers to switch
the video GDSC to HW/SW control modes at runtime using
dev_pm_genpd_set_hwmode API as per the requirement.

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

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

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

[Abel Vesa]

From: Jagadeesh Kona <quic_jkona@quicinc.com>

Use dev_pm_genpd_set_hwmode API to switch the vcodec gdsc to SW/HW
modes at runtime based on requirement for venus V6 variants.

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 in
vcodec_control_v4() can be dropped for V6 variants.

With newer implementation, the mode of vcodec gdsc gets switched only in
set_hwmode API and the GDSC should not be switched to HW control mode
before turning off the GDSC, else subsequent GDSC enable may fail, hence
add check to avoid switching the GDSC to HW mode before powering off the
GDSC on V6 variants.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index a1b127caa90a..55e8ec3f4ee9 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -412,10 +412,9 @@ static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
 	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) {
+	if (IS_V6(core))
+		return dev_pm_genpd_set_hwmode(core->pmdomains[coreid], !enable);
+	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 {
@@ -451,9 +450,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
 
 		vcodec_clks_disable(core, core->vcodec0_clks);
 
-		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
-		if (ret)
-			return ret;
+		if (!IS_V6(core)) {
+			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
+			if (ret)
+				return ret;
+		}
 
 		ret = pm_runtime_put_sync(core->pmdomains[1]);
 		if (ret < 0)
@@ -467,9 +468,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
 
 		vcodec_clks_disable(core, core->vcodec1_clks);
 
-		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
-		if (ret)
-			return ret;
+		if (!IS_V6(core)) {
+			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
+			if (ret)
+				return ret;
+		}
 
 		ret = pm_runtime_put_sync(core->pmdomains[2]);
 		if (ret < 0)

-- 
2.34.1

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

[Ulf Hansson]

[...]

> +
> +/**
> + * 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);

Not sure why I haven't spotted this before - but we should probably
assign dev_gpd_data(dev)->hw_mode here, rather than returning the
result from the callback directly.

> +
> +       return dev_gpd_data(dev)->hw_mode;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_hwmode);

[...]

Kind regards
Uffe

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

[Ulf Hansson]

On Mon, 22 Jan 2024 at 09:47, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v4:
> - Re-worded 1st patch commit message, as per Bjorn's suggestion, and added
>   Dmitry's R-b tag
> - Added Bjorn's and Dmitry's R-b tags to the 2nd patch
> - Re-worded 3rd patch commit message, to better explain the HW_CTRL_TRIGGER flag.
> - Added mode transition delay when setting mode for GDSC
> - Added status polling if GDSSC is enabled when transitioning from HW to SW
> - Re-worded 4th patch commit message to better explain why the
>   HW_CTRL_TRIGGER needs to be used instead
> - Drop changes to SC7180, SDM845 and SM8550 video CC drivers, as only
>   SC7280 and SM8250 have been tested so far. More platforms (with v6 venus)
>   will be added eventually.
> - Call genpd set_hwmode API only for v6 and dropped the vcodec_pmdomains_hwctrl.
> - Re-worded 5th patch commit message accordingly.
> - Link to v3:
>   https://lore.kernel.org/r/20231101-gdsc-hwctrl-v3-0-0740ae6b2b04@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/clk/qcom/gdsc.c                        | 54 +++++++++++++++++
>  drivers/clk/qcom/gdsc.h                        |  1 +
>  drivers/clk/qcom/videocc-sc7280.c              |  2 +-
>  drivers/clk/qcom/videocc-sm8250.c              |  4 +-
>  drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++----
>  drivers/pmdomain/core.c                        | 83 +++++++++++++++++++++++++-
>  include/linux/pm_domain.h                      | 17 ++++++
>  7 files changed, 169 insertions(+), 15 deletions(-)
> ---

Bjorn, if it helps, I can funnel this complete series via my pmdomain tree?

Another option is that I host an immutable branch with patch1 and
patch2 for you to pull in? Just let me know what you prefer.

Kind regards
Uffe

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

[Bjorn Andersson]

On Mon, Jan 22, 2024 at 10:47:03AM +0200, 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.
> 

This provides insight into why we end up with both HW_CTRL and
HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but
rather just an implementation detail.

> But consumer drivers also require the GDSC mode to be switched dynamically
> at runtime based on requirement for certain usecases. Some of these
> usecases are switching the GDSC to SW mode to keep it ON during the
> enablement of clocks that are dependent on GDSC and while programming
> certain configurations that require GDSC to be ON. Introduce a new
> HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev
> callbacks which allows the consumer drivers to switch the GDSC back and
> forth between HW/SW modes dynamically at runtime using new
> dev_pm_genpd_set_hwmode API.
> 

This still expresses the need for HW_CTRL_TRIGGER in terms of "some
drivers need for some use case". We don't need these many words to say:
"Introduce HW_CTRL_TRIGGER for client drivers that need it."


I find that it would be useful to document that every time a GDSC is
turned on the mode will be switched to SW...

> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/qcom/gdsc.h |  1 +
>  2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 5358e28122ab..71626eb20101 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -363,6 +363,56 @@ 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);
> +	u32 val;
> +	int ret;
> +
> +	if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) {

Why is this a restriction only for GDSCs supplied by regulators? I don't
find anything preventing this API from being called on GDSCs supplied by
other genpd instances.

Also note that regulator_is_enabled() is racy, in that it tells us if
the regulator is currently turned on, not if we're the one holding that
vote. As such this might change at any moment - and hence shouldn't be
significant here.

> +		pr_err("Cannot set mode while parent is disabled\n");
> +		return -EIO;
> +	}
> +
> +	ret = gdsc_hwctrl(sc, mode);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for 1usec for mode transition to properly complete */
> +	udelay(1);
> +
> +	if (!mode) {
> +		ret = regmap_read(sc->regmap, sc->gdscr, &val);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * While switching from HW to SW mode, if GDSC is in enabled
> +		 * state, poll for GDSC to complete the power up.
> +		 */

I had to give this some thought, to conclude that this is relevant if HW
has the GDSC disabled and we're switching to SW - which would then
enable it. I think this comment can be improved slightly, to save the
reader the need for figuring out this on their own.

> +		if (!(val & SW_COLLAPSE_MASK))

This not being true, would imply that gdsc_disable() has been called
already, in which case there's no guarantee that the parent still
supplies power.

In the introduced API power on and hw control are orthogonal states, but
not so in this implementation. This need to made clear, to reduce future
surprises.

> +			return gdsc_poll_status(sc, GDSC_ON);
> +	}
> +
> +	return 0;
> +}
> +
> +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;

return !!(val & HW_CONTROL_MASK);

Regards,
Bjorn

> +}
> +
>  static int gdsc_init(struct gdsc *sc)
>  {
>  	u32 mask, val;
> @@ -451,6 +501,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: Re: [PATCH v4 3/5] clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode

[Bjorn Andersson]

On Tue, Jan 30, 2024 at 05:00:28PM -0600, Bjorn Andersson wrote:
> On Mon, Jan 22, 2024 at 10:47:03AM +0200, 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.
> > 
> 
> This provides insight into why we end up with both HW_CTRL and
> HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but
> rather just an implementation detail.
> 
> > But consumer drivers also require the GDSC mode to be switched dynamically
> > at runtime based on requirement for certain usecases. Some of these
> > usecases are switching the GDSC to SW mode to keep it ON during the
> > enablement of clocks that are dependent on GDSC and while programming
> > certain configurations that require GDSC to be ON. Introduce a new
> > HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev
> > callbacks which allows the consumer drivers to switch the GDSC back and
> > forth between HW/SW modes dynamically at runtime using new
> > dev_pm_genpd_set_hwmode API.
> > 
> 
> This still expresses the need for HW_CTRL_TRIGGER in terms of "some
> drivers need for some use case". We don't need these many words to say:
> "Introduce HW_CTRL_TRIGGER for client drivers that need it."
> 
> 
> I find that it would be useful to document that every time a GDSC is
> turned on the mode will be switched to SW...
> 

I believe I'm wrong here. Reading the patch again, I think we might
retain the mode across a disable/enable cycle. I at least don't see
anything explicit returning us to SW mode.

According to Linux though, the GDSC is off, so as described below, there
will be no votes for supplying resources.

Regards,
Bjorn

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

[Bjorn Andersson]

On Mon, Jan 22, 2024 at 10:47:05AM +0200, Abel Vesa wrote:
> From: Jagadeesh Kona <quic_jkona@quicinc.com>
> 
> Use dev_pm_genpd_set_hwmode API to switch the vcodec gdsc to SW/HW
> modes at runtime based on requirement for venus V6 variants.
> 
> 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 in
> vcodec_control_v4() can be dropped for V6 variants.
> 

The purpose of this commit is to warrant the need of this new mechanism,
but I don't find that it actually describes a problem to be solved.

> With newer implementation, the mode of vcodec gdsc gets switched only in

Does "With newer implementation" mean "after these patches are applied"?

> set_hwmode API and the GDSC should not be switched to HW control mode
> before turning off the GDSC, else subsequent GDSC enable may fail, hence
> add check to avoid switching the GDSC to HW mode before powering off the
> GDSC on V6 variants.
> 

Is this saying that "if we return the GDSC to HW control after turning
off the clocks, it might not be possible to turn it on again"?

How come? Today this GDSC is operating in HW control mode, before,
during and after the clock operation.

> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index a1b127caa90a..55e8ec3f4ee9 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -412,10 +412,9 @@ static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
>  	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) {
> +	if (IS_V6(core))
> +		return dev_pm_genpd_set_hwmode(core->pmdomains[coreid], !enable);
> +	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 {
> @@ -451,9 +450,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>  
>  		vcodec_clks_disable(core, core->vcodec0_clks);
>  
> -		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
> -		if (ret)
> -			return ret;
> +		if (!IS_V6(core)) {
> +			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);

First I had this expectation that the GDSC will always be in SW control
when the GDSC turns on - like the downstream implementation.

In this case I felt we should have a similar condition in
poweron_coreid() - as there's no point in switching to SW mode when we
know we're in SW mode already.


But as I finally realized that this is not the case, I now see that by
skipping the transition to HW mode here, dev_pm_genpd_set_hwmode() will
find the domain in SW mode, and through

  if (dev_gpd_data(dev)->hw_mode == enable)

Will turn the vcodec_control_v4(, true) into a nop.

So, my first first instinct of feeling that this should be symmetric
between poweron/poweroff was reasonable...I think...


I find that this interface does not match the expectations that people
will bring from downstream and this example isn't helpful in explaining
how to use the new interface.

PS. I trust there's no case whre legacy_binding = true, or that that
code path does not need similar workaround?

Regards,
Bjorn

> +			if (ret)
> +				return ret;
> +		}
>  
>  		ret = pm_runtime_put_sync(core->pmdomains[1]);
>  		if (ret < 0)
> @@ -467,9 +468,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>  
>  		vcodec_clks_disable(core, core->vcodec1_clks);
>  
> -		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
> -		if (ret)
> -			return ret;
> +		if (!IS_V6(core)) {
> +			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
> +			if (ret)
> +				return ret;
> +		}
>  
>  		ret = pm_runtime_put_sync(core->pmdomains[2]);
>  		if (ret < 0)
> 
> -- 
> 2.34.1
> 

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

[Bjorn Andersson]

On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
> power domain at runtime, control the power domain in SW mode for certain
> configurations and handover the control to HW mode for other usecases.
> 
> 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>
> ---
>  drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h | 17 ++++++++++++
>  2 files changed, 86 insertions(+)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index a1f6cba3ae6c..41b6411d0ef5 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -548,6 +548,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.

This isn't proper kernel-doc

> + *
> + * @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)

Between this and the gdsc patch, the hw_mode state might not match the
hardware state at boot.

With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
false) will not bring control to SW - which might be fatal.

Regards,
Bjorn

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

[Ulf Hansson]

On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
> > power domain at runtime, control the power domain in SW mode for certain
> > configurations and handover the control to HW mode for other usecases.
> >
> > 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>
> > ---
> >  drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_domain.h | 17 ++++++++++++
> >  2 files changed, 86 insertions(+)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index a1f6cba3ae6c..41b6411d0ef5 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -548,6 +548,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.
>
> This isn't proper kernel-doc

Sorry, I didn't quite get that. What is wrong?

>
> > + *
> > + * @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)
>
> Between this and the gdsc patch, the hw_mode state might not match the
> hardware state at boot.
>
> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
> false) will not bring control to SW - which might be fatal.

Right, good point.

I think we have two ways to deal with this:
1) If the provider is supporting ->get_hwmode_dev(), we can let
genpd_add_device() invoke it to synchronize the state.
2) If the provider doesn't support ->get_hwmode_dev() we need to call
->set_hwmode_dev() to allow an initial state to be set.

The question is then, if we need to allow ->get_hwmode_dev() to be
optional, if the ->set_hwmode_dev() is supported - or if we can
require it. What's your thoughts around this?

Kind regards
Uffe

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

[Bjorn Andersson]

On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
> > > power domain at runtime, control the power domain in SW mode for certain
> > > configurations and handover the control to HW mode for other usecases.
> > >
> > > 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>
> > > ---
> > >  drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/pm_domain.h | 17 ++++++++++++
> > >  2 files changed, 86 insertions(+)
> > >
> > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > > index a1f6cba3ae6c..41b6411d0ef5 100644
> > > --- a/drivers/pmdomain/core.c
> > > +++ b/drivers/pmdomain/core.c
> > > @@ -548,6 +548,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.
> >
> > This isn't proper kernel-doc
> 
> Sorry, I didn't quite get that. What is wrong?
> 

https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
says that there should be () after the function name, and below there
should be a Return:

> >
> > > + *
> > > + * @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)
> >
> > Between this and the gdsc patch, the hw_mode state might not match the
> > hardware state at boot.
> >
> > With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
> > false) will not bring control to SW - which might be fatal.
> 
> Right, good point.
> 
> I think we have two ways to deal with this:
> 1) If the provider is supporting ->get_hwmode_dev(), we can let
> genpd_add_device() invoke it to synchronize the state.

I'd suggest that we skip the optimization for now and just let the
update hit the driver on each call.

> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
> ->set_hwmode_dev() to allow an initial state to be set.
> 
> The question is then, if we need to allow ->get_hwmode_dev() to be
> optional, if the ->set_hwmode_dev() is supported - or if we can
> require it. What's your thoughts around this?
> 

Iiuc this resource can be shared between multiple clients, and we're
in either case returning the shared state. That would mean a client
acting upon the returned value, is subject to races.

I'm therefore inclined to say that we shouldn't have a getter, other
than for debugging purposes, in which case reading the HW-state or
failing would be reasonable outcomes.

Regards,
Bjorn

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

[Ulf Hansson]

On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> > On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
> > >
> > > On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
> > > > power domain at runtime, control the power domain in SW mode for certain
> > > > configurations and handover the control to HW mode for other usecases.
> > > >
> > > > 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>
> > > > ---
> > > >  drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/pm_domain.h | 17 ++++++++++++
> > > >  2 files changed, 86 insertions(+)
> > > >
> > > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > > > index a1f6cba3ae6c..41b6411d0ef5 100644
> > > > --- a/drivers/pmdomain/core.c
> > > > +++ b/drivers/pmdomain/core.c
> > > > @@ -548,6 +548,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.
> > >
> > > This isn't proper kernel-doc
> >
> > Sorry, I didn't quite get that. What is wrong?
> >
>
> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
> says that there should be () after the function name, and below there
> should be a Return:

Thanks for the pointers!

>
> > >
> > > > + *
> > > > + * @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)
> > >
> > > Between this and the gdsc patch, the hw_mode state might not match the
> > > hardware state at boot.
> > >
> > > With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
> > > false) will not bring control to SW - which might be fatal.
> >
> > Right, good point.
> >
> > I think we have two ways to deal with this:
> > 1) If the provider is supporting ->get_hwmode_dev(), we can let
> > genpd_add_device() invoke it to synchronize the state.
>
> I'd suggest that we skip the optimization for now and just let the
> update hit the driver on each call.

Okay.

>
> > 2) If the provider doesn't support ->get_hwmode_dev() we need to call
> > ->set_hwmode_dev() to allow an initial state to be set.
> >
> > The question is then, if we need to allow ->get_hwmode_dev() to be
> > optional, if the ->set_hwmode_dev() is supported - or if we can
> > require it. What's your thoughts around this?
> >
>
> Iiuc this resource can be shared between multiple clients, and we're
> in either case returning the shared state. That would mean a client
> acting upon the returned value, is subject to races.

Not sure I understand this, but I also don't have in-depth knowledge
of how the HW works.

Isn't the HW mode set on a per device basis?

>
> I'm therefore inclined to say that we shouldn't have a getter, other
> than for debugging purposes, in which case reading the HW-state or
> failing would be reasonable outcomes.

If you only want this for debug purposes, it seems better to keep it
closer to the rpmh code, rather than adding generic callbacks to the
genpd interface.

So to conclude, you think having a ->set_hwmode_dev() callback should
be sufficient and no caching of the current state?

Abel, what's your thoughts around this?

Kind regards
Uffe

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

[Jagadeesh Kona]

On 1/31/2024 4:30 AM, Bjorn Andersson wrote:
> On Mon, Jan 22, 2024 at 10:47:03AM +0200, 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.
>>
> 
> This provides insight into why we end up with both HW_CTRL and
> HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but
> rather just an implementation detail.
> 
>> But consumer drivers also require the GDSC mode to be switched dynamically
>> at runtime based on requirement for certain usecases. Some of these
>> usecases are switching the GDSC to SW mode to keep it ON during the
>> enablement of clocks that are dependent on GDSC and while programming
>> certain configurations that require GDSC to be ON. Introduce a new
>> HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev
>> callbacks which allows the consumer drivers to switch the GDSC back and
>> forth between HW/SW modes dynamically at runtime using new
>> dev_pm_genpd_set_hwmode API.
>>
> 
> This still expresses the need for HW_CTRL_TRIGGER in terms of "some
> drivers need for some use case". We don't need these many words to say:
> "Introduce HW_CTRL_TRIGGER for client drivers that need it."
> 

Thanks Bjorn for your review.

Sure will update the commit text to be more precise in next series.

> 
> I find that it would be useful to document that every time a GDSC is
> turned on the mode will be switched to SW...
> 
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>> ---
>>   drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/clk/qcom/gdsc.h |  1 +
>>   2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index 5358e28122ab..71626eb20101 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -363,6 +363,56 @@ 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);
>> +	u32 val;
>> +	int ret;
>> +
>> +	if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) {
> 
> Why is this a restriction only for GDSCs supplied by regulators? I don't
> find anything preventing this API from being called on GDSCs supplied by
> other genpd instances.

> 
> Also note that regulator_is_enabled() is racy, in that it tells us if
> the regulator is currently turned on, not if we're the one holding that
> vote. As such this might change at any moment - and hence shouldn't be
> significant here.
>
Below is the consumer's sequence that switch the GDSC's b/w HW & SW modes:-
1) Enable the GDSC in SW mode
2) Enable required clocks
3) Switch the GDSC to HW mode using dev_pm_genpd_set_hwmode(true)
4) Usecase start
5) Usecase end
6) Switch the GDSC back to SW mode using dev_pm_genpd_set_hwmode(false)
7) Disable clocks
8) Disable GDSC

Hence the new API dev_pm_genpd_set_hwmode() will always be called 
between gdsc_enable() and gdsc_disable(), which ensures GDSC's parent 
power domain/regulator is ON when this callback is being called. Also, 
we can remove the above regulator_is_enabled() check as well.

>> +		pr_err("Cannot set mode while parent is disabled\n");
>> +		return -EIO;
>> +	}
>> +
>> +	ret = gdsc_hwctrl(sc, mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for 1usec for mode transition to properly complete */
>> +	udelay(1);
>> +
>> +	if (!mode) {
>> +		ret = regmap_read(sc->regmap, sc->gdscr, &val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/*
>> +		 * While switching from HW to SW mode, if GDSC is in enabled
>> +		 * state, poll for GDSC to complete the power up.
>> +		 */
> 
> I had to give this some thought, to conclude that this is relevant if HW
> has the GDSC disabled and we're switching to SW - which would then
> enable it. I think this comment can be improved slightly, to save the
> reader the need for figuring out this on their own.
> 

Sure, I will improvise the comment in next series.

>> +		if (!(val & SW_COLLAPSE_MASK))
> 
> This not being true, would imply that gdsc_disable() has been called
> already, in which case there's no guarantee that the parent still
> supplies power.
> 
> In the introduced API power on and hw control are orthogonal states, but
> not so in this implementation. This need to made clear, to reduce future
> surprises.
> 

Yes, above SW_COLLAPSE_MASK check is also not required and will remove 
it in next series.

>> +			return gdsc_poll_status(sc, GDSC_ON);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +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;
> 
> return !!(val & HW_CONTROL_MASK);
> 

Sure, will update this in the next series.

> Regards,
> Bjorn
> 
>> +}
>> +
>>   static int gdsc_init(struct gdsc *sc)
>>   {
>>   	u32 mask, val;
>> @@ -451,6 +501,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 v4 5/5] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode

[Jagadeesh Kona]

On 1/31/2024 6:35 AM, Bjorn Andersson wrote:
> On Mon, Jan 22, 2024 at 10:47:05AM +0200, Abel Vesa wrote:
>> From: Jagadeesh Kona <quic_jkona@quicinc.com>
>>
>> Use dev_pm_genpd_set_hwmode API to switch the vcodec gdsc to SW/HW
>> modes at runtime based on requirement for venus V6 variants.
>>
>> 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 in
>> vcodec_control_v4() can be dropped for V6 variants.
>>
> 
> The purpose of this commit is to warrant the need of this new mechanism,
> but I don't find that it actually describes a problem to be solved.
> 
>> With newer implementation, the mode of vcodec gdsc gets switched only in
> 
> Does "With newer implementation" mean "after these patches are applied"?
> 

Thanks Bjorn for your review!

Yes, after all these patches are applied, will update the commit text to 
be bit more precise.

>> set_hwmode API and the GDSC should not be switched to HW control mode
>> before turning off the GDSC, else subsequent GDSC enable may fail, hence
>> add check to avoid switching the GDSC to HW mode before powering off the
>> GDSC on V6 variants.
>>
> 
> Is this saying that "if we return the GDSC to HW control after turning
> off the clocks, it might not be possible to turn it on again"?
> 

Yes, if the GDSC is left in HW control mode before GDSC disable, the 
subsequent GDSC enable callback may fail while polling for GDSC status, 
since HW can keep the GDSC in disabled state.


> How come? Today this GDSC is operating in HW control mode, before,
> during and after the clock operation.
> 

Currently once GDSC is moved to HW control mode, Venus driver is using 
it's POWER_CONTROL register to keep the GDSC ON before the clock 
operations and reset it back after clock operations to handover control 
back to HW.

And these venus POWER_CONTROL register addresses are not constant and 
vary from one venus variant to other. With this new API Venus driver can 
avoid these register writes and use this standard API everywhere to 
switch the GDSC mode as required.

>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index a1b127caa90a..55e8ec3f4ee9 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -412,10 +412,9 @@ static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
>>   	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) {
>> +	if (IS_V6(core))
>> +		return dev_pm_genpd_set_hwmode(core->pmdomains[coreid], !enable);
>> +	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 {
>> @@ -451,9 +450,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>>   
>>   		vcodec_clks_disable(core, core->vcodec0_clks);
>>   
>> -		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
>> -		if (ret)
>> -			return ret;
>> +		if (!IS_V6(core)) {
>> +			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
> 
> First I had this expectation that the GDSC will always be in SW control
> when the GDSC turns on - like the downstream implementation.
> 
> In this case I felt we should have a similar condition in
> poweron_coreid() - as there's no point in switching to SW mode when we
> know we're in SW mode already.
> 
> 
> But as I finally realized that this is not the case, I now see that by
> skipping the transition to HW mode here, dev_pm_genpd_set_hwmode() will
> find the domain in SW mode, and through
> 
>    if (dev_gpd_data(dev)->hw_mode == enable)
> 
> Will turn the vcodec_control_v4(, true) into a nop.
> 
> So, my first first instinct of feeling that this should be symmetric
> between poweron/poweroff was reasonable...I think...
> 

Yes, we can add similar check in poweron_coreid() also to be symmetric 
but since it will be nop haven't added it. Shall I add similar check in 
poweron_coreid() as well?

> 
> I find that this interface does not match the expectations that people
> will bring from downstream and this example isn't helpful in explaining
> how to use the new interface.
> 

There are 3 consumers that currently use this HW control mode for 
GDSC's:- display, camera and display.

Display driver is able to operate with GDSC always in HW mode. Camera 
drivers don't have power saving features enabled on upstream yet and 
hence not using the HW control mode of GDSC's currently, but will need 
this API support to enable camera power saving features on upstream.

Currently on upstream, only venus driver requires GDSC HW and SW modes 
switching, and hence added support in this driver to use the new interface.


> PS. I trust there's no case whre legacy_binding = true, or that that
> code path does not need similar workaround?
> 
This change is applicable only to sc7280 and sm8250 targets for which 
legacy_binding will be false.

Thanks,
Jagadeesh

> Regards,
> Bjorn
> 
>> +			if (ret)
>> +				return ret;
>> +		}
>>   
>>   		ret = pm_runtime_put_sync(core->pmdomains[1]);
>>   		if (ret < 0)
>> @@ -467,9 +468,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>>   
>>   		vcodec_clks_disable(core, core->vcodec1_clks);
>>   
>> -		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
>> -		if (ret)
>> -			return ret;
>> +		if (!IS_V6(core)) {
>> +			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
>> +			if (ret)
>> +				return ret;
>> +		}
>>   
>>   		ret = pm_runtime_put_sync(core->pmdomains[2]);
>>   		if (ret < 0)
>>
>> -- 
>> 2.34.1
>>

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

[Jagadeesh Kona]

On 1/31/2024 5:49 AM, Bjorn Andersson wrote:
> On Tue, Jan 30, 2024 at 05:00:28PM -0600, Bjorn Andersson wrote:
>> On Mon, Jan 22, 2024 at 10:47:03AM +0200, 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.
>>>
>>
>> This provides insight into why we end up with both HW_CTRL and
>> HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but
>> rather just an implementation detail.
>>
>>> But consumer drivers also require the GDSC mode to be switched dynamically
>>> at runtime based on requirement for certain usecases. Some of these
>>> usecases are switching the GDSC to SW mode to keep it ON during the
>>> enablement of clocks that are dependent on GDSC and while programming
>>> certain configurations that require GDSC to be ON. Introduce a new
>>> HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev
>>> callbacks which allows the consumer drivers to switch the GDSC back and
>>> forth between HW/SW modes dynamically at runtime using new
>>> dev_pm_genpd_set_hwmode API.
>>>
>>
>> This still expresses the need for HW_CTRL_TRIGGER in terms of "some
>> drivers need for some use case". We don't need these many words to say:
>> "Introduce HW_CTRL_TRIGGER for client drivers that need it."
>>
>>
>> I find that it would be useful to document that every time a GDSC is
>> turned on the mode will be switched to SW...
>>
> 
> I believe I'm wrong here. Reading the patch again, I think we might
> retain the mode across a disable/enable cycle. I at least don't see
> anything explicit returning us to SW mode.
> > According to Linux though, the GDSC is off, so as described below, there
> will be no votes for supplying resources.
> 

Yes, With HW_CTRL_TRIGGER flag, the GDSC mode gets switched only in new 
set_hwmode_dev() callback when consumers explicitly call for it. This is 
to ensure GenPD f/w's hwmode aligns with the actual GDSC mode in HW.

With the new API, the expectation is consumer drivers should move the 
GDSC to SW control mode before disabling the GDSC, which ensures that, 
GDSC is turned ON in SW mode every time.

Thanks,
Jagadeesh

> Regards,
> Bjorn

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

[Jagadeesh Kona]

On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote:
>>
>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
>>>>
>>>> On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
>>>>> power domain at runtime, control the power domain in SW mode for certain
>>>>> configurations and handover the control to HW mode for other usecases.
>>>>>
>>>>> 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>
>>>>> ---
>>>>>   drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   include/linux/pm_domain.h | 17 ++++++++++++
>>>>>   2 files changed, 86 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644
>>>>> --- a/drivers/pmdomain/core.c
>>>>> +++ b/drivers/pmdomain/core.c
>>>>> @@ -548,6 +548,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.
>>>>
>>>> This isn't proper kernel-doc
>>>
>>> Sorry, I didn't quite get that. What is wrong?
>>>
>>
>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>> says that there should be () after the function name, and below there
>> should be a Return:
> 
> Thanks for the pointers!
> 
>>
>>>>
>>>>> + *
>>>>> + * @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)
>>>>
>>>> Between this and the gdsc patch, the hw_mode state might not match the
>>>> hardware state at boot.
>>>>
>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
>>>> false) will not bring control to SW - which might be fatal.
>>>
>>> Right, good point.
>>>
>>> I think we have two ways to deal with this:
>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let
>>> genpd_add_device() invoke it to synchronize the state.
>>
>> I'd suggest that we skip the optimization for now and just let the
>> update hit the driver on each call.
> 
> Okay.
> 
>>
>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
>>> ->set_hwmode_dev() to allow an initial state to be set.
>>>
>>> The question is then, if we need to allow ->get_hwmode_dev() to be
>>> optional, if the ->set_hwmode_dev() is supported - or if we can
>>> require it. What's your thoughts around this?
>>>
>>
>> Iiuc this resource can be shared between multiple clients, and we're
>> in either case returning the shared state. That would mean a client
>> acting upon the returned value, is subject to races.
> 
> Not sure I understand this, but I also don't have in-depth knowledge
> of how the HW works.
> 
> Isn't the HW mode set on a per device basis?
> 
>>
>> I'm therefore inclined to say that we shouldn't have a getter, other
>> than for debugging purposes, in which case reading the HW-state or
>> failing would be reasonable outcomes.
> 
> If you only want this for debug purposes, it seems better to keep it
> closer to the rpmh code, rather than adding generic callbacks to the
> genpd interface.
> 
> So to conclude, you think having a ->set_hwmode_dev() callback should
> be sufficient and no caching of the current state?
> 
> Abel, what's your thoughts around this?
> 

We believe it is good to have get_hwmode_dev() callback supported from 
GenPD, since if multiple devices share a GenPD, and if one device moves 
the GenPD to HW mode, the other device won't be aware of it and second 
device's dev_gpd_data(dev)->hw_mode will still be false.

If we have this dev_pm_genpd_get_hwmode() API supported and if we assign 
dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev() 
callback, consumer drivers can use this API to sync the actual HW mode 
of the GenPD.

Thanks,
Jagadeesh

> Kind regards
> Uffe

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

[Ulf Hansson]

On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>
>
>
> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> > On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote:
> >>
> >> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> >>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
> >>>>
> >>>> On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
> >>>>> power domain at runtime, control the power domain in SW mode for certain
> >>>>> configurations and handover the control to HW mode for other usecases.
> >>>>>
> >>>>> 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>
> >>>>> ---
> >>>>>   drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>   include/linux/pm_domain.h | 17 ++++++++++++
> >>>>>   2 files changed, 86 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> >>>>> index a1f6cba3ae6c..41b6411d0ef5 100644
> >>>>> --- a/drivers/pmdomain/core.c
> >>>>> +++ b/drivers/pmdomain/core.c
> >>>>> @@ -548,6 +548,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.
> >>>>
> >>>> This isn't proper kernel-doc
> >>>
> >>> Sorry, I didn't quite get that. What is wrong?
> >>>
> >>
> >> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
> >> says that there should be () after the function name, and below there
> >> should be a Return:
> >
> > Thanks for the pointers!
> >
> >>
> >>>>
> >>>>> + *
> >>>>> + * @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)
> >>>>
> >>>> Between this and the gdsc patch, the hw_mode state might not match the
> >>>> hardware state at boot.
> >>>>
> >>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
> >>>> false) will not bring control to SW - which might be fatal.
> >>>
> >>> Right, good point.
> >>>
> >>> I think we have two ways to deal with this:
> >>> 1) If the provider is supporting ->get_hwmode_dev(), we can let
> >>> genpd_add_device() invoke it to synchronize the state.
> >>
> >> I'd suggest that we skip the optimization for now and just let the
> >> update hit the driver on each call.
> >
> > Okay.
> >
> >>
> >>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
> >>> ->set_hwmode_dev() to allow an initial state to be set.
> >>>
> >>> The question is then, if we need to allow ->get_hwmode_dev() to be
> >>> optional, if the ->set_hwmode_dev() is supported - or if we can
> >>> require it. What's your thoughts around this?
> >>>
> >>
> >> Iiuc this resource can be shared between multiple clients, and we're
> >> in either case returning the shared state. That would mean a client
> >> acting upon the returned value, is subject to races.
> >
> > Not sure I understand this, but I also don't have in-depth knowledge
> > of how the HW works.
> >
> > Isn't the HW mode set on a per device basis?
> >
> >>
> >> I'm therefore inclined to say that we shouldn't have a getter, other
> >> than for debugging purposes, in which case reading the HW-state or
> >> failing would be reasonable outcomes.
> >
> > If you only want this for debug purposes, it seems better to keep it
> > closer to the rpmh code, rather than adding generic callbacks to the
> > genpd interface.
> >
> > So to conclude, you think having a ->set_hwmode_dev() callback should
> > be sufficient and no caching of the current state?
> >
> > Abel, what's your thoughts around this?
> >
>
> We believe it is good to have get_hwmode_dev() callback supported from
> GenPD, since if multiple devices share a GenPD, and if one device moves
> the GenPD to HW mode, the other device won't be aware of it and second
> device's dev_gpd_data(dev)->hw_mode will still be false.
>
> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign
> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev()
> callback, consumer drivers can use this API to sync the actual HW mode
> of the GenPD.

Hmm, I thought the HW mode was being set on a per device basis, via
its PM domain. Did I get that wrong?

Are you saying there could be multiple devices sharing the same PM
domain and thus also sharing the same HW mode? In that case, it sure
sounds like we have synchronization issues to deal with too.

Kind regards
Uffe

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

[Jagadeesh Kona]

On 2/13/2024 7:21 PM, Ulf Hansson wrote:
> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>>
>>
>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote:
>>>>
>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
>>>>>>
>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
>>>>>>> power domain at runtime, control the power domain in SW mode for certain
>>>>>>> configurations and handover the control to HW mode for other usecases.
>>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>>    drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>    include/linux/pm_domain.h | 17 ++++++++++++
>>>>>>>    2 files changed, 86 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644
>>>>>>> --- a/drivers/pmdomain/core.c
>>>>>>> +++ b/drivers/pmdomain/core.c
>>>>>>> @@ -548,6 +548,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.
>>>>>>
>>>>>> This isn't proper kernel-doc
>>>>>
>>>>> Sorry, I didn't quite get that. What is wrong?
>>>>>
>>>>
>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>>>> says that there should be () after the function name, and below there
>>>> should be a Return:
>>>
>>> Thanks for the pointers!
>>>
>>>>
>>>>>>
>>>>>>> + *
>>>>>>> + * @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)
>>>>>>
>>>>>> Between this and the gdsc patch, the hw_mode state might not match the
>>>>>> hardware state at boot.
>>>>>>
>>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
>>>>>> false) will not bring control to SW - which might be fatal.
>>>>>
>>>>> Right, good point.
>>>>>
>>>>> I think we have two ways to deal with this:
>>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let
>>>>> genpd_add_device() invoke it to synchronize the state.
>>>>
>>>> I'd suggest that we skip the optimization for now and just let the
>>>> update hit the driver on each call.
>>>
>>> Okay.
>>>
>>>>
>>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
>>>>> ->set_hwmode_dev() to allow an initial state to be set.
>>>>>
>>>>> The question is then, if we need to allow ->get_hwmode_dev() to be
>>>>> optional, if the ->set_hwmode_dev() is supported - or if we can
>>>>> require it. What's your thoughts around this?
>>>>>
>>>>
>>>> Iiuc this resource can be shared between multiple clients, and we're
>>>> in either case returning the shared state. That would mean a client
>>>> acting upon the returned value, is subject to races.
>>>
>>> Not sure I understand this, but I also don't have in-depth knowledge
>>> of how the HW works.
>>>
>>> Isn't the HW mode set on a per device basis?
>>>
>>>>
>>>> I'm therefore inclined to say that we shouldn't have a getter, other
>>>> than for debugging purposes, in which case reading the HW-state or
>>>> failing would be reasonable outcomes.
>>>
>>> If you only want this for debug purposes, it seems better to keep it
>>> closer to the rpmh code, rather than adding generic callbacks to the
>>> genpd interface.
>>>
>>> So to conclude, you think having a ->set_hwmode_dev() callback should
>>> be sufficient and no caching of the current state?
>>>
>>> Abel, what's your thoughts around this?
>>>
>>
>> We believe it is good to have get_hwmode_dev() callback supported from
>> GenPD, since if multiple devices share a GenPD, and if one device moves
>> the GenPD to HW mode, the other device won't be aware of it and second
>> device's dev_gpd_data(dev)->hw_mode will still be false.
>>
>> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign
>> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev()
>> callback, consumer drivers can use this API to sync the actual HW mode
>> of the GenPD.
> 
> Hmm, I thought the HW mode was being set on a per device basis, via
> its PM domain. Did I get that wrong?
> 
> Are you saying there could be multiple devices sharing the same PM
> domain and thus also sharing the same HW mode? In that case, it sure
> sounds like we have synchronization issues to deal with too.
> 

Sorry my bad, currently we don't have usecase where multiple devices 
sharing the same PM domain that have HW control support, so there is no 
synchronization issue.

But it would be good to have .get_hwmode_dev() callback for consumer 
drivers to query the actual GenPD mode from HW, whenever they require it.

Thanks,
Jagadeesh

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

[Ulf Hansson]

On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>
>
>
> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
> > On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> >>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote:
> >>>>
> >>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> >>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
> >>>>>>
> >>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
> >>>>>>> power domain at runtime, control the power domain in SW mode for certain
> >>>>>>> configurations and handover the control to HW mode for other usecases.
> >>>>>>>
> >>>>>>> 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>
> >>>>>>> ---
> >>>>>>>    drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>    include/linux/pm_domain.h | 17 ++++++++++++
> >>>>>>>    2 files changed, 86 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> >>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644
> >>>>>>> --- a/drivers/pmdomain/core.c
> >>>>>>> +++ b/drivers/pmdomain/core.c
> >>>>>>> @@ -548,6 +548,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.
> >>>>>>
> >>>>>> This isn't proper kernel-doc
> >>>>>
> >>>>> Sorry, I didn't quite get that. What is wrong?
> >>>>>
> >>>>
> >>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
> >>>> says that there should be () after the function name, and below there
> >>>> should be a Return:
> >>>
> >>> Thanks for the pointers!
> >>>
> >>>>
> >>>>>>
> >>>>>>> + *
> >>>>>>> + * @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)
> >>>>>>
> >>>>>> Between this and the gdsc patch, the hw_mode state might not match the
> >>>>>> hardware state at boot.
> >>>>>>
> >>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
> >>>>>> false) will not bring control to SW - which might be fatal.
> >>>>>
> >>>>> Right, good point.
> >>>>>
> >>>>> I think we have two ways to deal with this:
> >>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let
> >>>>> genpd_add_device() invoke it to synchronize the state.
> >>>>
> >>>> I'd suggest that we skip the optimization for now and just let the
> >>>> update hit the driver on each call.
> >>>
> >>> Okay.
> >>>
> >>>>
> >>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
> >>>>> ->set_hwmode_dev() to allow an initial state to be set.
> >>>>>
> >>>>> The question is then, if we need to allow ->get_hwmode_dev() to be
> >>>>> optional, if the ->set_hwmode_dev() is supported - or if we can
> >>>>> require it. What's your thoughts around this?
> >>>>>
> >>>>
> >>>> Iiuc this resource can be shared between multiple clients, and we're
> >>>> in either case returning the shared state. That would mean a client
> >>>> acting upon the returned value, is subject to races.
> >>>
> >>> Not sure I understand this, but I also don't have in-depth knowledge
> >>> of how the HW works.
> >>>
> >>> Isn't the HW mode set on a per device basis?
> >>>
> >>>>
> >>>> I'm therefore inclined to say that we shouldn't have a getter, other
> >>>> than for debugging purposes, in which case reading the HW-state or
> >>>> failing would be reasonable outcomes.
> >>>
> >>> If you only want this for debug purposes, it seems better to keep it
> >>> closer to the rpmh code, rather than adding generic callbacks to the
> >>> genpd interface.
> >>>
> >>> So to conclude, you think having a ->set_hwmode_dev() callback should
> >>> be sufficient and no caching of the current state?
> >>>
> >>> Abel, what's your thoughts around this?
> >>>
> >>
> >> We believe it is good to have get_hwmode_dev() callback supported from
> >> GenPD, since if multiple devices share a GenPD, and if one device moves
> >> the GenPD to HW mode, the other device won't be aware of it and second
> >> device's dev_gpd_data(dev)->hw_mode will still be false.
> >>
> >> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign
> >> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev()
> >> callback, consumer drivers can use this API to sync the actual HW mode
> >> of the GenPD.
> >
> > Hmm, I thought the HW mode was being set on a per device basis, via
> > its PM domain. Did I get that wrong?
> >
> > Are you saying there could be multiple devices sharing the same PM
> > domain and thus also sharing the same HW mode? In that case, it sure
> > sounds like we have synchronization issues to deal with too.
> >
>
> Sorry my bad, currently we don't have usecase where multiple devices
> sharing the same PM domain that have HW control support, so there is no
> synchronization issue.

Okay, good!

>
> But it would be good to have .get_hwmode_dev() callback for consumer
> drivers to query the actual GenPD mode from HW, whenever they require it.

Okay, no objection from my side.

Then the final question is if we need a variable to keep a cache of
the current HW mode for each device. Perhaps we should start simple
and just always invoke the callbacks from genpd, what do you think?

Kind regards
Uffe

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

[Jagadeesh Kona]

On 2/15/2024 9:57 PM, Ulf Hansson wrote:
> On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>>
>>
>> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
>>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
>>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote:
>>>>>>
>>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
>>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
>>>>>>>>
>>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
>>>>>>>>> power domain at runtime, control the power domain in SW mode for certain
>>>>>>>>> configurations and handover the control to HW mode for other usecases.
>>>>>>>>>
>>>>>>>>> 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>
>>>>>>>>> ---
>>>>>>>>>     drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>     include/linux/pm_domain.h | 17 ++++++++++++
>>>>>>>>>     2 files changed, 86 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>>>>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644
>>>>>>>>> --- a/drivers/pmdomain/core.c
>>>>>>>>> +++ b/drivers/pmdomain/core.c
>>>>>>>>> @@ -548,6 +548,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.
>>>>>>>>
>>>>>>>> This isn't proper kernel-doc
>>>>>>>
>>>>>>> Sorry, I didn't quite get that. What is wrong?
>>>>>>>
>>>>>>
>>>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>>>>>> says that there should be () after the function name, and below there
>>>>>> should be a Return:
>>>>>
>>>>> Thanks for the pointers!
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>> + *
>>>>>>>>> + * @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)
>>>>>>>>
>>>>>>>> Between this and the gdsc patch, the hw_mode state might not match the
>>>>>>>> hardware state at boot.
>>>>>>>>
>>>>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
>>>>>>>> false) will not bring control to SW - which might be fatal.
>>>>>>>
>>>>>>> Right, good point.
>>>>>>>
>>>>>>> I think we have two ways to deal with this:
>>>>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let
>>>>>>> genpd_add_device() invoke it to synchronize the state.
>>>>>>
>>>>>> I'd suggest that we skip the optimization for now and just let the
>>>>>> update hit the driver on each call.
>>>>>
>>>>> Okay.
>>>>>
>>>>>>
>>>>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
>>>>>>> ->set_hwmode_dev() to allow an initial state to be set.
>>>>>>>
>>>>>>> The question is then, if we need to allow ->get_hwmode_dev() to be
>>>>>>> optional, if the ->set_hwmode_dev() is supported - or if we can
>>>>>>> require it. What's your thoughts around this?
>>>>>>>
>>>>>>
>>>>>> Iiuc this resource can be shared between multiple clients, and we're
>>>>>> in either case returning the shared state. That would mean a client
>>>>>> acting upon the returned value, is subject to races.
>>>>>
>>>>> Not sure I understand this, but I also don't have in-depth knowledge
>>>>> of how the HW works.
>>>>>
>>>>> Isn't the HW mode set on a per device basis?
>>>>>
>>>>>>
>>>>>> I'm therefore inclined to say that we shouldn't have a getter, other
>>>>>> than for debugging purposes, in which case reading the HW-state or
>>>>>> failing would be reasonable outcomes.
>>>>>
>>>>> If you only want this for debug purposes, it seems better to keep it
>>>>> closer to the rpmh code, rather than adding generic callbacks to the
>>>>> genpd interface.
>>>>>
>>>>> So to conclude, you think having a ->set_hwmode_dev() callback should
>>>>> be sufficient and no caching of the current state?
>>>>>
>>>>> Abel, what's your thoughts around this?
>>>>>
>>>>
>>>> We believe it is good to have get_hwmode_dev() callback supported from
>>>> GenPD, since if multiple devices share a GenPD, and if one device moves
>>>> the GenPD to HW mode, the other device won't be aware of it and second
>>>> device's dev_gpd_data(dev)->hw_mode will still be false.
>>>>
>>>> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign
>>>> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev()
>>>> callback, consumer drivers can use this API to sync the actual HW mode
>>>> of the GenPD.
>>>
>>> Hmm, I thought the HW mode was being set on a per device basis, via
>>> its PM domain. Did I get that wrong?
>>>
>>> Are you saying there could be multiple devices sharing the same PM
>>> domain and thus also sharing the same HW mode? In that case, it sure
>>> sounds like we have synchronization issues to deal with too.
>>>
>>
>> Sorry my bad, currently we don't have usecase where multiple devices
>> sharing the same PM domain that have HW control support, so there is no
>> synchronization issue.
> 
> Okay, good!
> 
>>
>> But it would be good to have .get_hwmode_dev() callback for consumer
>> drivers to query the actual GenPD mode from HW, whenever they require it.
> 
> Okay, no objection from my side.
> 
> Then the final question is if we need a variable to keep a cache of
> the current HW mode for each device. Perhaps we should start simple
> and just always invoke the callbacks from genpd, what do you think?
> 

Yes, agree, we can remove the variable and just always invoke the 
callbacks from genpd. But we may need the variable to reflect GenPD
mode in debugfs genpd_summary, or need to invoke get callback there as 
well to get the current mode.

Thanks,
Jagadeesh

> Kind regards
> Uffe

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

[Ulf Hansson]

On Fri, 16 Feb 2024 at 09:01, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>
>
>
> On 2/15/2024 9:57 PM, Ulf Hansson wrote:
> > On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
> >>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> >>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote:
> >>>>>>
> >>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> >>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
> >>>>>>>>
> >>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
> >>>>>>>>> power domain at runtime, control the power domain in SW mode for certain
> >>>>>>>>> configurations and handover the control to HW mode for other usecases.
> >>>>>>>>>
> >>>>>>>>> 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>
> >>>>>>>>> ---
> >>>>>>>>>     drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>     include/linux/pm_domain.h | 17 ++++++++++++
> >>>>>>>>>     2 files changed, 86 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> >>>>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644
> >>>>>>>>> --- a/drivers/pmdomain/core.c
> >>>>>>>>> +++ b/drivers/pmdomain/core.c
> >>>>>>>>> @@ -548,6 +548,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.
> >>>>>>>>
> >>>>>>>> This isn't proper kernel-doc
> >>>>>>>
> >>>>>>> Sorry, I didn't quite get that. What is wrong?
> >>>>>>>
> >>>>>>
> >>>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
> >>>>>> says that there should be () after the function name, and below there
> >>>>>> should be a Return:
> >>>>>
> >>>>> Thanks for the pointers!
> >>>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>>> + *
> >>>>>>>>> + * @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)
> >>>>>>>>
> >>>>>>>> Between this and the gdsc patch, the hw_mode state might not match the
> >>>>>>>> hardware state at boot.
> >>>>>>>>
> >>>>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
> >>>>>>>> false) will not bring control to SW - which might be fatal.
> >>>>>>>
> >>>>>>> Right, good point.
> >>>>>>>
> >>>>>>> I think we have two ways to deal with this:
> >>>>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let
> >>>>>>> genpd_add_device() invoke it to synchronize the state.
> >>>>>>
> >>>>>> I'd suggest that we skip the optimization for now and just let the
> >>>>>> update hit the driver on each call.
> >>>>>
> >>>>> Okay.
> >>>>>
> >>>>>>
> >>>>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
> >>>>>>> ->set_hwmode_dev() to allow an initial state to be set.
> >>>>>>>
> >>>>>>> The question is then, if we need to allow ->get_hwmode_dev() to be
> >>>>>>> optional, if the ->set_hwmode_dev() is supported - or if we can
> >>>>>>> require it. What's your thoughts around this?
> >>>>>>>
> >>>>>>
> >>>>>> Iiuc this resource can be shared between multiple clients, and we're
> >>>>>> in either case returning the shared state. That would mean a client
> >>>>>> acting upon the returned value, is subject to races.
> >>>>>
> >>>>> Not sure I understand this, but I also don't have in-depth knowledge
> >>>>> of how the HW works.
> >>>>>
> >>>>> Isn't the HW mode set on a per device basis?
> >>>>>
> >>>>>>
> >>>>>> I'm therefore inclined to say that we shouldn't have a getter, other
> >>>>>> than for debugging purposes, in which case reading the HW-state or
> >>>>>> failing would be reasonable outcomes.
> >>>>>
> >>>>> If you only want this for debug purposes, it seems better to keep it
> >>>>> closer to the rpmh code, rather than adding generic callbacks to the
> >>>>> genpd interface.
> >>>>>
> >>>>> So to conclude, you think having a ->set_hwmode_dev() callback should
> >>>>> be sufficient and no caching of the current state?
> >>>>>
> >>>>> Abel, what's your thoughts around this?
> >>>>>
> >>>>
> >>>> We believe it is good to have get_hwmode_dev() callback supported from
> >>>> GenPD, since if multiple devices share a GenPD, and if one device moves
> >>>> the GenPD to HW mode, the other device won't be aware of it and second
> >>>> device's dev_gpd_data(dev)->hw_mode will still be false.
> >>>>
> >>>> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign
> >>>> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev()
> >>>> callback, consumer drivers can use this API to sync the actual HW mode
> >>>> of the GenPD.
> >>>
> >>> Hmm, I thought the HW mode was being set on a per device basis, via
> >>> its PM domain. Did I get that wrong?
> >>>
> >>> Are you saying there could be multiple devices sharing the same PM
> >>> domain and thus also sharing the same HW mode? In that case, it sure
> >>> sounds like we have synchronization issues to deal with too.
> >>>
> >>
> >> Sorry my bad, currently we don't have usecase where multiple devices
> >> sharing the same PM domain that have HW control support, so there is no
> >> synchronization issue.
> >
> > Okay, good!
> >
> >>
> >> But it would be good to have .get_hwmode_dev() callback for consumer
> >> drivers to query the actual GenPD mode from HW, whenever they require it.
> >
> > Okay, no objection from my side.
> >
> > Then the final question is if we need a variable to keep a cache of
> > the current HW mode for each device. Perhaps we should start simple
> > and just always invoke the callbacks from genpd, what do you think?
> >
>
> Yes, agree, we can remove the variable and just always invoke the
> callbacks from genpd. But we may need the variable to reflect GenPD
> mode in debugfs genpd_summary, or need to invoke get callback there as
> well to get the current mode.

Hmm, after some more thinking I believe it may be best to keep the
variable after all. For reasons you point out above.

However, we need a way to synchronize the initial HW mode state for a
device. Therefore I suggest we invoke the ->get_hwmode_dev() callback
from genpd_add_device() and store its return value in the variable.
Later the variable can be used for debugfs and returned from
dev_pm_genpd_get_hwmode() too.

That should work, right?

Kind regards
Uffe

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

[Jagadeesh Kona]

On 2/28/2024 8:23 PM, Ulf Hansson wrote:
> On Fri, 16 Feb 2024 at 09:01, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>>
>>
>> On 2/15/2024 9:57 PM, Ulf Hansson wrote:
>>> On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
>>>>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
>>>>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote:
>>>>>>>>
>>>>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
>>>>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +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 consumer driver should be able to change the behavior of
>>>>>>>>>>> power domain at runtime, control the power domain in SW mode for certain
>>>>>>>>>>> configurations and handover the control to HW mode for other usecases.
>>>>>>>>>>>
>>>>>>>>>>> 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>
>>>>>>>>>>> ---
>>>>>>>>>>>      drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>      include/linux/pm_domain.h | 17 ++++++++++++
>>>>>>>>>>>      2 files changed, 86 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>>>>>>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644
>>>>>>>>>>> --- a/drivers/pmdomain/core.c
>>>>>>>>>>> +++ b/drivers/pmdomain/core.c
>>>>>>>>>>> @@ -548,6 +548,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.
>>>>>>>>>>
>>>>>>>>>> This isn't proper kernel-doc
>>>>>>>>>
>>>>>>>>> Sorry, I didn't quite get that. What is wrong?
>>>>>>>>>
>>>>>>>>
>>>>>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>>>>>>>> says that there should be () after the function name, and below there
>>>>>>>> should be a Return:
>>>>>>>
>>>>>>> Thanks for the pointers!
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> + *
>>>>>>>>>>> + * @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)
>>>>>>>>>>
>>>>>>>>>> Between this and the gdsc patch, the hw_mode state might not match the
>>>>>>>>>> hardware state at boot.
>>>>>>>>>>
>>>>>>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
>>>>>>>>>> false) will not bring control to SW - which might be fatal.
>>>>>>>>>
>>>>>>>>> Right, good point.
>>>>>>>>>
>>>>>>>>> I think we have two ways to deal with this:
>>>>>>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let
>>>>>>>>> genpd_add_device() invoke it to synchronize the state.
>>>>>>>>
>>>>>>>> I'd suggest that we skip the optimization for now and just let the
>>>>>>>> update hit the driver on each call.
>>>>>>>
>>>>>>> Okay.
>>>>>>>
>>>>>>>>
>>>>>>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
>>>>>>>>> ->set_hwmode_dev() to allow an initial state to be set.
>>>>>>>>>
>>>>>>>>> The question is then, if we need to allow ->get_hwmode_dev() to be
>>>>>>>>> optional, if the ->set_hwmode_dev() is supported - or if we can
>>>>>>>>> require it. What's your thoughts around this?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Iiuc this resource can be shared between multiple clients, and we're
>>>>>>>> in either case returning the shared state. That would mean a client
>>>>>>>> acting upon the returned value, is subject to races.
>>>>>>>
>>>>>>> Not sure I understand this, but I also don't have in-depth knowledge
>>>>>>> of how the HW works.
>>>>>>>
>>>>>>> Isn't the HW mode set on a per device basis?
>>>>>>>
>>>>>>>>
>>>>>>>> I'm therefore inclined to say that we shouldn't have a getter, other
>>>>>>>> than for debugging purposes, in which case reading the HW-state or
>>>>>>>> failing would be reasonable outcomes.
>>>>>>>
>>>>>>> If you only want this for debug purposes, it seems better to keep it
>>>>>>> closer to the rpmh code, rather than adding generic callbacks to the
>>>>>>> genpd interface.
>>>>>>>
>>>>>>> So to conclude, you think having a ->set_hwmode_dev() callback should
>>>>>>> be sufficient and no caching of the current state?
>>>>>>>
>>>>>>> Abel, what's your thoughts around this?
>>>>>>>
>>>>>>
>>>>>> We believe it is good to have get_hwmode_dev() callback supported from
>>>>>> GenPD, since if multiple devices share a GenPD, and if one device moves
>>>>>> the GenPD to HW mode, the other device won't be aware of it and second
>>>>>> device's dev_gpd_data(dev)->hw_mode will still be false.
>>>>>>
>>>>>> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign
>>>>>> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev()
>>>>>> callback, consumer drivers can use this API to sync the actual HW mode
>>>>>> of the GenPD.
>>>>>
>>>>> Hmm, I thought the HW mode was being set on a per device basis, via
>>>>> its PM domain. Did I get that wrong?
>>>>>
>>>>> Are you saying there could be multiple devices sharing the same PM
>>>>> domain and thus also sharing the same HW mode? In that case, it sure
>>>>> sounds like we have synchronization issues to deal with too.
>>>>>
>>>>
>>>> Sorry my bad, currently we don't have usecase where multiple devices
>>>> sharing the same PM domain that have HW control support, so there is no
>>>> synchronization issue.
>>>
>>> Okay, good!
>>>
>>>>
>>>> But it would be good to have .get_hwmode_dev() callback for consumer
>>>> drivers to query the actual GenPD mode from HW, whenever they require it.
>>>
>>> Okay, no objection from my side.
>>>
>>> Then the final question is if we need a variable to keep a cache of
>>> the current HW mode for each device. Perhaps we should start simple
>>> and just always invoke the callbacks from genpd, what do you think?
>>>
>>
>> Yes, agree, we can remove the variable and just always invoke the
>> callbacks from genpd. But we may need the variable to reflect GenPD
>> mode in debugfs genpd_summary, or need to invoke get callback there as
>> well to get the current mode.
> 
> Hmm, after some more thinking I believe it may be best to keep the
> variable after all. For reasons you point out above.
> 
> However, we need a way to synchronize the initial HW mode state for a
> device. Therefore I suggest we invoke the ->get_hwmode_dev() callback
> from genpd_add_device() and store its return value in the variable.
> Later the variable can be used for debugfs and returned from
> dev_pm_genpd_get_hwmode() too.
> 
> That should work, right?
> 

Yes, it should work.

Thanks,
Jagadeesh

> Kind regards
> Uffe