Add descriptions and valid values to all GPIO_* registers

[username223]

This PR updates every GPIO_* register with descriptions, values and access modifiers.

Everything was sourced from the SDK.

It is very probable that the amount of boilerplate could be reduced by inheritance, although I'm not too sure how it works in relation to svd2rust.

I have also changed the names of some fields that were previously just 32 bit fields after splitting them into the appropriate fields. I'm not sure how much that changes on the Rust side.

Let me know if anything should be changed.

[9names]

Nice work. I've looked through the changes and it all looks good to me. I haven't validated the values vs SDK though. I'll try to get around to that tomorrow if someone else doesn't get there first.

[username223]

Thanks for the feedback.

You're using an inconsistent naming convention for the enumeratedValue names, sometimes they're UPPER_CASE (e.g. SWGPIO_1), sometimes they're lowercase with a dash (e.g. no-clear) and sometimes they're lowercase with an underscore (e.g. positive_pulse) - please be consistent.

Ah, sorry. I've changed everything to use underscore only. The uppercase names are taken directly from the SDK defines, while the lowercase ones are implicit in the SDK or undefined.

I'm not sure how we want to proceed from here, because there's definitely value in using the same names as the SDK, but there's also value in having a unified naming convention.

All the GPIO function select enums have different names because they have limited mapping configurations - for documentation, this is good, but for code generation, this is not good.

Should we keep the more descriptive name, or should we unify the names under their peripheral name?

I think the dedicated names are necessary, since the GPIO pins don't all have the same values available and some have the same value but a different name. For example, GPIO20 has SF-D0 as value 2 while GPIO12 has UNUSED2 (unused values have not been added to the SVD). There are also GPIO pins with the same mapping but for example different channels for PWM in the name, which I think is valuable to keep.

If we use peripheral names, is it possible to define an enum in the SVD format, and then reuse it in the function select registers, to reduce the number of duplicate lines?

I believe the enumeratedValues struct compiles down to an enum, at least when building for C. I don't think it's possible to globally define a struct and then use that. I believe that it's possible to reduce duplication in fields that have the exact same values like pd bitfields that only have enabled and disabled using the <field derivedFrom="TimerCtrl0"> notation although I don't know how this works together with svd2rust or the underlaying svd parser.

The GPIO function select register should have a default value consistent with what's defined as the reset value in the reference manual. You - or someone else - can add this.

I'll get on it. I checked the documentation for SVD and it's only available at the register level like so:

<register>
  ...
  <resetValue>0x00008001</resetValue>
  <resetMask>0x0000ffff</resetMask>
  ...
</register>

Which is pretty weird when working with individual bitfields. I completely missed the resetMask in the documentation so I assumed that I had to know the reset value for every single bit.

Looking further at the SDK I believe the default values are listed on the far right:

    /* 0x100 : GPIO_CFGCTL0 */
    union {
        struct {
            uint32_t reg_gpio_0_ie                  :  1; /* [    0],        r/w,        0x1 */
            uint32_t reg_gpio_0_smt                 :  1; /* [    1],        r/w,        0x1 */
            uint32_t reg_gpio_0_drv                 :  2; /* [ 3: 2],        r/w,        0x0 */
            uint32_t reg_gpio_0_pu                  :  1; /* [    4],        r/w,        0x0 */
            uint32_t reg_gpio_0_pd                  :  1; /* [    5],        r/w,        0x0 */
            uint32_t reserved_6_7                   :  2; /* [ 7: 6],       rsvd,        0x0 */
            uint32_t reg_gpio_0_func_sel            :  4; /* [11: 8],        r/w,        0x1 */
            uint32_t real_gpio_0_func_sel           :  4; /* [15:12],          r,        0x1 */
            uint32_t reg_gpio_1_ie                  :  1; /* [   16],        r/w,        0x1 */
            uint32_t reg_gpio_1_smt                 :  1; /* [   17],        r/w,        0x1 */
            uint32_t reg_gpio_1_drv                 :  2; /* [19:18],        r/w,        0x0 */
            uint32_t reg_gpio_1_pu                  :  1; /* [   20],        r/w,        0x0 */
            uint32_t reg_gpio_1_pd                  :  1; /* [   21],        r/w,        0x0 */
            uint32_t reserved_22_23                 :  2; /* [23:22],       rsvd,        0x0 */
            uint32_t reg_gpio_1_func_sel            :  4; /* [27:24],        r/w,        0x1 */
            uint32_t real_gpio_1_func_sel           :  4; /* [31:28],          r,        0x1 */
        }BF;
        uint32_t WORD;
    } GPIO_CFGCTL0;

As an aside, all the GPIO registers are currently just in the GLB peripheral group. The docs mention that it is possible to cluster registers together. It might make sense to collect related registers, although I do not know how this would affect Rust codegen.

[username223]

I've got reset values for all registers as well now. These are taken directly from the SDK. The reset masks also apply to bitfields that aren't in the SVD due to being reserved. I'm unsure if this is the correct way to do things, but the SDK has all reserved values as being 0.

Might be good for someone to check my work on the GPIO configuration registers since I might have made a mistake in my script.

[9names]

I have manually confirmed that the reset values in your changes match the SDK values

[username223]

Updated to remove the reserved values introduced initially. These are warnings in the SVDConv utility.

Additionally, I found an enumerated values description for the "real" function selects.

[9names]

I was ready to merge this, but I tried running svd2rust and building, and it fails This generates a function with the name async, which is a reserved keyword in Rust

<enumeratedValues>
    <name>GPIO0ControlMode</name>
    <enumeratedValue>
        <name>sync</name>
        <value>0</value>
    </enumeratedValue>
    <enumeratedValue>
        <name>async</name>
        <value>1</value>
    </enumeratedValue>
</enumeratedValues>

generates

impl<'a> REG_GPIO_0_INTERRUPT_CONTROL_MODE_W<'a> {
    #[doc = r"Writes `variant` to the field"]
    #[inline(always)]
    pub fn variant(self, variant: REG_GPIO_0_INTERRUPT_CONTROL_MODE_A) -> &'a mut W {
        {
            self.bit(variant.into())
        }
    }
    #[doc = "`0`"]
    #[inline(always)]
    pub fn sync(self) -> &'a mut W {
        self.variant(REG_GPIO_0_INTERRUPT_CONTROL_MODE_A::SYNC)
    }
    #[doc = "`1`"]
    #[inline(always)]
    pub fn async(self) -> &'a mut W {
        self.variant(REG_GPIO_0_INTERRUPT_CONTROL_MODE_A::ASYNC)
    }

rustc says we can escape these to use them, but that means we'd have to fix svd2rust to get it to generate the correct code

error: expected identifier, found keyword `async`
   --> src/glb/gpio_int_mode_set1.rs:161:12
    |
161 |     pub fn async(self) -> &'a mut W {
    |            ^^^^^ expected identifier, found keyword
    |
help: you can escape reserved keywords to use them as identifiers
    |
161 |     pub fn r#async(self) -> &'a mut W {

[9names]

I did a rebuild pulling your SVD into upstream main instead of doing it in your branch, and it didn't complain any more. There must have been some build system changes done upstream since you branched. Please disregard the above comment.

[username223]

Otherwise, in order to get ahead of the issue I could just change the names to synchronous and asynchronous? It might prevent future issues.

[username223]

Pushed a fix to the sync/async thing. There's really no reason not to change it, since we don't gain anything from the shorter value names and we might as well try to prevent issues even if our specific version of the codegen software doesn't have the issue.