cmsis-svd / cmsis-svd-data

Aggregration of ARM Cortex-M (and other) CMSIS SVDs
Apache License 2.0
38 stars 17 forks source link

Freescale/MKL25Z4.svd: clusters should be used for register groupings #31

Open jgrivera67 opened 8 years ago

jgrivera67 commented 8 years ago

Currently, in Freescale Kinetis SVD files (such as https://github.com/posborne/cmsis-svd/blob/master/data/Freescale/MKL25Z4.svd), what logically is an array of a grouping of registers is being represented in the SVD file as separate "dim" arrays of registers. For example:

        <register>
          <dim>6</dim>
          <dimIncrement>0x8</dimIncrement>
          <dimIndex>0,1,2,3,4,5</dimIndex>
          <name>C%sSC</name>
          <description>Channel (n) Status and Control</description>
          **<addressOffset>0xC</addressOffset>**
          <size>32</size>
          <access>read-write</access>
          <resetValue>0</resetValue>
          <resetMask>0xFFFFFFFF</resetMask>
          <fields>
            ...
          </fields>
        </register>
        <register>
          <dim>6</dim>
          <dimIncrement>0x8</dimIncrement>
          <dimIndex>0,1,2,3,4,5</dimIndex>
          <name>C%sV</name>
          <description>Channel (n) Value</description>
          **<addressOffset>0x10</addressOffset>**
          <size>32</size>
          <access>read-write</access>
          <resetValue>0</resetValue>
          <resetMask>0xFFFFFFFF</resetMask>
          <fields>
          ...
          </fields>
        </register>

This does not take advantage of the cluster construct in SVD syntax, which would be both more readable and would allow tools like SVDConv generate code that is easier to work with. For the example above, the code generated by the ARM SVDConv tool looks like this:

typedef struct {                                    /*!< TPM0 Structure                                                        */
     ...
     __IO uint32_t  C0SC;                              /*!< Channel (n) Status and Control                                        */
     __IO uint32_t  C0V;                               /*!< Channel (n) Value                                                     */
     __IO uint32_t  C1SC;                              /*!< Channel (n) Status and Control                                        */
     __IO uint32_t  C1V;                               /*!< Channel (n) Value                                                     */
     __IO uint32_t  C2SC;                              /*!< Channel (n) Status and Control                                        */
     __IO uint32_t  C2V;                               /*!< Channel (n) Value                                                     */
     __IO uint32_t  C3SC;                              /*!< Channel (n) Status and Control                                        */
     __IO uint32_t  C3V;                               /*!< Channel (n) Value                                                     */
     __IO uint32_t  C4SC;                              /*!< Channel (n) Status and Control                                        */
     __IO uint32_t  C4V;                               /*!< Channel (n) Value                                                     */
     __IO uint32_t  C5SC;                              /*!< Channel (n) Status and Control                                        */
     __IO uint32_t  C5V;                               /*!< Channel (n) Value                                                     */
     ...
} TPM0_Type;`

In contrast, if we rewrite the SVD fragment shown above, using a \ cluster**, as shown below:

        <cluster>
            <dim>6</dim>
            <dimIncrement>8</dimIncrement>
            <dimIndex>0-5</dimIndex>
            <name>CHANNELS[%s]</name>
            <description>Grouping of Channels</description>
            <addressOffset>0xC</addressOffset>
            <register>
              <name>CnSC</name>
              <description>Channel (n) Status and Control</description>
              <addressOffset>0x0</addressOffset>
              <size>32</size>
              <access>read-write</access>
              <resetValue>0</resetValue>
              <resetMask>0xFFFFFFFF</resetMask>
              <fields>
               ...
              </fields>
            </register>
            <register>
              <name>CnV</name>
              <description>Channel (n) Value</description>
              <addressOffset>0x4</addressOffset>
              <size>32</size>
              <access>read-write</access>
              <resetValue>0</resetValue>
              <resetMask>0xFFFFFFFF</resetMask>
              <fields>
              ...
              </fields>
            </register>
        </cluster>

the corresponding C code generated by the ARM SVDConv tool would look like this:

typedef struct {
  __IO uint32_t  CnSC;                              /*!< Channel (n) Status and Control                                        */
  __IO uint32_t  CnV;                               /*!< Channel (n) Value                                                     */
} TPM0_CHANNELS_Type;

typedef struct {                                    /*!< TPM0 Structure                                                        */
  ...
  TPM0_CHANNELS_Type CHANNELS[6];                   /*!< Grouping of Channels                                                  */
  ...
} TPM0_Type;

which is both more readable and easier to work with in code that uses it. So, All the Freescale Kinetis SVD files should be updated to use clusters where appropriate.

posborne commented 8 years ago

Unfortunately, due to the license on the SVD files, we don't directly change SVD files as part of this project. We might be able to convince the vendor (in this case Freescale/NXP) to make the change.

CC @flit