Open-CMSIS-Pack / devtools

Open-CMSIS-Pack development tools - C++
Apache License 2.0
69 stars 50 forks source link

[packchk] Does it check for incomplete <processors>? #1535

Open slhultgren opened 1 month ago

slhultgren commented 1 month ago

Describe The Problem To Be Solved The Keil.STM32MP1xx_DFP.1.3.1 pack contains <processors> on <devices> that never define the Dendian attribute for the finally merged <processor>. Is this correct? As far as I can tell in the specification, Dcore, Dfpu, Dmpu, Ddsp, Dendian, Dclock, and DcoreVersion are all required to be described on some level of the device "tree" in the .pdsc.

(Optional): Suggest A Solution Packchk should verify that all devices in the .pdsc also get "complete" processor definitions The pack mentioned above should be updated?

jkrech commented 1 month ago

Thanks a lot for highlighting this gap in packchk. I think that even the second example in the specification is not helpful. @edriouk, I assume that for some of the attributes the RTE Model use default values. e.g. if Dfpu is not specified NO_FPU is assumed. I think we need to update the documentation accordingly. How about Dendian, is there a default value and what value is it?

edriouk commented 1 month ago

As far as I can see, there is no default in RTE Model for Dendian.

slhultgren commented 1 month ago

So, was the conclusion yesterday that we should update the specification for the <processor> to state that the Dfpu, Dmpu, Ddsp, Dendian, Dmve, Dtz are optional?

And if not specified, the following default values are assumed?

Dfpu=NO_FPU
Dmpu=NO_MPU
Ddsp=NO_DSP
Dendian=Little-endian
Dmve=NO_MVE
Dtz=NO_TZ

?

Maybe we need to list default values for all the properties in the <processor>, not sure. Or we don't need to change the specification, but instead just update the packchk to check for incomplete processors, and then the tools should reject any device with an incomplete processor?

@edriouk does the above partial list of default values seem correct?

ReinhardKeil commented 1 month ago

@slhultgren I agree we should define default values. Your list above seems fine but needs to be validated so that it matches tool implementation.

edriouk commented 1 month ago

@slhultgren , @ReinhardKeil, the supplied list corresponds to currently used defaults in tools. Dendian does not have a default value so far, but it can be easily added. Dfpu default depends on Dmve: Dmve=MVE_FPU assumes Dfpu=DP_FPU

slhultgren commented 1 month ago

Dfpu default depends on Dmve: Dmve=MVE_FPU assumes Dfpu=DP_FPU

So this means e.g. this incompatible combination:

Dfpu=NO_FPU
Dmve=MVE_FPU

Would actually become this at runtime in the tool?

Dfpu=DP_FPU
Dmve=MVE_FPU

? (meaning that Dmve if set, always overwrites Dfpu)

edriouk commented 1 month ago

@slhultgren, yes, Dfpu dependency on Dmve is a runtime tool setting, it should overwrite Dfpu in case of Dmve=DP_FPU

jkrech commented 1 month ago

Practically we need to rely on default values for processor features, as we keep adding new ones over time. We need to update the specification as this statement is incorrect:

While the different attributes can be spreaded over the family levels, they add-up and at the leaf (device level), a complete set of attributes should be present (at least Dcore, Dfpu, Dmpu, Ddsp, Dendian, Dclock, and DcoreVersion).

```ini
Dmpu=NO_MPU
Dfpu=NO_FPU
Ddsp=NO_DSP
Dmve=NO_MVE
Dtz=NO_TZ
Dcdecp=0x00
Dpacbti=NO_PACBTI
Dendian=Little-endian
Punits=1

Mandatory = no default

Dcore
DcoreVersion
Dclock

Mandatory for multi-core devices only!

  Pname

packchk needs to validate that mandatory attributes are specified for each device (variant).