* [PATCH 01/10] mac_newworld: Drop some variables
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-17 21:05 ` Philippe Mathieu-Daudé via
2022-09-25 8:41 ` Mark Cave-Ayland
2022-09-16 23:07 ` [PATCH 02/10] mac_oldworld: Drop some more variables BALATON Zoltan
` (9 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
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>
---
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] 38+ messages in thread* Re: [PATCH 01/10] mac_newworld: Drop some variables
2022-09-16 23:07 ` [PATCH 01/10] mac_newworld: Drop some variables BALATON Zoltan
@ 2022-09-17 21:05 ` Philippe Mathieu-Daudé via
2022-09-25 8:41 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-17 21:05 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On 17/9/22 01:07, BALATON Zoltan wrote:
> 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>
> ---
> hw/ppc/mac_newworld.c | 65 +++++++++++++++++++------------------------
> 1 file changed, 29 insertions(+), 36 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 01/10] mac_newworld: Drop some variables
2022-09-16 23:07 ` [PATCH 01/10] mac_newworld: Drop some variables BALATON Zoltan
2022-09-17 21:05 ` Philippe Mathieu-Daudé via
@ 2022-09-25 8:41 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 8:41 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> 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>
> ---
> 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);
> }
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 02/10] mac_oldworld: Drop some more variables
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
2022-09-16 23:07 ` [PATCH 01/10] mac_newworld: Drop some variables BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-17 21:06 ` Philippe Mathieu-Daudé via
2022-09-25 8:42 ` Mark Cave-Ayland
2022-09-16 23:07 ` [PATCH 03/10] mac_{old|new}world: Set default values for some local variables BALATON Zoltan
` (8 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
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>
---
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] 38+ messages in thread* Re: [PATCH 02/10] mac_oldworld: Drop some more variables
2022-09-16 23:07 ` [PATCH 02/10] mac_oldworld: Drop some more variables BALATON Zoltan
@ 2022-09-17 21:06 ` Philippe Mathieu-Daudé via
2022-09-25 8:42 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-17 21:06 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On 17/9/22 01:07, BALATON Zoltan wrote:
> 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>
> ---
> hw/ppc/mac_oldworld.c | 43 +++++++++++++++++++++----------------------
> 1 file changed, 21 insertions(+), 22 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 02/10] mac_oldworld: Drop some more variables
2022-09-16 23:07 ` [PATCH 02/10] mac_oldworld: Drop some more variables BALATON Zoltan
2022-09-17 21:06 ` Philippe Mathieu-Daudé via
@ 2022-09-25 8:42 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 8:42 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> 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>
> ---
> 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);
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 03/10] mac_{old|new}world: Set default values for some local variables
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
2022-09-16 23:07 ` [PATCH 01/10] mac_newworld: Drop some variables BALATON Zoltan
2022-09-16 23:07 ` [PATCH 02/10] mac_oldworld: Drop some more variables BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-25 8:48 ` Mark Cave-Ayland
2022-09-16 23:07 ` [PATCH 04/10] mac_newworld: Simplify creation of Uninorth devices BALATON Zoltan
` (7 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
Some lines can be dropped making the code flow simpler and easier to
follow by setting default values at variable declaration for some
variables in both mac_oldworld.c and mac_newworld.c.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ppc/mac_newworld.c | 28 +++++-----------------------
hw/ppc/mac_oldworld.c | 27 +++++----------------------
2 files changed, 10 insertions(+), 45 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 27e4e8d136..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;
@@ -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++) {
@@ -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.
@@ -343,13 +332,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..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;
@@ -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++) {
@@ -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++) {
/*
@@ -223,13 +213,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] 38+ messages in thread* Re: [PATCH 03/10] mac_{old|new}world: Set default values for some local variables
2022-09-16 23:07 ` [PATCH 03/10] mac_{old|new}world: Set default values for some local variables BALATON Zoltan
@ 2022-09-25 8:48 ` Mark Cave-Ayland
2022-09-25 9:16 ` BALATON Zoltan
0 siblings, 1 reply; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 8:48 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> Some lines can be dropped making the code flow simpler and easier to
> follow by setting default values at variable declaration for some
> variables in both mac_oldworld.c and mac_newworld.c.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/mac_newworld.c | 28 +++++-----------------------
> hw/ppc/mac_oldworld.c | 27 +++++----------------------
> 2 files changed, 10 insertions(+), 45 deletions(-)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 27e4e8d136..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;
> @@ -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++) {
> @@ -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;
This bit seems a bit odd...
> 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;
and also here. The only reason I can think that someone would explicitly set these
variables back to 0 would be if there are cases in the load_*() functions where
non-zero values could be returned for failure. It's worth having a look to confirm
this and see if this also needs some additional tweaks to the logic flow here.
> ppc_boot_device = '\0';
> /* We consider that NewWorld PowerMac never have any floppy drive
> * For now, OHW cannot boot from the network.
> @@ -343,13 +332,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..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;
> @@ -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++) {
> @@ -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++) {
> /*
> @@ -223,13 +213,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);
... and obviously the same comments for mac_oldworld.c too.
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 03/10] mac_{old|new}world: Set default values for some local variables
2022-09-25 8:48 ` Mark Cave-Ayland
@ 2022-09-25 9:16 ` BALATON Zoltan
2022-09-25 9:41 ` BALATON Zoltan
2022-09-29 7:00 ` Mark Cave-Ayland
0 siblings, 2 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-25 9:16 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
> On 17/09/2022 00:07, BALATON Zoltan wrote:
>> Some lines can be dropped making the code flow simpler and easier to
>> follow by setting default values at variable declaration for some
>> variables in both mac_oldworld.c and mac_newworld.c.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/ppc/mac_newworld.c | 28 +++++-----------------------
>> hw/ppc/mac_oldworld.c | 27 +++++----------------------
>> 2 files changed, 10 insertions(+), 45 deletions(-)
>>
>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>> index 27e4e8d136..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;
>> @@ -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++) {
>> @@ -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;
>
> This bit seems a bit odd...
>
>> 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;
>
> and also here. The only reason I can think that someone would explicitly set
> these variables back to 0 would be if there are cases in the load_*()
> functions where non-zero values could be returned for failure. It's worth
> having a look to confirm this and see if this also needs some additional
> tweaks to the logic flow here.
They aren't set back to 0 but set here the first time. Nothing touches
these variables before this if-else do this patch just moves the zero init
to the variable declaration and only leaves the cases which set a value
different than zero here which I think is easier to follow.
Regards,
BALATON Zoltan
>> ppc_boot_device = '\0';
>> /* We consider that NewWorld PowerMac never have any floppy drive
>> * For now, OHW cannot boot from the network.
>> @@ -343,13 +332,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..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;
>> @@ -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++) {
>> @@ -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++) {
>> /*
>> @@ -223,13 +213,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);
>
> ... and obviously the same comments for mac_oldworld.c too.
>
>
> ATB,
>
> Mark.
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 03/10] mac_{old|new}world: Set default values for some local variables
2022-09-25 9:16 ` BALATON Zoltan
@ 2022-09-25 9:41 ` BALATON Zoltan
2022-09-29 7:00 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-25 9:41 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
On Sun, 25 Sep 2022, BALATON Zoltan wrote:
> On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
>> On 17/09/2022 00:07, BALATON Zoltan wrote:
>>> Some lines can be dropped making the code flow simpler and easier to
>>> follow by setting default values at variable declaration for some
>>> variables in both mac_oldworld.c and mac_newworld.c.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/ppc/mac_newworld.c | 28 +++++-----------------------
>>> hw/ppc/mac_oldworld.c | 27 +++++----------------------
>>> 2 files changed, 10 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>> index 27e4e8d136..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;
>>> @@ -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++) {
>>> @@ -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;
>>
>> This bit seems a bit odd...
>>
>>> 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;
>>
>> and also here. The only reason I can think that someone would explicitly
>> set these variables back to 0 would be if there are cases in the load_*()
>> functions where non-zero values could be returned for failure. It's worth
>> having a look to confirm this and see if this also needs some additional
>> tweaks to the logic flow here.
>
> They aren't set back to 0 but set here the first time. Nothing touches these
> variables before this if-else do this patch just moves the zero init to the
> variable declaration and only leaves the cases which set a value different
> than zero here which I think is easier to follow.
Checked again in the original before this patch to make sure but these are
in else branches of ifs setting the same variable to some value so I
think it's either set to value or 0 and these lines settin 0 can'r run if
the other part setting a value has run so these can't set it back, these
are just the default 0 values so setting that at the beginning can drop
these lines. What am I missing? How can these set a value back to 0?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 03/10] mac_{old|new}world: Set default values for some local variables
2022-09-25 9:16 ` BALATON Zoltan
2022-09-25 9:41 ` BALATON Zoltan
@ 2022-09-29 7:00 ` Mark Cave-Ayland
2022-10-03 22:08 ` BALATON Zoltan
1 sibling, 1 reply; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-29 7:00 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc
On 25/09/2022 10:16, BALATON Zoltan wrote:
> On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
>> On 17/09/2022 00:07, BALATON Zoltan wrote:
>>> Some lines can be dropped making the code flow simpler and easier to
>>> follow by setting default values at variable declaration for some
>>> variables in both mac_oldworld.c and mac_newworld.c.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/ppc/mac_newworld.c | 28 +++++-----------------------
>>> hw/ppc/mac_oldworld.c | 27 +++++----------------------
>>> 2 files changed, 10 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>> index 27e4e8d136..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;
>>> @@ -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++) {
>>> @@ -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;
>>
>> This bit seems a bit odd...
>>
>>> 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;
>>
>> and also here. The only reason I can think that someone would explicitly set these
>> variables back to 0 would be if there are cases in the load_*() functions where
>> non-zero values could be returned for failure. It's worth having a look to confirm
>> this and see if this also needs some additional tweaks to the logic flow here.
>
> They aren't set back to 0 but set here the first time. Nothing touches these
> variables before this if-else do this patch just moves the zero init to the variable
> declaration and only leaves the cases which set a value different than zero here
> which I think is easier to follow.
Okay - in that case if you can test with a non-kernel ELF to verify this, and add a
note confirming that everything still works for the error paths then that will be fine.
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 03/10] mac_{old|new}world: Set default values for some local variables
2022-09-29 7:00 ` Mark Cave-Ayland
@ 2022-10-03 22:08 ` BALATON Zoltan
2022-10-13 21:25 ` Mark Cave-Ayland
0 siblings, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2022-10-03 22:08 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 5324 bytes --]
On Thu, 29 Sep 2022, Mark Cave-Ayland wrote:
> On 25/09/2022 10:16, BALATON Zoltan wrote:
>> On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
>>> On 17/09/2022 00:07, BALATON Zoltan wrote:
>>>> Some lines can be dropped making the code flow simpler and easier to
>>>> follow by setting default values at variable declaration for some
>>>> variables in both mac_oldworld.c and mac_newworld.c.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> hw/ppc/mac_newworld.c | 28 +++++-----------------------
>>>> hw/ppc/mac_oldworld.c | 27 +++++----------------------
>>>> 2 files changed, 10 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>>> index 27e4e8d136..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;
>>>> @@ -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++) {
>>>> @@ -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;
>>>
>>> This bit seems a bit odd...
>>>
>>>> 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;
>>>
>>> and also here. The only reason I can think that someone would explicitly
>>> set these variables back to 0 would be if there are cases in the load_*()
>>> functions where non-zero values could be returned for failure. It's worth
>>> having a look to confirm this and see if this also needs some additional
>>> tweaks to the logic flow here.
>>
>> They aren't set back to 0 but set here the first time. Nothing touches
>> these variables before this if-else do this patch just moves the zero init
>> to the variable declaration and only leaves the cases which set a value
>> different than zero here which I think is easier to follow.
>
> Okay - in that case if you can test with a non-kernel ELF to verify this, and
> add a note confirming that everything still works for the error paths then
> that will be fine.
I've originally added non-elf loading to be able to use -bios macrom.bin
which I've now verified that it still works so this should be OK. I've
also split this patch up to more parts for easier review in the later
versions of the series but what it does is basically instead of
int x;
if (cond) {
x = a;
} else {
x = 0;
}
we do
int x = 0;
if (cond) {
x = a;
}
which I thought would be simple to review.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 03/10] mac_{old|new}world: Set default values for some local variables
2022-10-03 22:08 ` BALATON Zoltan
@ 2022-10-13 21:25 ` Mark Cave-Ayland
0 siblings, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-10-13 21:25 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc
On 03/10/2022 23:08, BALATON Zoltan wrote:
> On Thu, 29 Sep 2022, Mark Cave-Ayland wrote:
>> On 25/09/2022 10:16, BALATON Zoltan wrote:
>>> On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
>>>> On 17/09/2022 00:07, BALATON Zoltan wrote:
>>>>> Some lines can be dropped making the code flow simpler and easier to
>>>>> follow by setting default values at variable declaration for some
>>>>> variables in both mac_oldworld.c and mac_newworld.c.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>> hw/ppc/mac_newworld.c | 28 +++++-----------------------
>>>>> hw/ppc/mac_oldworld.c | 27 +++++----------------------
>>>>> 2 files changed, 10 insertions(+), 45 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>>>> index 27e4e8d136..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;
>>>>> @@ -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++) {
>>>>> @@ -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;
>>>>
>>>> This bit seems a bit odd...
>>>>
>>>>> 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;
>>>>
>>>> and also here. The only reason I can think that someone would explicitly set
>>>> these variables back to 0 would be if there are cases in the load_*() functions
>>>> where non-zero values could be returned for failure. It's worth having a look to
>>>> confirm this and see if this also needs some additional tweaks to the logic flow
>>>> here.
>>>
>>> They aren't set back to 0 but set here the first time. Nothing touches these
>>> variables before this if-else do this patch just moves the zero init to the
>>> variable declaration and only leaves the cases which set a value different than
>>> zero here which I think is easier to follow.
>>
>> Okay - in that case if you can test with a non-kernel ELF to verify this, and add a
>> note confirming that everything still works for the error paths then that will be
>> fine.
>
> I've originally added non-elf loading to be able to use -bios macrom.bin which I've
> now verified that it still works so this should be OK. I've also split this patch up
> to more parts for easier review in the later versions of the series but what it does
> is basically instead of
>
> int x;
> if (cond) {
> x = a;
> } else {
> x = 0;
> }
>
> we do
>
> int x = 0;
> if (cond) {
> x = a;
> }
>
> which I thought would be simple to review.
Great - thanks for confirming that this still works for all intended cases.
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 04/10] mac_newworld: Simplify creation of Uninorth devices
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
` (2 preceding siblings ...)
2022-09-16 23:07 ` [PATCH 03/10] mac_{old|new}world: Set default values for some local variables BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-25 8:57 ` Mark Cave-Ayland
2022-09-16 23:07 ` [PATCH 05/10] mac_{old|new}world: Reduce number of QOM casts BALATON Zoltan
` (6 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
Avoid open coding sysbus_create_simple where not necessary and
reorganise code a bit to avoid some casts to make the code more
readable.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ppc/mac_newworld.c | 50 ++++++++++++++++---------------------------
1 file changed, 19 insertions(+), 31 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 6bc3bd19be..1038477793 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,56 +268,51 @@ static void ppc_core99_init(MachineState *machine)
}
}
+ /* UniN init */
+ sysbus_create_simple(TYPE_UNI_NORTH, 0xf8000000, NULL);
+
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);
+ s = SYS_BUS_DEVICE(sysbus_create_simple(TYPE_U3_AGP_HOST_BRIDGE,
+ 0xf0800000, NULL));
+ uninorth_pci = U3_AGP_HOST_BRIDGE(s);
+ sysbus_mmio_map(s, 1, 0xf0c00000);
/* PCI hole */
memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
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);
- s = SYS_BUS_DEVICE(uninorth_agp_dev);
- sysbus_realize_and_unref(s, &error_fatal);
- sysbus_mmio_map(s, 0, 0xf0800000);
- sysbus_mmio_map(s, 1, 0xf0c00000);
+ uninorth_agp_dev = sysbus_create_simple(TYPE_UNI_NORTH_AGP_HOST_BRIDGE,
+ 0xf0800000, NULL);
+ sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_agp_dev), 1, 0xf0c00000);
/* Uninorth internal bus */
- uninorth_internal_dev = qdev_new(
- TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE);
- s = SYS_BUS_DEVICE(uninorth_internal_dev);
- sysbus_realize_and_unref(s, &error_fatal);
- sysbus_mmio_map(s, 0, 0xf4800000);
- sysbus_mmio_map(s, 1, 0xf4c00000);
+ uninorth_internal_dev = sysbus_create_simple(
+ TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE,
+ 0xf4800000, NULL);
+ sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_internal_dev), 1, 0xf4c00000);
/* Uninorth main bus */
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,
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] 38+ messages in thread* Re: [PATCH 04/10] mac_newworld: Simplify creation of Uninorth devices
2022-09-16 23:07 ` [PATCH 04/10] mac_newworld: Simplify creation of Uninorth devices BALATON Zoltan
@ 2022-09-25 8:57 ` Mark Cave-Ayland
2022-09-25 9:20 ` BALATON Zoltan
0 siblings, 1 reply; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 8:57 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> Avoid open coding sysbus_create_simple where not necessary and
> reorganise code a bit to avoid some casts to make the code more
> readable.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/mac_newworld.c | 50 ++++++++++++++++---------------------------
> 1 file changed, 19 insertions(+), 31 deletions(-)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 6bc3bd19be..1038477793 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));
Curious - is there a reason that the initialisation of UniNorth is moved to later in
the file?
> openpic_irqs = g_new0(IrqLines, machine->smp.cpus);
> for (i = 0; i < machine->smp.cpus; i++) {
> /* Mac99 IRQ connection between OpenPIC outputs pins
> @@ -275,56 +268,51 @@ static void ppc_core99_init(MachineState *machine)
> }
> }
>
> + /* UniN init */
> + sysbus_create_simple(TYPE_UNI_NORTH, 0xf8000000, NULL);
> +
I've had a look at sysbus_create_simple() as I'm not overly familiar with it, but
this is one to add to the legacy functions we really shouldn't be using these days.
Obvious flaws from looking at the code are i) it attempts to map/wire devices in a
_simple() function in contrast to all the other _simple() functions and ii) it
assumes that properties are ordered (we can't guarantee this, as per the current
array property breakage). So please keep this as-is.
> 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);
> + s = SYS_BUS_DEVICE(sysbus_create_simple(TYPE_U3_AGP_HOST_BRIDGE,
> + 0xf0800000, NULL));
> + uninorth_pci = U3_AGP_HOST_BRIDGE(s);
> + sysbus_mmio_map(s, 1, 0xf0c00000);
> /* PCI hole */
> memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
> 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);
> - s = SYS_BUS_DEVICE(uninorth_agp_dev);
> - sysbus_realize_and_unref(s, &error_fatal);
> - sysbus_mmio_map(s, 0, 0xf0800000);
> - sysbus_mmio_map(s, 1, 0xf0c00000);
> + uninorth_agp_dev = sysbus_create_simple(TYPE_UNI_NORTH_AGP_HOST_BRIDGE,
> + 0xf0800000, NULL);
> + sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_agp_dev), 1, 0xf0c00000);
Yeah sysbus_create_simple() makes this uglier.
> /* Uninorth internal bus */
> - uninorth_internal_dev = qdev_new(
> - TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE);
> - s = SYS_BUS_DEVICE(uninorth_internal_dev);
> - sysbus_realize_and_unref(s, &error_fatal);
> - sysbus_mmio_map(s, 0, 0xf4800000);
> - sysbus_mmio_map(s, 1, 0xf4c00000);
> + uninorth_internal_dev = sysbus_create_simple(
> + TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE,
> + 0xf4800000, NULL);
> + sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_internal_dev), 1, 0xf4c00000);
>
> /* Uninorth main bus */
> 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,
> 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;
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 04/10] mac_newworld: Simplify creation of Uninorth devices
2022-09-25 8:57 ` Mark Cave-Ayland
@ 2022-09-25 9:20 ` BALATON Zoltan
0 siblings, 0 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-25 9:20 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
> On 17/09/2022 00:07, BALATON Zoltan wrote:
>> Avoid open coding sysbus_create_simple where not necessary and
>> reorganise code a bit to avoid some casts to make the code more
>> readable.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/ppc/mac_newworld.c | 50 ++++++++++++++++---------------------------
>> 1 file changed, 19 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>> index 6bc3bd19be..1038477793 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));
>
> Curious - is there a reason that the initialisation of UniNorth is moved to
> later in the file?
To move it togerher with the other parts of the uninorth. Creating this
one here maybe was needed before for some reason but apparently nothing
needs it until later so creating all the pci stuff together is more
logical.
>> openpic_irqs = g_new0(IrqLines, machine->smp.cpus);
>> for (i = 0; i < machine->smp.cpus; i++) {
>> /* Mac99 IRQ connection between OpenPIC outputs pins
>> @@ -275,56 +268,51 @@ static void ppc_core99_init(MachineState *machine)
>> }
>> }
>> + /* UniN init */
>> + sysbus_create_simple(TYPE_UNI_NORTH, 0xf8000000, NULL);
>> +
>
> I've had a look at sysbus_create_simple() as I'm not overly familiar with it,
> but this is one to add to the legacy functions we really shouldn't be using
> these days.
>
> Obvious flaws from looking at the code are i) it attempts to map/wire devices
> in a _simple() function in contrast to all the other _simple() functions and
> ii) it assumes that properties are ordered (we can't guarantee this, as per
> the current array property breakage). So please keep this as-is.
I don't think creating devices with qdev calls is easier to read than
sysbud_create_simple but OK, I'll drop this part then.
Regards,
BALATON Zoltan
>> 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);
>> + s = SYS_BUS_DEVICE(sysbus_create_simple(TYPE_U3_AGP_HOST_BRIDGE,
>> + 0xf0800000, NULL));
>> + uninorth_pci = U3_AGP_HOST_BRIDGE(s);
>> + sysbus_mmio_map(s, 1, 0xf0c00000);
>> /* PCI hole */
>> memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
>> 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);
>> - s = SYS_BUS_DEVICE(uninorth_agp_dev);
>> - sysbus_realize_and_unref(s, &error_fatal);
>> - sysbus_mmio_map(s, 0, 0xf0800000);
>> - sysbus_mmio_map(s, 1, 0xf0c00000);
>> + uninorth_agp_dev =
>> sysbus_create_simple(TYPE_UNI_NORTH_AGP_HOST_BRIDGE,
>> + 0xf0800000, NULL);
>> + sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_agp_dev), 1, 0xf0c00000);
>
> Yeah sysbus_create_simple() makes this uglier.
>
>> /* Uninorth internal bus */
>> - uninorth_internal_dev = qdev_new(
>> - TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE);
>> - s = SYS_BUS_DEVICE(uninorth_internal_dev);
>> - sysbus_realize_and_unref(s, &error_fatal);
>> - sysbus_mmio_map(s, 0, 0xf4800000);
>> - sysbus_mmio_map(s, 1, 0xf4c00000);
>> + uninorth_internal_dev = sysbus_create_simple(
>> +
>> TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE,
>> + 0xf4800000, NULL);
>> + sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_internal_dev), 1,
>> 0xf4c00000);
>> /* Uninorth main bus */
>> 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,
>> 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;
>
> ATB,
>
> Mark.
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 05/10] mac_{old|new}world: Reduce number of QOM casts
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
` (3 preceding siblings ...)
2022-09-16 23:07 ` [PATCH 04/10] mac_newworld: Simplify creation of Uninorth devices BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-17 21:13 ` Philippe Mathieu-Daudé via
2022-09-25 9:09 ` Mark Cave-Ayland
2022-09-16 23:07 ` [PATCH 06/10] hw/ppc/mac.h: Move newworld specific atuff out from shared header BALATON Zoltan
` (5 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
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>
---
hw/ppc/mac_newworld.c | 60 ++++++++++++++++++++-----------------------
hw/ppc/mac_oldworld.c | 26 ++++++++-----------
2 files changed, 39 insertions(+), 47 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 1038477793..ae90e5c353 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;
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:
@@ -275,9 +274,9 @@ static void ppc_core99_init(MachineState *machine)
machine_arch = ARCH_MAC99_U3;
/* 970 gets a U3 bus */
/* Uninorth AGP bus */
- s = SYS_BUS_DEVICE(sysbus_create_simple(TYPE_U3_AGP_HOST_BRIDGE,
- 0xf0800000, NULL));
- uninorth_pci = U3_AGP_HOST_BRIDGE(s);
+ uninorth_pci = sysbus_create_simple(TYPE_U3_AGP_HOST_BRIDGE,
+ 0xf0800000, NULL);
+ s = SYS_BUS_DEVICE(uninorth_pci);
sysbus_mmio_map(s, 1, 0xf0c00000);
/* PCI hole */
memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
@@ -300,10 +299,9 @@ static void ppc_core99_init(MachineState *machine)
sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_internal_dev), 1, 0xf4c00000);
/* Uninorth main bus */
- 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 = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
+ qdev_prop_set_uint32(uninorth_pci, "ofw-addr", 0xf2000000);
+ s = SYS_BUS_DEVICE(uninorth_pci);
sysbus_realize_and_unref(s, &error_fatal);
sysbus_mmio_map(s, 0, 0xf2800000);
sysbus_mmio_map(s, 1, 0xf2c00000);
@@ -324,21 +322,21 @@ static void ppc_core99_init(MachineState *machine)
pci_bus = PCI_HOST_BRIDGE(uninorth_pci)->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, i,
qdev_get_gpio_in(pic_dev, 0x1b + i));
}
@@ -370,19 +368,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 cb67e44081..a497507f1d 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;
@@ -231,17 +230,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));
@@ -269,16 +267,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] 38+ messages in thread* Re: [PATCH 05/10] mac_{old|new}world: Reduce number of QOM casts
2022-09-16 23:07 ` [PATCH 05/10] mac_{old|new}world: Reduce number of QOM casts BALATON Zoltan
@ 2022-09-17 21:13 ` Philippe Mathieu-Daudé via
2022-09-25 9:09 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-17 21:13 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On 17/9/22 01:07, BALATON Zoltan wrote:
> 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>
> ---
> hw/ppc/mac_newworld.c | 60 ++++++++++++++++++++-----------------------
> hw/ppc/mac_oldworld.c | 26 ++++++++-----------
> 2 files changed, 39 insertions(+), 47 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 05/10] mac_{old|new}world: Reduce number of QOM casts
2022-09-16 23:07 ` [PATCH 05/10] mac_{old|new}world: Reduce number of QOM casts BALATON Zoltan
2022-09-17 21:13 ` Philippe Mathieu-Daudé via
@ 2022-09-25 9:09 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 9:09 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> 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>
> ---
> hw/ppc/mac_newworld.c | 60 ++++++++++++++++++++-----------------------
> hw/ppc/mac_oldworld.c | 26 ++++++++-----------
> 2 files changed, 39 insertions(+), 47 deletions(-)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 1038477793..ae90e5c353 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;
Can you rename uninorth_pci to uninorth_pci_dev? The idea of the _dev suffix is to
allow you to easily spot which variables are DEVICE casts.
> 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:
> @@ -275,9 +274,9 @@ static void ppc_core99_init(MachineState *machine)
> machine_arch = ARCH_MAC99_U3;
> /* 970 gets a U3 bus */
> /* Uninorth AGP bus */
> - s = SYS_BUS_DEVICE(sysbus_create_simple(TYPE_U3_AGP_HOST_BRIDGE,
> - 0xf0800000, NULL));
> - uninorth_pci = U3_AGP_HOST_BRIDGE(s);
> + uninorth_pci = sysbus_create_simple(TYPE_U3_AGP_HOST_BRIDGE,
> + 0xf0800000, NULL);
> + s = SYS_BUS_DEVICE(uninorth_pci);
> sysbus_mmio_map(s, 1, 0xf0c00000);
> /* PCI hole */
> memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
> @@ -300,10 +299,9 @@ static void ppc_core99_init(MachineState *machine)
> sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_internal_dev), 1, 0xf4c00000);
>
> /* Uninorth main bus */
> - 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 = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
> + qdev_prop_set_uint32(uninorth_pci, "ofw-addr", 0xf2000000);
> + s = SYS_BUS_DEVICE(uninorth_pci);
> sysbus_realize_and_unref(s, &error_fatal);
> sysbus_mmio_map(s, 0, 0xf2800000);
> sysbus_mmio_map(s, 1, 0xf2c00000);
> @@ -324,21 +322,21 @@ static void ppc_core99_init(MachineState *machine)
> pci_bus = PCI_HOST_BRIDGE(uninorth_pci)->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, i,
> qdev_get_gpio_in(pic_dev, 0x1b + i));
> }
>
> @@ -370,19 +368,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 cb67e44081..a497507f1d 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;
> @@ -231,17 +230,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));
> @@ -269,16 +267,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);
with that:
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 06/10] hw/ppc/mac.h: Move newworld specific atuff out from shared header
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
` (4 preceding siblings ...)
2022-09-16 23:07 ` [PATCH 05/10] mac_{old|new}world: Reduce number of QOM casts BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-25 9:13 ` Mark Cave-Ayland
2022-09-16 23:07 ` [PATCH 07/10] hw/ppc/mac.h: Move macio " BALATON Zoltan
` (4 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 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>
---
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 ae90e5c353..14cc8cd6ea 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 a497507f1d..f323a49d7a 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] 38+ messages in thread* Re: [PATCH 06/10] hw/ppc/mac.h: Move newworld specific atuff out from shared header
2022-09-16 23:07 ` [PATCH 06/10] hw/ppc/mac.h: Move newworld specific atuff out from shared header BALATON Zoltan
@ 2022-09-25 9:13 ` Mark Cave-Ayland
0 siblings, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 9:13 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> 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>
> ---
> 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 ae90e5c353..14cc8cd6ea 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 a497507f1d..f323a49d7a 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"
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 07/10] hw/ppc/mac.h: Move macio specific atuff out from shared header
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
` (5 preceding siblings ...)
2022-09-16 23:07 ` [PATCH 06/10] hw/ppc/mac.h: Move newworld specific atuff out from shared header BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-17 21:16 ` Philippe Mathieu-Daudé via
` (2 more replies)
2022-09-16 23:07 ` [PATCH 08/10] hw/ppc/mac.h: Move grackle-pcihost declaration " BALATON Zoltan
` (3 subsequent siblings)
10 siblings, 3 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
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>
---
hw/misc/macio/macio.c | 26 ++++++++++++++++++++++++--
hw/ppc/mac.h | 23 -----------------------
2 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index c1fad43f6c..eca5983a4d 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -37,8 +37,30 @@
#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
+
+/* 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
+
+/* 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"
--
2.30.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 07/10] hw/ppc/mac.h: Move macio specific atuff out from shared header
2022-09-16 23:07 ` [PATCH 07/10] hw/ppc/mac.h: Move macio " BALATON Zoltan
@ 2022-09-17 21:16 ` Philippe Mathieu-Daudé via
2022-09-17 21:17 ` Philippe Mathieu-Daudé via
2022-09-25 9:17 ` Mark Cave-Ayland
2 siblings, 0 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-17 21:16 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On 17/9/22 01:07, BALATON Zoltan wrote:
> 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>
> ---
> hw/misc/macio/macio.c | 26 ++++++++++++++++++++++++--
> hw/ppc/mac.h | 23 -----------------------
> 2 files changed, 24 insertions(+), 25 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 07/10] hw/ppc/mac.h: Move macio specific atuff out from shared header
2022-09-16 23:07 ` [PATCH 07/10] hw/ppc/mac.h: Move macio " BALATON Zoltan
2022-09-17 21:16 ` Philippe Mathieu-Daudé via
@ 2022-09-17 21:17 ` Philippe Mathieu-Daudé via
2022-09-25 9:17 ` Mark Cave-Ayland
2 siblings, 0 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-17 21:17 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On 17/9/22 01:07, BALATON Zoltan wrote:
> 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>
> ---
> hw/misc/macio/macio.c | 26 ++++++++++++++++++++++++--
> hw/ppc/mac.h | 23 -----------------------
> 2 files changed, 24 insertions(+), 25 deletions(-)
BTW Subject: "Move macio specifics out of shared header"?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 07/10] hw/ppc/mac.h: Move macio specific atuff out from shared header
2022-09-16 23:07 ` [PATCH 07/10] hw/ppc/mac.h: Move macio " BALATON Zoltan
2022-09-17 21:16 ` Philippe Mathieu-Daudé via
2022-09-17 21:17 ` Philippe Mathieu-Daudé via
@ 2022-09-25 9:17 ` Mark Cave-Ayland
2022-09-25 9:23 ` BALATON Zoltan
2 siblings, 1 reply; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 9:17 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
Typo in subject: s/atuff/stuff/
> 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>
> ---
> hw/misc/macio/macio.c | 26 ++++++++++++++++++++++++--
> hw/ppc/mac.h | 23 -----------------------
> 2 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index c1fad43f6c..eca5983a4d 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -37,8 +37,30 @@
> #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
> +
> +/* 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
> +
> +/* Note: this code is strongly inspired by the corresponding code in PearPC */
These IRQ numbers are effectively hardcoded because of the board layout (and at some
point some of this wiring may move to the board), so I think macio.h is the best
place for these to allow for use in multiple places if needed.
> /*
> * 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"
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 07/10] hw/ppc/mac.h: Move macio specific atuff out from shared header
2022-09-25 9:17 ` Mark Cave-Ayland
@ 2022-09-25 9:23 ` BALATON Zoltan
0 siblings, 0 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-25 9:23 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
> On 17/09/2022 00:07, BALATON Zoltan wrote:
> Typo in subject: s/atuff/stuff/
>> 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>
>> ---
>> hw/misc/macio/macio.c | 26 ++++++++++++++++++++++++--
>> hw/ppc/mac.h | 23 -----------------------
>> 2 files changed, 24 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index c1fad43f6c..eca5983a4d 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -37,8 +37,30 @@
>> #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
>> +
>> +/* 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
>> +
>> +/* Note: this code is strongly inspired by the corresponding code in
>> PearPC */
>
> These IRQ numbers are effectively hardcoded because of the board layout (and
> at some point some of this wiring may move to the board), so I think macio.h
> is the best place for these to allow for use in multiple places if needed.
They aren't needed anywhere else currently that's what I've moved them
here and they could be moved elsewhere when needed but I can put it in the
header too if you like just don't see the need for that.
Regards,
BALATON Zoltan
>> /*
>> * 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"
>
>
> ATB,
>
> Mark.
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 08/10] hw/ppc/mac.h: Move grackle-pcihost declaration out from shared header
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
` (6 preceding siblings ...)
2022-09-16 23:07 ` [PATCH 07/10] hw/ppc/mac.h: Move macio " BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-25 9:21 ` Mark Cave-Ayland
2022-09-16 23:07 ` [PATCH 09/10] hw/ppc/mac.h: Move PROM and KERNEL defines to board code BALATON Zoltan
` (2 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
It is only used by mac_oldworld anyway and it already instantiates
a few devices by name so this allows reducing the shared header further.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/pci-host/grackle.c | 1 +
hw/ppc/mac.h | 3 ---
hw/ppc/mac_oldworld.c | 2 +-
3 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index b05facf463..5282123004 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -34,6 +34,7 @@
#include "trace.h"
#include "qom/object.h"
+#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
OBJECT_DECLARE_SIMPLE_TYPE(GrackleState, GRACKLE_PCI_HOST_BRIDGE)
struct GrackleState {
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 f323a49d7a..a4094226bc 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -214,7 +214,7 @@ static void ppc_heathrow_init(MachineState *machine)
}
/* Grackle PCI host bridge */
- grackle_dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
+ grackle_dev = qdev_new("grackle-pcihost");
qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000);
s = SYS_BUS_DEVICE(grackle_dev);
sysbus_realize_and_unref(s, &error_fatal);
--
2.30.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 08/10] hw/ppc/mac.h: Move grackle-pcihost declaration out from shared header
2022-09-16 23:07 ` [PATCH 08/10] hw/ppc/mac.h: Move grackle-pcihost declaration " BALATON Zoltan
@ 2022-09-25 9:21 ` Mark Cave-Ayland
2022-09-25 9:26 ` BALATON Zoltan
0 siblings, 1 reply; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 9:21 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> It is only used by mac_oldworld anyway and it already instantiates
> a few devices by name so this allows reducing the shared header further.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/pci-host/grackle.c | 1 +
> hw/ppc/mac.h | 3 ---
> hw/ppc/mac_oldworld.c | 2 +-
> 3 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index b05facf463..5282123004 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -34,6 +34,7 @@
> #include "trace.h"
> #include "qom/object.h"
>
> +#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
> OBJECT_DECLARE_SIMPLE_TYPE(GrackleState, GRACKLE_PCI_HOST_BRIDGE)
>
> struct GrackleState {
> 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 f323a49d7a..a4094226bc 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -214,7 +214,7 @@ static void ppc_heathrow_init(MachineState *machine)
> }
>
> /* Grackle PCI host bridge */
> - grackle_dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
> + grackle_dev = qdev_new("grackle-pcihost");
> qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000);
> s = SYS_BUS_DEVICE(grackle_dev);
> sysbus_realize_and_unref(s, &error_fatal);
This is the wrong way around - we want to move towards using TYPE_ macros everywhere
for device instantiation instead of hardcoded strings.
What's really missing here is that the QOM structs and definitions for grackle.c
should be moved to a new include/hw/pci-host/grackle.h file from mac.h and included
where necessary.
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 08/10] hw/ppc/mac.h: Move grackle-pcihost declaration out from shared header
2022-09-25 9:21 ` Mark Cave-Ayland
@ 2022-09-25 9:26 ` BALATON Zoltan
2022-09-29 7:07 ` Mark Cave-Ayland
0 siblings, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-25 9:26 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
> On 17/09/2022 00:07, BALATON Zoltan wrote:
>> It is only used by mac_oldworld anyway and it already instantiates
>> a few devices by name so this allows reducing the shared header further.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/pci-host/grackle.c | 1 +
>> hw/ppc/mac.h | 3 ---
>> hw/ppc/mac_oldworld.c | 2 +-
>> 3 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
>> index b05facf463..5282123004 100644
>> --- a/hw/pci-host/grackle.c
>> +++ b/hw/pci-host/grackle.c
>> @@ -34,6 +34,7 @@
>> #include "trace.h"
>> #include "qom/object.h"
>> +#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
>> OBJECT_DECLARE_SIMPLE_TYPE(GrackleState, GRACKLE_PCI_HOST_BRIDGE)
>> struct GrackleState {
>> 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 f323a49d7a..a4094226bc 100644
>> --- a/hw/ppc/mac_oldworld.c
>> +++ b/hw/ppc/mac_oldworld.c
>> @@ -214,7 +214,7 @@ static void ppc_heathrow_init(MachineState *machine)
>> }
>> /* Grackle PCI host bridge */
>> - grackle_dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
>> + grackle_dev = qdev_new("grackle-pcihost");
>> qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000);
>> s = SYS_BUS_DEVICE(grackle_dev);
>> sysbus_realize_and_unref(s, &error_fatal);
>
> This is the wrong way around - we want to move towards using TYPE_ macros
> everywhere for device instantiation instead of hardcoded strings.
>
> What's really missing here is that the QOM structs and definitions for
> grackle.c should be moved to a new include/hw/pci-host/grackle.h file from
> mac.h and included where necessary.
That could be done any time later, this patch is good enough for now,
there are other devices instantiated this way in mac_oldworld now. I don't
want to chnage grackle here, just clean up the mac.h.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 08/10] hw/ppc/mac.h: Move grackle-pcihost declaration out from shared header
2022-09-25 9:26 ` BALATON Zoltan
@ 2022-09-29 7:07 ` Mark Cave-Ayland
0 siblings, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-29 7:07 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc
On 25/09/2022 10:26, BALATON Zoltan wrote:
> On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
>> On 17/09/2022 00:07, BALATON Zoltan wrote:
>>> It is only used by mac_oldworld anyway and it already instantiates
>>> a few devices by name so this allows reducing the shared header further.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/pci-host/grackle.c | 1 +
>>> hw/ppc/mac.h | 3 ---
>>> hw/ppc/mac_oldworld.c | 2 +-
>>> 3 files changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
>>> index b05facf463..5282123004 100644
>>> --- a/hw/pci-host/grackle.c
>>> +++ b/hw/pci-host/grackle.c
>>> @@ -34,6 +34,7 @@
>>> #include "trace.h"
>>> #include "qom/object.h"
>>> +#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
>>> OBJECT_DECLARE_SIMPLE_TYPE(GrackleState, GRACKLE_PCI_HOST_BRIDGE)
>>> struct GrackleState {
>>> 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 f323a49d7a..a4094226bc 100644
>>> --- a/hw/ppc/mac_oldworld.c
>>> +++ b/hw/ppc/mac_oldworld.c
>>> @@ -214,7 +214,7 @@ static void ppc_heathrow_init(MachineState *machine)
>>> }
>>> /* Grackle PCI host bridge */
>>> - grackle_dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
>>> + grackle_dev = qdev_new("grackle-pcihost");
>>> qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000);
>>> s = SYS_BUS_DEVICE(grackle_dev);
>>> sysbus_realize_and_unref(s, &error_fatal);
>>
>> This is the wrong way around - we want to move towards using TYPE_ macros
>> everywhere for device instantiation instead of hardcoded strings.
>>
>> What's really missing here is that the QOM structs and definitions for grackle.c
>> should be moved to a new include/hw/pci-host/grackle.h file from mac.h and included
>> where necessary.
>
> That could be done any time later, this patch is good enough for now, there are other
> devices instantiated this way in mac_oldworld now. I don't want to chnage grackle
> here, just clean up the mac.h.
It is a long-standing philosophy for QEMU that if outdated code is touched then there
should be a best effort to update it to the latest coding standards. Moving the QOM
definition to a separate header file is not too dissimilar to patch 10, so will be a
fairly trivial change.
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 09/10] hw/ppc/mac.h: Move PROM and KERNEL defines to board code
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
` (7 preceding siblings ...)
2022-09-16 23:07 ` [PATCH 08/10] hw/ppc/mac.h: Move grackle-pcihost declaration " BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-17 21:15 ` Philippe Mathieu-Daudé via
2022-09-25 9:22 ` Mark Cave-Ayland
2022-09-16 23:07 ` [PATCH 10/10] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h BALATON Zoltan
2022-09-24 13:09 ` [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
10 siblings, 2 replies; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
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. The NVRAM_SIZE define is not used so
it can be dropped. This further reduces the mac.h header.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ppc/mac.h | 6 ------
hw/ppc/mac_newworld.c | 4 ++++
hw/ppc/mac_oldworld.c | 7 ++++++-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index fe77a6c6db..3c0c3cc43d 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -29,12 +29,6 @@
#include "exec/memory.h"
#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"
OBJECT_DECLARE_SIMPLE_TYPE(MacIONVRAMState, MACIO_NVRAM)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 14cc8cd6ea..1cb10726d3 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 a4094226bc..e196090f49 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -57,10 +57,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] 38+ messages in thread* Re: [PATCH 09/10] hw/ppc/mac.h: Move PROM and KERNEL defines to board code
2022-09-16 23:07 ` [PATCH 09/10] hw/ppc/mac.h: Move PROM and KERNEL defines to board code BALATON Zoltan
@ 2022-09-17 21:15 ` Philippe Mathieu-Daudé via
2022-09-25 9:22 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-17 21:15 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On 17/9/22 01:07, BALATON Zoltan wrote:
> 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. The NVRAM_SIZE define is not used so
> it can be dropped. This further reduces the mac.h header.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/mac.h | 6 ------
> hw/ppc/mac_newworld.c | 4 ++++
> hw/ppc/mac_oldworld.c | 7 ++++++-
> 3 files changed, 10 insertions(+), 7 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 09/10] hw/ppc/mac.h: Move PROM and KERNEL defines to board code
2022-09-16 23:07 ` [PATCH 09/10] hw/ppc/mac.h: Move PROM and KERNEL defines to board code BALATON Zoltan
2022-09-17 21:15 ` Philippe Mathieu-Daudé via
@ 2022-09-25 9:22 ` Mark Cave-Ayland
1 sibling, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 9:22 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> 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. The NVRAM_SIZE define is not used so
> it can be dropped. This further reduces the mac.h header.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/mac.h | 6 ------
> hw/ppc/mac_newworld.c | 4 ++++
> hw/ppc/mac_oldworld.c | 7 ++++++-
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
> index fe77a6c6db..3c0c3cc43d 100644
> --- a/hw/ppc/mac.h
> +++ b/hw/ppc/mac.h
> @@ -29,12 +29,6 @@
> #include "exec/memory.h"
> #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"
> OBJECT_DECLARE_SIMPLE_TYPE(MacIONVRAMState, MACIO_NVRAM)
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 14cc8cd6ea..1cb10726d3 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 a4094226bc..e196090f49 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -57,10 +57,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)
> {
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 10/10] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
` (8 preceding siblings ...)
2022-09-16 23:07 ` [PATCH 09/10] hw/ppc/mac.h: Move PROM and KERNEL defines to board code BALATON Zoltan
@ 2022-09-16 23:07 ` BALATON Zoltan
2022-09-25 9:23 ` Mark Cave-Ayland
2022-09-24 13:09 ` [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
10 siblings, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-16 23:07 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>
---
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 | 12 ++++++------
15 files changed, 10 insertions(+), 19 deletions(-)
rename hw/ppc/mac.h => include/hw/nvram/mac_nvram.h (88%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 1729c0901c..6d34e81246 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 eca5983a4d..782207647c 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 5282123004..d9c11d22e0 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -25,7 +25,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"
#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 1cb10726d3..9cec30bbd3 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 e196090f49..27d7c19f00 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 6c05f3bfd2..aebe990270 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 88%
rename from hw/ppc/mac.h
rename to include/hw/nvram/mac_nvram.h
index 3c0c3cc43d..805554b2c6 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,13 +23,12 @@
* 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"
-/* Mac NVRAM */
#define TYPE_MACIO_NVRAM "macio-nvram"
OBJECT_DECLARE_SIMPLE_TYPE(MacIONVRAMState, MACIO_NVRAM)
@@ -45,5 +44,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] 38+ messages in thread* Re: [PATCH 10/10] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h
2022-09-16 23:07 ` [PATCH 10/10] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h BALATON Zoltan
@ 2022-09-25 9:23 ` Mark Cave-Ayland
0 siblings, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-25 9:23 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 17/09/2022 00:07, BALATON Zoltan wrote:
> 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>
> ---
> 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 | 12 ++++++------
> 15 files changed, 10 insertions(+), 19 deletions(-)
> rename hw/ppc/mac.h => include/hw/nvram/mac_nvram.h (88%)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1729c0901c..6d34e81246 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 eca5983a4d..782207647c 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 5282123004..d9c11d22e0 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -25,7 +25,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"
> #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 1cb10726d3..9cec30bbd3 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 e196090f49..27d7c19f00 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 6c05f3bfd2..aebe990270 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 88%
> rename from hw/ppc/mac.h
> rename to include/hw/nvram/mac_nvram.h
> index 3c0c3cc43d..805554b2c6 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,13 +23,12 @@
> * 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"
>
> -/* Mac NVRAM */
> #define TYPE_MACIO_NVRAM "macio-nvram"
> OBJECT_DECLARE_SIMPLE_TYPE(MacIONVRAMState, MACIO_NVRAM)
>
> @@ -45,5 +44,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 */
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 00/10] Misc ppc/mac machines clean up
2022-09-16 23:07 [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
` (9 preceding siblings ...)
2022-09-16 23:07 ` [PATCH 10/10] hw/ppc/mac.h: Rename to include/hw/nvram/mac_nvram.h BALATON Zoltan
@ 2022-09-24 13:09 ` BALATON Zoltan
2022-09-24 16:20 ` Mark Cave-Ayland
10 siblings, 1 reply; 38+ messages in thread
From: BALATON Zoltan @ 2022-09-24 13:09 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland
On Sat, 17 Sep 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? Patches still needing review: 3, 4, 6, 8, 10
Regards,
BALATON Zoltan
> BALATON Zoltan (10):
> mac_newworld: Drop some variables
> mac_oldworld: Drop some more variables
> mac_{old|new}world: Set default values for some local variables
> mac_newworld: Simplify creation of Uninorth devices
> mac_{old|new}world: Reduce number of QOM casts
> hw/ppc/mac.h: Move newworld specific atuff out from shared header
> hw/ppc/mac.h: Move macio specific atuff out from shared header
> hw/ppc/mac.h: Move grackle-pcihost declaration out from shared 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
>
> 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 | 27 ++++-
> hw/misc/macio/pmu.c | 1 -
> hw/nvram/mac_nvram.c | 2 +-
> hw/pci-host/grackle.c | 2 +-
> hw/pci-host/uninorth.c | 1 -
> hw/ppc/mac.h | 105 ----------------
> hw/ppc/mac_newworld.c | 220 ++++++++++++++++------------------
> hw/ppc/mac_oldworld.c | 105 +++++++---------
> include/hw/misc/macio/macio.h | 2 +-
> include/hw/nvram/mac_nvram.h | 49 ++++++++
> 16 files changed, 222 insertions(+), 298 deletions(-)
> delete mode 100644 hw/ppc/mac.h
> create mode 100644 include/hw/nvram/mac_nvram.h
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 00/10] Misc ppc/mac machines clean up
2022-09-24 13:09 ` [PATCH 00/10] Misc ppc/mac machines clean up BALATON Zoltan
@ 2022-09-24 16:20 ` Mark Cave-Ayland
0 siblings, 0 replies; 38+ messages in thread
From: Mark Cave-Ayland @ 2022-09-24 16:20 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
On 24/09/2022 14:09, BALATON Zoltan wrote:
> On Sat, 17 Sep 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? Patches still needing review: 3, 4, 6, 8, 10
>
> Regards,
> BALATON Zoltan
I've tagged this series for review, and plan to have a look over it either later
today or tomorrow.
ATB,
Mark.
^ permalink raw reply [flat|nested] 38+ messages in thread