| Message ID | 2ff9b0ca151cab09512b37d855d03eee4a62812a.1664108862.git.balaton@eik.bme.hu |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Misc ppc/mac machines clean up | expand |
On 25/09/2022 13:38, 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 1fa7b770b7..1355d032ff 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); Why did you include this patch again in v2 when I nacked it in v1? ATB, Mark.
On Thu, 29 Sep 2022, Mark Cave-Ayland wrote: > On 25/09/2022 13:38, 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 1fa7b770b7..1355d032ff 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); > > Why did you include this patch again in v2 when I nacked it in v1? You did not nack it just said you'd prefer a header instead. As a reviwer you express your opinion not an absolute decision that can't be discussed. I've replied to that but could not drop this patch as it's needed for later patches to get mac.h cleaned so until we agree on something this has left unchanged. I'm not a fan of one line headers and splitting up files into separate directories that is harder to work with and also think reorganising grackle is a separate clean up out of scope for this series but OK, I'll move the TYPE define in a new header in the next version. I'll wait for your reply on sysbus_mmio_map before sending a new version. Regards, BALATON Zoltan
On 29/09/2022 12:42, BALATON Zoltan wrote: > On Thu, 29 Sep 2022, Mark Cave-Ayland wrote: >> On 25/09/2022 13:38, 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 1fa7b770b7..1355d032ff 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); >> >> Why did you include this patch again in v2 when I nacked it in v1? > > You did not nack it just said you'd prefer a header instead. As a reviwer you express > your opinion not an absolute decision that can't be discussed. I've replied to that > but could not drop this patch as it's needed for later patches to get mac.h cleaned > so until we agree on something this has left unchanged. I'm not a fan of one line > headers and splitting up files into separate directories that is harder to work with > and also think reorganising grackle is a separate clean up out of scope for this > series but OK, I'll move the TYPE define in a new header in the next version. I'll > wait for your reply on sysbus_mmio_map before sending a new version. I should add this obviously also includes moving the GrackleState struct to the new header file at the same time, as per typical QOM practice. ATB, Mark.
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 1fa7b770b7..1355d032ff 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);
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(-)