On Mon, Jul 01, 2019 at 02:15:49PM +0100, Suzuki K Poulose wrote:
On 27/06/2019 09:35, Andrew Murray wrote:
Some hardware will ignore bit TRCPDCR.PU which is used to signal to hardware that power should not be removed from the trace unit. Let's mitigate against this by conditionally saving and restoring the trace unit state when the CPU enters low power states.
This patchset introduces a firmware property named 'arm,coresight-needs-save-restore' - when this is present the hardware state will be conditionally saved and restored.
A module parameter 'pm_save_enable' is also introduced which can be configured to override the firmware property. This can be set to never allow save/restore, to conditionally allow it, or to do as the firmware indicates (default).
The hardware state is only ever saved and restored when the claim tags indicate that coresight is in use.
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index 86945f054cf8..eff317cd3a03 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -18,6 +18,7 @@ #include <linux/stat.h> #include <linux/clk.h> #include <linux/cpu.h> +#include <linux/cpu_pm.h> #include <linux/coresight.h> #include <linux/coresight-pmu.h> #include <linux/pm_wakeup.h> @@ -26,6 +27,7 @@ #include <linux/uaccess.h> #include <linux/perf_event.h> #include <linux/pm_runtime.h> +#include <linux/property.h> #include <asm/sections.h> #include <asm/local.h> #include <asm/virt.h> @@ -37,6 +39,16 @@ static int boot_enable; module_param(boot_enable, int, 0444); MODULE_PARM_DESC(boot_enable, "Enable tracing on boot");
+#define PARAM_PM_SAVE_DISABLE 0 +#define PARAM_PM_SAVE_ENABLE 1 +#define PARAM_PM_SAVE_FIRMWARE 2
+static int pm_save_enable = PARAM_PM_SAVE_FIRMWARE; +module_param(pm_save_enable, int, 0644); +MODULE_PARM_DESC(pm_save_enable, "Save/restore state on power down: "
"0 = disabled, 1 = enabled, 2 = firmware");nit: Please could you add a comment to explain the implications of "firmware" and how this interacts with "disabled" as it is explained in your thread with Leo. That will be quite helpful for someone looking to use the parameter.
Sure.
+static int etm4_cpu_pm_register(struct etmv4_drvdata *drvdata) +{
- drvdata->nb.notifier_call = etm4_cpu_pm_notify;
- return cpu_pm_register_notifier(&drvdata->nb);
+}
Do we need to tie the notifer_block to etmv4_drvdata ? We could simply use a static one for the entire driver, given we don't rely on the "drvdata". Also, we register for only one ETM device at the moment, which is problematic if that CPU goes down and we decide to unregister the ETM device (in the future).
Yes there is no reason to tie this up with drvdata, it can be static to the driver. I'll make this change and ensure there is no issue if any CPU goes down.
+static void etm4_cpu_pm_unregister(struct etmv4_drvdata *drvdata) +{
- if (drvdata->nb.notifier_call)
cpu_pm_unregister_notifier(&drvdata->nb);+} +#else +static int etm4_cpu_pm_register(struct etmv4_drvdata *drvdata) { return 0; } +static void etm4_cpu_pm_unregister(struct etmv4_drvdata *drvdata) { } +#endif
+static inline bool etm4_needs_save_restore(struct device *dev) +{
- return fwnode_property_present(dev->fwnode,
"arm,coresight-needs-save-restore");+}
- static int etm4_probe(struct amba_device *adev, const struct amba_id *id) { int ret;
@@ -1095,6 +1386,8 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) dev_set_drvdata(dev, drvdata);
- drvdata->pm_save_enable = etm4_needs_save_restore(dev);
- /* Validity for the resource is already checked by the AMBA core */ base = devm_ioremap_resource(dev, res); if (IS_ERR(base))
@@ -1126,6 +1419,10 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) if (ret < 0) goto err_arch_supported; hp_online = ret;
ret = etm4_cpu_pm_register(drvdata);if (ret)goto err_arch_supported;/**
- struct etm4_drvdata - specifics associated to an ETM component
- @base: Memory mapped base address for this component.
@@ -336,6 +396,8 @@ struct etmv4_config {
- @atbtrig: If the implementation can support ATB triggers
- @lpoverride: If the implementation can support low-power state over.
- @config: structure holding configuration parameters.
- @save_state: State to be preserved across power loss
*/ struct etmv4_drvdata { void __iomem *base;
- @nb: CPU PM notifier
@@ -367,6 +429,7 @@ struct etmv4_drvdata { u8 q_support; bool sticky_enable; bool boot_enable;
- bool pm_save_enable; bool os_unlock; bool instrp0; bool trcbb;
@@ -381,6 +444,9 @@ struct etmv4_drvdata { bool atbtrig; bool lpoverride; struct etmv4_config config;
- struct etmv4_save_state save_state;
We may potentially reduce the memory usage by dynamically allocating this structure the variable below. So you may make this a ptr instead.
Sure.
- bool state_needs_restore;
Rest looks good to me.
Thanks,
Andrew Murray
Suzuki