Hi Andrew,
On Fri, Jun 28, 2019 at 10:22:28AM +0100, Andrew Murray wrote:
On Fri, Jun 28, 2019 at 05:12:46PM +0800, Leo Yan wrote:
On Fri, Jun 28, 2019 at 09:53:58AM +0100, Andrew Murray wrote:
[...]
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
[...]
+#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");
I understand if set pm_save_enable = 2 (firmware), then driver will depend on drvdata->pm_save_enable to make decision for context saving and restoring.
Maybe we can simplize to set pm_save_enable for binary value: 0 (disabled) or 1 (enabled). The reason is if we set the module parameter 'pm_save_enable = 1', then we can set every ETM device's drvdata->pm_save_enable in initialization phase. So in the probe function, we can use below code:
drvdata->pm_save_enable = pm_save_enable ? : etm4_needs_save_restore(dev);
This means that when the module parameter is set to 1, then we only save/restore if the firmware suggests it is needed.
Sorry, I seemingly can't read code today.
If the module parameter is set to 1, then we will always set every device 'drvdata->pm_save_enable' to 1. So in this case, the module parameter will override the firmware property and always save/restore contexts for ETM.
However - what happens on hardware that ignores the PU bit (and thus requires save/restore), yet it's firmware doesn't have the 'arm,coresight-needs-save-restore' property? There is no way to override the firmware and always save/restore.
Actually I suggested to give the module parameter with high priority and when the module parameter has set to 1, then it can override firmware 'arm,coresight-needs-save-restore' property.
If we set the module parameter in kernel command line or when load the module, its value can be used in the function etm4_probe(). So in the driver probing, it detects the module parameter is 1, then it can directly set every device 'drvdata->pm_save_enable' to 1. Thus we can always save/restore context for ignoring the PU bit case.
In any case, not only do we want to override the firmware to always save/restore. Sometimes we may also want to override the firmware to never save/restore (despite the firmware having the 'arm,coresight-needs-save-restore' flag present). For example to debug power management.
Thus with this current approach you can override the firmware to either enable or disable save/restore.
Thanks for explanation and agree with this. Just a suggestion, maybe we can initialize 'drvdata->pm_save_enable' in probe as below:
if (pm_save_enable == PARAM_PM_SAVE_FIRMWARE) drvdata->pm_save_enable = etm4_needs_save_restore(dev); else drvdata->pm_save_enable = pm_save_enable;
From then on, we can only use 'drvdata->pm_save_enable' and don't need
to check the module parameter anymore.
Thanks, Leo Yan