Received-SPF: pass (google.com: domain of linux-kernel+bounces-18049-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240105055840.1977897-1-lcherian@marvell.com> <20240105055840.1977897-7-lcherian@marvell.com>
In-Reply-To: <20240105055840.1977897-7-lcherian@marvell.com>
From: Mike Leach <mike.leach@linaro.org>
Date: Fri, 5 Jan 2024 16:11:18 +0000
Message-ID: <CAJ9a7VhStuDsNz-BiVe-bW5E91UxQttKTbE4x+M+8NmdEKtAJw@mail.gmail.com>
Subject: Re: [PATCH v6 6/7] coresight: tmc: Stop trace capture on FlIn
To: Linu Cherian <lcherian@marvell.com>
Cc: suzuki.poulose@arm.com, james.clark@arm.com, leo.yan@linaro.org, 
	linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org, 
	linux-kernel@vger.kernel.org, robh+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
	devicetree@vger.kernel.org, sgoutham@marvell.com, gcherian@marvell.com
Content-Type: text/plain; charset="UTF-8"

Hi,

On Fri, 5 Jan 2024 at 05:59, Linu Cherian <lcherian@marvell.com> wrote:
>
> Configure TMC ETR and ETF to flush and stop trace capture
> on FlIn event. As a side effect, do manual flush only if
> auto flush fails.
>
> Signed-off-by: Linu Cherian <lcherian@marvell.com>
> ---
> Changelog from v5:
> * No changes
>
>  drivers/hwtracing/coresight/coresight-tmc-etf.c | 10 ++++++++--
>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 10 ++++++++--
>  drivers/hwtracing/coresight/coresight-tmc.h     |  3 +++
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 72c2315f4e23..57a9a9300d36 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -34,7 +34,7 @@ static int __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>         writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
>         writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
>                        TMC_FFCR_FON_FLIN | TMC_FFCR_FON_TRIG_EVT |
> -                      TMC_FFCR_TRIGON_TRIGIN,
> +                      TMC_FFCR_TRIGON_TRIGIN | TMC_FFCR_STOP_ON_FLUSH,
>                        drvdata->base + TMC_FFCR);
>

This is a problem. Setting TMC_FFCR_STOP_ON_FLUSH changes the
fundamentals of trigger event handling. Without this bit ETM can
generate multiple event  + triggers which are then embedded into the
formatted trace stream for later search.
With this new bit the capture will stop on the first event. Setting
this bit should be dependent on the mode being set to ETR_MODE_RESRV


>         writel_relaxed(drvdata->trigger_cntr, drvdata->base + TMC_TRG);
> @@ -615,7 +615,13 @@ static int tmc_panic_sync_etf(struct coresight_device *csdev)
>         if (val != TMC_MODE_CIRCULAR_BUFFER)
>                 goto out;
>
> -       tmc_flush_and_stop(drvdata);
> +       val = readl(drvdata->base + TMC_FFSR);
> +       /* Do manual flush and stop only if its not auto-stopped */
> +       if (!(val & TMC_FFSR_FT_STOPPED)) {
> +               dev_info(&csdev->dev,
> +                        "%s: Triggering manual flush\n", __func__);
> +               tmc_flush_and_stop(drvdata);
> +       }
>
Is there some reason to believe that the stop on flush will not work?

Using this conditional skips the  tmc_wait_for_tmcready() called by
tmc_flush_and_stop() if the formatter is stopped - which is a
different condition test on a different register.

Why is this block of code not in the patch that introduces the
tmc_panic_sync_etf()

>         /* Sync registers from hardware to metadata region */
>         mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index ab7521bbb2f5..4b3c7ec7f62b 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1113,7 +1113,7 @@ static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
>
>         writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
>                        TMC_FFCR_FON_FLIN | TMC_FFCR_FON_TRIG_EVT |
> -                      TMC_FFCR_TRIGON_TRIGIN,
> +                      TMC_FFCR_TRIGON_TRIGIN | TMC_FFCR_STOP_ON_FLUSH,
>                        drvdata->base + TMC_FFCR);
>         writel_relaxed(drvdata->trigger_cntr, drvdata->base + TMC_TRG);
>         tmc_enable_hw(drvdata);
> @@ -1846,7 +1846,13 @@ static int tmc_panic_sync_etr(struct coresight_device *csdev)
>         if (!(val & TMC_CTL_CAPT_EN))
>                 goto out;
>
> -       tmc_flush_and_stop(drvdata);
> +       val = readl(drvdata->base + TMC_FFSR);
> +       /* Do manual flush and stop only if its not auto-stopped */
> +       if (!(val & TMC_FFSR_FT_STOPPED)) {
> +               dev_info(&csdev->dev,
> +                        "%s: Triggering manual flush\n", __func__);
> +               tmc_flush_and_stop(drvdata);
> +       }
>

Above comments for etf apply equally to etr.

Regards

Mike

>         /* Sync registers from hardware to metadata region */
>         mdata->size = csdev_access_relaxed_read32(csa, TMC_RSZ);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 6e1e910d5ea4..cf9313b302c7 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -77,6 +77,9 @@
>  #define TMC_AXICTL_AXCACHE_OS  (0xf << 2)
>  #define TMC_AXICTL_ARCACHE_OS  (0xf << 16)
>
> +/* TMC_FFSR - 0x300 */
> +#define TMC_FFSR_FT_STOPPED    BIT(1)
> +
>  /* TMC_FFCR - 0x304 */
>  #define TMC_FFCR_FLUSHMAN_BIT  6
>  #define TMC_FFCR_EN_FMT                BIT(0)
> --
> 2.34.1
>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK