esynr3z / corsair

Control and Status Register map generator for HDL projects
https://corsair.readthedocs.io
MIT License
95 stars 33 forks source link

Minor bug in VHDL file generation (csr_*_ren signal) #4

Closed arnfol closed 1 year ago

arnfol commented 2 years ago

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:

csrconfig ```ini [globcfg] data_width = 32 address_width = 12 register_reset = sync_neg [vhdl_entity] generator = Vhdl interface = axil path = bug_test.vhd ```
regs.yml ```yaml 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?

arnfol commented 2 years ago

I have fixed my problem with changing the fields type from "wo" to "rw".

esynr3z commented 2 years ago

Thanks for your feedback! I'm super happy that this tool is useful for you.

Actually, it is an intended behaviour. It is assumed that for "wo" mode read is a "don' care" operation.

You are right in the end, you have to use "rw" to get both "ren" and "wen" signals.

arnfol commented 2 years ago

Sounds reasonable. I added a small disclaimer, so this behaviour would be reflected by documentation.

arnfol commented 2 years ago

Okay, so we have not understood each other correctly. There is a compilation error in generated VHDL file when using wo and a in one bitfield. The expected behavior is:

rw + a gives you ren & wen output ports wo + a gives you wen output port ro + a gives you ren output port

But currently when using wo + a gives me both ren & wen output ports, but some of the internal signals for ren port are not generated, which causes a compilation error. I have not checked other combinations, but assume that there also could be a problem.

ValeriiPlumerai commented 2 years ago

same bug for verilog file generation

v0lker commented 1 year ago

i think this might be fixed with PR https://github.com/esynr3z/corsair/pull/27

arnfol commented 1 year ago

@v0lker thanks a lot for your work!