Re: [PATCH v2 05/13] mac_oldworld: Do not open code sysbus_mmio_map()

[BALATON Zoltan]

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