ulixxe / usb_cdc

Single/Multi-channel Full Speed USB interface for FPGA and ASIC designs
MIT License
158 stars 11 forks source link

Characters interchanged #1

Closed Mecrisp closed 3 years ago

Mecrisp commented 3 years ago

Thank you for the very useful implementation ! That is exactly what I need for the FOMU with UP5K FPGA.

I inserted usb_cdc into a design with a softcore:

  // ######   USB-CDC terminal   ##############################

  assign usb_dp_pu = resetq;

  wire usb_p_tx;
  wire usb_n_tx;
  wire usb_p_rx;
  wire usb_n_rx;
  wire usb_tx_en;

   SB_IO #(
       .PIN_TYPE(6'b 1010_01), // PIN_OUTPUT_TRISTATE - PIN_INPUT
       .PULLUP(1'b 0)
   ) iobuf_usbp (
       .PACKAGE_PIN(usb_dp),
       .OUTPUT_ENABLE(usb_tx_en),
       .D_OUT_0(usb_p_tx),
       .D_IN_0(usb_p_rx)
   );

   SB_IO #(
       .PIN_TYPE(6'b 1010_01), // PIN_OUTPUT_TRISTATE - PIN_INPUT
       .PULLUP(1'b 0)
   ) iobuf_usbn (
       .PACKAGE_PIN(usb_dn),
       .OUTPUT_ENABLE(usb_tx_en),
       .D_OUT_0(usb_n_tx),
       .D_IN_0(usb_n_rx)
   );

  usb_cdc #(.VENDORID(16'h0483), .PRODUCTID(16'h5740), .USE_APP_CLK(1)) _terminal
  (
    // Part running on 48 MHz:

    .clk_i(clki),
    .tx_en_o(usb_tx_en),
    .tx_dp_o(usb_p_tx),
    .tx_dn_o(usb_n_tx),
    .rx_dp_i(usb_p_rx),
    .rx_dn_i(usb_n_rx),

    // Part running on 12 MHz:

    .app_clk_i(clk),
    .rstn_i(resetq),

    .out_data_o(terminal_data),
    .out_valid_o(terminal_valid),
    .out_ready_i(terminal_valid), // terminal_rd),

    .in_data_i(transmission),
    .in_ready_o(terminal_ready),
    .in_valid_i(terminal_ready & transmission_prepared)
  );

  reg [7:0] transmission;
  reg       transmission_prepared;

  wire terminal_valid, terminal_ready;
  wire [7:0] terminal_data;
  wire terminal_wr = io_wr & io_addr[12];
  wire terminal_rd = io_rd & io_addr[12];

  always @(posedge clk) begin
    if (terminal_wr) begin
      transmission          <= io_dout[7:0];
      transmission_prepared <= 1'b1;
    end else begin
      transmission_prepared <= transmission_prepared & ~terminal_ready;
    end
  end

And I use transmission_prepared like a busy flag:

    (io_addr[12] ? { 8'd0, terminal_data}                                           : 16'd0) |
    (io_addr[13] ? {14'd0, terminal_valid, !transmission_prepared}                  : 16'd0) |

The processor checks for transmission_prepared being low, and then sends the next character. For testing, it emits an upcounter.

USB-CDC attaches correctly, lsusb is fine, but opening picocom on /dev/ttyACM0 gives something strange:


▒▒▒ &%$#"!)('&.-,+3210/7654<;:98@?>=<DCBAIHGFEMLKSRQPOWVUTSRZYX`_^]\[cba`hgonmlkjihponvuts{zyxw~}���������������������������������������������������������������������������������������������������������������������������������

▒! $#+*)('&.-,+*219876543;:98@?GFEDCBA@HGFNMLTSRQPONV^]\[ZYXW_^]\dcbjihgfemlksrqponmuts{zyx�~}|�������������������������������������������������������������������������������������������������������������������������������

▒▒"! %$,+*)('/.-,4321987654<;:BA@?>=<DCKJIHGFNMLKSRQPXWVUTS[Zba`_^]\dcbjihgfemlksrqpowvuts{z���~}|{�����������������������������������������������������������������������������������������������������������������������������

       ▒! &%$#+*)('/.-,432:987654<;:9A@?GFEDCBJIHGONMLTSRQPXWVU]\[ZYa`_^fedckjihgfnmltsrqpxwvuts{z���~}������������������������������������������������������������������������������������������������������������������������������

▒▒▒"! ('&%$#+*)10/.-,+3219876>=<;:9A@?GFEDCKJIHPONMUTSRQPXWV^]\[Zba`_^]edckjihgonmlkjrqyxwvut|{zyx��������������������������������������������������������������������������������������������������������������������������������        

        ▒▒$#"! '&%$,+*)10/.-5432:98765=<;CBA@?GFEDCKJIQPONMLKSRQYXW_^]\[ZYa`_^fedlkjihgonmltsrqyxwvu}|{z���~}�����������������������������������������������������������������������������������������������������������������������������

The characters seem to be interchanged in their order and sometimes characters are missing, for example:

DCBA IHGFE MLK SRQP OWVUT SR ZYX

I am still trying to nail down the problem. Is there a special dependency on the length or timing of "in_valid_i" and "out_ready_i" ?

Mecrisp commented 3 years ago

Same problem persists when I set both clocks to 24 MHz and use this configuration: usb_cdc #(.VENDORID(16'h0483), .PRODUCTID(16'h5740), .BIT_SAMPLES(2)) _terminal


       ▒▒ $#"*)('&%$,43210/.-54<;:987?>=<DCBAIHGFEDLKSRQPONMLT\[ZYXWV^]\dcba`hgfedlkjrqponmlt|{zyxwvu}|���������������������������������������������������������������������������������������������������������������������������������

▒! $#+*)('&%$,43210/.654<;:98@?>=<DCBJIHGFEDLTSRQPONMUT\[ZYXW_^]\dcbaihgfedlksrqponmlt|{zyxwv~}|���������������������������������������������������������������������������������������������������������������������������������
Mecrisp commented 3 years ago

Tried directly writing data by the processor bus which sets io_rd and io_wr for one clock cycle each:

  // ######   USB-CDC terminal   ##############################

  assign usb_dp_pu = resetq;

  wire usb_p_tx;
  wire usb_n_tx;
  wire usb_p_rx;
  wire usb_n_rx;
  wire usb_tx_en;

   SB_IO #(
       .PIN_TYPE(6'b 1010_01), // PIN_OUTPUT_TRISTATE - PIN_INPUT
       .PULLUP(1'b 0)
   ) iobuf_usbp (
       .PACKAGE_PIN(usb_dp),
       .OUTPUT_ENABLE(usb_tx_en),
       .D_OUT_0(usb_p_tx),
       .D_IN_0(usb_p_rx)
   );

   SB_IO #(
       .PIN_TYPE(6'b 1010_01), // PIN_OUTPUT_TRISTATE - PIN_INPUT
       .PULLUP(1'b 0)
   ) iobuf_usbn (
       .PACKAGE_PIN(usb_dn),
       .OUTPUT_ENABLE(usb_tx_en),
       .D_OUT_0(usb_n_tx),
       .D_IN_0(usb_n_rx)
   );

  usb_cdc #(.VENDORID(16'h0483), .PRODUCTID(16'h5740), .USE_APP_CLK(1)) _terminal
  (
    // Part running on 48 MHz:

    .clk_i(clki),
    .tx_en_o(usb_tx_en),
    .tx_dp_o(usb_p_tx),
    .tx_dn_o(usb_n_tx),
    .rx_dp_i(usb_p_rx),
    .rx_dn_i(usb_n_rx),

    // Part running on 12 MHz:

    .app_clk_i(clk),
    .rstn_i(resetq),

    .out_data_o(terminal_data),
    .out_valid_o(terminal_valid),
    .out_ready_i(terminal_valid), // terminal_rd),

    .in_data_i(io_dout[7:0]),
    .in_ready_o(terminal_ready),
    .in_valid_i(terminal_wr)
  );

  wire terminal_valid, terminal_ready;
  wire [7:0] terminal_data;
  wire terminal_wr = io_wr & io_addr[12];
  wire terminal_rd = io_rd & io_addr[12];

Results in still characters still interchanged in groups, but with a little more loss of characters:


        ▒▒$#"! ('&.-,+*)(08765432198765=<DCBA@?GFEDCKJRQPONMUTSRQYXW_^]\[ZYa`_gfedlkjihponmutsrqyxwv~}|{z���~������������������������������������������������������������������������������������������������������������������������������

        ▒▒▒&%$#"! ('&%$,+3210/.-543219A@?>=<;CBA@?GFNMLKJIHPONVUTSRZYXWV^]\dcba`_^fedlkjihponmltsrzyxwvu}|{z��������������������������������������������������������������������������������������������������������������������������������

▒▒$#"*)('&%-,+*2108765432:9876>FEDCBA@HGFEMLKJRQPONVUTS[ZYXW_^]\[cbaihgfedlkjihpowvutsrqyxwvu}|�������������������������������������������������������������������������������������������������������������������������������

▒▒▒ '&%$#"! ('&%-,43210/.65432:9A@?>=<DCBJIHGFNMLKJRQPOWVUTS[ZYX`_^]\dcbaihgfedlkjiqpowvutsrzyxw~}|{�������������������������������������������������������������������������������������������������������������������������������

▒▒"! %$#+*)('&%-,+321087654<;:9A@?>=<DCBAIHGFNMLKJRQPOWVUTS[ZYX`_^]edcba`hgfnmlkjiqponvut|{zyxwv~}|{���������������������������������������������������������������������������������
Mecrisp commented 3 years ago

I generate the clocks from an 48 MHz oscillator this way:


  reg [1:0] divider;

  always @(posedge clki) divider <= divider + 1;

  wire clk = divider[1]; // 12 MHz
ulixxe commented 3 years ago

To debug the issue, it is helpful to check if the USB interface works correctly in loopback mode. Try with these usb_cdc connections:

...
    .out_data_o(terminal_data),
    .out_valid_o(terminal_valid),
    .out_ready_i(terminal_ready),

    .in_data_i(terminal_data),
    .in_ready_o(terminal_ready),
    .in_valid_i(terminal_valid)
...

If it works then be sure that .in_valid_i is asserted high only when .in_data_i is valid. After .in_data_i has been consumed, i.e. with both .in_ready_o and .in_valid_i high, then (if no new .in_data_i is available) .in_valid_i must be asserted low. .out_data_o uses the same logic. Data is consumed when both valid and ready are high. After data has been consumed, if new data is available then valid is kept high, otherwise valid is asserted low.

Mecrisp commented 3 years ago

Thank you for the quick reply ! I inserted the loopback as suggested, with 48 MHz USB clock and 12 MHz application clock. usb_cdc #(.VENDORID(16'h0483), .PRODUCTID(16'h5740), .USE_APP_CLK(1)) _terminal I am using this input: " abcdefghijklmnopqrstuvwxyz" When slowly typed manually, I get: " d abcmefghijklvnopq" when pressing additional spaces, this follows: "rstu wxyz "

Again: " a jbcdefghisklmnopq" and with additional spaces "r tuvwxyz "

Now quickly copied by drag and drop into picocom: " bcdefghijblmnopqrstlvwxpq" and with additional spaces typed manually "rstuv xyz "

Again: " abc efghijklmeopqrstuvwopq" and "rs uvwxyz "

Now a test with loopback and both clocks set to 24 MHz:

usb_cdc #(.VENDORID(16'h0483), .PRODUCTID(16'h5740), .BIT_SAMPLES(2)) _terminal Slowly typed manually " abcghlxpq" "t yz " again " cgjo" "tuv "

Strange. I now stripped down the code to the minimum, with a split 48MHz/12MHz clock as this:


`default_nettype none

`include "usb_cdc/usb_cdc.v"
`include "usb_cdc/bulk_endp.v"
`include "usb_cdc/ctrl_endp.v"
`include "usb_cdc/phy_rx.v"
`include "usb_cdc/phy_tx.v"
`include "usb_cdc/sie.v"

module top (
    input clki, // 48MHz Clock input

    output rgb0, // LED outputs
    output rgb1,
    output rgb2,

    inout usb_dp, // USB pins
    inout usb_dn,
    output usb_dp_pu
);

  // ######   Clock   #########################################

  reg [1:0] divider;

  always @(posedge clki) divider <= divider + 1;

  wire clk = divider[1]; // 12 MHz

  // ######   Reset logic   ###################################

  wire button = 1'b1;

  reg [7:0] reset_cnt = 0;
  wire resetq = &reset_cnt;

  always @(posedge clk) begin
    if (button) reset_cnt <= reset_cnt + !resetq;
    else        reset_cnt <= 0;
  end

  // ######   USB-CDC terminal   ##############################

  assign usb_dp_pu = resetq;

  wire usb_p_tx;
  wire usb_n_tx;
  wire usb_p_rx;
  wire usb_n_rx;
  wire usb_tx_en;

   SB_IO #(
       .PIN_TYPE(6'b 1010_01), // PIN_OUTPUT_TRISTATE - PIN_INPUT
       .PULLUP(1'b 0)
   ) iobuf_usbp (
       .PACKAGE_PIN(usb_dp),
       .OUTPUT_ENABLE(usb_tx_en),
       .D_OUT_0(usb_p_tx),
       .D_IN_0(usb_p_rx)
   );

   SB_IO #(
       .PIN_TYPE(6'b 1010_01), // PIN_OUTPUT_TRISTATE - PIN_INPUT
       .PULLUP(1'b 0)
   ) iobuf_usbn (
       .PACKAGE_PIN(usb_dn),
       .OUTPUT_ENABLE(usb_tx_en),
       .D_OUT_0(usb_n_tx),
       .D_IN_0(usb_n_rx)
   );

  usb_cdc #(.VENDORID(16'h0483), .PRODUCTID(16'h5740), .USE_APP_CLK(1)) _terminal
  (
    // Part running on 48 MHz:

    .clk_i(clki),
    .tx_en_o(usb_tx_en),
    .tx_dp_o(usb_p_tx),
    .tx_dn_o(usb_n_tx),
    .rx_dp_i(usb_p_rx),
    .rx_dn_i(usb_n_rx),

    // Part running on 12 MHz:

    .app_clk_i(clk),
    .rstn_i(resetq),

    .out_data_o(terminal_data),
    .out_valid_o(terminal_valid),
    .out_ready_i(terminal_ready),

    .in_data_i(terminal_data),
    .in_ready_o(terminal_ready),
    .in_valid_i(terminal_valid)
  );

  wire terminal_valid, terminal_ready;
  wire [7:0] terminal_data;

  // ######   Blink   #########################################

  // Instantiate iCE40 LED driver hard logic.
  //
  // Note that it's possible to drive the LEDs directly,
  // however that is not current-limited and results in
  // overvolting the red LED.

  SB_RGBA_DRV #(
      .CURRENT_MODE("0b1"),       // half current
      .RGB0_CURRENT("0b000011"),  // 4 mA
      .RGB1_CURRENT("0b000011"),  // 4 mA
      .RGB2_CURRENT("0b000011")   // 4 mA
  ) RGBA_DRIVER (
      .CURREN(1'b1),
      .RGBLEDEN(1'b1),
      .RGB1PWM( terminal_valid),    // Red
      .RGB0PWM( terminal_data[0]),  // Green
      .RGB2PWM(~terminal_ready),    // Blue
      .RGB0(rgb0),
      .RGB1(rgb1),
      .RGB2(rgb2)
  );

endmodule

" h abcdefgqijklmnopzrstuvwxy " " iabcdefghrjklmnopq stuvwxyz "

ulixxe commented 3 years ago

So, it seems that USB enumeration is working correctly and that there is a problem with the FIFO interface of the bulk endpoint. Maybe can be a timing issue (some setup violation). Can you try with a lower app_clk frequency? For example you can try with 48MHz USB clk and 3MHz app freq (.BIT_SAMPLES(4), .USE_APP_CLK(1), .APP_CLK_RATIO(16)).

Another possibility is that there is a bug in synchronization logic between USB clk and app clk. In this case, it can be helpful to try loopback mode with the same clocks (.USE_APP_CLK(0)).

Mecrisp commented 3 years ago

Enumeration always works correctly.

I am using Yosys 0.9+4241 (git sha1 8733e192, clang -fPIC -Os).

Mecrisp commented 3 years ago

Update to Yosys 0.10+36 (git sha1 bf79ff59, clang 11.0.1-2 -fPIC -Os) results in same artifacts.

By the way, I would expect the echo in loopback mode to be there immediately, but instead, a given character is echoed back at least five characters later.

Example: Lots of spaces, then 16 "a" and then 16 "b" gives " a aaaaaaaaaaaabaaabbbbbbb" and with additional spaces "bbbbbbb b "

ulixxe commented 3 years ago

Maybe the Linux driver manages USB CDC class devices in a different way than Windows 10. I developed it with a Windows 10 machine, and I never tried it on a Linux one. Could you try it on a Windows 10 machine? You can use CoolTerm (https://freeware.the-meiers.org) as virtual terminal.

Mecrisp commented 3 years ago

I hope I can borrow the Windows laptop of my sister at the weekend, I am using Debian 11 here.

So far, the flow of Yosys, NextPNR and Icestorm compiles the code without errors or warnings, but there might be subtle differences in unspecified corner cases. To find these, I compiled the usb_cdc code with Verilator, which is a strict Verilog rule checker.

First it fails on the localparam placement:


%Error-UNSUPPORTED: usb_cdc/sie.v:112:24: Unsupported: Parameters in functions
  112 |       localparam [4:0] POLY5 = 5'b00101;
      |                        ^~~~~
                    usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                    usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                    usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                    usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Error-UNSUPPORTED: usb_cdc/sie.v:138:25: Unsupported: Parameters in functions
  138 |       localparam [15:0] POLY16 = 16'h8005;
      |                         ^~~~~~
                    usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                    usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                    usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                    usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Error: usb_cdc/sie.v:118:42: Can't find definition of variable: 'POLY5'
  118 |               crc5 = {crc5[3:0], 1'b0} ^ POLY5;
      |                                          ^~~~~
        usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
        usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
        usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
        usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
        usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
        j1a.v:8:1: ... note: In file included from j1a.v
%Error: usb_cdc/sie.v:144:45: Can't find definition of variable: 'POLY16'
                            : ... Suggested alternative: 'POLY5'
  144 |               crc16 = {crc16[14:0], 1'b0} ^ POLY16;
      |                                             ^~~~~~
        usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
        usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
        usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
        usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
        usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
        j1a.v:8:1: ... note: In file included from j1a.v
%Error: Exiting due to 4 error(s)

I moved the definitions out of the functions, and now a bunch of non-fatal warnings pop up:


%Warning-WIDTH: usb_cdc/sie.v:186:53: Operator ADD expects 32 bits on the LHS, but LHS's VARREF 'IN_BULK_MAXPACKETSIZE' generates 8 bits.
                                    : ... In instance j1a._terminal.u_sie
  186 |                      ceil_log2(IN_BULK_MAXPACKETSIZE+1) : ceil_log2(CTRL_MAXPACKETSIZE+1);
      |                                                     ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
                ... Use "/* verilator lint_off WIDTH */" and lint_on around source to disable this message.
%Warning-WIDTH: usb_cdc/sie.v:186:87: Operator ADD expects 32 bits on the LHS, but LHS's VARREF 'CTRL_MAXPACKETSIZE' generates 8 bits.
                                    : ... In instance j1a._terminal.u_sie
  186 |                      ceil_log2(IN_BULK_MAXPACKETSIZE+1) : ceil_log2(CTRL_MAXPACKETSIZE+1);
      |                                                                                       ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-LITENDIAN: usb_cdc/ctrl_endp.v:86:15: Little bit endian vector: MSB < LSB of bit range: 0:143
                                             : ... In instance j1a._terminal.u_ctrl_endp
   86 |    localparam [0:8*'h12-1] dev_descr = {8'h12,
      |               ^
                    usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Warning-LITENDIAN: usb_cdc/ctrl_endp.v:106:15: Little bit endian vector: MSB < LSB of bit range: 0:535
                                              : ... In instance j1a._terminal.u_ctrl_endp
  106 |    localparam [0:8*'h43-1] conf_descr = {8'h09,
      |               ^
                    usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/phy_tx.v:73:27: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'clk_cnt_q' generates 2 bits.
                                      : ... In instance j1a._terminal.u_sie.u_phy_tx
   73 |             if (clk_cnt_q == BIT_SAMPLES-1)
      |                           ^~
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/phy_tx.v:81:33: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'clk_cnt_q' generates 2 bits.
                                      : ... In instance j1a._terminal.u_sie.u_phy_tx
   81 |    assign clk_gate = (clk_cnt_q == BIT_SAMPLES-1) ? 1'b1 : 1'b0;
      |                                 ^~
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/phy_rx.v:89:27: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'clk_cnt_q' generates 2 bits.
                                      : ... In instance j1a._terminal.u_sie.u_phy_rx
   89 |             if (clk_cnt_q == BIT_SAMPLES-1)
      |                           ^~
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/phy_rx.v:120:33: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'clk_cnt_q' generates 2 bits.
                                       : ... In instance j1a._terminal.u_sie.u_phy_rx
  120 |    assign clk_gate = (clk_cnt_q == (VALID_SAMPLES-1)) ? 1'b1 : 1'b0;
      |                                 ^~
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/sie.v:132:17: Bit extraction of var[4:0] requires 3 bit index, not 4 bits.
                                    : ... In instance j1a._terminal.u_sie
  132 |             rev5[i] = data[4-i];
      |                 ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/sie.v:144:22: Bit extraction of var[7:0] requires 3 bit index, not 4 bits.
                                    : ... In instance j1a._terminal.u_sie
  144 |             if ((data[i] ^ crc16[15]) == 1'b1)
      |                      ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/sie.v:157:17: Bit extraction of var[7:0] requires 3 bit index, not 4 bits.
                                    : ... In instance j1a._terminal.u_sie
  157 |             rev8[i] = data[7-i];
      |                 ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-LITENDIAN: usb_cdc/bulk_endp.v:87:8: Little bit endian vector: MSB < LSB of bit range: 0:71
                                            : ... In instance j1a._terminal.u_bulk_endp
   87 |    reg [0:8*OUT_LENGTH-1] out_fifo_q, out_fifo_d;
      |        ^
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:140:26: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'out_last_qq' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  140 |          if (out_last_qq == OUT_LENGTH-1)
      |                          ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-LITENDIAN: usb_cdc/bulk_endp.v:152:8: Little bit endian vector: MSB < LSB of bit range: 0:71
                                             : ... In instance j1a._terminal.u_bulk_endp
  152 |    reg [0:8*IN_LENGTH-1] in_fifo_q;
      |        ^
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:205:35: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'in_first_qq' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  205 |                   if (in_first_qq == IN_LENGTH-1)
      |                                   ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:225:32: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'in_last_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  225 |    assign in_full = (in_last_q == ((in_first_q == 'd0) ? IN_LENGTH-1: in_first_q-1) ? 1'b1 : 1'b0);
      |                                ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:225:81: Operator SUB expects 32 or 8 bits on the LHS, but LHS's VARREF 'in_first_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  225 |    assign in_full = (in_last_q == ((in_first_q == 'd0) ? IN_LENGTH-1: in_first_q-1) ? 1'b1 : 1'b0);
      |                                                                                 ^
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:229:74: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'delay_out_cnt_q' generates 2 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  229 |          assign app_out_valid_o = ((out_empty == 1'b0 && delay_out_cnt_q == BIT_SAMPLES-1) ? 1'b1 : 1'b0);
      |                                                                          ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:240:46: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'out_last_qq' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  240 |                   out_full_q <= (out_last_qq == ((out_first_q == 'd0) ? OUT_LENGTH-1: out_first_q-1) ? 1'b1 : 1'b0);
      |                                              ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:240:98: Operator SUB expects 32 or 8 bits on the LHS, but LHS's VARREF 'out_first_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  240 |                   out_full_q <= (out_last_qq == ((out_first_q == 'd0) ? OUT_LENGTH-1: out_first_q-1) ? 1'b1 : 1'b0);
      |                                                                                                  ^
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:244:41: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'out_first_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  244 |                         if (out_first_q == OUT_LENGTH-1)
      |                                         ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:237:36: Operator NEQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'delay_out_cnt_q' generates 2 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  237 |                if (delay_out_cnt_q != BIT_SAMPLES-1) begin
      |                                    ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:254:70: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'delay_in_cnt_q' generates 2 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  254 |          assign app_in_ready_o = ((in_full == 1'b0 && delay_in_cnt_q == BIT_SAMPLES-1) ? 1'b1 : 1'b0);
      |                                                                      ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:269:39: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'in_last_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  269 |                         if (in_last_q == IN_LENGTH-1)
      |                                       ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:262:35: Operator NEQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'delay_in_cnt_q' generates 2 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  262 |                if (delay_in_cnt_q != BIT_SAMPLES-1) begin
      |                                   ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Error: Exiting due to 25 warning(s)

Could you please have a glance over this list of warnings to see if anything might seriously affect the bulk endpoint operation ?

Mecrisp commented 3 years ago

PS: Due to the localparam change, line numbers in sie.v are now off by one.

Mecrisp commented 3 years ago

Again with line numbers matching your original source code:


%Warning-WIDTH: usb_cdc/sie.v:185:53: Operator ADD expects 32 bits on the LHS, but LHS's VARREF 'IN_BULK_MAXPACKETSIZE' generates 8 bits.
                                    : ... In instance j1a._terminal.u_sie
  185 |                      ceil_log2(IN_BULK_MAXPACKETSIZE+1) : ceil_log2(CTRL_MAXPACKETSIZE+1);
      |                                                     ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
                ... Use "/* verilator lint_off WIDTH */" and lint_on around source to disable this message.
%Warning-WIDTH: usb_cdc/sie.v:185:87: Operator ADD expects 32 bits on the LHS, but LHS's VARREF 'CTRL_MAXPACKETSIZE' generates 8 bits.
                                    : ... In instance j1a._terminal.u_sie
  185 |                      ceil_log2(IN_BULK_MAXPACKETSIZE+1) : ceil_log2(CTRL_MAXPACKETSIZE+1);
      |                                                                                       ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-LITENDIAN: usb_cdc/ctrl_endp.v:86:15: Little bit endian vector: MSB < LSB of bit range: 0:143
                                             : ... In instance j1a._terminal.u_ctrl_endp
   86 |    localparam [0:8*'h12-1] dev_descr = {8'h12,  
      |               ^
                    usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Warning-LITENDIAN: usb_cdc/ctrl_endp.v:106:15: Little bit endian vector: MSB < LSB of bit range: 0:535
                                              : ... In instance j1a._terminal.u_ctrl_endp
  106 |    localparam [0:8*'h43-1] conf_descr = {8'h09,  
      |               ^
                    usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/phy_tx.v:73:27: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'clk_cnt_q' generates 2 bits.
                                      : ... In instance j1a._terminal.u_sie.u_phy_tx
   73 |             if (clk_cnt_q == BIT_SAMPLES-1)
      |                           ^~
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/phy_tx.v:81:33: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'clk_cnt_q' generates 2 bits.
                                      : ... In instance j1a._terminal.u_sie.u_phy_tx
   81 |    assign clk_gate = (clk_cnt_q == BIT_SAMPLES-1) ? 1'b1 : 1'b0;
      |                                 ^~
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/phy_rx.v:89:27: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'clk_cnt_q' generates 2 bits.
                                      : ... In instance j1a._terminal.u_sie.u_phy_rx
   89 |             if (clk_cnt_q == BIT_SAMPLES-1)
      |                           ^~
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/phy_rx.v:120:33: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'clk_cnt_q' generates 2 bits.
                                       : ... In instance j1a._terminal.u_sie.u_phy_rx
  120 |    assign clk_gate = (clk_cnt_q == (VALID_SAMPLES-1)) ? 1'b1 : 1'b0;
      |                                 ^~
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/sie.v:131:17: Bit extraction of var[4:0] requires 3 bit index, not 4 bits.
                                    : ... In instance j1a._terminal.u_sie
  131 |             rev5[i] = data[4-i];
      |                 ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/sie.v:143:22: Bit extraction of var[7:0] requires 3 bit index, not 4 bits.
                                    : ... In instance j1a._terminal.u_sie
  143 |             if ((data[i] ^ crc16[15]) == 1'b1)
      |                      ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/sie.v:156:17: Bit extraction of var[7:0] requires 3 bit index, not 4 bits.
                                    : ... In instance j1a._terminal.u_sie
  156 |             rev8[i] = data[7-i];
      |                 ^
                usb_cdc/phy_tx.v:182:1: ... note: In file included from phy_tx.v
                usb_cdc/phy_rx.v:252:1: ... note: In file included from phy_rx.v
                usb_cdc/ctrl_endp.v:750:1: ... note: In file included from ctrl_endp.v
                usb_cdc/bulk_endp.v:387:1: ... note: In file included from bulk_endp.v
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-LITENDIAN: usb_cdc/bulk_endp.v:87:8: Little bit endian vector: MSB < LSB of bit range: 0:71
                                            : ... In instance j1a._terminal.u_bulk_endp
   87 |    reg [0:8*OUT_LENGTH-1] out_fifo_q, out_fifo_d;
      |        ^
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:140:26: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'out_last_qq' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  140 |          if (out_last_qq == OUT_LENGTH-1)
      |                          ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-LITENDIAN: usb_cdc/bulk_endp.v:152:8: Little bit endian vector: MSB < LSB of bit range: 0:71
                                             : ... In instance j1a._terminal.u_bulk_endp
  152 |    reg [0:8*IN_LENGTH-1] in_fifo_q;
      |        ^
                    usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                    j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:205:35: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'in_first_qq' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  205 |                   if (in_first_qq == IN_LENGTH-1)
      |                                   ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:225:32: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'in_last_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  225 |    assign in_full = (in_last_q == ((in_first_q == 'd0) ? IN_LENGTH-1: in_first_q-1) ? 1'b1 : 1'b0);
      |                                ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:225:81: Operator SUB expects 32 or 8 bits on the LHS, but LHS's VARREF 'in_first_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  225 |    assign in_full = (in_last_q == ((in_first_q == 'd0) ? IN_LENGTH-1: in_first_q-1) ? 1'b1 : 1'b0);
      |                                                                                 ^
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:229:74: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'delay_out_cnt_q' generates 2 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  229 |          assign app_out_valid_o = ((out_empty == 1'b0 && delay_out_cnt_q == BIT_SAMPLES-1) ? 1'b1 : 1'b0);
      |                                                                          ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:240:46: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'out_last_qq' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  240 |                   out_full_q <= (out_last_qq == ((out_first_q == 'd0) ? OUT_LENGTH-1: out_first_q-1) ? 1'b1 : 1'b0);
      |                                              ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:240:98: Operator SUB expects 32 or 8 bits on the LHS, but LHS's VARREF 'out_first_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  240 |                   out_full_q <= (out_last_qq == ((out_first_q == 'd0) ? OUT_LENGTH-1: out_first_q-1) ? 1'b1 : 1'b0);
      |                                                                                                  ^
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:244:41: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'out_first_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  244 |                         if (out_first_q == OUT_LENGTH-1)
      |                                         ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:237:36: Operator NEQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'delay_out_cnt_q' generates 2 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  237 |                if (delay_out_cnt_q != BIT_SAMPLES-1) begin
      |                                    ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:254:70: Operator EQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'delay_in_cnt_q' generates 2 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  254 |          assign app_in_ready_o = ((in_full == 1'b0 && delay_in_cnt_q == BIT_SAMPLES-1) ? 1'b1 : 1'b0);
      |                                                                      ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:269:39: Operator EQ expects 32 or 8 bits on the LHS, but LHS's VARREF 'in_last_q' generates 4 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  269 |                         if (in_last_q == IN_LENGTH-1)
      |                                       ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Warning-WIDTH: usb_cdc/bulk_endp.v:262:35: Operator NEQ expects 32 or 3 bits on the LHS, but LHS's VARREF 'delay_in_cnt_q' generates 2 bits.
                                          : ... In instance j1a._terminal.u_bulk_endp
  262 |                if (delay_in_cnt_q != BIT_SAMPLES-1) begin
      |                                   ^~
                usb_cdc/usb_cdc.v:170:1: ... note: In file included from usb_cdc.v
                j1a.v:8:1: ... note: In file included from j1a.v
%Error: Exiting due to 25 warning(s)
ulixxe commented 3 years ago

Yesterday I tried my TinyFPGA (programmed with USB_CDC and a stack machine that I'm developing) with CoolTerm on Linux (CentOS 8) and on Mac (High Sierra) without any issue. This weekend I hope to try the USB_CDC alone in loopback configuration and with your clock setting. Maybe the root cause of the issue is on Yosys verilog compilation. I'm using Lattice iCEcube2 with Synopsys Synplify Pro as compiler. I will check the warnings that you report.

Mecrisp commented 3 years ago

Could you find ambiguities in the list of warnings ?

By the way, if you are currently working on a Forth stack machine in FPGA, have a look at my project Mecrisp-Ice, which is a descendant of Swapforth and the J1a processor by James Bowman, enhanced by interrupts: http://mecrisp.sourceforge.net/

There is Mecrisp-Quintus also, a Forth which runs on RISC-V. Bruno Levy and me even managed to squeeze a fairly traditional one into the Icestick: https://github.com/BrunoLevy/learn-fpga/blob/master/FemtoRV/RTL/PROCESSOR/femtorv32_quark.v

ulixxe commented 3 years ago

I was able to reproduce the issue by compiling the loopback design with yosys. Instead, with the same design and iCecube2, the issue doesn't happen. I will try to fix verilator warnings to check if this solves the issue with yosys.

Mecrisp commented 3 years ago

I am glad you could reproduce it!

ulixxe commented 3 years ago

I fixed almost all verilator warnings. My loopback implementation on TinyFPGA now works. I think it is due to a Yosys bug. Indeed the changes in bulk_endp.v that trigger the issue are:

- reg [0:8*OUT_LENGTH-1] out_fifo_q, out_fifo_d;
+ reg [8*OUT_LENGTH-1:0] out_fifo_q, out_fifo_d;

- reg [0:8*IN_LENGTH-1] in_fifo_q;
+ reg [8*IN_LENGTH-1:0] in_fifo_q;

The functionality of the code doesn't change with them. So the only explanation is a Yosys bug. Could you try if the new code version solves the issue on your Fomu loopback implementation?

Mecrisp commented 3 years ago

I can confirm that the terminal works nicely now compiled by Yosys.

Mixing little endian and big endian vectors may result in ambigous conditions, so I assume both Icecube2 and Yosys behave correctly, but with different behaviour in unspecified corner cases, hence the warning message on checking in Verilator.

For example, https://hdlbits.01xz.net/wiki/Vector1 gives:

The endianness (or, informally, "direction") of a vector is whether the the least significant bit has a lower index (little-endian, e.g., [3:0]) or a higher index (big-endian, e.g., [0:3]). In Verilog, once a vector is declared with a particular endianness, it must always be used the same way. e.g., writing vec[0:3] when vec is declared wire [3:0] vec; is illegal. Being consistent with endianness is good practice, as weird bugs occur if vectors of different endianness are assigned or used together.

Many thanks for solving the issue ! Let me know if I can return the favour in the area of Forth.

ulixxe commented 3 years ago

Thanks for helping me to improve the code. So, I will close this issue.

I know the J1 processor. It has its stacks implemented with two banks of registers. I want to implement a small stack machine with its stacks on memory. This will allow a fast task switch in a multitasking environment. I'm working on it during my spare time. It is pretty complete. I'm working now to improve the instructions for the stack switch. See the attached pdf with some more info about it. uf16.pdf

Thank you for your availability to help me on Forth! I will be back.

ulixxe commented 3 weeks ago

Hi Matthias, if you are interested, I put my stack machine on Github: https://github.com/ulixxe/uf16 I will be glad if you have time to give a look. See you

Mecrisp commented 3 weeks ago

Thank you for pointing me to your design! I was just thinking of you, too, as I updated to your latest USB-CDC code both in Mecrisp-Ice and Mecrisp-Quintus this week, and it works like a charm.

I especially like your exception handling baked into uf16.v, and diving a little bit deeper, getting the stack spill and reload to and from memory working in all corner cases is impressive. It find it interesting to see your solution to the fundamental design choice in the instruction set to balance the available bits in the instruction space between address width in calls, literal bits directly available, and ALU/microcode opcodes. For your short literal opcode, I wonder how the choice between unsigned 12 bit literal vs. with sign extension pays off in the total program size.

ulixxe commented 3 weeks ago

I greatly appreciate your feedback—thank you! Regarding the short literal, I believe that having access to the literal -1 and the negative range of [-2048, -1] is worth sacrificing the positive range of [2048, 4095].