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.poul...@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 <liuqi...@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, > >>> 1) actlr1 is not an architected name for the said encoding > >>> 2) 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. > 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 > > . > > >