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

Florian Fainelli
2024-05-28T23:19:18Z

[-- Attachment #1: Type: text/plain, Size: 4801 bytes --]

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@suse.com>
> ---

[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 #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]