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

[Hervé Poussineau]

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
> is_parallel_enabled(s).

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

> > +
> > +type_init(pc87312_register_types)
> > +
>
> 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,

Thanks

Hervé