lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.57k stars 765 forks source link

[top_earlgrey, rtl] Instantiate EDN1 with NumEndPoints set to 1 #15393

Closed sriyerg closed 8 months ago

sriyerg commented 2 years ago

The top_earlgrey.sv (autogenerated) instantiates 2 EDN instances. On the second instance (u_edn1), only 1 endpoint is connected (OTBN), the rest are unconnected and tied off.

This results in additional overhead in terms of coverage exclusions for the tied off end points, since they are truly unreachable, and more importantly, redundant logic in the design.

Filing an issue to see if it is feasible instead to instantiate it with NumEndPoints set to 1 given the tooling complexities.

estimate 4

sriyerg commented 2 years ago

@tjaychen @msfschaffner @eunchan @martin-lueker FYI.

msfschaffner commented 2 years ago

Moving this to M3, since we have a workaround for this in place (unused ports are excluded from coverage).

GregAC commented 1 year ago

Triaged for otbn I think this should be done for M2.5 assuming it doesn't generated significant extra design effort

andreaskurth commented 1 year ago

Triaged for edn. Suggest https://github.com/lowRISC/opentitan/labels/Priority%3AP3 for M2.5 because the design behaves within spec and DV verifies it, so I would do this "if we have time left".

GregAC commented 1 year ago

estimate range: 2 - 4

johngt commented 1 year ago

Changing priority based on @andreaskurth comment

msfschaffner commented 10 months ago

Probably this does not save a lot of area since the synthesis tool should prune any tied off EDN output ports away. However, since it is not very clean to have so many dangling ports on EDN1, we could do a best effort attempt on parameterizing this down to just one output port.

I.e., invest 1h to see whether this is feasible at all by creating a second DV target. If this works with no major issues, the NumEndPoints should be declared as an exposed parameter in the edn.hjson and overridden accordingly in the top_earlgrey.hjson.

vogelpi commented 8 months ago

I came across some Yosys synthesis results from last year and also had a look at the FPGA numbers (both before @h-filali 's recent change removing the redundant output FIFO).

In my view, the different synthesis tools are pretty efficient at carving out the 7 tied off endpoints in EDN1. I don't expect further gains from instantiating a version with NumEndPoints set to 1 that would justify the effort at this point.

In my view we should close this issue.

msfschaffner commented 8 months ago

Thanks for the numbers, @vogelpi! I agree, let's close it.