[sdc] update sdc after PD review

[meisnere]

Please review the two files, confirm them and use it for future sdc edits.

chip_earlgrey_asic.ot.sdc replaces the previous chip_earlgrey_asic.sdc

[andreaskurth]

Thanks @meisnere! Is there a reason why this SDC file isn't named chip_earlgrey_asic.sdc? That would provide a diff compared to what is currently in the repo

[vogelpi]

Thanks @meisnere ! This is a lot of constraints to review, I did a first pass and left some feedback.

[vogelpi]

?

[vogelpi]

This TODO has already been in the previous SDC file used for ES. @meisnere , @OTshimeon please comment on whether this is actually needed for the production tapeout.

[vogelpi]

This TODO has already been in the previous SDC file used for ES. @meisnere , @OTshimeon please comment on whether this is actually needed for the production tapeout.

[vogelpi]

Thanks @meisnere and @OTshimeon for updating the PR. I did another pass and will now set up a shared document to continue the review of some items internally. To proceed with this PR, I need input from you on the following things:

• Please comment on the remaining open questions.
• Please run util/fix_trailing_whitespace.py hw/top_earlgrey/syn/chip_earlgrey_asic.sdc to remove trailing whitespaces. The PR currently fails lint and we can't merge it like this. For details, see the "checks" section below.
• The constraints we've checked and approved last TUE in a meeting (they were in a different file previously) seem not to be part of this PR. Please explain why and what the planned course of action is here. My understanding was that these constraints need to be moved to this SDC file as well. FYI @moidx .

[meisnere]

Add the last few missing constraints - line 1664 and below.

[GregAC]

I've reviewed the JTAG clock (lines 199 - 268) related constraints. I have no concerns though a couple of questions.

We have these set_clock_sense commands to tell the tool about inverted clocks in the LC and RV_DM JTAG TAPs:

opentitan/hw/top_earlgrey/syn/chip_earlgrey_asic.sdc

Lines 228 to 229 in 5e0f7c8

	set_clock_sense -negative ${LC_JTAG_TCK_INV_PIN}
	set_clock_sense -negative ${RV_JTAG_TCK_INV_PIN}

What we don't have is an explicit create_generated_clock for these. These clocks are derived from the incoming JTAG clock using a prim_clock_inv:

opentitan/hw/vendor/pulp_riscv_dbg/src/dmi_jtag_tap.sv

Lines 190 to 197 in 3e047f4

	prim_clock_inv #(
	.HasScanMode(1'b1),
	.NoFpgaBufG(1'b1)
	) i_tck_inv (
	.clk_i ( tck_i ),
	.clk_no ( tck_n ),
	.scanmode_i ( testmode_i )
	);

and clearly will be slightly skewed relative to the non inverted clock. I presume this is something the tool can just handle in its analysis and doesn't need explicit constraints to do properly (or maybe we have suitable constraints that generically applied to all prim_clock_inv instances).

I also don't see any constraints for trst_n which is connected to the IO4 pad. Should we have some? I don't see any obvious constraints around other resets so I presume this is fine but wanted to check.

[a-will]

I also don't see any constraints for trst_n which is connected to the IO4 pad. Should we have some? I don't see any obvious constraints around other resets so I presume this is fine but wanted to check.

Nope. I believe there is no need for a timing requirement for nTRST. It's an async reset, and the user is required to assert it only when TCK is inactive. Because the acceptable reset and guard times are on the order of hundreds of milliseconds, we give the tool freedom to place and route.

[hcallahan-lowrisc]

I reviewed the spi constraints section (L289 to 1569) and have no major concerns.
Most of my comments were just nits/suggestions, which can mostly be ignored to keep the diff small if needed.

The only non-nit was about different output_delay constraints for some of the if {$spec_constr} blocks, which probably just needs a comment to articulate why.

[hcallahan-lowrisc]

Nit. A documentation comment at the top of the file explaining the use of this switch (why/when the two sets of constraints are used) would be helpful.

Nit. Also, in most locations, the if/else is used to switch between the same command just with a different set of input variables. I think it may be easier to follow, and use fewer LoC, if a single set of variables were conditionally defined under one if/else block, and then the commands don't have to be duplicated each time.

[hcallahan-lowrisc]

Nit. this section is quite magical without additional context. Hopefully some of the rationale / calcs can move into additional visible collateral in the future.

[hcallahan-lowrisc]

Nit. This comment appears stale, given the addition of IOA2 below?

[vogelpi]

I did have another look at this but could not find a reason why IOA2 should get these constraints. IOA2 doesn't seem to be related to SPI TPM at all. It's used a GPIO and in some cases we use it for SPI Host 1. But SPI Host 1 is not used for passthrough / TPM mode. This is only done for SPI device and SPI Host 0. So it's not clear to me why this is needed actually.

[vogelpi]

The PD team got back to me about this: Some time in the past, IOA2 was determined to be used as chip select for quad SPI. During the tapeout, it was agreed that IOA7 should be the primary chip select target whereas IOA2 should remain as a secondary opportunistic target. I'll add a comment into the SDC to document this.

[hcallahan-lowrisc]

The else case (and the one above on L909) do not have output_delay constraints. All other $spec_constr if/else blocks duplicate the commands within each case. Is that intentional?

[vogelpi]

Yes, it actually is intentional. The PD provided feedback on this. The spec_constr == 1 case in these two particular cases verifies the fast passthrough mode where the direct timing paths from an upstream host to a downstream Flash device is checked. The SPI Device is then defined as input and the SPI Host as output.

In contrast, the spec_constr == 0 case verifies the slow passthrough mode where the direct timing paths from a downstream Flash to an upstream host are checked. Then, the SPI device is defined as output and the SPI Host is defined as input. In this case, SPI Device gets no input delay constraints and SPI Host not output delay constraints.

I'll document this in the SDC.

[vogelpi]

Ah, and the PD team also confirmed that both constraint sets got verified for the tapeout.

[alees24]

I've been through the USB section of the SDC file (lines [53:121]) and checked all of the numbers and signals and I'm happy that the constraints are good. The critical signals have appropriate time constraints, with margins.
The cio_usb_sense_i signal changes much less frequently than the other inputs and is not at all timing-critical, but over-constraining it is harmless.
The explanatory commenting in the USB section should be helpful to others, thank you.

[vogelpi]

Thanks @hcallahan-lowrisc , @a-will , @GregAC , @alees24 for your reviews!

I've included the obvious fixes and @andreaskurth appended another commit with additional constraints seperately discussed and approved by the PD team. We will merge this once CI passes.

We will discuss the remaining questions and opens with the PD team in a follow-up.

[andreaskurth]

Agree with @vogelpi. Thx all!