| Message ID | e273efdbf75cbc37b5c35798da7fde34877ac3b8.1658804819.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote: > @@ -440,6 +442,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb) > trb->ctrl |= 0x20; > } > > +static uint64_t xhci_trb_norm_buf(struct xhci_trb *trb) const please. > +{ > + return trb->params; > +} > + > +static uint32_t xhci_trb_norm_len(struct xhci_trb *trb) And again. > +{ > + return trb->status & 0x1FFFF; > +} > + > /** > * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event > * TRBs are read-only from software > @@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct xhci_trb *trb) > return trb->status >> 24; > } > > +/* Amount of data _not_ transferred */ > +static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb) > +{ > + return trb->status & 0x1FFFF; > +} Same as xhci_trb_norm_len()? > @@ -985,6 +1054,33 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, > } > > /** > + * Ensure DbC has a pending transfer TRB to receive data into. > + * > + * @param dbc the dbc to flush > + * @param trb the ring for the TRBs to transfer > + * @param wrk the work ring to receive data into > + */ > +static void dbc_enqueue_in(struct dbc *dbc, struct xhci_trb_ring *trb, > + struct dbc_work_ring *wrk) I can't seem to be able to spot any use of this function - it being static, how do things build for you? > +{ > + struct dbc_reg *reg = dbc->dbc_reg; > + uint32_t db = (readl(®->db) & 0xFFFF00FF) | (trb->db << 8); I think I've seen this constant in earlier patches. Can this be a #define please, such that one can easily connect all the places where the same things is meant? > + > + /* Check if there is already queued TRB */ > + if ( xhci_trb_ring_size(trb) >= 1 ) > + return; > + > + if ( dbc_work_ring_full(wrk) ) > + return; What made me spot the lack of caller are these return statements. Without letting the caller know of the failure, how would it know to make another attempt later? Jan
On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote: > On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote: > > @@ -440,6 +442,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb) > > trb->ctrl |= 0x20; > > } > > > > +static uint64_t xhci_trb_norm_buf(struct xhci_trb *trb) > > const please. > > > +{ > > + return trb->params; > > +} > > + > > +static uint32_t xhci_trb_norm_len(struct xhci_trb *trb) > > And again. > > > +{ > > + return trb->status & 0x1FFFF; > > +} > > + > > /** > > * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event > > * TRBs are read-only from software > > @@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct xhci_trb *trb) > > return trb->status >> 24; > > } > > > > +/* Amount of data _not_ transferred */ > > +static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb) > > +{ > > + return trb->status & 0x1FFFF; > > +} > > Same as xhci_trb_norm_len()? Yes, I was considering to use that, but technically those are different packets, only incidentally using the same bits. > > > @@ -985,6 +1054,33 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, > > } > > > > /** > > + * Ensure DbC has a pending transfer TRB to receive data into. > > + * > > + * @param dbc the dbc to flush > > + * @param trb the ring for the TRBs to transfer > > + * @param wrk the work ring to receive data into > > + */ > > +static void dbc_enqueue_in(struct dbc *dbc, struct xhci_trb_ring *trb, > > + struct dbc_work_ring *wrk) > > I can't seem to be able to spot any use of this function - it being > static, how do things build for you? It's in dbc_uart_poll(). > > > +{ > > + struct dbc_reg *reg = dbc->dbc_reg; > > + uint32_t db = (readl(®->db) & 0xFFFF00FF) | (trb->db << 8); > > I think I've seen this constant in earlier patches. Can this be > a #define please, such that one can easily connect all the places > where the same things is meant? Ok. > > + > > + /* Check if there is already queued TRB */ > > + if ( xhci_trb_ring_size(trb) >= 1 ) > > + return; > > + > > + if ( dbc_work_ring_full(wrk) ) > > + return; > > What made me spot the lack of caller are these return statements. > Without letting the caller know of the failure, how would it know > to make another attempt later? Next iteration of dbc_uart_poll() will take care of it.
On 05.08.2022 11:58, Marek Marczykowski-Górecki wrote: > On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote: >> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote: >>> @@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct xhci_trb *trb) >>> return trb->status >> 24; >>> } >>> >>> +/* Amount of data _not_ transferred */ >>> +static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb) >>> +{ >>> + return trb->status & 0x1FFFF; >>> +} >> >> Same as xhci_trb_norm_len()? > > Yes, I was considering to use that, but technically those are different > packets, only incidentally using the same bits. I see. That's the problem with using literal numbers rather than #define-s. But for a driver like this I didn't want to be overly picky, and hence would have wanted to let you get away without introducing many more. >>> @@ -985,6 +1054,33 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, >>> } >>> >>> /** >>> + * Ensure DbC has a pending transfer TRB to receive data into. >>> + * >>> + * @param dbc the dbc to flush >>> + * @param trb the ring for the TRBs to transfer >>> + * @param wrk the work ring to receive data into >>> + */ >>> +static void dbc_enqueue_in(struct dbc *dbc, struct xhci_trb_ring *trb, >>> + struct dbc_work_ring *wrk) >> >> I can't seem to be able to spot any use of this function - it being >> static, how do things build for you? > > It's in dbc_uart_poll(). Oh, interesting. This then means that patch 1 on its own doesn't build. Jan
On Fri, Aug 05, 2022 at 02:38:26PM +0200, Jan Beulich wrote: > On 05.08.2022 11:58, Marek Marczykowski-Górecki wrote: > > On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote: > >> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote: > >>> @@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct xhci_trb *trb) > >>> return trb->status >> 24; > >>> } > >>> > >>> +/* Amount of data _not_ transferred */ > >>> +static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb) > >>> +{ > >>> + return trb->status & 0x1FFFF; > >>> +} > >> > >> Same as xhci_trb_norm_len()? > > > > Yes, I was considering to use that, but technically those are different > > packets, only incidentally using the same bits. > > I see. That's the problem with using literal numbers rather than #define-s. > But for a driver like this I didn't want to be overly picky, and hence > would have wanted to let you get away without introducing many more. That's kind of the purpose of those helper functions - to extract specific fields according to the xhci interface, one per function. An alternative could be #define _instead of_ those functions, but I think that would be worse. What I mean, is the function name serves as description of that those constants are about. > >>> @@ -985,6 +1054,33 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, > >>> } > >>> > >>> /** > >>> + * Ensure DbC has a pending transfer TRB to receive data into. > >>> + * > >>> + * @param dbc the dbc to flush > >>> + * @param trb the ring for the TRBs to transfer > >>> + * @param wrk the work ring to receive data into > >>> + */ > >>> +static void dbc_enqueue_in(struct dbc *dbc, struct xhci_trb_ring *trb, > >>> + struct dbc_work_ring *wrk) > >> > >> I can't seem to be able to spot any use of this function - it being > >> static, how do things build for you? > > > > It's in dbc_uart_poll(). > > Oh, interesting. This then means that patch 1 on its own doesn't build. Right, I need to move the call into this patch.
On 05.08.2022 14:47, Marek Marczykowski-Górecki wrote: > On Fri, Aug 05, 2022 at 02:38:26PM +0200, Jan Beulich wrote: >> On 05.08.2022 11:58, Marek Marczykowski-Górecki wrote: >>> On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote: >>>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote: >>>>> @@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct xhci_trb *trb) >>>>> return trb->status >> 24; >>>>> } >>>>> >>>>> +/* Amount of data _not_ transferred */ >>>>> +static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb) >>>>> +{ >>>>> + return trb->status & 0x1FFFF; >>>>> +} >>>> >>>> Same as xhci_trb_norm_len()? >>> >>> Yes, I was considering to use that, but technically those are different >>> packets, only incidentally using the same bits. >> >> I see. That's the problem with using literal numbers rather than #define-s. >> But for a driver like this I didn't want to be overly picky, and hence >> would have wanted to let you get away without introducing many more. > > That's kind of the purpose of those helper functions - to extract > specific fields according to the xhci interface, one per function. An > alternative could be #define _instead of_ those functions, but I think > that would be worse. What I mean, is the function name serves as > description of that those constants are about. Right - as said, fair enough for a driver like this. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index cc1e1989b17e..07174badac8f 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -729,8 +729,8 @@ Available alternatives, with their meaning, are: Specify the USB controller to use, either by instance number (when going over the PCI busses sequentially) or by PCI device (must be on segment 0). -Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability (output -only). XHCI driver will wait indefinitely for the debug host to connect - make +Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability. +XHCI driver will wait indefinitely for the debug host to connect - make sure the cable is connected. The `share` option for xhci controls who else can use the controller: * `none`: use the controller exclusively for console, even hardware domain diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c index 805b447f2300..ccf4f9bbe2b7 100644 --- a/xen/drivers/char/xhci-dbc.c +++ b/xen/drivers/char/xhci-dbc.c @@ -109,6 +109,7 @@ enum { enum { XHCI_TRB_CC_SUCCESS = 1, XHCI_TRB_CC_TRB_ERR = 5, + XHCI_TRB_CC_SHORT_PACKET = 13, }; /* DbC endpoint types */ @@ -243,6 +244,7 @@ struct dbc { struct xhci_trb_ring dbc_oring; struct xhci_trb_ring dbc_iring; struct dbc_work_ring dbc_owork; + struct dbc_work_ring dbc_iwork; struct xhci_string_descriptor *dbc_str; pci_sbdf_t sbdf; @@ -440,6 +442,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb) trb->ctrl |= 0x20; } +static uint64_t xhci_trb_norm_buf(struct xhci_trb *trb) +{ + return trb->params; +} + +static uint32_t xhci_trb_norm_len(struct xhci_trb *trb) +{ + return trb->status & 0x1FFFF; +} + /** * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event * TRBs are read-only from software @@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct xhci_trb *trb) return trb->status >> 24; } +/* Amount of data _not_ transferred */ +static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb) +{ + return trb->status & 0x1FFFF; +} + /* Fields for link TRBs (section 6.4.4.1) */ static void xhci_trb_link_set_rsp(struct xhci_trb *trb, uint64_t rsp) { @@ -495,6 +513,14 @@ static int xhci_trb_ring_full(const struct xhci_trb_ring *ring) return ((ring->enq + 1) & (DBC_TRB_RING_CAP - 1)) == ring->deq; } +static int xhci_trb_ring_size(const struct xhci_trb_ring *ring) +{ + if ( ring->enq >= ring->deq ) + return ring->enq - ring->deq; + + return DBC_TRB_RING_CAP - ring->deq + ring->enq; +} + static int dbc_work_ring_full(const struct dbc_work_ring *ring) { return ((ring->enq + 1) & (DBC_WORK_RING_CAP - 1)) == ring->deq; @@ -508,6 +534,14 @@ static uint64_t dbc_work_ring_size(const struct dbc_work_ring *ring) return DBC_WORK_RING_CAP - ring->deq + ring->enq; } +static uint64_t dbc_work_ring_space_to_end(const struct dbc_work_ring *ring) +{ + if ( ring->enq >= ring->deq ) + return DBC_WORK_RING_CAP - ring->enq; + + return ring->deq - ring->enq; +} + static void dbc_push_trb(struct dbc *dbc, struct xhci_trb_ring *ring, uint64_t dma, uint64_t len) { @@ -568,6 +602,31 @@ static int64_t dbc_push_work(struct dbc *dbc, struct dbc_work_ring *ring, return i; } +static void dbc_rx_trb(struct dbc *dbc, struct xhci_trb *trb, + uint64_t not_transferred) +{ + struct dbc_work_ring *ring = &dbc->dbc_iwork; + unsigned int rx_len; + unsigned int end, start = ring->enq; + + if ( xhci_trb_type(trb) != XHCI_TRB_NORM ) + /* Can be Link TRB for example. */ + return; + + ASSERT(xhci_trb_norm_buf(trb) == ring->dma + ring->enq); + ASSERT(xhci_trb_norm_len(trb) >= not_transferred); + rx_len = xhci_trb_norm_len(trb) - not_transferred; + + /* It can hit the ring end, but should not wrap around. */ + ASSERT(ring->enq + rx_len <= DBC_WORK_RING_CAP); + ring->enq = (ring->enq + rx_len) & (DBC_WORK_RING_CAP - 1); + + end = ring->enq; + + if ( end > start ) + cache_flush(&ring->buf[start], end - start); +} + /* * Note that if IN transfer support is added, then this * will need to be changed; it assumes an OUT transfer ring only @@ -577,6 +636,7 @@ static void dbc_pop_events(struct dbc *dbc) struct dbc_reg *reg = dbc->dbc_reg; struct xhci_trb_ring *er = &dbc->dbc_ering; struct xhci_trb_ring *tr = &dbc->dbc_oring; + struct xhci_trb_ring *ir = &dbc->dbc_iring; struct xhci_trb *event = &er->trb[er->deq]; uint64_t erdp = readq(®->erdp); uint32_t portsc; @@ -602,6 +662,14 @@ static void dbc_pop_events(struct dbc *dbc) trb_idx = (event_ptr - tr->dma) >> XHCI_TRB_SHIFT; tr->deq = (trb_idx + 1) & (DBC_TRB_RING_CAP - 1); } + else if ( event_ptr - ir->dma < DBC_TRB_RING_BYTES ) + { + trb_idx = (event_ptr - ir->dma) >> XHCI_TRB_SHIFT; + if ( xhci_trb_tfre_cc(event) == XHCI_TRB_CC_SUCCESS || + xhci_trb_tfre_cc(event) == XHCI_TRB_CC_SHORT_PACKET ) + dbc_rx_trb(dbc, &ir->trb[trb_idx], xhci_trb_tfre_len(event)); + ir->deq = (trb_idx + 1) & (DBC_TRB_RING_CAP - 1); + } else dbc_alert("event: TRB 0x%lx not found in any ring\n", event_ptr); @@ -872,6 +940,7 @@ static bool __init dbc_open(struct dbc *dbc) return false; dbc_init_work_ring(dbc, &dbc->dbc_owork); + dbc_init_work_ring(dbc, &dbc->dbc_iwork); dbc_enable_dbc(dbc); dbc->open = true; @@ -985,6 +1054,33 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, } /** + * Ensure DbC has a pending transfer TRB to receive data into. + * + * @param dbc the dbc to flush + * @param trb the ring for the TRBs to transfer + * @param wrk the work ring to receive data into + */ +static void dbc_enqueue_in(struct dbc *dbc, struct xhci_trb_ring *trb, + struct dbc_work_ring *wrk) +{ + struct dbc_reg *reg = dbc->dbc_reg; + uint32_t db = (readl(®->db) & 0xFFFF00FF) | (trb->db << 8); + + /* Check if there is already queued TRB */ + if ( xhci_trb_ring_size(trb) >= 1 ) + return; + + if ( dbc_work_ring_full(wrk) ) + return; + + dbc_push_trb(dbc, trb, wrk->dma + wrk->enq, + dbc_work_ring_space_to_end(wrk)); + + wmb(); + writel(db, ®->db); +} + +/** * Queue a single character to the DbC. A transfer TRB will be created * if the character is a newline and the DbC will be notified that data is * available for writing to the debug host. @@ -1007,6 +1103,19 @@ static int64_t dbc_putc(struct dbc *dbc, char c) return 1; } +static int dbc_getc(struct dbc *dbc, char *c) +{ + struct dbc_work_ring *wrk = &dbc->dbc_iwork; + + if ( dbc_work_ring_size(wrk) == 0 ) + return 0; + + *c = wrk->buf[wrk->deq]; + wrk->deq = (wrk->deq + 1) & (DBC_WORK_RING_CAP - 1); + + return 1; +} + struct dbc_uart { struct dbc dbc; struct timer timer; @@ -1032,6 +1141,9 @@ static void cf_check dbc_uart_poll(void *data) spin_unlock_irqrestore(&port->tx_lock, flags); } + while ( dbc_work_ring_size(&dbc->dbc_iwork) ) + serial_rx_interrupt(port, guest_cpu_user_regs()); + serial_tx_interrupt(port, guest_cpu_user_regs()); set_timer(&uart->timer, NOW() + MICROSECS(DBC_POLL_INTERVAL)); } @@ -1092,6 +1204,12 @@ static void cf_check dbc_uart_putc(struct serial_port *port, char c) dbc_putc(&uart->dbc, c); } +static int cf_check dbc_uart_getc(struct serial_port *port, char *c) +{ + struct dbc_uart *uart = port->uart; + return dbc_getc(&uart->dbc, c); +} + static void cf_check dbc_uart_flush(struct serial_port *port) { s_time_t goal; @@ -1111,6 +1229,7 @@ static struct uart_driver dbc_uart_driver = { .init_postirq = dbc_uart_init_postirq, .tx_ready = dbc_uart_tx_ready, .putc = dbc_uart_putc, + .getc = dbc_uart_getc, .flush = dbc_uart_flush, }; @@ -1119,6 +1238,7 @@ struct dbc_dma_bufs { struct xhci_trb out_trb[DBC_TRB_RING_CAP]; struct xhci_trb in_trb[DBC_TRB_RING_CAP]; uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE); + uint8_t in_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE); struct xhci_erst_segment erst __aligned(16); struct xhci_dbc_ctx ctx __aligned(16); struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT]; @@ -1200,6 +1320,7 @@ void __init xhci_dbc_uart_init(void) dbc->dbc_oring.trb = dbc_dma_bufs.out_trb; dbc->dbc_iring.trb = dbc_dma_bufs.in_trb; dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf; + dbc->dbc_iwork.buf = dbc_dma_bufs.in_wrk_buf; dbc->dbc_str = dbc_dma_bufs.str_buf; if ( dbc_open(dbc) )
Add another work ring buffer for received data, and point IN TRB at it. Ensure there is always at least one pending IN TRB, so the controller has a way to send incoming data to the driver. Note that both "success" and "short packet" completion codes are okay - in fact it will be "short packet" most of the time, as the TRB length is about maximum size, not required size. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- New patch in v3 --- docs/misc/xen-command-line.pandoc | 4 +- xen/drivers/char/xhci-dbc.c | 121 +++++++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 2 deletions(-)