opentitan icon indicating copy to clipboard operation
opentitan copied to clipboard

[ascon] Make alerts visible in the status register

Open m-temp opened this issue 1 year ago • 4 comments

Description

From https://github.com/lowRISC/opentitan/issues/22452

We have defined the following fields in the Ascon Status Register: https://github.com/lowRISC/opentitan/blob/0ff694a0812fe2f67da5716385bb3bc7443a9d48/hw/ip/ascon/data/ascon.hjson#L615-L651

However the information in which register the update error happened is not exposed by the register top. There is only a generic ORed hadowed_update_err_o. https://github.com/lowRISC/opentitan/blob/0ff694a0812fe2f67da5716385bb3bc7443a9d48/hw/ip/ascon/rtl/ascon_reg_top.sv#L2643-L2656

How should we proceed? Can we change the register interface and remove the itemized/dedicated error fields and only have one generic?

m-temp avatar Aug 30 '24 14:08 m-temp

How about using the same approach as AES? AFAIK, AES also has multiple shadowed CSRs and maps update errors to one recoverable alert and one status bit (take a look at aes.hjson and aes.sv). If that approach worked for AES, it will probably also work for Ascon?

andreaskurth avatar Sep 02 '24 14:09 andreaskurth

I agree, however, this implies to change the (approved) specification and I think it would be good to have this discussed and documented in this issue. WDYT @vogelpi @andrea-caforio

FYI: Internally, AES has two shadowed registers, with two different alerts (one from the reggen/aes_reg_top.sv, and one from the aes_ctrl_reg_shadowed.sv implementation), however, they are -as you said- ored/mapped to the same alert/error bit.

m-temp avatar Sep 02 '24 15:09 m-temp

I think combining the update and storage errors of all shadowed registers into a single recoverable alert and a fatal alert is very reasonable. Aligning the specification with this approach should not be controversial. I am supportive of this. Thanks @m-temp for starting the discussion.

vogelpi avatar Sep 16 '24 08:09 vogelpi

Also agreed on this approach from my side, not controversial at all.

cdgori avatar Sep 20 '24 19:09 cdgori