Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some Improvement Suggestions #1

Open
kazutoiris opened this issue Apr 3, 2024 · 1 comment
Open

Some Improvement Suggestions #1

kazutoiris opened this issue Apr 3, 2024 · 1 comment

Comments

@kazutoiris
Copy link

Recommendations for Improvement

  1. 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).

    assign s_arch_sys_en = s_apb4_wr_hdshk && s_apb4_addr == `ARCHINFO_SYS;
    assign s_arch_sys_d = s_arch_sys_en ? apb4.pwdata[`ARCHINFO_SYS_WIDTH-1:0] : s_arch_sys_q;
    dfferc #(`ARCHINFO_SYS_WIDTH, `SYS_VAL) u_arch_sys_dfferc (
    apb4.pclk,
    apb4.presetn,
    s_arch_sys_en,
    s_arch_sys_d,
    s_arch_sys_q
    );

  2. 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.

    always_comb begin
    apb4.prdata = '0;
    if (s_apb4_rd_hdshk) begin
    unique case (s_apb4_addr)
    `ARCHINFO_SYS: apb4.prdata[`ARCHINFO_SYS_WIDTH-1:0] = s_arch_sys_q;
    `ARCHINFO_IDL: apb4.prdata[`ARCHINFO_IDL_WIDTH-1:0] = s_arch_idl_q;
    `ARCHINFO_IDH: apb4.prdata[`ARCHINFO_IDH_WIDTH-1:0] = s_arch_idh_q;
    default: apb4.prdata = '0;
    endcase
    end
    end

    Below is an example from a Xilinx project.

    https://github.com/Xilinx/systemctlm-cosim-demo/blob/3b1501c21487d265c3275a45d468a65b5d4f1989/apb_timer.v#L61-L92

@maksyuki
Copy link
Member

maksyuki commented Apr 3, 2024

Thanks for your issue! I will fix these clock gate enable signals issues in all IP repos in the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants