diff mbox series

[05/12] hw/pci-host/bonito: Do not use SysBus API to map local MMIO region

Message ID 20231018141151.87466-6-philmd@linaro.org
State New
Headers show
Series hw: Strengthen SysBus & QBus API | expand

Commit Message

Philippe Mathieu-Daudé Oct. 18, 2023, 2:11 p.m. UTC 
There is no point in exposing an internal MMIO region via
SysBus and directly mapping it in the very same device.

Just map it without using the SysBus API.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/bonito.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Comments

Thomas Huth Oct. 18, 2023, 4:05 p.m. UTC | #1 
On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> There is no point in exposing an internal MMIO region via
> SysBus and directly mapping it in the very same device.
> 
> Just map it without using the SysBus API.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/bonito.c | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index ee6cb85e97..3b803bcad3 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -654,7 +654,6 @@ static void bonito_host_realize(DeviceState *dev, Error **errp)
>   static void bonito_pci_realize(PCIDevice *dev, Error **errp)
>   {
>       PCIBonitoState *s = PCI_BONITO(dev);
> -    SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
>       PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
>       BonitoState *bs = s->pcihost;
>       MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
> @@ -668,48 +667,48 @@ static void bonito_pci_realize(PCIDevice *dev, Error **errp)
>       /* set the north bridge register mapping */
>       memory_region_init_io(&s->iomem, OBJECT(s), &bonito_ops, s,
>                             "north-bridge-register", BONITO_INTERNAL_REG_SIZE);
> -    sysbus_init_mmio(sysbus, &s->iomem);
> -    sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_INTERNAL_REG_BASE,
> +                                &s->iomem);
>   
>       /* set the north bridge pci configure  mapping */
>       memory_region_init_io(&phb->conf_mem, OBJECT(s), &bonito_pciconf_ops, s,
>                             "north-bridge-pci-config", BONITO_PCICONFIG_SIZE);
> -    sysbus_init_mmio(sysbus, &phb->conf_mem);
> -    sysbus_mmio_map(sysbus, 1, BONITO_PCICONFIG_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_PCICONFIG_BASE,
> +                                &phb->conf_mem);
>   
>       /* set the south bridge pci configure  mapping */
>       memory_region_init_io(&phb->data_mem, OBJECT(s), &bonito_spciconf_ops, s,
>                             "south-bridge-pci-config", BONITO_SPCICONFIG_SIZE);
> -    sysbus_init_mmio(sysbus, &phb->data_mem);
> -    sysbus_mmio_map(sysbus, 2, BONITO_SPCICONFIG_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_SPCICONFIG_BASE,
> +                                &phb->data_mem);
>   
>       create_unimplemented_device("bonito", BONITO_REG_BASE, BONITO_REG_SIZE);
>   
>       memory_region_init_io(&s->iomem_ldma, OBJECT(s), &bonito_ldma_ops, s,
>                             "ldma", 0x100);
> -    sysbus_init_mmio(sysbus, &s->iomem_ldma);
> -    sysbus_mmio_map(sysbus, 3, 0x1fe00200);
> +    memory_region_add_subregion(get_system_memory(), 0x1fe00200,
> +                                &s->iomem_ldma);
>   
>       /* PCI copier */
>       memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
>                             "cop", 0x100);
> -    sysbus_init_mmio(sysbus, &s->iomem_cop);
> -    sysbus_mmio_map(sysbus, 4, 0x1fe00300);
> +    memory_region_add_subregion(get_system_memory(), 0x1fe00300,
> +                                &s->iomem_cop);
>   
>       create_unimplemented_device("ROMCS", BONITO_FLASH_BASE, 60 * MiB);
>   
>       /* Map PCI IO Space  0x1fd0 0000 - 0x1fd1 0000 */
>       memory_region_init_alias(&s->bonito_pciio, OBJECT(s), "isa_mmio",
>                                get_system_io(), 0, BONITO_PCIIO_SIZE);
> -    sysbus_init_mmio(sysbus, &s->bonito_pciio);
> -    sysbus_mmio_map(sysbus, 5, BONITO_PCIIO_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_PCIIO_BASE,
> +                                &s->bonito_pciio);
>   
>       /* add pci local io mapping */
>   
>       memory_region_init_alias(&s->bonito_localio, OBJECT(s), "IOCS[0]",
>                                get_system_io(), 0, 256 * KiB);
> -    sysbus_init_mmio(sysbus, &s->bonito_localio);
> -    sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_DEV_BASE,
> +                                &s->bonito_localio);
>       create_unimplemented_device("IOCS[1]", BONITO_DEV_BASE + 1 * 256 * KiB,
>                                   256 * KiB);
>       create_unimplemented_device("IOCS[2]", BONITO_DEV_BASE + 2 * 256 * KiB,

Would it make sense to cache the return value of get_system_memory() in a 
local variable instead of calling it again and again ...?

  Thomas
Philippe Mathieu-Daudé Oct. 18, 2023, 6:23 p.m. UTC | #2 
On 18/10/23 18:05, Thomas Huth wrote:
> On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
>> There is no point in exposing an internal MMIO region via
>> SysBus and directly mapping it in the very same device.
>>
>> Just map it without using the SysBus API.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/pci-host/bonito.c | 29 ++++++++++++++---------------
>>   1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>> index ee6cb85e97..3b803bcad3 100644
>> --- a/hw/pci-host/bonito.c
>> +++ b/hw/pci-host/bonito.c
>> @@ -654,7 +654,6 @@ static void bonito_host_realize(DeviceState *dev, 
>> Error **errp)
>>   static void bonito_pci_realize(PCIDevice *dev, Error **errp)
>>   {
>>       PCIBonitoState *s = PCI_BONITO(dev);
>> -    SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
>>       PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
>>       BonitoState *bs = s->pcihost;
>>       MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
>> @@ -668,48 +667,48 @@ static void bonito_pci_realize(PCIDevice *dev, 
>> Error **errp)
>>       /* set the north bridge register mapping */
>>       memory_region_init_io(&s->iomem, OBJECT(s), &bonito_ops, s,
>>                             "north-bridge-register", 
>> BONITO_INTERNAL_REG_SIZE);
>> -    sysbus_init_mmio(sysbus, &s->iomem);
>> -    sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_INTERNAL_REG_BASE,
>> +                                &s->iomem);
>>       /* set the north bridge pci configure  mapping */
>>       memory_region_init_io(&phb->conf_mem, OBJECT(s), 
>> &bonito_pciconf_ops, s,
>>                             "north-bridge-pci-config", 
>> BONITO_PCICONFIG_SIZE);
>> -    sysbus_init_mmio(sysbus, &phb->conf_mem);
>> -    sysbus_mmio_map(sysbus, 1, BONITO_PCICONFIG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_PCICONFIG_BASE,
>> +                                &phb->conf_mem);
>>       /* set the south bridge pci configure  mapping */
>>       memory_region_init_io(&phb->data_mem, OBJECT(s), 
>> &bonito_spciconf_ops, s,
>>                             "south-bridge-pci-config", 
>> BONITO_SPCICONFIG_SIZE);
>> -    sysbus_init_mmio(sysbus, &phb->data_mem);
>> -    sysbus_mmio_map(sysbus, 2, BONITO_SPCICONFIG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_SPCICONFIG_BASE,
>> +                                &phb->data_mem);
>>       create_unimplemented_device("bonito", BONITO_REG_BASE, 
>> BONITO_REG_SIZE);
>>       memory_region_init_io(&s->iomem_ldma, OBJECT(s), 
>> &bonito_ldma_ops, s,
>>                             "ldma", 0x100);
>> -    sysbus_init_mmio(sysbus, &s->iomem_ldma);
>> -    sysbus_mmio_map(sysbus, 3, 0x1fe00200);
>> +    memory_region_add_subregion(get_system_memory(), 0x1fe00200,
>> +                                &s->iomem_ldma);
>>       /* PCI copier */
>>       memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
>>                             "cop", 0x100);
>> -    sysbus_init_mmio(sysbus, &s->iomem_cop);
>> -    sysbus_mmio_map(sysbus, 4, 0x1fe00300);
>> +    memory_region_add_subregion(get_system_memory(), 0x1fe00300,
>> +                                &s->iomem_cop);
>>       create_unimplemented_device("ROMCS", BONITO_FLASH_BASE, 60 * MiB);
>>       /* Map PCI IO Space  0x1fd0 0000 - 0x1fd1 0000 */
>>       memory_region_init_alias(&s->bonito_pciio, OBJECT(s), "isa_mmio",
>>                                get_system_io(), 0, BONITO_PCIIO_SIZE);
>> -    sysbus_init_mmio(sysbus, &s->bonito_pciio);
>> -    sysbus_mmio_map(sysbus, 5, BONITO_PCIIO_BASE);
>> +    memory_region_add_subregion(get_system_memory(), BONITO_PCIIO_BASE,
>> +                                &s->bonito_pciio);
>>       /* add pci local io mapping */
>>       memory_region_init_alias(&s->bonito_localio, OBJECT(s), "IOCS[0]",
>>                                get_system_io(), 0, 256 * KiB);
>> -    sysbus_init_mmio(sysbus, &s->bonito_localio);
>> -    sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
>> +    memory_region_add_subregion(get_system_memory(), BONITO_DEV_BASE,
>> +                                &s->bonito_localio);
>>       create_unimplemented_device("IOCS[1]", BONITO_DEV_BASE + 1 * 256 
>> * KiB,
>>                                   256 * KiB);
>>       create_unimplemented_device("IOCS[2]", BONITO_DEV_BASE + 2 * 256 
>> * KiB,
> 
> Would it make sense to cache the return value of get_system_memory() in 
> a local variable instead of calling it again and again ...?


This is done later, at this point I'm just doing mechanical
transformation with:

     @@
     expression sbdev;
     expression index;
     expression addr;
     expression subregion;
     @@
     -    sysbus_init_mmio(sbdev, subregion);
          ... when != sbdev
     -    sysbus_mmio_map(sbdev, index, addr);
     +    memory_region_add_subregion(get_system_memory(), addr, subregion);

     @@
     expression sbdev;
     expression index;
     expression addr;
     expression subregion;
     expression priority;
     @@
     -    sysbus_init_mmio(sbdev, subregion);
          ... when != sbdev
     -    sysbus_mmio_map_overlap(sbdev, index, addr, priority);
     +    memory_region_add_subregion_overlap(get_system_memory(), addr,
     +                                        subregion, priority);
diff mbox series

Patch

diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index ee6cb85e97..3b803bcad3 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -654,7 +654,6 @@  static void bonito_host_realize(DeviceState *dev, Error **errp)
 static void bonito_pci_realize(PCIDevice *dev, Error **errp)
 {
     PCIBonitoState *s = PCI_BONITO(dev);
-    SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
     PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
     BonitoState *bs = s->pcihost;
     MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
@@ -668,48 +667,48 @@  static void bonito_pci_realize(PCIDevice *dev, Error **errp)
     /* set the north bridge register mapping */
     memory_region_init_io(&s->iomem, OBJECT(s), &bonito_ops, s,
                           "north-bridge-register", BONITO_INTERNAL_REG_SIZE);
-    sysbus_init_mmio(sysbus, &s->iomem);
-    sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_INTERNAL_REG_BASE,
+                                &s->iomem);
 
     /* set the north bridge pci configure  mapping */
     memory_region_init_io(&phb->conf_mem, OBJECT(s), &bonito_pciconf_ops, s,
                           "north-bridge-pci-config", BONITO_PCICONFIG_SIZE);
-    sysbus_init_mmio(sysbus, &phb->conf_mem);
-    sysbus_mmio_map(sysbus, 1, BONITO_PCICONFIG_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_PCICONFIG_BASE,
+                                &phb->conf_mem);
 
     /* set the south bridge pci configure  mapping */
     memory_region_init_io(&phb->data_mem, OBJECT(s), &bonito_spciconf_ops, s,
                           "south-bridge-pci-config", BONITO_SPCICONFIG_SIZE);
-    sysbus_init_mmio(sysbus, &phb->data_mem);
-    sysbus_mmio_map(sysbus, 2, BONITO_SPCICONFIG_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_SPCICONFIG_BASE,
+                                &phb->data_mem);
 
     create_unimplemented_device("bonito", BONITO_REG_BASE, BONITO_REG_SIZE);
 
     memory_region_init_io(&s->iomem_ldma, OBJECT(s), &bonito_ldma_ops, s,
                           "ldma", 0x100);
-    sysbus_init_mmio(sysbus, &s->iomem_ldma);
-    sysbus_mmio_map(sysbus, 3, 0x1fe00200);
+    memory_region_add_subregion(get_system_memory(), 0x1fe00200,
+                                &s->iomem_ldma);
 
     /* PCI copier */
     memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
                           "cop", 0x100);
-    sysbus_init_mmio(sysbus, &s->iomem_cop);
-    sysbus_mmio_map(sysbus, 4, 0x1fe00300);
+    memory_region_add_subregion(get_system_memory(), 0x1fe00300,
+                                &s->iomem_cop);
 
     create_unimplemented_device("ROMCS", BONITO_FLASH_BASE, 60 * MiB);
 
     /* Map PCI IO Space  0x1fd0 0000 - 0x1fd1 0000 */
     memory_region_init_alias(&s->bonito_pciio, OBJECT(s), "isa_mmio",
                              get_system_io(), 0, BONITO_PCIIO_SIZE);
-    sysbus_init_mmio(sysbus, &s->bonito_pciio);
-    sysbus_mmio_map(sysbus, 5, BONITO_PCIIO_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_PCIIO_BASE,
+                                &s->bonito_pciio);
 
     /* add pci local io mapping */
 
     memory_region_init_alias(&s->bonito_localio, OBJECT(s), "IOCS[0]",
                              get_system_io(), 0, 256 * KiB);
-    sysbus_init_mmio(sysbus, &s->bonito_localio);
-    sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_DEV_BASE,
+                                &s->bonito_localio);
     create_unimplemented_device("IOCS[1]", BONITO_DEV_BASE + 1 * 256 * KiB,
                                 256 * KiB);
     create_unimplemented_device("IOCS[2]", BONITO_DEV_BASE + 2 * 256 * KiB,