Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 31 Aug 2020 16:13:08 -0600
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Qi Liu <liuqi115@huawei.com>
Cc:     gregkh@linuxfoundation.org, suzuki.poulose@arm.com,
        mike.leach@linaro.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linuxarm@huawei.com
Subject: Re: [RFC PATCH v2] coresight: etm4x: Modify core-commit of cpu to
 avoid the overflow of HiSilicon ETM
Message-ID: <20200831221308.GB207013@xps15>
References: <1597824397-29894-1-git-send-email-liuqi115@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1597824397-29894-1-git-send-email-liuqi115@huawei.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 <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

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

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.

> +
>  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

> 
>  	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
>