| Message ID | 20230515100438.359690-1-marcin.juszkiewicz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/arm/sbsa-ref: add GIC node into DT | expand |
On Mon, 15 May 2023 at 11:04, Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> wrote: > > Let add GIC information into DeviceTree as part of SBSA-REF versioning. > > Trusted Firmware will read it and provide to next firmware level. > > Bumps platform version to 0.1 one so we can check is node is present. (Missing signed-off-by.) I thought the idea with the sbsa-ref dtb was that it was really just a minimal provision of data to the firmware, not a real DTB, and that (as on real hardware) creating the DTB to pass to the next level of the guest code was the job of the guest firmware. Why do we need to provide a full GIC DTB node ? thanks -- PMM
W dniu 15.05.2023 o 12:15, Peter Maydell pisze:> On Mon, 15 May 2023 at 11:04, Marcin Juszkiewicz > <marcin.juszkiewicz@linaro.org> wrote: >> >> Let add GIC information into DeviceTree as part of SBSA-REF versioning. >> >> Trusted Firmware will read it and provide to next firmware level. >> >> Bumps platform version to 0.1 one so we can check is node is present. > > (Missing signed-off-by.) Oops. Will add in next revision. > I thought the idea with the sbsa-ref dtb was that it was > really just a minimal provision of data to the firmware, > not a real DTB, and that (as on real hardware) creating > the DTB to pass to the next level of the guest code was > the job of the guest firmware. > > Why do we need to provide a full GIC DTB node ? First version had "/magic-numbers/gicd.base" node when I was checking will it work as expected. Then there was discussion about making it proper DT node to avoid "DT has schema for it" replies. I took code from arm/hw/virt.c for it.
On 2023-05-15 11:15, Peter Maydell wrote: > On Mon, 15 May 2023 at 11:04, Marcin Juszkiewicz > <marcin.juszkiewicz@linaro.org> wrote: >> >> Let add GIC information into DeviceTree as part of SBSA-REF versioning. >> >> Trusted Firmware will read it and provide to next firmware level. >> >> Bumps platform version to 0.1 one so we can check is node is present. > > (Missing signed-off-by.) > > I thought the idea with the sbsa-ref dtb was that it was > really just a minimal provision of data to the firmware, > not a real DTB, and that (as on real hardware) creating > the DTB to pass to the next level of the guest code was > the job of the guest firmware. > > Why do we need to provide a full GIC DTB node ? This was actually something we were discussing in private, but felt useful bringing public (for future reference if nothing else). I believe this is a near-verbatim copy from the virt machine to kick that off :) Longer-term, I want to be able to present different specific gic implementations through this interface. I believe the 0.1 variant needs only the Distributor and Redistributors base addresses. / Leif
W dniu 15.05.2023 o 12:27, Leif Lindholm pisze: > On 2023-05-15 11:15, Peter Maydell wrote: >> Why do we need to provide a full GIC DTB node ? > Longer-term, I want to be able to present different specific gic > implementations through this interface. > I believe the 0.1 variant needs only the Distributor and Redistributors > base addresses. TF-A uses only those from this node and ignores rest. https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/20953
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c index 06bd1c5ec4..55dde901f0 100644 --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -29,6 +29,7 @@ #include "exec/hwaddr.h" #include "kvm_arm.h" #include "hw/arm/boot.h" +#include "hw/arm/fdt.h" #include "hw/arm/smmuv3.h" #include "hw/block/flash.h" #include "hw/boards.h" @@ -170,6 +171,39 @@ static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx) return arm_cpu_mp_affinity(idx, clustersz); } +static void sbsa_fdt_add_gic_node(SBSAMachineState *sms) +{ + char *nodename; + int gic_phandle; + + gic_phandle = qemu_fdt_alloc_phandle(sms->fdt); + qemu_fdt_setprop_cell(sms->fdt, "/", "interrupt-parent", gic_phandle); + + nodename = g_strdup_printf("/intc@%" PRIx64, + sbsa_ref_memmap[SBSA_GIC_DIST].base); + qemu_fdt_add_subnode(sms->fdt, nodename); + qemu_fdt_setprop_cell(sms->fdt, nodename, "#interrupt-cells", 3); + qemu_fdt_setprop(sms->fdt, nodename, "interrupt-controller", NULL, 0); + qemu_fdt_setprop_cell(sms->fdt, nodename, "#address-cells", 0x2); + qemu_fdt_setprop_cell(sms->fdt, nodename, "#size-cells", 0x2); + qemu_fdt_setprop(sms->fdt, nodename, "ranges", NULL, 0); + + qemu_fdt_setprop_string(sms->fdt, nodename, "compatible", + "arm,gic-v3"); + + qemu_fdt_setprop_sized_cells(sms->fdt, nodename, "reg", + 2, sbsa_ref_memmap[SBSA_GIC_DIST].base, + 2, sbsa_ref_memmap[SBSA_GIC_DIST].size, + 2, sbsa_ref_memmap[SBSA_GIC_REDIST].base, + 2, sbsa_ref_memmap[SBSA_GIC_REDIST].size); + + qemu_fdt_setprop_cells(sms->fdt, nodename, "interrupts", + GIC_FDT_IRQ_TYPE_PPI, ARCH_GIC_MAINT_IRQ, + GIC_FDT_IRQ_FLAGS_LEVEL_HI); + + qemu_fdt_setprop_cell(sms->fdt, nodename, "phandle", gic_phandle); + g_free(nodename); +} /* * Firmware on this machine only uses ACPI table to load OS, these limited * device tree nodes are just to let firmware know the info which varies from @@ -206,7 +240,7 @@ static void create_fdt(SBSAMachineState *sms) * fw compatibility. */ qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0); - qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 0); + qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 1); if (ms->numa_state->have_numa_distance) { int size = nb_numa_nodes * nb_numa_nodes * 3 * sizeof(uint32_t); @@ -262,6 +296,8 @@ static void create_fdt(SBSAMachineState *sms) g_free(nodename); } + + sbsa_fdt_add_gic_node(sms); } #define SBSA_FLASH_SECTOR_SIZE (256 * KiB)