| Message ID | 1346917367-24691-4-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 6 September 2012 08:42, Gerd Hoffmann <kraxel@redhat.com> wrote: > @@ -789,6 +790,13 @@ static int serial_isa_initfn(ISADevice *dev) > isa->isairq = isa_serial_irq[isa->index]; > index++; > > + if (isa->iobase == 0x3f8) { > + s->reason = QEMU_WAKEUP_REASON_GPE_a; > + s->wakeup = 1; > + } else { > + s->reason = QEMU_WAKEUP_REASON_OTHER; > + } > + It seems a bit odd that this is done in the ISA serial model itself and not by the next level up wiring up some output of the ISA serial device to some appropriate input... -- PMM
On 09/06/12 09:48, Peter Maydell wrote: > On 6 September 2012 08:42, Gerd Hoffmann <kraxel@redhat.com> wrote: >> @@ -789,6 +790,13 @@ static int serial_isa_initfn(ISADevice *dev) >> isa->isairq = isa_serial_irq[isa->index]; >> index++; >> >> + if (isa->iobase == 0x3f8) { >> + s->reason = QEMU_WAKEUP_REASON_GPE_a; >> + s->wakeup = 1; >> + } else { >> + s->reason = QEMU_WAKEUP_REASON_OTHER; >> + } >> + > > It seems a bit odd that this is done in the ISA serial model > itself and not by the next level up wiring up some output > of the ISA serial device to some appropriate input... Suggestions how to do that are welcome. Preferably some which don't break on 'qemu -nodefault -device isa-serial,chardev=foo'. cheers, Gerd
On Thu, Sep 6, 2012 at 10:47 AM, Gerd Hoffmann <kraxel@redhat.com> wrote: > On 09/06/12 09:48, Peter Maydell wrote: >> On 6 September 2012 08:42, Gerd Hoffmann <kraxel@redhat.com> wrote: >>> @@ -789,6 +790,13 @@ static int serial_isa_initfn(ISADevice *dev) >>> isa->isairq = isa_serial_irq[isa->index]; >>> index++; >>> >>> + if (isa->iobase == 0x3f8) { >>> + s->reason = QEMU_WAKEUP_REASON_GPE_a; >>> + s->wakeup = 1; >>> + } else { >>> + s->reason = QEMU_WAKEUP_REASON_OTHER; >>> + } >>> + >> >> It seems a bit odd that this is done in the ISA serial model >> itself and not by the next level up wiring up some output >> of the ISA serial device to some appropriate input... > > Suggestions how to do that are welcome. Preferably some which don't > break on 'qemu -nodefault -device isa-serial,chardev=foo'. Add a qdev property? The base address check can't be correct, the serial device could be the only one in the board and wired to wakeup but still use a different iobase. One way could be to check if chr == serial_hds[0] or rather, pass the wakeup reason code from board level based on this check. > > cheers, > Gerd > >
Il 08/09/2012 09:15, Blue Swirl ha scritto: >> Preferably some which don't >> > break on 'qemu -nodefault -device isa-serial,chardev=foo'. > Add a qdev property? The base address check can't be correct, the > serial device could be the only one in the board and wired to wakeup > but still use a different iobase. Could work, but the default value for the property would still be "depending on the iobase". > One way could be to check if chr == serial_hds[0] or rather, pass the > wakeup reason code from board level based on this check. That doesn't work for -device. Paolo
Hi, >>> It seems a bit odd that this is done in the ISA serial model >>> itself and not by the next level up wiring up some output >>> of the ISA serial device to some appropriate input... >> >> Suggestions how to do that are welcome. Preferably some which don't >> break on 'qemu -nodefault -device isa-serial,chardev=foo'. > > Add a qdev property? And offload it to the user / management tool to get that correct? Don't think this is a good idea. > The base address check can't be correct, the > serial device could be the only one in the board and wired to wakeup > but still use a different iobase. We might want to add a check for TARGET_I386, but for x86 it actually _is_ correct as this matches the acpi dsdt information. > One way could be to check if chr == serial_hds[0] or rather, pass the > wakeup reason code from board level based on this check. Wouldn't work. cheers, Gerd
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 08/09/2012 09:15, Blue Swirl ha scritto: >>> Preferably some which don't >>> > break on 'qemu -nodefault -device isa-serial,chardev=foo'. >> Add a qdev property? The base address check can't be correct, the >> serial device could be the only one in the board and wired to wakeup >> but still use a different iobase. > > Could work, but the default value for the property would still be > "depending on the iobase". > >> One way could be to check if chr == serial_hds[0] or rather, pass the >> wakeup reason code from board level based on this check. > > That doesn't work for -device. I think the wiring is wrong here. Why would a device assert the wakeup reason? I assume on bare metal, the device asserts a single line and then something devices how to treat the output from a given device. IOW, shouldn't it be something more like, SerialState exports a qemu_irq and then the machine code decides whether to route that irq to a call to qemu_system_wakeup(). Then depending on whether this is coming from serial[1], determines which reason is used. Regards, Anthony Liguori > > Paolo
Il 10/09/2012 18:02, Anthony Liguori ha scritto: > I think the wiring is wrong here. Why would a device assert the wakeup > reason? > > I assume on bare metal, the device asserts a single line and then > something devices how to treat the output from a given device. > > IOW, shouldn't it be something more like, SerialState exports a qemu_irq > and then the machine code decides whether to route that irq to a call to > qemu_system_wakeup(). Then depending on whether this is coming from > serial[1], determines which reason is used. But the only way to do it, that works for -device, is to look at the SerialState's iobase. So it would still be a hack, only in ACPI rather than isa-serial.c. Paolo
On 10 September 2012 17:07, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 10/09/2012 18:02, Anthony Liguori ha scritto: >> IOW, shouldn't it be something more like, SerialState exports a qemu_irq >> and then the machine code decides whether to route that irq to a call to >> qemu_system_wakeup(). Then depending on whether this is coming from >> serial[1], determines which reason is used. > > But the only way to do it, that works for -device, is to look at the > SerialState's iobase. This just says that -device is broken (which we kinda already knew :-)) -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 10 September 2012 17:07, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 10/09/2012 18:02, Anthony Liguori ha scritto: >>> IOW, shouldn't it be something more like, SerialState exports a qemu_irq >>> and then the machine code decides whether to route that irq to a call to >>> qemu_system_wakeup(). Then depending on whether this is coming from >>> serial[1], determines which reason is used. >> >> But the only way to do it, that works for -device, is to look at the >> SerialState's iobase. > > This just says that -device is broken (which we kinda already knew > :-)) There are opposing forces here. libvirt wants to use -device because we don't provide a way for machine creates devices to be configurable. We can't fix this until we separate realization from initialization. After that point, libvirt could address properties on machine-created devices directly. I think the best short term solution is to have a post '-device' handler in machines that could be used to make the appropriate connections. Power et al. need this anyway for dealing with device tree creation so it seems to be inevitable. Regards, Anthony Liguori > > -- PMM
diff --git a/hw/serial.c b/hw/serial.c index a421d1e..06f7e28 100644 --- a/hw/serial.c +++ b/hw/serial.c @@ -140,6 +140,7 @@ struct SerialState { int baudbase; int tsr_retry; uint32_t wakeup; + WakeupReason reason; uint64_t last_xmit_ts; /* Time when the last byte was successfully sent out of the tsr */ SerialFIFO recv_fifo; @@ -641,7 +642,7 @@ static void serial_receive1(void *opaque, const uint8_t *buf, int size) SerialState *s = opaque; if (s->wakeup) { - qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER); + qemu_system_wakeup_request(s->reason); } if(s->fcr & UART_FCR_FE) { int i; @@ -789,6 +790,13 @@ static int serial_isa_initfn(ISADevice *dev) isa->isairq = isa_serial_irq[isa->index]; index++; + if (isa->iobase == 0x3f8) { + s->reason = QEMU_WAKEUP_REASON_GPE_a; + s->wakeup = 1; + } else { + s->reason = QEMU_WAKEUP_REASON_OTHER; + } + s->baudbase = 115200; isa_init_irq(dev, &s->irq, isa->isairq); serial_init_core(s);
serial port #1 gets gpe 0x0a. Now that the wakeup is guest-configurable via acpi we also enable it unconditionally. Other serial ports are unchanged: they continue to use the "other" exit reason and are disabled unless explicitly enabled via wakeup property. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/serial.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)