| Message ID | 20201026083401.13231-1-mark.cave-ayland@ilande.co.uk |
|---|---|
| Headers | show |
| Series | dev-serial: minor fixes and improvements | expand |
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 >
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 >
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 >
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
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
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
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.
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
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
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>
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>
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.
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.
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
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(-)