On 2020/11/12 1:03, Mathieu Poirier wrote:
On Wed, Nov 11, 2020 at 04:58:23PM +0800, Qi Liu wrote:
Hi Mathieu,
On 2020/9/10 0:26, Mathieu Poirier wrote:
On Wed, Sep 09, 2020 at 12:30:02PM +0100, Mike Leach wrote:
Hi,
On Wed, 2 Sep 2020 at 11:36, Suzuki K Poulose suzuki.poulose@arm.com wrote:
On 08/27/2020 09:44 PM, Mathieu Poirier wrote:
Hi Liu,
On Wed, Aug 19, 2020 at 04:06:37PM +0800, Qi Liu wrote: > When too much trace information is generated on-chip, the ETM will > overflow, and cause data loss. This is a common phenomenon on ETM > devices. > > But sometimes we do not want to lose performance trace data, so we > suppress the speed of instructions sent from CPU core to ETM to > avoid the overflow of ETM. > > Signed-off-by: Qi Liu liuqi115@huawei.com > --- > > Changes since v1: > - ETM on HiSilicon Hip09 platform supports backpressure, so does > not need to modify core commit. > > drivers/hwtracing/coresight/coresight-etm4x.c | 43 +++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c > index 7797a57..7641f89 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > @@ -43,6 +43,10 @@ MODULE_PARM_DESC(boot_enable, "Enable tracing on boot"); > #define PARAM_PM_SAVE_NEVER 1 /* never save any state */ > #define PARAM_PM_SAVE_SELF_HOSTED 2 /* save self-hosted state only */ > > +#define CORE_COMMIT_CLEAR 0x3000 > +#define CORE_COMMIT_SHIFT 12 > +#define HISI_ETM_AMBA_ID_V1 0x000b6d01 > + > static int pm_save_enable = PARAM_PM_SAVE_FIRMWARE; > module_param(pm_save_enable, int, 0444); > MODULE_PARM_DESC(pm_save_enable, > @@ -104,11 +108,40 @@ struct etm4_enable_arg { > int rc; > }; > > +static void etm4_cpu_actlr1_cfg(void *info) > +{ > + struct etm4_enable_arg *arg = (struct etm4_enable_arg *)info; > + u64 val; > + > + asm volatile("mrs %0,s3_1_c15_c2_5" : "=r"(val));
The ID register (S3_1_C15_c2_5) falls into implementation defined space. See Arm ARM DDI 0487F.a, section "D12.3.2 Reserved encodings for IMPLEMENTATION DEFINED registers".
So, please stop calling this "etm4_cpu_actlr1_cfg". Since,
- actlr1 is not an architected name for the said encoding
- The id register could mean something else on another CPU.
Rather this should indicate platform/CPU specific. e.g,
etm4_cpu_hisilicon_config_core_commit()
> + val &= ~CORE_COMMIT_CLEAR; > + val |= arg->rc << CORE_COMMIT_SHIFT; > + asm volatile("msr s3_1_c15_c2_5,%0" : : "r"(val)); > +} > + > +static void etm4_config_core_commit(int cpu, int val) > +{ > + struct etm4_enable_arg arg = {0}; > + > + arg.rc = val; > + smp_call_function_single(cpu, etm4_cpu_actlr1_cfg, &arg, 1); Function etm4_enable/disable_hw() are already running on the CPU they are supposed to so no need to call smp_call_function_single().
> +} > + > static int etm4_enable_hw(struct etmv4_drvdata *drvdata) > { > int i, rc; > + struct amba_device *adev; > struct etmv4_config *config = &drvdata->config; > struct device *etm_dev = &drvdata->csdev->dev; > + struct device *dev = drvdata->csdev->dev.parent; > + > + adev = container_of(dev, struct amba_device, dev); > + /* > + * If ETM device is HiSilicon ETM device, reduce the > + * core-commit to avoid ETM overflow. > + */ > + if (adev->periphid == HISI_ETM_AMBA_ID_V1)
Please could you move this check to the function above ? Ideally, it would be good to have something like :
etm4_config_impdef_features();And :
etm4_config_impdef_features(struct etm4_drvdata *drvdata) { etm4_cpu_hisilicon_config_core_commit(drvdata); }In addition to the above, Is it worth having this implementation defined code gated in the kernel configuration - like we do for core features sometimes? i,.e. CONFIG_ETM4X_IMPDEF_FEATURE (controls overall impdef support in the driver) and CONFIG_ETM4X_IMPDEF_HISILICON (depends on CONFIG_ETM4X_IMPDEF_FEATURE )
This way we keep non ETM architectural code off platforms that cannot use it / test it.
That's a good idea - they do the same for CPU erratas.
Considering that users sometimes use the same set of code on different platforms, how about using both CONFIG andperiphid to keep core-commit code off the platforms that do not need it? i, .e. CONFIG_ETM4X_IMPDEF_FEATURE ( controls overall impdef support in the driver ) periphid ( match impdef code with the target platform )
This way we could keep the same set of code working on different platforms, and it could help to ensure compatibility.
I'm not 100% sure of what you mean by "same set of code working on different platforms"... Up to know the way we have been handling peripheral IDs has worked quite well and I don't intend on changing it unless there is a really good reason.
Hi Mathieu,
Perhaps I didn't make it clear and here is the code to express what I mean:
#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
#define HISI_HIP08_AMBA_ID 0x000b6d01 #define HISI_HIP08_AMBA_MASK 0xfffff static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata) { struct device *dev = drvdata->csdev->dev.parent; struct amba_device *adev = container_of(dev, struct amba_device, dev);
/* * If ETM device is HiSilicon ETM device, reduce the * core-commit to avoid ETM overflow. */ if (adev->periphid & HISI_HIP08_AMBA_MASK == HISI_HIP08_AMBA_ID) etm4_hisi_config_core_commit(1); }
#else static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata) { }
#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
Thanks Qi
I'll update this patch if this solution is ok : )
Thanks! Qi
Do you have any documentation on this back pressure feature? I doubt this is specific to Hip09 platform and as such would prefer to have a more generic approach that works on any platform that supports it.
Anyone on the CS mailing list that knows what this is about?
I believe this is hisilicon specific. May be same across their CPUs, may be a specific one. There is no architectural guarantee.
This could be an implementation of the feature provided by the TRCSTALLCTRL register - which allows PE to be stalled in response to the ETM fifos approaching overflow. At present we do nothing with this feature as we have yet to see a target with it implemented, but if this is the case then it is an ETMv4 architectural feature that could be added into the main driver code, with use/access gated by the relevent TRCIDR bit.
Regards
Mike
Cheers Suzuki
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
.
.