[PATCH v2 0/3] pmdomain: ti_sci: collect and send low-power mode constraints

[PATCH v2 0/3] pmdomain: ti_sci: collect and send low-power mode constraints

[Kevin Hilman]

The latest (10.x) version of the firmware for the PM co-processor (aka
device manager, or DM) adds support for a "managed" mode, where the DM
firmware will select the specific low power state which is entered
when Linux requests a system-wide suspend.

In this mode, the DM will always attempt the deepest low-power state
available for the SoC.

However, Linux (or OSes running on other cores) may want to constrain
the DM for certain use cases.  For example, the deepest state may have
a wakeup/resume latency that is too long for certain use cases.  Or,
some wakeup-capable devices may potentially be powered off in deep
low-power states, but if one of those devices is enabled as a wakeup
source, it should not be powered off.

These kinds of constraints are are already known in Linux by the use
of existing APIs such as per-device PM QoS and device wakeup APIs, but
now we need to communicate these constraints to the DM.

For TI SoCs with TI SCI support, all DM-managed devices will be
connected to a TI SCI PM domain.  So the goal of this series is to use
the PM domain driver for TI SCI devices to collect constraints, and
communicate them to the DM via the new TI SCI APIs.

This is all managed by TI SCI PM domain code.  No new APIs are needed
by Linux drivers.  Any device that is managed by TI SCI will be
checked for QoS constraints or wakeup capability and the constraints
will be collected and sent to the DM.

This series depends on the support for the new TI SCI APIs (v10) and
was also tested with this series to update 8250_omap serial support
for AM62x[2].

[1] https://lore.kernel.org/all/20240801195422.2296347-1-msp@baylibre.com
[2] https://lore.kernel.org/all/20240807141227.1093006-1-msp@baylibre.com/

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
Changes in v2:

- To simplify this version a bit, drop the pmdomain ->power_off()
  changes.  Constraints only sent during ->suspend() path.  The pmdomain
  path was an optimization that may be added back later.
- With the above simplification, drop the extra state variables that
  had been added to keep track of constraint status.
- Link to v1: https://lore.kernel.org/r/20240805-lpm-v6-10-constraints-pmdomain-v1-0-d186b68ded4c@baylibre.com

---
Kevin Hilman (3):
      pmdomain: ti_sci: add per-device latency constraint management
      pmdomain: ti_sci: add wakeup constraint management
      pmdomain: ti_sci: handle wake IRQs for IO daisy chain wakeups

 drivers/pmdomain/ti/ti_sci_pm_domains.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)
---
base-commit: ad7eb1b6b92ee0c959a0a6ae846ddadd7a79ea64
change-id: 20240802-lpm-v6-10-constraints-pmdomain-f33df5aef449

Best regards,
-- 
Kevin Hilman <khilman@baylibre.com>

[PATCH v2 1/3] pmdomain: ti_sci: add per-device latency constraint management

[Kevin Hilman]

For each device in a TI SCI PM domain, check whether the device has
any resume latency constraints set via per-device PM QoS.  If
constraints are set, send them to DM via the new SCI constraints API.

Checking for constraints happen for each device before system-wide
suspend (via ->suspend() hook.)

An important detail here is that the PM domain driver inserts itself
into the path of both the ->suspend() and ->resume() hook path
of *all* devices in the PM domain.  This allows generic PM domain code
to handle the constraint management and communication with TI SCI.

Further, this allows device drivers to use existing PM QoS APIs to
add/update constraints.

DM firmware clears constraints during its resume, so Linux has
to check/update/send constraints each time system suspends.

Co-developed-by: Vibhore Vardhan <vibhore@ti.com>
Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
Signed-off-by: Kevin Hilman <khilman@baylibre.com>
Signed-off-by: Dhruva Gole <d-gole@ti.com>
---
 drivers/pmdomain/ti/ti_sci_pm_domains.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
index 1510d5ddae3d..963272fa387b 100644
--- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
+++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
@@ -13,6 +13,8 @@
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/slab.h>
+#include <linux/pm_qos.h>
+#include <linux/pm_runtime.h>
 #include <linux/soc/ti/ti_sci_protocol.h>
 #include <dt-bindings/soc/ti,sci_pm_domain.h>
 
@@ -51,6 +53,29 @@ struct ti_sci_pm_domain {
 
 #define genpd_to_ti_sci_pd(gpd) container_of(gpd, struct ti_sci_pm_domain, pd)
 
+static inline bool ti_sci_pd_is_valid_constraint(s32 val)
+{
+	return val != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+}
+
+static int ti_sci_pd_set_lat_constraint(struct device *dev, s32 val)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+	struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd);
+	const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
+	int ret;
+
+	ret = ti_sci->ops.pm_ops.set_latency_constraint(ti_sci, val, TISCI_MSG_CONSTRAINT_SET);
+	if (ret)
+		dev_err(dev, "ti_sci_pd: set latency constraint failed: ret=%d\n",
+			ret);
+	else
+		dev_dbg(dev, "ti_sci_pd: ID:%d set latency constraint %d\n",
+			pd->idx, val);
+
+	return ret;
+}
+
 /*
  * ti_sci_pd_power_off(): genpd power down hook
  * @domain: pointer to the powerdomain to power off
@@ -79,6 +104,22 @@ static int ti_sci_pd_power_on(struct generic_pm_domain *domain)
 		return ti_sci->ops.dev_ops.get_device(ti_sci, pd->idx);
 }
 
+static int ti_sci_pd_suspend(struct device *dev)
+{
+	int ret;
+	s32 val;
+
+	ret = pm_generic_suspend(dev);
+	if (ret)
+		return ret;
+
+	val = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
+	if (ti_sci_pd_is_valid_constraint(val))
+		ti_sci_pd_set_lat_constraint(dev, val);
+
+	return 0;
+}
+
 /*
  * ti_sci_pd_xlate(): translation service for TI SCI genpds
  * @genpdspec: DT identification data for the genpd
@@ -188,6 +229,13 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev)
 				pd->pd.power_on = ti_sci_pd_power_on;
 				pd->idx = args.args[0];
 				pd->parent = pd_provider;
+				/*
+				 * If SCI constraint functions are present, then firmware
+				 * supports the constraints API.
+				 */
+				if (pd_provider->ti_sci->ops.pm_ops.set_device_constraint &&
+				    pd_provider->ti_sci->ops.pm_ops.set_latency_constraint)
+					pd->pd.domain.ops.suspend = ti_sci_pd_suspend;
 
 				pm_genpd_init(&pd->pd, NULL, true);
 

-- 
2.46.0

[PATCH v2 2/3] pmdomain: ti_sci: add wakeup constraint management

[Kevin Hilman]

During system-wide suspend, check all devices connected to PM domain
to see if they are wakeup-enabled.  If so, set a TI SCI device
constraint.

Note: DM firmware clears all constraints on resume.

Co-developed-by: Vibhore Vardhan <vibhore@ti.com>
Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
Signed-off-by: Kevin Hilman <khilman@baylibre.com>
Signed-off-by: Dhruva Gole <d-gole@ti.com>
---
 drivers/pmdomain/ti/ti_sci_pm_domains.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
index 963272fa387b..8af907579152 100644
--- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
+++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
@@ -76,6 +76,23 @@ static int ti_sci_pd_set_lat_constraint(struct device *dev, s32 val)
 	return ret;
 }
 
+static inline bool ti_sci_pd_set_wkup_constraint(struct device *dev)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+	struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd);
+	const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
+	int ret;
+
+	if (device_may_wakeup(dev)) {
+		ret = ti_sci->ops.pm_ops.set_device_constraint(ti_sci, pd->idx,
+							       TISCI_MSG_CONSTRAINT_SET);
+		if (!ret)
+			dev_dbg(dev, "ti_sci_pd: ID:%d set device constraint.\n", pd->idx);
+	}
+
+	return true;
+}
+
 /*
  * ti_sci_pd_power_off(): genpd power down hook
  * @domain: pointer to the powerdomain to power off
@@ -117,6 +134,8 @@ static int ti_sci_pd_suspend(struct device *dev)
 	if (ti_sci_pd_is_valid_constraint(val))
 		ti_sci_pd_set_lat_constraint(dev, val);
 
+	ti_sci_pd_set_wkup_constraint(dev);
+
 	return 0;
 }
 

-- 
2.46.0

[PATCH v2 3/3] pmdomain: ti_sci: handle wake IRQs for IO daisy chain wakeups

[Kevin Hilman]

When a device supports IO daisy-chain wakeups, it uses a dedicated
wake IRQ.  Devices with IO daisy-chain wakeups enabled should not set
wakeup constraints since these can happen even from deep power states,
so should not prevent the DM from picking deep power states.

Wake IRQs are set with dev_pm_set_wake_irq() or
dev_pm_set_dedicated_wake_irq().  The latter is used by the serial
driver used on K3 platforms (drivers/tty/serial/8250/8250_omap.c)
when the interrupts-extended property is used to describe the
dedicated wakeup interrupt.

Detect these wake IRQs in the suspend path, and if set, skip sending
constraint.

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/pmdomain/ti/ti_sci_pm_domains.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
index 8af907579152..f7c327cb059c 100644
--- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
+++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
@@ -84,6 +84,15 @@ static inline bool ti_sci_pd_set_wkup_constraint(struct device *dev)
 	int ret;
 
 	if (device_may_wakeup(dev)) {
+		/*
+		 * If device can wakeup using IO daisy chain wakeups,
+		 * we do not want to set a constraint.
+		 */
+		if (dev->power.wakeirq) {
+			dev_dbg(dev, "%s: has wake IRQ, not setting constraints\n", __func__);
+			return false;
+		}
+
 		ret = ti_sci->ops.pm_ops.set_device_constraint(ti_sci, pd->idx,
 							       TISCI_MSG_CONSTRAINT_SET);
 		if (!ret)

-- 
2.46.0

Re: [PATCH v2 0/3] pmdomain: ti_sci: collect and send low-power mode constraints

[Dhruva Gole]

On Aug 19, 2024 at 17:00:10 -0700, Kevin Hilman wrote:
> The latest (10.x) version of the firmware for the PM co-processor (aka
> device manager, or DM) adds support for a "managed" mode, where the DM
> firmware will select the specific low power state which is entered
> when Linux requests a system-wide suspend.
> 
> In this mode, the DM will always attempt the deepest low-power state
> available for the SoC.
> 
> However, Linux (or OSes running on other cores) may want to constrain
> the DM for certain use cases.  For example, the deepest state may have
> a wakeup/resume latency that is too long for certain use cases.  Or,
> some wakeup-capable devices may potentially be powered off in deep
> low-power states, but if one of those devices is enabled as a wakeup
> source, it should not be powered off.
> 
> These kinds of constraints are are already known in Linux by the use
> of existing APIs such as per-device PM QoS and device wakeup APIs, but
> now we need to communicate these constraints to the DM.
> 
> For TI SoCs with TI SCI support, all DM-managed devices will be
> connected to a TI SCI PM domain.  So the goal of this series is to use
> the PM domain driver for TI SCI devices to collect constraints, and
> communicate them to the DM via the new TI SCI APIs.
> 
> This is all managed by TI SCI PM domain code.  No new APIs are needed
> by Linux drivers.  Any device that is managed by TI SCI will be
> checked for QoS constraints or wakeup capability and the constraints
> will be collected and sent to the DM.
> 
> This series depends on the support for the new TI SCI APIs (v10) and
> was also tested with this series to update 8250_omap serial support
> for AM62x[2].
> 
> [1] https://lore.kernel.org/all/20240801195422.2296347-1-msp@baylibre.com
> [2] https://lore.kernel.org/all/20240807141227.1093006-1-msp@baylibre.com/
> 
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---

Since I applied this series to the v10 TISCI series as well, and tested:

For the series,
Tested-by: Dhruva Gole <d-gole@ti.com>

Logs:
https://gist.github.com/DhruvaG2000/658d0d4683b13ab41025df19a8eafc2f

Tree with all the patches applied:
https://github.com/DhruvaG2000/v-linux/tree/lpm-k3-next

-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

Re: [PATCH v2 1/3] pmdomain: ti_sci: add per-device latency constraint management

[Ulf Hansson]

On Tue, 20 Aug 2024 at 02:00, Kevin Hilman <khilman@baylibre.com> wrote:
>
> For each device in a TI SCI PM domain, check whether the device has
> any resume latency constraints set via per-device PM QoS.  If
> constraints are set, send them to DM via the new SCI constraints API.
>
> Checking for constraints happen for each device before system-wide
> suspend (via ->suspend() hook.)
>
> An important detail here is that the PM domain driver inserts itself
> into the path of both the ->suspend() and ->resume() hook path
> of *all* devices in the PM domain.  This allows generic PM domain code
> to handle the constraint management and communication with TI SCI.
>
> Further, this allows device drivers to use existing PM QoS APIs to
> add/update constraints.
>
> DM firmware clears constraints during its resume, so Linux has
> to check/update/send constraints each time system suspends.
>
> Co-developed-by: Vibhore Vardhan <vibhore@ti.com>
> Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> Signed-off-by: Dhruva Gole <d-gole@ti.com>
> ---
>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> index 1510d5ddae3d..963272fa387b 100644
> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> @@ -13,6 +13,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
>  #include <linux/slab.h>
> +#include <linux/pm_qos.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/soc/ti/ti_sci_protocol.h>
>  #include <dt-bindings/soc/ti,sci_pm_domain.h>
>
> @@ -51,6 +53,29 @@ struct ti_sci_pm_domain {
>
>  #define genpd_to_ti_sci_pd(gpd) container_of(gpd, struct ti_sci_pm_domain, pd)
>
> +static inline bool ti_sci_pd_is_valid_constraint(s32 val)
> +{
> +       return val != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +}

Should we perhaps add a generic helper function for this? Seems like a
similar check is done at other places in the kernel too.

> +
> +static int ti_sci_pd_set_lat_constraint(struct device *dev, s32 val)
> +{

Looks like you may want to turn this into a static void rather than
static int. At least the caller, below, doesn't care about the return
value.

> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +       struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd);
> +       const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
> +       int ret;
> +
> +       ret = ti_sci->ops.pm_ops.set_latency_constraint(ti_sci, val, TISCI_MSG_CONSTRAINT_SET);
> +       if (ret)
> +               dev_err(dev, "ti_sci_pd: set latency constraint failed: ret=%d\n",
> +                       ret);
> +       else
> +               dev_dbg(dev, "ti_sci_pd: ID:%d set latency constraint %d\n",
> +                       pd->idx, val);
> +
> +       return ret;
> +}
> +
>  /*
>   * ti_sci_pd_power_off(): genpd power down hook
>   * @domain: pointer to the powerdomain to power off
> @@ -79,6 +104,22 @@ static int ti_sci_pd_power_on(struct generic_pm_domain *domain)
>                 return ti_sci->ops.dev_ops.get_device(ti_sci, pd->idx);
>  }
>
> +static int ti_sci_pd_suspend(struct device *dev)
> +{
> +       int ret;
> +       s32 val;
> +
> +       ret = pm_generic_suspend(dev);
> +       if (ret)
> +               return ret;
> +
> +       val = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
> +       if (ti_sci_pd_is_valid_constraint(val))
> +               ti_sci_pd_set_lat_constraint(dev, val);
> +
> +       return 0;
> +}
> +
>  /*
>   * ti_sci_pd_xlate(): translation service for TI SCI genpds
>   * @genpdspec: DT identification data for the genpd
> @@ -188,6 +229,13 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev)
>                                 pd->pd.power_on = ti_sci_pd_power_on;
>                                 pd->idx = args.args[0];
>                                 pd->parent = pd_provider;
> +                               /*
> +                                * If SCI constraint functions are present, then firmware
> +                                * supports the constraints API.
> +                                */
> +                               if (pd_provider->ti_sci->ops.pm_ops.set_device_constraint &&
> +                                   pd_provider->ti_sci->ops.pm_ops.set_latency_constraint)
> +                                       pd->pd.domain.ops.suspend = ti_sci_pd_suspend;
>
>                                 pm_genpd_init(&pd->pd, NULL, true);
>
>
> --
> 2.46.0
>

Other than the minor things above, this looks good to me.

Kind regards
Uffe

Re: [PATCH v2 2/3] pmdomain: ti_sci: add wakeup constraint management

[Ulf Hansson]

On Tue, 20 Aug 2024 at 02:00, Kevin Hilman <khilman@baylibre.com> wrote:
>
> During system-wide suspend, check all devices connected to PM domain
> to see if they are wakeup-enabled.  If so, set a TI SCI device
> constraint.
>
> Note: DM firmware clears all constraints on resume.
>
> Co-developed-by: Vibhore Vardhan <vibhore@ti.com>
> Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> Signed-off-by: Dhruva Gole <d-gole@ti.com>
> ---
>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> index 963272fa387b..8af907579152 100644
> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> @@ -76,6 +76,23 @@ static int ti_sci_pd_set_lat_constraint(struct device *dev, s32 val)
>         return ret;
>  }
>
> +static inline bool ti_sci_pd_set_wkup_constraint(struct device *dev)
> +{

Similar comment as for patch1, should this be a static void intstead?

> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +       struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd);
> +       const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
> +       int ret;
> +
> +       if (device_may_wakeup(dev)) {
> +               ret = ti_sci->ops.pm_ops.set_device_constraint(ti_sci, pd->idx,
> +                                                              TISCI_MSG_CONSTRAINT_SET);
> +               if (!ret)
> +                       dev_dbg(dev, "ti_sci_pd: ID:%d set device constraint.\n", pd->idx);
> +       }
> +
> +       return true;
> +}
> +
>  /*
>   * ti_sci_pd_power_off(): genpd power down hook
>   * @domain: pointer to the powerdomain to power off
> @@ -117,6 +134,8 @@ static int ti_sci_pd_suspend(struct device *dev)
>         if (ti_sci_pd_is_valid_constraint(val))
>                 ti_sci_pd_set_lat_constraint(dev, val);
>
> +       ti_sci_pd_set_wkup_constraint(dev);
> +
>         return 0;
>  }
>
>
> --
> 2.46.0
>

Kind regards
Uffe

Re: [PATCH v2 0/3] pmdomain: ti_sci: collect and send low-power mode constraints

[Ulf Hansson]

On Tue, 20 Aug 2024 at 02:00, Kevin Hilman <khilman@baylibre.com> wrote:
>
> The latest (10.x) version of the firmware for the PM co-processor (aka
> device manager, or DM) adds support for a "managed" mode, where the DM
> firmware will select the specific low power state which is entered
> when Linux requests a system-wide suspend.
>
> In this mode, the DM will always attempt the deepest low-power state
> available for the SoC.
>
> However, Linux (or OSes running on other cores) may want to constrain
> the DM for certain use cases.  For example, the deepest state may have
> a wakeup/resume latency that is too long for certain use cases.  Or,
> some wakeup-capable devices may potentially be powered off in deep
> low-power states, but if one of those devices is enabled as a wakeup
> source, it should not be powered off.
>
> These kinds of constraints are are already known in Linux by the use
> of existing APIs such as per-device PM QoS and device wakeup APIs, but
> now we need to communicate these constraints to the DM.
>
> For TI SoCs with TI SCI support, all DM-managed devices will be
> connected to a TI SCI PM domain.  So the goal of this series is to use
> the PM domain driver for TI SCI devices to collect constraints, and
> communicate them to the DM via the new TI SCI APIs.
>
> This is all managed by TI SCI PM domain code.  No new APIs are needed
> by Linux drivers.  Any device that is managed by TI SCI will be
> checked for QoS constraints or wakeup capability and the constraints
> will be collected and sent to the DM.
>
> This series depends on the support for the new TI SCI APIs (v10) and
> was also tested with this series to update 8250_omap serial support
> for AM62x[2].
>
> [1] https://lore.kernel.org/all/20240801195422.2296347-1-msp@baylibre.com
> [2] https://lore.kernel.org/all/20240807141227.1093006-1-msp@baylibre.com/
>
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---
> Changes in v2:
>
> - To simplify this version a bit, drop the pmdomain ->power_off()
>   changes.  Constraints only sent during ->suspend() path.  The pmdomain
>   path was an optimization that may be added back later.
> - With the above simplification, drop the extra state variables that
>   had been added to keep track of constraint status.
> - Link to v1: https://lore.kernel.org/r/20240805-lpm-v6-10-constraints-pmdomain-v1-0-d186b68ded4c@baylibre.com
>
> ---
> Kevin Hilman (3):
>       pmdomain: ti_sci: add per-device latency constraint management
>       pmdomain: ti_sci: add wakeup constraint management
>       pmdomain: ti_sci: handle wake IRQs for IO daisy chain wakeups
>
>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> ---
> base-commit: ad7eb1b6b92ee0c959a0a6ae846ddadd7a79ea64
> change-id: 20240802-lpm-v6-10-constraints-pmdomain-f33df5aef449
>
> Best regards,
> --
> Kevin Hilman <khilman@baylibre.com>

Besides a couple of minor things that I have commented on for each
patch, this looks okay to me!

Taking into account the other series that this depends on, what is the
best merging strategy? Is it safe for me to take it through my
pmdomain tree?

Kind regards
Uffe

Re: [PATCH v2 1/3] pmdomain: ti_sci: add per-device latency constraint management

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Tue, 20 Aug 2024 at 02:00, Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> For each device in a TI SCI PM domain, check whether the device has
>> any resume latency constraints set via per-device PM QoS.  If
>> constraints are set, send them to DM via the new SCI constraints API.
>>
>> Checking for constraints happen for each device before system-wide
>> suspend (via ->suspend() hook.)
>>
>> An important detail here is that the PM domain driver inserts itself
>> into the path of both the ->suspend() and ->resume() hook path
>> of *all* devices in the PM domain.  This allows generic PM domain code
>> to handle the constraint management and communication with TI SCI.
>>
>> Further, this allows device drivers to use existing PM QoS APIs to
>> add/update constraints.
>>
>> DM firmware clears constraints during its resume, so Linux has
>> to check/update/send constraints each time system suspends.
>>
>> Co-developed-by: Vibhore Vardhan <vibhore@ti.com>
>> Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> Signed-off-by: Dhruva Gole <d-gole@ti.com>
>> ---
>>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> index 1510d5ddae3d..963272fa387b 100644
>> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> @@ -13,6 +13,8 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_domain.h>
>>  #include <linux/slab.h>
>> +#include <linux/pm_qos.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/soc/ti/ti_sci_protocol.h>
>>  #include <dt-bindings/soc/ti,sci_pm_domain.h>
>>
>> @@ -51,6 +53,29 @@ struct ti_sci_pm_domain {
>>
>>  #define genpd_to_ti_sci_pd(gpd) container_of(gpd, struct ti_sci_pm_domain, pd)
>>
>> +static inline bool ti_sci_pd_is_valid_constraint(s32 val)
>> +{
>> +       return val != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>> +}
>
> Should we perhaps add a generic helper function for this? Seems like a
> similar check is done at other places in the kernel too.

Maybe, but I don't see a lot of this same usage under drivers/*.  But
I'll have a closer look for a follow-up patch that might be able to make
a common helper for this.

>> +
>> +static int ti_sci_pd_set_lat_constraint(struct device *dev, s32 val)
>> +{
>
> Looks like you may want to turn this into a static void rather than
> static int. At least the caller, below, doesn't care about the return
> value.

Yup, OK.

>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +       struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd);
>> +       const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
>> +       int ret;
>> +
>> +       ret = ti_sci->ops.pm_ops.set_latency_constraint(ti_sci, val, TISCI_MSG_CONSTRAINT_SET);
>> +       if (ret)
>> +               dev_err(dev, "ti_sci_pd: set latency constraint failed: ret=%d\n",
>> +                       ret);
>> +       else
>> +               dev_dbg(dev, "ti_sci_pd: ID:%d set latency constraint %d\n",
>> +                       pd->idx, val);
>> +
>> +       return ret;
>> +}
>> +
>>  /*
>>   * ti_sci_pd_power_off(): genpd power down hook
>>   * @domain: pointer to the powerdomain to power off
>> @@ -79,6 +104,22 @@ static int ti_sci_pd_power_on(struct generic_pm_domain *domain)
>>                 return ti_sci->ops.dev_ops.get_device(ti_sci, pd->idx);
>>  }
>>
>> +static int ti_sci_pd_suspend(struct device *dev)
>> +{
>> +       int ret;
>> +       s32 val;
>> +
>> +       ret = pm_generic_suspend(dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       val = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
>> +       if (ti_sci_pd_is_valid_constraint(val))
>> +               ti_sci_pd_set_lat_constraint(dev, val);
>> +
>> +       return 0;
>> +}
>> +
>>  /*
>>   * ti_sci_pd_xlate(): translation service for TI SCI genpds
>>   * @genpdspec: DT identification data for the genpd
>> @@ -188,6 +229,13 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev)
>>                                 pd->pd.power_on = ti_sci_pd_power_on;
>>                                 pd->idx = args.args[0];
>>                                 pd->parent = pd_provider;
>> +                               /*
>> +                                * If SCI constraint functions are present, then firmware
>> +                                * supports the constraints API.
>> +                                */
>> +                               if (pd_provider->ti_sci->ops.pm_ops.set_device_constraint &&
>> +                                   pd_provider->ti_sci->ops.pm_ops.set_latency_constraint)
>> +                                       pd->pd.domain.ops.suspend = ti_sci_pd_suspend;
>>
>>                                 pm_genpd_init(&pd->pd, NULL, true);
>>
>>
>> --
>> 2.46.0
>>
>
> Other than the minor things above, this looks good to me.

Thanks for the review!   Will spin a v3 shortly.

Kevin

Re: [PATCH v2 2/3] pmdomain: ti_sci: add wakeup constraint management

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Tue, 20 Aug 2024 at 02:00, Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> During system-wide suspend, check all devices connected to PM domain
>> to see if they are wakeup-enabled.  If so, set a TI SCI device
>> constraint.
>>
>> Note: DM firmware clears all constraints on resume.
>>
>> Co-developed-by: Vibhore Vardhan <vibhore@ti.com>
>> Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> Signed-off-by: Dhruva Gole <d-gole@ti.com>
>> ---
>>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> index 963272fa387b..8af907579152 100644
>> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> @@ -76,6 +76,23 @@ static int ti_sci_pd_set_lat_constraint(struct device *dev, s32 val)
>>         return ret;
>>  }
>>
>> +static inline bool ti_sci_pd_set_wkup_constraint(struct device *dev)
>> +{
>
> Similar comment as for patch1, should this be a static void intstead?
>

Yes, will do in v3.

Kevin

Re: [PATCH v2 0/3] pmdomain: ti_sci: collect and send low-power mode constraints

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Tue, 20 Aug 2024 at 02:00, Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> The latest (10.x) version of the firmware for the PM co-processor (aka
>> device manager, or DM) adds support for a "managed" mode, where the DM
>> firmware will select the specific low power state which is entered
>> when Linux requests a system-wide suspend.
>>
>> In this mode, the DM will always attempt the deepest low-power state
>> available for the SoC.
>>
>> However, Linux (or OSes running on other cores) may want to constrain
>> the DM for certain use cases.  For example, the deepest state may have
>> a wakeup/resume latency that is too long for certain use cases.  Or,
>> some wakeup-capable devices may potentially be powered off in deep
>> low-power states, but if one of those devices is enabled as a wakeup
>> source, it should not be powered off.
>>
>> These kinds of constraints are are already known in Linux by the use
>> of existing APIs such as per-device PM QoS and device wakeup APIs, but
>> now we need to communicate these constraints to the DM.
>>
>> For TI SoCs with TI SCI support, all DM-managed devices will be
>> connected to a TI SCI PM domain.  So the goal of this series is to use
>> the PM domain driver for TI SCI devices to collect constraints, and
>> communicate them to the DM via the new TI SCI APIs.
>>
>> This is all managed by TI SCI PM domain code.  No new APIs are needed
>> by Linux drivers.  Any device that is managed by TI SCI will be
>> checked for QoS constraints or wakeup capability and the constraints
>> will be collected and sent to the DM.
>>
>> This series depends on the support for the new TI SCI APIs (v10) and
>> was also tested with this series to update 8250_omap serial support
>> for AM62x[2].
>>
>> [1] https://lore.kernel.org/all/20240801195422.2296347-1-msp@baylibre.com
>> [2] https://lore.kernel.org/all/20240807141227.1093006-1-msp@baylibre.com/
>>
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>> Changes in v2:
>>
>> - To simplify this version a bit, drop the pmdomain ->power_off()
>>   changes.  Constraints only sent during ->suspend() path.  The pmdomain
>>   path was an optimization that may be added back later.
>> - With the above simplification, drop the extra state variables that
>>   had been added to keep track of constraint status.
>> - Link to v1: https://lore.kernel.org/r/20240805-lpm-v6-10-constraints-pmdomain-v1-0-d186b68ded4c@baylibre.com
>>
>> ---
>> Kevin Hilman (3):
>>       pmdomain: ti_sci: add per-device latency constraint management
>>       pmdomain: ti_sci: add wakeup constraint management
>>       pmdomain: ti_sci: handle wake IRQs for IO daisy chain wakeups
>>
>>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>> ---
>> base-commit: ad7eb1b6b92ee0c959a0a6ae846ddadd7a79ea64
>> change-id: 20240802-lpm-v6-10-constraints-pmdomain-f33df5aef449
>>
>> Best regards,
>> --
>> Kevin Hilman <khilman@baylibre.com>
>
> Besides a couple of minor things that I have commented on for each
> patch, this looks okay to me!
>
> Taking into account the other series that this depends on, what is the
> best merging strategy? Is it safe for me to take it through my
> pmdomain tree?


That other series should be merged shortly, so I will check with
Nishanth (on cc) if he can create an immutable branch/tag that you could
use in your tree.

It has a build-time dependency on that other series, so I think this is
the best way.

Alternatively, if you don't expect this to clash with other changes in
your tree, with your ack/reviewed-by, Nishanth could merge this series
via his tree and we could avoid the cross-tree shuffle.

Either way will work.  Up to you.

Kevin

Re: [PATCH v2 0/3] pmdomain: ti_sci: collect and send low-power mode constraints

[Ulf Hansson]

On Fri, 6 Sept 2024 at 00:07, Kevin Hilman <khilman@baylibre.com> wrote:
>
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
> > On Tue, 20 Aug 2024 at 02:00, Kevin Hilman <khilman@baylibre.com> wrote:
> >>
> >> The latest (10.x) version of the firmware for the PM co-processor (aka
> >> device manager, or DM) adds support for a "managed" mode, where the DM
> >> firmware will select the specific low power state which is entered
> >> when Linux requests a system-wide suspend.
> >>
> >> In this mode, the DM will always attempt the deepest low-power state
> >> available for the SoC.
> >>
> >> However, Linux (or OSes running on other cores) may want to constrain
> >> the DM for certain use cases.  For example, the deepest state may have
> >> a wakeup/resume latency that is too long for certain use cases.  Or,
> >> some wakeup-capable devices may potentially be powered off in deep
> >> low-power states, but if one of those devices is enabled as a wakeup
> >> source, it should not be powered off.
> >>
> >> These kinds of constraints are are already known in Linux by the use
> >> of existing APIs such as per-device PM QoS and device wakeup APIs, but
> >> now we need to communicate these constraints to the DM.
> >>
> >> For TI SoCs with TI SCI support, all DM-managed devices will be
> >> connected to a TI SCI PM domain.  So the goal of this series is to use
> >> the PM domain driver for TI SCI devices to collect constraints, and
> >> communicate them to the DM via the new TI SCI APIs.
> >>
> >> This is all managed by TI SCI PM domain code.  No new APIs are needed
> >> by Linux drivers.  Any device that is managed by TI SCI will be
> >> checked for QoS constraints or wakeup capability and the constraints
> >> will be collected and sent to the DM.
> >>
> >> This series depends on the support for the new TI SCI APIs (v10) and
> >> was also tested with this series to update 8250_omap serial support
> >> for AM62x[2].
> >>
> >> [1] https://lore.kernel.org/all/20240801195422.2296347-1-msp@baylibre.com
> >> [2] https://lore.kernel.org/all/20240807141227.1093006-1-msp@baylibre.com/
> >>
> >> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> >> ---
> >> Changes in v2:
> >>
> >> - To simplify this version a bit, drop the pmdomain ->power_off()
> >>   changes.  Constraints only sent during ->suspend() path.  The pmdomain
> >>   path was an optimization that may be added back later.
> >> - With the above simplification, drop the extra state variables that
> >>   had been added to keep track of constraint status.
> >> - Link to v1: https://lore.kernel.org/r/20240805-lpm-v6-10-constraints-pmdomain-v1-0-d186b68ded4c@baylibre.com
> >>
> >> ---
> >> Kevin Hilman (3):
> >>       pmdomain: ti_sci: add per-device latency constraint management
> >>       pmdomain: ti_sci: add wakeup constraint management
> >>       pmdomain: ti_sci: handle wake IRQs for IO daisy chain wakeups
> >>
> >>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 76 insertions(+)
> >> ---
> >> base-commit: ad7eb1b6b92ee0c959a0a6ae846ddadd7a79ea64
> >> change-id: 20240802-lpm-v6-10-constraints-pmdomain-f33df5aef449
> >>
> >> Best regards,
> >> --
> >> Kevin Hilman <khilman@baylibre.com>
> >
> > Besides a couple of minor things that I have commented on for each
> > patch, this looks okay to me!
> >
> > Taking into account the other series that this depends on, what is the
> > best merging strategy? Is it safe for me to take it through my
> > pmdomain tree?
>
>
> That other series should be merged shortly, so I will check with
> Nishanth (on cc) if he can create an immutable branch/tag that you could
> use in your tree.
>
> It has a build-time dependency on that other series, so I think this is
> the best way.
>
> Alternatively, if you don't expect this to clash with other changes in
> your tree, with your ack/reviewed-by, Nishanth could merge this series
> via his tree and we could avoid the cross-tree shuffle.

At the moment there shouldn't be any clashes I think. Let's use
Nishanth's tree and see how it goes.

I will ack/review the patches when they are ready.

Kind regards
Uffe