Nit: I think this is a character too far in?

Fixed.

Could you restructure this always_comb block a bit? I've spent some time reading the code and I don't yet understand that it's doing the right thing. In particular, the current version looks like we've got a "ready" signal going into u_csrng_ctr_drbg_gen that depends on the value of its cmd_rsp_data_o port. I don't think that quite looks right to me?

My personal preference would be to write this as a ternary. I would first generate the selector signal with appropriate comments e.g.:

// Keep forwarding gen block responses to the state db unless the cmd block has a valid response.
assign gen_rsp_to_db = (ctr_drbg_cmd_rsp_data.cmd != GEN) && (ctr_drbg_cmd_rsp_vld == 1'b1) ? 1'b0 : 1'b1; 

And then the ternary:

assign state_db_wr_vld  = gen_rsp_to_db ? ctr_drbg_gen_rsp_vld  : ctr_drbg_cmd_rsp_vld;
assign state_db_wr_data = gen_rsp_to_db ? ctr_drbg_gen_rsp_data : ctr_drbg_cmd_rsp_data;
...

I think this would be more readable as there is a single line defining the controls.

Thanks to both of you for the feedback. I agree that the way I had it was hard to follow. The fundamental issue here is that we do a sort-of three-way data stream routing, where the rsp from _cmd can go one of two endpoints, with the state db endpoint having influence on whether the _gen_rsp channel can write to state db or not.

I think @vogelpi suggestion allows for much better readable code. I have implemented it with some slight adaptions. Please have a look again, and let me know if this part is now (somewhat) clear.

Thanks for changing the implementation and the clear comments. That's very helpful.

If it's easy, I'd consider splitting this commit in two, where the first part removes the FIFO (and wires zero to this error signal) and the second part removes the register field. I think the result would probably be a lot easier to follow.

Unfortunately, splitting this up would be quite a bit of work, as this affects in total six commits in this and the follow-up PR (which would then become twelve or 13 commits, given your suggestion of splitting one of them into three). Apart from this, I would like to keep the regfile changes together with the rtl and dv changes in a single commit for each FIFO, respectively, for the following reasons:

• Doing otherwise would break consistency with the already merged [csrng/rtl] Remove four prim_fifo_sync from the data path #28428
• Splitting the changes up would make the commits not self-contained anymore, which they currently are

Of course, please let me know if you feel strongly that the split up would be necessary.

A bit like with the previous commit, can this one be split as well? Here, it would be in three parts. I think the SFIFO_RCSTATE_ERR field could be removed as a later commit (and driven with zero beforehand). Similarly, I think DRBG_CMD_SM_ERR be added as a first commit (driven with zero). That way, all the "register shuffling cruft" gets separated from the real work.

I realise that I'm repeating myself (sorry!), but I think it would make sense to split this commit into two pieces: the first one would remove the fifo and wire zero into the error field; the second would remove the field.

If this really cannot happen (I believe you that it can't happen) we should add an SVA to assert this.

Good idea. Added.

Thanks!

My personal preference would be to write this as a ternary. I would first generate the selector signal with appropriate comments e.g.:

// Keep forwarding gen block responses to the state db unless the cmd block has a valid response.
assign gen_rsp_to_db = (ctr_drbg_cmd_rsp_data.cmd != GEN) && (ctr_drbg_cmd_rsp_vld == 1'b1) ? 1'b0 : 1'b1; 

And then the ternary:

assign state_db_wr_vld  = gen_rsp_to_db ? ctr_drbg_gen_rsp_vld  : ctr_drbg_cmd_rsp_vld;
assign state_db_wr_data = gen_rsp_to_db ? ctr_drbg_gen_rsp_data : ctr_drbg_cmd_rsp_data;
...

I think this would be more readable as there is a single line defining the controls.

Can we please add a TODO into your issue to check this behavior (skip update if pdata = 0) is what we really want? Because the NIST spec is a bit ambiguous on this and I have seen software implementations which treat the case of pdata = 0 and pdata_len = 0 differently.

We don't need to change this right now, but I think we should have a discussion on how we want to move forward.

Good idea - added the TODO and a matching item in #28153

Thanks Florian.

I think some more comments would help here.

Agree, added.

Perfect, thanks!

Is it actually possible for the cmd_complete_i pulse coming in while being in the MainSmClrAData state?

With all these FIFOs removed, yes. The latency between the command req_rdy and rsp_vld can now be very small, depending on the command at hand, and hence cmd_rdy_i and cmd_complete_i can be asserted in the same cycle. I actually had some tests being stuck due to a missed cmd_complete_i in the main FSM as the reason for adding this condition.

Okay, thanks for explaining. The code looks a bit complex and I wanted to make sure this complexity is really needed. All good!

Fixed.

Good idea. Added.

Thanks to both of you for the feedback. I agree that the way I had it was hard to follow. The fundamental issue here is that we do a sort-of three-way data stream routing, where the rsp from _cmd can go one of two endpoints, with the state db endpoint having influence on whether the _gen_rsp channel can write to state db or not.

I think @vogelpi suggestion allows for much better readable code. I have implemented it with some slight adaptions. Please have a look again, and let me know if this part is now (somewhat) clear.

Unfortunately, splitting this up would be quite a bit of work, as this affects in total six commits in this and the follow-up PR (which would then become twelve or 13 commits, given your suggestion of splitting one of them into three). Apart from this, I would like to keep the regfile changes together with the rtl and dv changes in a single commit for each FIFO, respectively, for the following reasons:

• Doing otherwise would break consistency with the already merged [csrng/rtl] Remove four prim_fifo_sync from the data path #28428
• Splitting the changes up would make the commits not self-contained anymore, which they currently are

Of course, please let me know if you feel strongly that the split up would be necessary.

Agree, added.

Good idea - added the TODO and a matching item in #28153

With all these FIFOs removed, yes. The latency between the command req_rdy and rsp_vld can now be very small, depending on the command at hand, and hence cmd_rdy_i and cmd_complete_i can be asserted in the same cycle. I actually had some tests being stuck due to a missed cmd_complete_i in the main FSM as the reason for adding this condition.