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

[Florian Fainelli <florian.fainelli@broadcom.com>]

[-- 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 --]