[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [Qemu-devel] [PATCH 5/6] prep: add pc87312 Super I/O emul

From: Hervé Poussineau
Subject: Re: [Qemu-ppc] [Qemu-devel] [PATCH 5/6] prep: add pc87312 Super I/O emulation
Date: Mon, 19 Mar 2012 19:26:44 +0100
User-agent: Thunderbird (Windows/20090812)

Andreas Färber a écrit :
Am 17.03.2012 15:39, schrieb Hervé Poussineau:
This provides floppy and IDE controllers as well as serial and parallel ports.
However, dynamic configuration of devices is not yet supported.

Cc: Andreas Färber <address@hidden>
Signed-off-by: Hervé Poussineau <address@hidden>
 Makefile.objs |    1 +
 hw/pc87312.c  |  425 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 426 insertions(+), 0 deletions(-)
 create mode 100644 hw/pc87312.c

diff --git a/hw/pc87312.c b/hw/pc87312.c
new file mode 100644
index 0000000..1e28dbd
--- /dev/null
+++ b/hw/pc87312.c
@@ -0,0 +1,425 @@
+ * QEMU National Semiconductor PC87312 (Super I/O)
+ *
+ * Copyright (c) 2010-2012 Herve Poussineau

FWIW mind to add

Copyright (c) 2011 Andreas Färber


Yes, of course. Sorry about that.

+    chr = s->parallel.chr;
+    if (s->parallel.chr != NULL && is_parallel_enabled(s)) {

This logic still seems to be flawed: it should not depend on
s->parallel.chr. If that is NULL we need to create a null char device to
match the device that's present in hardware according to

Ok, will remove the 'chr != NULL' check.

+        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); /* HACK */

HACK alarm in [PATCH]: What for?

The problem is composition. Main board contains a pc87312, which contains a isa-parallel. Main board doesn't know the isa-parallel device.

isa-parallel device must have a chardev (set by a property). pc87312 creates the isa-parallel device, so it must know the chardev. I did it also with a chardev property. Unfortunately, a chardev can only be used once, and I have not found a better way to give chardev from main board to superI/O to parallel.
Do you have any better idea?

+    for (i = 0; i < 2; i++) {
+        chr = s->uart[i].chr;

+        if (chr != NULL && is_uart_enabled(s, i)) {
+            qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); /* HACK */

2x ditto.

Same answer :)

+static void pc87312_class_initfn(ObjectClass *klass, void *data)

I always thought initfn was used for instances...

Ok, will change to pc87312_class_init


Trailing empty line.

Ok, will remove.

So what about the ugly ISA hot-plug issue that originally stalled our
two series? I'm missing a Change Log about that. You changed the initial
configuration to the one used by 40P firmware IIRC and stopped
supporting the chipset's reconfiguration? Either way any limitation of
this implementation should be prominently documented please.

Yes, chipset reconfiguration required ISA hot-plug issue, so I didn't want to wait for this serie. In commit message, I wrote "However, dynamic configuration of devices is not yet supported.", and you get an error message at each reconfiguration:

+static void reconfigure_devices(PC87312State *s)
+ error_report("pc87312: unsupported device reconfiguration (%02x %02x %02x)",
+                 s->FER, s->FAR, s->PTR);

What else should I add?
Anyway, if/when dynamic reconfiguration will be implemented, only this method will be changed.

About the initial configuration, it is controlled by "config" property.
In patch 6/6, when I use the Super I/O chip in 'prep' machine, I set this property to 13, which means fdc + serial0 + serial1 + parallel0. For IBM 40p, the initial configuration should be 15 (fdc + ide + serial0 + parallel0).
So I don't think the Super I/O is exclusively tied to IBM 40p.

Thanks for your work on this,



reply via email to

[Prev in Thread] Current Thread [Next in Thread]