Re: [PATCH v2 5/5] coresight: etm4x: save/restore state across CPU low power states

From: Suzuki K Poulose <suzuki.poulose@arm•com>
To: andrew.murray@arm•com, mathieu.poirier@linaro•org,
	alexander.shishkin@linux•intel.com
Cc: coresight@lists•linaro.org, Sudeep.Holla@arm•com,
	linux-arm-kernel@lists•infradead.org, mike.leach@linaro•org
Subject: Re: [PATCH v2 5/5] coresight: etm4x: save/restore state across CPU low power states
Date: Mon, 1 Jul 2019 14:15:49 +0100	[thread overview]
Message-ID: <13dde41d-5ef4-e9a0-7310-ed5ead5e8230@arm.com> (raw)
In-Reply-To: <20190627083525.37463-6-andrew.murray@arm.com>

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.

> +
> +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).

> +
> +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
> + * @nb:		CPU PM notifier
>    */
>   struct etmv4_drvdata {
>   	void __iomem			*base;
> @@ -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.

> +	bool				state_needs_restore;

Rest looks good to me.

Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel