Re: [PATCH 04/14] sysbus: Add sysbus_mmio_map_in

[Peter Delevoryas]

> On Jun 23, 2022, at 5:15 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Thu, 23 Jun 2022 at 11:56, Peter Delevoryas <pdel@fb.com> wrote:
>> 
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> hw/core/sysbus.c    | 6 ++++++
>> include/hw/sysbus.h | 2 ++
>> 2 files changed, 8 insertions(+)
>> 
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index cb4d6bae9d..7b63ec3fed 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -160,6 +160,12 @@ void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr 
>> addr)
>>     sysbus_mmio_map_common(dev, n, addr, false, 0, get_system_memory());
>> }
>> 
>> +void sysbus_mmio_map_in(SysBusDevice *dev, int n, hwaddr addr,
>> +                        MemoryRegion *system_memory)
>> +{
>> +    sysbus_mmio_map_common(dev, n, addr, false, 0, system_memory);
>> +}
>> +
>> void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
>>                              int priority)
>> {
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index a7c23d5fb1..f4578029e4 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -80,6 +80,8 @@ void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq 
>> irq);
>> bool sysbus_is_irq_connected(SysBusDevice *dev, int n);
>> qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n);
>> void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
>> +void sysbus_mmio_map_in(SysBusDevice *dev, int n, hwaddr addr,
>> +                        MemoryRegion *system_memory);
>> void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
>>                              int priority);
> 
> What's this going to be used for?

At the moment, just for SoC's to map peripherals into its memory region.

> 
> The current standard way to map a sysbus MMIO region into something
> other than the global system memory region is to do:
>   memory_region_add_subregion(&container, addr,
>                               sysbus_mmio_get_region(sbd, 0));

Oh!! Wait a minute. I should just change the Aspeed SoC code to
do that. I was kinda thinking that we wanted all the SoC code
to go through sysbus_mmio_map_common, but I realize now that
it’s really just a small helper that does some error checking,
and we can just implement that in an aspeed-specific helper if
necessary.

> 
> I'd rather not have two ways to do the same thing; we have
> far too many of those already.

Totally agree, I wasn’t very confident in this patch anyways.
I’ll remove it from the series and refactor the SoC board
patch following this to do what you suggested.

Thanks!!
Peter

> 
> thanks
> -- PMM