* [PATCH 0/9] dev-serial: minor fixes and improvements
@ 2020-10-26 8:33 Mark Cave-Ayland
2020-10-26 8:33 ` [PATCH 1/9] dev-serial: style changes to improve readability and checkpatch fixes Mark Cave-Ayland
` (9 more replies)
0 siblings, 10 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:33 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
This series comes from a client project that I have been working on over the
past few months which involves communicating with serial hardware and
associated simulators using the QEMU USB serial device.
With thanks to Craig Stevens at Renesas for giving permission for these
patches to be upstreamed.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Mark Cave-Ayland (9):
dev-serial: style changes to improve readability and checkpatch fixes
dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments
dev-serial: convert from DPRINTF to trace-events
dev-serial: add trace-events for baud rate and data parameters
dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent
macros from usb.h
dev-serial: add always-plugged property to ensure USB device is always
attached
dev-serial: add support for setting data_bits in QEMUSerialSetParams
dev-serial: fix FTDI_GET_MDM_ST response
dev-serial: store flow control and xon/xoff characters
hw/usb/dev-serial.c | 339 +++++++++++++++++++++++++++-----------------
hw/usb/trace-events | 13 ++
2 files changed, 218 insertions(+), 134 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/9] dev-serial: style changes to improve readability and checkpatch fixes
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
@ 2020-10-26 8:33 ` Mark Cave-Ayland
2020-10-26 9:35 ` Samuel Thibault
2020-10-26 8:33 ` [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments Mark Cave-Ayland
` (8 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:33 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 230 ++++++++++++++++++++++++--------------------
1 file changed, 126 insertions(+), 104 deletions(-)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index b1622b7c7f..7a5fa3770e 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -33,72 +33,75 @@ do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
#define RECV_BUF (512 - (2 * 8))
/* Commands */
-#define FTDI_RESET 0
-#define FTDI_SET_MDM_CTRL 1
-#define FTDI_SET_FLOW_CTRL 2
-#define FTDI_SET_BAUD 3
-#define FTDI_SET_DATA 4
-#define FTDI_GET_MDM_ST 5
-#define FTDI_SET_EVENT_CHR 6
-#define FTDI_SET_ERROR_CHR 7
-#define FTDI_SET_LATENCY 9
-#define FTDI_GET_LATENCY 10
-
-#define DeviceOutVendor ((USB_DIR_OUT|USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8)
-#define DeviceInVendor ((USB_DIR_IN |USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8)
+#define FTDI_RESET 0
+#define FTDI_SET_MDM_CTRL 1
+#define FTDI_SET_FLOW_CTRL 2
+#define FTDI_SET_BAUD 3
+#define FTDI_SET_DATA 4
+#define FTDI_GET_MDM_ST 5
+#define FTDI_SET_EVENT_CHR 6
+#define FTDI_SET_ERROR_CHR 7
+#define FTDI_SET_LATENCY 9
+#define FTDI_GET_LATENCY 10
+
+#define DeviceOutVendor \
+ ((USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
+#define DeviceInVendor \
+ ((USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
/* RESET */
-#define FTDI_RESET_SIO 0
-#define FTDI_RESET_RX 1
-#define FTDI_RESET_TX 2
+#define FTDI_RESET_SIO 0
+#define FTDI_RESET_RX 1
+#define FTDI_RESET_TX 2
/* SET_MDM_CTRL */
-#define FTDI_DTR 1
-#define FTDI_SET_DTR (FTDI_DTR << 8)
-#define FTDI_RTS 2
-#define FTDI_SET_RTS (FTDI_RTS << 8)
+#define FTDI_DTR 1
+#define FTDI_SET_DTR (FTDI_DTR << 8)
+#define FTDI_RTS 2
+#define FTDI_SET_RTS (FTDI_RTS << 8)
/* SET_FLOW_CTRL */
-#define FTDI_RTS_CTS_HS 1
-#define FTDI_DTR_DSR_HS 2
-#define FTDI_XON_XOFF_HS 4
+#define FTDI_RTS_CTS_HS 1
+#define FTDI_DTR_DSR_HS 2
+#define FTDI_XON_XOFF_HS 4
/* SET_DATA */
-#define FTDI_PARITY (0x7 << 8)
-#define FTDI_ODD (0x1 << 8)
-#define FTDI_EVEN (0x2 << 8)
-#define FTDI_MARK (0x3 << 8)
-#define FTDI_SPACE (0x4 << 8)
+#define FTDI_PARITY (0x7 << 8)
+#define FTDI_ODD (0x1 << 8)
+#define FTDI_EVEN (0x2 << 8)
+#define FTDI_MARK (0x3 << 8)
+#define FTDI_SPACE (0x4 << 8)
-#define FTDI_STOP (0x3 << 11)
-#define FTDI_STOP1 (0x0 << 11)
-#define FTDI_STOP15 (0x1 << 11)
-#define FTDI_STOP2 (0x2 << 11)
+#define FTDI_STOP (0x3 << 11)
+#define FTDI_STOP1 (0x0 << 11)
+#define FTDI_STOP15 (0x1 << 11)
+#define FTDI_STOP2 (0x2 << 11)
/* GET_MDM_ST */
/* TODO: should be sent every 40ms */
-#define FTDI_CTS (1<<4) // CTS line status
-#define FTDI_DSR (1<<5) // DSR line status
-#define FTDI_RI (1<<6) // RI line status
-#define FTDI_RLSD (1<<7) // Receive Line Signal Detect
+#define FTDI_CTS (1 << 4) /* CTS line status */
+#define FTDI_DSR (1 << 5) /* DSR line status */
+#define FTDI_RI (1 << 6) /* RI line status */
+#define FTDI_RLSD (1 << 7) /* Receive Line Signal Detect */
/* Status */
-#define FTDI_DR (1<<0) // Data Ready
-#define FTDI_OE (1<<1) // Overrun Err
-#define FTDI_PE (1<<2) // Parity Err
-#define FTDI_FE (1<<3) // Framing Err
-#define FTDI_BI (1<<4) // Break Interrupt
-#define FTDI_THRE (1<<5) // Transmitter Holding Register
-#define FTDI_TEMT (1<<6) // Transmitter Empty
-#define FTDI_FIFO (1<<7) // Error in FIFO
+#define FTDI_DR (1 << 0) /* Data Ready */
+#define FTDI_OE (1 << 1) /* Overrun Err */
+#define FTDI_PE (1 << 2) /* Parity Err */
+#define FTDI_FE (1 << 3) /* Framing Err */
+#define FTDI_BI (1 << 4) /* Break Interrupt */
+#define FTDI_THRE (1 << 5) /* Transmitter Holding Register */
+#define FTDI_TEMT (1 << 6) /* Transmitter Empty */
+#define FTDI_FIFO (1 << 7) /* Error in FIFO */
struct USBSerialState {
USBDevice dev;
+
USBEndpoint *intr;
uint8_t recv_buf[RECV_BUF];
uint16_t recv_ptr;
@@ -216,29 +219,34 @@ static uint8_t usb_get_modem_lines(USBSerialState *s)
if (qemu_chr_fe_ioctl(&s->cs,
CHR_IOCTL_SERIAL_GET_TIOCM, &flags) == -ENOTSUP) {
- return FTDI_CTS|FTDI_DSR|FTDI_RLSD;
+ return FTDI_CTS | FTDI_DSR | FTDI_RLSD;
}
ret = 0;
- if (flags & CHR_TIOCM_CTS)
+ if (flags & CHR_TIOCM_CTS) {
ret |= FTDI_CTS;
- if (flags & CHR_TIOCM_DSR)
+ }
+ if (flags & CHR_TIOCM_DSR) {
ret |= FTDI_DSR;
- if (flags & CHR_TIOCM_RI)
+ }
+ if (flags & CHR_TIOCM_RI) {
ret |= FTDI_RI;
- if (flags & CHR_TIOCM_CAR)
+ }
+ if (flags & CHR_TIOCM_CAR) {
ret |= FTDI_RLSD;
+ }
return ret;
}
static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
- int request, int value, int index, int length, uint8_t *data)
+ int request, int value, int index,
+ int length, uint8_t *data)
{
USBSerialState *s = (USBSerialState *)dev;
int ret;
- DPRINTF("got control %x, value %x\n",request, value);
+ DPRINTF("got control %x, value %x\n", request, value);
ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
if (ret >= 0) {
return;
@@ -248,7 +256,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
case EndpointOutRequest | USB_REQ_CLEAR_FEATURE:
break;
- /* Class specific requests. */
+ /* Class specific requests. */
case DeviceOutVendor | FTDI_RESET:
switch (value) {
case FTDI_RESET_SIO:
@@ -269,16 +277,18 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
static int flags;
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_GET_TIOCM, &flags);
if (value & FTDI_SET_RTS) {
- if (value & FTDI_RTS)
+ if (value & FTDI_RTS) {
flags |= CHR_TIOCM_RTS;
- else
+ } else {
flags &= ~CHR_TIOCM_RTS;
+ }
}
if (value & FTDI_SET_DTR) {
- if (value & FTDI_DTR)
+ if (value & FTDI_DTR) {
flags |= CHR_TIOCM_DTR;
- else
+ } else {
flags &= ~CHR_TIOCM_DTR;
+ }
}
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
break;
@@ -293,10 +303,12 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
int divisor = value & 0x3fff;
/* chip special cases */
- if (divisor == 1 && subdivisor8 == 0)
+ if (divisor == 1 && subdivisor8 == 0) {
subdivisor8 = 4;
- if (divisor == 0 && subdivisor8 == 0)
+ }
+ if (divisor == 0 && subdivisor8 == 0) {
divisor = 1;
+ }
s->params.speed = (48000000 / 2) / (8 * divisor + subdivisor8);
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
@@ -304,30 +316,32 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
}
case DeviceOutVendor | FTDI_SET_DATA:
switch (value & FTDI_PARITY) {
- case 0:
- s->params.parity = 'N';
- break;
- case FTDI_ODD:
- s->params.parity = 'O';
- break;
- case FTDI_EVEN:
- s->params.parity = 'E';
- break;
- default:
- DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
- goto fail;
+ case 0:
+ s->params.parity = 'N';
+ break;
+ case FTDI_ODD:
+ s->params.parity = 'O';
+ break;
+ case FTDI_EVEN:
+ s->params.parity = 'E';
+ break;
+ default:
+ DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
+ goto fail;
}
+
switch (value & FTDI_STOP) {
- case FTDI_STOP1:
- s->params.stop_bits = 1;
- break;
- case FTDI_STOP2:
- s->params.stop_bits = 2;
- break;
- default:
- DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
- goto fail;
+ case FTDI_STOP1:
+ s->params.stop_bits = 1;
+ break;
+ case FTDI_STOP2:
+ s->params.stop_bits = 2;
+ break;
+ default:
+ DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
+ goto fail;
}
+
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
/* TODO: TX ON/OFF */
break;
@@ -423,20 +437,24 @@ static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
switch (p->pid) {
case USB_TOKEN_OUT:
- if (devep != 2)
+ if (devep != 2) {
goto fail;
+ }
for (i = 0; i < p->iov.niov; i++) {
iov = p->iov.iov + i;
- /* XXX this blocks entire thread. Rewrite to use
- * qemu_chr_fe_write and background I/O callbacks */
+ /*
+ * XXX this blocks entire thread. Rewrite to use
+ * qemu_chr_fe_write and background I/O callbacks
+ */
qemu_chr_fe_write_all(&s->cs, iov->iov_base, iov->iov_len);
}
p->actual_length = p->iov.size;
break;
case USB_TOKEN_IN:
- if (devep != 1)
+ if (devep != 1) {
goto fail;
+ }
usb_serial_token_in(s, p);
break;
@@ -464,21 +482,24 @@ static void usb_serial_read(void *opaque, const uint8_t *buf, int size)
int first_size, start;
/* room in the buffer? */
- if (size > (RECV_BUF - s->recv_used))
+ if (size > (RECV_BUF - s->recv_used)) {
size = RECV_BUF - s->recv_used;
+ }
start = s->recv_ptr + s->recv_used;
if (start < RECV_BUF) {
/* copy data to end of buffer */
first_size = RECV_BUF - start;
- if (first_size > size)
+ if (first_size > size) {
first_size = size;
+ }
memcpy(s->recv_buf + start, buf, first_size);
/* wrap around to front if needed */
- if (size > first_size)
+ if (size > first_size) {
memcpy(s->recv_buf, buf + first_size, size - first_size);
+ }
} else {
start -= RECV_BUF;
memcpy(s->recv_buf + start, buf, size);
@@ -493,23 +514,23 @@ static void usb_serial_event(void *opaque, QEMUChrEvent event)
USBSerialState *s = opaque;
switch (event) {
- case CHR_EVENT_BREAK:
- s->event_trigger |= FTDI_BI;
- break;
- case CHR_EVENT_OPENED:
- if (!s->dev.attached) {
- usb_device_attach(&s->dev, &error_abort);
- }
- break;
- case CHR_EVENT_CLOSED:
- if (s->dev.attached) {
- usb_device_detach(&s->dev);
- }
- break;
- case CHR_EVENT_MUX_IN:
- case CHR_EVENT_MUX_OUT:
- /* Ignore */
- break;
+ case CHR_EVENT_BREAK:
+ s->event_trigger |= FTDI_BI;
+ break;
+ case CHR_EVENT_OPENED:
+ if (!s->dev.attached) {
+ usb_device_attach(&s->dev, &error_abort);
+ }
+ break;
+ case CHR_EVENT_CLOSED:
+ if (s->dev.attached) {
+ usb_device_detach(&s->dev);
+ }
+ break;
+ case CHR_EVENT_MUX_IN:
+ case CHR_EVENT_MUX_OUT:
+ /* Ignore */
+ break;
}
}
@@ -549,8 +570,9 @@ static USBDevice *usb_braille_init(const char *unused)
Chardev *cdrv;
cdrv = qemu_chr_new("braille", "braille", NULL);
- if (!cdrv)
+ if (!cdrv) {
return NULL;
+ }
dev = usb_new("usb-braille");
qdev_prop_set_chr(&dev->qdev, "chardev", cdrv);
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
2020-10-26 8:33 ` [PATCH 1/9] dev-serial: style changes to improve readability and checkpatch fixes Mark Cave-Ayland
@ 2020-10-26 8:33 ` Mark Cave-Ayland
2020-10-26 9:37 ` Samuel Thibault
2020-10-27 9:04 ` Philippe Mathieu-Daudé
2020-10-26 8:33 ` [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events Mark Cave-Ayland
` (7 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:33 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 7a5fa3770e..77ce89d38b 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -204,7 +204,7 @@ static void usb_serial_reset(USBSerialState *s)
static void usb_serial_handle_reset(USBDevice *dev)
{
- USBSerialState *s = (USBSerialState *)dev;
+ USBSerialState *s = USB_SERIAL(dev);
DPRINTF("Reset\n");
@@ -243,7 +243,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
int request, int value, int index,
int length, uint8_t *data)
{
- USBSerialState *s = (USBSerialState *)dev;
+ USBSerialState *s = USB_SERIAL(dev);
int ret;
DPRINTF("got control %x, value %x\n", request, value);
@@ -430,7 +430,7 @@ static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
{
- USBSerialState *s = (USBSerialState *)dev;
+ USBSerialState *s = USB_SERIAL(dev);
uint8_t devep = p->ep->nr;
struct iovec *iov;
int i;
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
2020-10-26 8:33 ` [PATCH 1/9] dev-serial: style changes to improve readability and checkpatch fixes Mark Cave-Ayland
2020-10-26 8:33 ` [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments Mark Cave-Ayland
@ 2020-10-26 8:33 ` Mark Cave-Ayland
2020-10-26 9:38 ` Samuel Thibault
2020-10-27 9:08 ` Philippe Mathieu-Daudé
2020-10-26 8:33 ` [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters Mark Cave-Ayland
` (6 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:33 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 28 ++++++++++++++--------------
hw/usb/trace-events | 8 ++++++++
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 77ce89d38b..abc316c7bf 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -20,15 +20,8 @@
#include "chardev/char-serial.h"
#include "chardev/char-fe.h"
#include "qom/object.h"
+#include "trace.h"
-//#define DEBUG_Serial
-
-#ifdef DEBUG_Serial
-#define DPRINTF(fmt, ...) \
-do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) do {} while(0)
-#endif
#define RECV_BUF (512 - (2 * 8))
@@ -205,8 +198,9 @@ static void usb_serial_reset(USBSerialState *s)
static void usb_serial_handle_reset(USBDevice *dev)
{
USBSerialState *s = USB_SERIAL(dev);
+ USBBus *bus = usb_bus_from_device(dev);
- DPRINTF("Reset\n");
+ trace_usb_serial_reset(bus->busnr, dev->addr);
usb_serial_reset(s);
/* TODO: Reset char device, send BREAK? */
@@ -244,9 +238,11 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
int length, uint8_t *data)
{
USBSerialState *s = USB_SERIAL(dev);
+ USBBus *bus = usb_bus_from_device(dev);
int ret;
- DPRINTF("got control %x, value %x\n", request, value);
+ trace_usb_serial_handle_control(bus->busnr, dev->addr, request, value);
+
ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
if (ret >= 0) {
return;
@@ -326,7 +322,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
s->params.parity = 'E';
break;
default:
- DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
+ trace_usb_serial_unsupported_parity(bus->busnr, dev->addr,
+ value & FTDI_PARITY);
goto fail;
}
@@ -338,7 +335,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
s->params.stop_bits = 2;
break;
default:
- DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
+ trace_usb_serial_unsupported_stopbits(bus->busnr, dev->addr,
+ value & FTDI_STOP);
goto fail;
}
@@ -367,7 +365,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
break;
default:
fail:
- DPRINTF("got unsupported/bogus control %x, value %x\n", request, value);
+ trace_usb_serial_unsupported_control(bus->busnr, dev->addr, request,
+ value);
p->status = USB_RET_STALL;
break;
}
@@ -431,6 +430,7 @@ static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
{
USBSerialState *s = USB_SERIAL(dev);
+ USBBus *bus = usb_bus_from_device(dev);
uint8_t devep = p->ep->nr;
struct iovec *iov;
int i;
@@ -459,7 +459,7 @@ static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
break;
default:
- DPRINTF("Bad token\n");
+ trace_usb_serial_bad_token(bus->busnr, dev->addr);
fail:
p->status = USB_RET_STALL;
break;
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 72e4298780..e5871cbbbc 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -320,3 +320,11 @@ usb_host_parse_interface(int bus, int addr, int num, int alt, int active) "dev %
usb_host_parse_endpoint(int bus, int addr, int ep, const char *dir, const char *type, int active) "dev %d:%d, ep %d, %s, %s, active %d"
usb_host_parse_error(int bus, int addr, const char *errmsg) "dev %d:%d, msg %s"
usb_host_remote_wakeup_removed(int bus, int addr) "dev %d:%d"
+
+# dev-serial.c
+usb_serial_reset(int bus, int addr) "dev %d:%d reset"
+usb_serial_handle_control(int bus, int addr, int request, int value) "dev %d:%d got control 0x%x, value 0x%x"
+usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d unsupported parity %d"
+usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d unsupported stop bits %d"
+usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
+usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
` (2 preceding siblings ...)
2020-10-26 8:33 ` [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events Mark Cave-Ayland
@ 2020-10-26 8:33 ` Mark Cave-Ayland
2020-10-26 9:40 ` Samuel Thibault
2020-10-27 9:08 ` Philippe Mathieu-Daudé
2020-10-26 8:33 ` [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h Mark Cave-Ayland
` (5 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:33 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 3 +++
hw/usb/trace-events | 2 ++
2 files changed, 5 insertions(+)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index abc316c7bf..badf8785db 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -307,6 +307,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
}
s->params.speed = (48000000 / 2) / (8 * divisor + subdivisor8);
+ trace_usb_serial_set_baud(bus->busnr, dev->addr, s->params.speed);
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
break;
}
@@ -340,6 +341,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
goto fail;
}
+ trace_usb_serial_set_data(bus->busnr, dev->addr, s->params.parity,
+ s->params.data_bits, s->params.stop_bits);
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
/* TODO: TX ON/OFF */
break;
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index e5871cbbbc..9e984b2e0c 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -328,3 +328,5 @@ usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d unsupport
usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d unsupported stop bits %d"
usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
+usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%d baud rate %d"
+usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev %d:%d parity %c, data bits %d, stop bits %d"
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
` (3 preceding siblings ...)
2020-10-26 8:33 ` [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters Mark Cave-Ayland
@ 2020-10-26 8:33 ` Mark Cave-Ayland
2020-10-26 9:41 ` Samuel Thibault
2020-10-27 9:09 ` Philippe Mathieu-Daudé
2020-10-26 8:33 ` [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached Mark Cave-Ayland
` (4 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:33 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
The DeviceOutVendor and DeviceInVendor macros can be replaced with their
equivalent VendorDeviceOutRequest and VendorDeviceRequest macros from usb.h.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index badf8785db..92c35615eb 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -37,11 +37,6 @@
#define FTDI_SET_LATENCY 9
#define FTDI_GET_LATENCY 10
-#define DeviceOutVendor \
- ((USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
-#define DeviceInVendor \
- ((USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
-
/* RESET */
#define FTDI_RESET_SIO 0
@@ -253,7 +248,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
break;
/* Class specific requests. */
- case DeviceOutVendor | FTDI_RESET:
+ case VendorDeviceOutRequest | FTDI_RESET:
switch (value) {
case FTDI_RESET_SIO:
usb_serial_reset(s);
@@ -268,7 +263,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
break;
}
break;
- case DeviceOutVendor | FTDI_SET_MDM_CTRL:
+ case VendorDeviceOutRequest | FTDI_SET_MDM_CTRL:
{
static int flags;
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_GET_TIOCM, &flags);
@@ -289,10 +284,10 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
break;
}
- case DeviceOutVendor | FTDI_SET_FLOW_CTRL:
+ case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL:
/* TODO: ioctl */
break;
- case DeviceOutVendor | FTDI_SET_BAUD: {
+ case VendorDeviceOutRequest | FTDI_SET_BAUD: {
static const int subdivisors8[8] = { 0, 4, 2, 1, 3, 5, 6, 7 };
int subdivisor8 = subdivisors8[((value & 0xc000) >> 14)
| ((index & 1) << 2)];
@@ -311,7 +306,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
break;
}
- case DeviceOutVendor | FTDI_SET_DATA:
+ case VendorDeviceOutRequest | FTDI_SET_DATA:
switch (value & FTDI_PARITY) {
case 0:
s->params.parity = 'N';
@@ -346,23 +341,23 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
/* TODO: TX ON/OFF */
break;
- case DeviceInVendor | FTDI_GET_MDM_ST:
+ case VendorDeviceRequest | FTDI_GET_MDM_ST:
data[0] = usb_get_modem_lines(s) | 1;
data[1] = FTDI_THRE | FTDI_TEMT;
p->actual_length = 2;
break;
- case DeviceOutVendor | FTDI_SET_EVENT_CHR:
+ case VendorDeviceOutRequest | FTDI_SET_EVENT_CHR:
/* TODO: handle it */
s->event_chr = value;
break;
- case DeviceOutVendor | FTDI_SET_ERROR_CHR:
+ case VendorDeviceOutRequest | FTDI_SET_ERROR_CHR:
/* TODO: handle it */
s->error_chr = value;
break;
- case DeviceOutVendor | FTDI_SET_LATENCY:
+ case VendorDeviceOutRequest | FTDI_SET_LATENCY:
s->latency = value;
break;
- case DeviceInVendor | FTDI_GET_LATENCY:
+ case VendorDeviceRequest | FTDI_GET_LATENCY:
data[0] = s->latency;
p->actual_length = 1;
break;
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
` (4 preceding siblings ...)
2020-10-26 8:33 ` [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h Mark Cave-Ayland
@ 2020-10-26 8:33 ` Mark Cave-Ayland
2020-10-26 9:45 ` Samuel Thibault
2020-10-27 8:09 ` Gerd Hoffmann
2020-10-26 8:33 ` [PATCH 7/9] dev-serial: add support for setting data_bits in QEMUSerialSetParams Mark Cave-Ayland
` (3 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:33 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
Some operating systems will generate a new device ID when a USB device is unplugged
and then replugged into the USB. If this is done whilst switching between multiple
applications over a virtual serial port, the change of device ID requires going
back into the OS/application to locate the new device accordingly.
Add a new always-plugged property that if specified will ensure that the device
always remains attached to the USB regardless of the state of the backend
chardev.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 92c35615eb..919e25e1d9 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -97,6 +97,7 @@ struct USBSerialState {
uint8_t event_chr;
uint8_t error_chr;
uint8_t event_trigger;
+ bool always_plugged;
QEMUSerialSetParams params;
int latency; /* ms */
CharBackend cs;
@@ -516,12 +517,12 @@ static void usb_serial_event(void *opaque, QEMUChrEvent event)
s->event_trigger |= FTDI_BI;
break;
case CHR_EVENT_OPENED:
- if (!s->dev.attached) {
+ if (!s->always_plugged && !s->dev.attached) {
usb_device_attach(&s->dev, &error_abort);
}
break;
case CHR_EVENT_CLOSED:
- if (s->dev.attached) {
+ if (!s->always_plugged && s->dev.attached) {
usb_device_detach(&s->dev);
}
break;
@@ -556,7 +557,8 @@ static void usb_serial_realize(USBDevice *dev, Error **errp)
usb_serial_event, NULL, s, NULL, true);
usb_serial_handle_reset(dev);
- if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
+ if (s->always_plugged || (qemu_chr_fe_backend_open(&s->cs) &&
+ !dev->attached)) {
usb_device_attach(dev, &error_abort);
}
s->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
@@ -584,6 +586,7 @@ static const VMStateDescription vmstate_usb_serial = {
static Property serial_properties[] = {
DEFINE_PROP_CHR("chardev", USBSerialState, cs),
+ DEFINE_PROP_BOOL("always-plugged", USBSerialState, always_plugged, false),
DEFINE_PROP_END_OF_LIST(),
};
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 7/9] dev-serial: add support for setting data_bits in QEMUSerialSetParams
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
` (5 preceding siblings ...)
2020-10-26 8:33 ` [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached Mark Cave-Ayland
@ 2020-10-26 8:33 ` Mark Cave-Ayland
2020-10-26 9:46 ` Samuel Thibault
2020-10-26 8:34 ` [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response Mark Cave-Ayland
` (2 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:33 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
Also implement the behaviour reported in Linux's ftdi_sio.c whereby if an invalid
data_bits value is provided then the hardware defaults to using 8.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 17 +++++++++++++++++
hw/usb/trace-events | 1 +
2 files changed, 18 insertions(+)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 919e25e1d9..4c374d0790 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -308,6 +308,23 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
break;
}
case VendorDeviceOutRequest | FTDI_SET_DATA:
+ switch (value & 0xff) {
+ case 7:
+ s->params.data_bits = 7;
+ break;
+ case 8:
+ s->params.data_bits = 8;
+ break;
+ default:
+ /*
+ * According to a comment in Linux's ftdi_sio.c original FTDI
+ * chips fall back to 8 data bits for unsupported data_bits
+ */
+ trace_usb_serial_unsupported_data_bits(bus->busnr, dev->addr,
+ value & 0xff);
+ s->params.data_bits = 8;
+ }
+
switch (value & FTDI_PARITY) {
case 0:
s->params.parity = 'N';
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 9e984b2e0c..0d0a3e5f2a 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -327,6 +327,7 @@ usb_serial_handle_control(int bus, int addr, int request, int value) "dev %d:%d
usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d unsupported parity %d"
usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d unsupported stop bits %d"
usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
+usb_serial_unsupported_data_bits(int bus, int addr, int value) "dev %d:%d unsupported data bits %d, falling back to 8"
usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%d baud rate %d"
usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev %d:%d parity %c, data bits %d, stop bits %d"
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
` (6 preceding siblings ...)
2020-10-26 8:33 ` [PATCH 7/9] dev-serial: add support for setting data_bits in QEMUSerialSetParams Mark Cave-Ayland
@ 2020-10-26 8:34 ` Mark Cave-Ayland
2020-10-26 9:54 ` Samuel Thibault
2020-10-26 8:34 ` [PATCH 9/9] dev-serial: store flow control and xon/xoff characters Mark Cave-Ayland
2020-10-26 9:59 ` [PATCH 0/9] dev-serial: minor fixes and improvements Samuel Thibault
9 siblings, 1 reply; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:34 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
The FTDI_GET_MDM_ST response should only return a single byte indicating the
modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header
file).
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 4c374d0790..fa734bcf54 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
/* TODO: TX ON/OFF */
break;
case VendorDeviceRequest | FTDI_GET_MDM_ST:
- data[0] = usb_get_modem_lines(s) | 1;
- data[1] = FTDI_THRE | FTDI_TEMT;
- p->actual_length = 2;
+ data[0] = usb_get_modem_lines(s);
+ p->actual_length = 1;
break;
case VendorDeviceOutRequest | FTDI_SET_EVENT_CHR:
/* TODO: handle it */
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 9/9] dev-serial: store flow control and xon/xoff characters
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
` (7 preceding siblings ...)
2020-10-26 8:34 ` [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response Mark Cave-Ayland
@ 2020-10-26 8:34 ` Mark Cave-Ayland
2020-10-26 9:58 ` Samuel Thibault
2020-10-26 9:59 ` [PATCH 0/9] dev-serial: minor fixes and improvements Samuel Thibault
9 siblings, 1 reply; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 8:34 UTC (permalink / raw)
To: qemu-devel, kraxel, samuel.thibault
Note that whilst the device does not do anything with these values, they are
logged with trace events and stored to allow future implementation.
The default flow control is set to none at reset as documented in the Linux
ftdi_sio.h header file.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/usb/dev-serial.c | 38 +++++++++++++++++++++++++++++++++++---
hw/usb/trace-events | 2 ++
2 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index fa734bcf54..3dbc56d77a 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -52,6 +52,7 @@
/* SET_FLOW_CTRL */
+#define FTDI_NO_HS 0
#define FTDI_RTS_CTS_HS 1
#define FTDI_DTR_DSR_HS 2
#define FTDI_XON_XOFF_HS 4
@@ -98,6 +99,9 @@ struct USBSerialState {
uint8_t error_chr;
uint8_t event_trigger;
bool always_plugged;
+ uint8_t flow_control;
+ uint8_t xon;
+ uint8_t xoff;
QEMUSerialSetParams params;
int latency; /* ms */
CharBackend cs;
@@ -181,14 +185,36 @@ static const USBDesc desc_braille = {
.str = desc_strings,
};
+static void usb_serial_set_flow_control(USBSerialState *s,
+ uint8_t flow_control)
+{
+ USBDevice *dev = USB_DEVICE(s);
+ USBBus *bus = usb_bus_from_device(dev);
+
+ /* TODO: ioctl */
+ s->flow_control = flow_control;
+ trace_usb_serial_set_flow_control(bus->busnr, dev->addr, flow_control);
+}
+
+static void usb_serial_set_xonxoff(USBSerialState *s, int xonxoff)
+{
+ USBDevice *dev = USB_DEVICE(s);
+ USBBus *bus = usb_bus_from_device(dev);
+
+ s->xon = xonxoff & 0xff;
+ s->xoff = (xonxoff >> 8) & 0xff;
+
+ trace_usb_serial_set_xonxoff(bus->busnr, dev->addr, s->xon, s->xoff);
+}
+
static void usb_serial_reset(USBSerialState *s)
{
- /* TODO: Set flow control to none */
s->event_chr = 0x0d;
s->event_trigger = 0;
s->recv_ptr = 0;
s->recv_used = 0;
/* TODO: purge in char driver */
+ usb_serial_set_flow_control(s, FTDI_NO_HS);
}
static void usb_serial_handle_reset(USBDevice *dev)
@@ -285,9 +311,15 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
break;
}
- case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL:
- /* TODO: ioctl */
+ case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL: {
+ uint8_t flow_control = index >> 8;
+
+ usb_serial_set_flow_control(s, flow_control);
+ if (flow_control & FTDI_XON_XOFF_HS) {
+ usb_serial_set_xonxoff(s, value);
+ }
break;
+ }
case VendorDeviceOutRequest | FTDI_SET_BAUD: {
static const int subdivisors8[8] = { 0, 4, 2, 1, 3, 5, 6, 7 };
int subdivisor8 = subdivisors8[((value & 0xc000) >> 14)
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 0d0a3e5f2a..725b7077ad 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -331,3 +331,5 @@ usb_serial_unsupported_data_bits(int bus, int addr, int value) "dev %d:%d unsupp
usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%d baud rate %d"
usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev %d:%d parity %c, data bits %d, stop bits %d"
+usb_serial_set_flow_control(int bus, int addr, int index) "dev %d:%d flow control %d"
+usb_serial_set_xonxoff(int bus, int addr, uint8_t xon, uint8_t xoff) "dev %d:%d xon 0x%x xoff 0x%x"
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] dev-serial: style changes to improve readability and checkpatch fixes
2020-10-26 8:33 ` [PATCH 1/9] dev-serial: style changes to improve readability and checkpatch fixes Mark Cave-Ayland
@ 2020-10-26 9:35 ` Samuel Thibault
0 siblings, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:35 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Mark Cave-Ayland, le lun. 26 oct. 2020 08:33:53 +0000, a ecrit:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Samuel thibault <samuel.thibault@ens-lyon.org>
> ---
> hw/usb/dev-serial.c | 230 ++++++++++++++++++++++++--------------------
> 1 file changed, 126 insertions(+), 104 deletions(-)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index b1622b7c7f..7a5fa3770e 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -33,72 +33,75 @@ do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
> #define RECV_BUF (512 - (2 * 8))
>
> /* Commands */
> -#define FTDI_RESET 0
> -#define FTDI_SET_MDM_CTRL 1
> -#define FTDI_SET_FLOW_CTRL 2
> -#define FTDI_SET_BAUD 3
> -#define FTDI_SET_DATA 4
> -#define FTDI_GET_MDM_ST 5
> -#define FTDI_SET_EVENT_CHR 6
> -#define FTDI_SET_ERROR_CHR 7
> -#define FTDI_SET_LATENCY 9
> -#define FTDI_GET_LATENCY 10
> -
> -#define DeviceOutVendor ((USB_DIR_OUT|USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8)
> -#define DeviceInVendor ((USB_DIR_IN |USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8)
> +#define FTDI_RESET 0
> +#define FTDI_SET_MDM_CTRL 1
> +#define FTDI_SET_FLOW_CTRL 2
> +#define FTDI_SET_BAUD 3
> +#define FTDI_SET_DATA 4
> +#define FTDI_GET_MDM_ST 5
> +#define FTDI_SET_EVENT_CHR 6
> +#define FTDI_SET_ERROR_CHR 7
> +#define FTDI_SET_LATENCY 9
> +#define FTDI_GET_LATENCY 10
> +
> +#define DeviceOutVendor \
> + ((USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
> +#define DeviceInVendor \
> + ((USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
>
> /* RESET */
>
> -#define FTDI_RESET_SIO 0
> -#define FTDI_RESET_RX 1
> -#define FTDI_RESET_TX 2
> +#define FTDI_RESET_SIO 0
> +#define FTDI_RESET_RX 1
> +#define FTDI_RESET_TX 2
>
> /* SET_MDM_CTRL */
>
> -#define FTDI_DTR 1
> -#define FTDI_SET_DTR (FTDI_DTR << 8)
> -#define FTDI_RTS 2
> -#define FTDI_SET_RTS (FTDI_RTS << 8)
> +#define FTDI_DTR 1
> +#define FTDI_SET_DTR (FTDI_DTR << 8)
> +#define FTDI_RTS 2
> +#define FTDI_SET_RTS (FTDI_RTS << 8)
>
> /* SET_FLOW_CTRL */
>
> -#define FTDI_RTS_CTS_HS 1
> -#define FTDI_DTR_DSR_HS 2
> -#define FTDI_XON_XOFF_HS 4
> +#define FTDI_RTS_CTS_HS 1
> +#define FTDI_DTR_DSR_HS 2
> +#define FTDI_XON_XOFF_HS 4
>
> /* SET_DATA */
>
> -#define FTDI_PARITY (0x7 << 8)
> -#define FTDI_ODD (0x1 << 8)
> -#define FTDI_EVEN (0x2 << 8)
> -#define FTDI_MARK (0x3 << 8)
> -#define FTDI_SPACE (0x4 << 8)
> +#define FTDI_PARITY (0x7 << 8)
> +#define FTDI_ODD (0x1 << 8)
> +#define FTDI_EVEN (0x2 << 8)
> +#define FTDI_MARK (0x3 << 8)
> +#define FTDI_SPACE (0x4 << 8)
>
> -#define FTDI_STOP (0x3 << 11)
> -#define FTDI_STOP1 (0x0 << 11)
> -#define FTDI_STOP15 (0x1 << 11)
> -#define FTDI_STOP2 (0x2 << 11)
> +#define FTDI_STOP (0x3 << 11)
> +#define FTDI_STOP1 (0x0 << 11)
> +#define FTDI_STOP15 (0x1 << 11)
> +#define FTDI_STOP2 (0x2 << 11)
>
> /* GET_MDM_ST */
> /* TODO: should be sent every 40ms */
> -#define FTDI_CTS (1<<4) // CTS line status
> -#define FTDI_DSR (1<<5) // DSR line status
> -#define FTDI_RI (1<<6) // RI line status
> -#define FTDI_RLSD (1<<7) // Receive Line Signal Detect
> +#define FTDI_CTS (1 << 4) /* CTS line status */
> +#define FTDI_DSR (1 << 5) /* DSR line status */
> +#define FTDI_RI (1 << 6) /* RI line status */
> +#define FTDI_RLSD (1 << 7) /* Receive Line Signal Detect */
>
> /* Status */
>
> -#define FTDI_DR (1<<0) // Data Ready
> -#define FTDI_OE (1<<1) // Overrun Err
> -#define FTDI_PE (1<<2) // Parity Err
> -#define FTDI_FE (1<<3) // Framing Err
> -#define FTDI_BI (1<<4) // Break Interrupt
> -#define FTDI_THRE (1<<5) // Transmitter Holding Register
> -#define FTDI_TEMT (1<<6) // Transmitter Empty
> -#define FTDI_FIFO (1<<7) // Error in FIFO
> +#define FTDI_DR (1 << 0) /* Data Ready */
> +#define FTDI_OE (1 << 1) /* Overrun Err */
> +#define FTDI_PE (1 << 2) /* Parity Err */
> +#define FTDI_FE (1 << 3) /* Framing Err */
> +#define FTDI_BI (1 << 4) /* Break Interrupt */
> +#define FTDI_THRE (1 << 5) /* Transmitter Holding Register */
> +#define FTDI_TEMT (1 << 6) /* Transmitter Empty */
> +#define FTDI_FIFO (1 << 7) /* Error in FIFO */
>
> struct USBSerialState {
> USBDevice dev;
> +
> USBEndpoint *intr;
> uint8_t recv_buf[RECV_BUF];
> uint16_t recv_ptr;
> @@ -216,29 +219,34 @@ static uint8_t usb_get_modem_lines(USBSerialState *s)
>
> if (qemu_chr_fe_ioctl(&s->cs,
> CHR_IOCTL_SERIAL_GET_TIOCM, &flags) == -ENOTSUP) {
> - return FTDI_CTS|FTDI_DSR|FTDI_RLSD;
> + return FTDI_CTS | FTDI_DSR | FTDI_RLSD;
> }
>
> ret = 0;
> - if (flags & CHR_TIOCM_CTS)
> + if (flags & CHR_TIOCM_CTS) {
> ret |= FTDI_CTS;
> - if (flags & CHR_TIOCM_DSR)
> + }
> + if (flags & CHR_TIOCM_DSR) {
> ret |= FTDI_DSR;
> - if (flags & CHR_TIOCM_RI)
> + }
> + if (flags & CHR_TIOCM_RI) {
> ret |= FTDI_RI;
> - if (flags & CHR_TIOCM_CAR)
> + }
> + if (flags & CHR_TIOCM_CAR) {
> ret |= FTDI_RLSD;
> + }
>
> return ret;
> }
>
> static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> - int request, int value, int index, int length, uint8_t *data)
> + int request, int value, int index,
> + int length, uint8_t *data)
> {
> USBSerialState *s = (USBSerialState *)dev;
> int ret;
>
> - DPRINTF("got control %x, value %x\n",request, value);
> + DPRINTF("got control %x, value %x\n", request, value);
> ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
> if (ret >= 0) {
> return;
> @@ -248,7 +256,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> case EndpointOutRequest | USB_REQ_CLEAR_FEATURE:
> break;
>
> - /* Class specific requests. */
> + /* Class specific requests. */
> case DeviceOutVendor | FTDI_RESET:
> switch (value) {
> case FTDI_RESET_SIO:
> @@ -269,16 +277,18 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> static int flags;
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_GET_TIOCM, &flags);
> if (value & FTDI_SET_RTS) {
> - if (value & FTDI_RTS)
> + if (value & FTDI_RTS) {
> flags |= CHR_TIOCM_RTS;
> - else
> + } else {
> flags &= ~CHR_TIOCM_RTS;
> + }
> }
> if (value & FTDI_SET_DTR) {
> - if (value & FTDI_DTR)
> + if (value & FTDI_DTR) {
> flags |= CHR_TIOCM_DTR;
> - else
> + } else {
> flags &= ~CHR_TIOCM_DTR;
> + }
> }
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
> break;
> @@ -293,10 +303,12 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> int divisor = value & 0x3fff;
>
> /* chip special cases */
> - if (divisor == 1 && subdivisor8 == 0)
> + if (divisor == 1 && subdivisor8 == 0) {
> subdivisor8 = 4;
> - if (divisor == 0 && subdivisor8 == 0)
> + }
> + if (divisor == 0 && subdivisor8 == 0) {
> divisor = 1;
> + }
>
> s->params.speed = (48000000 / 2) / (8 * divisor + subdivisor8);
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
> @@ -304,30 +316,32 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> }
> case DeviceOutVendor | FTDI_SET_DATA:
> switch (value & FTDI_PARITY) {
> - case 0:
> - s->params.parity = 'N';
> - break;
> - case FTDI_ODD:
> - s->params.parity = 'O';
> - break;
> - case FTDI_EVEN:
> - s->params.parity = 'E';
> - break;
> - default:
> - DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
> - goto fail;
> + case 0:
> + s->params.parity = 'N';
> + break;
> + case FTDI_ODD:
> + s->params.parity = 'O';
> + break;
> + case FTDI_EVEN:
> + s->params.parity = 'E';
> + break;
> + default:
> + DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
> + goto fail;
> }
> +
> switch (value & FTDI_STOP) {
> - case FTDI_STOP1:
> - s->params.stop_bits = 1;
> - break;
> - case FTDI_STOP2:
> - s->params.stop_bits = 2;
> - break;
> - default:
> - DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
> - goto fail;
> + case FTDI_STOP1:
> + s->params.stop_bits = 1;
> + break;
> + case FTDI_STOP2:
> + s->params.stop_bits = 2;
> + break;
> + default:
> + DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
> + goto fail;
> }
> +
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
> /* TODO: TX ON/OFF */
> break;
> @@ -423,20 +437,24 @@ static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
>
> switch (p->pid) {
> case USB_TOKEN_OUT:
> - if (devep != 2)
> + if (devep != 2) {
> goto fail;
> + }
> for (i = 0; i < p->iov.niov; i++) {
> iov = p->iov.iov + i;
> - /* XXX this blocks entire thread. Rewrite to use
> - * qemu_chr_fe_write and background I/O callbacks */
> + /*
> + * XXX this blocks entire thread. Rewrite to use
> + * qemu_chr_fe_write and background I/O callbacks
> + */
> qemu_chr_fe_write_all(&s->cs, iov->iov_base, iov->iov_len);
> }
> p->actual_length = p->iov.size;
> break;
>
> case USB_TOKEN_IN:
> - if (devep != 1)
> + if (devep != 1) {
> goto fail;
> + }
> usb_serial_token_in(s, p);
> break;
>
> @@ -464,21 +482,24 @@ static void usb_serial_read(void *opaque, const uint8_t *buf, int size)
> int first_size, start;
>
> /* room in the buffer? */
> - if (size > (RECV_BUF - s->recv_used))
> + if (size > (RECV_BUF - s->recv_used)) {
> size = RECV_BUF - s->recv_used;
> + }
>
> start = s->recv_ptr + s->recv_used;
> if (start < RECV_BUF) {
> /* copy data to end of buffer */
> first_size = RECV_BUF - start;
> - if (first_size > size)
> + if (first_size > size) {
> first_size = size;
> + }
>
> memcpy(s->recv_buf + start, buf, first_size);
>
> /* wrap around to front if needed */
> - if (size > first_size)
> + if (size > first_size) {
> memcpy(s->recv_buf, buf + first_size, size - first_size);
> + }
> } else {
> start -= RECV_BUF;
> memcpy(s->recv_buf + start, buf, size);
> @@ -493,23 +514,23 @@ static void usb_serial_event(void *opaque, QEMUChrEvent event)
> USBSerialState *s = opaque;
>
> switch (event) {
> - case CHR_EVENT_BREAK:
> - s->event_trigger |= FTDI_BI;
> - break;
> - case CHR_EVENT_OPENED:
> - if (!s->dev.attached) {
> - usb_device_attach(&s->dev, &error_abort);
> - }
> - break;
> - case CHR_EVENT_CLOSED:
> - if (s->dev.attached) {
> - usb_device_detach(&s->dev);
> - }
> - break;
> - case CHR_EVENT_MUX_IN:
> - case CHR_EVENT_MUX_OUT:
> - /* Ignore */
> - break;
> + case CHR_EVENT_BREAK:
> + s->event_trigger |= FTDI_BI;
> + break;
> + case CHR_EVENT_OPENED:
> + if (!s->dev.attached) {
> + usb_device_attach(&s->dev, &error_abort);
> + }
> + break;
> + case CHR_EVENT_CLOSED:
> + if (s->dev.attached) {
> + usb_device_detach(&s->dev);
> + }
> + break;
> + case CHR_EVENT_MUX_IN:
> + case CHR_EVENT_MUX_OUT:
> + /* Ignore */
> + break;
> }
> }
>
> @@ -549,8 +570,9 @@ static USBDevice *usb_braille_init(const char *unused)
> Chardev *cdrv;
>
> cdrv = qemu_chr_new("braille", "braille", NULL);
> - if (!cdrv)
> + if (!cdrv) {
> return NULL;
> + }
>
> dev = usb_new("usb-braille");
> qdev_prop_set_chr(&dev->qdev, "chardev", cdrv);
> --
> 2.20.1
>
--
Samuel
<N> M. MIMRAM Samuel Antonin
<N> en voila un qui etait predestiné
-+- #ens-mim - Quelles gueules qu'ils ont les ptits nouveaux ? -+-
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments
2020-10-26 8:33 ` [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments Mark Cave-Ayland
@ 2020-10-26 9:37 ` Samuel Thibault
2020-10-27 9:04 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:37 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Mark Cave-Ayland, le lun. 26 oct. 2020 08:33:54 +0000, a ecrit:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> hw/usb/dev-serial.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 7a5fa3770e..77ce89d38b 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -204,7 +204,7 @@ static void usb_serial_reset(USBSerialState *s)
>
> static void usb_serial_handle_reset(USBDevice *dev)
> {
> - USBSerialState *s = (USBSerialState *)dev;
> + USBSerialState *s = USB_SERIAL(dev);
>
> DPRINTF("Reset\n");
>
> @@ -243,7 +243,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> int request, int value, int index,
> int length, uint8_t *data)
> {
> - USBSerialState *s = (USBSerialState *)dev;
> + USBSerialState *s = USB_SERIAL(dev);
> int ret;
>
> DPRINTF("got control %x, value %x\n", request, value);
> @@ -430,7 +430,7 @@ static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
>
> static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
> {
> - USBSerialState *s = (USBSerialState *)dev;
> + USBSerialState *s = USB_SERIAL(dev);
> uint8_t devep = p->ep->nr;
> struct iovec *iov;
> int i;
> --
> 2.20.1
>
--
Samuel
<h> t: bah c'est tendre le pattern pour se faire matcher, hein
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events
2020-10-26 8:33 ` [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events Mark Cave-Ayland
@ 2020-10-26 9:38 ` Samuel Thibault
2020-10-27 9:08 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:38 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Mark Cave-Ayland, le lun. 26 oct. 2020 08:33:55 +0000, a ecrit:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> hw/usb/dev-serial.c | 28 ++++++++++++++--------------
> hw/usb/trace-events | 8 ++++++++
> 2 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 77ce89d38b..abc316c7bf 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -20,15 +20,8 @@
> #include "chardev/char-serial.h"
> #include "chardev/char-fe.h"
> #include "qom/object.h"
> +#include "trace.h"
>
> -//#define DEBUG_Serial
> -
> -#ifdef DEBUG_Serial
> -#define DPRINTF(fmt, ...) \
> -do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) do {} while(0)
> -#endif
>
> #define RECV_BUF (512 - (2 * 8))
>
> @@ -205,8 +198,9 @@ static void usb_serial_reset(USBSerialState *s)
> static void usb_serial_handle_reset(USBDevice *dev)
> {
> USBSerialState *s = USB_SERIAL(dev);
> + USBBus *bus = usb_bus_from_device(dev);
>
> - DPRINTF("Reset\n");
> + trace_usb_serial_reset(bus->busnr, dev->addr);
>
> usb_serial_reset(s);
> /* TODO: Reset char device, send BREAK? */
> @@ -244,9 +238,11 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> int length, uint8_t *data)
> {
> USBSerialState *s = USB_SERIAL(dev);
> + USBBus *bus = usb_bus_from_device(dev);
> int ret;
>
> - DPRINTF("got control %x, value %x\n", request, value);
> + trace_usb_serial_handle_control(bus->busnr, dev->addr, request, value);
> +
> ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
> if (ret >= 0) {
> return;
> @@ -326,7 +322,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> s->params.parity = 'E';
> break;
> default:
> - DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
> + trace_usb_serial_unsupported_parity(bus->busnr, dev->addr,
> + value & FTDI_PARITY);
> goto fail;
> }
>
> @@ -338,7 +335,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> s->params.stop_bits = 2;
> break;
> default:
> - DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
> + trace_usb_serial_unsupported_stopbits(bus->busnr, dev->addr,
> + value & FTDI_STOP);
> goto fail;
> }
>
> @@ -367,7 +365,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> break;
> default:
> fail:
> - DPRINTF("got unsupported/bogus control %x, value %x\n", request, value);
> + trace_usb_serial_unsupported_control(bus->busnr, dev->addr, request,
> + value);
> p->status = USB_RET_STALL;
> break;
> }
> @@ -431,6 +430,7 @@ static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
> static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
> {
> USBSerialState *s = USB_SERIAL(dev);
> + USBBus *bus = usb_bus_from_device(dev);
> uint8_t devep = p->ep->nr;
> struct iovec *iov;
> int i;
> @@ -459,7 +459,7 @@ static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
> break;
>
> default:
> - DPRINTF("Bad token\n");
> + trace_usb_serial_bad_token(bus->busnr, dev->addr);
> fail:
> p->status = USB_RET_STALL;
> break;
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index 72e4298780..e5871cbbbc 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -320,3 +320,11 @@ usb_host_parse_interface(int bus, int addr, int num, int alt, int active) "dev %
> usb_host_parse_endpoint(int bus, int addr, int ep, const char *dir, const char *type, int active) "dev %d:%d, ep %d, %s, %s, active %d"
> usb_host_parse_error(int bus, int addr, const char *errmsg) "dev %d:%d, msg %s"
> usb_host_remote_wakeup_removed(int bus, int addr) "dev %d:%d"
> +
> +# dev-serial.c
> +usb_serial_reset(int bus, int addr) "dev %d:%d reset"
> +usb_serial_handle_control(int bus, int addr, int request, int value) "dev %d:%d got control 0x%x, value 0x%x"
> +usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d unsupported parity %d"
> +usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d unsupported stop bits %d"
> +usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
> +usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
> --
> 2.20.1
>
--
Samuel
The nice thing about Windows is - It does not just crash, it displays a
dialog box and lets you press 'OK' first.
(Arno Schaefer's .sig)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters
2020-10-26 8:33 ` [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters Mark Cave-Ayland
@ 2020-10-26 9:40 ` Samuel Thibault
2020-10-27 9:08 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:40 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Mark Cave-Ayland, le lun. 26 oct. 2020 08:33:56 +0000, a ecrit:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> hw/usb/dev-serial.c | 3 +++
> hw/usb/trace-events | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index abc316c7bf..badf8785db 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -307,6 +307,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> }
>
> s->params.speed = (48000000 / 2) / (8 * divisor + subdivisor8);
> + trace_usb_serial_set_baud(bus->busnr, dev->addr, s->params.speed);
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
> break;
> }
> @@ -340,6 +341,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> goto fail;
> }
>
> + trace_usb_serial_set_data(bus->busnr, dev->addr, s->params.parity,
> + s->params.data_bits, s->params.stop_bits);
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
> /* TODO: TX ON/OFF */
> break;
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index e5871cbbbc..9e984b2e0c 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -328,3 +328,5 @@ usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d unsupport
> usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d unsupported stop bits %d"
> usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
> usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
> +usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%d baud rate %d"
> +usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev %d:%d parity %c, data bits %d, stop bits %d"
> --
> 2.20.1
>
--
Samuel
J'ai un gros problème: j'ai cet exercice à rendre demain lundi, mais ma
TI 89 ne sait pas le faire...
Est-ce que quelqu'un pourrait m'aider??
-+- OD In Guide du Neuneu Usenet : Comment ça ! Il faut réfléchir ?-+-
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h
2020-10-26 8:33 ` [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h Mark Cave-Ayland
@ 2020-10-26 9:41 ` Samuel Thibault
2020-10-27 9:09 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:41 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Mark Cave-Ayland, le lun. 26 oct. 2020 08:33:57 +0000, a ecrit:
> The DeviceOutVendor and DeviceInVendor macros can be replaced with their
> equivalent VendorDeviceOutRequest and VendorDeviceRequest macros from usb.h.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> hw/usb/dev-serial.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index badf8785db..92c35615eb 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -37,11 +37,6 @@
> #define FTDI_SET_LATENCY 9
> #define FTDI_GET_LATENCY 10
>
> -#define DeviceOutVendor \
> - ((USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
> -#define DeviceInVendor \
> - ((USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
> -
> /* RESET */
>
> #define FTDI_RESET_SIO 0
> @@ -253,7 +248,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> break;
>
> /* Class specific requests. */
> - case DeviceOutVendor | FTDI_RESET:
> + case VendorDeviceOutRequest | FTDI_RESET:
> switch (value) {
> case FTDI_RESET_SIO:
> usb_serial_reset(s);
> @@ -268,7 +263,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> break;
> }
> break;
> - case DeviceOutVendor | FTDI_SET_MDM_CTRL:
> + case VendorDeviceOutRequest | FTDI_SET_MDM_CTRL:
> {
> static int flags;
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_GET_TIOCM, &flags);
> @@ -289,10 +284,10 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
> break;
> }
> - case DeviceOutVendor | FTDI_SET_FLOW_CTRL:
> + case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL:
> /* TODO: ioctl */
> break;
> - case DeviceOutVendor | FTDI_SET_BAUD: {
> + case VendorDeviceOutRequest | FTDI_SET_BAUD: {
> static const int subdivisors8[8] = { 0, 4, 2, 1, 3, 5, 6, 7 };
> int subdivisor8 = subdivisors8[((value & 0xc000) >> 14)
> | ((index & 1) << 2)];
> @@ -311,7 +306,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
> break;
> }
> - case DeviceOutVendor | FTDI_SET_DATA:
> + case VendorDeviceOutRequest | FTDI_SET_DATA:
> switch (value & FTDI_PARITY) {
> case 0:
> s->params.parity = 'N';
> @@ -346,23 +341,23 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
> /* TODO: TX ON/OFF */
> break;
> - case DeviceInVendor | FTDI_GET_MDM_ST:
> + case VendorDeviceRequest | FTDI_GET_MDM_ST:
> data[0] = usb_get_modem_lines(s) | 1;
> data[1] = FTDI_THRE | FTDI_TEMT;
> p->actual_length = 2;
> break;
> - case DeviceOutVendor | FTDI_SET_EVENT_CHR:
> + case VendorDeviceOutRequest | FTDI_SET_EVENT_CHR:
> /* TODO: handle it */
> s->event_chr = value;
> break;
> - case DeviceOutVendor | FTDI_SET_ERROR_CHR:
> + case VendorDeviceOutRequest | FTDI_SET_ERROR_CHR:
> /* TODO: handle it */
> s->error_chr = value;
> break;
> - case DeviceOutVendor | FTDI_SET_LATENCY:
> + case VendorDeviceOutRequest | FTDI_SET_LATENCY:
> s->latency = value;
> break;
> - case DeviceInVendor | FTDI_GET_LATENCY:
> + case VendorDeviceRequest | FTDI_GET_LATENCY:
> data[0] = s->latency;
> p->actual_length = 1;
> break;
> --
> 2.20.1
>
--
Samuel
How do I type "for i in *.dvi do xdvi i done" in a GUI?
(Discussion in comp.os.linux.misc on the intuitiveness of interfaces.)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached
2020-10-26 8:33 ` [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached Mark Cave-Ayland
@ 2020-10-26 9:45 ` Samuel Thibault
2020-10-27 8:09 ` Gerd Hoffmann
1 sibling, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:45 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Mark Cave-Ayland, le lun. 26 oct. 2020 08:33:58 +0000, a ecrit:
> Some operating systems will generate a new device ID when a USB device is unplugged
> and then replugged into the USB. If this is done whilst switching between multiple
> applications over a virtual serial port, the change of device ID requires going
> back into the OS/application to locate the new device accordingly.
>
> Add a new always-plugged property that if specified will ensure that the device
> always remains attached to the USB regardless of the state of the backend
> chardev.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> hw/usb/dev-serial.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 92c35615eb..919e25e1d9 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -97,6 +97,7 @@ struct USBSerialState {
> uint8_t event_chr;
> uint8_t error_chr;
> uint8_t event_trigger;
> + bool always_plugged;
> QEMUSerialSetParams params;
> int latency; /* ms */
> CharBackend cs;
> @@ -516,12 +517,12 @@ static void usb_serial_event(void *opaque, QEMUChrEvent event)
> s->event_trigger |= FTDI_BI;
> break;
> case CHR_EVENT_OPENED:
> - if (!s->dev.attached) {
> + if (!s->always_plugged && !s->dev.attached) {
> usb_device_attach(&s->dev, &error_abort);
> }
> break;
> case CHR_EVENT_CLOSED:
> - if (s->dev.attached) {
> + if (!s->always_plugged && s->dev.attached) {
> usb_device_detach(&s->dev);
> }
> break;
> @@ -556,7 +557,8 @@ static void usb_serial_realize(USBDevice *dev, Error **errp)
> usb_serial_event, NULL, s, NULL, true);
> usb_serial_handle_reset(dev);
>
> - if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
> + if (s->always_plugged || (qemu_chr_fe_backend_open(&s->cs) &&
> + !dev->attached)) {
> usb_device_attach(dev, &error_abort);
> }
> s->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
> @@ -584,6 +586,7 @@ static const VMStateDescription vmstate_usb_serial = {
>
> static Property serial_properties[] = {
> DEFINE_PROP_CHR("chardev", USBSerialState, cs),
> + DEFINE_PROP_BOOL("always-plugged", USBSerialState, always_plugged, false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> --
> 2.20.1
>
--
Samuel
"...[Linux's] capacity to talk via any medium except smoke signals."
(By Dr. Greg Wettstein, Roger Maris Cancer Center)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 7/9] dev-serial: add support for setting data_bits in QEMUSerialSetParams
2020-10-26 8:33 ` [PATCH 7/9] dev-serial: add support for setting data_bits in QEMUSerialSetParams Mark Cave-Ayland
@ 2020-10-26 9:46 ` Samuel Thibault
0 siblings, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:46 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Mark Cave-Ayland, le lun. 26 oct. 2020 08:33:59 +0000, a ecrit:
> Also implement the behaviour reported in Linux's ftdi_sio.c whereby if an invalid
> data_bits value is provided then the hardware defaults to using 8.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> hw/usb/dev-serial.c | 17 +++++++++++++++++
> hw/usb/trace-events | 1 +
> 2 files changed, 18 insertions(+)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 919e25e1d9..4c374d0790 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -308,6 +308,23 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> break;
> }
> case VendorDeviceOutRequest | FTDI_SET_DATA:
> + switch (value & 0xff) {
> + case 7:
> + s->params.data_bits = 7;
> + break;
> + case 8:
> + s->params.data_bits = 8;
> + break;
> + default:
> + /*
> + * According to a comment in Linux's ftdi_sio.c original FTDI
> + * chips fall back to 8 data bits for unsupported data_bits
> + */
> + trace_usb_serial_unsupported_data_bits(bus->busnr, dev->addr,
> + value & 0xff);
> + s->params.data_bits = 8;
> + }
> +
> switch (value & FTDI_PARITY) {
> case 0:
> s->params.parity = 'N';
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index 9e984b2e0c..0d0a3e5f2a 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -327,6 +327,7 @@ usb_serial_handle_control(int bus, int addr, int request, int value) "dev %d:%d
> usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d unsupported parity %d"
> usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d unsupported stop bits %d"
> usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
> +usb_serial_unsupported_data_bits(int bus, int addr, int value) "dev %d:%d unsupported data bits %d, falling back to 8"
> usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
> usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%d baud rate %d"
> usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev %d:%d parity %c, data bits %d, stop bits %d"
> --
> 2.20.1
>
--
Samuel
> Il [e2fsck] a bien démarré, mais il m'a rendu la main aussitot en me
> disant "houlala, c'est pas beau à voir votre truc, je préfèrerai que
> vous teniez vous même la tronçonneuse" (traduction libre)
NC in Guide du linuxien pervers : "Bien configurer sa tronçonneuse."
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 8:34 ` [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response Mark Cave-Ayland
@ 2020-10-26 9:54 ` Samuel Thibault
2020-10-26 10:58 ` Mark Cave-Ayland
0 siblings, 1 reply; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:54 UTC (permalink / raw)
To: Mark Cave-Ayland, Aurelien Jarno, Jason Andryuk; +Cc: qemu-devel, kraxel
Hello,
(Cc-ing Aurelien who introduced the support for modem control, and Jason
who added the missing THRE and TEMT flags).
Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit:
> The FTDI_GET_MDM_ST response should only return a single byte indicating the
> modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header
> file).
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/usb/dev-serial.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 4c374d0790..fa734bcf54 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> /* TODO: TX ON/OFF */
> break;
> case VendorDeviceRequest | FTDI_GET_MDM_ST:
> - data[0] = usb_get_modem_lines(s) | 1;
> - data[1] = FTDI_THRE | FTDI_TEMT;
> - p->actual_length = 2;
> + data[0] = usb_get_modem_lines(s);
> + p->actual_length = 1;
Err, but Linux' drivers/usb/serial/ftdi_sio.c:ftdi_process_packet()
contradicts this:
if (len < 2) {
dev_dbg(&port->dev, "malformed packet\n");
return 0;
}
status = buf[0] & FTDI_STATUS_B0_MASK;
if (status != priv->prev_status) {
char diff_status = status ^ priv->prev_status;
if (diff_status & FTDI_RS0_CTS)
port->icount.cts++;
[...]
/* save if the transmitter is empty or not */
if (buf[1] & FTDI_RS_TEMT)
priv->transmit_empty = 1;
else
priv->transmit_empty = 0;
Did you actually get an issue with seeing this packet contain two bytes?
Samuel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] dev-serial: store flow control and xon/xoff characters
2020-10-26 8:34 ` [PATCH 9/9] dev-serial: store flow control and xon/xoff characters Mark Cave-Ayland
@ 2020-10-26 9:58 ` Samuel Thibault
0 siblings, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:58 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:01 +0000, a ecrit:
> Note that whilst the device does not do anything with these values, they are
> logged with trace events and stored to allow future implementation.
>
> The default flow control is set to none at reset as documented in the Linux
> ftdi_sio.h header file.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> hw/usb/dev-serial.c | 38 +++++++++++++++++++++++++++++++++++---
> hw/usb/trace-events | 2 ++
> 2 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index fa734bcf54..3dbc56d77a 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -52,6 +52,7 @@
>
> /* SET_FLOW_CTRL */
>
> +#define FTDI_NO_HS 0
> #define FTDI_RTS_CTS_HS 1
> #define FTDI_DTR_DSR_HS 2
> #define FTDI_XON_XOFF_HS 4
> @@ -98,6 +99,9 @@ struct USBSerialState {
> uint8_t error_chr;
> uint8_t event_trigger;
> bool always_plugged;
> + uint8_t flow_control;
> + uint8_t xon;
> + uint8_t xoff;
> QEMUSerialSetParams params;
> int latency; /* ms */
> CharBackend cs;
> @@ -181,14 +185,36 @@ static const USBDesc desc_braille = {
> .str = desc_strings,
> };
>
> +static void usb_serial_set_flow_control(USBSerialState *s,
> + uint8_t flow_control)
> +{
> + USBDevice *dev = USB_DEVICE(s);
> + USBBus *bus = usb_bus_from_device(dev);
> +
> + /* TODO: ioctl */
> + s->flow_control = flow_control;
> + trace_usb_serial_set_flow_control(bus->busnr, dev->addr, flow_control);
> +}
> +
> +static void usb_serial_set_xonxoff(USBSerialState *s, int xonxoff)
> +{
> + USBDevice *dev = USB_DEVICE(s);
> + USBBus *bus = usb_bus_from_device(dev);
> +
> + s->xon = xonxoff & 0xff;
> + s->xoff = (xonxoff >> 8) & 0xff;
> +
> + trace_usb_serial_set_xonxoff(bus->busnr, dev->addr, s->xon, s->xoff);
> +}
> +
> static void usb_serial_reset(USBSerialState *s)
> {
> - /* TODO: Set flow control to none */
> s->event_chr = 0x0d;
> s->event_trigger = 0;
> s->recv_ptr = 0;
> s->recv_used = 0;
> /* TODO: purge in char driver */
> + usb_serial_set_flow_control(s, FTDI_NO_HS);
> }
>
> static void usb_serial_handle_reset(USBDevice *dev)
> @@ -285,9 +311,15 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
> break;
> }
> - case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL:
> - /* TODO: ioctl */
> + case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL: {
> + uint8_t flow_control = index >> 8;
> +
> + usb_serial_set_flow_control(s, flow_control);
> + if (flow_control & FTDI_XON_XOFF_HS) {
> + usb_serial_set_xonxoff(s, value);
> + }
> break;
> + }
> case VendorDeviceOutRequest | FTDI_SET_BAUD: {
> static const int subdivisors8[8] = { 0, 4, 2, 1, 3, 5, 6, 7 };
> int subdivisor8 = subdivisors8[((value & 0xc000) >> 14)
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index 0d0a3e5f2a..725b7077ad 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -331,3 +331,5 @@ usb_serial_unsupported_data_bits(int bus, int addr, int value) "dev %d:%d unsupp
> usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
> usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%d baud rate %d"
> usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev %d:%d parity %c, data bits %d, stop bits %d"
> +usb_serial_set_flow_control(int bus, int addr, int index) "dev %d:%d flow control %d"
> +usb_serial_set_xonxoff(int bus, int addr, uint8_t xon, uint8_t xoff) "dev %d:%d xon 0x%x xoff 0x%x"
> --
> 2.20.1
>
--
Samuel
> dvips -o $@ $<
Faut faire gffe de pas te couper avec ton truc, t'as mis des ciseaux ($<)
partout :))
-+- Dom in Guide du linuxien pervers - "J'aime pas les Makefile !" -+-
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/9] dev-serial: minor fixes and improvements
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
` (8 preceding siblings ...)
2020-10-26 8:34 ` [PATCH 9/9] dev-serial: store flow control and xon/xoff characters Mark Cave-Ayland
@ 2020-10-26 9:59 ` Samuel Thibault
9 siblings, 0 replies; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 9:59 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, kraxel
Hello,
Mark Cave-Ayland, le lun. 26 oct. 2020 08:33:52 +0000, a ecrit:
> This series comes from a client project that I have been working on over the
> past few months which involves communicating with serial hardware and
> associated simulators using the QEMU USB serial device.
Thanks for these!
I only had concerned with the MDM_ST change, see the corresponding mail.
The other patches can be applied independently from that change.
Samuel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 9:54 ` Samuel Thibault
@ 2020-10-26 10:58 ` Mark Cave-Ayland
2020-10-26 11:14 ` Samuel Thibault
0 siblings, 1 reply; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 10:58 UTC (permalink / raw)
To: Samuel Thibault, Aurelien Jarno, Jason Andryuk; +Cc: qemu-devel, kraxel
On 26/10/2020 09:54, Samuel Thibault wrote:
> Hello,
>
> (Cc-ing Aurelien who introduced the support for modem control, and Jason
> who added the missing THRE and TEMT flags).
>
> Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit:
>> The FTDI_GET_MDM_ST response should only return a single byte indicating the
>> modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header
>> file).
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/usb/dev-serial.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
>> index 4c374d0790..fa734bcf54 100644
>> --- a/hw/usb/dev-serial.c
>> +++ b/hw/usb/dev-serial.c
>> @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
>> /* TODO: TX ON/OFF */
>> break;
>> case VendorDeviceRequest | FTDI_GET_MDM_ST:
>> - data[0] = usb_get_modem_lines(s) | 1;
>> - data[1] = FTDI_THRE | FTDI_TEMT;
>> - p->actual_length = 2;
>> + data[0] = usb_get_modem_lines(s);
>> + p->actual_length = 1;
>
> Err, but Linux' drivers/usb/serial/ftdi_sio.c:ftdi_process_packet()
> contradicts this:
>
> if (len < 2) {
> dev_dbg(&port->dev, "malformed packet\n");
> return 0;
> }
>
> status = buf[0] & FTDI_STATUS_B0_MASK;
> if (status != priv->prev_status) {
> char diff_status = status ^ priv->prev_status;
>
> if (diff_status & FTDI_RS0_CTS)
> port->icount.cts++;
>
> [...]
>
> /* save if the transmitter is empty or not */
> if (buf[1] & FTDI_RS_TEMT)
> priv->transmit_empty = 1;
> else
> priv->transmit_empty = 0;
>
> Did you actually get an issue with seeing this packet contain two bytes?
Hi Samuel,
Thanks for the review! There are 2 different places where the status is read: the one
above in ftdi_process_packet() relates to the control packet header for incoming data
which is always 2 bytes, whereas this relates to FTDI_SIO_GET_MODEM_STATUS_REQUEST in
https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.c#L2825.
I have a FTDI Chipi-X adapter to compare with and that returns 2 bytes, but it looks
like I mis-read the code and it's the SIO chipsets that return 1 byte which are older
than the chipset being emulated by QEMU. This came from reading
https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L415
which states that only 1 byte should be returned, but of course that comment could be
out of date.
A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in response to
FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length should be 2 bytes, the
comment about B0-B3 being zero and the response from my Chip-X above suggests that
the "| 1" should still be dropped from the response.
ATB,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 10:58 ` Mark Cave-Ayland
@ 2020-10-26 11:14 ` Samuel Thibault
2020-10-26 13:00 ` Jason Andryuk
0 siblings, 1 reply; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 11:14 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: kraxel, qemu-devel, Aurelien Jarno, Jason Andryuk
Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +0000, a ecrit:
> On 26/10/2020 09:54, Samuel Thibault wrote:
> > Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit:
> > > The FTDI_GET_MDM_ST response should only return a single byte indicating the
> > > modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header
> > > file).
> > >
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > ---
> > > hw/usb/dev-serial.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> > > index 4c374d0790..fa734bcf54 100644
> > > --- a/hw/usb/dev-serial.c
> > > +++ b/hw/usb/dev-serial.c
> > > @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> > > /* TODO: TX ON/OFF */
> > > break;
> > > case VendorDeviceRequest | FTDI_GET_MDM_ST:
> > > - data[0] = usb_get_modem_lines(s) | 1;
> > > - data[1] = FTDI_THRE | FTDI_TEMT;
> > > - p->actual_length = 2;
> > > + data[0] = usb_get_modem_lines(s);
> > > + p->actual_length = 1;
> >
[...]
> A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in
> response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length
> should be 2 bytes, the comment about B0-B3 being zero and the response from
> my Chip-X above suggests that the "| 1" should still be dropped from the
> response.
Aurelien, you introduced the "| 1" in
commit abb8a13918ecc1e8160aa78582de9d5224ea70df
Author: Aurelien Jarno <aurelien@aurel32.net>
Date: Wed Aug 13 04:23:17 2008 +0000
usb-serial: add support for modem lines
[...]
@@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int request, int value,
/* TODO: TX ON/OFF */
break;
case DeviceInVendor | FTDI_GET_MDM_ST:
- /* TODO: return modem status */
- data[0] = 0;
- ret = 1;
+ data[0] = usb_get_modem_lines(s) | 1;
+ data[1] = 0;
+ ret = 2;
break;
do you know exactly what it is for?
Samuel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 11:14 ` Samuel Thibault
@ 2020-10-26 13:00 ` Jason Andryuk
2020-10-26 13:40 ` Mark Cave-Ayland
0 siblings, 1 reply; 34+ messages in thread
From: Jason Andryuk @ 2020-10-26 13:00 UTC (permalink / raw)
To: Samuel Thibault; +Cc: Mark Cave-Ayland, QEMU, Aurelien Jarno, Gerd Hoffmann
On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
>
> Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +0000, a ecrit:
> > On 26/10/2020 09:54, Samuel Thibault wrote:
> > > Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit:
> > > > The FTDI_GET_MDM_ST response should only return a single byte indicating the
> > > > modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header
> > > > file).
> > > >
> > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > > ---
> > > > hw/usb/dev-serial.c | 5 ++---
> > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> > > > index 4c374d0790..fa734bcf54 100644
> > > > --- a/hw/usb/dev-serial.c
> > > > +++ b/hw/usb/dev-serial.c
> > > > @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> > > > /* TODO: TX ON/OFF */
> > > > break;
> > > > case VendorDeviceRequest | FTDI_GET_MDM_ST:
> > > > - data[0] = usb_get_modem_lines(s) | 1;
> > > > - data[1] = FTDI_THRE | FTDI_TEMT;
> > > > - p->actual_length = 2;
> > > > + data[0] = usb_get_modem_lines(s);
> > > > + p->actual_length = 1;
> > >
> [...]
> > A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in
> > response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length
> > should be 2 bytes, the comment about B0-B3 being zero and the response from
> > my Chip-X above suggests that the "| 1" should still be dropped from the
> > response.
>
> Aurelien, you introduced the "| 1" in
>
> commit abb8a13918ecc1e8160aa78582de9d5224ea70df
> Author: Aurelien Jarno <aurelien@aurel32.net>
> Date: Wed Aug 13 04:23:17 2008 +0000
>
> usb-serial: add support for modem lines
>
> [...]
> @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int request, int value,
> /* TODO: TX ON/OFF */
> break;
> case DeviceInVendor | FTDI_GET_MDM_ST:
> - /* TODO: return modem status */
> - data[0] = 0;
> - ret = 1;
> + data[0] = usb_get_modem_lines(s) | 1;
> + data[1] = 0;
> + ret = 2;
> break;
>
> do you know exactly what it is for?
Hi,
I'm not particularly familiar with the FTDI USB serial devices. I
found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.
A little searching found this:
https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541
That shows "B0 Reserved - must be 1", so maybe that is why "| 1" was added?
Regards,
Jason
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 13:00 ` Jason Andryuk
@ 2020-10-26 13:40 ` Mark Cave-Ayland
2020-10-26 14:04 ` Jason Andryuk
2020-10-26 15:04 ` Samuel Thibault
0 siblings, 2 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-26 13:40 UTC (permalink / raw)
To: Jason Andryuk, Samuel Thibault; +Cc: QEMU, Aurelien Jarno, Gerd Hoffmann
On 26/10/2020 13:00, Jason Andryuk wrote:
> On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
>>
>> Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +0000, a ecrit:
>>> On 26/10/2020 09:54, Samuel Thibault wrote:
>>>> Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit:
>>>>> The FTDI_GET_MDM_ST response should only return a single byte indicating the
>>>>> modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header
>>>>> file).
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>> hw/usb/dev-serial.c | 5 ++---
>>>>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
>>>>> index 4c374d0790..fa734bcf54 100644
>>>>> --- a/hw/usb/dev-serial.c
>>>>> +++ b/hw/usb/dev-serial.c
>>>>> @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
>>>>> /* TODO: TX ON/OFF */
>>>>> break;
>>>>> case VendorDeviceRequest | FTDI_GET_MDM_ST:
>>>>> - data[0] = usb_get_modem_lines(s) | 1;
>>>>> - data[1] = FTDI_THRE | FTDI_TEMT;
>>>>> - p->actual_length = 2;
>>>>> + data[0] = usb_get_modem_lines(s);
>>>>> + p->actual_length = 1;
>>>>
>> [...]
>>> A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in
>>> response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length
>>> should be 2 bytes, the comment about B0-B3 being zero and the response from
>>> my Chip-X above suggests that the "| 1" should still be dropped from the
>>> response.
>>
>> Aurelien, you introduced the "| 1" in
>>
>> commit abb8a13918ecc1e8160aa78582de9d5224ea70df
>> Author: Aurelien Jarno <aurelien@aurel32.net>
>> Date: Wed Aug 13 04:23:17 2008 +0000
>>
>> usb-serial: add support for modem lines
>>
>> [...]
>> @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int request, int value,
>> /* TODO: TX ON/OFF */
>> break;
>> case DeviceInVendor | FTDI_GET_MDM_ST:
>> - /* TODO: return modem status */
>> - data[0] = 0;
>> - ret = 1;
>> + data[0] = usb_get_modem_lines(s) | 1;
>> + data[1] = 0;
>> + ret = 2;
>> break;
>>
>> do you know exactly what it is for?
>
> Hi,
>
> I'm not particularly familiar with the FTDI USB serial devices. I
> found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.
>
> A little searching found this:
> https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541
>
> That shows "B0 Reserved - must be 1", so maybe that is why "| 1" was added?
Right - that's for the modem status returned as part of the first 2 status bytes for
incoming data which is slightly different from modem status returned directly from
FTDI_SIO_GET_MODEM_STATUS:
https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L423.
It is the latter which this patch changes and appears to match what I see on my
Chipi-X hardware here.
ATB,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 13:40 ` Mark Cave-Ayland
@ 2020-10-26 14:04 ` Jason Andryuk
2020-10-26 15:04 ` Samuel Thibault
1 sibling, 0 replies; 34+ messages in thread
From: Jason Andryuk @ 2020-10-26 14:04 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: Gerd Hoffmann, Samuel Thibault, Aurelien Jarno, QEMU
On Mon, Oct 26, 2020 at 9:40 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 26/10/2020 13:00, Jason Andryuk wrote:
>
> > On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> >>
> >> Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +0000, a ecrit:
> >>> On 26/10/2020 09:54, Samuel Thibault wrote:
> >>>> Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit:
> >>>>> The FTDI_GET_MDM_ST response should only return a single byte indicating the
> >>>>> modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header
> >>>>> file).
> >>>>>
> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>>>> ---
> >>>>> hw/usb/dev-serial.c | 5 ++---
> >>>>> 1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> >>>>> index 4c374d0790..fa734bcf54 100644
> >>>>> --- a/hw/usb/dev-serial.c
> >>>>> +++ b/hw/usb/dev-serial.c
> >>>>> @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> >>>>> /* TODO: TX ON/OFF */
> >>>>> break;
> >>>>> case VendorDeviceRequest | FTDI_GET_MDM_ST:
> >>>>> - data[0] = usb_get_modem_lines(s) | 1;
> >>>>> - data[1] = FTDI_THRE | FTDI_TEMT;
> >>>>> - p->actual_length = 2;
> >>>>> + data[0] = usb_get_modem_lines(s);
> >>>>> + p->actual_length = 1;
> >>>>
> >> [...]
> >>> A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in
> >>> response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length
> >>> should be 2 bytes, the comment about B0-B3 being zero and the response from
> >>> my Chip-X above suggests that the "| 1" should still be dropped from the
> >>> response.
> >>
> >> Aurelien, you introduced the "| 1" in
> >>
> >> commit abb8a13918ecc1e8160aa78582de9d5224ea70df
> >> Author: Aurelien Jarno <aurelien@aurel32.net>
> >> Date: Wed Aug 13 04:23:17 2008 +0000
> >>
> >> usb-serial: add support for modem lines
> >>
> >> [...]
> >> @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int request, int value,
> >> /* TODO: TX ON/OFF */
> >> break;
> >> case DeviceInVendor | FTDI_GET_MDM_ST:
> >> - /* TODO: return modem status */
> >> - data[0] = 0;
> >> - ret = 1;
> >> + data[0] = usb_get_modem_lines(s) | 1;
> >> + data[1] = 0;
> >> + ret = 2;
> >> break;
> >>
> >> do you know exactly what it is for?
> >
> > Hi,
> >
> > I'm not particularly familiar with the FTDI USB serial devices. I
> > found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.
> >
> > A little searching found this:
> > https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541
> >
> > That shows "B0 Reserved - must be 1", so maybe that is why "| 1" was added?
>
> Right - that's for the modem status returned as part of the first 2 status bytes for
> incoming data which is slightly different from modem status returned directly from
> FTDI_SIO_GET_MODEM_STATUS:
> https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L423.
Okay, sorry for the confusion there.
I thought your "it's the SIO chipsets that return 1 byte which are older
than the chipset being emulated by QEMU" meant you thought your change
to 1 byte was unnecessary. You also posted two bytes (0x1 0x60) from
your hardware.
> It is the latter which this patch changes and appears to match what I see on my
> Chipi-X hardware here.
I don't have my hardware readily available, but I must have been
seeing 2 bytes from FTDI_GET_MDM_ST with data[1] = FTDI_THRE |
FTDI_TEMT; to make the change.
I don't have the USB captures anymore to compare the lowest bit value.
So right now you are just interested in dropping the lowest bit
setting but preserving the 2 bytes modem status?
Regards,
Jason
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 13:40 ` Mark Cave-Ayland
2020-10-26 14:04 ` Jason Andryuk
@ 2020-10-26 15:04 ` Samuel Thibault
2020-10-27 13:18 ` Mark Cave-Ayland
1 sibling, 1 reply; 34+ messages in thread
From: Samuel Thibault @ 2020-10-26 15:04 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: Gerd Hoffmann, QEMU, Aurelien Jarno, Jason Andryuk
Mark Cave-Ayland, le lun. 26 oct. 2020 13:40:05 +0000, a ecrit:
> On 26/10/2020 13:00, Jason Andryuk wrote:
> > On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > > Aurelien, you introduced the "| 1" in
> > >
> > > commit abb8a13918ecc1e8160aa78582de9d5224ea70df
> > > Author: Aurelien Jarno <aurelien@aurel32.net>
> > > Date: Wed Aug 13 04:23:17 2008 +0000
> > >
> > > usb-serial: add support for modem lines
> > >
> > > [...]
> > > @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int request, int value,
> > > /* TODO: TX ON/OFF */
> > > break;
> > > case DeviceInVendor | FTDI_GET_MDM_ST:
> > > - /* TODO: return modem status */
> > > - data[0] = 0;
> > > - ret = 1;
> > > + data[0] = usb_get_modem_lines(s) | 1;
> > > + data[1] = 0;
> > > + ret = 2;
> > > break;
> >
> > I'm not particularly familiar with the FTDI USB serial devices. I
> > found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.
> >
> > A little searching found this:
> > https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541
> >
> > That shows "B0 Reserved - must be 1", so maybe that is why "| 1" was added?
>
> Right - that's for the modem status returned as part of the first 2 status
> bytes for incoming data which is slightly different from modem status
> returned directly from FTDI_SIO_GET_MODEM_STATUS: https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L423.
>
> It is the latter which this patch changes and appears to match what I see on
> my Chipi-X hardware here.
Aurelien, do you remember the reason for the addition of 1 here? It does
look like the confusion between the incoming data bytes and the modem
status bytes.
Samuel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached
2020-10-26 8:33 ` [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached Mark Cave-Ayland
2020-10-26 9:45 ` Samuel Thibault
@ 2020-10-27 8:09 ` Gerd Hoffmann
2020-10-27 13:23 ` Mark Cave-Ayland
1 sibling, 1 reply; 34+ messages in thread
From: Gerd Hoffmann @ 2020-10-27 8:09 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: samuel.thibault, qemu-devel
> case CHR_EVENT_OPENED:
> - if (!s->dev.attached) {
> + if (!s->always_plugged && !s->dev.attached) {
> usb_device_attach(&s->dev, &error_abort);
> }
Not needed (but doesn't hurt either).
> break;
> case CHR_EVENT_CLOSED:
> - if (s->dev.attached) {
> + if (!s->always_plugged && s->dev.attached) {
> usb_device_detach(&s->dev);
> }
Ok.
> - if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
> + if (s->always_plugged || (qemu_chr_fe_backend_open(&s->cs) &&
> + !dev->attached)) {
The dev->attached check should not be skipped, i.e. the logic should be
((always_plugged || open) && !attached).
take care,
Gerd
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments
2020-10-26 8:33 ` [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments Mark Cave-Ayland
2020-10-26 9:37 ` Samuel Thibault
@ 2020-10-27 9:04 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-27 9:04 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, kraxel, samuel.thibault
On 10/26/20 9:33 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/usb/dev-serial.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events
2020-10-26 8:33 ` [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events Mark Cave-Ayland
2020-10-26 9:38 ` Samuel Thibault
@ 2020-10-27 9:08 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-27 9:08 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, kraxel, samuel.thibault
On 10/26/20 9:33 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/usb/dev-serial.c | 28 ++++++++++++++--------------
> hw/usb/trace-events | 8 ++++++++
> 2 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 77ce89d38b..abc316c7bf 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -20,15 +20,8 @@
> #include "chardev/char-serial.h"
> #include "chardev/char-fe.h"
> #include "qom/object.h"
> +#include "trace.h"
>
> -//#define DEBUG_Serial
> -
> -#ifdef DEBUG_Serial
> -#define DPRINTF(fmt, ...) \
> -do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) do {} while(0)
> -#endif
>
> #define RECV_BUF (512 - (2 * 8))
>
> @@ -205,8 +198,9 @@ static void usb_serial_reset(USBSerialState *s)
> static void usb_serial_handle_reset(USBDevice *dev)
> {
> USBSerialState *s = USB_SERIAL(dev);
> + USBBus *bus = usb_bus_from_device(dev);
>
> - DPRINTF("Reset\n");
> + trace_usb_serial_reset(bus->busnr, dev->addr);
>
> usb_serial_reset(s);
> /* TODO: Reset char device, send BREAK? */
> @@ -244,9 +238,11 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> int length, uint8_t *data)
> {
> USBSerialState *s = USB_SERIAL(dev);
> + USBBus *bus = usb_bus_from_device(dev);
> int ret;
>
> - DPRINTF("got control %x, value %x\n", request, value);
> + trace_usb_serial_handle_control(bus->busnr, dev->addr, request, value);
> +
> ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
> if (ret >= 0) {
> return;
> @@ -326,7 +322,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> s->params.parity = 'E';
> break;
> default:
> - DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
> + trace_usb_serial_unsupported_parity(bus->busnr, dev->addr,
> + value & FTDI_PARITY);
> goto fail;
> }
>
> @@ -338,7 +335,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> s->params.stop_bits = 2;
> break;
> default:
> - DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
> + trace_usb_serial_unsupported_stopbits(bus->busnr, dev->addr,
> + value & FTDI_STOP);
> goto fail;
> }
>
> @@ -367,7 +365,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> break;
> default:
> fail:
> - DPRINTF("got unsupported/bogus control %x, value %x\n", request, value);
> + trace_usb_serial_unsupported_control(bus->busnr, dev->addr, request,
> + value);
> p->status = USB_RET_STALL;
> break;
> }
> @@ -431,6 +430,7 @@ static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
> static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
> {
> USBSerialState *s = USB_SERIAL(dev);
> + USBBus *bus = usb_bus_from_device(dev);
> uint8_t devep = p->ep->nr;
> struct iovec *iov;
> int i;
> @@ -459,7 +459,7 @@ static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
> break;
>
> default:
> - DPRINTF("Bad token\n");
> + trace_usb_serial_bad_token(bus->busnr, dev->addr);
> fail:
> p->status = USB_RET_STALL;
> break;
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index 72e4298780..e5871cbbbc 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -320,3 +320,11 @@ usb_host_parse_interface(int bus, int addr, int num, int alt, int active) "dev %
> usb_host_parse_endpoint(int bus, int addr, int ep, const char *dir, const char *type, int active) "dev %d:%d, ep %d, %s, %s, active %d"
> usb_host_parse_error(int bus, int addr, const char *errmsg) "dev %d:%d, msg %s"
> usb_host_remote_wakeup_removed(int bus, int addr) "dev %d:%d"
> +
> +# dev-serial.c
> +usb_serial_reset(int bus, int addr) "dev %d:%d reset"
> +usb_serial_handle_control(int bus, int addr, int request, int value) "dev %d:%d got control 0x%x, value 0x%x"
> +usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d unsupported parity %d"
> +usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d unsupported stop bits %d"
> +usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
> +usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
In all formats, 'addr' is unsigned -> "%u".
Using %u:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters
2020-10-26 8:33 ` [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters Mark Cave-Ayland
2020-10-26 9:40 ` Samuel Thibault
@ 2020-10-27 9:08 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-27 9:08 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, kraxel, samuel.thibault
On 10/26/20 9:33 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/usb/dev-serial.c | 3 +++
> hw/usb/trace-events | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index abc316c7bf..badf8785db 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -307,6 +307,7 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> }
>
> s->params.speed = (48000000 / 2) / (8 * divisor + subdivisor8);
> + trace_usb_serial_set_baud(bus->busnr, dev->addr, s->params.speed);
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
> break;
> }
> @@ -340,6 +341,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> goto fail;
> }
>
> + trace_usb_serial_set_data(bus->busnr, dev->addr, s->params.parity,
> + s->params.data_bits, s->params.stop_bits);
> qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
> /* TODO: TX ON/OFF */
> break;
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index e5871cbbbc..9e984b2e0c 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -328,3 +328,5 @@ usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d unsupport
> usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d unsupported stop bits %d"
> usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
> usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
> +usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%d baud rate %d"
> +usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev %d:%d parity %c, data bits %d, stop bits %d"
>
Using "%u" for addr:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h
2020-10-26 8:33 ` [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h Mark Cave-Ayland
2020-10-26 9:41 ` Samuel Thibault
@ 2020-10-27 9:09 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-27 9:09 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, kraxel, samuel.thibault
On 10/26/20 9:33 AM, Mark Cave-Ayland wrote:
> The DeviceOutVendor and DeviceInVendor macros can be replaced with their
> equivalent VendorDeviceOutRequest and VendorDeviceRequest macros from usb.h.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/usb/dev-serial.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-26 15:04 ` Samuel Thibault
@ 2020-10-27 13:18 ` Mark Cave-Ayland
2020-10-27 13:30 ` Jason Andryuk
0 siblings, 1 reply; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-27 13:18 UTC (permalink / raw)
To: Samuel Thibault; +Cc: Jason Andryuk, Gerd Hoffmann, Aurelien Jarno, QEMU
On 26/10/2020 15:04, Samuel Thibault wrote:
> Mark Cave-Ayland, le lun. 26 oct. 2020 13:40:05 +0000, a ecrit:
>> On 26/10/2020 13:00, Jason Andryuk wrote:
>>> On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
>>>> Aurelien, you introduced the "| 1" in
>>>>
>>>> commit abb8a13918ecc1e8160aa78582de9d5224ea70df
>>>> Author: Aurelien Jarno <aurelien@aurel32.net>
>>>> Date: Wed Aug 13 04:23:17 2008 +0000
>>>>
>>>> usb-serial: add support for modem lines
>>>>
>>>> [...]
>>>> @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int request, int value,
>>>> /* TODO: TX ON/OFF */
>>>> break;
>>>> case DeviceInVendor | FTDI_GET_MDM_ST:
>>>> - /* TODO: return modem status */
>>>> - data[0] = 0;
>>>> - ret = 1;
>>>> + data[0] = usb_get_modem_lines(s) | 1;
>>>> + data[1] = 0;
>>>> + ret = 2;
>>>> break;
>>>
>>> I'm not particularly familiar with the FTDI USB serial devices. I
>>> found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.
>>>
>>> A little searching found this:
>>> https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541
>>>
>>> That shows "B0 Reserved - must be 1", so maybe that is why "| 1" was added?
>>
>> Right - that's for the modem status returned as part of the first 2 status
>> bytes for incoming data which is slightly different from modem status
>> returned directly from FTDI_SIO_GET_MODEM_STATUS: https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L423.
>>
>> It is the latter which this patch changes and appears to match what I see on
>> my Chipi-X hardware here.
>
> Aurelien, do you remember the reason for the addition of 1 here? It does
> look like the confusion between the incoming data bytes and the modem
> status bytes.
I spent a bit of time this morning doing some further tests on Linux using 2 machines
and a test program to check CTS and usbmon:
usbmon when adapter unplugged:
ffff95a4bf2dd300 2366831506 S Ci:4:004:0 s c0 05 0000 0000 0002 2 <
ffff95a4bf2dd300 2366831607 C Ci:4:004:0 0 2 = 0160
usbmon when adapter plugged in and remote connected to minicom:
ffff95a4452a79c0 2457273895 S Ci:4:004:0 s c0 05 0000 0000 0002 2 <
ffff95a4452a79c0 2457274057 C Ci:4:004:0 0 2 = 3160
It seems I made a mistake here since the response is interpreted as 2 bytes rather
than a single little-endian word: with CTS asserted on the remote the first byte is
0x31 == FTDI_CTS | FTDI_DSR | 1, whilst the 2nd byte is 0x60 == FTDI_THRE | FTDI_TEMT
which matches the existing QEMU code rather than the comments in ftdi_sio.h.
So sorry for the noise: I'll drop this patch from the series and post a v2 shortly.
ATB,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached
2020-10-27 8:09 ` Gerd Hoffmann
@ 2020-10-27 13:23 ` Mark Cave-Ayland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2020-10-27 13:23 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: samuel.thibault, qemu-devel
On 27/10/2020 08:09, Gerd Hoffmann wrote:
>> case CHR_EVENT_OPENED:
>> - if (!s->dev.attached) {
>> + if (!s->always_plugged && !s->dev.attached) {
>> usb_device_attach(&s->dev, &error_abort);
>> }
>
> Not needed (but doesn't hurt either).
Okay I'll leave this as-is for now.
>> break;
>> case CHR_EVENT_CLOSED:
>> - if (s->dev.attached) {
>> + if (!s->always_plugged && s->dev.attached) {
>> usb_device_detach(&s->dev);
>> }
>
> Ok.
>
>> - if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
>> + if (s->always_plugged || (qemu_chr_fe_backend_open(&s->cs) &&
>> + !dev->attached)) {
>
> The dev->attached check should not be skipped, i.e. the logic should be
> ((always_plugged || open) && !attached).
Let me test this, and if it works I'll post a v2 shortly.
ATB,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
2020-10-27 13:18 ` Mark Cave-Ayland
@ 2020-10-27 13:30 ` Jason Andryuk
0 siblings, 0 replies; 34+ messages in thread
From: Jason Andryuk @ 2020-10-27 13:30 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: QEMU, Samuel Thibault, Aurelien Jarno, Gerd Hoffmann
On Tue, Oct 27, 2020 at 9:18 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> I spent a bit of time this morning doing some further tests on Linux using 2 machines
> and a test program to check CTS and usbmon:
>
> usbmon when adapter unplugged:
> ffff95a4bf2dd300 2366831506 S Ci:4:004:0 s c0 05 0000 0000 0002 2 <
> ffff95a4bf2dd300 2366831607 C Ci:4:004:0 0 2 = 0160
>
> usbmon when adapter plugged in and remote connected to minicom:
> ffff95a4452a79c0 2457273895 S Ci:4:004:0 s c0 05 0000 0000 0002 2 <
> ffff95a4452a79c0 2457274057 C Ci:4:004:0 0 2 = 3160
>
> It seems I made a mistake here since the response is interpreted as 2 bytes rather
> than a single little-endian word: with CTS asserted on the remote the first byte is
> 0x31 == FTDI_CTS | FTDI_DSR | 1, whilst the 2nd byte is 0x60 == FTDI_THRE | FTDI_TEMT
> which matches the existing QEMU code rather than the comments in ftdi_sio.h.
>
> So sorry for the noise: I'll drop this patch from the series and post a v2 shortly.
No worries. Thanks for investigating.
(I had the thought that maybe reserve bit 0 differentiates one and two
byte responses? Bit 0 clear indicates a 1-byte response and set
indicates a 2 bit response. But I'm just guessing.)
Regards,
Jason
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2020-10-27 13:51 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26 8:33 [PATCH 0/9] dev-serial: minor fixes and improvements Mark Cave-Ayland
2020-10-26 8:33 ` [PATCH 1/9] dev-serial: style changes to improve readability and checkpatch fixes Mark Cave-Ayland
2020-10-26 9:35 ` Samuel Thibault
2020-10-26 8:33 ` [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments Mark Cave-Ayland
2020-10-26 9:37 ` Samuel Thibault
2020-10-27 9:04 ` Philippe Mathieu-Daudé
2020-10-26 8:33 ` [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events Mark Cave-Ayland
2020-10-26 9:38 ` Samuel Thibault
2020-10-27 9:08 ` Philippe Mathieu-Daudé
2020-10-26 8:33 ` [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters Mark Cave-Ayland
2020-10-26 9:40 ` Samuel Thibault
2020-10-27 9:08 ` Philippe Mathieu-Daudé
2020-10-26 8:33 ` [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h Mark Cave-Ayland
2020-10-26 9:41 ` Samuel Thibault
2020-10-27 9:09 ` Philippe Mathieu-Daudé
2020-10-26 8:33 ` [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached Mark Cave-Ayland
2020-10-26 9:45 ` Samuel Thibault
2020-10-27 8:09 ` Gerd Hoffmann
2020-10-27 13:23 ` Mark Cave-Ayland
2020-10-26 8:33 ` [PATCH 7/9] dev-serial: add support for setting data_bits in QEMUSerialSetParams Mark Cave-Ayland
2020-10-26 9:46 ` Samuel Thibault
2020-10-26 8:34 ` [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response Mark Cave-Ayland
2020-10-26 9:54 ` Samuel Thibault
2020-10-26 10:58 ` Mark Cave-Ayland
2020-10-26 11:14 ` Samuel Thibault
2020-10-26 13:00 ` Jason Andryuk
2020-10-26 13:40 ` Mark Cave-Ayland
2020-10-26 14:04 ` Jason Andryuk
2020-10-26 15:04 ` Samuel Thibault
2020-10-27 13:18 ` Mark Cave-Ayland
2020-10-27 13:30 ` Jason Andryuk
2020-10-26 8:34 ` [PATCH 9/9] dev-serial: store flow control and xon/xoff characters Mark Cave-Ayland
2020-10-26 9:58 ` Samuel Thibault
2020-10-26 9:59 ` [PATCH 0/9] dev-serial: minor fixes and improvements Samuel Thibault
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.