[v1] riscv: opensbi boot test and cleanups

[PATCH 13/15] hw/riscv/spike.c: simplify create_fdt()

Posted by Daniel Henrique Barboza 3 years, 1 month ago

'mem_size' and 'cmdline' aren't being used and the MachineState pointer
is being retrieved via a MACHINE() macro.

Remove 'mem_size' and 'cmdline' and add MachineState as a parameter.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/spike.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 2b9af5689e..181bf394a0 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -48,15 +48,14 @@ static const MemMapEntry spike_memmap[] = {
     [SPIKE_DRAM] =     { 0x80000000,        0x0 },
 };
 
-static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
-                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
+static void create_fdt(MachineState *mc, SpikeState *s,
+                       const MemMapEntry *memmap, bool is_32_bit)
 {
     void *fdt;
     int fdt_size;
     uint64_t addr, size;
     unsigned long clint_addr;
     int cpu, socket;
-    MachineState *mc = MACHINE(s);
     uint32_t *clint_cells;
     uint32_t cpu_phandle, intc_phandle, phandle = 1;
     char *name, *mem_name, *clint_name, *clust_name;
@@ -254,8 +253,7 @@ static void spike_board_init(MachineState *machine)
                                 mask_rom);
 
     /* Create device tree */
-    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32bit(&s->soc[0]));
+    create_fdt(machine, s, memmap, riscv_is_32bit(&s->soc[0]));
 
     /*
      * Not like other RISC-V machines that use plain binary bios images,
-- 
2.38.1

Re: [PATCH 13/15] hw/riscv/spike.c: simplify create_fdt()

Posted by Bin Meng 3 years, 1 month ago

On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 'mem_size' and 'cmdline' aren't being used and the MachineState pointer
> is being retrieved via a MACHINE() macro.
>
> Remove 'mem_size' and 'cmdline' and add MachineState as a parameter.

Why do you add MachineState as a parameter? What's the problem of
using the MACHINE() macro?

>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/spike.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 2b9af5689e..181bf394a0 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -48,15 +48,14 @@ static const MemMapEntry spike_memmap[] = {
>      [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>  };
>
> -static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
> -                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
> +static void create_fdt(MachineState *mc, SpikeState *s,
> +                       const MemMapEntry *memmap, bool is_32_bit)
>  {
>      void *fdt;
>      int fdt_size;
>      uint64_t addr, size;
>      unsigned long clint_addr;
>      int cpu, socket;
> -    MachineState *mc = MACHINE(s);
>      uint32_t *clint_cells;
>      uint32_t cpu_phandle, intc_phandle, phandle = 1;
>      char *name, *mem_name, *clint_name, *clust_name;
> @@ -254,8 +253,7 @@ static void spike_board_init(MachineState *machine)
>                                  mask_rom);
>
>      /* Create device tree */
> -    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> -               riscv_is_32bit(&s->soc[0]));
> +    create_fdt(machine, s, memmap, riscv_is_32bit(&s->soc[0]));
>
>      /*
>       * Not like other RISC-V machines that use plain binary bios images,
> --

Regards,
Bin

Re: [PATCH 13/15] hw/riscv/spike.c: simplify create_fdt()

Posted by Daniel Henrique Barboza 3 years, 1 month ago


On 12/23/22 10:06, Bin Meng wrote:
> On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>> 'mem_size' and 'cmdline' aren't being used and the MachineState pointer
>> is being retrieved via a MACHINE() macro.
>>
>> Remove 'mem_size' and 'cmdline' and add MachineState as a parameter.
> Why do you add MachineState as a parameter? What's the problem of
> using the MACHINE() macro?

Yeah, I went overboard with the macro removal in this case and in patch 14.
I'll also redo patch 15 to avoid the qdev_get_machine() call but keeping the
macro.



Daniel



>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/spike.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index 2b9af5689e..181bf394a0 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -48,15 +48,14 @@ static const MemMapEntry spike_memmap[] = {
>>       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>>   };
>>
>> -static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>> -                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
>> +static void create_fdt(MachineState *mc, SpikeState *s,
>> +                       const MemMapEntry *memmap, bool is_32_bit)
>>   {
>>       void *fdt;
>>       int fdt_size;
>>       uint64_t addr, size;
>>       unsigned long clint_addr;
>>       int cpu, socket;
>> -    MachineState *mc = MACHINE(s);
>>       uint32_t *clint_cells;
>>       uint32_t cpu_phandle, intc_phandle, phandle = 1;
>>       char *name, *mem_name, *clint_name, *clust_name;
>> @@ -254,8 +253,7 @@ static void spike_board_init(MachineState *machine)
>>                                   mask_rom);
>>
>>       /* Create device tree */
>> -    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
>> -               riscv_is_32bit(&s->soc[0]));
>> +    create_fdt(machine, s, memmap, riscv_is_32bit(&s->soc[0]));
>>
>>       /*
>>        * Not like other RISC-V machines that use plain binary bios images,
>> --
> Regards,
> Bin