On Mon, Jul 01, 2019 at 10:54:44AM +0100, Andrew Murray wrote:
On Mon, Jul 01, 2019 at 05:48:11PM +0800, Leo Yan wrote:
On Mon, Jul 01, 2019 at 10:34:58AM +0100, Andrew Murray wrote:
[...]
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.
This is OK, however it means you can't then change the mode once the driver is probed. I.e. you can't echo values into /sys/module/coresight-etm4x/pm_save_enable at runtime. This was useful to me during testing, I assumed it may be useful for others too (especially given that you can't unload the module).
Ah, okay, this is fine for me. Thanks for sharing the knowledge.
Thanks for the review, can I add your Reviewed-By?
Yes, please. Usually, I would like to give a test for patches, this patch is important for productions after enable CoreSight with CPU idle; but some platforms are easily to hang after enable CPU idle with Coresight.
So if you suggest to test this patch set on some public platform, I can give a testing on it (if I have the boards).
Thanks, Leo Yan