[PATCH 1/3] coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read

[Suzuki K Poulose]

Hi Steve

Thanks for the patches. Have a few comments below.

On 20/01/2023 00:51, Steve Clevenger wrote:
> Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access.
> Ampere Computing erratum AC03_DEBUG_06 describes an Ampere
> Computing design decision MMIO reads are considered the same as an
> external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access
> results in a bus fault followed by a kernel panic.  A TRCIDR1 read
> is valid regardless of TRCOSLAR.OSLK provided MMIO access
> (now deprecated) is supported.
> AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
> https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation

Please could you add this erratum to the :

Documentation/arm64/silicon-errata.rst ?

Given the ETM is v4.6, doesn't it support system instructions and
that is causing this issue of "MMIO access is considered external" ?
If it does, I think we should drop all of this and simply wire the
system instruction access support.

> 
> Add Ampere ETM PID required for Coresight ETM driver support.
> 
> Signed-off-by: Steve Clevenger <scclevenger at os.amperecomputing.com>
> ---
>   .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
>   drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
>   2 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 1cc052979e01..533be1928a09 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
>   	drvdata = dev_get_drvdata(init_arg->dev);
>   	csa = init_arg->csa;
>   
> +	/* Detect the support for OS Lock before we actually use it */
> +	etm_detect_os_lock(drvdata, csa);
> + > +	/*
> +	 * For ETM implementations that consider MMIO an external access
> +	 * clear TRCOSLAR.OSLK early.
> +	 */
> +	if (drvdata->mmio_external)
> +		etm4_os_unlock_csa(drvdata, csa);
> +
>   	/*
>   	 * If we are unable to detect the access mechanism,
>   	 * or unable to detect the trace unit type, fail
> -	 * early.
> +	 * early. Reset TRCOSLAR.OSLK if cleared.
>   	 */
> -	if (!etm4_init_csdev_access(drvdata, csa))
> +	if (!etm4_init_csdev_access(drvdata, csa)) {
> +		if (drvdata->mmio_external)
> +			etm4_os_lock(drvdata);

Couldn't this unlock/lock sequence be moved into the 
etm4_init_csdev_iomem_access() where it actually matters ?

Or thinking more about it, we could actually move the unlock step early
for all ETMs irrespective of whether they are affected by this erratum.
Of course, putting this back, if we fail to detect the ETM properly.
I don't see any issue with that.

>   		return;
> +	}
>   
> -	/* Detect the support for OS Lock before we actually use it */
> -	etm_detect_os_lock(drvdata, csa);
> +	/*
> +	 * Make sure all registers are accessible
> +	 * TRCOSLAR.OSLK may already be clear
> +	 */
> +	if (!drvdata->mmio_external)
> +		etm4_os_unlock_csa(drvdata, csa);
>   
> -	/* Make sure all registers are accessible */
> -	etm4_os_unlock_csa(drvdata, csa);
>   	etm4_cs_unlock(drvdata, csa);
>   
>   	etm4_check_arch_features(drvdata, init_arg->pid);
> @@ -2027,6 +2042,14 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>   	init_arg.csa = &access;
>   	init_arg.pid = etm_pid;
>   
> +	/*
> +	 * Ampere ETM v4.6 considers MMIO access as external. This mask
> +	 * isolates the manufacturer JEP106 ID in the PID.
> +	 * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
> +	 */

Does it affect all Ampere ETMs ? You seem to be ignoring the 
PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0.

> +	if ((init_arg.pid & 0x000FF000) == 0x00096000)
> +		drvdata->mmio_external = true;
Like I said, we may be able to get rid of this flag and do the step for 
all ETMs. But before all of that, I would like to see if this is problem
because we are skipping the system instruction route.

Suzuki

>   	/*
>   	 * Serialize against CPUHP callbacks to avoid race condition
>   	 * between the smp call and saving the delayed probe.
> @@ -2192,6 +2215,7 @@ static const struct amba_id etm4_ids[] = {
>   	CS_AMBA_ID(0x000bb95e),			/* Cortex-A57 */
>   	CS_AMBA_ID(0x000bb95a),			/* Cortex-A72 */
>   	CS_AMBA_ID(0x000bb959),			/* Cortex-A73 */
> +	CS_AMBA_UCI_ID(0x00096000, uci_id_etm4),/* Ampere ARMv8 */
>   	CS_AMBA_UCI_ID(0x000bb9da, uci_id_etm4),/* Cortex-A35 */
>   	CS_AMBA_UCI_ID(0x000bbd05, uci_id_etm4),/* Cortex-A55 */
>   	CS_AMBA_UCI_ID(0x000bbd0a, uci_id_etm4),/* Cortex-A75 */
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 4b21bb79f168..cf4f9f2e1807 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -1015,6 +1015,7 @@ struct etmv4_save_state {
>    * @skip_power_up: Indicates if an implementation can skip powering up
>    *		   the trace unit.
>    * @arch_features: Bitmap of arch features of etmv4 devices.
> + * @mmio_external: True if ETM considers MMIO an external access.
>    */
>   struct etmv4_drvdata {
>   	void __iomem			*base;
> @@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
>   	bool				state_needs_restore;
>   	bool				skip_power_up;
>   	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
> +	bool				mmio_external;
>   };
>   
>   /* Address comparator access types */