Re: [PATCH v6 11/41] Temporarily disable unimplemented rpi4b devices

[Philippe Mathieu-Daudé]

On 26/2/24 01:02, Sergey Kambalin 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.

This patch really looks like a respin of
https://lore.kernel.org/qemu-devel/20201018205551.1537927-4-f4bug@amsat.org/

Keeping previous author S-o-b or adding a "based on work ..."
is usually welcomed.

> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/arm/raspi.c                  |  5 +--
>   hw/arm/raspi4b.c                | 62 +++++++++++++++++++++++++++++++++
>   include/hw/arm/raspi_platform.h |  4 +++
>   3 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 8b1a046912..a7a662f40d 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 36a4593928..49dec6e53a 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)
> @@ -30,6 +31,66 @@ 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)
> + */
> +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)
> +{
> +    uint64_t ram_size;
> +
> +    /* Temporarily disable following devices until they are implemented */
> +    const char *nodes_to_remove[] = {
> +        "brcm,bcm2711-pcie",
> +        "brcm,bcm2711-rng200",
> +        "brcm,bcm2711-thermal",
> +        "brcm,bcm2711-genet-v5",
> +    };
> +
> +    for (int i = 0; i < ARRAY_SIZE(nodes_to_remove); i++) {
> +        const char *dev_str = nodes_to_remove[i];
> +
> +        int offset = fdt_node_offset_by_compatible(fdt, -1, dev_str);
> +        if (offset >= 0) {
> +            if (!fdt_nop_node(fdt, offset)) {

Peter, I remember a discussion where you wre not keen on altering DTB
for non-Virt machines.

Since these devices are all implemented at the end of the series, why
not add the devices then the raspi4 board at the end, so this patch is
not even required?

Regards,

Phil.

> +                warn_report("bcm2711 dtc: %s has been disabled!", dev_str);
> +            }
> +        }
> +    }
> +
> +    ram_size = board_ram_size(info->board_id);
> +
> +    if (info->ram_size > UPPER_RAM_BASE) {
> +        raspi_add_memory_node(fdt, UPPER_RAM_BASE, ram_size - UPPER_RAM_BASE);
> +    }
> +}