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, 9 Nov 2020 11:32:55 -0700
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Suzuki K Poulose <suzuki.poulose@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org,
        coresight@lists.linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 22/26] coresight: etm4x: Add necessary synchronization
 for sysreg access
Message-ID: <20201109183255.GA3396611@xps15>
References: <20201028220945.3826358-1-suzuki.poulose@arm.com>
 <20201028220945.3826358-24-suzuki.poulose@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201028220945.3826358-24-suzuki.poulose@arm.com>
Precedence: bulk

On Wed, Oct 28, 2020 at 10:09:41PM +0000, Suzuki K Poulose wrote:
> As per the specification any update to the TRCPRGCTLR must be synchronized
> by a context synchronization event (in our case an explicist ISB) before
> the TRCSTATR is checked.
> 
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e36bc1c722c7..4bc2f15b6332 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -178,6 +178,15 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  	/* Disable the trace unit before programming trace registers */
>  	etm4x_relaxed_write32(csa, 0, TRCPRGCTLR);
>  
> +	/*
> +	 * If we use system instructions, we need to synchronize the
> +	 * write to the TRCPRGCTLR, before accessing the TRCSTATR.
> +	 * See ARM IHI0064F, section
> +	 * "4.3.7 Synchronization of register updates"
> +	 */
> +	if (!csa->io_mem)
> +		isb();
> +

When I first read the documentation on system instruction section 4.3.7 really
got me thinking...  

At the very top, right after the title "Synchronization of register updates" one
can read "Software running on the PE...".  Later in the text, when specifying
the synchronisation rules, the term "trace analyzer" is used.  _Typically_ a trace
analyzer is an external box.

Arm documentation is precise and usually doesn't overlook that kind of detail.
The question is to understand if a context synchronisation event is also needed
when programming is done on the PE.  If so I think the documentation should be
amended. 

In that case:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>


>  	/* wait for TRCSTATR.IDLE to go up */
>  	if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 1))
>  		dev_err(etm_dev,
> @@ -256,6 +265,10 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  	/* Enable the trace unit */
>  	etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);
>  
> +	/* Synchronize the register updates for sysreg access */
> +	if (!csa->io_mem)
> +		isb();
> +
>  	/* wait for TRCSTATR.IDLE to go back down to '0' */
>  	if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 0))
>  		dev_err(etm_dev,
> -- 
> 2.24.1
>