* [PATCH v3 00/13] Misc ppc/mac machines clean up
@ 2022-10-03 20:13 BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 01/13] mac_newworld: Drop some variables BALATON Zoltan
` (13 more replies)
0 siblings, 14 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
This series includes some clean ups to mac_newworld and mac_oldworld
to make them a bit simpler and more readable, It also removes the
shared mac.h file that turns out was more of a random collection of
unrelated things. Getting rid of this mac.h improves the locality of
device models and reduces unnecessary interdependency.
v3: Some more patch spliting and changes I've noticed and address more
review comments
v2: Split some patches and add a few more I've noticed now and address
review comments
BALATON Zoltan (13):
mac_newworld: Drop some variables
mac_oldworld: Drop some more variables
mac_{old|new}world: Set tbfreq at declaration
mac_{old|new}world: Avoid else branch by setting default value
mac_{old|new}world: Simplify cmdline_base calculation
mac_newworld: Clean up creation of Uninorth devices
mac_{old|new}world: Reduce number of QOM casts
hw/ppc/mac.h: Move newworld specific parts out from shared header
hw/ppc/mac.h: Move macio specific parts out from shared header
hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header
hw/ppc/mac.h: Move PROM and KERNEL defines to board code
hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h
mac_nvram: Use NVRAM_SIZE constant
MAINTAINERS | 2 +
hw/ide/macio.c | 1 -
hw/intc/heathrow_pic.c | 1 -
hw/intc/openpic.c | 1 -
hw/misc/macio/cuda.c | 1 -
hw/misc/macio/gpio.c | 1 -
hw/misc/macio/macio.c | 8 +-
hw/misc/macio/pmu.c | 1 -
hw/nvram/mac_nvram.c | 2 +-
hw/pci-host/grackle.c | 15 +--
hw/pci-host/uninorth.c | 1 -
hw/ppc/mac.h | 105 ----------------
hw/ppc/mac_newworld.c | 225 ++++++++++++++++------------------
hw/ppc/mac_oldworld.c | 111 +++++++----------
include/hw/misc/macio/macio.h | 23 +++-
include/hw/nvram/mac_nvram.h | 51 ++++++++
include/hw/pci-host/grackle.h | 44 +++++++
17 files changed, 280 insertions(+), 313 deletions(-)
delete mode 100644 hw/ppc/mac.h
create mode 100644 include/hw/nvram/mac_nvram.h
create mode 100644 include/hw/pci-host/grackle.h
--
2.30.4
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 01/13] mac_newworld: Drop some variables
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 02/13] mac_oldworld: Drop some more variables BALATON Zoltan
` (12 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 7766 bytes --]
Values not used frequently enough may not worth putting in a local
variable, especially with names almost as long as the original value
because that does not improve readability, to the contrary it makes it
harder to see what value is used. Drop a few such variables. This is
the same clean up that was done for mac_oldworld in commit b8df32555ce5.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/ppc/mac_newworld.c | 65 +++++++++++++++++++------------------------
1 file changed, 29 insertions(+), 36 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index cf7eb72391..27e4e8d136 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -106,18 +106,13 @@ static void ppc_core99_reset(void *opaque)
/* PowerPC Mac99 hardware initialisation */
static void ppc_core99_init(MachineState *machine)
{
- ram_addr_t ram_size = machine->ram_size;
- const char *bios_name = machine->firmware ?: PROM_FILENAME;
- const char *kernel_filename = machine->kernel_filename;
- const char *kernel_cmdline = machine->kernel_cmdline;
- const char *initrd_filename = machine->initrd_filename;
- const char *boot_device = machine->boot_config.order;
Core99MachineState *core99_machine = CORE99_MACHINE(machine);
PowerPCCPU *cpu = NULL;
CPUPPCState *env = NULL;
char *filename;
IrqLines *openpic_irqs;
- int linux_boot, i, j, k;
+ int i, j, k, ppc_boot_device, machine_arch, bios_size;
+ const char *bios_name = machine->firmware ?: PROM_FILENAME;
MemoryRegion *bios = g_new(MemoryRegion, 1);
hwaddr kernel_base, initrd_base, cmdline_base = 0;
long kernel_size, initrd_size;
@@ -129,22 +124,16 @@ static void ppc_core99_init(MachineState *machine)
MACIOIDEState *macio_ide;
BusState *adb_bus;
MacIONVRAMState *nvr;
- int bios_size;
- int ppc_boot_device;
DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
void *fw_cfg;
- int machine_arch;
SysBusDevice *s;
DeviceState *dev, *pic_dev;
DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
hwaddr nvram_addr = 0xFFF04000;
uint64_t tbfreq;
- unsigned int smp_cpus = machine->smp.cpus;
-
- linux_boot = (kernel_filename != NULL);
/* init CPUs */
- for (i = 0; i < smp_cpus; i++) {
+ for (i = 0; i < machine->smp.cpus; i++) {
cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
env = &cpu->env;
@@ -184,7 +173,7 @@ static void ppc_core99_init(MachineState *machine)
exit(1);
}
- if (linux_boot) {
+ if (machine->kernel_filename) {
int bswap_needed;
#ifdef BSWAP_NEEDED
@@ -194,29 +183,31 @@ static void ppc_core99_init(MachineState *machine)
#endif
kernel_base = KERNEL_LOAD_ADDR;
- kernel_size = load_elf(kernel_filename, NULL,
+ kernel_size = load_elf(machine->kernel_filename, NULL,
translate_kernel_address, NULL, NULL, NULL,
NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
if (kernel_size < 0)
- kernel_size = load_aout(kernel_filename, kernel_base,
- ram_size - kernel_base, bswap_needed,
- TARGET_PAGE_SIZE);
+ kernel_size = load_aout(machine->kernel_filename, kernel_base,
+ machine->ram_size - kernel_base,
+ bswap_needed, TARGET_PAGE_SIZE);
if (kernel_size < 0)
- kernel_size = load_image_targphys(kernel_filename,
+ kernel_size = load_image_targphys(machine->kernel_filename,
kernel_base,
- ram_size - kernel_base);
+ machine->ram_size - kernel_base);
if (kernel_size < 0) {
- error_report("could not load kernel '%s'", kernel_filename);
+ error_report("could not load kernel '%s'",
+ machine->kernel_filename);
exit(1);
}
/* load initrd */
- if (initrd_filename) {
+ if (machine->initrd_filename) {
initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
- initrd_size = load_image_targphys(initrd_filename, initrd_base,
- ram_size - initrd_base);
+ initrd_size = load_image_targphys(machine->initrd_filename,
+ initrd_base,
+ machine->ram_size - initrd_base);
if (initrd_size < 0) {
error_report("could not load initial ram disk '%s'",
- initrd_filename);
+ machine->initrd_filename);
exit(1);
}
cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
@@ -235,9 +226,10 @@ static void ppc_core99_init(MachineState *machine)
/* We consider that NewWorld PowerMac never have any floppy drive
* For now, OHW cannot boot from the network.
*/
- for (i = 0; boot_device[i] != '\0'; i++) {
- if (boot_device[i] >= 'c' && boot_device[i] <= 'f') {
- ppc_boot_device = boot_device[i];
+ for (i = 0; machine->boot_config.order[i] != '\0'; i++) {
+ if (machine->boot_config.order[i] >= 'c' &&
+ machine->boot_config.order[i] <= 'f') {
+ ppc_boot_device = machine->boot_config.order[i];
break;
}
}
@@ -254,8 +246,8 @@ static void ppc_core99_init(MachineState *machine)
memory_region_add_subregion(get_system_memory(), 0xf8000000,
sysbus_mmio_get_region(s, 0));
- openpic_irqs = g_new0(IrqLines, smp_cpus);
- for (i = 0; i < smp_cpus; i++) {
+ openpic_irqs = g_new0(IrqLines, machine->smp.cpus);
+ for (i = 0; i < machine->smp.cpus; i++) {
/* Mac99 IRQ connection between OpenPIC outputs pins
* and PowerPC input pins
*/
@@ -398,7 +390,7 @@ static void ppc_core99_init(MachineState *machine)
/* OpenPIC */
s = SYS_BUS_DEVICE(pic_dev);
k = 0;
- for (i = 0; i < smp_cpus; i++) {
+ for (i = 0; i < machine->smp.cpus; i++) {
for (j = 0; j < OPENPIC_OUTPUT_NB; j++) {
sysbus_connect_irq(s, k++, openpic_irqs[i].irq[j]);
}
@@ -480,15 +472,16 @@ static void ppc_core99_init(MachineState *machine)
sysbus_mmio_map(s, 0, CFG_ADDR);
sysbus_mmio_map(s, 1, CFG_ADDR + 2);
- fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
+ fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)machine->smp.cpus);
fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)machine->smp.max_cpus);
- fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
+ fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, kernel_base);
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
- if (kernel_cmdline) {
+ if (machine->kernel_cmdline) {
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, cmdline_base);
- pstrcpy_targphys("cmdline", cmdline_base, TARGET_PAGE_SIZE, kernel_cmdline);
+ pstrcpy_targphys("cmdline", cmdline_base, TARGET_PAGE_SIZE,
+ machine->kernel_cmdline);
} else {
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, 0);
}
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 02/13] mac_oldworld: Drop some more variables
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 01/13] mac_newworld: Drop some variables BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 03/13] mac_{old|new}world: Set tbfreq at declaration BALATON Zoltan
` (11 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6091 bytes --]
Drop some more local variables additionally to commit b8df32555ce5 to
match clean ups done to mac_newwold in previous patch.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/ppc/mac_oldworld.c | 43 +++++++++++++++++++++----------------------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 03732ca7ed..86512d31ad 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -80,14 +80,13 @@ static void ppc_heathrow_reset(void *opaque)
static void ppc_heathrow_init(MachineState *machine)
{
- ram_addr_t ram_size = machine->ram_size;
const char *bios_name = machine->firmware ?: PROM_FILENAME;
- const char *boot_device = machine->boot_config.order;
PowerPCCPU *cpu = NULL;
CPUPPCState *env = NULL;
char *filename;
- int i;
+ int i, bios_size;
MemoryRegion *bios = g_new(MemoryRegion, 1);
+ uint64_t bios_addr;
uint32_t kernel_base, initrd_base, cmdline_base = 0;
int32_t kernel_size, initrd_size;
PCIBus *pci_bus;
@@ -97,16 +96,13 @@ static void ppc_heathrow_init(MachineState *machine)
SysBusDevice *s;
DeviceState *dev, *pic_dev, *grackle_dev;
BusState *adb_bus;
- uint64_t bios_addr;
- int bios_size;
- unsigned int smp_cpus = machine->smp.cpus;
uint16_t ppc_boot_device;
DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
void *fw_cfg;
uint64_t tbfreq;
/* init CPUs */
- for (i = 0; i < smp_cpus; i++) {
+ for (i = 0; i < machine->smp.cpus; i++) {
cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
env = &cpu->env;
@@ -116,9 +112,9 @@ static void ppc_heathrow_init(MachineState *machine)
}
/* allocate RAM */
- if (ram_size > 2047 * MiB) {
+ if (machine->ram_size > 2047 * MiB) {
error_report("Too much memory for this machine: %" PRId64 " MB, "
- "maximum 2047 MB", ram_size / MiB);
+ "maximum 2047 MB", machine->ram_size / MiB);
exit(1);
}
@@ -165,12 +161,12 @@ static void ppc_heathrow_init(MachineState *machine)
NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
if (kernel_size < 0)
kernel_size = load_aout(machine->kernel_filename, kernel_base,
- ram_size - kernel_base, bswap_needed,
- TARGET_PAGE_SIZE);
+ machine->ram_size - kernel_base,
+ bswap_needed, TARGET_PAGE_SIZE);
if (kernel_size < 0)
kernel_size = load_image_targphys(machine->kernel_filename,
kernel_base,
- ram_size - kernel_base);
+ machine->ram_size - kernel_base);
if (kernel_size < 0) {
error_report("could not load kernel '%s'",
machine->kernel_filename);
@@ -182,7 +178,7 @@ static void ppc_heathrow_init(MachineState *machine)
KERNEL_GAP);
initrd_size = load_image_targphys(machine->initrd_filename,
initrd_base,
- ram_size - initrd_base);
+ machine->ram_size - initrd_base);
if (initrd_size < 0) {
error_report("could not load initial ram disk '%s'",
machine->initrd_filename);
@@ -201,19 +197,22 @@ static void ppc_heathrow_init(MachineState *machine)
initrd_base = 0;
initrd_size = 0;
ppc_boot_device = '\0';
- for (i = 0; boot_device[i] != '\0'; i++) {
- /* TOFIX: for now, the second IDE channel is not properly
+ for (i = 0; machine->boot_config.order[i] != '\0'; i++) {
+ /*
+ * TOFIX: for now, the second IDE channel is not properly
* used by OHW. The Mac floppy disk are not emulated.
* For now, OHW cannot boot from the network.
*/
#if 0
- if (boot_device[i] >= 'a' && boot_device[i] <= 'f') {
- ppc_boot_device = boot_device[i];
+ if (machine->boot_config.order[i] >= 'a' &&
+ machine->boot_config.order[i] <= 'f') {
+ ppc_boot_device = machine->boot_config.order[i];
break;
}
#else
- if (boot_device[i] >= 'c' && boot_device[i] <= 'd') {
- ppc_boot_device = boot_device[i];
+ if (machine->boot_config.order[i] >= 'c' &&
+ machine->boot_config.order[i] <= 'd') {
+ ppc_boot_device = machine->boot_config.order[i];
break;
}
#endif
@@ -266,7 +265,7 @@ static void ppc_heathrow_init(MachineState *machine)
}
/* Connect the heathrow PIC outputs to the 6xx bus */
- for (i = 0; i < smp_cpus; i++) {
+ for (i = 0; i < machine->smp.cpus; i++) {
switch (PPC_INPUT(env)) {
case PPC_FLAGS_INPUT_6xx:
/* XXX: we register only 1 output pin for heathrow PIC */
@@ -323,9 +322,9 @@ static void ppc_heathrow_init(MachineState *machine)
sysbus_mmio_map(s, 0, CFG_ADDR);
sysbus_mmio_map(s, 1, CFG_ADDR + 2);
- fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
+ fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)machine->smp.cpus);
fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)machine->smp.max_cpus);
- fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
+ fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, kernel_base);
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 03/13] mac_{old|new}world: Set tbfreq at declaration
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 01/13] mac_newworld: Drop some variables BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 02/13] mac_oldworld: Drop some more variables BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 04/13] mac_{old|new}world: Avoid else branch by setting default value BALATON Zoltan
` (10 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
The tbfreq variable is only set once in an if-else which can be done
at the variable declaration saving some lines of code and making it
simpler.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/ppc/mac_newworld.c | 9 +--------
hw/ppc/mac_oldworld.c | 9 +--------
2 files changed, 2 insertions(+), 16 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 27e4e8d136..6327694f85 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -130,7 +130,7 @@ static void ppc_core99_init(MachineState *machine)
DeviceState *dev, *pic_dev;
DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
hwaddr nvram_addr = 0xFFF04000;
- uint64_t tbfreq;
+ uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
/* init CPUs */
for (i = 0; i < machine->smp.cpus; i++) {
@@ -343,13 +343,6 @@ static void ppc_core99_init(MachineState *machine)
has_adb = (core99_machine->via_config == CORE99_VIA_CONFIG_CUDA ||
core99_machine->via_config == CORE99_VIA_CONFIG_PMU_ADB);
- /* Timebase Frequency */
- if (kvm_enabled()) {
- tbfreq = kvmppc_get_tbfreq();
- } else {
- tbfreq = TBFREQ;
- }
-
/* init basic PC hardware */
pci_bus = PCI_HOST_BRIDGE(uninorth_pci)->bus;
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 86512d31ad..5cabc410e7 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -99,7 +99,7 @@ static void ppc_heathrow_init(MachineState *machine)
uint16_t ppc_boot_device;
DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
void *fw_cfg;
- uint64_t tbfreq;
+ uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
/* init CPUs */
for (i = 0; i < machine->smp.cpus; i++) {
@@ -223,13 +223,6 @@ static void ppc_heathrow_init(MachineState *machine)
}
}
- /* Timebase Frequency */
- if (kvm_enabled()) {
- tbfreq = kvmppc_get_tbfreq();
- } else {
- tbfreq = TBFREQ;
- }
-
/* Grackle PCI host bridge */
grackle_dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000);
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 04/13] mac_{old|new}world: Avoid else branch by setting default value
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (2 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 03/13] mac_{old|new}world: Set tbfreq at declaration BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation BALATON Zoltan
` (9 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
Several variables are set in if-else branches where the else branch
can be removed by setting a default value at the variable declaration
which leads to simlpler code that is easier to follow.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ppc/mac_newworld.c | 19 ++++---------------
hw/ppc/mac_oldworld.c | 18 ++++--------------
2 files changed, 8 insertions(+), 29 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 6327694f85..6bc3bd19be 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -111,11 +111,11 @@ static void ppc_core99_init(MachineState *machine)
CPUPPCState *env = NULL;
char *filename;
IrqLines *openpic_irqs;
- int i, j, k, ppc_boot_device, machine_arch, bios_size;
+ int i, j, k, ppc_boot_device, machine_arch, bios_size = -1;
const char *bios_name = machine->firmware ?: PROM_FILENAME;
MemoryRegion *bios = g_new(MemoryRegion, 1);
- hwaddr kernel_base, initrd_base, cmdline_base = 0;
- long kernel_size, initrd_size;
+ hwaddr kernel_base = 0, initrd_base = 0, cmdline_base = 0;
+ long kernel_size = 0, initrd_size = 0;
UNINHostState *uninorth_pci;
PCIBus *pci_bus;
PCIDevice *macio;
@@ -165,8 +165,6 @@ static void ppc_core99_init(MachineState *machine)
bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
}
g_free(filename);
- } else {
- bios_size = -1;
}
if (bios_size < 0 || bios_size > PROM_SIZE) {
error_report("could not load PowerPC bios '%s'", bios_name);
@@ -174,15 +172,12 @@ static void ppc_core99_init(MachineState *machine)
}
if (machine->kernel_filename) {
- int bswap_needed;
+ int bswap_needed = 0;
#ifdef BSWAP_NEEDED
bswap_needed = 1;
-#else
- bswap_needed = 0;
#endif
kernel_base = KERNEL_LOAD_ADDR;
-
kernel_size = load_elf(machine->kernel_filename, NULL,
translate_kernel_address, NULL, NULL, NULL,
NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
@@ -212,16 +207,10 @@ static void ppc_core99_init(MachineState *machine)
}
cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
} else {
- initrd_base = 0;
- initrd_size = 0;
cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
}
ppc_boot_device = 'm';
} else {
- kernel_base = 0;
- kernel_size = 0;
- initrd_base = 0;
- initrd_size = 0;
ppc_boot_device = '\0';
/* We consider that NewWorld PowerMac never have any floppy drive
* For now, OHW cannot boot from the network.
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 5cabc410e7..cb67e44081 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -84,11 +84,11 @@ static void ppc_heathrow_init(MachineState *machine)
PowerPCCPU *cpu = NULL;
CPUPPCState *env = NULL;
char *filename;
- int i, bios_size;
+ int i, bios_size = -1;
MemoryRegion *bios = g_new(MemoryRegion, 1);
uint64_t bios_addr;
- uint32_t kernel_base, initrd_base, cmdline_base = 0;
- int32_t kernel_size, initrd_size;
+ uint32_t kernel_base = 0, initrd_base = 0, cmdline_base = 0;
+ int32_t kernel_size = 0, initrd_size = 0;
PCIBus *pci_bus;
PCIDevice *macio;
MACIOIDEState *macio_ide;
@@ -139,8 +139,6 @@ static void ppc_heathrow_init(MachineState *machine)
bios_addr = PROM_BASE;
}
g_free(filename);
- } else {
- bios_size = -1;
}
if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) {
error_report("could not load PowerPC bios '%s'", bios_name);
@@ -148,12 +146,10 @@ static void ppc_heathrow_init(MachineState *machine)
}
if (machine->kernel_filename) {
- int bswap_needed;
+ int bswap_needed = 0;
#ifdef BSWAP_NEEDED
bswap_needed = 1;
-#else
- bswap_needed = 0;
#endif
kernel_base = KERNEL_LOAD_ADDR;
kernel_size = load_elf(machine->kernel_filename, NULL,
@@ -186,16 +182,10 @@ static void ppc_heathrow_init(MachineState *machine)
}
cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
} else {
- initrd_base = 0;
- initrd_size = 0;
cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
}
ppc_boot_device = 'm';
} else {
- kernel_base = 0;
- kernel_size = 0;
- initrd_base = 0;
- initrd_size = 0;
ppc_boot_device = '\0';
for (i = 0; machine->boot_config.order[i] != '\0'; i++) {
/*
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (3 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 04/13] mac_{old|new}world: Avoid else branch by setting default value BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-14 8:57 ` Mark Cave-Ayland
2022-10-03 20:13 ` [PATCH v3 06/13] mac_newworld: Clean up creation of Uninorth devices BALATON Zoltan
` (8 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
By slight reorganisation we can avoid an else branch and some code
duplication which makes it easier to follow the code.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ppc/mac_newworld.c | 6 +++---
hw/ppc/mac_oldworld.c | 7 +++----
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 6bc3bd19be..73b01e8c6d 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
machine->kernel_filename);
exit(1);
}
+ cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
+ KERNEL_GAP);
/* load initrd */
if (machine->initrd_filename) {
- initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
+ initrd_base = cmdline_base;
initrd_size = load_image_targphys(machine->initrd_filename,
initrd_base,
machine->ram_size - initrd_base);
@@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
exit(1);
}
cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
- } else {
- cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
}
ppc_boot_device = 'm';
} else {
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index cb67e44081..b424729a39 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState *machine)
machine->kernel_filename);
exit(1);
}
+ cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
+ KERNEL_GAP);
/* load initrd */
if (machine->initrd_filename) {
- initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
- KERNEL_GAP);
+ initrd_base = cmdline_base;
initrd_size = load_image_targphys(machine->initrd_filename,
initrd_base,
machine->ram_size - initrd_base);
@@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
exit(1);
}
cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
- } else {
- cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
}
ppc_boot_device = 'm';
} else {
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 06/13] mac_newworld: Clean up creation of Uninorth devices
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (4 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-14 9:04 ` Mark Cave-Ayland
2022-10-03 20:13 ` [PATCH v3 07/13] mac_{old|new}world: Reduce number of QOM casts BALATON Zoltan
` (7 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
Map regions in ascending order and reorganise code a bit to avoid some
casts and move Uninorth parts together.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ppc/mac_newworld.c | 38 ++++++++++++++++++--------------------
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 73b01e8c6d..be2cb5f057 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -228,13 +228,6 @@ static void ppc_core99_init(MachineState *machine)
}
}
- /* UniN init */
- dev = qdev_new(TYPE_UNI_NORTH);
- s = SYS_BUS_DEVICE(dev);
- sysbus_realize_and_unref(s, &error_fatal);
- memory_region_add_subregion(get_system_memory(), 0xf8000000,
- sysbus_mmio_get_region(s, 0));
-
openpic_irqs = g_new0(IrqLines, machine->smp.cpus);
for (i = 0; i < machine->smp.cpus; i++) {
/* Mac99 IRQ connection between OpenPIC outputs pins
@@ -275,24 +268,31 @@ static void ppc_core99_init(MachineState *machine)
}
}
+ /* UniN init */
+ s = SYS_BUS_DEVICE(qdev_new(TYPE_UNI_NORTH));
+ sysbus_realize_and_unref(s, &error_fatal);
+ memory_region_add_subregion(get_system_memory(), 0xf8000000,
+ sysbus_mmio_get_region(s, 0));
+
+
if (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) {
+ machine_arch = ARCH_MAC99_U3;
/* 970 gets a U3 bus */
/* Uninorth AGP bus */
dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
- sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
s = SYS_BUS_DEVICE(dev);
+ sysbus_realize_and_unref(s, &error_fatal);
+ sysbus_mmio_map(s, 0, 0xf0800000);
+ sysbus_mmio_map(s, 1, 0xf0c00000);
/* PCI hole */
- memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
+ memory_region_add_subregion(get_system_memory(), 0x80000000,
sysbus_mmio_get_region(s, 2));
/* Register 8 MB of ISA IO space */
memory_region_add_subregion(get_system_memory(), 0xf2000000,
sysbus_mmio_get_region(s, 3));
- sysbus_mmio_map(s, 0, 0xf0800000);
- sysbus_mmio_map(s, 1, 0xf0c00000);
-
- machine_arch = ARCH_MAC99_U3;
} else {
+ machine_arch = ARCH_MAC99;
/* Use values found on a real PowerMac */
/* Uninorth AGP bus */
uninorth_agp_dev = qdev_new(TYPE_UNI_NORTH_AGP_HOST_BRIDGE);
@@ -309,22 +309,20 @@ static void ppc_core99_init(MachineState *machine)
sysbus_mmio_map(s, 0, 0xf4800000);
sysbus_mmio_map(s, 1, 0xf4c00000);
- /* Uninorth main bus */
+ /* Uninorth main bus - this must be last to make it the default */
dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
qdev_prop_set_uint32(dev, "ofw-addr", 0xf2000000);
- sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
uninorth_pci = UNI_NORTH_PCI_HOST_BRIDGE(dev);
s = SYS_BUS_DEVICE(dev);
+ sysbus_realize_and_unref(s, &error_fatal);
+ sysbus_mmio_map(s, 0, 0xf2800000);
+ sysbus_mmio_map(s, 1, 0xf2c00000);
/* PCI hole */
- memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
+ memory_region_add_subregion(get_system_memory(), 0x80000000,
sysbus_mmio_get_region(s, 2));
/* Register 8 MB of ISA IO space */
memory_region_add_subregion(get_system_memory(), 0xf2000000,
sysbus_mmio_get_region(s, 3));
- sysbus_mmio_map(s, 0, 0xf2800000);
- sysbus_mmio_map(s, 1, 0xf2c00000);
-
- machine_arch = ARCH_MAC99;
}
machine->usb |= defaults_enabled() && !machine->usb_disabled;
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 07/13] mac_{old|new}world: Reduce number of QOM casts
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (5 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 06/13] mac_newworld: Clean up creation of Uninorth devices BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 08/13] hw/ppc/mac.h: Move newworld specific parts out from shared header BALATON Zoltan
` (6 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 10395 bytes --]
By storing the device pointers in a variable with the right type the
number of QOM casts can be reduced which also makes the code more
readable.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/ppc/mac_newworld.c | 61 ++++++++++++++++++++-----------------------
hw/ppc/mac_oldworld.c | 26 ++++++++----------
2 files changed, 39 insertions(+), 48 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index be2cb5f057..1abdc581fa 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -116,18 +116,16 @@ static void ppc_core99_init(MachineState *machine)
MemoryRegion *bios = g_new(MemoryRegion, 1);
hwaddr kernel_base = 0, initrd_base = 0, cmdline_base = 0;
long kernel_size = 0, initrd_size = 0;
- UNINHostState *uninorth_pci;
PCIBus *pci_bus;
- PCIDevice *macio;
- ESCCState *escc;
bool has_pmu, has_adb;
+ Object *macio;
MACIOIDEState *macio_ide;
BusState *adb_bus;
MacIONVRAMState *nvr;
DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
void *fw_cfg;
SysBusDevice *s;
- DeviceState *dev, *pic_dev;
+ DeviceState *dev, *pic_dev, *uninorth_pci_dev;
DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
hwaddr nvram_addr = 0xFFF04000;
uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
@@ -229,6 +227,7 @@ static void ppc_core99_init(MachineState *machine)
}
openpic_irqs = g_new0(IrqLines, machine->smp.cpus);
+ dev = DEVICE(cpu);
for (i = 0; i < machine->smp.cpus; i++) {
/* Mac99 IRQ connection between OpenPIC outputs pins
* and PowerPC input pins
@@ -236,30 +235,30 @@ static void ppc_core99_init(MachineState *machine)
switch (PPC_INPUT(env)) {
case PPC_FLAGS_INPUT_6xx:
openpic_irqs[i].irq[OPENPIC_OUTPUT_INT] =
- qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT);
+ qdev_get_gpio_in(dev, PPC6xx_INPUT_INT);
openpic_irqs[i].irq[OPENPIC_OUTPUT_CINT] =
- qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT);
+ qdev_get_gpio_in(dev, PPC6xx_INPUT_INT);
openpic_irqs[i].irq[OPENPIC_OUTPUT_MCK] =
- qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_MCP);
+ qdev_get_gpio_in(dev, PPC6xx_INPUT_MCP);
/* Not connected ? */
openpic_irqs[i].irq[OPENPIC_OUTPUT_DEBUG] = NULL;
/* Check this */
openpic_irqs[i].irq[OPENPIC_OUTPUT_RESET] =
- qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_HRESET);
+ qdev_get_gpio_in(dev, PPC6xx_INPUT_HRESET);
break;
#if defined(TARGET_PPC64)
case PPC_FLAGS_INPUT_970:
openpic_irqs[i].irq[OPENPIC_OUTPUT_INT] =
- qdev_get_gpio_in(DEVICE(cpu), PPC970_INPUT_INT);
+ qdev_get_gpio_in(dev, PPC970_INPUT_INT);
openpic_irqs[i].irq[OPENPIC_OUTPUT_CINT] =
- qdev_get_gpio_in(DEVICE(cpu), PPC970_INPUT_INT);
+ qdev_get_gpio_in(dev, PPC970_INPUT_INT);
openpic_irqs[i].irq[OPENPIC_OUTPUT_MCK] =
- qdev_get_gpio_in(DEVICE(cpu), PPC970_INPUT_MCP);
+ qdev_get_gpio_in(dev, PPC970_INPUT_MCP);
/* Not connected ? */
openpic_irqs[i].irq[OPENPIC_OUTPUT_DEBUG] = NULL;
/* Check this */
openpic_irqs[i].irq[OPENPIC_OUTPUT_RESET] =
- qdev_get_gpio_in(DEVICE(cpu), PPC970_INPUT_HRESET);
+ qdev_get_gpio_in(dev, PPC970_INPUT_HRESET);
break;
#endif /* defined(TARGET_PPC64) */
default:
@@ -279,9 +278,8 @@ static void ppc_core99_init(MachineState *machine)
machine_arch = ARCH_MAC99_U3;
/* 970 gets a U3 bus */
/* Uninorth AGP bus */
- dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
- uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
- s = SYS_BUS_DEVICE(dev);
+ uninorth_pci_dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
+ s = SYS_BUS_DEVICE(uninorth_pci_dev);
sysbus_realize_and_unref(s, &error_fatal);
sysbus_mmio_map(s, 0, 0xf0800000);
sysbus_mmio_map(s, 1, 0xf0c00000);
@@ -310,10 +308,9 @@ static void ppc_core99_init(MachineState *machine)
sysbus_mmio_map(s, 1, 0xf4c00000);
/* Uninorth main bus - this must be last to make it the default */
- dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
- qdev_prop_set_uint32(dev, "ofw-addr", 0xf2000000);
- uninorth_pci = UNI_NORTH_PCI_HOST_BRIDGE(dev);
- s = SYS_BUS_DEVICE(dev);
+ uninorth_pci_dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
+ qdev_prop_set_uint32(uninorth_pci_dev, "ofw-addr", 0xf2000000);
+ s = SYS_BUS_DEVICE(uninorth_pci_dev);
sysbus_realize_and_unref(s, &error_fatal);
sysbus_mmio_map(s, 0, 0xf2800000);
sysbus_mmio_map(s, 1, 0xf2c00000);
@@ -331,24 +328,24 @@ static void ppc_core99_init(MachineState *machine)
core99_machine->via_config == CORE99_VIA_CONFIG_PMU_ADB);
/* init basic PC hardware */
- pci_bus = PCI_HOST_BRIDGE(uninorth_pci)->bus;
+ pci_bus = PCI_HOST_BRIDGE(uninorth_pci_dev)->bus;
/* MacIO */
- macio = pci_new(-1, TYPE_NEWWORLD_MACIO);
+ macio = OBJECT(pci_new(-1, TYPE_NEWWORLD_MACIO));
dev = DEVICE(macio);
qdev_prop_set_uint64(dev, "frequency", tbfreq);
qdev_prop_set_bit(dev, "has-pmu", has_pmu);
qdev_prop_set_bit(dev, "has-adb", has_adb);
- escc = ESCC(object_resolve_path_component(OBJECT(macio), "escc"));
- qdev_prop_set_chr(DEVICE(escc), "chrA", serial_hd(0));
- qdev_prop_set_chr(DEVICE(escc), "chrB", serial_hd(1));
+ dev = DEVICE(object_resolve_path_component(macio, "escc"));
+ qdev_prop_set_chr(dev, "chrA", serial_hd(0));
+ qdev_prop_set_chr(dev, "chrB", serial_hd(1));
- pci_realize_and_unref(macio, pci_bus, &error_fatal);
+ pci_realize_and_unref(PCI_DEVICE(macio), pci_bus, &error_fatal);
- pic_dev = DEVICE(object_resolve_path_component(OBJECT(macio), "pic"));
+ pic_dev = DEVICE(object_resolve_path_component(macio, "pic"));
for (i = 0; i < 4; i++) {
- qdev_connect_gpio_out(DEVICE(uninorth_pci), i,
+ qdev_connect_gpio_out(uninorth_pci_dev, i,
qdev_get_gpio_in(pic_dev, 0x1b + i));
}
@@ -380,19 +377,17 @@ static void ppc_core99_init(MachineState *machine)
/* We only emulate 2 out of 3 IDE controllers for now */
ide_drive_get(hd, ARRAY_SIZE(hd));
- macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
- "ide[0]"));
+ macio_ide = MACIO_IDE(object_resolve_path_component(macio, "ide[0]"));
macio_ide_init_drives(macio_ide, hd);
- macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
- "ide[1]"));
+ macio_ide = MACIO_IDE(object_resolve_path_component(macio, "ide[1]"));
macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
if (has_adb) {
if (has_pmu) {
- dev = DEVICE(object_resolve_path_component(OBJECT(macio), "pmu"));
+ dev = DEVICE(object_resolve_path_component(macio, "pmu"));
} else {
- dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
+ dev = DEVICE(object_resolve_path_component(macio, "cuda"));
}
adb_bus = qdev_get_child_bus(dev, "adb.0");
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index b424729a39..be06ea04ff 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -90,9 +90,8 @@ static void ppc_heathrow_init(MachineState *machine)
uint32_t kernel_base = 0, initrd_base = 0, cmdline_base = 0;
int32_t kernel_size = 0, initrd_size = 0;
PCIBus *pci_bus;
- PCIDevice *macio;
+ Object *macio;
MACIOIDEState *macio_ide;
- ESCCState *escc;
SysBusDevice *s;
DeviceState *dev, *pic_dev, *grackle_dev;
BusState *adb_bus;
@@ -230,17 +229,16 @@ static void ppc_heathrow_init(MachineState *machine)
pci_bus = PCI_HOST_BRIDGE(grackle_dev)->bus;
/* MacIO */
- macio = pci_new(PCI_DEVFN(16, 0), TYPE_OLDWORLD_MACIO);
- dev = DEVICE(macio);
- qdev_prop_set_uint64(dev, "frequency", tbfreq);
+ macio = OBJECT(pci_new(PCI_DEVFN(16, 0), TYPE_OLDWORLD_MACIO));
+ qdev_prop_set_uint64(DEVICE(macio), "frequency", tbfreq);
- escc = ESCC(object_resolve_path_component(OBJECT(macio), "escc"));
- qdev_prop_set_chr(DEVICE(escc), "chrA", serial_hd(0));
- qdev_prop_set_chr(DEVICE(escc), "chrB", serial_hd(1));
+ dev = DEVICE(object_resolve_path_component(macio, "escc"));
+ qdev_prop_set_chr(dev, "chrA", serial_hd(0));
+ qdev_prop_set_chr(dev, "chrB", serial_hd(1));
- pci_realize_and_unref(macio, pci_bus, &error_fatal);
+ pci_realize_and_unref(PCI_DEVICE(macio), pci_bus, &error_fatal);
- pic_dev = DEVICE(object_resolve_path_component(OBJECT(macio), "pic"));
+ pic_dev = DEVICE(object_resolve_path_component(macio, "pic"));
for (i = 0; i < 4; i++) {
qdev_connect_gpio_out(grackle_dev, i,
qdev_get_gpio_in(pic_dev, 0x15 + i));
@@ -268,16 +266,14 @@ static void ppc_heathrow_init(MachineState *machine)
/* MacIO IDE */
ide_drive_get(hd, ARRAY_SIZE(hd));
- macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
- "ide[0]"));
+ macio_ide = MACIO_IDE(object_resolve_path_component(macio, "ide[0]"));
macio_ide_init_drives(macio_ide, hd);
- macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
- "ide[1]"));
+ macio_ide = MACIO_IDE(object_resolve_path_component(macio, "ide[1]"));
macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
/* MacIO CUDA/ADB */
- dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
+ dev = DEVICE(object_resolve_path_component(macio, "cuda"));
adb_bus = qdev_get_child_bus(dev, "adb.0");
dev = qdev_new(TYPE_ADB_KEYBOARD);
qdev_realize_and_unref(dev, adb_bus, &error_fatal);
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 08/13] hw/ppc/mac.h: Move newworld specific parts out from shared header
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (6 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 07/13] mac_{old|new}world: Reduce number of QOM casts BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 09/13] hw/ppc/mac.h: Move macio " BALATON Zoltan
` (5 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
Move the parts specific to and only used by mac99 out from the shared
mac.h into mac_newworld.c where they better belong.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/ppc/mac.h | 24 ------------------------
hw/ppc/mac_newworld.c | 19 +++++++++++++++++++
hw/ppc/mac_oldworld.c | 1 +
3 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index a1fa8f8e41..e97087c7e7 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -26,15 +26,8 @@
#ifndef PPC_MAC_H
#define PPC_MAC_H
-#include "qemu/units.h"
#include "exec/memory.h"
-#include "hw/boards.h"
#include "hw/sysbus.h"
-#include "hw/input/adb.h"
-#include "hw/misc/mos6522.h"
-#include "hw/pci/pci_host.h"
-#include "hw/pci-host/uninorth.h"
-#include "qom/object.h"
#define NVRAM_SIZE 0x2000
#define PROM_FILENAME "openbios-ppc"
@@ -65,23 +58,6 @@
#define NEWWORLD_EXTING_GPIO1 0x2f
#define NEWWORLD_EXTING_GPIO9 0x37
-/* Core99 machine */
-#define TYPE_CORE99_MACHINE MACHINE_TYPE_NAME("mac99")
-typedef struct Core99MachineState Core99MachineState;
-DECLARE_INSTANCE_CHECKER(Core99MachineState, CORE99_MACHINE,
- TYPE_CORE99_MACHINE)
-
-#define CORE99_VIA_CONFIG_CUDA 0x0
-#define CORE99_VIA_CONFIG_PMU 0x1
-#define CORE99_VIA_CONFIG_PMU_ADB 0x2
-
-struct Core99MachineState {
- /*< private >*/
- MachineState parent;
-
- uint8_t via_config;
-};
-
/* Grackle PCI */
#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 1abdc581fa..33caecd236 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -48,10 +48,13 @@
#include "qemu/osdep.h"
#include "qemu/datadir.h"
+#include "qemu/units.h"
#include "qapi/error.h"
#include "hw/ppc/ppc.h"
#include "hw/qdev-properties.h"
#include "hw/ppc/mac.h"
+#include "hw/boards.h"
+#include "hw/pci-host/uninorth.h"
#include "hw/input/adb.h"
#include "hw/ppc/mac_dbdma.h"
#include "hw/pci/pci.h"
@@ -83,6 +86,22 @@
#define PROM_BASE 0xfff00000
#define PROM_SIZE (1 * MiB)
+#define TYPE_CORE99_MACHINE MACHINE_TYPE_NAME("mac99")
+typedef struct Core99MachineState Core99MachineState;
+DECLARE_INSTANCE_CHECKER(Core99MachineState, CORE99_MACHINE,
+ TYPE_CORE99_MACHINE)
+
+#define CORE99_VIA_CONFIG_CUDA 0x0
+#define CORE99_VIA_CONFIG_PMU 0x1
+#define CORE99_VIA_CONFIG_PMU_ADB 0x2
+
+struct Core99MachineState {
+ /*< private >*/
+ MachineState parent;
+
+ uint8_t via_config;
+};
+
static void fw_cfg_boot_set(void *opaque, const char *boot_device,
Error **errp)
{
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index be06ea04ff..a10c884503 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -31,6 +31,7 @@
#include "hw/ppc/ppc.h"
#include "hw/qdev-properties.h"
#include "mac.h"
+#include "hw/boards.h"
#include "hw/input/adb.h"
#include "sysemu/sysemu.h"
#include "net/net.h"
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 09/13] hw/ppc/mac.h: Move macio specific parts out from shared header
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (7 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 08/13] hw/ppc/mac.h: Move newworld specific parts out from shared header BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 10/13] hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header BALATON Zoltan
` (4 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3046 bytes --]
Move the parts specific to and only used by macio out from the shared
mac.h into macio.c where they better belong.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/misc/macio/macio.c | 5 +++--
hw/ppc/mac.h | 23 -----------------------
include/hw/misc/macio/macio.h | 21 +++++++++++++++++++++
3 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index c1fad43f6c..f9f0758b03 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -37,8 +37,9 @@
#include "hw/intc/heathrow_pic.h"
#include "trace.h"
-/* Note: this code is strongly inspirated from the corresponding code
- * in PearPC */
+#define ESCC_CLOCK 3686400
+
+/* Note: this code is strongly inspired by the corresponding code in PearPC */
/*
* The mac-io has two interfaces to the ESCC. One is called "escc-legacy",
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index e97087c7e7..55cb02c990 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -35,29 +35,6 @@
#define KERNEL_LOAD_ADDR 0x01000000
#define KERNEL_GAP 0x00100000
-#define ESCC_CLOCK 3686400
-
-/* Old World IRQs */
-#define OLDWORLD_CUDA_IRQ 0x12
-#define OLDWORLD_ESCCB_IRQ 0x10
-#define OLDWORLD_ESCCA_IRQ 0xf
-#define OLDWORLD_IDE0_IRQ 0xd
-#define OLDWORLD_IDE0_DMA_IRQ 0x2
-#define OLDWORLD_IDE1_IRQ 0xe
-#define OLDWORLD_IDE1_DMA_IRQ 0x3
-
-/* New World IRQs */
-#define NEWWORLD_CUDA_IRQ 0x19
-#define NEWWORLD_PMU_IRQ 0x19
-#define NEWWORLD_ESCCB_IRQ 0x24
-#define NEWWORLD_ESCCA_IRQ 0x25
-#define NEWWORLD_IDE0_IRQ 0xd
-#define NEWWORLD_IDE0_DMA_IRQ 0x2
-#define NEWWORLD_IDE1_IRQ 0xe
-#define NEWWORLD_IDE1_DMA_IRQ 0x3
-#define NEWWORLD_EXTING_GPIO1 0x2f
-#define NEWWORLD_EXTING_GPIO9 0x37
-
/* Grackle PCI */
#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 6c05f3bfd2..26cf15b1ce 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -38,6 +38,27 @@
#include "hw/ppc/openpic.h"
#include "qom/object.h"
+/* Old World IRQs */
+#define OLDWORLD_CUDA_IRQ 0x12
+#define OLDWORLD_ESCCB_IRQ 0x10
+#define OLDWORLD_ESCCA_IRQ 0xf
+#define OLDWORLD_IDE0_IRQ 0xd
+#define OLDWORLD_IDE0_DMA_IRQ 0x2
+#define OLDWORLD_IDE1_IRQ 0xe
+#define OLDWORLD_IDE1_DMA_IRQ 0x3
+
+/* New World IRQs */
+#define NEWWORLD_CUDA_IRQ 0x19
+#define NEWWORLD_PMU_IRQ 0x19
+#define NEWWORLD_ESCCB_IRQ 0x24
+#define NEWWORLD_ESCCA_IRQ 0x25
+#define NEWWORLD_IDE0_IRQ 0xd
+#define NEWWORLD_IDE0_DMA_IRQ 0x2
+#define NEWWORLD_IDE1_IRQ 0xe
+#define NEWWORLD_IDE1_DMA_IRQ 0x3
+#define NEWWORLD_EXTING_GPIO1 0x2f
+#define NEWWORLD_EXTING_GPIO9 0x37
+
/* MacIO virtual bus */
#define TYPE_MACIO_BUS "macio-bus"
OBJECT_DECLARE_SIMPLE_TYPE(MacIOBusState, MACIO_BUS)
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 10/13] hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (8 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 09/13] hw/ppc/mac.h: Move macio " BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-14 9:08 ` Mark Cave-Ayland
2022-10-03 20:13 ` [PATCH v3 11/13] hw/ppc/mac.h: Move PROM and KERNEL defines to board code BALATON Zoltan
` (3 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
MAINTAINERS | 1 +
hw/pci-host/grackle.c | 14 +----------
hw/ppc/mac.h | 3 ---
hw/ppc/mac_oldworld.c | 1 +
include/hw/pci-host/grackle.h | 44 +++++++++++++++++++++++++++++++++++
5 files changed, 47 insertions(+), 16 deletions(-)
create mode 100644 include/hw/pci-host/grackle.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 789172b2a8..6e6819afcd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1345,6 +1345,7 @@ F: hw/intc/heathrow_pic.c
F: hw/input/adb*
F: include/hw/intc/heathrow_pic.h
F: include/hw/input/adb*
+F: include/hw/pci-host/grackle.h
F: pc-bios/qemu_vga.ndrv
PReP
diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index b05facf463..e4c7303859 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -24,7 +24,6 @@
*/
#include "qemu/osdep.h"
-#include "hw/pci/pci_host.h"
#include "hw/ppc/mac.h"
#include "hw/qdev-properties.h"
#include "hw/pci/pci.h"
@@ -33,18 +32,7 @@
#include "qemu/module.h"
#include "trace.h"
#include "qom/object.h"
-
-OBJECT_DECLARE_SIMPLE_TYPE(GrackleState, GRACKLE_PCI_HOST_BRIDGE)
-
-struct GrackleState {
- PCIHostState parent_obj;
-
- uint32_t ofw_addr;
- qemu_irq irqs[4];
- MemoryRegion pci_mmio;
- MemoryRegion pci_hole;
- MemoryRegion pci_io;
-};
+#include "hw/pci-host/grackle.h"
/* Don't know if this matches real hardware, but it agrees with OHW. */
static int pci_grackle_map_irq(PCIDevice *pci_dev, int irq_num)
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index 55cb02c990..fe77a6c6db 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -35,9 +35,6 @@
#define KERNEL_LOAD_ADDR 0x01000000
#define KERNEL_GAP 0x00100000
-/* Grackle PCI */
-#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
-
/* Mac NVRAM */
#define TYPE_MACIO_NVRAM "macio-nvram"
OBJECT_DECLARE_SIMPLE_TYPE(MacIONVRAMState, MACIO_NVRAM)
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index a10c884503..e1a22f8eba 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -38,6 +38,7 @@
#include "hw/isa/isa.h"
#include "hw/pci/pci.h"
#include "hw/pci/pci_host.h"
+#include "hw/pci-host/grackle.h"
#include "hw/nvram/fw_cfg.h"
#include "hw/char/escc.h"
#include "hw/misc/macio/macio.h"
diff --git a/include/hw/pci-host/grackle.h b/include/hw/pci-host/grackle.h
new file mode 100644
index 0000000000..7ad3a779f0
--- /dev/null
+++ b/include/hw/pci-host/grackle.h
@@ -0,0 +1,44 @@
+/*
+ * QEMU Grackle PCI host (heathrow OldWorld PowerMac)
+ *
+ * Copyright (c) 2006-2007 Fabrice Bellard
+ * Copyright (c) 2007 Jocelyn Mayer
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef GRACKLE_H
+#define GRACKLE_H
+
+#include "hw/pci/pci_host.h"
+
+#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
+OBJECT_DECLARE_SIMPLE_TYPE(GrackleState, GRACKLE_PCI_HOST_BRIDGE)
+
+struct GrackleState {
+ PCIHostState parent_obj;
+
+ uint32_t ofw_addr;
+ qemu_irq irqs[4];
+ MemoryRegion pci_mmio;
+ MemoryRegion pci_hole;
+ MemoryRegion pci_io;
+};
+
+#endif
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 11/13] hw/ppc/mac.h: Move PROM and KERNEL defines to board code
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (9 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 10/13] hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 12/13] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h BALATON Zoltan
` (2 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]
The PROM_FILENAME and KERNEL_* defines are used by mac_oldworld and
mac_newworld but they don't have to be identical so these could be
moved to the individual boards.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/ppc/mac.h | 4 ----
hw/ppc/mac_newworld.c | 4 ++++
hw/ppc/mac_oldworld.c | 7 ++++++-
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index fe77a6c6db..3e2df262ee 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -30,10 +30,6 @@
#include "hw/sysbus.h"
#define NVRAM_SIZE 0x2000
-#define PROM_FILENAME "openbios-ppc"
-
-#define KERNEL_LOAD_ADDR 0x01000000
-#define KERNEL_GAP 0x00100000
/* Mac NVRAM */
#define TYPE_MACIO_NVRAM "macio-nvram"
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 33caecd236..34531c64a7 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -83,9 +83,13 @@
#define NDRV_VGA_FILENAME "qemu_vga.ndrv"
+#define PROM_FILENAME "openbios-ppc"
#define PROM_BASE 0xfff00000
#define PROM_SIZE (1 * MiB)
+#define KERNEL_LOAD_ADDR 0x01000000
+#define KERNEL_GAP 0x00100000
+
#define TYPE_CORE99_MACHINE MACHINE_TYPE_NAME("mac99")
typedef struct Core99MachineState Core99MachineState;
DECLARE_INSTANCE_CHECKER(Core99MachineState, CORE99_MACHINE,
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index e1a22f8eba..5213cbcc04 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -58,10 +58,15 @@
#define NDRV_VGA_FILENAME "qemu_vga.ndrv"
-#define GRACKLE_BASE 0xfec00000
+#define PROM_FILENAME "openbios-ppc"
#define PROM_BASE 0xffc00000
#define PROM_SIZE (4 * MiB)
+#define KERNEL_LOAD_ADDR 0x01000000
+#define KERNEL_GAP 0x00100000
+
+#define GRACKLE_BASE 0xfec00000
+
static void fw_cfg_boot_set(void *opaque, const char *boot_device,
Error **errp)
{
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 12/13] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (10 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 11/13] hw/ppc/mac.h: Move PROM and KERNEL defines to board code BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 13/13] mac_nvram: Use NVRAM_SIZE constant BALATON Zoltan
2022-10-11 10:22 ` [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
All that is left in mac.h now belongs to the nvram emulation so rename
it accordingly and only include it where it is really used.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
MAINTAINERS | 1 +
hw/ide/macio.c | 1 -
hw/intc/heathrow_pic.c | 1 -
hw/intc/openpic.c | 1 -
hw/misc/macio/cuda.c | 1 -
hw/misc/macio/gpio.c | 1 -
hw/misc/macio/macio.c | 1 -
hw/misc/macio/pmu.c | 1 -
hw/nvram/mac_nvram.c | 2 +-
hw/pci-host/grackle.c | 1 -
hw/pci-host/uninorth.c | 1 -
hw/ppc/mac_newworld.c | 2 +-
hw/ppc/mac_oldworld.c | 1 -
include/hw/misc/macio/macio.h | 2 +-
hw/ppc/mac.h => include/hw/nvram/mac_nvram.h | 11 ++++++-----
15 files changed, 10 insertions(+), 18 deletions(-)
rename hw/ppc/mac.h => include/hw/nvram/mac_nvram.h (89%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 6e6819afcd..69bf07e92b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1328,6 +1328,7 @@ F: hw/nvram/mac_nvram.c
F: hw/input/adb*
F: include/hw/misc/macio/
F: include/hw/misc/mos6522.h
+F: include/hw/nvram/mac_nvram.h
F: include/hw/ppc/mac_dbdma.h
F: include/hw/pci-host/uninorth.h
F: include/hw/input/adb*
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 1c15c37ec5..e604466acb 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -24,7 +24,6 @@
*/
#include "qemu/osdep.h"
-#include "hw/ppc/mac.h"
#include "hw/ppc/mac_dbdma.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
diff --git a/hw/intc/heathrow_pic.c b/hw/intc/heathrow_pic.c
index cb97c315da..13048a2735 100644
--- a/hw/intc/heathrow_pic.c
+++ b/hw/intc/heathrow_pic.c
@@ -24,7 +24,6 @@
*/
#include "qemu/osdep.h"
-#include "hw/ppc/mac.h"
#include "migration/vmstate.h"
#include "qemu/module.h"
#include "hw/intc/heathrow_pic.h"
diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index b0787e8ee7..c757adbe53 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -32,7 +32,6 @@
#include "qemu/osdep.h"
#include "hw/irq.h"
-#include "hw/ppc/mac.h"
#include "hw/pci/pci.h"
#include "hw/ppc/openpic.h"
#include "hw/ppc/ppc_e500.h"
diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 1498113cfc..0d4c13319a 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -25,7 +25,6 @@
#include "qemu/osdep.h"
#include "hw/irq.h"
-#include "hw/ppc/mac.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "hw/input/adb.h"
diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
index b1bcf830c3..c8ac5633b2 100644
--- a/hw/misc/macio/gpio.c
+++ b/hw/misc/macio/gpio.c
@@ -24,7 +24,6 @@
*/
#include "qemu/osdep.h"
-#include "hw/ppc/mac.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "hw/misc/macio/macio.h"
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index f9f0758b03..93a7c7bbc8 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -26,7 +26,6 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qemu/module.h"
-#include "hw/ppc/mac.h"
#include "hw/misc/macio/cuda.h"
#include "hw/pci/pci.h"
#include "hw/ppc/mac_dbdma.h"
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index 336502a84b..70562ed8d0 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -29,7 +29,6 @@
*/
#include "qemu/osdep.h"
-#include "hw/ppc/mac.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "hw/input/adb.h"
diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
index 11f2d31cdb..3d9ddda217 100644
--- a/hw/nvram/mac_nvram.c
+++ b/hw/nvram/mac_nvram.c
@@ -25,7 +25,7 @@
#include "qemu/osdep.h"
#include "hw/nvram/chrp_nvram.h"
-#include "hw/ppc/mac.h"
+#include "hw/nvram/mac_nvram.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "qemu/cutils.h"
diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index e4c7303859..95945ac0f4 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -24,7 +24,6 @@
*/
#include "qemu/osdep.h"
-#include "hw/ppc/mac.h"
#include "hw/qdev-properties.h"
#include "hw/pci/pci.h"
#include "hw/irq.h"
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index d25b62d6a5..aebd44d265 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -24,7 +24,6 @@
#include "qemu/osdep.h"
#include "hw/irq.h"
-#include "hw/ppc/mac.h"
#include "hw/qdev-properties.h"
#include "qemu/module.h"
#include "hw/pci/pci.h"
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 34531c64a7..7a12949fc1 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -52,7 +52,7 @@
#include "qapi/error.h"
#include "hw/ppc/ppc.h"
#include "hw/qdev-properties.h"
-#include "hw/ppc/mac.h"
+#include "hw/nvram/mac_nvram.h"
#include "hw/boards.h"
#include "hw/pci-host/uninorth.h"
#include "hw/input/adb.h"
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 5213cbcc04..e2b5dd0650 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -30,7 +30,6 @@
#include "qapi/error.h"
#include "hw/ppc/ppc.h"
#include "hw/qdev-properties.h"
-#include "mac.h"
#include "hw/boards.h"
#include "hw/input/adb.h"
#include "sysemu/sysemu.h"
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 26cf15b1ce..95d30a1745 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -33,7 +33,7 @@
#include "hw/misc/macio/cuda.h"
#include "hw/misc/macio/gpio.h"
#include "hw/misc/macio/pmu.h"
-#include "hw/ppc/mac.h"
+#include "hw/nvram/mac_nvram.h"
#include "hw/ppc/mac_dbdma.h"
#include "hw/ppc/openpic.h"
#include "qom/object.h"
diff --git a/hw/ppc/mac.h b/include/hw/nvram/mac_nvram.h
similarity index 89%
rename from hw/ppc/mac.h
rename to include/hw/nvram/mac_nvram.h
index 3e2df262ee..baa9f6a5a6 100644
--- a/hw/ppc/mac.h
+++ b/include/hw/nvram/mac_nvram.h
@@ -1,5 +1,5 @@
/*
- * QEMU PowerMac emulation shared definitions and prototypes
+ * PowerMac NVRAM emulation
*
* Copyright (c) 2004-2007 Fabrice Bellard
* Copyright (c) 2007 Jocelyn Mayer
@@ -23,8 +23,8 @@
* THE SOFTWARE.
*/
-#ifndef PPC_MAC_H
-#define PPC_MAC_H
+#ifndef MAC_NVRAM_H
+#define MAC_NVRAM_H
#include "exec/memory.h"
#include "hw/sysbus.h"
@@ -47,5 +47,6 @@ struct MacIONVRAMState {
uint8_t *data;
};
-void pmac_format_nvram_partition (MacIONVRAMState *nvr, int len);
-#endif /* PPC_MAC_H */
+void pmac_format_nvram_partition(MacIONVRAMState *nvr, int len);
+
+#endif /* MAC_NVRAM_H */
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 13/13] mac_nvram: Use NVRAM_SIZE constant
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (11 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 12/13] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h BALATON Zoltan
@ 2022-10-03 20:13 ` BALATON Zoltan
2022-10-11 10:22 ` [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
13 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-03 20:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
The NVRAM_SIZE constant was defined but not used. Rename it to
MACIO_NVRAM_SIZE to match the device model and use it where appropriate.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/misc/macio/macio.c | 2 +-
hw/ppc/mac_newworld.c | 4 ++--
include/hw/nvram/mac_nvram.h | 3 +--
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 93a7c7bbc8..08dbdd7fc0 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -226,7 +226,7 @@ static void macio_oldworld_init(Object *obj)
object_initialize_child(OBJECT(s), "nvram", &os->nvram, TYPE_MACIO_NVRAM);
dev = DEVICE(&os->nvram);
- qdev_prop_set_uint32(dev, "size", 0x2000);
+ qdev_prop_set_uint32(dev, "size", MACIO_NVRAM_SIZE);
qdev_prop_set_uint32(dev, "it_shift", 4);
for (i = 0; i < 2; i++) {
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 7a12949fc1..50bbd86099 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -451,12 +451,12 @@ static void ppc_core99_init(MachineState *machine)
nvram_addr = 0xFFE00000;
}
dev = qdev_new(TYPE_MACIO_NVRAM);
- qdev_prop_set_uint32(dev, "size", 0x2000);
+ qdev_prop_set_uint32(dev, "size", MACIO_NVRAM_SIZE);
qdev_prop_set_uint32(dev, "it_shift", 1);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, nvram_addr);
nvr = MACIO_NVRAM(dev);
- pmac_format_nvram_partition(nvr, 0x2000);
+ pmac_format_nvram_partition(nvr, MACIO_NVRAM_SIZE);
/* No PCI init: the BIOS will do it */
dev = qdev_new(TYPE_FW_CFG_MEM);
diff --git a/include/hw/nvram/mac_nvram.h b/include/hw/nvram/mac_nvram.h
index baa9f6a5a6..b780aca470 100644
--- a/include/hw/nvram/mac_nvram.h
+++ b/include/hw/nvram/mac_nvram.h
@@ -29,9 +29,8 @@
#include "exec/memory.h"
#include "hw/sysbus.h"
-#define NVRAM_SIZE 0x2000
+#define MACIO_NVRAM_SIZE 0x2000
-/* Mac NVRAM */
#define TYPE_MACIO_NVRAM "macio-nvram"
OBJECT_DECLARE_SIMPLE_TYPE(MacIONVRAMState, MACIO_NVRAM)
--
2.30.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 00/13] Misc ppc/mac machines clean up
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
` (12 preceding siblings ...)
2022-10-03 20:13 ` [PATCH v3 13/13] mac_nvram: Use NVRAM_SIZE constant BALATON Zoltan
@ 2022-10-11 10:22 ` BALATON Zoltan
2022-10-18 11:37 ` BALATON Zoltan
13 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-11 10:22 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On Mon, 3 Oct 2022, BALATON Zoltan wrote:
> This series includes some clean ups to mac_newworld and mac_oldworld
> to make them a bit simpler and more readable, It also removes the
> shared mac.h file that turns out was more of a random collection of
> unrelated things. Getting rid of this mac.h improves the locality of
> device models and reduces unnecessary interdependency.
Ping?
> v3: Some more patch spliting and changes I've noticed and address more
> review comments
> v2: Split some patches and add a few more I've noticed now and address
> review comments
>
> BALATON Zoltan (13):
> mac_newworld: Drop some variables
> mac_oldworld: Drop some more variables
> mac_{old|new}world: Set tbfreq at declaration
> mac_{old|new}world: Avoid else branch by setting default value
> mac_{old|new}world: Simplify cmdline_base calculation
> mac_newworld: Clean up creation of Uninorth devices
> mac_{old|new}world: Reduce number of QOM casts
> hw/ppc/mac.h: Move newworld specific parts out from shared header
> hw/ppc/mac.h: Move macio specific parts out from shared header
> hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header
> hw/ppc/mac.h: Move PROM and KERNEL defines to board code
> hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h
> mac_nvram: Use NVRAM_SIZE constant
>
> MAINTAINERS | 2 +
> hw/ide/macio.c | 1 -
> hw/intc/heathrow_pic.c | 1 -
> hw/intc/openpic.c | 1 -
> hw/misc/macio/cuda.c | 1 -
> hw/misc/macio/gpio.c | 1 -
> hw/misc/macio/macio.c | 8 +-
> hw/misc/macio/pmu.c | 1 -
> hw/nvram/mac_nvram.c | 2 +-
> hw/pci-host/grackle.c | 15 +--
> hw/pci-host/uninorth.c | 1 -
> hw/ppc/mac.h | 105 ----------------
> hw/ppc/mac_newworld.c | 225 ++++++++++++++++------------------
> hw/ppc/mac_oldworld.c | 111 +++++++----------
> include/hw/misc/macio/macio.h | 23 +++-
> include/hw/nvram/mac_nvram.h | 51 ++++++++
> include/hw/pci-host/grackle.h | 44 +++++++
> 17 files changed, 280 insertions(+), 313 deletions(-)
> delete mode 100644 hw/ppc/mac.h
> create mode 100644 include/hw/nvram/mac_nvram.h
> create mode 100644 include/hw/pci-host/grackle.h
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation
2022-10-03 20:13 ` [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation BALATON Zoltan
@ 2022-10-14 8:57 ` Mark Cave-Ayland
2022-10-14 11:56 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2022-10-14 8:57 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 03/10/2022 21:13, BALATON Zoltan wrote:
> By slight reorganisation we can avoid an else branch and some code
> duplication which makes it easier to follow the code.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/mac_newworld.c | 6 +++---
> hw/ppc/mac_oldworld.c | 7 +++----
> 2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 6bc3bd19be..73b01e8c6d 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
> machine->kernel_filename);
> exit(1);
> }
> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> + KERNEL_GAP);
> /* load initrd */
> if (machine->initrd_filename) {
> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
> + initrd_base = cmdline_base;
> initrd_size = load_image_targphys(machine->initrd_filename,
> initrd_base,
> machine->ram_size - initrd_base);
> @@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
> exit(1);
> }
> cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
> - } else {
> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
> }
> ppc_boot_device = 'm';
> } else {
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index cb67e44081..b424729a39 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState *machine)
> machine->kernel_filename);
> exit(1);
> }
> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> + KERNEL_GAP);
> /* load initrd */
> if (machine->initrd_filename) {
> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
> - KERNEL_GAP);
> + initrd_base = cmdline_base;
> initrd_size = load_image_targphys(machine->initrd_filename,
> initrd_base,
> machine->ram_size - initrd_base);
> @@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
> exit(1);
> }
> cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
> - } else {
> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
> }
> ppc_boot_device = 'm';
> } else {
Is there any particular reason why you would want to avoid the else branch? I don't
feel this patch is an improvement since by always setting cmdline_base to a non-zero
value, as a reviewer I then have to check all other uses of cmdline_base in the file
to ensure that this doesn't cause any issues.
I much prefer the existing version since setting the values of cmdline_base and
initrd_base is very clearly scoped within the if statement.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 06/13] mac_newworld: Clean up creation of Uninorth devices
2022-10-03 20:13 ` [PATCH v3 06/13] mac_newworld: Clean up creation of Uninorth devices BALATON Zoltan
@ 2022-10-14 9:04 ` Mark Cave-Ayland
0 siblings, 0 replies; 24+ messages in thread
From: Mark Cave-Ayland @ 2022-10-14 9:04 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 03/10/2022 21:13, BALATON Zoltan wrote:
> Map regions in ascending order and reorganise code a bit to avoid some
> casts and move Uninorth parts together.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/mac_newworld.c | 38 ++++++++++++++++++--------------------
> 1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 73b01e8c6d..be2cb5f057 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -228,13 +228,6 @@ static void ppc_core99_init(MachineState *machine)
> }
> }
>
> - /* UniN init */
> - dev = qdev_new(TYPE_UNI_NORTH);
> - s = SYS_BUS_DEVICE(dev);
> - sysbus_realize_and_unref(s, &error_fatal);
> - memory_region_add_subregion(get_system_memory(), 0xf8000000,
> - sysbus_mmio_get_region(s, 0));
> -
> openpic_irqs = g_new0(IrqLines, machine->smp.cpus);
> for (i = 0; i < machine->smp.cpus; i++) {
> /* Mac99 IRQ connection between OpenPIC outputs pins
> @@ -275,24 +268,31 @@ static void ppc_core99_init(MachineState *machine)
> }
> }
>
> + /* UniN init */
> + s = SYS_BUS_DEVICE(qdev_new(TYPE_UNI_NORTH));
> + sysbus_realize_and_unref(s, &error_fatal);
> + memory_region_add_subregion(get_system_memory(), 0xf8000000,
> + sysbus_mmio_get_region(s, 0));
> +
> +
> if (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) {
> + machine_arch = ARCH_MAC99_U3;
> /* 970 gets a U3 bus */
> /* Uninorth AGP bus */
> dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
> s = SYS_BUS_DEVICE(dev);
> + sysbus_realize_and_unref(s, &error_fatal);
> + sysbus_mmio_map(s, 0, 0xf0800000);
> + sysbus_mmio_map(s, 1, 0xf0c00000);
> /* PCI hole */
> - memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
> + memory_region_add_subregion(get_system_memory(), 0x80000000,
> sysbus_mmio_get_region(s, 2));
> /* Register 8 MB of ISA IO space */
> memory_region_add_subregion(get_system_memory(), 0xf2000000,
> sysbus_mmio_get_region(s, 3));
> - sysbus_mmio_map(s, 0, 0xf0800000);
> - sysbus_mmio_map(s, 1, 0xf0c00000);
> -
> - machine_arch = ARCH_MAC99_U3;
> } else {
> + machine_arch = ARCH_MAC99;
> /* Use values found on a real PowerMac */
> /* Uninorth AGP bus */
> uninorth_agp_dev = qdev_new(TYPE_UNI_NORTH_AGP_HOST_BRIDGE);
> @@ -309,22 +309,20 @@ static void ppc_core99_init(MachineState *machine)
> sysbus_mmio_map(s, 0, 0xf4800000);
> sysbus_mmio_map(s, 1, 0xf4c00000);
>
> - /* Uninorth main bus */
> + /* Uninorth main bus - this must be last to make it the default */
> dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
> qdev_prop_set_uint32(dev, "ofw-addr", 0xf2000000);
> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> uninorth_pci = UNI_NORTH_PCI_HOST_BRIDGE(dev);
> s = SYS_BUS_DEVICE(dev);
> + sysbus_realize_and_unref(s, &error_fatal);
> + sysbus_mmio_map(s, 0, 0xf2800000);
> + sysbus_mmio_map(s, 1, 0xf2c00000);
> /* PCI hole */
> - memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
> + memory_region_add_subregion(get_system_memory(), 0x80000000,
> sysbus_mmio_get_region(s, 2));
> /* Register 8 MB of ISA IO space */
> memory_region_add_subregion(get_system_memory(), 0xf2000000,
> sysbus_mmio_get_region(s, 3));
> - sysbus_mmio_map(s, 0, 0xf2800000);
> - sysbus_mmio_map(s, 1, 0xf2c00000);
> -
> - machine_arch = ARCH_MAC99;
> }
>
> machine->usb |= defaults_enabled() && !machine->usb_disabled;
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 10/13] hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header
2022-10-03 20:13 ` [PATCH v3 10/13] hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header BALATON Zoltan
@ 2022-10-14 9:08 ` Mark Cave-Ayland
0 siblings, 0 replies; 24+ messages in thread
From: Mark Cave-Ayland @ 2022-10-14 9:08 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 03/10/2022 21:13, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> MAINTAINERS | 1 +
> hw/pci-host/grackle.c | 14 +----------
> hw/ppc/mac.h | 3 ---
> hw/ppc/mac_oldworld.c | 1 +
> include/hw/pci-host/grackle.h | 44 +++++++++++++++++++++++++++++++++++
> 5 files changed, 47 insertions(+), 16 deletions(-)
> create mode 100644 include/hw/pci-host/grackle.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 789172b2a8..6e6819afcd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1345,6 +1345,7 @@ F: hw/intc/heathrow_pic.c
> F: hw/input/adb*
> F: include/hw/intc/heathrow_pic.h
> F: include/hw/input/adb*
> +F: include/hw/pci-host/grackle.h
> F: pc-bios/qemu_vga.ndrv
>
> PReP
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index b05facf463..e4c7303859 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -24,7 +24,6 @@
> */
>
> #include "qemu/osdep.h"
> -#include "hw/pci/pci_host.h"
> #include "hw/ppc/mac.h"
> #include "hw/qdev-properties.h"
> #include "hw/pci/pci.h"
> @@ -33,18 +32,7 @@
> #include "qemu/module.h"
> #include "trace.h"
> #include "qom/object.h"
> -
> -OBJECT_DECLARE_SIMPLE_TYPE(GrackleState, GRACKLE_PCI_HOST_BRIDGE)
> -
> -struct GrackleState {
> - PCIHostState parent_obj;
> -
> - uint32_t ofw_addr;
> - qemu_irq irqs[4];
> - MemoryRegion pci_mmio;
> - MemoryRegion pci_hole;
> - MemoryRegion pci_io;
> -};
> +#include "hw/pci-host/grackle.h"
>
> /* Don't know if this matches real hardware, but it agrees with OHW. */
> static int pci_grackle_map_irq(PCIDevice *pci_dev, int irq_num)
> diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
> index 55cb02c990..fe77a6c6db 100644
> --- a/hw/ppc/mac.h
> +++ b/hw/ppc/mac.h
> @@ -35,9 +35,6 @@
> #define KERNEL_LOAD_ADDR 0x01000000
> #define KERNEL_GAP 0x00100000
>
> -/* Grackle PCI */
> -#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
> -
> /* Mac NVRAM */
> #define TYPE_MACIO_NVRAM "macio-nvram"
> OBJECT_DECLARE_SIMPLE_TYPE(MacIONVRAMState, MACIO_NVRAM)
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index a10c884503..e1a22f8eba 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -38,6 +38,7 @@
> #include "hw/isa/isa.h"
> #include "hw/pci/pci.h"
> #include "hw/pci/pci_host.h"
> +#include "hw/pci-host/grackle.h"
> #include "hw/nvram/fw_cfg.h"
> #include "hw/char/escc.h"
> #include "hw/misc/macio/macio.h"
> diff --git a/include/hw/pci-host/grackle.h b/include/hw/pci-host/grackle.h
> new file mode 100644
> index 0000000000..7ad3a779f0
> --- /dev/null
> +++ b/include/hw/pci-host/grackle.h
> @@ -0,0 +1,44 @@
> +/*
> + * QEMU Grackle PCI host (heathrow OldWorld PowerMac)
> + *
> + * Copyright (c) 2006-2007 Fabrice Bellard
> + * Copyright (c) 2007 Jocelyn Mayer
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef GRACKLE_H
> +#define GRACKLE_H
> +
> +#include "hw/pci/pci_host.h"
> +
> +#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
> +OBJECT_DECLARE_SIMPLE_TYPE(GrackleState, GRACKLE_PCI_HOST_BRIDGE)
> +
> +struct GrackleState {
> + PCIHostState parent_obj;
> +
> + uint32_t ofw_addr;
> + qemu_irq irqs[4];
> + MemoryRegion pci_mmio;
> + MemoryRegion pci_hole;
> + MemoryRegion pci_io;
> +};
> +
> +#endif
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation
2022-10-14 8:57 ` Mark Cave-Ayland
@ 2022-10-14 11:56 ` BALATON Zoltan
2022-10-14 15:25 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-14 11:56 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
On Fri, 14 Oct 2022, Mark Cave-Ayland wrote:
> On 03/10/2022 21:13, BALATON Zoltan wrote:
>
>> By slight reorganisation we can avoid an else branch and some code
>> duplication which makes it easier to follow the code.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/ppc/mac_newworld.c | 6 +++---
>> hw/ppc/mac_oldworld.c | 7 +++----
>> 2 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>> index 6bc3bd19be..73b01e8c6d 100644
>> --- a/hw/ppc/mac_newworld.c
>> +++ b/hw/ppc/mac_newworld.c
>> @@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
>> machine->kernel_filename);
>> exit(1);
>> }
>> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>> + KERNEL_GAP);
>> /* load initrd */
>> if (machine->initrd_filename) {
>> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>> KERNEL_GAP);
>> + initrd_base = cmdline_base;
>> initrd_size = load_image_targphys(machine->initrd_filename,
>> initrd_base,
>> machine->ram_size -
>> initrd_base);
>> @@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
>> exit(1);
>> }
>> cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
>> - } else {
>> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>> KERNEL_GAP);
>> }
>> ppc_boot_device = 'm';
>> } else {
>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>> index cb67e44081..b424729a39 100644
>> --- a/hw/ppc/mac_oldworld.c
>> +++ b/hw/ppc/mac_oldworld.c
>> @@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState *machine)
>> machine->kernel_filename);
>> exit(1);
>> }
>> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>> + KERNEL_GAP);
>> /* load initrd */
>> if (machine->initrd_filename) {
>> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>> - KERNEL_GAP);
>> + initrd_base = cmdline_base;
>> initrd_size = load_image_targphys(machine->initrd_filename,
>> initrd_base,
>> machine->ram_size -
>> initrd_base);
>> @@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
>> exit(1);
>> }
>> cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
>> - } else {
>> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>> KERNEL_GAP);
>> }
>> ppc_boot_device = 'm';
>> } else {
>
> Is there any particular reason why you would want to avoid the else branch? I
It avoids code duplication and to me it's easier to follow than an else
branch. With this patch cmdline_base calculation is clearly tied to
kernel_base and kernel_size if only kernel is used and initrd_base
initrd_size when initrd is used. With the else branch it's less obvious
because it's set much later in the else branch while initrd_base
duplicates it above. So avoiding this duplication makes the code easier to
read and less prone to errors if the calculation is ever modified.
> don't feel this patch is an improvement since by always setting cmdline_base
> to a non-zero value, as a reviewer I then have to check all other uses of
> cmdline_base in the file to ensure that this doesn't cause any issues.
There aren't that many uses that it's difficult to check and this only
need to be done once.
> I much prefer the existing version since setting the values of cmdline_base
> and initrd_base is very clearly scoped within the if statement.
What can I say, it's hard to argue with preferences but avoiding code
duplication is worth the effort reviewing this patch in my opinion.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation
2022-10-14 11:56 ` BALATON Zoltan
@ 2022-10-14 15:25 ` BALATON Zoltan
2022-10-28 8:54 ` Mark Cave-Ayland
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-14 15:25 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
On Fri, 14 Oct 2022, BALATON Zoltan wrote:
> On Fri, 14 Oct 2022, Mark Cave-Ayland wrote:
>> On 03/10/2022 21:13, BALATON Zoltan wrote:
>>
>>> By slight reorganisation we can avoid an else branch and some code
>>> duplication which makes it easier to follow the code.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/ppc/mac_newworld.c | 6 +++---
>>> hw/ppc/mac_oldworld.c | 7 +++----
>>> 2 files changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>> index 6bc3bd19be..73b01e8c6d 100644
>>> --- a/hw/ppc/mac_newworld.c
>>> +++ b/hw/ppc/mac_newworld.c
>>> @@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
>>> machine->kernel_filename);
>>> exit(1);
>>> }
>>> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>> + KERNEL_GAP);
>>> /* load initrd */
>>> if (machine->initrd_filename) {
>>> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>> KERNEL_GAP);
>>> + initrd_base = cmdline_base;
>>> initrd_size = load_image_targphys(machine->initrd_filename,
>>> initrd_base,
>>> machine->ram_size -
>>> initrd_base);
>>> @@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
>>> exit(1);
>>> }
>>> cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
>>> - } else {
>>> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>> KERNEL_GAP);
>>> }
>>> ppc_boot_device = 'm';
>>> } else {
>>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>>> index cb67e44081..b424729a39 100644
>>> --- a/hw/ppc/mac_oldworld.c
>>> +++ b/hw/ppc/mac_oldworld.c
>>> @@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState *machine)
>>> machine->kernel_filename);
>>> exit(1);
>>> }
>>> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>> + KERNEL_GAP);
>>> /* load initrd */
>>> if (machine->initrd_filename) {
>>> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>> - KERNEL_GAP);
>>> + initrd_base = cmdline_base;
>>> initrd_size = load_image_targphys(machine->initrd_filename,
>>> initrd_base,
>>> machine->ram_size -
>>> initrd_base);
>>> @@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
>>> exit(1);
>>> }
>>> cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
>>> - } else {
>>> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>> KERNEL_GAP);
>>> }
>>> ppc_boot_device = 'm';
>>> } else {
>>
>> Is there any particular reason why you would want to avoid the else branch?
>> I
>
> It avoids code duplication and to me it's easier to follow than an else
> branch. With this patch cmdline_base calculation is clearly tied to
> kernel_base and kernel_size if only kernel is used and initrd_base
> initrd_size when initrd is used. With the else branch it's less obvious
> because it's set much later in the else branch while initrd_base duplicates
> it above. So avoiding this duplication makes the code easier to read and less
> prone to errors if the calculation is ever modified.
>
>> don't feel this patch is an improvement since by always setting
>> cmdline_base to a non-zero value, as a reviewer I then have to check all
>> other uses of cmdline_base in the file to ensure that this doesn't cause
>> any issues.
>
> There aren't that many uses that it's difficult to check and this only need
> to be done once.
>
>> I much prefer the existing version since setting the values of cmdline_base
>> and initrd_base is very clearly scoped within the if statement.
>
> What can I say, it's hard to argue with preferences but avoiding code
> duplication is worth the effort reviewing this patch in my opinion.
Also compare the before and after this series:
https://github.com/patchew-project/qemu/blob/master/hw/ppc/mac_newworld.c
https://github.com/patchew-project/qemu/blob/9c1cd2828b3d3bd3a7068134a57ae9bb07f5a681/hw/ppc/mac_newworld.c
I think the result is much easier to follow without else brances as you
can read it from top to bottom instead of jumping around in else branches
that is less clear and also more lines. Also setting default value avoids
any used uninitialised cases which you would need to check if you set vars
in if-else instead so unlike what you say this does not introduce such
cases but closes the possibility instead. So please take the time to
review it in exchange of the time I've put it in writing it.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 00/13] Misc ppc/mac machines clean up
2022-10-11 10:22 ` [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
@ 2022-10-18 11:37 ` BALATON Zoltan
2022-10-28 9:03 ` Mark Cave-Ayland
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-18 11:37 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On Tue, 11 Oct 2022, BALATON Zoltan wrote:
> On Mon, 3 Oct 2022, BALATON Zoltan wrote:
>> This series includes some clean ups to mac_newworld and mac_oldworld
>> to make them a bit simpler and more readable, It also removes the
>> shared mac.h file that turns out was more of a random collection of
>> unrelated things. Getting rid of this mac.h improves the locality of
>> device models and reduces unnecessary interdependency.
>
> Ping?
Ping^2 Only patch 4-5 still need a review. This series is a quite simple
clean up with no functional change and it's on the list for a month now
with this v3 is waiting for the last two weeks. I hoped to do some more
changes after this was merged but with this rate of maintainer activity
I'm not sure even this simple clean up can make it until the freeze and
there seems to be no hope to get in more changes this year, We need to do
something about this situation as it hinders development. It should not be
so difficult to make even simple changes.
Regards,
BALATON Zoltan
>> v3: Some more patch spliting and changes I've noticed and address more
>> review comments
>> v2: Split some patches and add a few more I've noticed now and address
>> review comments
>>
>> BALATON Zoltan (13):
>> mac_newworld: Drop some variables
>> mac_oldworld: Drop some more variables
>> mac_{old|new}world: Set tbfreq at declaration
>> mac_{old|new}world: Avoid else branch by setting default value
>> mac_{old|new}world: Simplify cmdline_base calculation
>> mac_newworld: Clean up creation of Uninorth devices
>> mac_{old|new}world: Reduce number of QOM casts
>> hw/ppc/mac.h: Move newworld specific parts out from shared header
>> hw/ppc/mac.h: Move macio specific parts out from shared header
>> hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header
>> hw/ppc/mac.h: Move PROM and KERNEL defines to board code
>> hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h
>> mac_nvram: Use NVRAM_SIZE constant
>>
>> MAINTAINERS | 2 +
>> hw/ide/macio.c | 1 -
>> hw/intc/heathrow_pic.c | 1 -
>> hw/intc/openpic.c | 1 -
>> hw/misc/macio/cuda.c | 1 -
>> hw/misc/macio/gpio.c | 1 -
>> hw/misc/macio/macio.c | 8 +-
>> hw/misc/macio/pmu.c | 1 -
>> hw/nvram/mac_nvram.c | 2 +-
>> hw/pci-host/grackle.c | 15 +--
>> hw/pci-host/uninorth.c | 1 -
>> hw/ppc/mac.h | 105 ----------------
>> hw/ppc/mac_newworld.c | 225 ++++++++++++++++------------------
>> hw/ppc/mac_oldworld.c | 111 +++++++----------
>> include/hw/misc/macio/macio.h | 23 +++-
>> include/hw/nvram/mac_nvram.h | 51 ++++++++
>> include/hw/pci-host/grackle.h | 44 +++++++
>> 17 files changed, 280 insertions(+), 313 deletions(-)
>> delete mode 100644 hw/ppc/mac.h
>> create mode 100644 include/hw/nvram/mac_nvram.h
>> create mode 100644 include/hw/pci-host/grackle.h
>>
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation
2022-10-14 15:25 ` BALATON Zoltan
@ 2022-10-28 8:54 ` Mark Cave-Ayland
2022-10-28 11:59 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2022-10-28 8:54 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc
On 14/10/2022 16:25, BALATON Zoltan wrote:
> On Fri, 14 Oct 2022, BALATON Zoltan wrote:
>> On Fri, 14 Oct 2022, Mark Cave-Ayland wrote:
>>> On 03/10/2022 21:13, BALATON Zoltan wrote:
>>>
>>>> By slight reorganisation we can avoid an else branch and some code
>>>> duplication which makes it easier to follow the code.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> hw/ppc/mac_newworld.c | 6 +++---
>>>> hw/ppc/mac_oldworld.c | 7 +++----
>>>> 2 files changed, 6 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>>> index 6bc3bd19be..73b01e8c6d 100644
>>>> --- a/hw/ppc/mac_newworld.c
>>>> +++ b/hw/ppc/mac_newworld.c
>>>> @@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
>>>> machine->kernel_filename);
>>>> exit(1);
>>>> }
>>>> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>> + KERNEL_GAP);
>>>> /* load initrd */
>>>> if (machine->initrd_filename) {
>>>> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>> KERNEL_GAP);
>>>> + initrd_base = cmdline_base;
>>>> initrd_size = load_image_targphys(machine->initrd_filename,
>>>> initrd_base,
>>>> machine->ram_size - initrd_base);
>>>> @@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
>>>> exit(1);
>>>> }
>>>> cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
>>>> - } else {
>>>> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>> KERNEL_GAP);
>>>> }
>>>> ppc_boot_device = 'm';
>>>> } else {
>>>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>>>> index cb67e44081..b424729a39 100644
>>>> --- a/hw/ppc/mac_oldworld.c
>>>> +++ b/hw/ppc/mac_oldworld.c
>>>> @@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState *machine)
>>>> machine->kernel_filename);
>>>> exit(1);
>>>> }
>>>> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>> + KERNEL_GAP);
>>>> /* load initrd */
>>>> if (machine->initrd_filename) {
>>>> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>> - KERNEL_GAP);
>>>> + initrd_base = cmdline_base;
>>>> initrd_size = load_image_targphys(machine->initrd_filename,
>>>> initrd_base,
>>>> machine->ram_size - initrd_base);
>>>> @@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
>>>> exit(1);
>>>> }
>>>> cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
>>>> - } else {
>>>> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>> KERNEL_GAP);
>>>> }
>>>> ppc_boot_device = 'm';
>>>> } else {
>>>
>>> Is there any particular reason why you would want to avoid the else branch? I
>>
>> It avoids code duplication and to me it's easier to follow than an else branch.
>> With this patch cmdline_base calculation is clearly tied to kernel_base and
>> kernel_size if only kernel is used and initrd_base initrd_size when initrd is used.
>> With the else branch it's less obvious because it's set much later in the else
>> branch while initrd_base duplicates it above. So avoiding this duplication makes
>> the code easier to read and less prone to errors if the calculation is ever modified.
>>
>>> don't feel this patch is an improvement since by always setting cmdline_base to a
>>> non-zero value, as a reviewer I then have to check all other uses of cmdline_base
>>> in the file to ensure that this doesn't cause any issues.
>>
>> There aren't that many uses that it's difficult to check and this only need to be
>> done once.
>>
>>> I much prefer the existing version since setting the values of cmdline_base and
>>> initrd_base is very clearly scoped within the if statement.
>>
>> What can I say, it's hard to argue with preferences but avoiding code duplication
>> is worth the effort reviewing this patch in my opinion.
>
> Also compare the before and after this series:
>
> https://github.com/patchew-project/qemu/blob/master/hw/ppc/mac_newworld.c
> https://github.com/patchew-project/qemu/blob/9c1cd2828b3d3bd3a7068134a57ae9bb07f5a681/hw/ppc/mac_newworld.c
>
>
> I think the result is much easier to follow without else brances as you can read it
> from top to bottom instead of jumping around in else branches that is less clear and
> also more lines. Also setting default value avoids any used uninitialised cases which
> you would need to check if you set vars in if-else instead so unlike what you say
> this does not introduce such cases but closes the possibility instead. So please take
> the time to review it in exchange of the time I've put it in writing it.
I've revisited this looking at the links provided above, and after consideration my
opinion is that that having the more localised scoping of the variables is more
worthwhile (i.e. the compiler can better catch errors) rather than eliminating the
duplication for a couple of lines. Please drop this patch before posting the next
version of the series.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 00/13] Misc ppc/mac machines clean up
2022-10-18 11:37 ` BALATON Zoltan
@ 2022-10-28 9:03 ` Mark Cave-Ayland
0 siblings, 0 replies; 24+ messages in thread
From: Mark Cave-Ayland @ 2022-10-28 9:03 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 18/10/2022 12:37, BALATON Zoltan wrote:
> On Tue, 11 Oct 2022, BALATON Zoltan wrote:
>> On Mon, 3 Oct 2022, BALATON Zoltan wrote:
>>> This series includes some clean ups to mac_newworld and mac_oldworld
>>> to make them a bit simpler and more readable, It also removes the
>>> shared mac.h file that turns out was more of a random collection of
>>> unrelated things. Getting rid of this mac.h improves the locality of
>>> device models and reduces unnecessary interdependency.
>>
>> Ping?
>
> Ping^2 Only patch 4-5 still need a review. This series is a quite simple clean up
> with no functional change and it's on the list for a month now with this v3 is
> waiting for the last two weeks. I hoped to do some more changes after this was merged
> but with this rate of maintainer activity I'm not sure even this simple clean up can
> make it until the freeze and there seems to be no hope to get in more changes this
> year, We need to do something about this situation as it hinders development. It
> should not be so difficult to make even simple changes.
I've had a look at patch 4 in the v3 series and that seems okay to me (I think I may
have simply missed adding a R-B tag?), so if you can resend a v4 with patch 5 removed
then I will send a PR for before freeze.
I appreciate that the rate of patch review can be frustrating, but this is something
that is true for all QEMU developers. Remember there is no commercial sponsorship for
the Mac PPC machines so the speed of review and testing is often limited by work
deadlines and/or personal circumstances.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation
2022-10-28 8:54 ` Mark Cave-Ayland
@ 2022-10-28 11:59 ` BALATON Zoltan
0 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2022-10-28 11:59 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 6838 bytes --]
On Fri, 28 Oct 2022, Mark Cave-Ayland wrote:
> On 14/10/2022 16:25, BALATON Zoltan wrote:
>> On Fri, 14 Oct 2022, BALATON Zoltan wrote:
>>> On Fri, 14 Oct 2022, Mark Cave-Ayland wrote:
>>>> On 03/10/2022 21:13, BALATON Zoltan wrote:
>>>>
>>>>> By slight reorganisation we can avoid an else branch and some code
>>>>> duplication which makes it easier to follow the code.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>> hw/ppc/mac_newworld.c | 6 +++---
>>>>> hw/ppc/mac_oldworld.c | 7 +++----
>>>>> 2 files changed, 6 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>>>> index 6bc3bd19be..73b01e8c6d 100644
>>>>> --- a/hw/ppc/mac_newworld.c
>>>>> +++ b/hw/ppc/mac_newworld.c
>>>>> @@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
>>>>> machine->kernel_filename);
>>>>> exit(1);
>>>>> }
>>>>> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>>> + KERNEL_GAP);
>>>>> /* load initrd */
>>>>> if (machine->initrd_filename) {
>>>>> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>>> KERNEL_GAP);
>>>>> + initrd_base = cmdline_base;
>>>>> initrd_size =
>>>>> load_image_targphys(machine->initrd_filename,
>>>>> initrd_base,
>>>>> machine->ram_size -
>>>>> initrd_base);
>>>>> @@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
>>>>> exit(1);
>>>>> }
>>>>> cmdline_base = TARGET_PAGE_ALIGN(initrd_base +
>>>>> initrd_size);
>>>>> - } else {
>>>>> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size
>>>>> + KERNEL_GAP);
>>>>> }
>>>>> ppc_boot_device = 'm';
>>>>> } else {
>>>>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>>>>> index cb67e44081..b424729a39 100644
>>>>> --- a/hw/ppc/mac_oldworld.c
>>>>> +++ b/hw/ppc/mac_oldworld.c
>>>>> @@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState
>>>>> *machine)
>>>>> machine->kernel_filename);
>>>>> exit(1);
>>>>> }
>>>>> + cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>>> + KERNEL_GAP);
>>>>> /* load initrd */
>>>>> if (machine->initrd_filename) {
>>>>> - initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
>>>>> - KERNEL_GAP);
>>>>> + initrd_base = cmdline_base;
>>>>> initrd_size =
>>>>> load_image_targphys(machine->initrd_filename,
>>>>> initrd_base,
>>>>> machine->ram_size -
>>>>> initrd_base);
>>>>> @@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
>>>>> exit(1);
>>>>> }
>>>>> cmdline_base = TARGET_PAGE_ALIGN(initrd_base +
>>>>> initrd_size);
>>>>> - } else {
>>>>> - cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size
>>>>> + KERNEL_GAP);
>>>>> }
>>>>> ppc_boot_device = 'm';
>>>>> } else {
>>>>
>>>> Is there any particular reason why you would want to avoid the else
>>>> branch? I
>>>
>>> It avoids code duplication and to me it's easier to follow than an else
>>> branch. With this patch cmdline_base calculation is clearly tied to
>>> kernel_base and kernel_size if only kernel is used and initrd_base
>>> initrd_size when initrd is used. With the else branch it's less obvious
>>> because it's set much later in the else branch while initrd_base
>>> duplicates it above. So avoiding this duplication makes the code easier to
>>> read and less prone to errors if the calculation is ever modified.
>>>
>>>> don't feel this patch is an improvement since by always setting
>>>> cmdline_base to a non-zero value, as a reviewer I then have to check all
>>>> other uses of cmdline_base in the file to ensure that this doesn't cause
>>>> any issues.
>>>
>>> There aren't that many uses that it's difficult to check and this only
>>> need to be done once.
>>>
>>>> I much prefer the existing version since setting the values of
>>>> cmdline_base and initrd_base is very clearly scoped within the if
>>>> statement.
>>>
>>> What can I say, it's hard to argue with preferences but avoiding code
>>> duplication is worth the effort reviewing this patch in my opinion.
>>
>> Also compare the before and after this series:
>>
>> https://github.com/patchew-project/qemu/blob/master/hw/ppc/mac_newworld.c
>> https://github.com/patchew-project/qemu/blob/9c1cd2828b3d3bd3a7068134a57ae9bb07f5a681/hw/ppc/mac_newworld.c
>>
>> I think the result is much easier to follow without else brances as you can
>> read it from top to bottom instead of jumping around in else branches that
>> is less clear and also more lines. Also setting default value avoids any
>> used uninitialised cases which you would need to check if you set vars in
>> if-else instead so unlike what you say this does not introduce such cases
>> but closes the possibility instead. So please take the time to review it in
>> exchange of the time I've put it in writing it.
>
> I've revisited this looking at the links provided above, and after
> consideration my opinion is that that having the more localised scoping of
> the variables is more worthwhile (i.e. the compiler can better catch errors)
> rather than eliminating the duplication for a couple of lines. Please drop
> this patch before posting the next version of the series.
I think duplicating the same calculation for both initrd_base and
cmdline_base in the else branch is error prone so removing that is better
but I've sent a v6 with this patch removed so you can chose which one to
apply before the freeze.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-10-28 11:59 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-03 20:13 [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 01/13] mac_newworld: Drop some variables BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 02/13] mac_oldworld: Drop some more variables BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 03/13] mac_{old|new}world: Set tbfreq at declaration BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 04/13] mac_{old|new}world: Avoid else branch by setting default value BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 05/13] mac_{old|new}world: Simplify cmdline_base calculation BALATON Zoltan
2022-10-14 8:57 ` Mark Cave-Ayland
2022-10-14 11:56 ` BALATON Zoltan
2022-10-14 15:25 ` BALATON Zoltan
2022-10-28 8:54 ` Mark Cave-Ayland
2022-10-28 11:59 ` BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 06/13] mac_newworld: Clean up creation of Uninorth devices BALATON Zoltan
2022-10-14 9:04 ` Mark Cave-Ayland
2022-10-03 20:13 ` [PATCH v3 07/13] mac_{old|new}world: Reduce number of QOM casts BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 08/13] hw/ppc/mac.h: Move newworld specific parts out from shared header BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 09/13] hw/ppc/mac.h: Move macio " BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 10/13] hw/ppc/mac.h: Move grackle-pcihost type declaration out to a header BALATON Zoltan
2022-10-14 9:08 ` Mark Cave-Ayland
2022-10-03 20:13 ` [PATCH v3 11/13] hw/ppc/mac.h: Move PROM and KERNEL defines to board code BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 12/13] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h BALATON Zoltan
2022-10-03 20:13 ` [PATCH v3 13/13] mac_nvram: Use NVRAM_SIZE constant BALATON Zoltan
2022-10-11 10:22 ` [PATCH v3 00/13] Misc ppc/mac machines clean up BALATON Zoltan
2022-10-18 11:37 ` BALATON Zoltan
2022-10-28 9:03 ` Mark Cave-Ayland
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.