| Message ID | f6b0aa3528e9bd538c7111cd29e89a0c3623fbe5.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: > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/ppc/mac_oldworld.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c > index cb67e44081..75fbd2a7df 100644 > --- a/hw/ppc/mac_oldworld.c > +++ b/hw/ppc/mac_oldworld.c > @@ -218,16 +218,12 @@ static void ppc_heathrow_init(MachineState *machine) > qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000); > s = SYS_BUS_DEVICE(grackle_dev); > sysbus_realize_and_unref(s, &error_fatal); > - > sysbus_mmio_map(s, 0, GRACKLE_BASE); > sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000); > /* PCI hole */ > - memory_region_add_subregion(get_system_memory(), 0x80000000ULL, > - sysbus_mmio_get_region(s, 2)); > + sysbus_mmio_map(s, 2, 0x80000000); > /* Register 2 MB of ISA IO space */ > - memory_region_add_subregion(get_system_memory(), 0xfe000000, > - sysbus_mmio_get_region(s, 3)); > - > + sysbus_mmio_map(s, 3, 0xfe000000); > pci_bus = PCI_HOST_BRIDGE(grackle_dev)->bus; > > /* MacIO */ Please drop this patch for now. The code was written on assumption that both sysbus and sysbus devices would be going away soon, and there are certainly discussions under way about coming up with a migration strategy to allow them to be completely removed. ATB, Mark.
On Thu, 29 Sep 2022, Mark Cave-Ayland wrote: > On 25/09/2022 13:38, BALATON Zoltan wrote: > >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/ppc/mac_oldworld.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c >> index cb67e44081..75fbd2a7df 100644 >> --- a/hw/ppc/mac_oldworld.c >> +++ b/hw/ppc/mac_oldworld.c >> @@ -218,16 +218,12 @@ static void ppc_heathrow_init(MachineState *machine) >> qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000); >> s = SYS_BUS_DEVICE(grackle_dev); >> sysbus_realize_and_unref(s, &error_fatal); >> - >> sysbus_mmio_map(s, 0, GRACKLE_BASE); >> sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000); >> /* PCI hole */ >> - memory_region_add_subregion(get_system_memory(), 0x80000000ULL, >> - sysbus_mmio_get_region(s, 2)); >> + sysbus_mmio_map(s, 2, 0x80000000); >> /* Register 2 MB of ISA IO space */ >> - memory_region_add_subregion(get_system_memory(), 0xfe000000, >> - sysbus_mmio_get_region(s, 3)); >> - >> + sysbus_mmio_map(s, 3, 0xfe000000); >> pci_bus = PCI_HOST_BRIDGE(grackle_dev)->bus; >> /* MacIO */ > > Please drop this patch for now. The code was written on assumption that both > sysbus and sysbus devices would be going away soon, and there are certainly > discussions under way about coming up with a migration strategy to allow them > to be completely removed. This patch actually simplifies transition from sysbus to whatever else will be decided because then you'll surely have a way to replace sysbus_mmio_map() that's used everywhere else with something. This file now has both sysbus_mmio_map and sysbus mmio_get_region so using only one will make it easier to convert it and until then it's easier to read so I don't agree with this suggestion and want to stick to these patches (same with uninorth). Please reconsider your decision. Regards, BALATON Zoltan
On 29/09/2022 12:32, BALATON Zoltan wrote: > On Thu, 29 Sep 2022, Mark Cave-Ayland wrote: >> On 25/09/2022 13:38, BALATON Zoltan wrote: >> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> hw/ppc/mac_oldworld.c | 8 ++------ >>> 1 file changed, 2 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c >>> index cb67e44081..75fbd2a7df 100644 >>> --- a/hw/ppc/mac_oldworld.c >>> +++ b/hw/ppc/mac_oldworld.c >>> @@ -218,16 +218,12 @@ static void ppc_heathrow_init(MachineState *machine) >>> qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000); >>> s = SYS_BUS_DEVICE(grackle_dev); >>> sysbus_realize_and_unref(s, &error_fatal); >>> - >>> sysbus_mmio_map(s, 0, GRACKLE_BASE); >>> sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000); >>> /* PCI hole */ >>> - memory_region_add_subregion(get_system_memory(), 0x80000000ULL, >>> - sysbus_mmio_get_region(s, 2)); >>> + sysbus_mmio_map(s, 2, 0x80000000); >>> /* Register 2 MB of ISA IO space */ >>> - memory_region_add_subregion(get_system_memory(), 0xfe000000, >>> - sysbus_mmio_get_region(s, 3)); >>> - >>> + sysbus_mmio_map(s, 3, 0xfe000000); >>> pci_bus = PCI_HOST_BRIDGE(grackle_dev)->bus; >>> /* MacIO */ >> >> Please drop this patch for now. The code was written on assumption that both sysbus >> and sysbus devices would be going away soon, and there are certainly discussions >> under way about coming up with a migration strategy to allow them to be completely >> removed. > > This patch actually simplifies transition from sysbus to whatever else will be > decided because then you'll surely have a way to replace sysbus_mmio_map() that's > used everywhere else with something. This file now has both sysbus_mmio_map and > sysbus mmio_get_region so using only one will make it easier to convert it and until > then it's easier to read so I don't agree with this suggestion and want to stick to > these patches (same with uninorth). Please reconsider your decision. When sysbus eventually goes then mapping devices will most likely be handled by the memory API as above rather than using an explicit _map() API, so let's keep that rather than converting everything to use sysbus_mmio_map(). ATB, Mark.
On Mon, 3 Oct 2022, Mark Cave-Ayland wrote: > On 29/09/2022 12:32, BALATON Zoltan wrote: > >> On Thu, 29 Sep 2022, Mark Cave-Ayland wrote: >>> On 25/09/2022 13:38, BALATON Zoltan wrote: >>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> hw/ppc/mac_oldworld.c | 8 ++------ >>>> 1 file changed, 2 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c >>>> index cb67e44081..75fbd2a7df 100644 >>>> --- a/hw/ppc/mac_oldworld.c >>>> +++ b/hw/ppc/mac_oldworld.c >>>> @@ -218,16 +218,12 @@ static void ppc_heathrow_init(MachineState >>>> *machine) >>>> qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000); >>>> s = SYS_BUS_DEVICE(grackle_dev); >>>> sysbus_realize_and_unref(s, &error_fatal); >>>> - >>>> sysbus_mmio_map(s, 0, GRACKLE_BASE); >>>> sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000); >>>> /* PCI hole */ >>>> - memory_region_add_subregion(get_system_memory(), 0x80000000ULL, >>>> - sysbus_mmio_get_region(s, 2)); >>>> + sysbus_mmio_map(s, 2, 0x80000000); >>>> /* Register 2 MB of ISA IO space */ >>>> - memory_region_add_subregion(get_system_memory(), 0xfe000000, >>>> - sysbus_mmio_get_region(s, 3)); >>>> - >>>> + sysbus_mmio_map(s, 3, 0xfe000000); >>>> pci_bus = PCI_HOST_BRIDGE(grackle_dev)->bus; >>>> /* MacIO */ >>> >>> Please drop this patch for now. The code was written on assumption that >>> both sysbus and sysbus devices would be going away soon, and there are >>> certainly discussions under way about coming up with a migration strategy >>> to allow them to be completely removed. >> >> This patch actually simplifies transition from sysbus to whatever else will >> be decided because then you'll surely have a way to replace >> sysbus_mmio_map() that's used everywhere else with something. This file now >> has both sysbus_mmio_map and sysbus mmio_get_region so using only one will >> make it easier to convert it and until then it's easier to read so I don't >> agree with this suggestion and want to stick to these patches (same with >> uninorth). Please reconsider your decision. > > When sysbus eventually goes then mapping devices will most likely be handled > by the memory API as above rather than using an explicit _map() API, so let's > keep that rather than converting everything to use sysbus_mmio_map(). Hopefully not as that would make code very unreadable and hard to get for people unfamiliar with QOM so to not scare off potential contributors please invent an easier way, at least define a macro or a function for such common operation when sysus goes away. But it may be a long time until sysbus will be gone and until then we leave this code inconsistent now using two ways to map areaswhich I don't agree with but since you cannot be convinced I've dropped these changes for now. Regards, BALATON Zoltan
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c index cb67e44081..75fbd2a7df 100644 --- a/hw/ppc/mac_oldworld.c +++ b/hw/ppc/mac_oldworld.c @@ -218,16 +218,12 @@ static void ppc_heathrow_init(MachineState *machine) qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000); s = SYS_BUS_DEVICE(grackle_dev); sysbus_realize_and_unref(s, &error_fatal); - sysbus_mmio_map(s, 0, GRACKLE_BASE); sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000); /* PCI hole */ - memory_region_add_subregion(get_system_memory(), 0x80000000ULL, - sysbus_mmio_get_region(s, 2)); + sysbus_mmio_map(s, 2, 0x80000000); /* Register 2 MB of ISA IO space */ - memory_region_add_subregion(get_system_memory(), 0xfe000000, - sysbus_mmio_get_region(s, 3)); - + sysbus_mmio_map(s, 3, 0xfe000000); pci_bus = PCI_HOST_BRIDGE(grackle_dev)->bus; /* MacIO */
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/ppc/mac_oldworld.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)