Re: [PATCH 12/44] Temporary disable unimplemented rpi4b devices

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

On Wed, 26 Jul 2023 at 14:56, Sergey Kambalin <serg.oker@gmail.com> wrote:

"Temporarily".

It would be good to note in the commit message that the
"remove devices from dt" code will all go away in following
commits but that it allows a kernel to boot at this point
(assuming that is what it does). That documents the motivation
for what is otherwise a slightly ugly looking hack.

>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/arm/raspi.c                  |  2 +-
>  hw/arm/raspi4b.c                | 63 +++++++++++++++++++++++++++++++++
>  include/hw/arm/raspi_platform.h |  1 +
>  3 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index da1e9e7c13..cffdd8de4e 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -75,7 +75,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 4096522d85..d2053c9380 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-common")
>  OBJECT_DECLARE_SIMPLE_TYPE(Raspi4bMachineState, RASPI4B_MACHINE)
> @@ -34,6 +35,61 @@ struct Raspi4bMachineState {
>      uint32_t vcram_size;
>  };
>
> +
> +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;
> +}

Why do we need to add a memory node ? The commit message
doesn't say anything about doing this.

> +static void raspi4_modify_dtb(const struct arm_boot_info *info, void *fdt)
> +{
> +
> +    /* Temporary disable following devices until they are implemented*/
> +    const char *to_be_removed_from_dt_as_wa[] = {
> +        "brcm,bcm2711-pcie",
> +        "brcm,bcm2711-rng200",
> +        "brcm,bcm2711-thermal",
> +        "brcm,bcm2711-genet-v5",
> +    };
> +
> +    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);

If you need to get at board-specific info in this hook,
the standard way to do it is to get the MachineState pointer
from the arm_boot_info pointer, like this:
    RaspiBaseMachineState *s_base = container_of(binfo,
RaspiBaseMachineState, binfo);

(which works because the arm_boot_info struct is embedded inside the
RaspiBaseMachineState struct).

But in this specific case I think you can probably work with
info->ram_size (it's the ram size adjusted by the vcram_size, I
think, but that's probably OK.)

> +
> +    if (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);
> @@ -41,6 +97,13 @@ 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;
> +    /*
> +     * Hack to get board revision during device tree modification without
> +     * changes of common code.
> +     * The correct way is to set board_id to MACH_TYPE_BCM2708 and add board_rev
> +     * to the arm_boot_info structure.
> +     */
>      s_base->binfo.board_id = mc->board_rev;

Then you can avoid this hack.

thanks
-- PMM