[PATCH 1/3] coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read
Suzuki K Poulose
suzuki.poulose at arm.com
Mon Jan 23 09:33:42 PST 2023
On 23/01/2023 17:22, Steve Clevenger wrote:
>
>
> 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.
So, I don't understand why we are pushing towards enabling the
"obsolete" MMIO interface ? Is this because "ACPI" mandates it ?
Then, please don't. The spec needs an update to reflect the ETMs
with sysreg access and ETEs.
Why not stick to the system register access* ?
* PS: The ACPI support for the ETM/ETE needs additional changes to the
CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on
this at the moment and will be available soon.
The hack patch below should be sufficient to give it a try and if it works.
Kind regards
Suzuki
>
>>
>> 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