* [Qemu-devel] [PATCH for-4.1 0/4] hw/mips/r4k: Refactor the Super I/O chipset @ 2019-04-04 22:12 Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable Philippe Mathieu-Daudé ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-04 22:12 UTC (permalink / raw) To: qemu-devel, Thomas Huth Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini, Philippe Mathieu-Daudé Hi, This series refactor the superio code used in the mips_r4k machine, adding the TYPE_R4K_SUPERIO device. Doing so we improve the ISASuperIODevice, fixing an easy bug. This series also demonstrate that no machine access a I8042 ISA device out of a Super I/O chipset. Regards, Phil. Philippe Mathieu-Daudé (4): hw/isa/superio: Rename a variable hw/isa/superio: Support more than one IDE bus hw/isa/superio: Support chipsets with no Floppy Disk controller hw/mips/r4k: Refactor the Super I/O chipset hw/isa/isa-superio.c | 99 +++++++++++++++++++++------------------- hw/mips/mips_r4k.c | 61 ++++++++++++++++++------- include/hw/isa/superio.h | 2 +- 3 files changed, 98 insertions(+), 64 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable 2019-04-04 22:12 [Qemu-devel] [PATCH for-4.1 0/4] hw/mips/r4k: Refactor the Super I/O chipset Philippe Mathieu-Daudé @ 2019-04-04 22:12 ` Philippe Mathieu-Daudé 2019-04-05 4:14 ` Thomas Huth 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 2/4] hw/isa/superio: Support more than one IDE bus Philippe Mathieu-Daudé ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-04 22:12 UTC (permalink / raw) To: qemu-devel, Thomas Huth Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini, Philippe Mathieu-Daudé This patch is purely cosmetic. No functional change. This will ease the next patch where we re-indent an if() statement. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/isa/isa-superio.c | 69 ++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c index d54463bf035..c6845eaf578 100644 --- a/hw/isa/isa-superio.c +++ b/hw/isa/isa-superio.c @@ -22,8 +22,8 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) { - ISASuperIODevice *sio = ISA_SUPERIO(dev); - ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio); + ISASuperIODevice *s = ISA_SUPERIO(dev); + ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s); ISABus *bus = isa_bus_from_device(ISA_DEVICE(dev)); ISADevice *isa; DeviceState *d; @@ -34,12 +34,12 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) /* Parallel port */ for (i = 0; i < k->parallel.count; i++) { - if (i >= ARRAY_SIZE(sio->parallel)) { + if (i >= ARRAY_SIZE(s->parallel)) { warn_report("superio: ignoring %td parallel controllers", - k->parallel.count - ARRAY_SIZE(sio->parallel)); + k->parallel.count - ARRAY_SIZE(s->parallel)); break; } - if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) { + if (!k->parallel.is_enabled || k->parallel.is_enabled(s, i)) { /* FIXME use a qdev chardev prop instead of parallel_hds[] */ chr = parallel_hds[i]; if (chr == NULL) { @@ -53,33 +53,33 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) qdev_prop_set_uint32(d, "index", i); if (k->parallel.get_iobase) { qdev_prop_set_uint32(d, "iobase", - k->parallel.get_iobase(sio, i)); + k->parallel.get_iobase(s, i)); } if (k->parallel.get_irq) { - qdev_prop_set_uint32(d, "irq", k->parallel.get_irq(sio, i)); + qdev_prop_set_uint32(d, "irq", k->parallel.get_irq(s, i)); } qdev_prop_set_chr(d, "chardev", chr); qdev_init_nofail(d); - sio->parallel[i] = isa; + s->parallel[i] = isa; trace_superio_create_parallel(i, k->parallel.get_iobase ? - k->parallel.get_iobase(sio, i) : -1, + k->parallel.get_iobase(s, i) : -1, k->parallel.get_irq ? - k->parallel.get_irq(sio, i) : -1); + k->parallel.get_irq(s, i) : -1); object_property_add_child(OBJECT(dev), name, - OBJECT(sio->parallel[i]), NULL); + OBJECT(s->parallel[i]), NULL); g_free(name); } } /* Serial */ for (i = 0; i < k->serial.count; i++) { - if (i >= ARRAY_SIZE(sio->serial)) { + if (i >= ARRAY_SIZE(s->serial)) { warn_report("superio: ignoring %td serial controllers", - k->serial.count - ARRAY_SIZE(sio->serial)); + k->serial.count - ARRAY_SIZE(s->serial)); break; } - if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) { + if (!k->serial.is_enabled || k->serial.is_enabled(s, i)) { /* FIXME use a qdev chardev prop instead of serial_hd() */ chr = serial_hd(i); if (chr == NULL) { @@ -92,35 +92,34 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) d = DEVICE(isa); qdev_prop_set_uint32(d, "index", i); if (k->serial.get_iobase) { - qdev_prop_set_uint32(d, "iobase", - k->serial.get_iobase(sio, i)); + qdev_prop_set_uint32(d, "iobase", k->serial.get_iobase(s, i)); } if (k->serial.get_irq) { - qdev_prop_set_uint32(d, "irq", k->serial.get_irq(sio, i)); + qdev_prop_set_uint32(d, "irq", k->serial.get_irq(s, i)); } qdev_prop_set_chr(d, "chardev", chr); qdev_init_nofail(d); - sio->serial[i] = isa; + s->serial[i] = isa; trace_superio_create_serial(i, k->serial.get_iobase ? - k->serial.get_iobase(sio, i) : -1, + k->serial.get_iobase(s, i) : -1, k->serial.get_irq ? - k->serial.get_irq(sio, i) : -1); + k->serial.get_irq(s, i) : -1); object_property_add_child(OBJECT(dev), name, - OBJECT(sio->serial[0]), NULL); + OBJECT(s->serial[0]), NULL); g_free(name); } } /* Floppy disc */ - if (!k->floppy.is_enabled || k->floppy.is_enabled(sio, 0)) { + if (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0)) { isa = isa_create(bus, "isa-fdc"); d = DEVICE(isa); if (k->floppy.get_iobase) { - qdev_prop_set_uint32(d, "iobase", k->floppy.get_iobase(sio, 0)); + qdev_prop_set_uint32(d, "iobase", k->floppy.get_iobase(s, 0)); } if (k->floppy.get_irq) { - qdev_prop_set_uint32(d, "irq", k->floppy.get_irq(sio, 0)); + qdev_prop_set_uint32(d, "irq", k->floppy.get_irq(s, 0)); } /* FIXME use a qdev drive property instead of drive_get() */ drive = drive_get(IF_FLOPPY, 0, 0); @@ -135,37 +134,37 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) &error_fatal); } qdev_init_nofail(d); - sio->floppy = isa; + s->floppy = isa; trace_superio_create_floppy(0, k->floppy.get_iobase ? - k->floppy.get_iobase(sio, 0) : -1, + k->floppy.get_iobase(s, 0) : -1, k->floppy.get_irq ? - k->floppy.get_irq(sio, 0) : -1); + k->floppy.get_irq(s, 0) : -1); } /* Keyboard, mouse */ - sio->kbc = isa_create_simple(bus, TYPE_I8042); + s->kbc = isa_create_simple(bus, TYPE_I8042); /* IDE */ - if (k->ide.count && (!k->ide.is_enabled || k->ide.is_enabled(sio, 0))) { + if (k->ide.count && (!k->ide.is_enabled || k->ide.is_enabled(s, 0))) { isa = isa_create(bus, "isa-ide"); d = DEVICE(isa); if (k->ide.get_iobase) { - qdev_prop_set_uint32(d, "iobase", k->ide.get_iobase(sio, 0)); + qdev_prop_set_uint32(d, "iobase", k->ide.get_iobase(s, 0)); } if (k->ide.get_iobase) { - qdev_prop_set_uint32(d, "iobase2", k->ide.get_iobase(sio, 1)); + qdev_prop_set_uint32(d, "iobase2", k->ide.get_iobase(s, 1)); } if (k->ide.get_irq) { - qdev_prop_set_uint32(d, "irq", k->ide.get_irq(sio, 0)); + qdev_prop_set_uint32(d, "irq", k->ide.get_irq(s, 0)); } qdev_init_nofail(d); - sio->ide = isa; + s->ide = isa; trace_superio_create_ide(0, k->ide.get_iobase ? - k->ide.get_iobase(sio, 0) : -1, + k->ide.get_iobase(s, 0) : -1, k->ide.get_irq ? - k->ide.get_irq(sio, 0) : -1); + k->ide.get_irq(s, 0) : -1); } } -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable @ 2019-04-05 4:14 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2019-04-05 4:14 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: > This patch is purely cosmetic. No functional change. > This will ease the next patch where we re-indent an if() statement. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/isa/isa-superio.c | 69 ++++++++++++++++++++++---------------------- > 1 file changed, 34 insertions(+), 35 deletions(-) > > diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c > index d54463bf035..c6845eaf578 100644 > --- a/hw/isa/isa-superio.c > +++ b/hw/isa/isa-superio.c > @@ -22,8 +22,8 @@ > > static void isa_superio_realize(DeviceState *dev, Error **errp) > { > - ISASuperIODevice *sio = ISA_SUPERIO(dev); > - ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio); > + ISASuperIODevice *s = ISA_SUPERIO(dev); > + ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s); Sorry, but I have to say that I normally really don't like single-letter variable names. They are much harder to search for, and way less self-describing. If you get problems with the line length in a later patch, what about refactoring the related code into a separate function? So that you'd only have something like this in the realize function: for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) { if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) { isa_superio_init_ide(...); } } Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable @ 2019-04-05 4:14 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2019-04-05 4:14 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Aleksandar Rikalo, Paolo Bonzini, Aleksandar Markovic, Aurelien Jarno, Michael S. Tsirkin On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: > This patch is purely cosmetic. No functional change. > This will ease the next patch where we re-indent an if() statement. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/isa/isa-superio.c | 69 ++++++++++++++++++++++---------------------- > 1 file changed, 34 insertions(+), 35 deletions(-) > > diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c > index d54463bf035..c6845eaf578 100644 > --- a/hw/isa/isa-superio.c > +++ b/hw/isa/isa-superio.c > @@ -22,8 +22,8 @@ > > static void isa_superio_realize(DeviceState *dev, Error **errp) > { > - ISASuperIODevice *sio = ISA_SUPERIO(dev); > - ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio); > + ISASuperIODevice *s = ISA_SUPERIO(dev); > + ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s); Sorry, but I have to say that I normally really don't like single-letter variable names. They are much harder to search for, and way less self-describing. If you get problems with the line length in a later patch, what about refactoring the related code into a separate function? So that you'd only have something like this in the realize function: for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) { if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) { isa_superio_init_ide(...); } } Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable @ 2019-04-05 9:32 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-05 9:32 UTC (permalink / raw) To: Thomas Huth, qemu-devel Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini On 4/5/19 6:14 AM, Thomas Huth wrote: > On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: >> This patch is purely cosmetic. No functional change. >> This will ease the next patch where we re-indent an if() statement. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/isa/isa-superio.c | 69 ++++++++++++++++++++++---------------------- >> 1 file changed, 34 insertions(+), 35 deletions(-) >> >> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c >> index d54463bf035..c6845eaf578 100644 >> --- a/hw/isa/isa-superio.c >> +++ b/hw/isa/isa-superio.c >> @@ -22,8 +22,8 @@ >> >> static void isa_superio_realize(DeviceState *dev, Error **errp) >> { >> - ISASuperIODevice *sio = ISA_SUPERIO(dev); >> - ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio); >> + ISASuperIODevice *s = ISA_SUPERIO(dev); >> + ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s); > > Sorry, but I have to say that I normally really don't like single-letter > variable names. They are much harder to search for, and way less > self-describing. Me too ;) But sometimes I find the 80 chars/line limit generate indentation harder to review. But here I have no excuse :) > If you get problems with the line length in a later patch, what about > refactoring the related code into a separate function? So that you'd > only have something like this in the realize function: > > for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) { > if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) { > isa_superio_init_ide(...); > } > } Yes, fair enough. Note you used the shortened variables in your example! :P Regards, Phil. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable @ 2019-04-05 9:32 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-05 9:32 UTC (permalink / raw) To: Thomas Huth, qemu-devel Cc: Aleksandar Rikalo, Paolo Bonzini, Aleksandar Markovic, Aurelien Jarno, Michael S. Tsirkin On 4/5/19 6:14 AM, Thomas Huth wrote: > On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: >> This patch is purely cosmetic. No functional change. >> This will ease the next patch where we re-indent an if() statement. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/isa/isa-superio.c | 69 ++++++++++++++++++++++---------------------- >> 1 file changed, 34 insertions(+), 35 deletions(-) >> >> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c >> index d54463bf035..c6845eaf578 100644 >> --- a/hw/isa/isa-superio.c >> +++ b/hw/isa/isa-superio.c >> @@ -22,8 +22,8 @@ >> >> static void isa_superio_realize(DeviceState *dev, Error **errp) >> { >> - ISASuperIODevice *sio = ISA_SUPERIO(dev); >> - ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(sio); >> + ISASuperIODevice *s = ISA_SUPERIO(dev); >> + ISASuperIOClass *k = ISA_SUPERIO_GET_CLASS(s); > > Sorry, but I have to say that I normally really don't like single-letter > variable names. They are much harder to search for, and way less > self-describing. Me too ;) But sometimes I find the 80 chars/line limit generate indentation harder to review. But here I have no excuse :) > If you get problems with the line length in a later patch, what about > refactoring the related code into a separate function? So that you'd > only have something like this in the realize function: > > for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) { > if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) { > isa_superio_init_ide(...); > } > } Yes, fair enough. Note you used the shortened variables in your example! :P Regards, Phil. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH for-4.1 2/4] hw/isa/superio: Support more than one IDE bus 2019-04-04 22:12 [Qemu-devel] [PATCH for-4.1 0/4] hw/mips/r4k: Refactor the Super I/O chipset Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable Philippe Mathieu-Daudé @ 2019-04-04 22:12 ` Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset Philippe Mathieu-Daudé 3 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-04 22:12 UTC (permalink / raw) To: qemu-devel, Thomas Huth Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini, Philippe Mathieu-Daudé The current code is limited to a single IDE bus (supporting two IDE drives). Some Super I/O chipset provide two IDE buses (four IDE drives). Modify the model to support more that one IDE bus. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/isa/isa-superio.c | 43 ++++++++++++++++++++++------------------ include/hw/isa/superio.h | 2 +- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c index c6845eaf578..b0761ea1f96 100644 --- a/hw/isa/isa-superio.c +++ b/hw/isa/isa-superio.c @@ -18,6 +18,7 @@ #include "hw/isa/superio.h" #include "hw/input/i8042.h" #include "hw/char/serial.h" +#include "hw/ide.h" #include "trace.h" static void isa_superio_realize(DeviceState *dev, Error **errp) @@ -30,7 +31,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) Chardev *chr; DriveInfo *drive; char *name; - int i; + int i, j; /* Parallel port */ for (i = 0; i < k->parallel.count; i++) { @@ -146,25 +147,29 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) s->kbc = isa_create_simple(bus, TYPE_I8042); /* IDE */ - if (k->ide.count && (!k->ide.is_enabled || k->ide.is_enabled(s, 0))) { - isa = isa_create(bus, "isa-ide"); - d = DEVICE(isa); - if (k->ide.get_iobase) { - qdev_prop_set_uint32(d, "iobase", k->ide.get_iobase(s, 0)); - } - if (k->ide.get_iobase) { - qdev_prop_set_uint32(d, "iobase2", k->ide.get_iobase(s, 1)); - } - if (k->ide.get_irq) { - qdev_prop_set_uint32(d, "irq", k->ide.get_irq(s, 0)); + size_t bus_count = k->ide.count / MAX_IDE_DEVS; + s->ide = g_new(ISADevice *, bus_count); + for (i = 0, j = 0; i < bus_count; i++, j += MAX_IDE_DEVS) { + if (!k->ide.is_enabled || k->ide.is_enabled(s, j)) { + isa = isa_create(bus, "isa-ide"); + d = DEVICE(isa); + if (k->ide.get_iobase) { + qdev_prop_set_uint32(d, "iobase", k->ide.get_iobase(s, j)); + } + if (k->ide.get_iobase) { + qdev_prop_set_uint32(d, "iobase2", k->ide.get_iobase(s, j + 1)); + } + if (k->ide.get_irq) { + qdev_prop_set_uint32(d, "irq", k->ide.get_irq(s, j)); + } + qdev_init_nofail(d); + s->ide[i] = isa; + trace_superio_create_ide(i, + k->ide.get_iobase ? + k->ide.get_iobase(s, j) : -1, + k->ide.get_irq ? + k->ide.get_irq(s, j) : -1); } - qdev_init_nofail(d); - s->ide = isa; - trace_superio_create_ide(0, - k->ide.get_iobase ? - k->ide.get_iobase(s, 0) : -1, - k->ide.get_irq ? - k->ide.get_irq(s, 0) : -1); } } diff --git a/include/hw/isa/superio.h b/include/hw/isa/superio.h index 345f0060817..980a9691696 100644 --- a/include/hw/isa/superio.h +++ b/include/hw/isa/superio.h @@ -33,7 +33,7 @@ typedef struct ISASuperIODevice { ISADevice *serial[SUPERIO_MAX_SERIAL_PORTS]; ISADevice *floppy; ISADevice *kbc; - ISADevice *ide; + ISADevice **ide; } ISASuperIODevice; typedef struct ISASuperIOFuncs { -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller 2019-04-04 22:12 [Qemu-devel] [PATCH for-4.1 0/4] hw/mips/r4k: Refactor the Super I/O chipset Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 2/4] hw/isa/superio: Support more than one IDE bus Philippe Mathieu-Daudé @ 2019-04-04 22:12 ` Philippe Mathieu-Daudé 2019-04-05 4:24 ` Thomas Huth 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset Philippe Mathieu-Daudé 3 siblings, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-04 22:12 UTC (permalink / raw) To: qemu-devel, Thomas Huth Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini, Philippe Mathieu-Daudé Not all Super I/O chipsets provide a Floppy Disk Controller. Without this change, using a Super I/O with no FDC would abort QEMU with: Initialization of device isa-fdc failed: ISA controller does not support DMA Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/isa/isa-superio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c index b0761ea1f96..6956f06d529 100644 --- a/hw/isa/isa-superio.c +++ b/hw/isa/isa-superio.c @@ -113,7 +113,8 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) } /* Floppy disc */ - if (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0)) { + if (k->floppy.count + && (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0))) { isa = isa_create(bus, "isa-fdc"); d = DEVICE(isa); if (k->floppy.get_iobase) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller @ 2019-04-05 4:24 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2019-04-05 4:24 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: > Not all Super I/O chipsets provide a Floppy Disk Controller. > > Without this change, using a Super I/O with no FDC would abort QEMU with: > > Initialization of device isa-fdc failed: ISA controller does not support DMA > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/isa/isa-superio.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c > index b0761ea1f96..6956f06d529 100644 > --- a/hw/isa/isa-superio.c > +++ b/hw/isa/isa-superio.c > @@ -113,7 +113,8 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) > } > > /* Floppy disc */ > - if (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0)) { > + if (k->floppy.count > + && (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0))) { > isa = isa_create(bus, "isa-fdc"); > d = DEVICE(isa); > if (k->floppy.get_iobase) { Reviewed-by: Thomas Huth <thuth@redhat.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller @ 2019-04-05 4:24 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2019-04-05 4:24 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Aleksandar Rikalo, Paolo Bonzini, Aleksandar Markovic, Aurelien Jarno, Michael S. Tsirkin On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: > Not all Super I/O chipsets provide a Floppy Disk Controller. > > Without this change, using a Super I/O with no FDC would abort QEMU with: > > Initialization of device isa-fdc failed: ISA controller does not support DMA > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/isa/isa-superio.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c > index b0761ea1f96..6956f06d529 100644 > --- a/hw/isa/isa-superio.c > +++ b/hw/isa/isa-superio.c > @@ -113,7 +113,8 @@ static void isa_superio_realize(DeviceState *dev, Error **errp) > } > > /* Floppy disc */ > - if (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0)) { > + if (k->floppy.count > + && (!k->floppy.is_enabled || k->floppy.is_enabled(s, 0))) { > isa = isa_create(bus, "isa-fdc"); > d = DEVICE(isa); > if (k->floppy.get_iobase) { Reviewed-by: Thomas Huth <thuth@redhat.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset 2019-04-04 22:12 [Qemu-devel] [PATCH for-4.1 0/4] hw/mips/r4k: Refactor the Super I/O chipset Philippe Mathieu-Daudé ` (2 preceding siblings ...) 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller Philippe Mathieu-Daudé @ 2019-04-04 22:12 ` Philippe Mathieu-Daudé 2019-04-05 4:51 ` Thomas Huth 3 siblings, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-04 22:12 UTC (permalink / raw) To: qemu-devel, Thomas Huth Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini, Philippe Mathieu-Daudé ISA Super I/O are already modeled by the ISASuperIODevice abstract device. Since this board uses a generic ISA Super I/O chipset, refactor it as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 16 deletions(-) diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c index 93dbf76bb49..b51a9523b43 100644 --- a/hw/mips/mips_r4k.c +++ b/hw/mips/mips_r4k.c @@ -18,6 +18,7 @@ #include "hw/i386/pc.h" #include "hw/char/serial.h" #include "hw/isa/isa.h" +#include "hw/isa/superio.h" #include "net/net.h" #include "hw/net/ne2000-isa.h" #include "sysemu/sysemu.h" @@ -29,7 +30,6 @@ #include "hw/loader.h" #include "elf.h" #include "hw/timer/mc146818rtc.h" -#include "hw/input/i8042.h" #include "hw/timer/i8254.h" #include "exec/address-spaces.h" #include "sysemu/qtest.h" @@ -37,10 +37,6 @@ #define MAX_IDE_BUS 2 -static const int ide_iobase[2] = { 0x1f0, 0x170 }; -static const int ide_iobase2[2] = { 0x3f6, 0x376 }; -static const int ide_irq[2] = { 14, 15 }; - static ISADevice *pit; /* PIT i8254 */ /* i8254 PIT is attached to the IRQ0 at PIC i8259 */ @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; +#define TYPE_R4K_SUPERIO "r4k-superio" + +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index) +{ + static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 }; + + return ide_iobase[index]; +} + +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index) +{ + return index < MAX_IDE_DEVS ? 14 : 15; +} + +static void r4k_superio_class_init(ObjectClass *klass, void *data) +{ + ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass); + + sc->serial.count = MAX_ISA_SERIAL_PORTS; + sc->parallel.count = 0; + sc->floppy.count = 0; + sc->ide = (ISASuperIOFuncs){ + .count = MAX_IDE_BUS * MAX_IDE_DEVS, + .get_iobase = get_ide_iobase, + .get_irq = get_ide_irq, + }; +} + +static const TypeInfo r4k_superio_type_info = { + .name = TYPE_R4K_SUPERIO, + .parent = TYPE_ISA_SUPERIO, + .instance_size = sizeof(ISASuperIODevice), + .class_size = sizeof(ISASuperIOClass), + .class_init = r4k_superio_class_init, +}; + +static void r4k_superio_register_types(void) +{ + type_register_static(&r4k_superio_type_info); +} + +type_init(r4k_superio_register_types) + typedef struct ResetData { MIPSCPU *cpu; uint64_t vector; @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine) MIPSCPU *cpu; CPUMIPSState *env; ResetData *reset_info; - int i; qemu_irq *i8259; ISABus *isa_bus; - DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; DriveInfo *dinfo; int be; @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine) pit = i8254_pit_init(isa_bus, 0x40, 0, NULL); - serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS); - isa_vga_init(isa_bus); if (nd_table[0].used) isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]); - ide_drive_get(hd, ARRAY_SIZE(hd)); - for(i = 0; i < MAX_IDE_BUS; i++) - isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i], - hd[MAX_IDE_DEVS * i], - hd[MAX_IDE_DEVS * i + 1]); - - isa_create_simple(isa_bus, TYPE_I8042); + isa_create_simple(isa_bus, TYPE_R4K_SUPERIO); } static void mips_machine_init(MachineClass *mc) -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset @ 2019-04-05 4:51 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2019-04-05 4:51 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: > ISA Super I/O are already modeled by the ISASuperIODevice abstract > device. > Since this board uses a generic ISA Super I/O chipset, refactor it > as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice. Good idea! > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------ > 1 file changed, 45 insertions(+), 16 deletions(-) > > diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c > index 93dbf76bb49..b51a9523b43 100644 > --- a/hw/mips/mips_r4k.c > +++ b/hw/mips/mips_r4k.c > @@ -18,6 +18,7 @@ > #include "hw/i386/pc.h" > #include "hw/char/serial.h" > #include "hw/isa/isa.h" > +#include "hw/isa/superio.h" > #include "net/net.h" > #include "hw/net/ne2000-isa.h" > #include "sysemu/sysemu.h" > @@ -29,7 +30,6 @@ > #include "hw/loader.h" > #include "elf.h" > #include "hw/timer/mc146818rtc.h" > -#include "hw/input/i8042.h" > #include "hw/timer/i8254.h" > #include "exec/address-spaces.h" > #include "sysemu/qtest.h" > @@ -37,10 +37,6 @@ > > #define MAX_IDE_BUS 2 > > -static const int ide_iobase[2] = { 0x1f0, 0x170 }; > -static const int ide_iobase2[2] = { 0x3f6, 0x376 }; > -static const int ide_irq[2] = { 14, 15 }; > - > static ISADevice *pit; /* PIT i8254 */ > > /* i8254 PIT is attached to the IRQ0 at PIC i8259 */ > @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > +#define TYPE_R4K_SUPERIO "r4k-superio" > + > +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index) > +{ > + static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 }; > + > + return ide_iobase[index]; > +} > + > +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index) > +{ > + return index < MAX_IDE_DEVS ? 14 : 15; > +} > + > +static void r4k_superio_class_init(ObjectClass *klass, void *data) > +{ > + ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass); > + > + sc->serial.count = MAX_ISA_SERIAL_PORTS; > + sc->parallel.count = 0; > + sc->floppy.count = 0; > + sc->ide = (ISASuperIOFuncs){ > + .count = MAX_IDE_BUS * MAX_IDE_DEVS, > + .get_iobase = get_ide_iobase, > + .get_irq = get_ide_irq, > + }; > +} > + > +static const TypeInfo r4k_superio_type_info = { > + .name = TYPE_R4K_SUPERIO, > + .parent = TYPE_ISA_SUPERIO, > + .instance_size = sizeof(ISASuperIODevice), > + .class_size = sizeof(ISASuperIOClass), > + .class_init = r4k_superio_class_init, > +}; > + > +static void r4k_superio_register_types(void) > +{ > + type_register_static(&r4k_superio_type_info); > +} > + > +type_init(r4k_superio_register_types) > + > typedef struct ResetData { > MIPSCPU *cpu; > uint64_t vector; > @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine) > MIPSCPU *cpu; > CPUMIPSState *env; > ResetData *reset_info; > - int i; > qemu_irq *i8259; > ISABus *isa_bus; > - DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; > DriveInfo *dinfo; > int be; > > @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine) > > pit = i8254_pit_init(isa_bus, 0x40, 0, NULL); > > - serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS); > - > isa_vga_init(isa_bus); > > if (nd_table[0].used) > isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]); > > - ide_drive_get(hd, ARRAY_SIZE(hd)); > - for(i = 0; i < MAX_IDE_BUS; i++) > - isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i], > - hd[MAX_IDE_DEVS * i], > - hd[MAX_IDE_DEVS * i + 1]); > - > - isa_create_simple(isa_bus, TYPE_I8042); > + isa_create_simple(isa_bus, TYPE_R4K_SUPERIO); What about the ide_drive_get() and the ide_create_drive() that is done by isa_ide_init() internally? As far as I can see, the superio code does not do this job for you? So don't you have to do that manually here after creating the R4K_SUPERIO device? Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset @ 2019-04-05 4:51 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2019-04-05 4:51 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Aleksandar Rikalo, Paolo Bonzini, Aleksandar Markovic, Aurelien Jarno, Michael S. Tsirkin On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: > ISA Super I/O are already modeled by the ISASuperIODevice abstract > device. > Since this board uses a generic ISA Super I/O chipset, refactor it > as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice. Good idea! > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------ > 1 file changed, 45 insertions(+), 16 deletions(-) > > diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c > index 93dbf76bb49..b51a9523b43 100644 > --- a/hw/mips/mips_r4k.c > +++ b/hw/mips/mips_r4k.c > @@ -18,6 +18,7 @@ > #include "hw/i386/pc.h" > #include "hw/char/serial.h" > #include "hw/isa/isa.h" > +#include "hw/isa/superio.h" > #include "net/net.h" > #include "hw/net/ne2000-isa.h" > #include "sysemu/sysemu.h" > @@ -29,7 +30,6 @@ > #include "hw/loader.h" > #include "elf.h" > #include "hw/timer/mc146818rtc.h" > -#include "hw/input/i8042.h" > #include "hw/timer/i8254.h" > #include "exec/address-spaces.h" > #include "sysemu/qtest.h" > @@ -37,10 +37,6 @@ > > #define MAX_IDE_BUS 2 > > -static const int ide_iobase[2] = { 0x1f0, 0x170 }; > -static const int ide_iobase2[2] = { 0x3f6, 0x376 }; > -static const int ide_irq[2] = { 14, 15 }; > - > static ISADevice *pit; /* PIT i8254 */ > > /* i8254 PIT is attached to the IRQ0 at PIC i8259 */ > @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > +#define TYPE_R4K_SUPERIO "r4k-superio" > + > +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index) > +{ > + static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 }; > + > + return ide_iobase[index]; > +} > + > +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index) > +{ > + return index < MAX_IDE_DEVS ? 14 : 15; > +} > + > +static void r4k_superio_class_init(ObjectClass *klass, void *data) > +{ > + ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass); > + > + sc->serial.count = MAX_ISA_SERIAL_PORTS; > + sc->parallel.count = 0; > + sc->floppy.count = 0; > + sc->ide = (ISASuperIOFuncs){ > + .count = MAX_IDE_BUS * MAX_IDE_DEVS, > + .get_iobase = get_ide_iobase, > + .get_irq = get_ide_irq, > + }; > +} > + > +static const TypeInfo r4k_superio_type_info = { > + .name = TYPE_R4K_SUPERIO, > + .parent = TYPE_ISA_SUPERIO, > + .instance_size = sizeof(ISASuperIODevice), > + .class_size = sizeof(ISASuperIOClass), > + .class_init = r4k_superio_class_init, > +}; > + > +static void r4k_superio_register_types(void) > +{ > + type_register_static(&r4k_superio_type_info); > +} > + > +type_init(r4k_superio_register_types) > + > typedef struct ResetData { > MIPSCPU *cpu; > uint64_t vector; > @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine) > MIPSCPU *cpu; > CPUMIPSState *env; > ResetData *reset_info; > - int i; > qemu_irq *i8259; > ISABus *isa_bus; > - DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; > DriveInfo *dinfo; > int be; > > @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine) > > pit = i8254_pit_init(isa_bus, 0x40, 0, NULL); > > - serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS); > - > isa_vga_init(isa_bus); > > if (nd_table[0].used) > isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]); > > - ide_drive_get(hd, ARRAY_SIZE(hd)); > - for(i = 0; i < MAX_IDE_BUS; i++) > - isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i], > - hd[MAX_IDE_DEVS * i], > - hd[MAX_IDE_DEVS * i + 1]); > - > - isa_create_simple(isa_bus, TYPE_I8042); > + isa_create_simple(isa_bus, TYPE_R4K_SUPERIO); What about the ide_drive_get() and the ide_create_drive() that is done by isa_ide_init() internally? As far as I can see, the superio code does not do this job for you? So don't you have to do that manually here after creating the R4K_SUPERIO device? Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset @ 2019-04-05 9:49 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-05 9:49 UTC (permalink / raw) To: Thomas Huth, qemu-devel Cc: Aurelien Jarno, Michael S. Tsirkin, Aleksandar Markovic, Aleksandar Rikalo, Paolo Bonzini On 4/5/19 6:51 AM, Thomas Huth wrote: > On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: >> ISA Super I/O are already modeled by the ISASuperIODevice abstract >> device. >> Since this board uses a generic ISA Super I/O chipset, refactor it >> as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice. > > Good idea! > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 45 insertions(+), 16 deletions(-) >> >> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c >> index 93dbf76bb49..b51a9523b43 100644 >> --- a/hw/mips/mips_r4k.c >> +++ b/hw/mips/mips_r4k.c >> @@ -18,6 +18,7 @@ >> #include "hw/i386/pc.h" >> #include "hw/char/serial.h" >> #include "hw/isa/isa.h" >> +#include "hw/isa/superio.h" >> #include "net/net.h" >> #include "hw/net/ne2000-isa.h" >> #include "sysemu/sysemu.h" >> @@ -29,7 +30,6 @@ >> #include "hw/loader.h" >> #include "elf.h" >> #include "hw/timer/mc146818rtc.h" >> -#include "hw/input/i8042.h" >> #include "hw/timer/i8254.h" >> #include "exec/address-spaces.h" >> #include "sysemu/qtest.h" >> @@ -37,10 +37,6 @@ >> >> #define MAX_IDE_BUS 2 >> >> -static const int ide_iobase[2] = { 0x1f0, 0x170 }; >> -static const int ide_iobase2[2] = { 0x3f6, 0x376 }; >> -static const int ide_irq[2] = { 14, 15 }; >> - >> static ISADevice *pit; /* PIT i8254 */ >> >> /* i8254 PIT is attached to the IRQ0 at PIC i8259 */ >> @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = { >> .endianness = DEVICE_NATIVE_ENDIAN, >> }; >> >> +#define TYPE_R4K_SUPERIO "r4k-superio" >> + >> +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index) >> +{ >> + static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 }; >> + >> + return ide_iobase[index]; >> +} >> + >> +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index) >> +{ >> + return index < MAX_IDE_DEVS ? 14 : 15; >> +} >> + >> +static void r4k_superio_class_init(ObjectClass *klass, void *data) >> +{ >> + ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass); >> + >> + sc->serial.count = MAX_ISA_SERIAL_PORTS; >> + sc->parallel.count = 0; >> + sc->floppy.count = 0; >> + sc->ide = (ISASuperIOFuncs){ >> + .count = MAX_IDE_BUS * MAX_IDE_DEVS, >> + .get_iobase = get_ide_iobase, >> + .get_irq = get_ide_irq, >> + }; >> +} >> + >> +static const TypeInfo r4k_superio_type_info = { >> + .name = TYPE_R4K_SUPERIO, >> + .parent = TYPE_ISA_SUPERIO, >> + .instance_size = sizeof(ISASuperIODevice), >> + .class_size = sizeof(ISASuperIOClass), >> + .class_init = r4k_superio_class_init, >> +}; >> + >> +static void r4k_superio_register_types(void) >> +{ >> + type_register_static(&r4k_superio_type_info); >> +} >> + >> +type_init(r4k_superio_register_types) >> + >> typedef struct ResetData { >> MIPSCPU *cpu; >> uint64_t vector; >> @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine) >> MIPSCPU *cpu; >> CPUMIPSState *env; >> ResetData *reset_info; >> - int i; >> qemu_irq *i8259; >> ISABus *isa_bus; >> - DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; >> DriveInfo *dinfo; >> int be; >> >> @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine) >> >> pit = i8254_pit_init(isa_bus, 0x40, 0, NULL); >> >> - serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS); >> - >> isa_vga_init(isa_bus); >> >> if (nd_table[0].used) >> isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]); >> >> - ide_drive_get(hd, ARRAY_SIZE(hd)); >> - for(i = 0; i < MAX_IDE_BUS; i++) >> - isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i], >> - hd[MAX_IDE_DEVS * i], >> - hd[MAX_IDE_DEVS * i + 1]); >> - >> - isa_create_simple(isa_bus, TYPE_I8042); >> + isa_create_simple(isa_bus, TYPE_R4K_SUPERIO); > > What about the ide_drive_get() and the ide_create_drive() that is done > by isa_ide_init() internally? As far as I can see, the superio code does > not do this job for you? So don't you have to do that manually here > after creating the R4K_SUPERIO device? Oops... I was scared I did the same mistake with the Floppy controller in 7313b1f28be but hopefully not. However you made me notice I never considered the case MAX_FD>2. I'll send a mail asking if this case is still used. Thanks! Phil. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset @ 2019-04-05 9:49 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-05 9:49 UTC (permalink / raw) To: Thomas Huth, qemu-devel Cc: Aleksandar Rikalo, Paolo Bonzini, Aleksandar Markovic, Aurelien Jarno, Michael S. Tsirkin On 4/5/19 6:51 AM, Thomas Huth wrote: > On 05/04/2019 00.12, Philippe Mathieu-Daudé wrote: >> ISA Super I/O are already modeled by the ISASuperIODevice abstract >> device. >> Since this board uses a generic ISA Super I/O chipset, refactor it >> as the TYPE_R4K_SUPERIO device, child of ISASuperIODevice. > > Good idea! > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/mips/mips_r4k.c | 61 ++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 45 insertions(+), 16 deletions(-) >> >> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c >> index 93dbf76bb49..b51a9523b43 100644 >> --- a/hw/mips/mips_r4k.c >> +++ b/hw/mips/mips_r4k.c >> @@ -18,6 +18,7 @@ >> #include "hw/i386/pc.h" >> #include "hw/char/serial.h" >> #include "hw/isa/isa.h" >> +#include "hw/isa/superio.h" >> #include "net/net.h" >> #include "hw/net/ne2000-isa.h" >> #include "sysemu/sysemu.h" >> @@ -29,7 +30,6 @@ >> #include "hw/loader.h" >> #include "elf.h" >> #include "hw/timer/mc146818rtc.h" >> -#include "hw/input/i8042.h" >> #include "hw/timer/i8254.h" >> #include "exec/address-spaces.h" >> #include "sysemu/qtest.h" >> @@ -37,10 +37,6 @@ >> >> #define MAX_IDE_BUS 2 >> >> -static const int ide_iobase[2] = { 0x1f0, 0x170 }; >> -static const int ide_iobase2[2] = { 0x3f6, 0x376 }; >> -static const int ide_irq[2] = { 14, 15 }; >> - >> static ISADevice *pit; /* PIT i8254 */ >> >> /* i8254 PIT is attached to the IRQ0 at PIC i8259 */ >> @@ -73,6 +69,49 @@ static const MemoryRegionOps mips_qemu_ops = { >> .endianness = DEVICE_NATIVE_ENDIAN, >> }; >> >> +#define TYPE_R4K_SUPERIO "r4k-superio" >> + >> +static uint16_t get_ide_iobase(ISASuperIODevice *sio, uint8_t index) >> +{ >> + static const uint16_t ide_iobase[] = { 0x1f0, 0x3f6, 0x170, 0x376 }; >> + >> + return ide_iobase[index]; >> +} >> + >> +static unsigned int get_ide_irq(ISASuperIODevice *sio, uint8_t index) >> +{ >> + return index < MAX_IDE_DEVS ? 14 : 15; >> +} >> + >> +static void r4k_superio_class_init(ObjectClass *klass, void *data) >> +{ >> + ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass); >> + >> + sc->serial.count = MAX_ISA_SERIAL_PORTS; >> + sc->parallel.count = 0; >> + sc->floppy.count = 0; >> + sc->ide = (ISASuperIOFuncs){ >> + .count = MAX_IDE_BUS * MAX_IDE_DEVS, >> + .get_iobase = get_ide_iobase, >> + .get_irq = get_ide_irq, >> + }; >> +} >> + >> +static const TypeInfo r4k_superio_type_info = { >> + .name = TYPE_R4K_SUPERIO, >> + .parent = TYPE_ISA_SUPERIO, >> + .instance_size = sizeof(ISASuperIODevice), >> + .class_size = sizeof(ISASuperIOClass), >> + .class_init = r4k_superio_class_init, >> +}; >> + >> +static void r4k_superio_register_types(void) >> +{ >> + type_register_static(&r4k_superio_type_info); >> +} >> + >> +type_init(r4k_superio_register_types) >> + >> typedef struct ResetData { >> MIPSCPU *cpu; >> uint64_t vector; >> @@ -179,10 +218,8 @@ void mips_r4k_init(MachineState *machine) >> MIPSCPU *cpu; >> CPUMIPSState *env; >> ResetData *reset_info; >> - int i; >> qemu_irq *i8259; >> ISABus *isa_bus; >> - DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; >> DriveInfo *dinfo; >> int be; >> >> @@ -274,20 +311,12 @@ void mips_r4k_init(MachineState *machine) >> >> pit = i8254_pit_init(isa_bus, 0x40, 0, NULL); >> >> - serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS); >> - >> isa_vga_init(isa_bus); >> >> if (nd_table[0].used) >> isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]); >> >> - ide_drive_get(hd, ARRAY_SIZE(hd)); >> - for(i = 0; i < MAX_IDE_BUS; i++) >> - isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i], >> - hd[MAX_IDE_DEVS * i], >> - hd[MAX_IDE_DEVS * i + 1]); >> - >> - isa_create_simple(isa_bus, TYPE_I8042); >> + isa_create_simple(isa_bus, TYPE_R4K_SUPERIO); > > What about the ide_drive_get() and the ide_create_drive() that is done > by isa_ide_init() internally? As far as I can see, the superio code does > not do this job for you? So don't you have to do that manually here > after creating the R4K_SUPERIO device? Oops... I was scared I did the same mistake with the Floppy controller in 7313b1f28be but hopefully not. However you made me notice I never considered the case MAX_FD>2. I'll send a mail asking if this case is still used. Thanks! Phil. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-04-05 9:50 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-04 22:12 [Qemu-devel] [PATCH for-4.1 0/4] hw/mips/r4k: Refactor the Super I/O chipset Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 1/4] hw/isa/superio: Rename a variable Philippe Mathieu-Daudé 2019-04-05 4:14 ` Thomas Huth 2019-04-05 4:14 ` Thomas Huth 2019-04-05 9:32 ` Philippe Mathieu-Daudé 2019-04-05 9:32 ` Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 2/4] hw/isa/superio: Support more than one IDE bus Philippe Mathieu-Daudé 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 3/4] hw/isa/superio: Support chipsets with no Floppy Disk controller Philippe Mathieu-Daudé 2019-04-05 4:24 ` Thomas Huth 2019-04-05 4:24 ` Thomas Huth 2019-04-04 22:12 ` [Qemu-devel] [PATCH for-4.1 4/4] hw/mips/r4k: Refactor the Super I/O chipset Philippe Mathieu-Daudé 2019-04-05 4:51 ` Thomas Huth 2019-04-05 4:51 ` Thomas Huth 2019-04-05 9:49 ` Philippe Mathieu-Daudé 2019-04-05 9:49 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.