MIME-Version: 1.0
References: <20230317115728.1358368-1-suzuki.poulose@arm.com>
In-Reply-To: <20230317115728.1358368-1-suzuki.poulose@arm.com>
From:   Mike Leach <mike.leach@linaro.org>
Date:   Tue, 21 Mar 2023 09:53:23 +0000
Message-ID: <CAJ9a7Vh8VgPtdFNf1Y+QX+US7tJDQC7QwEpF_1ObwoiFVi6S4Q@mail.gmail.com>
Subject: Re: [PATCH] coresight: etm4x: Do not access TRCIDR1 for identification
To:     Suzuki K Poulose <suzuki.poulose@arm.com>
Cc:     coresight@lists.linaro.org, james.clark@arm.com,
        anshuman.khandual@arm.com, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org,
        Steve Clevenger <scclevenger@os.amperecomputing.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 17 Mar 2023 at 11:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> CoreSight ETM4x architecture clearly provides ways to identify a device
> via registers in the "Management" class, TRCDEVARCH and TRCDEVTYPE. These
> registers can be accessed without the Trace domain being powered on.
> We additionally added TRCIDR1 as fallback in order to cover for any
> ETMs that may not have implemented TRCDEVARCH. So far, nobody has
> reported hitting a WARNING we placed to catch such systems.
>
> Also, more importantly it is problematic to access TRCIDR1, which is a "Trace"
> register via MMIO access, without clearing the OSLK. But we cannot
> mess with the OSLK until we know for sure that this is an ETMv4 device.
> Thus, this kind of creates a chicken and egg problem unnecessarily for systems
> "which are compliant" to the ETMv4 architecture.
>
> Let us remove the TRCIDR1 fall back check and rely only on TRCDEVARCH.
>
> Reported-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
> Link: https://lore.kernel.org/all/143540e5623d4c7393d24833f2b80600d8d745d2.1677881753.git.scclevenger@os.amperecomputing.com/
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: James Clark <james.clark@arm.com>
> Fixes: 8b94db1edaee ("coresight: etm4x: Use TRCDEVARCH for component discovery")
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  .../hwtracing/coresight/coresight-etm4x-core.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 104333c2c8a3..c1b72d892d7d 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1070,25 +1070,17 @@ static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
>                                    struct csdev_access *csa)
>  {
>         u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
> -       u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1);
>
>         /*
>          * All ETMs must implement TRCDEVARCH to indicate that
> -        * the component is an ETMv4. To support any broken
> -        * implementations we fall back to TRCIDR1 check, which
> -        * is not really reliable.
> +        * the component is an ETMv4
>          */
> -       if ((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) {
> -               drvdata->arch = etm_devarch_to_arch(devarch);
> -       } else {
> -               pr_warn("CPU%d: ETM4x incompatible TRCDEVARCH: %x, falling back to TRCIDR1\n",
> -                       smp_processor_id(), devarch);
> -
> -               if (ETM_TRCIDR1_ARCH_MAJOR(idr1) != ETM_TRCIDR1_ARCH_ETMv4)
> -                       return false;
> -               drvdata->arch = etm_trcidr_to_arch(idr1);
> +       if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH) {
> +               pr_warn_once("TRCDEVARCH doesn't match ETMv4 architecture\n");
> +               return false;
>         }
>
> +       drvdata->arch = etm_devarch_to_arch(devarch);
>         *csa = CSDEV_ACCESS_IOMEM(drvdata->base);
>         return true;
>  }
> --
> 2.34.1
>
Reviewed-by: Mike Leach <mike.leach@linaro.org>

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