| Message ID | 20190816154615.39854-4-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm4x: save/restore ETMv4 context across CPU low power states | expand |
On Fri, Aug 16, 2019 at 04:46:15PM +0100, 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. > > 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) */ > > 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"); > > /* 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. > + */ Multi-line comment format error > + 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); > + > 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; > } > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 44e552de419c..65bfd2cb0283 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -286,6 +286,8 @@ extern void coresight_disclaim_device_unlocked(void __iomem *base); > extern char *coresight_alloc_device_name(struct coresight_dev_list *devs, > struct device *dev); > > +extern bool coresight_is_claimed_any(void __iomem *base); > + > extern bool coresight_loses_context_with_cpu(struct device *dev); > #else > static inline struct coresight_device * > @@ -309,6 +311,11 @@ static inline int coresight_claim_device(void __iomem *base) > static inline void coresight_disclaim_device(void __iomem *base) {} > static inline void coresight_disclaim_device_unlocked(void __iomem *base) {} > > +static inline bool coresight_is_claimed_any(void __iomem *base) > +{ > + return false; > +} > + > static inline bool coresight_loses_context_with_cpu(struct device *dev) > { > return false; > -- > 2.21.0 >
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 ? > > 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 ? > > /* 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. > 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. Kind regards Suzuki
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
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) */ 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"); /* 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); + 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; } diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 44e552de419c..65bfd2cb0283 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -286,6 +286,8 @@ extern void coresight_disclaim_device_unlocked(void __iomem *base); extern char *coresight_alloc_device_name(struct coresight_dev_list *devs, struct device *dev); +extern bool coresight_is_claimed_any(void __iomem *base); + extern bool coresight_loses_context_with_cpu(struct device *dev); #else static inline struct coresight_device * @@ -309,6 +311,11 @@ static inline int coresight_claim_device(void __iomem *base) static inline void coresight_disclaim_device(void __iomem *base) {} static inline void coresight_disclaim_device_unlocked(void __iomem *base) {} +static inline bool coresight_is_claimed_any(void __iomem *base) +{ + return false; +} + static inline bool coresight_loses_context_with_cpu(struct device *dev) { return false;
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. 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(-)