Re: [PATCH v4 4/4] arm64: dts: broadcom: Add support for BCM2712

From: Florian Fainelli
Date: Tue May 28 2024 - 19:19:32 EST

On 5/28/24 06:32, Andrea della Porta wrote:
The BCM2712 SoC family can be found on Raspberry Pi 5.
Add minimal SoC and board (Rpi5 specific) dts file to be able to
boot from SD card and use console on debug UART.

Signed-off-by: Andrea della Porta <andrea.porta@xxxxxxxx>
---

[snip]

diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
new file mode 100644
index 000000000000..71b0fa6c9594
--- /dev/null
+++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+ compatible = "brcm,bcm2712";
+
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ interrupt-parent = <&gicv2>;
+
+ axi: axi@1000000000 {
+ compatible = "simple-bus";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges = <0x10 0x00000000 0x10 0x00000000 0x01 0x00000000>;

That property does not look correct, you want to define an aperture that starts at 0x10_0000_0000 into the CPU's address space, and then have sub-node(s) whose "reg" property is to be encoded using #address-cells = <1> and #size-cells = <1> because that's enough to represent that address space.

+
+ sdio1: mmc@1000fff000 {
+ compatible = "brcm,bcm2712-sdhci",
+ "brcm,sdhci-brcmstb";
+ reg = <0x10 0x00fff000 0x0 0x260>,
+ <0x10 0x00fff400 0x0 0x200>;

That "reg" property should not be including the 0x10_0000_0000 offset at all, and we should just have:

reg = <0xfff000 0x260>,
<0xfff400 0x200>;


+ reg-names = "host", "cfg";
+ interrupts = <GIC_SPI 273 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk_emmc2>;
+ clock-names = "sw_sdio";
+ mmc-ddr-3_3v;
+ };
+
+ gicv2: interrupt-controller@107fff9000 {
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ compatible = "arm,gic-400";
+ reg = <0x10 0x7fff9000 0x0 0x1000>,
+ <0x10 0x7fffa000 0x0 0x2000>,
+ <0x10 0x7fffc000 0x0 0x2000>,
+ <0x10 0x7fffe000 0x0 0x2000>;

Likewise, this is supposed to be:

reg = <0x7fff9000 0x1000>,
<0x7fffa000 0x2000>,
<0x7fffc000 0x2000>,
<0x7fffe000 0x2000>;

[snip]

+
+ soc: soc@107c000000 {
+ compatible = "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ ranges = <0x7c000000 0x10 0x7c000000 0x04000000>;

That spans the previously defined axi node above (goes up to 0x10_8000_0000), once we make the necessary "ranges" adjustments to the axi node as indicated before.

I thought that based upon our conversation in v3, this was all going to be a single "soc" node?

+ /* Emulate a contiguous 30-bit address range for DMA */
+ dma-ranges = <0xc0000000 0x00 0x00000000 0x40000000>,
+ <0x7c000000 0x10 0x7c000000 0x04000000>;
+
+ system_timer: timer@7c003000 {
+ compatible = "brcm,bcm2835-system-timer";
+ reg = <0x7c003000 0x1000>;
+ interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
+ clock-frequency = <1000000>;
+ };
+
+ mailbox: mailbox@7c013880 {
+ compatible = "brcm,bcm2835-mbox";
+ reg = <0x7c013880 0x40>;
+ interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
+ #mbox-cells = <0>;
+ };
+
+ local_intc: local-intc@7cd00000 {
+ compatible = "brcm,bcm2836-l1-intc";
+ reg = <0x7cd00000 0x100>;
+ };
+
+ uart10: serial@7d001000 {
+ compatible = "arm,pl011", "arm,primecell";
+ reg = <0x7d001000 0x200>;
+ interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk_uart>, <&clk_vpu>;
+ clock-names = "uartclk", "apb_pclk";
+ arm,primecell-periphid = <0x00241011>;
+ status = "disabled";
+ };
+
+ interrupt-controller@7d517000 {
+ compatible = "brcm,bcm7271-l2-intc";
+ reg = <0x7d517000 0x10>;
+ interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ };
+
+ gio_aon: gpio@7d517c00 {
+ compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio";
+ reg = <0x7d517c00 0x40>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ // Don't use GIO_AON as an interrupt controller because it will
+ // clash with the firmware monitoring the PMIC interrupt via the VPU.

OK, so the comment is intended to explain why there is no 'interrupt-controller' property specified when one might expect to find one, and it has nothing to do with the "brcm,gpio-bank-widths" property, because that one is correct and does indicate the number of pins that need to be managed per bank. Some clarification is warranted here IMHO.
--
Florian

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature