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

[Peter Maydell <peter.maydell@linaro.org>]

On Wed, 26 Jul 2023 at 14:52, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/arm/bcm2835_peripherals.c    |  20 +++-
>  hw/arm/bcm2836.c                |   2 +
>  hw/arm/bcm2838.c                |   2 +
>  hw/arm/meson.build              |   2 +-
>  hw/arm/raspi.c                  |  28 +++--
>  hw/arm/raspi4b.c                | 182 ++++++++++++++++++++++++++++++++
>  include/hw/arm/raspi_platform.h |  11 ++
>  include/hw/display/bcm2835_fb.h |   2 +
>  8 files changed, 235 insertions(+), 14 deletions(-)
>  create mode 100644 hw/arm/raspi4b.c


> +static void raspi4b_machine_class_init(MachineClass *mc, uint32_t board_rev)
> +{
> +    object_class_property_add(OBJECT_CLASS(mc), "vcram-size", "uint32",
> +                              get_vcram_size, set_vcram_size, NULL, NULL);
> +    object_class_property_set_description(OBJECT_CLASS(mc), "vcram-size",
> +                                            "VideoCore RAM base address");
> +    object_class_property_add(OBJECT_CLASS(mc), "vcram-base", "uint32",
> +                              get_vcram_base, set_vcram_base, NULL, NULL);
> +    object_class_property_set_description(OBJECT_CLASS(mc), "vcram-base",
> +                                            "VideoCore RAM size");

What are these properties for? I assume you can't change them
on a real raspi 4b board, so why do we need to expose them
to users?

> +
> +    raspi_machine_class_common_init(mc, board_rev);
> +    mc->init = raspi4b_machine_init;
> +}
> +
> +static void raspi4b1g_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    RaspiBaseMachineClass *rmc = RASPI_BASE_MACHINE_CLASS(oc);
> +
> +    rmc->board_rev = 0xa03111;
> +    raspi4b_machine_class_init(mc, rmc->board_rev);
> +}
> +
> +static void raspi4b2g_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    RaspiBaseMachineClass *rmc = RASPI_BASE_MACHINE_CLASS(oc);
> +
> +    rmc->board_rev = 0xb03112;
> +    raspi4b_machine_class_init(mc, rmc->board_rev);
> +}
> +
> +static void raspi4b4g_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    RaspiBaseMachineClass *rmc = RASPI_BASE_MACHINE_CLASS(oc);
> +
> +    rmc->board_rev = 0xc03114;
> +    raspi4b_machine_class_init(mc, rmc->board_rev);
> +}
> +
> +static void raspi4b8g_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    RaspiBaseMachineClass *rmc = RASPI_BASE_MACHINE_CLASS(oc);
> +
> +    rmc->board_rev = 0xd03114;
> +    raspi4b_machine_class_init(mc, rmc->board_rev);
> +}

The only differences between these machine types are the
amount of RAM, right? We shouldn't expose that to users
via different board names. Provide a single board
"raspi4b", give it whichever default amount of memory
seems reasonable (I would suggest 1GB or 2GB, it avoids
annoying problems with "make check" on 32-bit hosts), and
let the user pick a different amount of RAM with the -m option
as they usually would. (You can sanitize that to check they
only picked a valid amount of RAM.)

This seems also like a good point to ask for an extra patch
which updates docs/system/arm/raspi.rst to document the
new board type.

thanks
-- PMM