Improved ioreg macro

[bgamari]

This is the beginning of a syntax extension to generate register definitions with type-safe bitfields and enable opportunistic use of bitbanding.

A register set is defined in a domain-specific language parsed by the ioregs! macro. There are three primitives in this language,

• registers are defined by a name, an offset from the beginning of
  the containing group, and a type. Register types include group of registers and primitive bitfield types ( reg32, reg16, reg8)
• fields are a range of bits within a bitfield, identified by a
  name and a type (either implicitly uint or bool, or an syntactically explicit enum)

Register and field types can both be arrays.

For example, consider this register set. This would be translated to the proposed DSL as follows,

ioregs!(
    FTM = {
        /// FlexTimer module.
        ///
        /// This is a timer module
        0x0  => reg32 SC  /// Status and control register
        {
             0..2 => PS,           /// Prescale
             3..4 => CLKS
             {
                 0x0 => NO_CLOCK,     /// no clock selected
                 0x1 => SYSTEM_CLOCK, /// use system clock
                 0x2 => FIXED_FREQ,   /// use fixed frequency clock
                 0x3 => EXTERNAL,     /// use external clock
             },
             5    => CPWMS,
             6    => TOIE,
             7    => TOF: ro,
        },

        0x4  => reg32 CNT /// Count register
        {
            0..15 => COUNT,
        },

        0x8  => reg32 MOD /// Modulo register
        {
            0..15 => MOD,
        },

        0xc  => group CH[8]                 /// Compare/capture channels
        {
            0x0 => reg32 CSC                /// Compare/capture channel status and control register
            {
                0 => DMA,
                2 => ELSA,
                3 => ELSB,
                4 => MSA,
                5 => MSB,
                6 => CHIE,
                7 => CHF,
            },

            0x4 => reg32 CV                 /// Channel counter value
            {
                0..15 => VAL,
            },
        },

        0x4c => reg32 CNTIN                 /// Counter initial value register
        {
            0..15 => INIT,
        },

        0x50 => reg32 STATUS                /// Channel status register
        {
            0..7 => CHF[8],                 /// Channel flags
        },
    }
)

which should produce something like,

pub struct FTM_SC     { _value: VolatileCell<u32> }
pub struct FTM_CNT    { _value: VolatileCell<u16> }
pub struct FTM_MOD    { _value: VolatileCell<u16> }
pub struct FTM_CNTIN  { _value: VolatileCell<u16> }
pub struct FTM_STATUS { _value: VolatileCell<u8> }

#[allow(uppercase_variables)]
pub struct FTM {
    pub SC: FTM_SC,
    _pad0: [VolatileCell<u8>, ..1],
    pub CNT: FTM_CNT,
    _pad1: [VolatileCell<u8>, ..1],
    pub MOD: FTM_MOD,
    pub CH: [Channel, ..7],
    _pad2: [VolatileCell<u8>, ..2],
    pub CNTIN: FTM_CNTIN,,
    _pad3: [VolatileCell<u8>, ..3],
    pub STATUS: FTM_STATUS
}

impl FTM_SC {
    pub fn set_PS(value: uint) {
        self.value.set((self.get_PS() & !0b111) | (&0b111 & val as u32) << 0);
    }
    pub fn get_PS() -> uint {
        (self.value.get() >> 0) & 0b111
    }
    ...
}

impl FTM_CNT { ... }
impl FTM_MOD { ... }
impl FTM_CNTIN { ... }

impl FTM_STATUS {
    pub fn set_CHF(n: uint, value: bool) { .. }
    pub fn get_CHF(n: uint) -> bool { ... }
}

#[allow(uppercase_variables)]
pub struct Channel {
    _CSC: VolatileCell<u32>,
    _CV:  VolatileCell<u32>,
}

impl Channel { ... }

Lowering semantics

Lowering the DSL representation to a Rust data structure would be performed as follows,

• For a register group GROUP produce a type pub struct GROUP { ... }
  where the fields are generated as follows,
  • For each register REG in the group,
    • If the register has a group type T, produce a field pub REG: T
    • If the register has an array group type T[N], produce a
      field pub REG: [T, ..N]
    • If the register has a primitive type T or T[N], produce a type
      pub struct GROUP_REG {_value: VolatileCell<T>} and,
      • If the register has an array type T[N], produce a field
        pub REG: [GROUP_REG, ..N] in struct GROUP
      • Otherwise, produce a field pub REG: GROUP_REG in struct GROUP

To generate the accessor functions, the following lowering would be performed,

• For a register group GROUP,

  • For each register REG in the group,
    • For each field FIELD of the register,
      • If the field has array type T[N], produce accessors

  pub fn get_FIELD(n: uint) -> T { ... }
  pub fn set_FIELD(n: uint, value: T) { ... }

    * Otherwise produce accessors
  

  pub fn get_FIELD() -> T { ... }
  pub fn set_FIELD(value: T) { ... }

[farcaller]

So, width is in bytes and pad is in bits? Any reason for that?

[farcaller]

CH: Channel[8],

What does [8] mean?

[bgamari]

@farcaller, I had intended for both width and pad to be in bits.

[bgamari]

@farcaller, Channel[8] means an array of Channels of length 8.

[farcaller]

Can you cherry-pick 7cbad1e on top of current master? I suppose that other commits are not related to ioreg.

[farcaller]

So, CH: Channel[8] is an array of 8 Channels, but SC: struct { ... } is like one object inline?

[bgamari]

@farcaller, I just elaborated a bit in the pull request message.

[bgamari]

@farcaller to summarize, field types are defined by an element type and a repeat count. The syntax for this is TYPE[COUNT]. TYPE can be one of three field types,

• a primitive type with width such as uint(3) or u16 (which would desugar to uint(16))
• struct { ... } containing subfields. For ease of implementation the length of a struct will likely be limited to multiples of a byte.
• enum(WIDTH) { value1 = 0x1, value2 = 0x2, ... } which defines a fixed domain of symbolic values

[bgamari]

@farcaller, I've cleaned up and updated the text and example above. Let me know what you think.

The biggest syntactic issue I see is the fact that the count always appears after the type as this ends up being a bit awkward in the case of long structs (e.g. Channel above). There are a couple of ways to fix this,

1. move the size to after the field name. e.g.,

ioregs!(FTM,
    ...
    CH[8]: struct Channel { ... },

1. allow structs to be defined independently from their fields. e.g.,

ioregs!(FTM,
    ...
    struct Channel { ... },
    CH: Channel[8],

Otherwise, I'm currently grappling with how to specify the memory map and use bitbanding. At the moment I'm thinking the memory map (including bitband windows) can be specified in a external file in a structured format like JSON. Bitbanding will be used in single-bit field accessors for peripheral instances covered by a bitband window. As not all peripheral instances fall in a bitband window, we may need to emit a separate type for each peripheral instance (e.g. FTM0 and FTM1).

[farcaller]

So I see two things that confuse me so far:

• struct in the definitions look like out of the line constructs. top-level ioreg is a struct as well, but an implicit one, why do we need to define other structs explicitly?
• [8] in the end just gets lost.

We are not the only ones trying to come up with a meta-language for peripheral definitions, there's at least this one. I don't say we need to generate that from yaml or whatever, the more things stay in rust code, the more we push rust to the limits.

[farcaller]

Let's say, if you don't think about code and accessing the peripheral, how would you like to see it's definition? We can convert a definition of about any complexity down to code, but it must be simple to read. This is about the only place we cannot cover with unit tests, so there would be errors in ioregs. We need to minimise the margin for error, to the primary goal for a new ioreg is to be simple and readable.

[bgamari]

@farcaller, is your first point just a syntactic concern? Perhaps just dropping the struct keyword would help? There's no good reason for it to be there; as long as enum remains there will be no syntactic ambiguity.

I absolutely agree with your point about the repeat count (e.g. [8]); in fact, I tried to make the same point above where I proposed a couple ways of fixing it. Opinions?

[bgamari]

@farcaller, so far I've been trying to guide the syntax towards being easy to read (although that's not to say that I've succeeded). In my mind, the current syntax (up to minor changes like dropping the struct keyword) is not bad. Beyond assert_length I've not yet explored the possibilities for introducing redundancy into the definition to validate its correctness, but this shouldn't be difficult to do.

[bgamari]

@farcaller, in the process of formalizing the semantics I've realized that this representation might not be quite sufficient. In particular, we really don't have enough information to unambiguously choose an access width. In practice this may not be a problem since even on the rather quirky K20 I haven't noticed any architectural assumptions on access width. That being said, there may very well be an architecture that does care.

The problem is that currently structs are used both to indicate the bounds of registers (e.g. FTM::SC in the above example) and for grouping related registers (e.g. FTM::CH above). Ideally we'd want to distinguish these uses so that the former can be used to determine the access width. Perhaps we could use the struct keyword to distinguish these two cases (perhaps using struct { ... } to mark groups and just braces to denote registers)? The only trouble is the semantics of the keyword become a bit non-obvious. Alternatively, we could just depart from Rust's keyword set and define group to be a keyword.

The trouble with this is that your point regarding the inconsistent use of struct (implicit at the top level and explicit otherwise) remains since the top-level would really be a group, not a register. One solution to this would be to just make the top-level group explicit.

Here is my latest proposed syntax, moving the repeat count behind the field name and using an explicit group keyword everywhere, https://gist.github.com/bgamari/18855cf909756b2df05a

[bgamari]

Alternatively, if we really wanted to leave Rust syntax behind, we could try something like the following. It has the advantage of not requiring explicit padding, is a bit easier to read due to its tabular nature, and can be trivially read off of most datasheets. Moreover, the lowering semantics are obvious.

To summarize, registers are introduced by their offset marked with a @. Fields are defined with their bit-range, a name, and a type. Groups of registers can be defined explicitly.

ioregs!(FTM =
    @0x0        SC
        0..2    PS:     uint
        3..4    CLKS:   enum { 0x0 = NO_CLOCK, 0x1 = SYSTEM_CLOCK, 0x2 = FIXED_FREQ, 0x3 = EXTERNAL }
        5       CPWMS:  bool
        6       TOIE:   bool
        7       TOF:    bool

    @0x4        CNT
        0..15   COUNT:  uint

    @0x8        MOD
        0..15   MOD:    uint

    group Channel {
        @0x0    CSC    "channel status and control register"
            0       DMA:    bool    "enable DMA"
            2       ELSA:   bool    "another docstring"
            3       ELSB:   bool
            4       MSA:    bool
            5       MSB:    bool
            6       CHIE:   bool
            7       CHF:    bool

        @0x4    CV
            0..15   VAL:    uint
    }

    @0xc        CHANNELS: Channel[8]

    @0x4c        CNTIN
        0..15   INIT:     uint

    @0x50        STATUS
        0..7    CHF:      bool[8]
)

[errordeveloper]

I do like the last version, although white space seem a bit vague... I'd introduce some additional delimiters, e.g.:

ioregs!(FTM =
    @(0x0) => SC: {
        <0..2>    PRESCALE: uint
        <3..4>    CLOCKS: enum {
                          0x0 = NO_CLOCK,
                          0x1 = SYSTEM_CLOCK,
                          0x2 = FIXED_FREQ,
                          0x3 = EXTERNAL,
        }
        <5>       CPWMS: bool
        <6>       TOIE: bool
        <7>       TOF: bool
     } // end of SC
)

Making bool a default would probably make sense...

[errordeveloper]

Another though, a side from defining an enum inline like this, where the value may be only one of those things, one would want to have something similar to Rust's bitflags! defined inline like that also.

[errordeveloper]

I do very much like the doc strings, I have to say!

[farcaller]

I can think a bit more straight after a nap so I got you a proposal on syntax. If it looks a bit like PTs, it's just because if only thing you have is a hammer... 😉

I really tried to use the same register from k20 but I couldn't find my way through freescale docs. I re-read the chapter a dozen times but I still can't figure where do you put those 8 regs in memory. So I took lpc's uart register which is quite fun (and has different compatibility issues so we have something to compare to).

Another thing to note is safety. I plan to introduce better unsafe usage rules to zinc runtime. Given that PT will [hopefully] shift the burden of input validation to compile time, all the runtime functions would be stripped of any checks (to make them faster), and will be marked unsafe. Same goes for full-width register access (i.e. writing u32 into it), as it must be fast (one STR), so it should be unsafe as well. What I'd like to have is something similar in capabilities to format! macro, that verifies that arguments match the format string.

https://gist.github.com/farcaller/30b04556330ee93d55c4

[errordeveloper]

@farcaller this looks better to me. The only thing I'm not quite sure about is pad 2 { write = 0 }... Why should a sane user have to consider either reading or writing to a reserved register? Surelly if they know of a magic memory location they absolutely have to write to, they will figure out the way of accessing it! I think paddings should inaccessible, full stop.

[farcaller]

I just dropped it in there for completeness, e.g. if you read-modify-write, those bits can be forced 0 in mask.

[bharrisau]

Sorry for not contributing much recently. IMHO, any macro that requires 'pad' is a non-starter. We have the power of syntax extensions, lets have a more clever solution that counting the correct number of pad invocations.

I don't mind any of the proposed syntaxes that have explicit offsets for ranges and bit number for fields. @bgamari did you want to double check your proposed syntax is as easy to map from datasheet to Rust for LPC and STM32 as it was for Kinetis? At then end of the day we want a solution that makes it quick and simple to transfer from the datasheet text to Rust, to minimise errors and make it quicker to port to new devices (and if it is modular-ish with offsets, it makes copy paste easier for similar but different platforms).

[bharrisau]

closes/cc #56

[farcaller]

any macro that requires 'pad' is a non-starter. We have the power of syntax extensions, lets have a more clever solution that counting the correct number of pad invocations

So are you against pad operation or the cursor concept? Apparently we have two options: specify offset / size explicitly or make some autoincrementing assumptions, both of those are prone to errors at some level.

[bharrisau]

Both the pad and cursor (so for both register definition and register location). Datasheets give the addresses/offsets explicitly, we shouldn't make ourselves have to count how many registers are padding in a spare peripheral definition.

At least the error checking for specifying offset/size exactly is easier to do, to error check padding I need to calculate the padding on the datasheet and also on the definition. Also, if I am debugging the assembly, when the offset is explicit, I can see it in the asm. Otherwise I need to keep counting.

[bgamari]

Good morning everyone!

@bharrisau, I'll admit that I didn't look as carefully at the other two platforms although checking again I don't see any issues. My second representation is literally exactly what most datasheets I've read provide.

I agree that explicit offsets is better than explicit padding. It's both easier to write and validate against the datasheet. That being said, I'm not entirely sure we need the padding at all if we take this route.

[bharrisau]

Probably not worth worrying too much about at the moment - but Rust gives us the ability to do things like

reg.set().field_a(42).field_b(3).write();
// Or
reg.set(field_a(42) + field_b(3));

Otherwise a macro_rules! could handle this for us.

[bharrisau]

Not sure if the Drop trait would let you have something like this (removing the need for the .write() call).

reg.set().field_a(42).field_b(3);

If that worked it would be good. set() would return a struct that accumulated the value and the mask, then update the register when the struct fell out of scope (the end of the line?). I'd have to check when the drop is called, it may end up being the end of the block.

Edit: The set() could probably be dropped also.

FTM.SC.CLKS(NO_CLOCK).CPWMS(true);

// Getters
let clk = FTM.SC.CLKS();
let cpwms = FTM.SC.CPWMS();

All compile time checked, and inlined into operations on two u32 values; the val and mask.

[bgamari]

@bharrisau I had considered something like that as well, unfortunately I seem to recall that drop is called at the end of the block. While you could technically just put the update in a block, it seems this would be a bit error-prone.

[bharrisau]

Have to check - I think for temporaries it is called much sooner. http://smallcultfollowing.com/babysteps/blog/2014/01/09/rvalue-lifetimes-in-rust/

[bgamari]

@bharrisau this was my attempt at checking this, https://gist.github.com/bgamari/4d56b64d64ae743b6939. The println! occurs before the drop.

[bharrisau]

What if it is without the let? I'm thinking the explicit .write() is probably better anyway for a few reasons.

[bgamari]

@bharrisau oh, you're right! as long as you it's a temporary it seems to be dropped at the end of the statement.

[bharrisau]

Now that we have agreement on the ioreg! format - lets do a new pull request for the actual API? Once @farcaller confirms the union question for the ioreg! format.

In terms what I want in the ioreg! format, I can propose them as optional extensions later

• Write only clear/set/toggle aliases
• Bit banding where supported (can probably handle through weak linkage now)
• Atomicity of read/modify/write accesses (or memory barrier requirements)
• Aliases for smaller registers (for registers supporting multiple access sizes)

Also note: we can now have an ioreg lint, to make sure people aren't using a let and messing up the drop semantics.

[bgamari]

@bharrisau sounds like a plan.

[farcaller]

So yes, we can do the units (I'm mostly concerned about register unions though) with overlapping indexes, but overlapping indexes are also a source of errors so I'd prefer if it would be explicit.

commas after blocks and commas after terminal element of a block are now optional

Lets not have any optional syntax, there's really no reason for having two options here.

[bgamari]

@farcaller the reason for the optional syntax was to allow consistency between definitions with and without blocks and it was easy to do.

@bharrisau this probably belongs on the (to-be-created) API pull request, but anyways here is my implementation of the read-modify-write interface. RegValue is a helper type and AReg1 would be generated by the syntax extension. I was hoping to use auto-derefing to avoid the need for AReg1::get but unfortunately Deref doesn't permit new values to be returned due to the reference. Oh well.

[bharrisau]

Let's move this to the mailing list for now.

[bharrisau]

The macro syntax will need flags for write-to-clear registers. Something
like 'writeclear' where the wo and ro flags are. The read-modify-write
needs to know to ignore them during a read so it doesn't write the 1 back
and clear the flag.

[farcaller]

What do we still need to figure out to make this happen?

[bgamari]

Closed is favor of new pull request so we can start discussing API considerations.

[bgamari]

@farcaller I just squashed together the patches. Next I intend on adding support for @bharrisau's setter interface. After that I'll add write-to-clear support.

[bharrisau]

Thanks for all this @bgamari. Don't feel like you need to get it all in one go.

[bgamari]

@bharrisau how does this look?

[bharrisau]

Not sure why you are trying to do it that way. Why do you want to do the
register read so soon? The example I posted on the mailing list does the
read just before the modify-write (and gets rid of the .get all together).

[bgamari]

@bharrisau ahh, I hadn't noticed that property of your approach until now. That is clearly the right way to do things. You unfortunately loose the ability for the accessor type to support getters, but I suppose these can be moved into a separate type which. In hindsight, it probably makes more sense for getting and setting to be distinguished in the type system.

[bgamari]

@bharrisau I just updated the gist. What do you think now?

[bharrisau]

Line 88 is redundant.
I would move the clear mask to the new function (line 42) - set the initial
mask value to (1 << 5).

I get why/where the .get() would be useful (to take a snapshot of the
register). Can you also add shortcut functions to enable reg.get_field1().

On 15 July 2014 22:11, Ben Gamari [email protected] wrote:

@bharrisau https://github.com/bharrisau I just updated the gist. What
do you think now?

—
Reply to this email directly or view it on GitHub
#90 (comment).

[bharrisau]

Hopefully most of this inlines away normally, otherwise it might need
liberal application of some #inline_always stuff.

On 16 July 2014 07:14, Ben Harris [email protected] wrote:

All the structs need to be for the size (for non-u32 registers).
Line 88 is redundant.
I would move the clear mask to the new function (line 42) - set the
initial mask value to (1 << 5).

I get why/where the .get() would be useful (to take a snapshot of the
register). Can you also add shortcut functions to enable reg.get_field1().

On 15 July 2014 22:11, Ben Gamari [email protected] wrote:

@bharrisau https://github.com/bharrisau I just updated the gist. What
do you think now?

—
Reply to this email directly or view it on GitHub
#90 (comment).

[bharrisau]

And you can probably dump the check on line 71.

On 16 July 2014 07:15, Ben Harris [email protected] wrote:

Hopefully most of this inlines away normally, otherwise it might need
liberal application of some #inline_always stuff.

On 16 July 2014 07:14, Ben Harris [email protected] wrote:

All the structs need to be for the size (for non-u32 registers).
Line 88 is redundant.
I would move the clear mask to the new function (line 42) - set the
initial mask value to (1 << 5).

I get why/where the .get() would be useful (to take a snapshot of the
register). Can you also add shortcut functions to enable reg.get_field1().

On 15 July 2014 22:11, Ben Gamari [email protected] wrote:

@bharrisau https://github.com/bharrisau I just updated the gist. What
do you think now?

—
Reply to this email directly or view it on GitHub
#90 (comment).

[bharrisau]

Ignore the bit about , was too early in the morning and I wasn't thinking.