| Message ID | 20240829201606.1407773-5-msp@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: ti_sci: Introduce system suspend support | expand |
Hi Markus, On Thu, Aug 29, 2024 at 10:16:05PM +0200, Markus Schneider-Pargmann wrote: > From: Kevin Hilman <khilman@baylibre.com> > > During system-wide suspend, check if any of the CPUs have PM QoS > resume latency constraints set. If so, set TI SCI constraint. > > TI SCI has a single system-wide latency constraint, so use the max of > any of the CPU latencies as the system-wide value. > > Note: DM firmware clears all constraints at resume time, so > constraints need to be checked/updated/sent at each system suspend. > > Co-developed-by: Vibhore Vardhan <vibhore@ti.com> > Signed-off-by: Vibhore Vardhan <vibhore@ti.com> > Reviewed-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Kevin Hilman <khilman@baylibre.com> > Tested-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > --- > drivers/firmware/ti_sci.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c > index 8482b7077eef..d5d64e5ffbd7 100644 > --- a/drivers/firmware/ti_sci.c > +++ b/drivers/firmware/ti_sci.c > @@ -10,6 +10,7 @@ > #define pr_fmt(fmt) "%s: " fmt, __func__ > > #include <linux/bitmap.h> > +#include <linux/cpu.h> > #include <linux/debugfs.h> > #include <linux/export.h> > #include <linux/io.h> > @@ -20,6 +21,7 @@ > #include <linux/of.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/pm_qos.h> > #include <linux/property.h> > #include <linux/semaphore.h> > #include <linux/slab.h> > @@ -3669,7 +3671,27 @@ static int ti_sci_prepare_system_suspend(struct ti_sci_info *info) > static int __maybe_unused ti_sci_suspend(struct device *dev) > { > struct ti_sci_info *info = dev_get_drvdata(dev); > - int ret; > + struct device *cpu_dev, *cpu_dev_max = NULL; > + s32 val, cpu_lat = 0; > + int i, ret; > + > + if (info->fw_caps & MSG_FLAG_CAPS_LPM_DM_MANAGED) { > + for_each_possible_cpu(i) { > + cpu_dev = get_cpu_device(i); > + val = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_RESUME_LATENCY); This change is now in -next as commit 458d22d2e064 ("firmware: ti_sci: add CPU latency constraint management"), where it breaks the build when this driver is built as a module because dev_pm_qos_read_value() is not exported to modules: ERROR: modpost: "dev_pm_qos_read_value" [drivers/firmware/ti_sci.ko] undefined! Obviously exporting it would fix the build but sometimes that is controversial, hence just the report. > + if (val != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) { > + cpu_lat = max(cpu_lat, val); > + cpu_dev_max = cpu_dev; > + } > + } > + if (cpu_dev_max) { > + dev_dbg(cpu_dev_max, "%s: sending max CPU latency=%u\n", __func__, cpu_lat); > + ret = ti_sci_cmd_set_latency_constraint(&info->handle, > + cpu_lat, TISCI_MSG_CONSTRAINT_SET); > + if (ret) > + return ret; > + } > + } > > ret = ti_sci_prepare_system_suspend(info); > if (ret) > -- > 2.45.2 >
On 17:54-20240902, Nathan Chancellor wrote: [...] > > @@ -3669,7 +3671,27 @@ static int ti_sci_prepare_system_suspend(struct ti_sci_info *info) > > static int __maybe_unused ti_sci_suspend(struct device *dev) > > { > > struct ti_sci_info *info = dev_get_drvdata(dev); > > - int ret; > > + struct device *cpu_dev, *cpu_dev_max = NULL; > > + s32 val, cpu_lat = 0; > > + int i, ret; > > + > > + if (info->fw_caps & MSG_FLAG_CAPS_LPM_DM_MANAGED) { > > + for_each_possible_cpu(i) { > > + cpu_dev = get_cpu_device(i); > > + val = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_RESUME_LATENCY); > > This change is now in -next as commit 458d22d2e064 ("firmware: ti_sci: > add CPU latency constraint management"), where it breaks the build > when this driver is built as a module because dev_pm_qos_read_value() is > not exported to modules: > > ERROR: modpost: "dev_pm_qos_read_value" [drivers/firmware/ti_sci.ko] undefined! > > Obviously exporting it would fix the build but sometimes that is > controversial, hence just the report. Thank you for the report. I will drop the series from my queue for now. That should give us some time to sort things out properly for the next window.
Hi, On Tue, Sep 03, 2024 at 07:45:40AM GMT, Nishanth Menon wrote: > On 17:54-20240902, Nathan Chancellor wrote: > [...] > > > > @@ -3669,7 +3671,27 @@ static int ti_sci_prepare_system_suspend(struct ti_sci_info *info) > > > static int __maybe_unused ti_sci_suspend(struct device *dev) > > > { > > > struct ti_sci_info *info = dev_get_drvdata(dev); > > > - int ret; > > > + struct device *cpu_dev, *cpu_dev_max = NULL; > > > + s32 val, cpu_lat = 0; > > > + int i, ret; > > > + > > > + if (info->fw_caps & MSG_FLAG_CAPS_LPM_DM_MANAGED) { > > > + for_each_possible_cpu(i) { > > > + cpu_dev = get_cpu_device(i); > > > + val = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_RESUME_LATENCY); > > > > This change is now in -next as commit 458d22d2e064 ("firmware: ti_sci: > > add CPU latency constraint management"), where it breaks the build > > when this driver is built as a module because dev_pm_qos_read_value() is > > not exported to modules: > > > > ERROR: modpost: "dev_pm_qos_read_value" [drivers/firmware/ti_sci.ko] undefined! > > > > Obviously exporting it would fix the build but sometimes that is > > controversial, hence just the report. > > Thank you for the report. I will drop the series from my queue for now. > That should give us some time to sort things out properly for the next > window. Thanks as well for reporting. I looked into this issue and it looks like many of the dev_pm_qos_* functions are already exported. Documentation/power/pm_qos_interface.rst also already lists the function for kernel-internal use along with many others. So I think adding the export is the right way and I prepared a patch for that. Also from what I saw and tested, I was only able to reproduce this issue with ARCH_KEYSTONE when the TI_SCI_PROTOCOL is selected as module. multi_v7_defconfig and keystone_defconfig both select it as built-in as well as ARCH_K3. For all other architectures, TI_SCI_PROTOCOL can not be selected as it depends on TI_MESSAGE_MANAGER which depends on KEYSTONE or K3. Best Markus
diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c index 8482b7077eef..d5d64e5ffbd7 100644 --- a/drivers/firmware/ti_sci.c +++ b/drivers/firmware/ti_sci.c @@ -10,6 +10,7 @@ #define pr_fmt(fmt) "%s: " fmt, __func__ #include <linux/bitmap.h> +#include <linux/cpu.h> #include <linux/debugfs.h> #include <linux/export.h> #include <linux/io.h> @@ -20,6 +21,7 @@ #include <linux/of.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/pm_qos.h> #include <linux/property.h> #include <linux/semaphore.h> #include <linux/slab.h> @@ -3669,7 +3671,27 @@ static int ti_sci_prepare_system_suspend(struct ti_sci_info *info) static int __maybe_unused ti_sci_suspend(struct device *dev) { struct ti_sci_info *info = dev_get_drvdata(dev); - int ret; + struct device *cpu_dev, *cpu_dev_max = NULL; + s32 val, cpu_lat = 0; + int i, ret; + + if (info->fw_caps & MSG_FLAG_CAPS_LPM_DM_MANAGED) { + for_each_possible_cpu(i) { + cpu_dev = get_cpu_device(i); + val = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_RESUME_LATENCY); + if (val != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) { + cpu_lat = max(cpu_lat, val); + cpu_dev_max = cpu_dev; + } + } + if (cpu_dev_max) { + dev_dbg(cpu_dev_max, "%s: sending max CPU latency=%u\n", __func__, cpu_lat); + ret = ti_sci_cmd_set_latency_constraint(&info->handle, + cpu_lat, TISCI_MSG_CONSTRAINT_SET); + if (ret) + return ret; + } + } ret = ti_sci_prepare_system_suspend(info); if (ret)