Date:   Wed, 9 Sep 2020 10:26:05 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Mike Leach <mike.leach@...aro.org>
Cc:     Suzuki K Poulose <suzuki.poulose@....com>, liuqi115@...wei.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linuxarm@...wei.com, Coresight ML <coresight@...ts.linaro.org>
Subject: Re: [RFC PATCH v2] coresight: etm4x: Modify core-commit of cpu to
 avoid the overflow of HiSilicon ETM

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@....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@...wei.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.
 
> 
> > >
> > > 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

Powered by blists - more mailing lists