im-tomu / fomu-pac-rs

A simple Rust Peripheral-Access-Crate for Fomu running the foboot-2.0 MCU
Other
2 stars 1 forks source link

Generated registers have wrong addresses #1

Closed Tiwalun closed 4 years ago

Tiwalun commented 4 years ago

I tried updating the workshop example in im-tomu/fomu-workshop#94 to use the new version, but it didn't work with the new example.

After looking at the code, it seems that the register sizes in the new fomu.svd file lead to issues in the code generated by svd2rust.

For example, the new register block for the RGB peripheral looks like this:

pub struct RegisterBlock {
    #[doc = "0x00 - This is the value for the ``SB_LEDDA_IP.DAT`` register. It is directly written into the ``SB_LEDDA_IP`` hardware block, so you should refer to http://www.latticesemi.com/view_document?document_id=50668. The contents of this register are written to the address specified in ``ADDR`` immediately upon writing this register."]
    pub dat: DAT,
    _reserved1: [u8; 3usize],
    #[doc = "0x04 - This register is directly connected to ``SB_LEDDA_IP.ADDR``. This register controls the address that is updated whenever ``DAT`` is written. Writing to this register has no immediate effect -- data isn't written until the ``DAT`` register is written."]
    pub addr: ADDR,
    _reserved2: [u8; 4usize],
    #[doc = "0x08 - Control logic for the RGB LED and LEDDA hardware PWM LED block."]
    pub ctrl: CTRL,
    _reserved3: [u8; 4usize],
    #[doc = "0x0c - Normally the hardware ``SB_LEDDA_IP`` block controls the brightness of the LED, creating a gentle fading pattern. However, by setting the appropriate bit in ``CTRL``, it is possible to manually control the three individual LEDs."]
    pub raw: RAW,
}

Note that the _reserved fields have a width of 4 bytes, which means that the offsets are wrong for the CTRL and RAW fields. The right size for the reserved fields would be 3 bytes, as one byte is taken up by the actual value itself. In contrast, version v0.0.1 had the following block, where all fields are 32 bit values:

pub struct RegisterBlock {
    #[doc = "0x00 - This is the value for the ``SB_LEDDA_IP.DAT`` register. It is directly written into the ``SB_LEDDA_IP`` hardware block, so you should refer to http://www.latticesemi.com/view_document?document_id=50668. The contents of this register are written to the address specified in ``ADDR`` immediately upon writing this register."]
    pub dat: DAT,
    #[doc = "0x04 - This register is directly connected to ``SB_LEDDA_IP.ADDR``. This register controls the address that is updated whenever ``DAT`` is written. Writing to this register has no immediate effect -- data isn't written until the ``DAT`` register is written."]
    pub addr: ADDR,
    #[doc = "0x08 - Control logic for the RGB LED and LEDDA hardware PWM LED block."]
    pub ctrl: CTRL,
    #[doc = "0x0c - Normally the hardware ``SB_LEDDA_IP`` block controls the brightness of the LED, creating a gentle fading pattern. However, by setting the appropriate bit in ``CTRL``, it is possible to manually control the three individual LEDs."]
    pub raw: RAW,
}

The problem seems to be caused by the "strange" bit sizes in the svd, e.g. the register with size 4 here:

<register>
    <name>ADDR</name>
    <description><![CDATA[This register is directly connected to ``SB_LEDDA_IP.ADDR``.  This register
controls the address that is updated whenever ``DAT`` is written.  Writing to
this register has no immediate effect -- data isn't written until the ``DAT``
register is written.]]></description>
    <addressOffset>0x0004</addressOffset>
    <resetValue>0x00</resetValue>
    <size>4</size>
    <access>read-write</access>
    <fields>
        <field>
            <name>addr</name>
            <msb>3</msb>
            <bitRange>[3:0]</bitRange>
            <lsb>0</lsb>
        </field>
    </fields>
</register>

The wrong generation of the padding fields seems to be an issue with svd2rust, but I think it might also be better to view these registers as 8 bit registers in the SVD, where only part of the register is used.

xobs commented 4 years ago

Is this a limit of svd2rust then? These registers do require a 32-bit write, but are only 8-bits wide.

This change is a result of https://github.com/xobs/lxsocdoc/commit/4883e9907ad9e73077b5465a7a2c8733faa454b7

david-sawatzke commented 4 years ago

Seems to be the fault of svd2rust, we should probably round the size up in lxsocdoc.

The issue seems to be here, which truncates the number instead of rounding up.

david-sawatzke commented 4 years ago

@Tiwalun Pushed the svd (&generated code) with 8 bit multiple register sizes.

Seems to work for me locally, could you verify if it works for you?

Tiwalun commented 4 years ago

@david-sawatzke Seems to work fine now, thanks for the fix!

I'm not sure if declaring the registers as 32 bit would be better. Right now, rustc seems to emit lb to load the register values, but if they are actually 32 bit register, it might be better to use lw instructions, i.e. just load the whole word.

@xobs Which kind of load would be better on the vexriscv CPU?

xobs commented 4 years ago

At this point, it actually doesn't matter. From a hardware perspective, the difference between lb and lw are which of the SEL lines are active, and the wishbone2csr module doesn't even consult the SEL line: https://github.com/enjoy-digital/litex/blob/master/litex/soc/interconnect/wishbone2csr.py