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

From: Steve Clevenger <scclevenger@os.amperecomputing.com>
To: Suzuki K Poulose <suzuki.poulose@arm.com>, mathieu.poirier@linaro.org
Cc: mike.leach@linaro.org, leo.yan@linaro.org,
	coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
	john.horley@arm.com,
	Anshuman Khandual <anshuman.khandual@arm.com>
Subject: Re: [PATCH 1/3] coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read
Date: Wed, 1 Feb 2023 21:20:02 -0800	[thread overview]
Message-ID: <38deb2d4-c8bf-7b17-3b41-e9535061b426@os.amperecomputing.com> (raw)
In-Reply-To: <c412f924-34d2-0c30-8d19-e9ca71e7afa1@arm.com>

Hi Suzuki,

I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch
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?

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