regfiles instantiated with interleaved registers fail validation

[bstreiff]

With:

Name: systemrdl-compiler
Version: 1.0.0

And the following test case:

addrmap test {
        default regwidth = 8;
        reg reg1_t {
                field {} FIELD1[7:0];
        };
        reg reg2_t {
                field {} FIELD2[7:0];
        };
        regfile file_t {
                reg1_t          REG1    @ 0x0;
                reg2_t          REG2    @ 0x8;
        };
        file_t          FILE1   @ 0x10;
        file_t          FILE2   @ 0x11;
};

I would expect this to elaborate with four registers:

• FILE1.REG1 at 0x10 (0x10 + 0x0)
• FILE1.REG2 at 0x18 (0x10 + 0x8)
• FILE2.REG1 at 0x11 (0x11 + 0x0)
• FILE2.REG2 at 0x19 (0x11 + 0x8)

However, I get the following error:

rdl/test.rdl:14:9: error: Instance 'FILE2' at offset +0x11:0x19 overlaps with 'FILE1' at offset +0x10:0x18
        file_t          FILE2   @ 0x11;
         ^^^^^
error: Elaborate aborted due to previous errors

The validator treats the regfile as a contiguous address space that isn't allowed to overlap. However, while the SystemRDL spec states that registers may not (generally) overlap (10.1.h), I can't find anything that states that regfiles may not do so. It seems like as long as the resulting registers meet the constraints in 10.1, than this construction should be valid. (There may be something that I'm missing here though.)

(My current workaround is to just ditch the regfile and instantiate the registers directly.)

[amykyta3]

Thanks for your feedback! You're right that the current validator performs a naive overlap check on regfile and addrmap regions. Since the spec doesn't explicitly address the recommended behavior in this situation, it is somewhat left open to interpretation.

For now, I went with the simpler/stricter interpretation for the following reasons:

• Allowing interleaved register files opens up all kinds of complexities when it comes to hierarchical output generators.
  • Consider a C/C++ header generator that builds a hierarchical typedef struct overlay. Using just nested structs, it is not possible to represent interleaving regfiles. (some situations may be able to get away with using unions, but it can get ugly real fast)
  • Other output formats may have similar issues with the interleaved concept.
• I wanted to keep the validator simple for now.
  • Proof of no address collisions gets significantly more complex/costly if I need to do deep inspection of objects. This gets really bad if you start considering arrays of interleaving regfiles.
  • Current validation scheme is fast, and has significantly fewer corner cases.
• Is it even necessary?
  • I'm not familiar with any situations where such interleaving would be useful. At first glance, my impression is that it would make a register model unnecessarily unwieldy, and potentially confusing to the reader.

To the last point, I'm interested in hearing more about your particular use-case.

• In what situations would such interleaving be preferred over other alternatives?
• Why would it improve readability/understandability of a register model?
• What problem would regfile interleaving solve?

If theres good enough justification to support interleaving, I'll certainly consider it.

[bstreiff]

Yeah, it's entirely possible that this isn't a majority use case. I'm a bit new to SystemRDL. What I'm trying to do to is model the registers of the YM2612, documentation for which can be found at http://www.smspower.org/maxim/Documents/YM2612

I freely admit that I find the ordering on some of these registers to be quite odd and I'd probably sort them differently if I had the chance, but unfortunately I'm at least three decades too late to have any sort of chance to provide feedback to Yamaha. 😄

My current stab at describing the register set is: https://github.com/bstreiff/genesis-rdl/commit/a1f0fe0b7a3eac83ee1a7cbaaf924c1bf471ec34

Registers 0x30:0x9F are repeated out multiple times in several sets of Channel/Operators, so, given the definitions for the registers:

reg detmul_t {
    field {} DT1[6:4];
    field {} MUL[3:0];
};
reg tl_t {
    field {} TL[6:0];
};
...

my first attempt was to try and stick these into a register file:

regfile chXopY_t {
    detmul_t DETMUL @ 0x00;
    tl_t TL @ 0x10;
    rateh_t RATEH @ 0x20;
    //...
};

and instantiate as such:

chXopY_t CH1OP1 @ 0x30;
chXopY_t CH2OP1 @ 0x31;
chXopY_t CH3OP1 @ 0x32;
chXopY_t CH1OP2 @ 0x34;
chXopY_t CH2OP2 @ 0x35;
chXopY_t CH3OP2 @ 0x36;
//...

This way each of the registers would be "grouped" under their appropriate ch/op. The idea is that a C header generator that takes this as input could then, in turn, treat these as:

#define YM2612_REG_CH1OP1_DETMUL 0x30
#define YM2612_REG_CH1OP1_TL 0x40
#define YM2612_REG_CH1OP1_RATEH 0x50
// ...
#define YM2612_REG_CH2OP1_DETMUL 0x31
#define YM2612_REG_CH2OP1_TL 0x41
#define YM2612_REG_CH2OP1_RATEH 0x51
// ...

That said, I suppose another valid representation would be to look at the problem from a different angle, and present the ch/ops as coiterated arrays of registers:

detmul_t DETMUL[16] @ 0x30;
tl_t TL[16] @ 0x40;
rateh_t RATEH[16] @ 0x50;
// ...

This would need some other sort of association from array index to chXopY index (maybe an enum index { CH1OP1 = 0; CH2OP1 = 1, ... };?)... plus every fourth array entry doesn't actually exist.

[amykyta3]

Aah I see what you're trying to do now. Yeah. Older devices aren't always known for their sensible address layouts (if only they transposed the CH/OP indexes!).

Given the weird memory layout of those OP registers, I think they way you have it laid out currently is probably the most reasonable way to do it. Otherwise you'll be trading away readability for conciseness. I guess it all depends on what will make it easiest for your end-aplication.

Unrelated: I noticed the way your Makefile calls validate.py, you're effectively only validating the addrmap from genesis.rdl. Addrmaps of the other two are getting defined, but elaboration only "instantiates" the last one (see top_def_name parameter of elaborate()). I assume your intent was to elaborate and validate each one individually.

[bstreiff]

It was! Thanks for the catch.