chipsalliance / caliptra-rtl

HW Design Collateral for Caliptra RoT IP
Apache License 2.0
76 stars 38 forks source link

EL2 Memory interface module ports at Caliptra top #179

Closed nstewart-amd closed 1 year ago

nstewart-amd commented 1 year ago

EL2 Memory interface Top-Level el2_mem_export interface is not defined with modport direction. No concern for simulation, but synthesis flows require direction declaration. Recommendation is to drive the modports all the way from el2_mem -> caliptra_top. Modified files attached.

el2_mem_if.tar.gz

Syed-mudabbir-ahsan commented 1 year ago

Hi stewart,

Along with the el2_mem_export interface update, we can see there are other updates in the modified caliptra_top.sv which you have shared, There is one extra ahb_lite_2to1_mux instance for SB and LSU AHB master mux and there are some extra port connections in some instances(ports are not defined inside the module of those instances).

Can you please let us know the reason of updates in the caliptra_top and change history or document capturing these changes or updates. We only expected multi-driven fixes but seeing these many changes.

Please help us to understand these changes soon.

Differences those are realised while comparing with previous caliptra_top database version::::

File: _diff_rot.txt

Difference in caliptra_top ports: There are extra ports while comparing to earlier version and those are related to QSPI and UART (Internal). UART has parameterization but not QSPI. Please let us know if they are for internal UART/QSPI so we can act accoridnly. Earlier seems to be tied off inside.

Extra Module/logic in caliptra_top.sv

  1. Extra Declarations (causing issue in compile ) logic fw_update_rst_window;

    // Caliptra ECC status signals rv_ecc_sts_t rv_ecc_sts;

Compile Log: xrun_comp.log Message Snapshot: rv_ecc_sts_t rv_ecc_sts; | xmvlog: *E,ILLPDL (/projects/cpd/devar/rtl_users/smahsan/rot_ss_caliptra/subip/caliptra-rtl-0.8/src/integration/rtl/caliptra_top.sv,196|26): Mixing of ansi & non-ansi style port declaration is not legal.

  1. Extra Instances: sb_ahb(); CALIPTRA_AHB_LITE_BUS_INF #( .AHB_LITE_ADDR_WIDTH(CALIPTRA_AHB_HADDR_SIZE), .AHB_LITE_DATA_WIDTH(CALIPTRA_AHB_HDATA_SIZE) ) lsu_ahb();

    CALIPTRA_AHB_LITE_BUS_INF #( .AHB_LITE_ADDR_WIDTH(CALIPTRA_AHB_HADDR_SIZE), .AHB_LITE_DATA_WIDTH(CALIPTRA_AHB_HDATA_SIZE) )

  2. Extra port force_bus_idle at ahb_lite_bus

  3. Extra ahb_lite_2to1_mux module addition with lsu_ahb and sb_ahb interfacing

  4. Extra ports at clk_gate

  5. noncore reset to uc reset change for other module of ahb_lite_2to1_mux; debugUnlock_or_scan_mode_switch requirement?

  6. There are changes in the key vault, pcr_vault --> What is the purpose of change?

We have upcoming significant release on 31th Aug, please help with these points. Waiting for your response.

Thanks, Syed Mudabbir Ahsan

bharatpillilli commented 1 year ago

@calebofearth , do we need to fix this in gen 1? If not, I would like to move it to future/gen 2

nstewart-amd commented 1 year ago

Bharat, This is must fix in our opinion. Current code results in multi-driven nets due to lack of direction definition on ports. Formal equivalency will fail without this change or similar correct definition of port direction.

Norman

calebofearth commented 1 year ago

@nstewart-amd, it would be preferable (for integrators who are already beginning to consume Caliptra) to keep the caliptra_top portlist consistent. Instead of propagating the ICCM/DCCM modports to the top-level independently, I'd suggest creating a new modport "veer_sram_src" that consolidates the ICCM/DCCM interfaces. (I'll also propose renaming the 'top' modport to 'veer_sram_sink' for clarity)

modport veer_sram_src (
    input clk,
    // ICCM
    output iccm_clken, iccm_wren_bank, iccm_addr_bank, iccm_bank_wr_data,
    input  iccm_bank_dout,
    // DCCM
    output dccm_clken, dccm_wren_bank, dccm_addr_bank, dccm_wr_data_bank,
    input  dccm_bank_dout
);

modport veer_sram_sink (
    input clk,
    // ICCM
    input  iccm_clken, iccm_wren_bank, iccm_addr_bank, iccm_bank_wr_data,
    output iccm_bank_dout,
    // DCCM
    input  dccm_clken, dccm_wren_bank, dccm_addr_bank, dccm_wr_data_bank,
    output dccm_bank_dout
);

Then we can change caliptra_top port from

    // Caliptra Memory Export Interface
    el2_mem_if                         el2_mem_export,

to

    // Caliptra Memory Export Interface
    el2_mem_if.veer_sram_src            el2_mem_export,

Would this resolve the aforementioned synthesis issue? Would you foresee any synth/tool issues with consolidating modports on the way up the hierarchy?

nstewart-amd commented 1 year ago

Caleb, I think this should be fine.
We found it a little cheaper/easier to port the iccm/dccm modports for proof of concept, but will adapt as needed if a consolidated interface/modport set is defined.

calebofearth commented 1 year ago

Thanks Norman, I've implemented it as above and it will be sync'ed over shortly. I did have to manually swizzle from one modport into another inside the VeeR core, but nothing major. Is that the expense you are referring to?

nstewart-amd commented 1 year ago

Hi Caleb, Yes. That sounds about right. It was easier just to port up to the top as is and avoid the swizzle, but that's less pretty than the single defined interface.

calebofearth commented 1 year ago

Thanks for clarifying!