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

[Suzuki K Poulose]

Hi Steve

fyi, the ACPI spec for CoreSight has been updated to relax the
MMIO region requirement for ETMv4x and ETE. Please find it here :

https://developer.arm.com/documentation/den0067/latest


Kind regards
Suzuki


On 02/02/2023 23:03, Steve Clevenger wrote:
> 
> 
> On 2/2/2023 9:27 AM, Suzuki K Poulose wrote:
>> On 02/02/2023 17:12, Steve Clevenger wrote:
>>> Hi Suzuki,
>>>
>>> Commented in-line.
>>>
>>> Steve C.
>>>
>>> On 2/2/2023 3:16 AM, Suzuki K Poulose wrote:
>>>> On 02/02/2023 05:20, Steve Clevenger wrote:
>>>>>
>>>>> Hi Suzuki,
>>>>>
>>>>> I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch
>>>>
>>>> Thanks for that.
>>>>
>>>>> and added references to the Ampere erratum in silicon-errata.rst.
>>>>> These will be submitted as separate patches.
>>>>>
>>>>> The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem
>>>>> for all manufacturers. I think this is what you asked for.
>>>>> The no_quad_mmio flag has moved to struct csdev_access, and the split
>>>>> 64-bit read/write logic has been implemented entirely in the header
>>>>> file
>>>>> coresight-etm4x.h is the existing calls.
>>>>> I'd like to retire this patch thread, and submit these as a new thread.
>>>>> Is there an objection?
>>>>
>>>> I would still like to use the system instructions method for the ETM,
>>>> than hacking the MMIO access for something that is obsolete.
>>>> The ACPI document for CoreSight will be published soon for review to
>>>> accommodate the description for ETMs without MMIO and it no longer
>>>> mandates the MemoryResource.
>>>>
>>>> What is the objection to using system instruction access here ?
>>> No objection going forward. For our initial release, we're not in a
>>> position to change the CoreSight DSDT based on a specification which is
>>> incomplete.
>>
>> There is on change to the CoreSight DSDT specification as such. The only
>> change to the "spec" is along the lines of :
>>
>> "MMIO interface is mandatory only if not accessible via system
>> instruction access "
>>
>>
>>> Based on a quick sysreg only build, I didn't collect trace samples. I
>>> haven't had time to chase this, but the reported error is "timeout while
>>> waiting for Trace Idle Status" on a TRCSTATR read. More testing is
>>
>> Are you able to access the other registers ?
>>
>> e.g,
>>
>> $ cat /sys/bus/coresight/devices/etm0/mgmt/trcpidr0
>>
> I'll need to determine which of the 3 coresight_timeout calls which read
> the TRCSTATR idle bit times out. In each case a number of sysreg
> accesses have occurred prior to any of these 3 calls.
> 
>> Suzuki
>>
>>
>>> required. If this isn't related to an Ampere sysreg access problem
>>> (doubtful), the next place I'd look is as a synchronization issue.
>>>
>>>>
>>>> Thanks
>>>> Suzuki
>>>>
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Steve
>>>>>
>>>>> On 1/23/2023 2:51 PM, Suzuki K Poulose wrote:
>>>>>>
>>>>>> Missed the reference, see below.
>>>>>>
>>>>>> On 23/01/2023 22:18, Suzuki K Poulose wrote:
>>>>>>> On 23/01/2023 19:48, Steve Clevenger wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>> The issue is having a single HID for ETMs (which from a spec point of
>>>>>>> view makes sense for) with and without MMIO access. That brings two
>>>>>>> different components in Linux (AMBA hook for ACPI and a platform
>>>>>>> driver)
>>>>>>> compete for the said HID. There are other reasons to disconnect the
>>>>>>> CoreSight from AMBA framework and manage them directly [0].
>>>>>>
>>>>>> [0]
>>>>>> https://lkml.kernel.org/r/e37e12ab-9701-2883-724a-2a281ad35df2@arm.com
>>>>>>
>>>>>>
>>>>
>>