Re: [PATCH v4 12/45] Temporarily disable unimplemented rpi4b devices

[Peter Maydell]

On Fri, 8 Dec 2023 at 02:33, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> This commit adds RPi4B device tree modifications:
> - disable pcie, rng200, thermal sensor and genet devices
>   (they're going to be re-enabled in the following commits)
> - create additional memory region in device tree
>   if RAM amount exceeds VC base address.
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/arm/raspi.c                  |  5 +--
>  hw/arm/raspi4b.c                | 60 +++++++++++++++++++++++++++++++++
>  include/hw/arm/raspi_platform.h |  4 +++
>  3 files changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index da1e9e7c13..895c305122 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -37,9 +37,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(RaspiMachineState, RASPI_MACHINE)
>  #define FIRMWARE_ADDR_3 0x80000 /* Pi 3 loads kernel.img here by default */
>  #define SPINTABLE_ADDR  0xd8 /* Pi 3 bootloader spintable */
>
> -/* Registered machine type (matches RPi Foundation bootloader and U-Boot) */
> -#define MACH_TYPE_BCM2708   3138
> -
>  struct RaspiMachineState {
>      /*< private >*/
>      RaspiBaseMachineState parent_obj;
> @@ -75,7 +72,7 @@ static const struct {
>      [PROCESSOR_ID_BCM2838] = {TYPE_BCM2838, BCM283X_NCPUS},
>  };
>
> -static uint64_t board_ram_size(uint32_t board_rev)
> +uint64_t board_ram_size(uint32_t board_rev)
>  {
>      assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
>      return 256 * MiB << FIELD_EX32(board_rev, REV_CODE, MEMORY_SIZE);
> diff --git a/hw/arm/raspi4b.c b/hw/arm/raspi4b.c
> index 2d33861c57..10376b62dc 100644
> --- a/hw/arm/raspi4b.c
> +++ b/hw/arm/raspi4b.c
> @@ -21,6 +21,7 @@
>  #include "hw/arm/boot.h"
>  #include "qom/object.h"
>  #include "hw/arm/bcm2838.h"
> +#include <libfdt.h>
>
>  #define TYPE_RASPI4B_MACHINE MACHINE_TYPE_NAME("raspi4b-2g")
>  OBJECT_DECLARE_SIMPLE_TYPE(Raspi4bMachineState, RASPI4B_MACHINE)
> @@ -32,6 +33,64 @@ struct Raspi4bMachineState {
>      BCM2838State soc;
>  };
>
> +/* Add second memory region if board RAM amount exceeds VC base address
> + * (see https://datasheets.raspberrypi.com/bcm2711/bcm2711-peripherals.pdf
> + * 1.2 Address Map)
> + */

Our coding style says that for multiline comments the initial
"/*" should be on a line of its own.

> +static int raspi_add_memory_node(void *fdt, hwaddr mem_base, hwaddr mem_len)
> +{
> +    int ret;
> +    uint32_t acells, scells;
> +    char *nodename = g_strdup_printf("/memory@%" PRIx64, mem_base);
> +
> +    acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
> +                                   NULL, &error_fatal);
> +    scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
> +                                   NULL, &error_fatal);
> +    if (acells == 0 || scells == 0) {
> +        fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 
> 0)\n");
> +        ret = -1;
> +    } else {
> +        qemu_fdt_add_subnode(fdt, nodename);
> +        qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
> +        ret = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
> +                                           acells, mem_base,
> +                                           scells, mem_len);
> +    }
> +
> +    g_free(nodename);
> +    return ret;
> +}
> +
> +static void raspi4_modify_dtb(const struct arm_boot_info *info, void *fdt)
> +{
> +
> +    /* Temporarily disable following devices until they are implemented*/

Missing space before "*/"

> +    const char *to_be_removed_from_dt_as_wa[] = {
> +        "brcm,bcm2711-pcie",
> +        "brcm,bcm2711-rng200",
> +        "brcm,bcm2711-thermal",
> +        "brcm,bcm2711-genet-v5",
> +    };

What does the "_as_wa" part of this variable name mean ?
Since it's local to this function we don't need to use a
super long variable name to keep it distinct; I think
something like "nodes_to_remove" would be fine.

> +
> +    for (int i = 0; i < ARRAY_SIZE(to_be_removed_from_dt_as_wa); i++) {
> +        const char *dev_str = to_be_removed_from_dt_as_wa[i];
> +
> +        int offset = fdt_node_offset_by_compatible(fdt, -1, dev_str);
> +        if (offset >= 0) {
> +            if (!fdt_nop_node(fdt, offset)) {
> +                warn_report("bcm2711 dtc: %s has been disabled!", dev_str);
> +            }
> +        }
> +    }
> +
> +    uint64_t ram_size = board_ram_size(info->board_id);

Variable declarations should always be at the start of a
code block, not in the middle.

> +
> +    if (info->ram_size > UPPER_RAM_BASE) {
> +        raspi_add_memory_node(fdt, UPPER_RAM_BASE, ram_size - 
> UPPER_RAM_BASE);
> +    }
> +}
> +
>  static void raspi4b_machine_init(MachineState *machine)
>  {
>      Raspi4bMachineState *s = RASPI4B_MACHINE(machine);
> @@ -39,6 +98,7 @@ static void raspi4b_machine_init(MachineState *machine)
>      RaspiBaseMachineClass *mc = RASPI_BASE_MACHINE_GET_CLASS(machine);
>      BCM2838State *soc = &s->soc;
>
> +    s_base->binfo.modify_dtb = raspi4_modify_dtb;
>      s_base->binfo.board_id = mc->board_rev;

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

thanks
-- PMM