Register names contain redundancy

[aykevl]

From a quick glance, it appears that all registers are prefixed with the peripheral name. This is redundant if structs are generated from these registers. For example, one might generate the following struct for the SPI register (if the clusters are ignored, see https://github.com/espressif/svd/issues/2#issuecomment-886033289):

typedef struct {
    volatile uint32_t SPI_MEM_CMD_REG;
    volatile uint32_t SPI_MEM_ADDR_REG;
    volatile uint32_t SPI_MEM_CTRL_REG;
    volatile uint32_t SPI_MEM_CTRL1_REG;
    volatile uint32_t SPI_MEM_CTRL2_REG;
    volatile uint32_t SPI_MEM_CLOCK_REG;
    volatile uint32_t SPI_MEM_USER_REG;
    volatile uint32_t SPI_MEM_USER1_REG;
    volatile uint32_t SPI_MEM_USER2_REG;
} SPI_Type;

#define SPI ((*SPI_Type)(0x60002000))

// using it
SPI->SPI_MEM_CMD_REG = ...

Here, you can see there is a lot of redundancy in the field names. The "SPI" part is repeated, and the "_REG" part is unnecessary. It would be much more useful to have something like this:

SPI->MEM_CMD = ...

or even:

SPI->MEM.CMD = ...

In other words, a struct like this:

typedef struct {
    struct {    
        volatile uint32_t CMD;    
        volatile uint32_t ADDR;    
        volatile uint32_t CTRL;        
        volatile uint32_t CTRL1;        
        volatile uint32_t CTRL2;        
        volatile uint32_t CLOCK;        
        volatile uint32_t USER;        
        volatile uint32_t USER1;        
        volatile uint32_t USER2;        
    } MEM;
} SPI_Type;

In fact, it looks like this data is already present: the ESP-IDF contains two versions of each register, a "reg" and a "struct" version (see spi_mem_reg.h and spi_mem_struct.h). I would much prefer if the SVD file was more like the "struct" version rather than the "reg" version.

In practice, I'm not using C but some other language. But it has the same issue, I am using C here for ease of understanding.

[jessebraham]

Thank you for opening this issue! I agree that the register naming is quite verbose and redundant. Fortunately it should be rather easy to strip the prefixes/suffixes during the generation process, so I will look into this.

[aykevl]

Yes, stripping prefixes/suffixes would make the SVD file a little bit easier to use.

However, what I'm asking is whether the same source as the *_struct.h header files can be used. For example, the SVD file contains entries like this:

        <register>
          <name>GPIO_FUNC1_IN_SEL_CFG_REG</name>
          <description>GPIO input function configuration register</description>
          <addressOffset>0x158</addressOffset>
          <size>32</size>
          <fields>
            ...
          </fields>
        </register>
        <register>
          <name>GPIO_FUNC2_IN_SEL_CFG_REG</name>
          <description>GPIO input function configuration register</description>
          <addressOffset>0x15c</addressOffset>
          <size>32</size>
          <fields>
            ...
          </fields>
        </register>

This makes all registers available to programs, but makes it hard to pick a GPIO number using an index.

Compare this with the following sample from gpio_struct.h:

    union {
        struct {
            uint32_t func_sel:         5;
            uint32_t sig_in_inv:       1;
            uint32_t sig_in_sel:       1;
            uint32_t reserved7:       25;
        };
        uint32_t val;
    } func_in_sel_cfg[128];

This is much easier to work with, because programs can refer to a register by an index number instead of having to use the register number directly. In the SVD file, this can be implemented as follows, using <dim> and <dimIncrement>:

        <register>
          <dim>128</dim>
          <dimIncrement>0x04</dimIncrement>
          <name>FUNC_IN_SEL_CFG[%s]</name>
          <description>GPIO input function configuration register</description>
          <addressOffset>0x158</addressOffset>
          <fields>
            ...
          </fields>
        </register>

There are many other cases like this, for example the registers I2C_COMD0_REG, I2C_COMD1_REG, I2C_COMD2_REG, ... which would be easier to use if they were presented as an array.

[jessebraham]

Okay thank you for the explanation. I understand now, sorry about that.

This seems to stem from our internal representation from which we are generating the SVD. I am unable to link to specific lines due to the size of the SVD, but you will find that there are already some registers using <dim>/<dimIncrement> along with the %s placeholder, so this feature is implemented in the generator. You can find these under the GPIOSD and IO_MUX peripherals. I will look into why the required information is not present for the remaining peripherals and hopefully come up with a solution, I may be able to modify things to make it work regardless.

I have also updated the generator to strip the peripheral name prefix from register/field names, and the _REG suffix from register names as mentioned above. I am currently blocked by a few internal issues but will update the public SVD once they are resolved.

[aykevl]

I am unable to link to specific lines due to the size of the SVD, but you will find that there are already some registers using <dim>/<dimIncrement> along with the %s placeholder, so this feature is implemented in the generator.

Interesting! I found two instances: GPIOSD_SIGMADELTA%s_REG and IO_MUX_GPIO%s_REG. Somehow my parser still converts that to a list of registers, but that's definitely a bug on my side (the SVD file looks correct in that regard!).

I will look into why the required information is not present for the remaining peripherals and hopefully come up with a solution, I may be able to modify things to make it work regardless.

That would be great! SVD file quality varies a lot by vendors (there is a huge difference between Nordic and STMicroelectronics for example) but very few even provide a channel to suggest improvements. I'm really happy you are open to feedback and try to make a good SVD file - that makes supporting the ESP32C3 a lot easier!

Could it be that there are two different data sources? It seems that the _struct.h header files contain a lot more information than the _reg.h header files.

(Off-topic sidenote: I think the ESP32-C3 will be a lot easier to support in TinyGo because it uses the RISC-V architecture, maybe I can even get WiFi/BT to work!).

[jessebraham]

I have some updates on this! Apologies for the delay.

At present I have collected the following registers into arrays:

• FUNC*_IN_SEL_CFG, FUNC*_OUT_SEL_CFG, and GPIO_PIN*_REG for the GPIO peripheral
• I2C_COMD*_REG for the I2C peripheral

I will be pushing an updated SVD early next week (Monday most likely) including these changes and others. If there are any additional registers you would like to see turned into arrays prior to that release please just let me know and I'll be happy to do so. I think the four groups above are the more common registers, though.

[jessebraham]

I believe most of the issues raised here have been addressed in 11b28a1, thank you again for bringing these to my attention. There is still a bit of redundancy in some field names, but we're getting there.

I am closing this issue for now, if you feel it has not been resolved then it can be re-opened or additional issues can be opened.

[aykevl]

Thanks a lot! This definitely makes the SVD file easier to use.

[aykevl]

I found one thing in the new SVD file that's a bit odd: the TIMG peripheral still has _REG suffixes. There are some other miscellaneous _REG suffixes, but this is the only peripheral that has the suffix on all registers.

[jessebraham]

Thanks for pointing that out, I'll make a note to look into it when I'm working on this project next.

[jessebraham]

Turns out some of the register names have a _REG_REG suffix, so only one is being stripped. Easy enough, will be fixed in the next release.