LPC82x: Wrong offsets in GPIO_PORT peripheral

[hannobraun]

I'm currently working on a Rust project using an LPC82x microcontroller by NXP. I've generated register mappings with svd2rust from this file: LPC82x.svd

Please note that this file won't work as-is. I've had to make some minor modifications to get the resulting Rust code to compile. I plan to release all of this soon, but haven't so far. Sorry it's not available right now, but I hope the full context won't be relevant for this issue.

The results have been mostly good so far, but the register offsets for the GPIO_PORT peripheral are wrong. Right at the start of GPIO_PORT's address space are 28 byte-sized registers, one for each I/O pin. Here's how this is specified in the SVD file:

<register>                                              
    <dim>29</dim>                                           
    <dimIncrement>0x1</dimIncrement>                                            
    <dimIndex>0-28</dimIndex>   
    <name>B%s</name>                                            
    <description>Byte pin registers port 0; pins PIO0_0 to PIO0_28</description>

    <addressOffset>0x0000</addressOffset>
    <size>1</size>
    <access>read-write</access>                                         
    <resetValue>0</resetValue>                                          
    <resetMask>0x00000000</resetMask>   
    
    <fields>                                            
        <field>                                     
            <name>PBYTE</name>                                  
            <description>Read: state of the pin PIO0_n, regardless of direction, masking, or alternate function, except that pins configured as analog I/O always read as 0. One register for each port pin: n = pin 0 to 28. Write: loads the pin's output bit.</description>                                  
            <bitRange>[0:0]</bitRange>                                  
        </field>                                        
    </fields>                                           
</register>

Here's the code that svd2rust generates from this:

#[doc = "0x00 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b0: B,
_reserved0: [u8; 1usize],
#[doc = "0x01 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b1: B,
_reserved1: [u8; 1usize],
#[doc = "0x02 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b2: B,
_reserved2: [u8; 1usize],
#[doc = "0x03 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b3: B,
_reserved3: [u8; 1usize],
#[doc = "0x04 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b4: B,
_reserved4: [u8; 1usize],
#[doc = "0x05 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b5: B,
_reserved5: [u8; 1usize],
#[doc = "0x06 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b6: B,
_reserved6: [u8; 1usize],
#[doc = "0x07 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b7: B,
_reserved7: [u8; 1usize],
#[doc = "0x08 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b8: B,
_reserved8: [u8; 1usize],
#[doc = "0x09 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b9: B,
_reserved9: [u8; 1usize],
#[doc = "0x0a - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b10: B,
_reserved10: [u8; 1usize],
#[doc = "0x0b - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b11: B,
_reserved11: [u8; 1usize],
#[doc = "0x0c - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b12: B,
_reserved12: [u8; 1usize],
#[doc = "0x0d - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b13: B,
_reserved13: [u8; 1usize],
#[doc = "0x0e - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b14: B,
_reserved14: [u8; 1usize],
#[doc = "0x0f - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b15: B,
_reserved15: [u8; 1usize],
#[doc = "0x10 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b16: B,
_reserved16: [u8; 1usize],
#[doc = "0x11 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b17: B,
_reserved17: [u8; 1usize],
#[doc = "0x12 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b18: B,
_reserved18: [u8; 1usize],
#[doc = "0x13 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b19: B,
_reserved19: [u8; 1usize],
#[doc = "0x14 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b20: B,
_reserved20: [u8; 1usize],
#[doc = "0x15 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b21: B,
_reserved21: [u8; 1usize],
#[doc = "0x16 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b22: B,
_reserved22: [u8; 1usize],
#[doc = "0x17 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b23: B,
_reserved23: [u8; 1usize],
#[doc = "0x18 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b24: B,
_reserved24: [u8; 1usize],
#[doc = "0x19 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b25: B,
_reserved25: [u8; 1usize],
#[doc = "0x1a - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b26: B,
_reserved26: [u8; 1usize],
#[doc = "0x1b - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b27: B,
_reserved27: [u8; 1usize],
#[doc = "0x1c - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b28: B,
_reserved28: [u8; 4068usize],
#[doc = "0x1000 - Word pin registers port 0"]
pub w0: W,

Here's the definition of B:

pub struct B {
    register: VolatileCell<u8>,
}

As you can see, svd2rust inserts one additional byte of reserved space per register. As B is already one byte wide, this adds one byte of additional offset per register. There's also an additional byte in the offset between b28 and w0 (second-to-last and last fields in the code block above).

I don't know SVD well enough to rule out a mistake there, but it seems obvious that something is wrong with svd2rust here. As you can see, the offsets it inserts in the documentation are the correct ones, they don't account for the additional byte of reserved space.

I think this problem is specific to registers that are 1 byte wide. The next block of registers looks pretty much the same, except that the registers are 4 bytes wide:

<register>                                              
    <dim>29</dim>                                           
    <dimIncrement>0x4</dimIncrement>                                            
    <dimIndex>0-28</dimIndex>                                           
    <name>W%s</name>                                            
    <description>Word pin registers port 0</description>                                            
    <addressOffset>0x1000</addressOffset>                                           
    <access>read-write</access>                                         
    <resetValue>0</resetValue>                                          
    <resetMask>0x00000000</resetMask>                                           
    <fields>                                            
        <field>                                     
            <name>PWORD</name>                                  
            <description>Read 0: pin PIOm_n is LOW.  Write 0: clear output bit. Read 0xFFFF FFFF: pin PIOm_n is HIGH.  Write any value 0x0000 0001 to 0xFFFF FFFF: set output bit. Only 0 or 0xFFFF FFFF can be read. Writing any value other than 0 will set the output bit. One register for each port pin: n = pin 0 to 28.</description>                                    
            <bitRange>[31:0]</bitRange>                                 
        </field>                                        
    </fields>                                           
</register>

The code that svd2rust generates for those looks correct:

#[doc = "0x1000 - Word pin registers port 0"]
pub w0: W,
#[doc = "0x1004 - Word pin registers port 0"]
pub w1: W,
#[doc = "0x1008 - Word pin registers port 0"]
pub w2: W,
#[doc = "0x100c - Word pin registers port 0"]
pub w3: W,
#[doc = "0x1010 - Word pin registers port 0"]
pub w4: W,
#[doc = "0x1014 - Word pin registers port 0"]
pub w5: W,
#[doc = "0x1018 - Word pin registers port 0"]
pub w6: W,
#[doc = "0x101c - Word pin registers port 0"]
pub w7: W,
#[doc = "0x1020 - Word pin registers port 0"]
pub w8: W,
#[doc = "0x1024 - Word pin registers port 0"]
pub w9: W,
#[doc = "0x1028 - Word pin registers port 0"]
pub w10: W,
#[doc = "0x102c - Word pin registers port 0"]
pub w11: W,
#[doc = "0x1030 - Word pin registers port 0"]
pub w12: W,
#[doc = "0x1034 - Word pin registers port 0"]
pub w13: W,
#[doc = "0x1038 - Word pin registers port 0"]
pub w14: W,
#[doc = "0x103c - Word pin registers port 0"]
pub w15: W,
#[doc = "0x1040 - Word pin registers port 0"]
pub w16: W,
#[doc = "0x1044 - Word pin registers port 0"]
pub w17: W,
#[doc = "0x1048 - Word pin registers port 0"]
pub w18: W,
#[doc = "0x104c - Word pin registers port 0"]
pub w19: W,
#[doc = "0x1050 - Word pin registers port 0"]
pub w20: W,
#[doc = "0x1054 - Word pin registers port 0"]
pub w21: W,
#[doc = "0x1058 - Word pin registers port 0"]
pub w22: W,
#[doc = "0x105c - Word pin registers port 0"]
pub w23: W,
#[doc = "0x1060 - Word pin registers port 0"]
pub w24: W,
#[doc = "0x1064 - Word pin registers port 0"]
pub w25: W,
#[doc = "0x1068 - Word pin registers port 0"]
pub w26: W,
#[doc = "0x106c - Word pin registers port 0"]
pub w27: W,
#[doc = "0x1070 - Word pin registers port 0"]
pub w28: W,

[kjetilkjeka]

Would you mind take a run with my branch from PR #121 and see if it works better?

The branch should not unroll the array into seperate registers. I think the code causing your error are slightly changed there.

[hannobraun]

That looks useful, thanks! I'll try it tomorrow.

[kjetilkjeka]

Unfortunately, the same bug exists on my branch (i tested with your code).

[hannobraun]

@kjetilkjeka Thanks for looking into it. I dug around your branch for a bit and found the culprit! svd2rust expects the register size to be in bits, while the value 1 in the SVD file is meant to be in bytes.

According to the SVD description, svd2rust's interpretation is correct, meaning the SVD file is wrong.

If I replace this:

<size>1</size>

With this:

<size>8</size>

Then svd2rust generates the code correctly, without any padding.

I haven't fully understood the code, but I believe the offset is computed from addressOffset and dimIncrement, while the padding is computed from size. I guess the correct thing to do would be for svd2rust to print a warning, if size isn't a multiple of 8.

I'm going to solve the problem on my side by patching the SVD file, as I had to do multiple times already. I'll contact NXP after I release the register mappings. Hopefully I can get my fixes upstreamed.

[kjetilkjeka]

This makes sense, it would be weird to have 28 bytes only using 1 bit of each of them.

I haven't fully understood the code, but I believe the offset is computed from addressOffset and dimIncrement, while the padding is computed from size

That's about right

I guess the correct thing to do would be for svd2rust to print a warning, if size isn't a multiple of 8.

The <size>1</size> is a legal svd format and perhaps the better way to handle it would be to create a u8 that acted both as the "data bit" and padding.

But everything is better than whats currently happening (failing silently).

[hannobraun]

This makes sense, it would be weird to have 28 bytes only using 1 bit of each of them.

That's exactly what's happening here, though. The registers are 8 bit wide, but 7 of those are reserved. The next 28 registers work mostly the same, but are even more wasteful at 32 bits wide each. Address space is cheap, I guess.

[jamesmunns]

Hey @hannobraun, is this issue still relevant? If so, could we please rework this issue into something that describes what svd2rust needs to change to support your use case?

[hannobraun]

@jamesmunns The issue is no longer relevant to me, since it was a bug in the SVD file that caused the bug in svd2rust, and I've since fixed the SVD file. However, even though the <size>1</size> that caused the problem was a bug in that specific SVD file, it's still valid SVD in general, accordring to @kjetilkjeka (source).

I'll undo the fix to the SVD file and re-test, to find out whether the problem still persists in svd2rust. It might be a while until I can find the time, though.

[hannobraun]

@jamesmunns This issue persists. I can still reproduce.

I'll summarize what's wrong. Here's the SVD snippet from my original comment:

<register>                                              
    <dim>29</dim>                                           
    <dimIncrement>0x1</dimIncrement>                                            
    <dimIndex>0-28</dimIndex>   
    <name>B%s</name>                                            
    <description>Byte pin registers port 0; pins PIO0_0 to PIO0_28</description>

    <addressOffset>0x0000</addressOffset>
    <size>1</size>
    <access>read-write</access>                                         
    <resetValue>0</resetValue>                                          
    <resetMask>0x00000000</resetMask>   
    
    <fields>                                            
        <field>                                     
            <name>PBYTE</name>                                  
            <description>Read: state of the pin PIO0_n, regardless of direction, masking, or alternate function, except that pins configured as analog I/O always read as 0. One register for each port pin: n = pin 0 to 28. Write: loads the pin's output bit.</description>                                  
            <bitRange>[0:0]</bitRange>                                  
        </field>                                        
    </fields>                                           
</register>

The important part is <size>1</size>, which specifies the size of the register in bits. In the case of my specific SVD file, this is wrong and the value needs to be 8. But according to @kjetilkjeka, this is valid SVD.

The code that svd2rust generates is clearly wrong. Here it is again:

#[doc = "0x00 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b0: B,
_reserved0: [u8; 1usize],
#[doc = "0x01 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b1: B,
_reserved1: [u8; 1usize],
...
#[doc = "0x1b - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b27: B,
_reserved27: [u8; 1usize],
#[doc = "0x1c - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b28: B,

Struct B is 8 bits wide:

pub struct B {
    register: VolatileCell<u8>,
}

So instead of generating 29 registers that each are 1 bit wide, svd2rust generates 29 8-bit registers with 8-bit offsets in between.

For the record, if I change <size>1</size> to <size>8</size> (which is the correct value in my SVD file), I get this:

#[doc = "0x00 - Byte pin registers port 0; pins PIO0_0 to PIO0_28"]
pub b: [B; 29],

Much nicer.

I started looking into the problem (second-to-last paragraph in that comment). No idea if my analysis still applies to the current code.

So, to summarize the summary: svd2rust silently produces garbage, if the size of a register is specified as 1 bit.

The easy solution would be to fail with an error message. Not ideal, but much better than the current situation, as the wrong offsets can mess up a whole peripheral.

@kjetilkjeka suggested a better (but less easy) solution:

perhaps the better way to handle it would be to create a u8 that acted both as the "data bit" and padding.

For my case, failing with an error is good enough.