[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 7/7] hw/char/serial: Let SerialState have an 'id' field
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 7/7] hw/char/serial: Let SerialState have an 'id' field |
Date: |
Sat, 12 Sep 2020 13:28:52 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
On 9/12/20 11:14 AM, Paolo Bonzini wrote:
> On 07/09/20 03:55, Philippe Mathieu-Daudé wrote:
>> When a SoC has multiple UARTs (some configured differently),
>> it is hard to associate events to their UART.
>>
>> To be able to distinct trace events between various instances,
>> add an 'id' field. Update the trace format accordingly.
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> include/hw/char/serial.h | 1 +
>> hw/char/serial.c | 7 ++++---
>> hw/char/trace-events | 6 +++---
>> 3 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
>> index 3d2a5b27e87..3ee2d096a85 100644
>> --- a/include/hw/char/serial.h
>> +++ b/include/hw/char/serial.h
>> @@ -75,6 +75,7 @@ typedef struct SerialState {
>> uint64_t char_transmit_time; /* time to transmit a char in ticks */
>> int poll_msl;
>>
>> + uint8_t id;
>> QEMUTimer *modem_status_poll;
>> MemoryRegion io;
>> } SerialState;
>> diff --git a/hw/char/serial.c b/hw/char/serial.c
>> index ade89fadb44..e5a6b939f13 100644
>> --- a/hw/char/serial.c
>> +++ b/hw/char/serial.c
>> @@ -177,7 +177,7 @@ static void serial_update_parameters(SerialState *s)
>> ssp.stop_bits = stop_bits;
>> s->char_transmit_time = (NANOSECONDS_PER_SECOND / speed) * frame_size;
>> qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
>> - trace_serial_update_parameters(speed, parity, data_bits, stop_bits);
>> + trace_serial_update_parameters(s->id, speed, parity, data_bits,
>> stop_bits);
>> }
>>
>> static void serial_update_msl(SerialState *s)
>> @@ -333,7 +333,7 @@ static void serial_ioport_write(void *opaque, hwaddr
>> addr, uint64_t val,
>> SerialState *s = opaque;
>>
>> assert(size == 1 && addr < 8);
>> - trace_serial_write(addr, val);
>> + trace_serial_write(s->id, addr, val);
>> switch(addr) {
>> default:
>> case 0:
>> @@ -550,7 +550,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr
>> addr, unsigned size)
>> ret = s->scr;
>> break;
>> }
>> - trace_serial_read(addr, ret);
>> + trace_serial_read(s->id, addr, ret);
>> return ret;
>> }
>>
>> @@ -1013,6 +1013,7 @@ static const TypeInfo serial_io_info = {
>> };
>>
>> static Property serial_properties[] = {
>> + DEFINE_PROP_UINT8("id", SerialState, id, 0),
>> DEFINE_PROP_CHR("chardev", SerialState, chr),
>> DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200),
>> DEFINE_PROP_BOOL("wakeup", SerialState, wakeup, false),
>> diff --git a/hw/char/trace-events b/hw/char/trace-events
>> index cd36b63f39d..40800c9334c 100644
>> --- a/hw/char/trace-events
>> +++ b/hw/char/trace-events
>> @@ -5,9 +5,9 @@ parallel_ioport_read(const char *desc, uint16_t addr,
>> uint8_t value) "read [%s]
>> parallel_ioport_write(const char *desc, uint16_t addr, uint8_t value)
>> "write [%s] addr 0x%02x val 0x%02x"
>>
>> # serial.c
>> -serial_read(uint16_t addr, uint8_t value) "read addr 0x%02x val 0x%02x"
>> -serial_write(uint16_t addr, uint8_t value) "write addr 0x%02x val 0x%02x"
>> -serial_update_parameters(uint64_t baudrate, char parity, int data_bits, int
>> stop_bits) "baudrate=%"PRIu64" parity='%c' data=%d stop=%d"
>> +serial_read(uint8_t id, uint8_t addr, uint8_t value) "id#%u read addr 0x%x
>> val 0x%02x"
>> +serial_write(uint8_t id, uint8_t addr, uint8_t value) "id#%u write addr
>> 0x%x val 0x%02x"
>> +serial_update_parameters(uint8_t id, uint64_t baudrate, char parity, int
>> data_bits, int stop_bits) "id#%u baudrate=%"PRIu64" parity=%c data=%d
>> stop=%d"
>>
>> # virtio-serial-bus.c
>> virtio_serial_send_control_event(unsigned int port, uint16_t event,
>> uint16_t value) "port %u, event %u, value %u"
>>
>
> I'm not sure about making this one a one-off for serial.c. You could
> add the SerialState* too, for example.
hw/char/serial-pci-multi.c:45
Ah indeed, not sure why I only used qdev_alias_all_properties()
on the ISA one. Probably because I don't use the other ones.
I'll send a new patch for the PCI-single device:
-- >8 --
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -81,7 +81,6 @@ static const VMStateDescription vmstate_pci_serial = {
};
static Property serial_pci_properties[] = {
- DEFINE_PROP_CHR("chardev", PCISerialState, state.chr),
DEFINE_PROP_UINT8("prog_if", PCISerialState, prog_if, 0x02),
DEFINE_PROP_END_OF_LIST(),
};
@@ -106,6 +105,8 @@ static void serial_pci_init(Object *o)
PCISerialState *ps = PCI_SERIAL(o);
object_initialize_child(o, "serial", &ps->state, TYPE_SERIAL);
+
+ qdev_alias_all_properties(DEVICE(&ps->state), o);
}
---
And amend to this one:
-- >8 --
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -105,6 +105,7 @@ static void multi_serial_pci_realize(PCIDevice *dev,
Error **errp)
for (i = 0; i < nports; i++) {
s = pci->state + i;
+ qdev_prop_set_uint8(s, "id", i);
if (!qdev_realize(DEVICE(s), NULL, errp)) {
multi_serial_pci_exit(dev);
return;
---
Then we are done.
> I have queued the other six though.
Thanks!
>
> Paolo
>
>
- [PATCH 2/7] hw/char/serial: Replace commented DPRINTF() by trace event, (continued)
- [PATCH 2/7] hw/char/serial: Replace commented DPRINTF() by trace event, Philippe Mathieu-Daudé, 2020/09/06
- [PATCH 4/7] hw/char/serial: Rename I/O read/write trace events, Philippe Mathieu-Daudé, 2020/09/06
- [PATCH 5/7] hw/char/serial: Make 'wakeup' property boolean, Philippe Mathieu-Daudé, 2020/09/06
- [PATCH 3/7] hw/char/serial: Remove old DEBUG_SERIAL commented code, Philippe Mathieu-Daudé, 2020/09/06
- [PATCH 6/7] hw/char/serial-isa: Alias QDEV properties from generic serial object, Philippe Mathieu-Daudé, 2020/09/06
- [PATCH 7/7] hw/char/serial: Let SerialState have an 'id' field, Philippe Mathieu-Daudé, 2020/09/06