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 Issues Need to be Fixed #1

Open
kazutoiris opened this issue Mar 27, 2024 · 1 comment
Open

Some Issues Need to be Fixed #1

kazutoiris opened this issue Mar 27, 2024 · 1 comment

Comments

@kazutoiris
Copy link

Issues to be Fixed

  1. There is a spelling error in the comment.

    uart/rtl/uart_define.sv

    Lines 44 to 45 in 0e33d44

    `define UART_DIV 4'b0001 // BASEADDR + 0x08
    `define UART_TRX 4'b0010 // BASEADDR + 0x04

  2. parity_err is always cleared.

    uart/rtl/apb4_uart.sv

    Lines 200 to 201 in 0e33d44

    .err_o (s_parity_err),
    .err_clr_i (1'b1),

  3. Besides clearing the Parity Error bit, other bits are also cleared (which should not be cleared).

    uart/rtl/apb4_uart.sv

    Lines 133 to 136 in 0e33d44

    `UART_LSR: begin
    s_clr_int = 1'b1;
    apb4.prdata[`UART_LSR_WIDTH-1:0] = s_uart_lsr_q;
    end

    uart/rtl/uart_irq.sv

    Lines 58 to 59 in 0e33d44

    if (clr_int_i) begin
    s_ip_d = 3'b000;

Recommendations for Improvement

  1. LCR is used to refer to Line Control Register, which may cause confusion, suggest extracting PEIE, TXIE and RXIE to Interrupt Enable Register (IER) or renaming the register's name.

    uart/rtl/uart_define.sv

    Lines 15 to 18 in 0e33d44

    * UART_LCR:
    * BITS: | 31:9 | 8:7 | 6 | 5 | 4:3 | 2 | 1 | 0 |
    * FIELDS: | RES | PS | PEN | STB | WLS | PEIE | TXIE | RXIE |
    * PERMS: | NONE | RW | RW | RW | RW | RW | RW | RW |

  2. Suggest renaming s_rx_push_ready to s_rx_fifo_full to avoid confusion.

    .full_o (s_rx_push_ready),

  3. pready is always true, suggest connecting !tx_fifo_full to pready for FIFO status detection.

    assign apb4.pready = 1'b1;

    .full_o (),

BTW, I suggest continuing to use the SHL-0.51 license, which is widely used by projects such as cva6 and has detailed licensing terms. The MulanPSL v2 license is still not mature enough, with vague terms and some unanswered issues (osslab-pku/OpenSourceLicense-FQA#4), so raising concerns about the prospect of maintenance being discontinued.

@maksyuki
Copy link
Member

Thanks for your issue! I will fix these typos and errors as soon as possible.

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