[hw,tlul_adapter_reg,rtl] Do not gate a_ready with a_valid

[Razer6]

This creates a combinational path between the input and output TLUL port. This can be pretty long and impacts the synthesis in high-frequency designs.

Originally, the busy_i signal was gated with a_vali for cases where reg_tops have different clock domains. There, the busy input is a function from the request... At the moment, this causes assertions to fail, e.g., the AReadyKnown assertion in the PWM. This is an IP that uses 2 clock domains.

[rswarbrick]

This might be obvious to others but it took me a while... The way that the version on master uses a_valid dates back to commit a49ceb6 in 2021.

[rswarbrick]

I'm trying to make sure I understand the reason for this proposed change. Is the point that tl_o.a_ready signal was depending on stuff from tl_i which wasn't necessarily on the clock we expect? I'm a bit confused: I would have expected both of those signals to be synchronous to clk_i.

What have I missed?

[vogelpi]

This doesn't work for register interfaces with built in CDC. Because for those, busy_i depends on whether the currently selected address has an ongoing transaction over the CDC. For example, in aon_timer_reg_top.sv:

  // register busy
  logic reg_busy_sel;
  assign reg_busy = reg_busy_sel | shadow_busy;
  always_comb begin
    reg_busy_sel = '0;
    unique case (1'b1)
      addr_hit[1]: begin
        reg_busy_sel = wkup_ctrl_busy;
      end
      addr_hit[2]: begin
        reg_busy_sel = wkup_thold_hi_busy;
      end

Register interfaces without built-in CDC only use shadow_busy which is statically zero when the module comes out of reset.

I agree that the change makes sense in general. But we should think how to handle the case with CDCs. Maybe we could simply factor tl_i.a_valid into busy_i in the reg top if and only if the register interface has a CDC? Then we would at least break this path for most IP blocks? WDYT?

[Razer6]

That sounds perfectly reasonable to me. When CDC is involved, it is typically done to the AON domain, which is way slower. There, these paths are not that critical I guess. But on the fast main clock it makes sense to cut the path between the input and output port. I added the change to the reg_top template and re-generated all IPs based on that.

[Razer6]

I'm trying to make sure I understand the reason for this proposed change. Is the point that tl_o.a_ready signal was depending on stuff from tl_i which wasn't necessarily on the clock we expect? I'm a bit confused: I would have expected both of those signals to be synchronous to clk_i.

What have I missed?

In general, a_valid in that equation adds a combinational path between tl_i and tl_o, which can be a pretty long one. This may lead to problems when trying to synthesize for frequencies > 1 GHz ;-). It seems that a_valid is not needed in the general case but only when dealing with CDC in the reg_top, where busy is a function of the TLUL request. Instead of generally adding a_valid to the equation now, we now only add that where it's really needed.

[vogelpi]

Thanks for making the change @Razer6 !

Lint currently fails in CI. The issue is that now English Breakfast is a real top meaning we check in it's autogenerated files, too. You should be able to run make -k -C hw top_and_cmdgen to fix this.

[vogelpi]

LGTM, thanks @Razer6 !

[vogelpi]

CHANGE AUTHORIZED: hw/ip/adc_ctrl/rtl/adc_ctrl_reg_top.sv
CHANGE AUTHORIZED: hw/ip/aon_timer/rtl/aon_timer_reg_top.sv
CHANGE AUTHORIZED: hw/ip/pwm/rtl/pwm_reg_top.sv
CHANGE AUTHORIZED: hw/ip/sysrst_ctrl/rtl/sysrst_ctrl_reg_top.sv
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_reg.sv
CHANGE AUTHORIZED: hw/ip/usbdev/rtl/usbdev_reg_top.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/clkmgr/rtl/clkmgr_reg_top.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pinmux/rtl/pinmux_reg_top.sv

The change introduced by this PR doesn't have a functional impact but it alleviates TLUL timing pressure for all IPs not involving CDCs in the register file.

[rswarbrick]

CHANGE AUTHORIZED: hw/ip/adc_ctrl/rtl/adc_ctrl_reg_top.sv
CHANGE AUTHORIZED: hw/ip/aon_timer/rtl/aon_timer_reg_top.sv
CHANGE AUTHORIZED: hw/ip/pwm/rtl/pwm_reg_top.sv
CHANGE AUTHORIZED: hw/ip/sysrst_ctrl/rtl/sysrst_ctrl_reg_top.sv
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_reg.sv
CHANGE AUTHORIZED: hw/ip/usbdev/rtl/usbdev_reg_top.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/clkmgr/rtl/clkmgr_reg_top.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pinmux/rtl/pinmux_reg_top.sv

This has been carefully reasoned. Thanks, @Razer6, for explaining what's going on.