| Message ID | 20240809135347.2112634-5-msp@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: ti_sci: Introduce system suspend support | expand |
Hello, On Aug 09, 2024 at 15:53:47 +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> > Signed-off-by: Kevin Hilman <khilman@baylibre.com> > Reviewed-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > --- > drivers/firmware/ti_sci.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c > index 5cbeca5df313..481b7649fde1 100644 > --- a/drivers/firmware/ti_sci.c > +++ b/drivers/firmware/ti_sci.c > @@ -9,6 +9,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> > @@ -19,6 +20,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> > @@ -3639,7 +3641,25 @@ static int ti_sci_prepare_system_suspend(struct ti_sci_info *info) > static int ti_sci_suspend(struct device *dev) > { > struct ti_sci_info *info = dev_get_drvdata(dev); > - int ret; > + struct device *cpu_dev; > + 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); > + } > + if (cpu_lat && cpu_lat != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) { > + dev_dbg(cpu_dev, "%s: sending max CPU latency=%u\n", __func__, cpu_lat); An interesting observation was made which caused us to suspect this code, the CPU on which the latency was actually being set was not being printed here. It was always the cpu3 cpu cpu3: ti_sci_suspend: sending max CPU latency=100 If you look at how this print comes, it's always after all the cpu indices have run, so by then the cpu_dev value will have always become = nproc in the system. This makes debugging it confusing.
Dhruva Gole <d-gole@ti.com> writes: > Hello, > > On Aug 09, 2024 at 15:53:47 +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> >> Signed-off-by: Kevin Hilman <khilman@baylibre.com> >> Reviewed-by: Dhruva Gole <d-gole@ti.com> >> Signed-off-by: Dhruva Gole <d-gole@ti.com> >> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> >> --- >> drivers/firmware/ti_sci.c | 22 +++++++++++++++++++++- >> 1 file changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c >> index 5cbeca5df313..481b7649fde1 100644 >> --- a/drivers/firmware/ti_sci.c >> +++ b/drivers/firmware/ti_sci.c >> @@ -9,6 +9,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> >> @@ -19,6 +20,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> >> @@ -3639,7 +3641,25 @@ static int ti_sci_prepare_system_suspend(struct ti_sci_info *info) >> static int ti_sci_suspend(struct device *dev) >> { >> struct ti_sci_info *info = dev_get_drvdata(dev); >> - int ret; >> + struct device *cpu_dev; >> + 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); >> + } >> + if (cpu_lat && cpu_lat != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) { >> + dev_dbg(cpu_dev, "%s: sending max CPU latency=%u\n", __func__, cpu_lat); > > An interesting observation was made which caused us to suspect this > code, the CPU on which the latency was actually being set was not being > printed here. It was always the cpu3 > > cpu cpu3: ti_sci_suspend: sending max CPU latency=100 > > If you look at how this print comes, it's always after all the cpu > indices have run, so by then the cpu_dev value will have always become > = nproc in the system. This makes debugging it confusing. Good catch. That's definitely a debug bug. :) Will fix in the next version. Thanks for the review & testing, Kevin
diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c index 5cbeca5df313..481b7649fde1 100644 --- a/drivers/firmware/ti_sci.c +++ b/drivers/firmware/ti_sci.c @@ -9,6 +9,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> @@ -19,6 +20,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> @@ -3639,7 +3641,25 @@ static int ti_sci_prepare_system_suspend(struct ti_sci_info *info) static int ti_sci_suspend(struct device *dev) { struct ti_sci_info *info = dev_get_drvdata(dev); - int ret; + struct device *cpu_dev; + 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); + } + if (cpu_lat && cpu_lat != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) { + dev_dbg(cpu_dev, "%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)