esynr3z / corsair Goto Github PK

as we all know from the standard (or references), the compiler is free to arrange the data as it likes[1].
it seems advisable to put a pragma pack around the structs and annotate the struct members with attribute packed ?

it doesn't suffice to just access the members properly (like in [2]) - we also need to guarantee that the bits are in the same location as they are for the FPGA side.

the only way to fix this that i know of would be to generate packing/un-packing code that uses e.g. logical operations, but that's quite tedious. it might be enough to be able to detect this issue, though? (not that that's much easier...)

thoughts?

[1] https://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf §6.7.2.1.10:
"An implementation may allocate any addressable storage unit large enough to hold a bit-
field. [...] The order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined. The alignment of the
addressable storage unit is unspecified."

for GCC it depends on the ABI: https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html

[2] https://wiki.sei.cmu.edu/confluence/display/c/EXP11-C.+Do+not+make+assumptions+regarding+the+layout+of+structures+with+bit-fields

i think it would be nice to be able to write this in the config

[globcfg]
base_address = MY_SYMBOLIC_CONSTANT

or maybe even

[globcfg]
base_address = MY_SYMBOLIC_CONSTANT + SOME_OFFSET

and to get this in the C headers:

#define REGS_BASE_ADDR MY_SYMBOLIC_CONSTANT

or

#define REGS_BASE_ADDR (MY_SYMBOLIC_CONSTANT + SOME_OFFSET)

respectively. maybe we don't need to be able to handle arbitrary strings, but it's probably the same amount of effort (?)

does anybody else like the idea?

Queue q hardware interface with ro access misses read when data is already available. Tested on vhdl target.

The waves:

Here, zeros are read instead of FFFEFF+ or 000164+. That happens because of:

rvalid_drv <=
    csr_prediction_prediction_rvalid_ff when (csr_prediction_ren = '1') else
    rvalid_ff;

Hi! First of all, thanks for the great work so far!

I would like to add a generator for CMSIS SVD files (a register description format for debugging, see here).

I've already done most of the work. The only thing is, that SVD wants to know, in which peripherals the registers are, e.g.

<peripherals>
    <peripheral>
        <!-- some other stuff -->
        <registers>
            <!-- list of registers for this peripheral -->
        </registers>
    </peripheral>
    <peripheral>
        <!-- second peripheral with list of registers -->
    </peripheral>
</peripherals>

At the moment this information is not provided in the register map. In order to minimize the impact on the current defintion and also be backwards compatible, I would suggest to just add an entry "peripheral" to each register, like so:

{
        "name": "CSR_NAME",
        "description": "",
        "address": "0x20",
        "peripheral": "my_peripheral",
        "bitfields": []
}

What's your opinion? Or is this not wished at all? Then I would just leave it on my fork.

csrconfig:

[globcfg]
base_address  = 0
data_width    = 16
address_width = 8
register_reset = async_neg
address_increment = none
address_alignment = none
force_name_case = none
regmap_path = exam.yaml

[v_module]
generator = Verilog
path = exam.v
read_filler = 0
interface = lb

exam.yaml:

regmap:
-   name: EXAM
    description: Examinate
    address: 1
    bitfields:
    -   name: exam
        description: |
            Examinate multistring values in yaml:
            0 - disable
            1 - enable
        reset: 0
        width: 1
        lsb: 0
        access: wo
        hardware: o
        enums: []
    -   name: none
        description: none description
        reset: 0
        width: 1
        lsb: 1
        access: rw
        hardware: io
        enums: []

exam.v:

...
//---------------------
// Bit field:
// EXAM[0] - exam - Examinate multistring values in yaml:
0 - disable
1 - enable

// access: wo, hardware: o
//---------------------
...

Syntax check:

verible-verilog-syntax playground/exam.v

playground/exam.v:51:1: syntax error at token "0"
playground/exam.v:52:1: syntax error at token "1"

At this moment if I correct understand the lanes for wavedrom bitfield creation sliced for 16 bits per lane:
https://github.com/esynr3z/corsair/blob/master/corsair/generators.py#L104

In generic case it is good solution but in some specific cases (almost all) would be nice manually specify number of lanes in csrconfig.

Just an example: my most adorable documentation - any datasheet from Atmel company. They use config 8 bits per lane and it is convenient way to have enough volume to print field name (even if it's not the shortest name):

Would be nice to have such flexibility in future release 🙏🏻

It is connected to issue #28.

While for rolh and roll access modes not-latching event was fixed in #37, this problem still exists for roc. If roll/rolh could be compared to their latched value, we cannot do the same for roc.
So, the question is:
@esynr3z should roc always be accompanied by enable (ie hardware options)? I have not found any rules on this in the documentation, but I think it has little sense without both enable and input.

If you read from a 'rolh' while it's receiving a '1' the value will never be latched due to the priority in the logic.

if (csr_test_rolh_ren = '1') then
csr_test_rolh_rolh_ff <= '0';
elsif (csr_test_rolh_rolh_in = '1') then
csr_test_rolh_rolh_ff <= csr_test_rolh_rolh_in;
end if;

In my opinion, the setting of the latch shall have higher priority than the clearing. Otherwise, you could have a value that will never be latched or read from the register.

Does Corsair support constants and expressions? For example, I would like to define two module constants X and Y, and then I would like to calculate different values utilizing these constants in the expressions.

Does Corsair support interrupts? I do not mean exactly the same way SystemRDL does, but is there any support for interrupts?

Based on good practice for auto-generated source files would be great to automaticaly add informative header to all auto-generated entities as comment:

DO NOT EDIT THIS AUTOGENERATED FILE. ALL CHANGES WILL BE LOST.

VHDL

-- DO NOT EDIT THIS AUTOGENERATED FILE. ALL CHANGES WILL BE LOST.

Verilog, SV, C, Asciidoc

// DO NOT EDIT THIS AUTOGENERATED FILE. ALL CHANGES WILL BE LOST.

Markdown

[//]: # (DO NOT EDIT THIS AUTOGENERATED FILE. ALL CHANGES WILL BE LOST.)

or

<!---
DO NOT EDIT THIS AUTOGENERATED FILE. ALL CHANGES WILL BE LOST.
--->

MD references:

• https://stackoverflow.com/questions/4823468/comments-in-markdown
• https://www.w3schools.io/file/markdown-comments/

Based on good practice of usage auto-generated source files in VCS should store only sources ("input" to generate some auto-generated entities), therefore auto-generated sources should not added under VCS control. To avoid this, we must also generate a .gitignore file which included all auto-generated files.

Most convenient way to do this required to add new variable to csrconfig in [globcfg] section like BASEDIR - the root path for other sections. All another path variables from secondary sections should complement BASEDIR path to get full path.

In this case we can place .gitignore in BASEDIR and place path and files into .gitignore relative to BASEDIR.

I think this is the easiest way to achieve goal: auto-generated files cannot accidentally add under VCS control.

would be great to automaticaly add informative header to all auto-generated entities as comment:

The one feature I am missing the most in corsair is a memory interface. It would be very nice to have the ability to tie address range to a memory while having some of the registers. Such a feature exists in airhdl.

i haven't looked at the implementation, but i think that the padding that is generated just uses that item's lsb, forgetting to subtract how many bits have been allocated before.

e. g. in the example https://github.com/esynr3z/corsair/blob/master/examples/regmap_yaml/regs.yaml

- name: DATA
    description: Data register
    address: 4
    bitfields:
    -   name: FIFO
        description: Write to push value to TX FIFO, read to get data from RX FIFO
        reset: 0
        width: 8
        lsb: 0
        access: rw
        hardware: q
        enums: []
    -   name: FERR
        description: Frame error flag. Read to clear.
        reset: 0
        width: 1
        lsb: 16
        access: rolh
        hardware: i
        enums: []
    -   name: PERR
        description: Parity error flag. Read to clear.
        reset: 0
        width: 1
        lsb: 17
        access: rolh
        hardware: i
        enums: []

there should be 8b of padding between FIFO and FERR, and none between FERR and PERR, presumably, but:
https://github.com/esynr3z/corsair/blob/master/examples/regmap_yaml/sw/regs.h

typedef struct {
    uint32_t FIFO : 8; // Write to push value to TX FIFO, read to get data from RX FIFO
    uint32_t :16; // reserved
    uint32_t FERR : 1; // Frame error flag. Read to clear.
    uint32_t :17; // reserved
    uint32_t PERR : 1; // Parity error flag. Read to clear.
} csr_data_t;

(i originally noticed this in a different context, where i'd have expected no padding between two items. if the above example isn't valid, please let me know and i'll update with a different one.)

The vhdl code generated by corsair has ADDR_W generic defined, which allows alternating address width. When accessing a register, a comparison of addr is done to the constant of fixed width ADDR_W, like this:

csr_prediction_ren <= ren when (raddr = "000000000000") else '0'; -- 0x0

When I set ADDR_W to a lower value than the default in the parent entity, Vivado did not raise any error or warning, but in the resulting design register never worked and always returned OKAY read response with zeros as if it was an adress miss.

Instead of always returning ok, if trying to write to a read only address, or read a write only, or have a bad address return an error instead.

Running:

pytest tests/hdl/test_rmap.py

With vivado (2021.2) as the simtool results in something like:

ERROR: [XSIM 43-4100] ".../corsair/tests/hdl/test_rmap/tb_ro.sv" Line 3. Module tb_ro has a timescale but at least one module in design doesn't have timescale.

for the verilog tests.

Need to either remove the timescale from those files, or add to other files for the test to pass.

Hi!
First of all, thank you a lot for this project, it is so convenient!

I faced a small bug in the VHDL generator - the signal csr_REGNAME_ren was not generated in one of the registers but was used inside the generated VHDL file. The problem appears in "wo" fields with the hardware attribute set to "oa". Basically, the generated VHDL file is lacking the following:

--- a/bug_test.vhd
+++ b/bug_test.vhd
@@ -75,6 +75,7 @@ signal axil_rvalid_int : std_logic;
 
 signal csr_reg1_rdata : std_logic_vector(31 downto 0);
 signal csr_reg1_wen : std_logic;
+signal csr_reg1_ren : std_logic;
 signal csr_reg1_field1_ff : std_logic_vector(15 downto 0);
 signal csr_reg1_field2_ff : std_logic_vector(15 downto 0);
 
@@ -170,6 +171,7 @@ end process;
 --------------------------------------------------------------------------------
 
 csr_reg1_wen <= wen when (waddr = "000000000000") else '0'; -- 0x0
+csr_reg1_ren <= ren when (waddr = "000000000000") else '0'; -- 0x0
 
 -----------------------

I am using v1.0.2 installed by pip. Here are my corsair configs to reproduce the problem:

[globcfg]
data_width = 32
address_width = 12
register_reset = sync_neg

[vhdl_entity]
generator = Vhdl
interface = axil
path = bug_test.vhd

regmap:
  - name: peak
    description: None
    address: 0x0
    bitfields:
    - {name: field1, reset: 0, width: 16, lsb: 0,  access: wo, hardware: oa, description: None}
    - {name: field2, reset: 0, width: 16, lsb: 16, access: wo, hardware: oa, description: None}

Could it be that using two bitfields in one register with a hardware attribute set to "oa" cause some kind of collision?

Add an option to have a timeout on transactions. If read or write is not ready, it should timeout the transaction and return an error. Related to issue #8

Problem:

When generating an AXI-lite interface, the internal output interface for a bitfield with hardware: oa option, the signal csr_*_out is one clock cycle behind csr_*_waccess.

This prevents from using waccess as a data_valid input for downstream modules.

Solution:

In the Verilog template corsair/templates/regmap_verilog.j2 we can change the combinatorial assignment
assign {{ port_bf_waccess(reg, bf) }} = wready && {{ sig_csr_wen(reg) }};
to a registered one.

Is this behaviour a bug or done on purpose? If latter, what are the expected uses of this signal?

In config used "mixed" terminology: LBS + Width to specify fields size and position inside word.

this is contrary to conventional terminology when used couple of terms: MSB & LSB or Offset & Width.

As an example of this point: https://github.com/sifive/duh/blob/master/docs/component.md#registers

name: 'data', bitOffset: 0, bitWidth: 5

It would be nice to add the complementary and meaningful term offset to the term width (instead currently using lsb) 🤓

Is there any way to define custom enumeration types using Corsair?

Hello everybody,

Maybe I didn't get the point, but I see that base_address variable is used nowhere but in the C/Python headers and so on. Shouldn't the variable used also for RTL modules for absolute address calculation? Something like:

absolute_reg_address = base_address + reg_address

Example:

Corsair register

-   name: UID
    description: Device ID register
    address: 0
    bitfields:
    -   name: UID
        reset: 0xAABBCC01
        width: 32
        lsb: 0
        access: ro
        hardware: i
        enums: []

I connect corsair registers map to the AXI interconnect core. The address of the core in the AXI interconnect, let's say, 0x8000_0000. It is a "base address" of the core. And this case corsair's core doesn't accept input addresses 0x8xxx_xxxx becasue of 0x8. Corsair waits an address without base offset.

Thanks.

PS. For sure, there is a workaround with select only part of input address for Corsair, but still

Does Corsair support different addressing modes? I do not mean byte and word addressing, but compact, regalign and fullalign from SystemRDL nomenclature. Do you always align address space size of particular slave to the power of 2?

When creating a write-only register with "oa" hardware access options, an undeclared signal is referenced in the code.

--------------------------------------------------------------------------------
-- CSR:
-- [0x108] - TEST_WO_OA - Test rolh
--------------------------------------------------------------------------------
csr_test_wo_oa_rdata(31 downto 1) <= (others => '0');

csr_test_wo_oa_wen <= wen when (waddr = "00000000000000000000000100001000") else '0'; -- 0x108

-----------------------
-- Bit field:
-- TEST_WO_OA(0) - WO_OA - test signal
-- access: wo, hardware: oa
-----------------------
csr_test_wo_oa_wo_oa_waccess <= wready and csr_test_wo_oa_wen;
csr_test_wo_oa_wo_oa_raccess <= rvalid and csr_test_wo_oa_ren;  -- THIS LINE CAUSES ERRORS 

csr_test_wo_oa_rdata(0) <= '0';

csr_test_wo_oa_wo_oa_out <= csr_test_wo_oa_wo_oa_ff;

process (clk) begin
if rising_edge(clk) then
if (rst = '1') then
    csr_test_wo_oa_wo_oa_ff <= '0'; -- 0x0
else
        if (csr_test_wo_oa_wen = '1') then
            if (wstrb(0) = '1') then
                csr_test_wo_oa_wo_oa_ff <= wdata(0);
            end if;
        else
            csr_test_wo_oa_wo_oa_ff <= csr_test_wo_oa_wo_oa_ff;
        end if;
end if;
end if;
end process;

The same happens with a read-only register with 'ia' access, where a _wraccess is undeclared.

The current implementation of verilog axi interface does not function correctly for a FWFT fifo.

If csr_data_fifo_rvalid_ff is held high by a fifo (such as with FWFT), there is a cycle mismatch which results in rdata register read a cycle early.

Snippet of regmap json file:

       {
            "name": "DATA",
            "description": "Data register",
            "bitfields": [
                {
                    "name": "FIFO",
                    "description": "Write to push value to TX FIFO, read to get data from RX FIFO",
                    "reset": 0,
                    "width": 32,
                    "lsb": 0,
                    "access": "rw",
                    "hardware": "q",
                    "enums": []
                }
            ]
        },

Generated code

reg [63:0] rdata_ff;
always @(posedge clk) begin
    if (rst) begin
        rdata_ff <= 64'h0;
    end else if (ren) begin
        case (raddr)
            40'h0: rdata_ff <= csr_id_rdata;
            40'h8: rdata_ff <= csr_data_rdata;
            40'h10: rdata_ff <= csr_stat_rdata;
            40'h18: rdata_ff <= csr_ctrl_rdata;
            default: rdata_ff <= 64'h0;
        endcase
    end else begin
        rdata_ff <= 64'h0;
    end
end
assign rdata = rdata_ff;

//------------------------------------------------------------------------------
// Read data valid
//------------------------------------------------------------------------------
reg rvalid_ff;
always @(posedge clk) begin
    if (rst) begin
        rvalid_ff <= 1'b0;
    end else if (ren && rvalid) begin
        rvalid_ff <= 1'b0;
    end else if (ren) begin
        rvalid_ff <= 1'b1;
    end
end

reg rvalid_drv;
always @(*) begin
    if (csr_data_ren)
        rvalid_drv = csr_data_fifo_rvalid_ff;
    else
        rvalid_drv = rvalid_ff;
end

assign rvalid = rvalid_drv;

It should be made clear if fifo with FWFT is supported or not. Perhaps a read latency field could be added to the reg map? In cases such as FWFT it would be 0. In HW it doesn't make sense to generate something to support both cases even if the resources are pretty negligible.

This would be a big change, but I think it would make more sense to have the hardware and access attributes at the reg level.

I can't think of a reasonable use case to have a single register 24 bit with 8 bits a read only status value and another set of 16 bits as a read/write queue for example and should be explicitly disallowed requiring separate registers.

{
  "name": "DATA",
  "description": "Data register",
  "bitfields": [
      {
          "name": "FIFO",
          "description": "Some fifo",
          "reset": 0,
          "width": 16,
          "lsb": 0,
          "access": "rw",
          "hardware": "q",
          "enums": []
      },
      {
          "name": "STATUS",
          "description": "A status value",
          "reset": 112233,
          "width": 8,
          "lsb": 16,
          "access": "ro",
          "hardware": "f",
          "enums": []
      }
  ]
},

Eg) from https://github.com/esynr3z/corsair/blob/master/corsair/bitfield.py#L165 to https://github.com/esynr3z/corsair/blob/master/corsair/reg.py#L111