Update TPI_FFCR definition

[Kochise]

See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0337e/BABIFBCI.html (for Cortex-M3, but not in core_cm3.h) vs. http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0439b/BABCCJHJ.html (for Cortex-M4)

[JonatanAntoni]

We need to be careful with removing these defines. We must not break backward compatibility.
What about keeping them for a while and just mark them deprecated?

[Kochise]

Ok, but then why not documenting them properly to promote them to "official" status ? Are they really "deprecated" ?

[JonatanAntoni]

Its a matter of how to keep a common style. You added #define TPI_FFCR_TrigIN_Pos 8U instead, i.e. upper case N instead of lower case one. Do you prefer to keep both? In this case we should agree on one official version. Or should we simply stay with the lower case version and drop your new one?

[Kochise]

I put those in CAPITAL because it is the way they are declared in http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0337e/BABIFBCI.html table 13.8 (direct link http://infocenter.arm.com/help/topic/com.arm.doc.ddi0337e/BABIFBCI.html#BIIJEJFB doesn't work) hence I wanted to stay as close as possible to the official documentation (for what it worth, considering the numerous divergences)

[JonatanAntoni]

That makes totally sense to me. But removing the non-capital form might break existing implementations. Thats why I think we might simply keep both for a while.

[Kochise]

Sounds reasonable enough to me.

[JonatanAntoni]

May I ask you to update the PR accordingly? I'll merge it asap.

[Kochise]

Done...

[JonatanAntoni]

Thanks for raising this valuable enhancement/fix.

[JonatanAntoni]

Hi @Kochise,

I tried to review your enhancements and stumbled across another issue. Looking into the technical reference manuals of the processor cores (CM3/4/7/23/33) does not reveal the bit definitions you added.

The situation is that the TPIU (Trace Port Interface Unit) is not part of the processor core but implemented by an independent device subsystem called CoreSight. I struggle with adding subsystem specifics to the core headers. It is up to the device vendor to select the subsystems while composing a SoC. Hence its not guaranteed that all (e.g.) Cortex-M4 based devices include the CoreSight SoC-400 subsystem. The other way around the CoreSight SoC-400 can be attached to all the compatible Cortex-M processor cores.

I would rather keep this separate, i.e. collecting all the CoreSight SoC-400 related stuff into a subsystem header file. For the time being we do not have subsystems covered by CMSIS.

Cheers,
Jonatan

[Kochise]

Which is rather strange considering that the links provided points the official ARM documentation and that some TPIU registers are already (partially) defined (ie. TPI_FFCR_TrigIn_xxx). So I see no objection to extract those registers' definition into another more appropriate file. Adding the ITM frame format and the ETM registers' definition would also be a great benefit.

[JonatanAntoni]

Hi @Kochise,

Your contribution is very appreciated. My matter is just to keep the structure clean, comprehensible and easy to use. The core header files are intended to be used as-is by all the different device vendors. I admit we are not always consistent to that rule, but the core header should only contain core specific definitions. From core perspective only two bits of this register are defined and all others are reserved, i.e. implementation defined. Its completely up to the device vendor to implement the TPIU, either by (a) using the CoreSight SoC-400 subsystem as-is provided by Arm or (b) a custom(ized) solution. I assume the majority of vendors use solution (a).

That's why I proposed to have additional subsystem header files. Furthermore such a solution would make it easier to reuse the same subsystem for devices with different cores without duplicating all the information.

Its hard to estimate the demand of having such defines. I expect only a minority of advanced users (like you?) ever configuring the debug and trace peripherals from inside the device. Typically these registers are under control of the debugger, e.g. uVision. That's why I am a bit reluctant of assigning development capacities (we are only a few guys here) to this topic at the moment.

What do you think about developing/providing this CoreSight SoC-400 subsystem definitions (and perhaps others) separately for the time being, e.g. CoreSight git repo and pack? That might be easier to maintain and allow us keeping track of the popularity.

Cheers,
Jonatan

[Kochise]

I understand, but since it is an ARM ip, it would make sense to have official support from your side, be it used or not by the vendors (ie. mpu and fpu definitions in separate files, yet provided into cmsis). Perhaps add a conditional flag to include those definitions if the vendor provide support (ie __FPU_USED -> __TPIU_USED)

[JonatanAntoni]

Sure. I think a clean solution might be including the core_XXX.h and all the subsystem headers by the device header. That's a fair enhancement request. I'll keep it as "future". Thanks again.

[Kochise]

Btw, is it possible to also get registers' definition in bitfield format, instead of the brain dead mask/shift definitions ?

typedef union uMcuCpuTrace_RegTpiFfcr
{ struct
	{ int	enftc:1		///< 0x00000001, Enable Formatting. Because TRACECTL is never present, this bit reads as zero.
	; int	enfcont:1	///< 0x00000002, Continuous Formatting, no TRACECTL. This bit is set on reset.
	; int	_2:2		///< 0x0000000C, Reserved
	; int	fonflln:1	///< 0x00000010, Generate flush using the FLUSHIN interface.
	; int	fontrig:1	///< 0x00000020, Generate flush using Trigger event.
	; int	fonman:1	///< 0x00000040, Manually generate a flush of the system.
	; int	_7:1		///< 0x00000080, Reserved
	; int	trigin:1	///< 0x00000100, Indicate a trigger on TRIGIN being asserted.
	; int	trigevt:1	///< 0x00000200, Indicate a trigger on a Trigger Event.
	; int	trigfi:1	///< 0x00000400, Indicates a trigger on Flush completion.
	; int	_11:1		///< 0x00000800, Reserved
	; int	stopfi:1	///< 0x00001000, Stop the formatter after a flush completes.
	; int	stoptrig:1	///< 0x00002000, Stop the formatter after a Trigger Event is observed.
	; int	_14:18		///< 0xFFFFC000, Reserved
	;}
; int		_
;} uMcuCpuTrace_RegTpiFfcr;

It doesn't prevent you from accessing the raw register using the '_' but this is a more elegant and modern way to access bits, since compilers and now processors like ARM do features bitfield manipulation instructions.

I have done quite an extensive (and almost exhaustive) bitfield definition of the whole ARM lineup, and the STM32L4 as well. That's why I found several bugs in those definitions. There should be some insider cross reading to avoid those mistakes before release.

Last question, you say "we are only a few guys here", you mean at ARM ? That same company making millions of profit ? And you are unable to assign some priorities on things like code review and quality checking ? The same with ST (I look at you, fella...)

[rizickus]

Bitfields are not portable between compilers and should be avoided. Look at the standard of C language. Prefer using bitmasks.

[Kochise]

To me, a compiler that doesn't support bitfield that are out in the wild since... like... C99, is not to be considered a compiler, especially if it is supposed to target ARM chips released after 1999. Seriously. I'm not joking here. I've quit the 8051 ages ago.

[rizickus]

It is not about the support itself, but the compiler specific behavior regarding bitfields. If standard does not specify exact layout, do not expect all compilers to have the same implementation-defined behavior. It is naïve.

[Kochise]

Ok, now I see what you mean, yet what the C99 standard states is that you are filling the target integer starting from bit 0 and upward. There might be some issues regarding little vs. big endian, yet since the "translation" is done using the same rule that the mask/bit stuff, you "should" end up getting the same result. Which I verified, at least using GCC.

[Kochise]

Just for information, the official Renesas' definition files for the SH line of cpus : https://www.renesas.com/sg/en/software/D1010526.html

Lack a load of comments and informations, but you get the idea.

[Kochise]

Wow, I completely forgot about these :) Still relevant after 2.5+ years ?

[JonatanAntoni]

Hi @Kochise,

this got somehow hijacked, I think. As far as I can recap, this PR simply adds a couple of additional bit masks and should be just fine. The other ideas discussed should go into an issue, instead.

May I ask you to:

• Rebase and squash your PR
• Update the file date and version (increase minor bit, set patch to 0) in the header

I am happy to merge the additions.

Cheers,
Jonatan

[Kochise]

I'll see what I can do, I no longer have the modified repository since I changed job and don't work on ARM things anymore.

[JonatanAntoni]

Can one of the admins verify this patch?