Open kazutoiris opened 7 months ago
Using the ternary operator for s_arch_sys_d is unnecessary; it can be directly linked with apb4.pwdata, considering that enable port in the u_arch_sys_dfferc module is already connected (there're some similar issues).
s_arch_sys_d
apb4.pwdata
enable
u_arch_sys_dfferc
https://github.com/oscc-ip/archinfo/blob/8ffae9bcc837e2db1810751154ad2c707e6dd292/rtl/apb4_archinfo.sv#L33-L41
It is recommended to use clock-edge style to avoid critical path issue. Additionally, it's advised not to always pull the pready signal high.
pready
https://github.com/oscc-ip/archinfo/blob/8ffae9bcc837e2db1810751154ad2c707e6dd292/rtl/apb4_archinfo.sv#L63-L73
Below is an example from a Xilinx project.
https://github.com/Xilinx/systemctlm-cosim-demo/blob/3b1501c21487d265c3275a45d468a65b5d4f1989/apb_timer.v#L61-L92
Thanks for your issue! I will fix these clock gate enable signals issues in all IP repos in the next few days.
Recommendations for Improvement
Using the ternary operator for
s_arch_sys_d
is unnecessary; it can be directly linked withapb4.pwdata
, considering thatenable
port in theu_arch_sys_dfferc
module is already connected (there're some similar issues).https://github.com/oscc-ip/archinfo/blob/8ffae9bcc837e2db1810751154ad2c707e6dd292/rtl/apb4_archinfo.sv#L33-L41
It is recommended to use clock-edge style to avoid critical path issue. Additionally, it's advised not to always pull the
pready
signal high.https://github.com/oscc-ip/archinfo/blob/8ffae9bcc837e2db1810751154ad2c707e6dd292/rtl/apb4_archinfo.sv#L63-L73
Below is an example from a Xilinx project.
https://github.com/Xilinx/systemctlm-cosim-demo/blob/3b1501c21487d265c3275a45d468a65b5d4f1989/apb_timer.v#L61-L92