diff mbox series

[v2,1/3] hw/mips/boston: Massage memory map information

Message ID 20210929151211.108-2-jiaxun.yang@flygoat.com (mailing list archive)
State New, archived
Headers show
Series hw/mips/boston: ELF kernel support | expand

Commit Message

Jiaxun Yang Sept. 29, 2021, 3:12 p.m. UTC 
Use memmap array to uinfy address of memory map.
That would allow us reuse address information for FDT generation.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
--
v2: Fix minor style issue, fix uart map size
---
 hw/mips/boston.c | 95 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 71 insertions(+), 24 deletions(-)

Comments

BALATON Zoltan Sept. 29, 2021, 3:32 p.m. UTC | #1 
On Wed, 29 Sep 2021, Jiaxun Yang wrote:
> Use memmap array to uinfy address of memory map.
> That would allow us reuse address information for FDT generation.
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> --
> v2: Fix minor style issue, fix uart map size
> ---
> hw/mips/boston.c | 95 ++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 71 insertions(+), 24 deletions(-)
>
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index 20b06865b2..5c720440fb 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -64,6 +64,44 @@ struct BostonState {
>     hwaddr fdt_base;
> };
>
> +enum {
> +    BOSTON_LOWDDR,
> +    BOSTON_PCIE0,
> +    BOSTON_PCIE1,
> +    BOSTON_PCIE2,
> +    BOSTON_PCIE2_MMIO,
> +    BOSTON_CM,
> +    BOSTON_GIC,
> +    BOSTON_CDMM,
> +    BOSTON_CPC,
> +    BOSTON_PLATREG,
> +    BOSTON_UART,
> +    BOSTON_LCD,
> +    BOSTON_FLASH,
> +    BOSTON_PCIE1_MMIO,
> +    BOSTON_PCIE0_MMIO,
> +    BOSTON_HIGHDDR,
> +};
> +
> +static const MemMapEntry boston_memmap[] = {
> +    [BOSTON_LOWDDR] =     {        0x0,    0x10000000 },

What's the advantage of having it in an array as opposed to just have 
simple defines for these? I did not see a case where you go through these 
as array elements so it seems it's just an unnecessarily complex way to 
write boston_memmap[BOSTON_LOWADDR] insted of BOSTON_LOWADDR. Did I miss 
something where having an array helps?

Regards,
BALATON Zoltan

> +    [BOSTON_PCIE0] =      { 0x10000000,     0x2000000 },
> +    [BOSTON_PCIE1] =      { 0x12000000,     0x2000000 },
> +    [BOSTON_PCIE2] =      { 0x14000000,     0x2000000 },
> +    [BOSTON_PCIE2_MMIO] = { 0x16000000,      0x100000 },
> +    [BOSTON_CM] =         { 0x16100000,       0x20000 },
> +    [BOSTON_GIC] =        { 0x16120000,       0x20000 },
> +    [BOSTON_CDMM] =       { 0x16140000,        0x8000 },
> +    [BOSTON_CPC] =        { 0x16200000,        0x8000 },
> +    [BOSTON_PLATREG] =    { 0x17ffd000,        0x1000 },
> +    [BOSTON_UART] =       { 0x17ffe000,          0x20 },
> +    [BOSTON_LCD] =        { 0x17fff000,           0x8 },
> +    [BOSTON_FLASH] =      { 0x18000000,     0x8000000 },
> +    [BOSTON_PCIE1_MMIO] = { 0x20000000,    0x20000000 },
> +    [BOSTON_PCIE0_MMIO] = { 0x40000000,    0x40000000 },
> +    [BOSTON_HIGHDDR] =    { 0x80000000,           0x0 },
> +};
> +
> enum boston_plat_reg {
>     PLAT_FPGA_BUILD     = 0x00,
>     PLAT_CORE_CL        = 0x04,
> @@ -275,24 +313,22 @@ type_init(boston_register_types)
>
> static void gen_firmware(uint32_t *p, hwaddr kernel_entry, hwaddr fdt_addr)
> {
> -    const uint32_t cm_base = 0x16100000;
> -    const uint32_t gic_base = 0x16120000;
> -    const uint32_t cpc_base = 0x16200000;
> -
>     /* Move CM GCRs */
>     bl_gen_write_ulong(&p,
>                        cpu_mips_phys_to_kseg1(NULL, GCR_BASE_ADDR + GCR_BASE_OFS),
> -                       cm_base);
> +                       boston_memmap[BOSTON_CM].base);
>
>     /* Move & enable GIC GCRs */
>     bl_gen_write_ulong(&p,
> -                       cpu_mips_phys_to_kseg1(NULL, cm_base + GCR_GIC_BASE_OFS),
> -                       gic_base | GCR_GIC_BASE_GICEN_MSK);
> +                       cpu_mips_phys_to_kseg1(NULL,
> +                            boston_memmap[BOSTON_CM].base + GCR_GIC_BASE_OFS),
> +                       boston_memmap[BOSTON_GIC].base | GCR_GIC_BASE_GICEN_MSK);
>
>     /* Move & enable CPC GCRs */
>     bl_gen_write_ulong(&p,
> -                       cpu_mips_phys_to_kseg1(NULL, cm_base + GCR_CPC_BASE_OFS),
> -                       cpc_base | GCR_CPC_BASE_CPCEN_MSK);
> +                       cpu_mips_phys_to_kseg1(NULL,
> +                            boston_memmap[BOSTON_CM].base + GCR_CPC_BASE_OFS),
> +                       boston_memmap[BOSTON_CPC].base | GCR_CPC_BASE_CPCEN_MSK);
>
>     /*
>      * Setup argument registers to follow the UHI boot protocol:
> @@ -333,8 +369,9 @@ static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
>     ram_low_sz = MIN(256 * MiB, machine->ram_size);
>     ram_high_sz = machine->ram_size - ram_low_sz;
>     qemu_fdt_setprop_sized_cells(fdt, "/memory@0", "reg",
> -                                 1, 0x00000000, 1, ram_low_sz,
> -                                 1, 0x90000000, 1, ram_high_sz);
> +                                 1, boston_memmap[BOSTON_LOWDDR].base, 1, ram_low_sz,
> +                                 1, boston_memmap[BOSTON_HIGHDDR].base + ram_low_sz,
> +                                 1, ram_high_sz);
>
>     fdt = g_realloc(fdt, fdt_totalsize(fdt));
>     qemu_fdt_dumpdtb(fdt, fdt_sz);
> @@ -438,11 +475,13 @@ static void boston_mach_init(MachineState *machine)
>     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>
>     flash =  g_new(MemoryRegion, 1);
> -    memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB,
> +    memory_region_init_rom(flash, NULL, "boston.flash", boston_memmap[BOSTON_FLASH].size,
>                            &error_fatal);
> -    memory_region_add_subregion_overlap(sys_mem, 0x18000000, flash, 0);
> +    memory_region_add_subregion_overlap(sys_mem, boston_memmap[BOSTON_FLASH].base,
> +                                        flash, 0);
>
> -    memory_region_add_subregion_overlap(sys_mem, 0x80000000, machine->ram, 0);
> +    memory_region_add_subregion_overlap(sys_mem, boston_memmap[BOSTON_HIGHDDR].base,
> +                                        machine->ram, 0);
>
>     ddr_low_alias = g_new(MemoryRegion, 1);
>     memory_region_init_alias(ddr_low_alias, NULL, "boston_low.ddr",
> @@ -451,32 +490,40 @@ static void boston_mach_init(MachineState *machine)
>     memory_region_add_subregion_overlap(sys_mem, 0, ddr_low_alias, 0);
>
>     xilinx_pcie_init(sys_mem, 0,
> -                     0x10000000, 32 * MiB,
> -                     0x40000000, 1 * GiB,
> +                     boston_memmap[BOSTON_PCIE0].base,
> +                     boston_memmap[BOSTON_PCIE0].size,
> +                     boston_memmap[BOSTON_PCIE0_MMIO].base,
> +                     boston_memmap[BOSTON_PCIE0_MMIO].size,
>                      get_cps_irq(&s->cps, 2), false);
>
>     xilinx_pcie_init(sys_mem, 1,
> -                     0x12000000, 32 * MiB,
> -                     0x20000000, 512 * MiB,
> +                     boston_memmap[BOSTON_PCIE1].base,
> +                     boston_memmap[BOSTON_PCIE1].size,
> +                     boston_memmap[BOSTON_PCIE1_MMIO].base,
> +                     boston_memmap[BOSTON_PCIE1_MMIO].size,
>                      get_cps_irq(&s->cps, 1), false);
>
>     pcie2 = xilinx_pcie_init(sys_mem, 2,
> -                             0x14000000, 32 * MiB,
> -                             0x16000000, 1 * MiB,
> +                             boston_memmap[BOSTON_PCIE2].base,
> +                             boston_memmap[BOSTON_PCIE2].size,
> +                             boston_memmap[BOSTON_PCIE2_MMIO].base,
> +                             boston_memmap[BOSTON_PCIE2_MMIO].size,
>                              get_cps_irq(&s->cps, 0), true);
>
>     platreg = g_new(MemoryRegion, 1);
>     memory_region_init_io(platreg, NULL, &boston_platreg_ops, s,
> -                          "boston-platregs", 0x1000);
> -    memory_region_add_subregion_overlap(sys_mem, 0x17ffd000, platreg, 0);
> +                          "boston-platregs",
> +                          boston_memmap[BOSTON_PLATREG].size);
> +    memory_region_add_subregion_overlap(sys_mem,
> +                          boston_memmap[BOSTON_PLATREG].base, platreg, 0);
>
> -    s->uart = serial_mm_init(sys_mem, 0x17ffe000, 2,
> +    s->uart = serial_mm_init(sys_mem, boston_memmap[BOSTON_UART].base, 2,
>                              get_cps_irq(&s->cps, 3), 10000000,
>                              serial_hd(0), DEVICE_NATIVE_ENDIAN);
>
>     lcd = g_new(MemoryRegion, 1);
>     memory_region_init_io(lcd, NULL, &boston_lcd_ops, s, "boston-lcd", 0x8);
> -    memory_region_add_subregion_overlap(sys_mem, 0x17fff000, lcd, 0);
> +    memory_region_add_subregion_overlap(sys_mem, boston_memmap[BOSTON_LCD].base, lcd, 0);
>
>     chr = qemu_chr_new("lcd", "vc:320x240", NULL);
>     qemu_chr_fe_init(&s->lcd_display, chr, NULL);
>
Jiaxun Yang Sept. 29, 2021, 4:41 p.m. UTC | #2 
在 2021/9/29 16:32, BALATON Zoltan 写道:
> On Wed, 29 Sep 2021, Jiaxun Yang wrote:
>> Use memmap array to uinfy address of memory map.
>> That would allow us reuse address information for FDT generation.
>>
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> -- 
>> v2: Fix minor style issue, fix uart map size
>> ---
>> hw/mips/boston.c | 95 ++++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 71 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
>> index 20b06865b2..5c720440fb 100644
>> --- a/hw/mips/boston.c
>> +++ b/hw/mips/boston.c
>> @@ -64,6 +64,44 @@ struct BostonState {
>>     hwaddr fdt_base;
>> };
>>
>> +enum {
>> +    BOSTON_LOWDDR,
>> +    BOSTON_PCIE0,
>> +    BOSTON_PCIE1,
>> +    BOSTON_PCIE2,
>> +    BOSTON_PCIE2_MMIO,
>> +    BOSTON_CM,
>> +    BOSTON_GIC,
>> +    BOSTON_CDMM,
>> +    BOSTON_CPC,
>> +    BOSTON_PLATREG,
>> +    BOSTON_UART,
>> +    BOSTON_LCD,
>> +    BOSTON_FLASH,
>> +    BOSTON_PCIE1_MMIO,
>> +    BOSTON_PCIE0_MMIO,
>> +    BOSTON_HIGHDDR,
>> +};
>> +
>> +static const MemMapEntry boston_memmap[] = {
>> +    [BOSTON_LOWDDR] =     {        0x0,    0x10000000 },
>
> What's the advantage of having it in an array as opposed to just have 
> simple defines for these? I did not see a case where you go through 
> these as array elements so it seems it's just an unnecessarily complex 
> way to write boston_memmap[BOSTON_LOWADDR] insted of BOSTON_LOWADDR. 
> Did I miss something where having an array helps?
I do consider it as sort of common practice which will make address 
allocation much more clear and easier for subsequent FDT generation 
patch to take both address and size. Many new boards such as arm-virt, 
arm-sbsa and riscv-virt is using MemMapEntry to manage their address 
allocation.

Thanks.
- Jiaxun
>
> Regards,
> BALATON Zoltan
>
diff mbox series

Patch

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 20b06865b2..5c720440fb 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -64,6 +64,44 @@  struct BostonState {
     hwaddr fdt_base;
 };
 
+enum {
+    BOSTON_LOWDDR,
+    BOSTON_PCIE0,
+    BOSTON_PCIE1,
+    BOSTON_PCIE2,
+    BOSTON_PCIE2_MMIO,
+    BOSTON_CM,
+    BOSTON_GIC,
+    BOSTON_CDMM,
+    BOSTON_CPC,
+    BOSTON_PLATREG,
+    BOSTON_UART,
+    BOSTON_LCD,
+    BOSTON_FLASH,
+    BOSTON_PCIE1_MMIO,
+    BOSTON_PCIE0_MMIO,
+    BOSTON_HIGHDDR,
+};
+
+static const MemMapEntry boston_memmap[] = {
+    [BOSTON_LOWDDR] =     {        0x0,    0x10000000 },
+    [BOSTON_PCIE0] =      { 0x10000000,     0x2000000 },
+    [BOSTON_PCIE1] =      { 0x12000000,     0x2000000 },
+    [BOSTON_PCIE2] =      { 0x14000000,     0x2000000 },
+    [BOSTON_PCIE2_MMIO] = { 0x16000000,      0x100000 },
+    [BOSTON_CM] =         { 0x16100000,       0x20000 },
+    [BOSTON_GIC] =        { 0x16120000,       0x20000 },
+    [BOSTON_CDMM] =       { 0x16140000,        0x8000 },
+    [BOSTON_CPC] =        { 0x16200000,        0x8000 },
+    [BOSTON_PLATREG] =    { 0x17ffd000,        0x1000 },
+    [BOSTON_UART] =       { 0x17ffe000,          0x20 },
+    [BOSTON_LCD] =        { 0x17fff000,           0x8 },
+    [BOSTON_FLASH] =      { 0x18000000,     0x8000000 },
+    [BOSTON_PCIE1_MMIO] = { 0x20000000,    0x20000000 },
+    [BOSTON_PCIE0_MMIO] = { 0x40000000,    0x40000000 },
+    [BOSTON_HIGHDDR] =    { 0x80000000,           0x0 },
+};
+
 enum boston_plat_reg {
     PLAT_FPGA_BUILD     = 0x00,
     PLAT_CORE_CL        = 0x04,
@@ -275,24 +313,22 @@  type_init(boston_register_types)
 
 static void gen_firmware(uint32_t *p, hwaddr kernel_entry, hwaddr fdt_addr)
 {
-    const uint32_t cm_base = 0x16100000;
-    const uint32_t gic_base = 0x16120000;
-    const uint32_t cpc_base = 0x16200000;
-
     /* Move CM GCRs */
     bl_gen_write_ulong(&p,
                        cpu_mips_phys_to_kseg1(NULL, GCR_BASE_ADDR + GCR_BASE_OFS),
-                       cm_base);
+                       boston_memmap[BOSTON_CM].base);
 
     /* Move & enable GIC GCRs */
     bl_gen_write_ulong(&p,
-                       cpu_mips_phys_to_kseg1(NULL, cm_base + GCR_GIC_BASE_OFS),
-                       gic_base | GCR_GIC_BASE_GICEN_MSK);
+                       cpu_mips_phys_to_kseg1(NULL,
+                            boston_memmap[BOSTON_CM].base + GCR_GIC_BASE_OFS),
+                       boston_memmap[BOSTON_GIC].base | GCR_GIC_BASE_GICEN_MSK);
 
     /* Move & enable CPC GCRs */
     bl_gen_write_ulong(&p,
-                       cpu_mips_phys_to_kseg1(NULL, cm_base + GCR_CPC_BASE_OFS),
-                       cpc_base | GCR_CPC_BASE_CPCEN_MSK);
+                       cpu_mips_phys_to_kseg1(NULL,
+                            boston_memmap[BOSTON_CM].base + GCR_CPC_BASE_OFS),
+                       boston_memmap[BOSTON_CPC].base | GCR_CPC_BASE_CPCEN_MSK);
 
     /*
      * Setup argument registers to follow the UHI boot protocol:
@@ -333,8 +369,9 @@  static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
     ram_low_sz = MIN(256 * MiB, machine->ram_size);
     ram_high_sz = machine->ram_size - ram_low_sz;
     qemu_fdt_setprop_sized_cells(fdt, "/memory@0", "reg",
-                                 1, 0x00000000, 1, ram_low_sz,
-                                 1, 0x90000000, 1, ram_high_sz);
+                                 1, boston_memmap[BOSTON_LOWDDR].base, 1, ram_low_sz,
+                                 1, boston_memmap[BOSTON_HIGHDDR].base + ram_low_sz,
+                                 1, ram_high_sz);
 
     fdt = g_realloc(fdt, fdt_totalsize(fdt));
     qemu_fdt_dumpdtb(fdt, fdt_sz);
@@ -438,11 +475,13 @@  static void boston_mach_init(MachineState *machine)
     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
 
     flash =  g_new(MemoryRegion, 1);
-    memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB,
+    memory_region_init_rom(flash, NULL, "boston.flash", boston_memmap[BOSTON_FLASH].size,
                            &error_fatal);
-    memory_region_add_subregion_overlap(sys_mem, 0x18000000, flash, 0);
+    memory_region_add_subregion_overlap(sys_mem, boston_memmap[BOSTON_FLASH].base,
+                                        flash, 0);
 
-    memory_region_add_subregion_overlap(sys_mem, 0x80000000, machine->ram, 0);
+    memory_region_add_subregion_overlap(sys_mem, boston_memmap[BOSTON_HIGHDDR].base,
+                                        machine->ram, 0);
 
     ddr_low_alias = g_new(MemoryRegion, 1);
     memory_region_init_alias(ddr_low_alias, NULL, "boston_low.ddr",
@@ -451,32 +490,40 @@  static void boston_mach_init(MachineState *machine)
     memory_region_add_subregion_overlap(sys_mem, 0, ddr_low_alias, 0);
 
     xilinx_pcie_init(sys_mem, 0,
-                     0x10000000, 32 * MiB,
-                     0x40000000, 1 * GiB,
+                     boston_memmap[BOSTON_PCIE0].base,
+                     boston_memmap[BOSTON_PCIE0].size,
+                     boston_memmap[BOSTON_PCIE0_MMIO].base,
+                     boston_memmap[BOSTON_PCIE0_MMIO].size,
                      get_cps_irq(&s->cps, 2), false);
 
     xilinx_pcie_init(sys_mem, 1,
-                     0x12000000, 32 * MiB,
-                     0x20000000, 512 * MiB,
+                     boston_memmap[BOSTON_PCIE1].base,
+                     boston_memmap[BOSTON_PCIE1].size,
+                     boston_memmap[BOSTON_PCIE1_MMIO].base,
+                     boston_memmap[BOSTON_PCIE1_MMIO].size,
                      get_cps_irq(&s->cps, 1), false);
 
     pcie2 = xilinx_pcie_init(sys_mem, 2,
-                             0x14000000, 32 * MiB,
-                             0x16000000, 1 * MiB,
+                             boston_memmap[BOSTON_PCIE2].base,
+                             boston_memmap[BOSTON_PCIE2].size,
+                             boston_memmap[BOSTON_PCIE2_MMIO].base,
+                             boston_memmap[BOSTON_PCIE2_MMIO].size,
                              get_cps_irq(&s->cps, 0), true);
 
     platreg = g_new(MemoryRegion, 1);
     memory_region_init_io(platreg, NULL, &boston_platreg_ops, s,
-                          "boston-platregs", 0x1000);
-    memory_region_add_subregion_overlap(sys_mem, 0x17ffd000, platreg, 0);
+                          "boston-platregs",
+                          boston_memmap[BOSTON_PLATREG].size);
+    memory_region_add_subregion_overlap(sys_mem,
+                          boston_memmap[BOSTON_PLATREG].base, platreg, 0);
 
-    s->uart = serial_mm_init(sys_mem, 0x17ffe000, 2,
+    s->uart = serial_mm_init(sys_mem, boston_memmap[BOSTON_UART].base, 2,
                              get_cps_irq(&s->cps, 3), 10000000,
                              serial_hd(0), DEVICE_NATIVE_ENDIAN);
 
     lcd = g_new(MemoryRegion, 1);
     memory_region_init_io(lcd, NULL, &boston_lcd_ops, s, "boston-lcd", 0x8);
-    memory_region_add_subregion_overlap(sys_mem, 0x17fff000, lcd, 0);
+    memory_region_add_subregion_overlap(sys_mem, boston_memmap[BOSTON_LCD].base, lcd, 0);
 
     chr = qemu_chr_new("lcd", "vc:320x240", NULL);
     qemu_chr_fe_init(&s->lcd_display, chr, NULL);