On Thu, Jun 27, 2019 at 05:01:28PM +0100, Suzuki K Poulose wrote:
On 06/27/2019 03:55 PM, Andrew Murray wrote:
On Thu, Jun 27, 2019 at 03:25:44PM +0100, Mike Leach wrote:
Hi Andrew,
On Thu, 27 Jun 2019 at 09:35, Andrew Murray andrew.murray@arm.com 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.
Signed-off-by: Andrew Murray andrew.murray@arm.com
...
state->trcvmidcctlr0 = readl(drvdata->base + TRCVMIDCCTLR0);state->trcvmidcctlr0 = readl(drvdata->base + TRCVMIDCCTLR1);state->trcclaimset = readl(drvdata->base + TRCCLAIMCLR);/* wait for TRCSTATR.IDLE to go up */if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 1)) {dev_err(etm_dev,"timeout while waiting for Idle Trace Status\n");etm4_os_unlock(drvdata);ret = -EBUSY;goto out;}drvdata->state_needs_restore = true;/* power can be removed from the trace unit now */control = readl_relaxed(drvdata->base + TRCPDCR);control &= ~TRCPDCR_PU;writel_relaxed(control, drvdata->base + TRCPDCR);Do we need to manipulate PU here? The premise of this set is PU is ignored. That said, there might be a scenario where PU is honoured but we are forcing this anyway, in which case, why is PU not manipulated in the _restore() function?
I don't think this should be here. The TRM doesn't suggest this so I'm not sure how I ended up with this.
As you suggest, if we are using this save/restore code then we really don't care if the unit remains powered or not.
I personally think keeping the code is a good idea. Its just 1 read and 1 write. If it serves to reduce the power consumption on "compliant" systems (where this option could be passed on by default), then why not ?
OK, in that case I'll also take Mike's suggestion to set the PU bit on restore. There is only a single use-case where this is useful - this is where a user, at run-time, switches off saving/restoring on a system that respects the PU bit (following on from a wake-up where we restore the state). On the next CPU power down the PU bit will be respected and the associated power domain kept up.
Thanks,
Andrew Murray
I'll remove this.
As I said above, I am in favor of keeping this.
u64 trcacvr[ETM_MAX_SINGLE_ADDR_CMP];u64 trcacatr[ETM_MAX_SINGLE_ADDR_CMP];u64 trcdvcvr[ETM_MAX_DATA_VAL_CMP];u64 trcdvcmr[ETM_MAX_DATA_VAL_CMP];These two sets of DATA registers - never used in the main code. Either use them or lose them. I recommend that we lose them - data trace is architecturally prohibited for A class cores in ETMv4.
The trcdvcvr and trcdvcmr registers are here because the TRM (ARM IHI 0064D, section 3.4.3 "Guidelines for trace unit registers to be saved and restored") lists these are registers that must be saved.
I'm happy to drop them, however can you point me in the direction of some documentation that specifies this? I'll add a comment.
Section "1.3.4 Possible functional configurations of an ETMv4 trace unit"
"Table 1-2 A summary of the features of an ETMv4 trace unit"
Cheers Suzuki