esynr3z / corsair

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

Access mode 'rolh' can miss a latch #28

Closed nmmalipes closed 1 year ago

nmmalipes commented 1 year ago

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.

arnfol commented 1 year ago

Thanks for the feedback!

I would disagree about the priority of events. If you are using this field as an interrupt flag and it is already set to 1, you expect it to be cleared after reading out, no matter if the event occurred while reading (because the event already occurred previously).

So, the case you are pointing to is probably when the field is not set yet, and software, for example, is polling this field. Then we can actually miss its assertion. So the code should be like this:

if (csr_test_rolh_ren = '1' and csr_test_rolh_rolh_ff = '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;
nmmalipes commented 1 year ago

Yes! that would fix my problem. Thanks.