Re: [PATCH v5 1/3] coresight: etm4x: save/restore state across CPU low power states

From: Andrew Murray <andrew.murray@arm•com>
To: Suzuki K Poulose <suzuki.poulose@arm•com>
Cc: Al.Grant@arm•com, mathieu.poirier@linaro•org,
	alexander.shishkin@linux•intel.com, coresight@lists•linaro.org,
	leo.yan@linaro•org, sudeep.holla@arm•com,
	linux-arm-kernel@lists•infradead.org, mike.leach@linaro•org
Subject: Re: [PATCH v5 1/3] coresight: etm4x: save/restore state across CPU low power states
Date: Fri, 13 Sep 2019 10:20:41 +0100	[thread overview]
Message-ID: <20190913092040.GJ9720@e119886-lin.cambridge.arm.com> (raw)
In-Reply-To: <3e3f7e86-c62b-e9d6-9d42-f4897b47847d@arm.com>

On Thu, Sep 12, 2019 at 03:03:44PM +0100, Suzuki K Poulose wrote:
> Hi Andrew,
> 
> On 08/16/2019 04:46 PM, 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-loses-context-with-cpu' - 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 or to conditionally allow it (only for
> > self-hosted). The default value is determined by firmware.
> > 
> > We avoid saving the hardware state when self-hosted coresight isn't
> > in use to reduce PM latency - we can't determine this by reading the
> > claim tags (TRCCLAIMCLR) as these are 'trace' registers which need
> > power and clocking, something we can't easily provide in the PM
> > context. Therefore we rely on the existing drvdata->mode internal
> > state that is set when self-hosted coresight is used (and powered).
> 
> The patch looks good to me. Some very minor comments below.
> 
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm•com>
> > ---
> >   drivers/hwtracing/coresight/coresight-etm4x.c | 318 ++++++++++++++++++
> >   drivers/hwtracing/coresight/coresight-etm4x.h |  64 ++++
> >   drivers/hwtracing/coresight/coresight.c       |   6 +
> >   include/linux/coresight.h                     |   6 +
> >   4 files changed, 394 insertions(+)
> > 
> 
> 
> > +static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> > +{
> > +	int i;
> > +	struct etmv4_save_state *state = drvdata->save_state;
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	writel_relaxed(state->trcclaimset, drvdata->base + TRCCLAIMSET);
> > +
> > +	writel_relaxed(state->trcprgctlr, drvdata->base + TRCPRGCTLR);
> > +	writel_relaxed(state->trcprocselr, drvdata->base + TRCPROCSELR);
> > +	writel_relaxed(state->trcconfigr, drvdata->base + TRCCONFIGR);
> > +	writel_relaxed(state->trcauxctlr, drvdata->base + TRCAUXCTLR);
> > +	writel_relaxed(state->trceventctl0r, drvdata->base + TRCEVENTCTL0R);
> > +	writel_relaxed(state->trceventctl1r, drvdata->base + TRCEVENTCTL1R);
> > +	writel_relaxed(state->trcstallctlr, drvdata->base + TRCSTALLCTLR);
> > +	writel_relaxed(state->trctsctlr, drvdata->base + TRCTSCTLR);
> > +	writel_relaxed(state->trcsyncpr, drvdata->base + TRCSYNCPR);
> > +	writel_relaxed(state->trcccctlr, drvdata->base + TRCCCCTLR);
> > +	writel_relaxed(state->trcbbctlr, drvdata->base + TRCBBCTLR);
> > +	writel_relaxed(state->trctraceidr, drvdata->base + TRCTRACEIDR);
> > +	writel_relaxed(state->trcqctlr, drvdata->base + TRCQCTLR);
> > +
> > +	writel_relaxed(state->trcvictlr, drvdata->base + TRCVICTLR);
> > +	writel_relaxed(state->trcviiectlr, drvdata->base + TRCVIIECTLR);
> > +	writel_relaxed(state->trcvissctlr, drvdata->base + TRCVISSCTLR);
> > +	writel_relaxed(state->trcvipcssctlr, drvdata->base + TRCVIPCSSCTLR);
> > +	writel_relaxed(state->trcvdctlr, drvdata->base + TRCVDCTLR);
> > +	writel_relaxed(state->trcvdsacctlr, drvdata->base + TRCVDSACCTLR);
> > +	writel_relaxed(state->trcvdarcctlr, drvdata->base + TRCVDARCCTLR);
> > +
> > +	for (i = 0; i < drvdata->nrseqstate; i++)
> > +		writel_relaxed(state->trcseqevr[i],
> > +					drvdata->base + TRCSEQEVRn(i));
> 
> minor nit: alignment issues here and below for the multi-line
> write_relaxed() invocations.
> ...

OK.

> 
> 
> > +static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> > +			      void *v)
> > +{
> > +	struct etmv4_drvdata *drvdata;
> > +	unsigned int cpu = smp_processor_id();
> > +
> > +	if (!etmdrvdata[cpu])
> > +		return 0;
> 
> 
> Please could we be consistent with the return value. i.e, use something
> in line with NOTIFY_*. NOTIFY_OK ?

Yes I think NOTIFY_OK is the correct thing to do here.

> With the above fixed:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm•com>

Thanks for the review,

Andrew Murray 

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