Re: [PATCH v4 11/45] Introduce Raspberry PI 4 machine

[Peter Maydell]

On Fri, 8 Dec 2023 at 02:33, Sergey Kambalin <serg.o...@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kamba...@auriga.com>
> ---

> @@ -329,11 +330,24 @@ void bcm_soc_peripherals_common_realize(DeviceState 
> *dev, Error **errp)
>          return;
>      }
>
> -    if (!object_property_set_uint(OBJECT(&s->fb), "vcram-base",
> -                                  ram_size - vcram_size, errp)) {
> +    vcram_base = object_property_get_uint(OBJECT(s), "vcram-base", &err);
> +    if (err) {
> +        error_propagate(errp, err);
>          return;
>      }
>
> +    if (vcram_base == 0) {
> +        vcram_base = (ram_size > UPPER_RAM_BASE ? UPPER_RAM_BASE : ram_size)
> +            - vcram_size;
> +    } else {
> +        if (vcram_base + vcram_size > UPPER_RAM_BASE) {
> +            vcram_base = UPPER_RAM_BASE - vcram_size;
> +        }
> +    }

I think this might be slightly clearer written as:

   if (vcram_base == 0) {
       vcram_base = ram_size - vcram_size;
   }
   vcram_base = MIN(vcram_base, UPPER_RAM_BASE - vcram_size);

> +    if (!object_property_set_uint(OBJECT(&s->fb), "vcram-base", vcram_base,
> +                                  errp)) {
> +        return;
> +    }
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->fb), errp)) {
>          return;
>      }

> @@ -293,11 +292,20 @@ static void raspi_base_machine_init(MachineState 
> *machine,
>
>      vcram_size = object_property_get_uint(OBJECT(soc), "vcram-size",
>                                            &error_abort);
> +    vcram_base = object_property_get_uint(OBJECT(soc), "vcram-base",
> +                                          &error_abort);
> +    if (!vcram_base) {
> +        boot_ram_size = (ram_size > UPPER_RAM_BASE ? UPPER_RAM_BASE : 
> ram_size)
> +            - vcram_size;
> +    } else {
> +        boot_ram_size = (vcram_base + vcram_size > UPPER_RAM_BASE ?
> +                                UPPER_RAM_BASE - vcram_size : vcram_base);
> +    }

This also could be written

    if (vcram_base == 0) {
        vcram_base = ram_size - vcram_size;
    }
    vcram_base = MIN(vcram_base, UPPER_RAM_BASE - vcram_size);
    boot_ram_size = vcram_base;

>      setup_boot(machine, &soc->cpu[0].core, board_processor_id(board_rev),
> -               machine->ram_size - vcram_size);
> +               boot_ram_size);
> }


> +struct Raspi4bMachineState {
> +    /*< private >*/
> +    RaspiBaseMachineState parent_obj;
> +    /*< public >*/
> +    BCM2838State soc;
> +};

Our coding style no longer requires the 'private' and 'public'
marker comments, so you can drop them.

Otherwise
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>

thanks
-- PMM