[PATCH 1/3] coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read
Steve Clevenger
scclevenger at os.amperecomputing.com
Mon Jan 23 11:48:02 PST 2023
On 1/23/2023 9:33 AM, Suzuki K Poulose wrote:
> 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.
I don't understand your postscript. Certainly there's driver work to be
done, but I also think the DEN0067 CoreSight ACPI specification needs
update. There's no example for defining a trace implementation without
memory mapped component descriptions. Considering the ACPI as it exists,
the component graphs are the most important. Please see my last exchange
with Mike Leach.
My patches were submitted based on the existing CoreSight driver, not
what it should be. Ampere does not have the flexibility to wait for a
decision on some details we've discussed. I'm available to work through
any concerns the maintainers have with my submissions. What is the best
way forward here?
> 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