usb: timing fixes in the status and IRQ paths

Add a synchronizer for the configured path, and a register for the IRQ
output; otherwise we get timing violations and could end up in a weird
state.

fpga/abcbus.sv

@@ -178,13 +178,13 @@ module abcbus (
        begin
 	  abc_clk_q <= { abc_clk_q[0], abc_clk_s };
 	  case ( { abc_clk_q == 2'b10, stb_1mhz } )
-	    5'b10: begin
+	    2'b10: begin
 	       if (abc_clk_ctr == 3'b111)
 		 abc_clk_active <= 1'b1;
 	       else
 		 abc_clk_ctr <= abc_clk_ctr + 1'b1;
 	    end
-	    5'b01: begin
+	    2'b01: begin
 	       if (abc_clk_ctr == 3'b000)
 		 abc_clk_active <= 1'b0;
 	       else
@@ -501,11 +501,11 @@ module abcbus (
 
    // Level triggered IRQ
    always @(posedge sys_clk)
-     irq <= is_busy | (bus_change_status & bus_change_mask);
+     irq <= is_busy | |(bus_change_status & bus_change_mask);
 
    // Read MUX
    always_comb
-     casez (cpu_addr[5:2])
+     casez (cpu_addr[6:2])
        5'b00000: cpu_rdata = { 28'b0, abc_status[0] };
        5'b00001: cpu_rdata = { 23'b0, ~iosel_en, ioselx[7:0] };
        5'b00010: cpu_rdata = { bus_change_mask, 2'b0, busy_mask,

fpga/max80.qpf

@@ -19,14 +19,14 @@
 #
 # Quartus Prime
 # Version 21.1.0 Build 842 10/21/2021 SJ Lite Edition
-# Date created = 22:18:18  January 09, 2022
+# Date created = 02:33:31  January 10, 2022
 #
 # -------------------------------------------------------------------------- #
 
 QUARTUS_VERSION = "21.1"
-DATE = "22:18:18  January 09, 2022"
+DATE = "02:33:31  January 10, 2022"
 
 # Revisions
 
-PROJECT_REVISION = "v1"
 PROJECT_REVISION = "v2"
+PROJECT_REVISION = "v1"

fpga/usb/usb_serial/src_v/usb_cdc_core.sv

@@ -77,8 +77,8 @@ module usb_cdc_channel
    // Core FIFO interface
    input	 clk_i,
    input	 rst_i,
-		 usb_endpoint.dstr data_ep,
-		 usb_endpoint.dstr intr_ep,
+   usb_endpoint.dstr data_ep,
+   usb_endpoint.dstr intr_ep,
 
    // Control signals
    input	 recv_break_i,
@@ -209,6 +209,8 @@ module usb_cdc_channel
    wire        recv_break_s;
    reg	       recv_break_q;
 
+   assign irq = irq_q;
+
    synchronizer #(.width(1)) break_synchro
      (
       .rst_n ( rst_n ),
@@ -220,8 +222,8 @@ module usb_cdc_channel
    always @(negedge rst_n or posedge sys_clk)
      if (~rst_n)
        begin
-	   water_ctl    <= 16'hc3c3 & water_ctl_mask;
-irq_mask     <= 16'b0;
+	  water_ctl   <= 16'hc3c3 & water_ctl_mask;
+	  irq_mask     <= 16'b0;
 	  irq_pol      <= 16'b0;
 	  recv_break_q <= 1'b0;
        end
@@ -275,7 +277,14 @@ irq_mask     <= 16'b0;
    assign status[9]    = recv_break_q;
    assign status[10]   = usb_configured;
 
-   assign irq = |((status ^ irq_pol) & irq_mask);
+   reg	       irq_q;
+   always @(negedge rst_n or posedge sys_clk)
+     if (~rst_n)
+       irq_q <= 1'b0;
+     else
+       irq_q <= |((status ^ irq_pol) & irq_mask & status_mask);
+
+   assign irq = irq_q;
 
    always @(*)
      case (cpu_addr)
@@ -1187,6 +1196,8 @@ module usb_cdc_core
 
    //-----------------------------------------------------------------
    // Channel streaming interfaces and CPU interfaces
+   // Some signals need to be synchronized to the CPU clock, and
+   // there is no reason to do it in each channel.
    //-----------------------------------------------------------------
 
    wire		       start_of_frame_ws;
@@ -1205,7 +1216,16 @@ module usb_cdc_core
 
    wire start_of_frame_s = start_of_frame_ws & ~start_of_frame_q;
 
-   tri0 [31:0]         rdata_chan[0:7];
+   wire configured_s;
+   synchronizer #(.width(1))
+   configured_sync (
+		    .rst_n ( 1'b1 ),
+		    .clk   ( sys_clk ),
+		    .d     ( configured_q ),
+		    .q     ( configured_s )
+		    );
+
+   wire [31:0] rdata_chan[0:7];
 
    generate
       genvar  cn;
@@ -1220,7 +1240,7 @@ module usb_cdc_core
 		 .cpu_addr         ( cpu_addr[3:2] ),
 		 .cpu_rdata        ( rdata_chan[cn] ),
 		 .cpu_wdata        ( cpu_wdata ),
-		 .cpu_wstrb        ( cpu_wstrb[0] ),
+		 .cpu_wstrb        ( cpu_wstrb ),
 		 .irq              ( irq[cn] ),
 
 		 .clk_i            ( clk_i ),
@@ -1228,13 +1248,17 @@ module usb_cdc_core
 		 .data_ep          ( usb_ep[cn*2+1].dstr ),
 		 .intr_ep          ( usb_ep[cn*2+2].dstr ),
 
-		 .usb_configured   ( configured_q ),
+		 .usb_configured   ( configured_s ),
 
 		 .recv_break_i     ( rx_break_r[cn] ),
 		 .recv_break_o     ( recv_break_o[cn] ),
 		 .start_of_frame_s ( start_of_frame_s )
 		 );
 	end // block: chan
+      for (cn = channels; cn < 8; cn++)
+	begin : nochan
+	   assign rdata_chan[cn] = 32'bx;
+	end
    endgenerate
 
    assign cpu_rdata_cdc = rdata_chan[cpu_addr[6:4]];

riscv-opts.mk

@@ -4,7 +4,8 @@ riscv_target_flags =	-fvisibility=hidden \
 			-frename-registers -fshort-enums -fshort-wchar \
 			-ffunction-sections -fdata-sections \
 			-mshorten-memrefs -mstrict-align -fno-exceptions \
-			-malign-data=natural
+			-malign-data=natural \
+			-falign-functions=4
 
 # Additional flags during application build
 riscv_flags = $(riscv_target_flags) \

rv32/abcdisk.c

@@ -277,7 +277,6 @@ static void disk_reset_state(struct ctl_state *state)
 static struct drive_state *
 name_to_drive(const char *drive)
 {
-    struct ctl_state *state;
     unsigned int ndrive;
 
     /* All drive names are three letters long */
@@ -294,7 +293,7 @@ name_to_drive(const char *drive)
     }
 
     for (int i = 0; i < CONTROLLER_TYPES; i++) {
-	struct ctl_state *state = &controllers[i];
+	struct ctl_state * const state = &controllers[i];
 
 	if (!memcmp(state->params->name, drive, 2))
 	    return &state->drv[ndrive];
@@ -392,35 +391,25 @@ static void unmount_drives(struct ctl_state *state)
 /* RST# = global reset, C3# = local reset, C1# = goto command start */
 #define IDLE_CALLBACK_MASK	((1 << 7)|(1 << 4)|(1 << 2))
 
-static ABC_CALLBACK(abcdisk_callback_out)
+static ABC_CALLBACK(abcdisk_callback_out_inp)
 {
-    struct ctl_state *state = container_of(dev, struct ctl_state, iodev);
+    struct ctl_state * const state = container_of(dev, struct ctl_state, iodev);
 
     dev->callback_mask = IDLE_CALLBACK_MASK;
-    ABC_INP1_DATA = dev->inp_data[1] = 0;
-    state->error = 0;
-    state->pending |= PEND_IO;
-}
-
-static ABC_CALLBACK(abcdisk_callback_inp)
-{
-    struct ctl_state *state = container_of(dev, struct ctl_state, iodev);
-
-    dev->callback_mask = IDLE_CALLBACK_MASK;
-    ABC_INP1_DATA = dev->inp_data[1] = 0;
+    __abc_set_inp_status(dev, 0);
     state->error = 0;
     state->pending |= PEND_IO;
 }
 
 static ABC_CALLBACK(abcdisk_callback_restart)
 {
-    struct ctl_state *state = container_of(dev, struct ctl_state, iodev);
+    struct ctl_state * const state = container_of(dev, struct ctl_state, iodev);
     state->pending |= PEND_STARTCMD;
 }
 
 static ABC_CALLBACK(abcdisk_callback_rst)
 {
-    struct ctl_state *state = container_of(dev, struct ctl_state, iodev);
+    struct ctl_state * const state = container_of(dev, struct ctl_state, iodev);
     state->pending |= PEND_RESET;
 }
 
@@ -465,7 +454,7 @@ static void init_drives(struct ctl_state *state)
 
 static void do_next_command(struct ctl_state *state)
 {
-    struct drive_state *drv = cur_drv_mutable(state);
+    struct drive_state * const drv = cur_drv_mutable(state);
     uint8_t *buf = cur_buf(state);
 
 #if 0
@@ -734,7 +723,7 @@ void __hot abcdisk_io_poll(void)
     }
 
     for (int i = 0; i < CONTROLLER_TYPES; i++) {
-	struct ctl_state *state = &controllers[i];
+	struct ctl_state * const state = &controllers[i];
 	enum pending pending = 0;
 
 	if (unmount_all && state->initialized) {
@@ -780,15 +769,15 @@ void abcdisk_init(void)
 	.inp_en = 3,
 	.status_first_out_mask = (uint8_t)~0x80,
 	.status_first_inp_mask = (uint8_t)~0x80,
-	.callback_out[0] = abcdisk_callback_out,
-	.callback_inp[0] = abcdisk_callback_inp,
+	.callback_out[0] = abcdisk_callback_out_inp,
+	.callback_inp[0] = abcdisk_callback_out_inp,
 	.callback_out[2] = abcdisk_callback_restart,
 	.callback_out[4] = abcdisk_callback_rst, /* C3# = local reset */
 	.callback_rst    = abcdisk_callback_rst
     };
 
     for (int i = 0; i < CONTROLLER_TYPES; i++) {
-	struct ctl_state *state = &controllers[i];
+	struct ctl_state * const state = &controllers[i];
 
 	state->params = &parameters[i];
 	state->iodev  = iodev_template;

rv32/abcio.c

@@ -9,8 +9,8 @@
 #include "irq.h"
 #include "abcio.h"
 
+__sbss struct abc_dev *_abc_selected_dev;
 static __bss_hot struct abc_dev *abc_device[65]; /* 65 == post-RST# = always NULL */
-static __sbss struct abc_dev *selected_dev;
 static __sdata uint8_t abc_devsel = 64;
 
 #define EVENT_MASK_ALWAYS 0x0082 /* RST# and CS# */
@@ -29,7 +29,7 @@ uint16_t event_mask(const struct abc_dev *dev)
 static inline __attribute__((always_inline))
 void refresh_dev(struct abc_dev *dev)
 {
-    if (dev == selected_dev) {
+    if (dev == abc_selected_dev()) {
 	ABC_BUSY_MASK = event_mask(dev);
 	ABC_INP = dev ? dev->inp_data_w : 0;
     }
@@ -38,15 +38,14 @@ void refresh_dev(struct abc_dev *dev)
 static inline __attribute__((always_inline))
 void abc_select(struct abc_dev *dev)
 {
-    selected_dev = dev;
+    _abc_selected_dev = dev;
     refresh_dev(dev);
 }
 
 IRQHANDLER(abc,0)
 {
     unsigned int what = ABC_BUSY_STATUS;
-    struct abc_dev *dev = selected_dev;
-    unsigned int callback = dev ? (what & dev->callback_mask) : 0;
+    struct abc_dev *dev = abc_selected_dev();
 
     if (what & 0xff) {
 	unsigned int addr = ABC_OUT_ADDR;
@@ -69,23 +68,20 @@ IRQHANDLER(abc,0)
 	case 1:
 	    dev = abc_device[abc_devsel = data & 0x3f];
 	    abc_select(dev);
+	    /* fall through */
 
+	case 2 ... 5:
 	    if (!dev)
 		break;
-
-	    callback = dev->callback_mask & what;
-	    goto handle_out;
-
-	case 2 ... 5:
 	    goto handle_out;
 
 	handle_out:
 	    dev->out_data[addr] = data;
-	    if (callback & (1 << addr))
+	    if (dev->callback_mask & (1 << addr))
 		dev->callback_out[addr](dev, data, addr);
 	    break;
 
-	case 7:
+	case 7:			/* Bus reset */
 	{
 	    struct abc_dev **devp;
 	    uint8_t old_devsel = abc_devsel;
@@ -93,6 +89,7 @@ IRQHANDLER(abc,0)
 	    abc_devsel = DEVSEL_NONE;
 	    abc_select(NULL);
 
+	    /* Broadcast to all interested devices */
 	    devp = &abc_device[0];
 	    while (devp <= &abc_device[63]) {
 		dev = *devp++;
@@ -108,25 +105,27 @@ IRQHANDLER(abc,0)
 	}
     }
 
-    if (what & 0x100) {
-	if (dev->inp_cnt && --dev->inp_cnt) {
-	    dev->inp_data[1] &= dev->status_first_inp_mask;
-	    ABC_INP1_DATA = dev->inp_data[1];
-	    ABC_INP0_DATA = dev->inp_data[0] = *dev->inp_buf++;
-	    /* No callback */
-	} else {
-	    uint8_t old_data = ABC_INP0_DATA;
-	    ABC_INP0_DATA = dev->inp_data[0] = dev->inp_data_def;
-	    if (callback & 0x100)
-		dev->callback_inp[0](dev, old_data, 0);
+    if (dev) {
+	if (what & 0x100) {
+	    if (dev->inp_cnt && --dev->inp_cnt) {
+		dev->inp_data[1] &= dev->status_first_inp_mask;
+		ABC_INP1_DATA = dev->inp_data[1];
+		ABC_INP0_DATA = dev->inp_data[0] = *dev->inp_buf++;
+		/* No callback */
+	    } else {
+		uint8_t old_data = ABC_INP0_DATA;
+		ABC_INP0_DATA = dev->inp_data[0] = dev->inp_data_def;
+		if (dev->callback_mask & 0x100)
+		    dev->callback_inp[0](dev, old_data, 0);
+	    }
 	}
-    }
 
-    if (what & 0x200) {
-	uint8_t old_data = ABC_INP1_DATA;
-	ABC_INP1_DATA = dev->inp_data[1];
-	if (callback & 0x200)
-	    dev->callback_inp[1](dev, old_data, 1);
+	if (what & 0x200) {
+	    uint8_t old_data = ABC_INP1_DATA;
+	    ABC_INP1_DATA = dev->inp_data[1];
+	    if (dev->callback_mask & 0x200)
+		dev->callback_inp[1](dev, old_data, 1);
+	}
     }
 
     /* May need to change event_mask here */
@@ -167,7 +166,7 @@ void __hot abc_set_inp_default(struct abc_dev *dev, uint8_t val)
     dev->inp_data_def = val;
     if (!dev->inp_cnt) {
 	dev->inp_data[0] = val;
-	if (dev == selected_dev)
+	if (dev == abc_selected_dev())
 	    ABC_INP0_DATA = val;
     }
 
@@ -179,7 +178,7 @@ void __hot abc_set_inp_status(struct abc_dev *dev, uint8_t val)
     irqmask_t irqmask = mask_irq(ABC_IRQ);
 
     dev->inp_data[1] = val;
-    if (dev == selected_dev)
+    if (dev == abc_selected_dev())
 	ABC_INP1_DATA = val;
 
     restore_irq(irqmask, ABC_IRQ);

rv32/abcio.h

@@ -47,6 +47,12 @@ struct abc_dev {
     abc_callback_t callback_rst;
 };
 
+extern struct abc_dev *_abc_selected_dev;
+static inline struct abc_dev *abc_selected_dev(void)
+{
+    return _abc_selected_dev;
+}
+
 void abc_setup_out_queue(struct abc_dev *dev, void *buf, size_t len,
 			 uint8_t status);
 void abc_setup_inp_queue(struct abc_dev *dev, const void *buf, size_t len,
@@ -54,6 +60,36 @@ void abc_setup_inp_queue(struct abc_dev *dev, const void *buf, size_t len,
 void abc_set_inp_default(struct abc_dev *dev, uint8_t val);
 void abc_set_inp_status(struct abc_dev *dev, uint8_t val);
 
+/*
+ * The _ versions can be used from interrupt handlers or when ABC_IRQ
+ * is known to be masked already; for the _inp_data() functions this ALSO
+ * requires that dev->inp_cnt is known to be zero (no currently configured
+ * queue.)
+ *
+ * The __ versions can be used when it is *also* known that the device
+ * in question is already selected (e.g. inside a callback.)
+ */
+static inline void _abc_set_inp_data(struct abc_dev *dev, uint8_t val)
+{
+    dev->inp_data[0] = dev->inp_data_def = val;
+    if (dev == abc_selected_dev())
+	ABC_INP0_DATA = val;
+}
+static inline void _abc_set_inp_status(struct abc_dev *dev, uint8_t val)
+{
+    dev->inp_data[1] = val;
+    if (dev == abc_selected_dev())
+	ABC_INP1_DATA = val;
+}
+static inline void __abc_set_inp_data(struct abc_dev *dev, uint8_t val)
+{
+    ABC_INP0_DATA = dev->inp_data[0] = dev->inp_data_def = val;
+}
+static inline void __abc_set_inp_status(struct abc_dev *dev, uint8_t val)
+{
+    ABC_INP1_DATA = dev->inp_data[1] = val;
+}
+
 void abc_register(struct abc_dev *dev, unsigned int devsel);
 void abc_init(void);
 

rv32/abcpun80.c

@@ -27,62 +27,64 @@
 			 TTY_STATUS_USB_CONFIG)
 
 static struct abc_dev pun80_iodev;
-static __hot void pun80_refresh_input(bool advance);
-
-IRQHANDLER(tty,PUN_TTY_CHAN)
-{
-    pun80_refresh_input(false);
-}
 
 /*
  * Convert to FT232H-compatible status codes, and advance
  * the receive FIFO if applicable.
  */
-static __hot void pun80_refresh_input(bool advance)
+static __hot void
+pun80_refresh_input(struct abc_dev *dev, bool advance)
 {
     static bool data_loaded;
     unsigned int status;
-    unsigned int pun80_status;
+    uint8_t  pun80_status;
 
     status = PUN_STATUS;
 
     if (!data_loaded || advance) {
 	data_loaded = !(status & TTY_STATUS_RX_EMPTY);
 	if (data_loaded) {
-	    abc_set_inp_default(&pun80_iodev, PUN_DATA);
+	    dev->inp_data_def = dev->inp_data[0] = PUN_DATA;
 	    status = PUN_STATUS;
 	}
     }
 
-    pun80_status = ~15;
+    PUN_IRQPOL = status;
+    pun80_status = ~15 | data_loaded;
 
     if (status & TTY_STATUS_USB_CONFIG)
 	pun80_status |= 8;
     if (!(status & TTY_STATUS_TX_FULL))
 	pun80_status |= 2;
-    if (data_loaded)
-	pun80_status |= 1;
 
-    abc_set_inp_status(&pun80_iodev, pun80_status);
+    dev->inp_data[1] = pun80_status;
 
-    PUN_IRQPOL = status;
+    if (abc_selected_dev() == dev)
+	ABC_INP = dev->inp_data_w;
 }
 
-ABC_CALLBACK(pun80_callback_out)
+static ABC_CALLBACK(pun80_callback_out)
 {
     PUN_DATA = data;
-    pun80_refresh_input(false);
+    pun80_refresh_input(dev, false);
 }
 
-ABC_CALLBACK(pun80_callback_inp)
+static ABC_CALLBACK(pun80_callback_inp)
 {
-    pun80_refresh_input(true);
+    pun80_refresh_input(dev, true);
+}
+
+IRQHANDLER(tty,PUN_TTY_CHAN)
+{
+    CON_DATA = '#';
+    pun80_refresh_input(&pun80_iodev, false);
 }
 
 /* The "flush" command in FT232H (C1#) is not needed with our USB stack */
 static struct abc_dev pun80_iodev = {
     .callback_mask		= (1 << 0)|(1 << 8),
     .inp_en			= 3,
+    .inp_data[1]                = 0xf0,
     .status_first_out_mask	= ~0,
     .status_first_inp_mask	= ~0,
     .callback_out[0]		= pun80_callback_out,
@@ -92,9 +94,16 @@ static struct abc_dev pun80_iodev = {
 void pun80_init(void)
 {
     mask_irq(PUN_IRQ);
-    PUN_IRQEN = PUN_IRQ_MASK;
-    pun80_refresh_input(false);
+    /*
+     * Immediately interrupt on:
+     *
+     * USB configured
+     * TX full
+     * RX not empty
+     */
+    PUN_IRQPOL = TTY_STATUS_RX_EMPTY;
+    PUN_IRQEN  = PUN_IRQ_MASK;
+    unmask_irq(PUN_IRQ);
 
     abc_register(&pun80_iodev, PUN_IOSEL);
-    unmask_irq(PUN_IRQ);
 }