Re: [PATCH v6 11/41] Temporarily disable unimplemented rpi4b devices
Hi Peter and Philippe!
Thank you for the review and feedback!
OK, I'll fix PCIE-relarted comments and the overlapping issue
BR,
Sergey Kambalin
Software Developer,
Auriga Inc.
От: Peter Maydell
Отправлено: 26 февраля 2024 г. 10:41:31
Кому: Philippe Mathieu-Daudé
Копия: Sergey Kambalin; qemu-...@nongnu.org; qemu-devel@nongnu.org; Kambalin,
Sergey
Тема: Re: [PATCH v6 11/41] Temporarily disable unimplemented rpi4b devices
On Mon, 26 Feb 2024 at 16:06, Philippe Mathieu-Daudé wrote:
>
> On 26/2/24 14:39, Peter Maydell wrote:
> > On Mon, 26 Feb 2024 at 13:35, Philippe Mathieu-Daudé
> > wrote:
> >>
> >> On 26/2/24 01:02, Sergey Kambalin wrote:
> >>> +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?
> >
> > I'm not super-keen on it, but as you say it goes away once all
> > the devices are implemented, so I'm not too worried.
> >
> > Doing it this way around would let us take the first 11 patches
> > in the series into git now (they've all been reviewed), which
> > gives us (I think) a functional raspi4 with some missing devices,
> > which seems useful in the interim until the rest of the series
> > gets reviewed and committed.
>
> Fine by me! Sergey, don't we also need patch #39 (Add missed BCM2835
> properties) to have a happy Linux boot?
>
> Patch #17 "Implement BCM2838 thermal sensor" could also go in but it
> doesn't apply cleanly on top of 1-12); maybe Sergey can send a series
> of "patches already reviewed" on top so they get in for v9, postponing
> pcie/network for after release.
I'll put together a pullreq tomorrow (see my other email for details
of which patches plus the necessary changes to the avocado tests).
Sergey -- I suggest you wait til that gets upstream, and then
rebase on that.
-- PMM
Re: [PATCH v6 11/41] Temporarily disable unimplemented rpi4b devices
On Mon, 26 Feb 2024 at 16:06, Philippe Mathieu-Daudé wrote:
>
> On 26/2/24 14:39, Peter Maydell wrote:
> > On Mon, 26 Feb 2024 at 13:35, Philippe Mathieu-Daudé
> > wrote:
> >>
> >> On 26/2/24 01:02, Sergey Kambalin wrote:
> >>> +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?
> >
> > I'm not super-keen on it, but as you say it goes away once all
> > the devices are implemented, so I'm not too worried.
> >
> > Doing it this way around would let us take the first 11 patches
> > in the series into git now (they've all been reviewed), which
> > gives us (I think) a functional raspi4 with some missing devices,
> > which seems useful in the interim until the rest of the series
> > gets reviewed and committed.
>
> Fine by me! Sergey, don't we also need patch #39 (Add missed BCM2835
> properties) to have a happy Linux boot?
>
> Patch #17 "Implement BCM2838 thermal sensor" could also go in but it
> doesn't apply cleanly on top of 1-12); maybe Sergey can send a series
> of "patches already reviewed" on top so they get in for v9, postponing
> pcie/network for after release.
I'll put together a pullreq tomorrow (see my other email for details
of which patches plus the necessary changes to the avocado tests).
Sergey -- I suggest you wait til that gets upstream, and then
rebase on that.
-- PMM
Re: [PATCH v6 11/41] Temporarily disable unimplemented rpi4b devices
On 26/2/24 14:39, Peter Maydell wrote:
On Mon, 26 Feb 2024 at 13:35, Philippe Mathieu-Daudé wrote:
On 26/2/24 01:02, Sergey Kambalin wrote:
+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?
I'm not super-keen on it, but as you say it goes away once all
the devices are implemented, so I'm not too worried.
Doing it this way around would let us take the first 11 patches
in the series into git now (they've all been reviewed), which
gives us (I think) a functional raspi4 with some missing devices,
which seems useful in the interim until the rest of the series
gets reviewed and committed.
Fine by me! Sergey, don't we also need patch #39 (Add missed BCM2835
properties) to have a happy Linux boot?
Patch #17 "Implement BCM2838 thermal sensor" could also go in but it
doesn't apply cleanly on top of 1-12); maybe Sergey can send a series
of "patches already reviewed" on top so they get in for v9, postponing
pcie/network for after release.
Regards,
Phil.
Re: [PATCH v6 11/41] Temporarily disable unimplemented rpi4b devices
On Mon, 26 Feb 2024 at 13:35, Philippe Mathieu-Daudé wrote:
>
> On 26/2/24 01:02, Sergey Kambalin wrote:
> > +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?
I'm not super-keen on it, but as you say it goes away once all
the devices are implemented, so I'm not too worried.
Doing it this way around would let us take the first 11 patches
in the series into git now (they've all been reviewed), which
gives us (I think) a functional raspi4 with some missing devices,
which seems useful in the interim until the rest of the series
gets reviewed and committed.
-- PMM
Re: [PATCH v6 11/41] Temporarily disable unimplemented rpi4b devices
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-f4...@amsat.org/
Keeping previous author S-o-b or adding a "based on work ..."
is usually welcomed.
Signed-off-by: Sergey Kambalin
Reviewed-by: Peter Maydell
---
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 0x8 /* 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
#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, _fatal);
+scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
+ NULL, _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);
+}
+}
[PATCH v6 11/41] Temporarily disable unimplemented rpi4b devices
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
Reviewed-by: Peter Maydell
---
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 0x8 /* 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
#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, _fatal);
+scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
+ NULL, _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)) {
+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);
+}
+}
+
static void raspi4b_machine_init(MachineState *machine)
{
Raspi4bMachineState *s = RASPI4B_MACHINE(machine);
@@ -37,6 +98,7 @@ static void raspi4b_machine_init(MachineState *machine)
RaspiBaseMachineClass *mc = RASPI_BASE_MACHINE_GET_CLASS(machine);
BCM2838State *soc = >soc;
+s_base->binfo.modify_dtb = raspi4_modify_dtb;
s_base->binfo.board_id = mc->board_rev;
object_initialize_child(OBJECT(machine), "soc", soc,
diff --git a/include/hw/arm/raspi_platform.h b/include/hw/arm/raspi_platform.h
index 45003e2425..0db146e592 100644
--- a/include/hw/arm/raspi_platform.h
+++ b/include/hw/arm/raspi_platform.h
@@ -31,6 +31,9 @@
#include "hw/boards.h"
#include "hw/arm/boot.h"
+/* Registered machine type (matches RPi Foundation bootloader and U-Boot) */
+#define MACH_TYPE_BCM2708 3138
+
#define TYPE_RASPI_BASE_MACHINE MACHINE_TYPE_NAME("raspi-base")
OBJECT_DECLARE_TYPE(RaspiBaseMachineState, RaspiBaseMachineClass,
RASPI_BASE_MACHINE)
@@ -59,6 +62,7 @@ void raspi_base_machine_init(MachineState *machine,
void
