On Mon, 31 Aug 2020 at 19:57, Qi Liu <liuqi...@huawei.com> wrote:
>
> Hi Mathieu,
>
> Thanks for your review.
>
> On 2020/9/1 6:13, Mathieu Poirier wrote:
> > Following Al's comment I have the following recommendations...
> >
> > 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
> > Do you have a name for the SoC that can be added so that other HiSilicon
> > SoC can
> > be added? I suggest something like:
> >
> > #define HISI_NAME_CORE_COMMIT_CLEAR
> > #define HISI_NAME_CORE_COMMIT_SHIFT
> > #define HISI_NAME_ETM_ID
> Will fix this next version.
> > Moreover I don't see an entry for 0x000b6d01 in the amba id table - is your
> > devices upstream? Needless to day that is mandatory in order to move
> > forward
> > with this work.
> A patch has been applied to add this ETM id and here is the link:
> https://lore.kernel.org/linux-arm-kernel/20200813210000.GO3393195@xps15/
>
Ah yes - my tree was on the wrong baseline when I reviewed your patch.
You can forget about this comment.
Thanks,
Mathieu
> >> +
> >> 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));
> >> + 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);
> >> +}
> >> +
> >> 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)
> >> + etm4_config_core_commit(drvdata->cpu, 1);
> > I susggest to add a function like etm4_enable_arch_specific() and do the
> > above
> > in there. The same goes for the disable path.
> >
> > Thanks,
> > Mathieu
> Thanks, I'll fix this next version : )
>
> Qi
> >
> >> CS_UNLOCK(drvdata->base);
> >>
> >> @@ -472,10 +505,20 @@ static void etm4_disable_hw(void *info)
> >> {
> >> u32 control;
> >> struct etmv4_drvdata *drvdata = info;
> >> + struct device *dev = drvdata->csdev->dev.parent;
> >> struct etmv4_config *config = &drvdata->config;
> >> struct device *etm_dev = &drvdata->csdev->dev;
> >> + struct amba_device *adev;
> >> int i;
> >>
> >> + adev = container_of(dev, struct amba_device, dev);
> >> + /*
> >> + * If ETM device is HiSilicon ETM device, resume the
> >> + * core-commit after ETM trace is complete.
> >> + */
> >> + if (adev->periphid == HISI_ETM_AMBA_ID_V1)
> >> + etm4_config_core_commit(drvdata->cpu, 0);
> >> +
> >> CS_UNLOCK(drvdata->base);
> >>
> >> if (!drvdata->skip_power_up) {
> >> --
> >> 2.8.1
> >>
> > .
> >
>
>
