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 3/3] coresight: etm4x: save/restore state for external agents
Date: Fri, 13 Sep 2019 11:32:53 +0100 [thread overview]
Message-ID: <20190913103252.GL9720@e119886-lin.cambridge.arm.com> (raw)
In-Reply-To: <4b34efe9-7266-30b6-6a8a-8a85bacfe5a3@arm.com>
On Thu, Sep 12, 2019 at 04:35:20PM +0100, Suzuki K Poulose wrote:
> Hi Andrew
>
> On 16/08/2019 16:46, 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. Much like
> > self-hosted debug, we should also save/restore the trace unit state when
> > it is in use by external agents.
> >
> > We wish to avoid saving the hardware state when coresight isn't in use
> > to reduce PM latency - However as external trace/debug is designed to be
> > unintrusive to the CPU, the only way of determining that an external agent is
> > present is to read the claim tags (TRCCLAIMCLR). Unfortunately this register
> > needs power and clocking - something it won't have when coresight isn't in use.
> > We also don't want to temporarily enable it due to the latency and PM context.
> >
> > Let's compromise by adding a module parameter that will keep the trace unit
> > powered and clocked, thus allowing us to only save/restore state when external
> > trace (or self-hosted) is in use. Though please note that this doesn't allow
> > for tracing from boot on hardware that needs save/restore as the CPU may idle
> > prior to the ETMv4 driver starting and adding PM hooks to save/restore.
> >
>
> This looks fine to me. Some minor comments below.
>
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> > drivers/hwtracing/coresight/coresight-etm4x.c | 27 ++++++++++++++++---
> > drivers/hwtracing/coresight/coresight.c | 2 +-
> > include/linux/coresight.h | 7 +++++
> > 3 files changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> > index 35a524eec36d..c5d527f7cbd5 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> > @@ -42,11 +42,12 @@ MODULE_PARM_DESC(boot_enable, "Enable tracing on boot");
> > #define PARAM_PM_SAVE_FIRMWARE 0 /* save self-hosted state as per firmware */
> > #define PARAM_PM_SAVE_NEVER 1 /* never save any state */
> > #define PARAM_PM_SAVE_SELF_HOSTED 2 /* save self-hosted state only */
> > +#define PARAM_PM_SAVE_EXTERNAL 3 /* save all state (keeps power on) */
>
> Should we say PARAM_PM_SAVE_ALWAYS instead ?
>
Yes I think this is OK.
> > static int pm_save_enable = PARAM_PM_SAVE_FIRMWARE;
> > module_param(pm_save_enable, int, 0444);
> > MODULE_PARM_DESC(pm_save_enable,
> > - "Save/restore state on power down: 1 = never, 2 = self-hosted");
> > + "Save/restore state on power down: 1 = never, 2 = self-hosted, 3 = self-hosted/external");
>
> similarly here and also mention that the power/clocks are not dropped in that
> case ? I see the comment above, but please could we make it more explicit ?
I'll change it to ... 3 = self-hosted/external (keeps power on)");
>
> > /* The number of ETMv4 currently registered */
> > static int etm4_count;
> > @@ -1331,6 +1332,22 @@ static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> > CS_LOCK(drvdata->base);
> > }
> > +static bool etm4_coresight_in_use(struct etmv4_drvdata *drvdata)
> > +{
> > + /* Self-hosted session in progress? */
> > + if (local_read(&drvdata->mode))
> > + return true;
> > +
> > + /* External agents can be detected through claim tags however we
> > + * only read these tags if the trace unit is powered.
> > + */
> > + if (drvdata->csdev && pm_runtime_active(drvdata->csdev->dev.parent))
> > + if (coresight_is_claimed_any(drvdata->base))
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> > void *v)
> > {
> > @@ -1350,8 +1367,8 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> > switch (cmd) {
> > case CPU_PM_ENTER:
> > - /* save the state if self-hosted coresight is in use */
> > - if (local_read(&drvdata->mode))
> > + /* Save the state if coresight is in use */
> > + if (etm4_coresight_in_use(drvdata))
> > if (etm4_cpu_save(drvdata))
> > return NOTIFY_BAD;
> > break;
> > @@ -1488,7 +1505,9 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
> > goto err_arch_supported;
> > }
> > - pm_runtime_put(&adev->dev);
> > + if (pm_save_enable != PARAM_PM_SAVE_EXTERNAL)
> > + pm_runtime_put(&adev->dev);
> > +
>
> It may be a good idea to explain why we don't drop the power here
> in a comment to help people reading the code. You could paste what
> is in the commit description in here to avoid another lookup.
Good idea. I'll modify it slightly however.
>
> > dev_info(&drvdata->csdev->dev, "CPU%d: ETM v%d.%d initialized\n",
> > drvdata->cpu, drvdata->arch >> 4, drvdata->arch & 0xf);
> > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > index e6ca899fea4e..474b7372864e 100644
> > --- a/drivers/hwtracing/coresight/coresight.c
> > +++ b/drivers/hwtracing/coresight/coresight.c
> > @@ -140,7 +140,7 @@ static inline bool coresight_is_claimed_self_hosted(void __iomem *base)
> > return coresight_read_claim_tags(base) == CORESIGHT_CLAIM_SELF_HOSTED;
> > }
> > -static inline bool coresight_is_claimed_any(void __iomem *base)
> > +bool coresight_is_claimed_any(void __iomem *base)
> > {
> > return coresight_read_claim_tags(base) != 0;
> > }
>
> minor nit: We may retain this as static inline and move this to the header file.
OK.
Thanks,
Andrew Murray
>
> Kind regards
> Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
prev parent reply other threads:[~2019-09-13 10:33 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-16 15:46 [PATCH v5 0/3] coresight: etm4x: save/restore ETMv4 context across CPU low power states Andrew Murray
2019-08-16 15:46 ` [PATCH v5 1/3] coresight: etm4x: save/restore state " Andrew Murray
2019-08-20 21:55 ` Mathieu Poirier
2019-09-13 8:50 ` Andrew Murray
2019-09-12 14:03 ` Suzuki K Poulose
2019-09-13 9:20 ` Andrew Murray
2019-08-16 15:46 ` [PATCH v5 2/3] dt-bindings: arm: coresight: Add support for coresight-loses-context-with-cpu Andrew Murray
2019-08-20 21:59 ` Mathieu Poirier
2019-09-13 9:22 ` Andrew Murray
2019-09-12 14:06 ` Suzuki K Poulose
2019-08-16 15:46 ` [PATCH v5 3/3] coresight: etm4x: save/restore state for external agents Andrew Murray
2019-08-20 22:01 ` Mathieu Poirier
2019-09-12 15:35 ` Suzuki K Poulose
2019-09-13 10:32 ` Andrew Murray [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190913103252.GL9720@e119886-lin.cambridge.arm.com \
--to=andrew.murray@arm.com \
--cc=Al.Grant@arm.com \
--cc=Sudeep.Holla@arm.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=coresight@lists.linaro.org \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@linaro.org \
--cc=suzuki.poulose@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).