-----Original Message----- From: Leo Yan leo.yan@linaro.org Sent: 24 August 2021 04:12 To: Al Grant Al.Grant@arm.com Cc: Tanmay Jagdale tanmay@marvell.com; Mike Leach mike.leach@linaro.org; coresight@lists.linaro.org; Sunil Kovvuri Goutham sgoutham@marvell.com; Linu Cherian lcherian@marvell.com; Bharat Bhushan bbhushan2@marvell.com Subject: Re: Query regarding AXI Write Burst Length in ETR driver
On Mon, Aug 23, 2021 at 06:13:04PM +0800, Leo Yan wrote:
On Mon, Aug 23, 2021 at 09:44:51AM +0000, Al Grant wrote:
Hi Al and Mike,
16 (0xF) should work for all silicon, as AXI allows burst sizes up to 16. So unless we've missed something, this is an implementation non-compliance and we should not be penalising compliant implementations by reducing the default burst size - the question is how we can enable the upstream kernel
to
workaround the issue on this chip only, and to me that sounds like it
needs
something that can be triggered by a setting in DT/ACPI.
Another possibility would be to introduce an errata workaround in Kconfig for your silicon. There are a number of these already in KConfig for PE issues e.g. CONFIG_ARM64_ERRATUM_826319, and we have introduced CONFIG_ETM4X_IMPDEF_FEATURE for silicon specific variants in the ETMv4 driver.
The latter config compiles in implementation defined workarounds which operate on the basis of matching the AMBA ID for the silicon. This means they will operate only if configured in KConfig and only on silicon where the workaround is needed.
Thanks a lot for the suggestions.
We are thinking of using "part-number" and "designer" rather than device tree property. While we use standard ARM core IP and Coresight SoC-600 IP, we cannot differentiate our silicon from others using ETR AMBA ID, PIDR and CPU MIDR registers.
We are proposing to expose Coresight ROM region to the driver and determine part number and designer by reading the following fields. part_number = (PIDR1.PART_1 << 8) | PIDR0.PART_0; designer = ((PIDR4.DES_2 << 7) & 0xf) | ((PIDR2.DES_1 << 4) & 0x7) | ((PIDR1.DES_0 & 0xf));
Using a combination of part number and designer from ROM region would help in identifying the Marvell implementation.
This option would be generic and could be helpful for other silicon with similar issues and can be applied across Coresight components
like ETF/ETR.
What are your thoughts on this approach ?
It's a valid approach if you can locate the CoreSight ROM region. Some CPUs don't set the CPU DBGDRAR (ROM region base address) register, so as a generic mechanism, there would need to be some alternative way of locating the ROM region, e.g. through DT or ACPI.
Sorry for sudden jumping into this topic, since I have some experience for DT binding, I agree with Al that DT (or ACPI) is the right way to move forward.
I'd like to clarify a bit more at here:
A good way is to use DT binding with a new node property to describe the burst size, e.g. in Documentation/devicetree/bindings/arm/coresight.txt, we can add an optional property like below:
Optional property for TMC:
- arm,burst-size: burst size initiated by the TMC on the AXI master interface. The burst size can be in the range [0..15], the setting supports one data transfer per burst to maximum of 16 data transfers per burst. If don't set this property, the driver will set to 15 (16 data transfers per burst) as default value.
I don't think it's a right way to use CoreSight ROM to read out part number and producer number, and based on these numbers to set burst size. The main reason is this will add SoC specific code into the driver, and DT binding is just used to decouple the platform specific with the driver code, so with the DT binding, the driver works as common as possible.
Using errata workaround also is not the right thing to do. Based on the TMC spec (ARM DDI 0461B), the burst size on AXI bus could be different on different SoCs
Hi Leo, where are you reading that this is a property of the AXI bus? There is a constraint that the burst size must not be greater than the TMC buffer size (see DDI 0461B 3.3.1), but buffer size is indicated by the TMC's MEMWIDTH register and the driver can determine that.
The situation I thought we were dealing with, is where the TMC presents a burst that the AXI spec says is valid but the downstream component in this SoC can't accept a burst of that size. Our understanding is that this would be a non-compliance in the downstream component - i.e. failure to handle a valid AXI transaction - and it would be appropriate to treat this as a SoC-specific issue triggering a SoC-specific workaround. (The alternative would be to represent the AXI topology in DT/ACPI and then drive the workaround from knowing what the downstream component is.)
Al
, so it's not a silicon bug if a SoC configures any burst size rather than '15'
(which is hard coded in current driver). We can use DT binding to allow a platform to pass its specific burst size and the driver could set the 'correct' burst size for register AXICTL. For backward compatibility, if this property is not set, the driver will set the default value 'TMC_AXICTL_WR_BURST_16'.
Thanks, Leo