[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3adb6776-fd13-427e-bc47-fb470e90045b@broadcom.com>
Date: Tue, 28 May 2024 16:19:14 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Andrea della Porta <andrea.porta@...e.com>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>, Ray Jui <rjui@...adcom.com>,
Scott Branden <sbranden@...adcom.com>,
Broadcom internal kernel review list
<bcm-kernel-feedback-list@...adcom.com>, Ulf Hansson
<ulf.hansson@...aro.org>, Adrian Hunter <adrian.hunter@...el.com>,
Kamal Dasu <kamal.dasu@...adcom.com>, Al Cooper <alcooperx@...il.com>,
Stefan Wahren <wahrenst@....net>, devicetree@...r.kernel.org,
linux-rpi-kernel@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org
Subject: Re: [PATCH v4 4/4] arm64: dts: broadcom: Add support for BCM2712
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@...e.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@...0000000 {
> + 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@...0fff000 {
> + 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@...fff9000 {
> + 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@...c000000 {
> + 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@...03000 {
> + 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@...13880 {
> + compatible = "brcm,bcm2835-mbox";
> + reg = <0x7c013880 0x40>;
> + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> + #mbox-cells = <0>;
> + };
> +
> + local_intc: local-intc@...00000 {
> + compatible = "brcm,bcm2836-l1-intc";
> + reg = <0x7cd00000 0x100>;
> + };
> +
> + uart10: serial@...01000 {
> + 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@...17000 {
> + compatible = "brcm,bcm7271-l2-intc";
> + reg = <0x7d517000 0x10>;
> + interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + };
> +
> + gio_aon: gpio@...17c00 {
> + 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
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)
Powered by blists - more mailing lists