On Thu, Jul 04, 2019 at 08:27:44AM -0600, Mathieu Poirier wrote:
On Thu, 4 Jul 2019 at 04:21, Andrew Murray andrew.murray@arm.com wrote:
On Mon, Jul 01, 2019 at 06:14:40PM +0800, Leo Yan wrote:
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).
I've only been able to test this on a Juno (which doesn't suffer from this issue). Perhaps Mathieu can suggest known platforms that require cpuidle disabled to use coresight?
The dragonboard 410c will be perfect for that.
Thanks for suggestion. After Andrew work out the new patch set, I will test it on DB410c board.
Thanks, Leo Yan