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

[Steve Clevenger <scclevenger@os.amperecomputing.com>]

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.
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
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
>>>
>>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel