* [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
* [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
* [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 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 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
* 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 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
* 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.