Replacing hand instantiated clock gate cells

Code has hand-instantiated clock gating cells which are defined in below library.

swerv_el2/rtl/lib/beh_lib.sv
module `TEC_RV_ICG

Generally such clock gates are not synthesized, rather a specific specially design cells is picked up from tech library as we don't want any random unbalanced combo logic on clock path, and timing has to be carefully done.

I do see this particular module being different from the rest in library and using a define for module name. So I am guessing that there was an intent to handle this differently and perhaps allow it to be replaced without having to edit the riscv core code.

But I am not able to see how to achieve that - could someone please clarify?

I recommend either:
a) Omit the module definition
or
b) ifdef synthesizable portion and leave intentional placeholder for per-integrator replacement.

a) Omit : force user substitution by TECH_SPECIFIC_EC_RV_ICG
751 tick_ifndef TECH_SPECIFIC_EC_RV_ICG
752 module tick_TEC_RV_ICG
...
775 endmodule
776 tick_endif

b) ifdef synthesizable portion and leave intentional placeholder for per-integrator replacement.
757 tick_ifndef TECH_SPECIFIC_EC_RV_ICG
758 logic en_ff;
759 logic enable;
...
773 assign Q = CK & en_ff;
774 tick_else //TECH_SPECIFIC_EC_RV_ICG
775 user_tech_icg user_tech_icg_inst (.Q(Q), .SE(SE), .EN(EN), .CK(CK)); //user tech cell replacement
776 tick_endif //TECH_SPECIFIC_EC_RV_ICG

Hello, please take look at the PR #54

looks good to me, assuming module user_clock_gate sits in area outside of the core (which it does seem so).

just add +define+TECH_RV_ICG= to synthesis or compilation command

Or patch this define in generated common_defines.h

@algrobman Adding the define is not enough as the module in question is defined inside beh_lib.sv:

Cores-VeeR-EL2/design/lib/beh_lib.sv

Lines 752 to 775 in ff9ad9b

    
           module `TEC_RV_ICG 
        
             ( 
        
              input logic SE, EN, CK, 
        
              output Q 
        
              ); 
        
              logic  en_ff; 
        
              logic  enable; 
        
              assign      enable = EN | SE; 
        
           `ifdef VERILATOR 
        
              always @(negedge CK) begin 
        
                 en_ff <= enable; 
        
              end 
        
           `else 
        
              always @(CK, enable) begin 
        
                 if(!CK) 
        
                   en_ff = enable; 
        
              end 
        
           `endif 
        
              assign Q = CK & en_ff; 
        
           endmodule

There will be name conflict with the custom one from the external tech library.

Since #54 is merged can this issue be closed? Or is there anything more to be done?

@rahuljainNV could you close if completed?

@nstewart-amd - are you able to use this option as part of Caliptra codebase?
just checking if this is working end to end with caliptra flow as well.

@nstewart-amd - are you able to use this option as part of Caliptra codebase? just checking if this is working end to end with caliptra flow as well.

@rahuljainNV - I think this will work. So far, it looks like we've been picking up the default inferred gaters since the code is written to default in that direction. My preference is that the integrator should have a "force function", that requires them to define the cell (else FAIL to compile). It's a bit of a silent failure at compile time here and will only get picked up by post compile design checks.

Also... please note that the same scenario exists the synchronizers. Most integrators will have a foundry/technology specific synchronizer, likely exceeding 2 flop depth.

	module `TEC_RV_ICG
	(
	input logic SE, EN, CK,
	output Q
	);

	logic en_ff;
	logic enable;

	assign enable = EN \| SE;

	`ifdef VERILATOR
	always @(negedge CK) begin
	en_ff <= enable;
	end
	`else
	always @(CK, enable) begin
	if(!CK)
	en_ff = enable;
	end
	`endif
	assign Q = CK & en_ff;

	endmodule