diff mbox series

[09/12] hw/pci-host/bonito: Map the different PCI ranges more detailled

Message ID 20200510210128.18343-10-f4bug@amsat.org
State New
Headers show
Series hw/mips/fuloong2e: PoC to fix hang after reboot | expand

Commit Message

Philippe Mathieu-Daudé May 10, 2020, 9:01 p.m. UTC 
Better describe the Bonito64 MEM HI/LO and I/O PCI ranges,
add more PCI regions as unimplemented.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/bonito.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

Comments

Aleksandar Markovic May 11, 2020, 6:12 a.m. UTC | #1 
нед, 10. мај 2020. у 23:01 Philippe Mathieu-Daudé <f4bug@amsat.org> је
написао/ла:
>
> Better describe the Bonito64 MEM HI/LO and I/O PCI ranges,
> add more PCI regions as unimplemented.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/pci-host/bonito.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index 9761780f44..90a6c52a38 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -39,6 +39,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qemu/error-report.h"
>  #include "hw/pci/pci.h"
>  #include "hw/irq.h"
> @@ -82,7 +83,7 @@
>  #define BONITO_PCILO1_BASE      0x14000000
>  #define BONITO_PCILO2_BASE      0x18000000
>  #define BONITO_PCIHI_BASE       0x20000000
> -#define BONITO_PCIHI_SIZE       0x20000000
> +#define BONITO_PCIHI_SIZE       0x60000000
>  #define BONITO_PCIHI_TOP        (BONITO_PCIHI_BASE + BONITO_PCIHI_SIZE - 1)
>  #define BONITO_PCIIO_BASE       0x1fd00000
>  #define BONITO_PCIIO_BASE_VA    0xbfd00000
> @@ -606,13 +607,25 @@ static void bonito_pcihost_realize(DeviceState *dev, Error **errp)
>      PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>      BonitoState *bs = BONITO_PCI_HOST_BRIDGE(dev);
>
> -    memory_region_init(&bs->pci_mem, OBJECT(dev), "pci.mem", BONITO_PCILO_SIZE);
> +    memory_region_init(&bs->pci_mem, OBJECT(dev), "pci.mem", BONITO_PCIHI_SIZE);
>      phb->bus = pci_register_root_bus(DEVICE(dev), "pci",
>                                       pci_bonito_set_irq, pci_bonito_map_irq,
>                                       dev, &bs->pci_mem, get_system_io(),
>                                       0x28, 32, TYPE_PCI_BUS);
> -    memory_region_add_subregion(get_system_memory(), BONITO_PCILO_BASE,
> -                                &bs->pci_mem);
> +
> +    MemoryRegion *pcimem_lo_alias = g_new(MemoryRegion, 3);
> +    for (size_t i = 0; i < 3; i++) {
> +        char *name = g_strdup_printf("pci.lomem%zu", i);
> +
> +        memory_region_init_alias(&pcimem_lo_alias[i], NULL, name,
> +                                 &bs->pci_mem, i * 64 * MiB, 64 * MiB);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    BONITO_PCILO_BASE + i * 64 * MiB,
> +                                    &pcimem_lo_alias[i]);
> +        g_free(name);
> +    }
> +
> +    create_unimplemented_device("pci.io", 0x1fd00000, 1 * MiB);

Hi, Philippe.

I am delighted with the whole series, finally cleaning Bonito code feels good.

I just want to ask you if it possible to get rid of constants like
0x1fd00000 (generally in this code, not limited to this patch), and
give the some nice "#define" names?

Yours,
Aleksandar

>  }
>
>  static void bonito_realize(PCIDevice *dev, Error **errp)
> @@ -620,6 +633,7 @@ static void bonito_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 = BONITO_PCI_HOST_BRIDGE(s->pcihost);
>
>      /*
>       * Bonito North Bridge, built on FPGA,
> @@ -652,6 +666,7 @@ static void bonito_realize(PCIDevice *dev, Error **errp)
>      sysbus_init_mmio(sysbus, &s->iomem_ldma);
>      sysbus_mmio_map(sysbus, 3, 0x1fe00200);
>
> +    /* PCI copier */
>      memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
>                            "cop", 0x100);
>      sysbus_init_mmio(sysbus, &s->iomem_cop);
> @@ -669,6 +684,13 @@ static void bonito_realize(PCIDevice *dev, Error **errp)
>      sysbus_init_mmio(sysbus, &s->bonito_localio);
>      sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
>
> +    MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
> +    memory_region_init_alias(pcimem_alias, NULL, "pci.mem.alias",
> +                             &bs->pci_mem, 0, BONITO_PCIHI_SIZE);
> +    memory_region_add_subregion(get_system_memory(),
> +                                0x20000000ul, pcimem_alias);
> +    create_unimplemented_device("PCI_2", 0x80000000ul, 2 * GiB); /* pci_hi */
> +
>      /* set the default value of north bridge pci config */
>      pci_set_word(dev->config + PCI_COMMAND, 0x0000);
>      pci_set_word(dev->config + PCI_STATUS, 0x0000);
> --
> 2.21.3
>
Philippe Mathieu-Daudé May 11, 2020, 6:33 a.m. UTC | #2 
On 5/11/20 8:12 AM, Aleksandar Markovic wrote:
> нед, 10. мај 2020. у 23:01 Philippe Mathieu-Daudé <f4bug@amsat.org> је
> написао/ла:
>>
>> Better describe the Bonito64 MEM HI/LO and I/O PCI ranges,
>> add more PCI regions as unimplemented.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/pci-host/bonito.c | 30 ++++++++++++++++++++++++++----
>>   1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>> index 9761780f44..90a6c52a38 100644
>> --- a/hw/pci-host/bonito.c
>> +++ b/hw/pci-host/bonito.c
>> @@ -39,6 +39,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include "qemu/units.h"
>>   #include "qemu/error-report.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/irq.h"
>> @@ -82,7 +83,7 @@
>>   #define BONITO_PCILO1_BASE      0x14000000
>>   #define BONITO_PCILO2_BASE      0x18000000
>>   #define BONITO_PCIHI_BASE       0x20000000
>> -#define BONITO_PCIHI_SIZE       0x20000000
>> +#define BONITO_PCIHI_SIZE       0x60000000
>>   #define BONITO_PCIHI_TOP        (BONITO_PCIHI_BASE + BONITO_PCIHI_SIZE - 1)
>>   #define BONITO_PCIIO_BASE       0x1fd00000
>>   #define BONITO_PCIIO_BASE_VA    0xbfd00000
>> @@ -606,13 +607,25 @@ static void bonito_pcihost_realize(DeviceState *dev, Error **errp)
>>       PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>>       BonitoState *bs = BONITO_PCI_HOST_BRIDGE(dev);
>>
>> -    memory_region_init(&bs->pci_mem, OBJECT(dev), "pci.mem", BONITO_PCILO_SIZE);
>> +    memory_region_init(&bs->pci_mem, OBJECT(dev), "pci.mem", BONITO_PCIHI_SIZE);
>>       phb->bus = pci_register_root_bus(DEVICE(dev), "pci",
>>                                        pci_bonito_set_irq, pci_bonito_map_irq,
>>                                        dev, &bs->pci_mem, get_system_io(),
>>                                        0x28, 32, TYPE_PCI_BUS);
>> -    memory_region_add_subregion(get_system_memory(), BONITO_PCILO_BASE,
>> -                                &bs->pci_mem);
>> +
>> +    MemoryRegion *pcimem_lo_alias = g_new(MemoryRegion, 3);
>> +    for (size_t i = 0; i < 3; i++) {
>> +        char *name = g_strdup_printf("pci.lomem%zu", i);
>> +
>> +        memory_region_init_alias(&pcimem_lo_alias[i], NULL, name,
>> +                                 &bs->pci_mem, i * 64 * MiB, 64 * MiB);
>> +        memory_region_add_subregion(get_system_memory(),
>> +                                    BONITO_PCILO_BASE + i * 64 * MiB,
>> +                                    &pcimem_lo_alias[i]);
>> +        g_free(name);
>> +    }
>> +
>> +    create_unimplemented_device("pci.io", 0x1fd00000, 1 * MiB);
> 
> Hi, Philippe.
> 
> I am delighted with the whole series, finally cleaning Bonito code feels good.
> 
> I just want to ask you if it possible to get rid of constants like
> 0x1fd00000 (generally in this code, not limited to this patch), and
> give the some nice "#define" names?

Yes sure. These patches are almost 1year old, gathering the result of a 
debugging session. Also ...

> 
> Yours,
> Aleksandar
> 
>>   }
>>
>>   static void bonito_realize(PCIDevice *dev, Error **errp)
>> @@ -620,6 +633,7 @@ static void bonito_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 = BONITO_PCI_HOST_BRIDGE(s->pcihost);
>>
>>       /*
>>        * Bonito North Bridge, built on FPGA,
>> @@ -652,6 +666,7 @@ static void bonito_realize(PCIDevice *dev, Error **errp)
>>       sysbus_init_mmio(sysbus, &s->iomem_ldma);
>>       sysbus_mmio_map(sysbus, 3, 0x1fe00200);
>>
>> +    /* PCI copier */
>>       memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
>>                             "cop", 0x100);
>>       sysbus_init_mmio(sysbus, &s->iomem_cop);
>> @@ -669,6 +684,13 @@ static void bonito_realize(PCIDevice *dev, Error **errp)
>>       sysbus_init_mmio(sysbus, &s->bonito_localio);
>>       sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
>>
>> +    MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);

... I'll declare the variable during function prologue ;)

>> +    memory_region_init_alias(pcimem_alias, NULL, "pci.mem.alias",
>> +                             &bs->pci_mem, 0, BONITO_PCIHI_SIZE);
>> +    memory_region_add_subregion(get_system_memory(),
>> +                                0x20000000ul, pcimem_alias);
>> +    create_unimplemented_device("PCI_2", 0x80000000ul, 2 * GiB); /* pci_hi */

... And add definitions for these addresses.

Thanks for the other reviews in this series!

>> +
>>       /* set the default value of north bridge pci config */
>>       pci_set_word(dev->config + PCI_COMMAND, 0x0000);
>>       pci_set_word(dev->config + PCI_STATUS, 0x0000);
>> --
>> 2.21.3
>>
>
diff mbox series

Patch

diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index 9761780f44..90a6c52a38 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -39,6 +39,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qemu/error-report.h"
 #include "hw/pci/pci.h"
 #include "hw/irq.h"
@@ -82,7 +83,7 @@ 
 #define BONITO_PCILO1_BASE      0x14000000
 #define BONITO_PCILO2_BASE      0x18000000
 #define BONITO_PCIHI_BASE       0x20000000
-#define BONITO_PCIHI_SIZE       0x20000000
+#define BONITO_PCIHI_SIZE       0x60000000
 #define BONITO_PCIHI_TOP        (BONITO_PCIHI_BASE + BONITO_PCIHI_SIZE - 1)
 #define BONITO_PCIIO_BASE       0x1fd00000
 #define BONITO_PCIIO_BASE_VA    0xbfd00000
@@ -606,13 +607,25 @@  static void bonito_pcihost_realize(DeviceState *dev, Error **errp)
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     BonitoState *bs = BONITO_PCI_HOST_BRIDGE(dev);
 
-    memory_region_init(&bs->pci_mem, OBJECT(dev), "pci.mem", BONITO_PCILO_SIZE);
+    memory_region_init(&bs->pci_mem, OBJECT(dev), "pci.mem", BONITO_PCIHI_SIZE);
     phb->bus = pci_register_root_bus(DEVICE(dev), "pci",
                                      pci_bonito_set_irq, pci_bonito_map_irq,
                                      dev, &bs->pci_mem, get_system_io(),
                                      0x28, 32, TYPE_PCI_BUS);
-    memory_region_add_subregion(get_system_memory(), BONITO_PCILO_BASE,
-                                &bs->pci_mem);
+
+    MemoryRegion *pcimem_lo_alias = g_new(MemoryRegion, 3);
+    for (size_t i = 0; i < 3; i++) {
+        char *name = g_strdup_printf("pci.lomem%zu", i);
+
+        memory_region_init_alias(&pcimem_lo_alias[i], NULL, name,
+                                 &bs->pci_mem, i * 64 * MiB, 64 * MiB);
+        memory_region_add_subregion(get_system_memory(),
+                                    BONITO_PCILO_BASE + i * 64 * MiB,
+                                    &pcimem_lo_alias[i]);
+        g_free(name);
+    }
+
+    create_unimplemented_device("pci.io", 0x1fd00000, 1 * MiB);
 }
 
 static void bonito_realize(PCIDevice *dev, Error **errp)
@@ -620,6 +633,7 @@  static void bonito_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 = BONITO_PCI_HOST_BRIDGE(s->pcihost);
 
     /*
      * Bonito North Bridge, built on FPGA,
@@ -652,6 +666,7 @@  static void bonito_realize(PCIDevice *dev, Error **errp)
     sysbus_init_mmio(sysbus, &s->iomem_ldma);
     sysbus_mmio_map(sysbus, 3, 0x1fe00200);
 
+    /* PCI copier */
     memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
                           "cop", 0x100);
     sysbus_init_mmio(sysbus, &s->iomem_cop);
@@ -669,6 +684,13 @@  static void bonito_realize(PCIDevice *dev, Error **errp)
     sysbus_init_mmio(sysbus, &s->bonito_localio);
     sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
 
+    MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
+    memory_region_init_alias(pcimem_alias, NULL, "pci.mem.alias",
+                             &bs->pci_mem, 0, BONITO_PCIHI_SIZE);
+    memory_region_add_subregion(get_system_memory(),
+                                0x20000000ul, pcimem_alias);
+    create_unimplemented_device("PCI_2", 0x80000000ul, 2 * GiB); /* pci_hi */
+
     /* set the default value of north bridge pci config */
     pci_set_word(dev->config + PCI_COMMAND, 0x0000);
     pci_set_word(dev->config + PCI_STATUS, 0x0000);