Some Issues Need to be Fixed

[kazutoiris]

Issues to be Fixed

1. s_valid is bound to s_apb4_wr_hdshk, s_apb4_addr, and s_done, but they are not reset/set simultaneously. Due to the delay of s_done relative to s_apb4_wr_hdshk and s_apb4_addr, there is a situation where s_done may be low when s_apb4_wr_hdshk and s_apb4_addr are high, resulting in the clk_int_even_div_simple not being set. It is suggested to directly remove the binding with s_done.

   pwm/rtl/apb4_pwm.sv

   Lines 82 to 91 in aa21b3c

   	assign s_valid = s_apb4_wr_hdshk && s_apb4_addr == `PWM_PSCR && s_done;
   	clk_int_even_div_simple #(`PWM_PSCR_WIDTH) u_clk_int_even_div_simple (
   	.clk_i (apb4.pclk),
   	.rst_n_i (apb4.presetn),
   	.div_i (s_pwm_pscr_q),
   	.div_valid_i(s_valid),
   	.div_ready_o(),
   	.div_done_o (s_done),
   	.clk_o (s_tc_clk)
   	);

2. s_pwm_cnt_q belongs to the slow clock domain s_tc_clk, while s_ov_irq_trg belongs to the fast clock domain apb4.pclk. They do not belong to the same clock domain. Therefore, after reading and clearing the interrupt register, it is possible to be set again due to the same interrupt. It is recommended to use edge detection instead of level detection.

   pwm/rtl/apb4_pwm.sv

   Lines 107 to 113 in aa21b3c

   	dffer #(`PWM_CNT_WIDTH) u_pwm_cnt_dffer (
   	s_tc_clk,
   	apb4.presetn,
   	s_pwm_cnt_en,
   	s_pwm_cnt_d,
   	s_pwm_cnt_q
   	);

   pwm/rtl/apb4_pwm.sv

   Lines 171 to 179 in aa21b3c

   	cdc_sync #(
   	.STAGE (2),
   	.DATA_WIDTH(1)
   	) u_irq_cdc_sync (
   	apb4.pclk,
   	apb4.presetn,
   	s_pwm_cnt_q >= s_pwm_cmp_q - 1,
   	s_ov_irq_trg
   	);

   pwm/rtl/apb4_pwm.sv

   Lines 186 to 188 in aa21b3c

   	end else if (~s_bit_ovif && s_bit_en && s_bit_ovie && s_ov_irq_trg) begin
   	s_pwm_stat_d = '1;
   	end

Recommendations for Improvement

1. It is suggested to use the apb4.pslverr signal to ensure conditions are met, rather than just commenting.

   pwm/rtl/apb4_pwm.sv

   Line 165 in aa21b3c

   // NOTE: need to assure the s_pwmcrrx_q less than s_pwmcmp_q

2. There are some duplicate signals, similar to Some Improvement Suggestions archinfo#1.

   pwm/rtl/apb4_pwm.sv

   Lines 115 to 123 in aa21b3c

   	assign s_pwm_cmp_en = s_apb4_wr_hdshk && s_apb4_addr == `PWM_CMP;
   	assign s_pwm_cmp_d = s_pwm_cmp_en ? apb4.pwdata[`PWM_CMP_WIDTH-1:0] : s_pwm_cmp_q;
   	dffer #(`PWM_CMP_WIDTH) u_pwm_cmp_dffer (
   	apb4.pclk,
   	apb4.presetn,
   	s_pwm_cmp_en,
   	s_pwm_cmp_d,
   	s_pwm_cmp_q
   	);