[PATCH 1/3] coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read
Steve Clevenger
scclevenger at os.amperecomputing.com
Mon Jan 23 09:22:26 PST 2023
On 1/23/2023 2:54 AM, Suzuki K Poulose wrote:
> On 21/01/2023 07:30, Steve Clevenger wrote:
>>
>> Hi Suzuki,
>>
>> Comments in-line. Please note the approach I attempted while adding in
>> the Ampere support was to otherwise not disturb existing driver code for
>> non-Ampere parts.
>>
>> Steve
>>
>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote:
>>> 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.
>
>> That's not the issue in this case. This MMIO access should've been
>> allowed by the Ampere ETMv4.6 implementation. Based on comments I've
>
> That doesn't answe the question. Please could you confirm the value of
> ID_AA64DFR0_EL1 on your system ?
This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected
to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So,
TraceVer, bits [7:4] are b0001. My understanding is the system register
interface must be implemented on all ETMv4.6 parts.
>
> Or, are you able to try this on your ACPI based system and see if you
> are able to use the etm ? (UNTESTED hack !)
>
>
> diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c
> index f5b443ab01c2..099966cbac5a 100644
> --- a/drivers/acpi/acpi_amba.c
> +++ b/drivers/acpi/acpi_amba.c
> @@ -22,7 +22,6 @@
> static const struct acpi_device_id amba_id_list[] = {
> {"ARMH0061", 0}, /* PL061 GPIO Device */
> {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */
> - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
> {"ARMHC501", 0}, /* ARM CoreSight ETR */
> {"ARMHC502", 0}, /* ARM CoreSight STM */
> {"ARMHC503", 0}, /* ARM CoreSight Debug */
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 1ea8f173cca0..66670533fd54 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -3,6 +3,7 @@
> * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> */
>
> +#include <linux/acpi.h>
> #include <linux/bitops.h>
> #include <linux/kernel.h>
> #include <linux/moduleparam.h>
> @@ -2286,12 +2287,22 @@ static const struct of_device_id
> etm4_sysreg_match[] = {
> {}
> };
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id etm4x_acpi_ids[] = {
> + {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids);
> +#endif
> +
> static struct platform_driver etm4_platform_driver = {
> .probe = etm4_probe_platform_dev,
> .remove = etm4_remove_platform_dev,
> .driver = {
> .name = "coresight-etm4x",
> .of_match_table = etm4_sysreg_match,
> + .acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
> .suppress_bind_attrs = true,
> },
> };
>
>
>
>
>> read in the driver code, the MMIO read access to TRCIDR1 occurs after a
>> TRCDEVARCH access. The comments suggest this was to accommodate
>> potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere
>> ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless
>> the TRCOSLAR.OSLK lock is cleared first.
>>
>>>
>>>>
>>>> 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.
>
>
>> I agree the lock could be cleared earlier in the code. That's what this
>> patch does for Ampere. If it's decided ok to do for other (or all)
>> manufacturers, then the Ampere specific ID check goes away in this
>> place. The Ampere ID check (and flag) to determine whether the [Patch
>> 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or
>> use an existing feature mask as suggested by Mike Leach in a later
>> review.
>>
>>>
>>>> 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.
>
>> This is the first Ampere ETMv4.x implementation. I wrote the ID check
>> like this specifically because Ampere does not intend to address this
>> for ETM designs in progress.
>
> I would recommend to make this mask stricter and apply this to the
> current implementation. When there are more, we could add this here,
> rather than having to leave this work around for all the possible cores.
>
>>
>>>
>>>> + 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.
>>>
>
>> We understand MMIO access is deprecated going forward. There is other
>> Linux code to be concerned about. For example, AMBA code reads the
>> component PID/CID. This discovery code uses mapped values digested from
>> the CoreSight ACPI which are the descriptions and graphs for the
>
> With the "proposed" ACPI support for system register, AMBA would not be
> involved at all.
>
>> manufacturer trace implementation. There may be other Linux code I'm not
>> aware. Note the ASL examples in ARM Document number: DEN0067 specify
>> MMIO locations for every CoreSight component.
>
> Yes, but this was never updated to cover the system register based
> implementations. I will chase this up.
>
>
> Suzuki
>
More information about the linux-arm-kernel
mailing list