lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <786bbf35-e9fe-445c-b6f9-21119e60fb34@broadcom.com>
Date: Fri, 10 May 2024 09:14:50 -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>,
 Eric Anholt <eric@...olt.net>, 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 v2 4/4] arm64: dts: broadcom: Add support for BCM2712

On 5/10/24 07:35, 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>
> ---
>   arch/arm64/boot/dts/broadcom/Makefile         |   1 +
>   .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     |  62 ++++
>   arch/arm64/boot/dts/broadcom/bcm2712.dtsi     | 302 ++++++++++++++++++
>   3 files changed, 365 insertions(+)
>   create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
>   create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712.dtsi
> 
> diff --git a/arch/arm64/boot/dts/broadcom/Makefile b/arch/arm64/boot/dts/broadcom/Makefile
> index 8b4591ddd27c..92565e9781ad 100644
> --- a/arch/arm64/boot/dts/broadcom/Makefile
> +++ b/arch/arm64/boot/dts/broadcom/Makefile
> @@ -6,6 +6,7 @@ DTC_FLAGS := -@
>   dtb-$(CONFIG_ARCH_BCM2835) += bcm2711-rpi-400.dtb \
>   			      bcm2711-rpi-4-b.dtb \
>   			      bcm2711-rpi-cm4-io.dtb \
> +			      bcm2712-rpi-5-b.dtb \
>   			      bcm2837-rpi-3-a-plus.dtb \
>   			      bcm2837-rpi-3-b.dtb \
>   			      bcm2837-rpi-3-b-plus.dtb \
> diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
> new file mode 100644
> index 000000000000..b5921437e09f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0

New DTS files should be under GPL-2.0-or-MIT AFAICT.

> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include "bcm2712.dtsi"
> +
> +/ {
> +	compatible = "raspberrypi,5-model-b", "brcm,bcm2712";
> +	model = "Raspberry Pi 5";
> +
> +	aliases {
> +		serial10 = &uart0;
> +	};
> +
> +	chosen: chosen {
> +		stdout-path = "serial10:115200n8";
> +	};
> +
> +	/* Will be filled by the bootloader */
> +	memory@0 {
> +		device_type = "memory";
> +		reg = <0 0 0x28000000>;

That should be a 4 cell member, see my comment on bcm2712.dtsi.

> +	};
> +
> +	sd_io_1v8_reg: sd-io-1v8-reg {
> +		compatible = "regulator-gpio";
> +		regulator-name = "vdd-sd-io";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +		regulator-settling-time-us = <5000>;
> +		gpios = <&gio_aon 3 GPIO_ACTIVE_HIGH>;
> +		states = <1800000 0x1>,
> +			 <3300000 0x0>;

Can we use decimal for the second cell as well?

> +	};
> +
> +	sd_vcc_reg: sd-vcc-reg {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc-sd";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-boot-on;
> +		enable-active-high;
> +		gpios = <&gio_aon 4 GPIO_ACTIVE_HIGH>;
> +	};
> +};
> +
> +/* The system UART */
> +&uart0 {
> +	status = "okay";
> +};
> +
> +/* SDIO1 is used to drive the SD card */
> +&sdio1 {
> +	vqmmc-supply = <&sd_io_1v8_reg>;
> +	vmmc-supply = <&sd_vcc_reg>;
> +	bus-width = <4>;
> +	sd-uhs-sdr50;
> +	sd-uhs-ddr50;
> +	sd-uhs-sdr104;
> +};
> diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
> new file mode 100644
> index 000000000000..398df13148bd
> --- /dev/null
> +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	compatible = "brcm,bcm2712";
> +
> +	#address-cells = <2>;
> +	#size-cells = <1>;

This should be #size-cells = <2> to be future proof and support over 4GB 
of DRAM, because the DDR controller and the memory map on that chip have 
been designed with that requirement.

> +
> +	interrupt-parent = <&gicv2>;
> +
> +	axi: axi {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges;

The AXI peripheral window should be defined in the ranges property. The 
aperture is from 0x10_0000_0000 to 0x10_3FFF_FFFF.

 From that point on you can define all peripherals under the axi node to 
be relative to that axi aperture, just like what you did for the legacy 
Pi peripherals in the subsequent bus node.

> +
> +		sdio1: mmc@...0fff000 {
> +			compatible = "brcm,bcm2712-sdhci",
> +				     "brcm,sdhci-brcmstb";
> +			reg = <0x10 0x00fff000  0x260>,
> +			      <0x10 0x00fff400  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  0x1000>,
> +			      <0x10 0x7fffa000  0x2000>,
> +			      <0x10 0x7fffc000  0x2000>,
> +			      <0x10 0x7fffe000  0x2000>;
> +			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) |
> +				      IRQ_TYPE_LEVEL_HIGH)>;

I would omit this interrupt property, because it is not clear to me 
whether the VGIC maintenance interrupt has actually been wired up.

> +		};
> +	};
> +
> +	clocks {
> +		/* The oscillator is the root of the clock tree. */
> +		clk_osc: clk-osc {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-output-names = "osc";
> +			clock-frequency = <54000000>;
> +		};
> +
> +		clk_vpu: clk-vpu {
> +			#clock-cells = <0>;
> +			compatible = "fixed-clock";
> +			clock-frequency = <750000000>;
> +			clock-output-names = "vpu-clock";
> +		};
> +
> +		clk_uart: clk-uart {
> +			#clock-cells = <0>;
> +			compatible = "fixed-clock";
> +			clock-frequency = <9216000>;
> +			clock-output-names = "uart-clock";
> +		};
> +
> +		clk_emmc2: clk-emmc2 {
> +			#clock-cells = <0>;
> +			compatible = "fixed-clock";
> +			clock-frequency = <200000000>;
> +			clock-output-names = "emmc2-clock";
> +		};
> +	};
> +
> +	cpus: cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		/* Source for d/i cache-line-size, cache-sets, cache-size
> +		 * https://developer.arm.com/documentation/100798/0401
> +		 * /L1-memory-system/About-the-L1-memory-system?lang=en
> +		 */
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a76";
> +			reg = <0x000>;
> +			enable-method = "psci";
> +			d-cache-size = <0x10000>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <256>; // 64KiB(size)/64(line-size)=1024ways/4-way set
> +			i-cache-size = <0x10000>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <256>; // 64KiB(size)/64(line-size)=1024ways/4-way set
> +			next-level-cache = <&l2_cache_l0>;
> +		};
> +
> +		cpu1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a76";
> +			reg = <0x100>;
> +			enable-method = "psci";
> +			d-cache-size = <0x10000>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <256>; // 64KiB(size)/64(line-size)=1024ways/4-way set
> +			i-cache-size = <0x10000>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <256>; // 64KiB(size)/64(line-size)=1024ways/4-way set
> +			next-level-cache = <&l2_cache_l1>;
> +		};
> +
> +		cpu2: cpu@2 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a76";
> +			reg = <0x200>;
> +			enable-method = "psci";
> +			d-cache-size = <0x10000>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <256>; // 64KiB(size)/64(line-size)=1024ways/4-way set
> +			i-cache-size = <0x10000>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <256>; // 64KiB(size)/64(line-size)=1024ways/4-way set
> +			next-level-cache = <&l2_cache_l2>;
> +		};
> +
> +		cpu3: cpu@3 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a76";
> +			reg = <0x300>;
> +			enable-method = "psci";
> +			d-cache-size = <0x10000>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <256>; // 64KiB(size)/64(line-size)=1024ways/4-way set
> +			i-cache-size = <0x10000>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <256>; // 64KiB(size)/64(line-size)=1024ways/4-way set
> +			next-level-cache = <&l2_cache_l3>;
> +		};
> +
> +		/* Source for cache-line-size and cache-sets:
> +		 * https://developer.arm.com/documentation/100798/0401
> +		 * /L2-memory-system/About-the-L2-memory-system?lang=en
> +		 * and for cache-size:
> +		 * https://www.raspberrypi.com/documentation/computers
> +		 * /processors.html#bcm2712
> +		 */
> +		l2_cache_l0: l2-cache-l0 {
> +			compatible = "cache";
> +			cache-size = <0x80000>;
> +			cache-line-size = <128>;
> +			cache-sets = <1024>; // 512KiB(size)/64(line-size)=8192ways/8-way set
> +			cache-level = <2>;
> +			cache-unified;
> +			next-level-cache = <&l3_cache>;
> +		};

Since each of these L2 caches are per-CPU core, they should be under 
their respective CPU node. This is what I did for the BCM2712 sister 
chip, and it looks like this:

                 cpu@0 {
                         operating-points = <0x0 0x0 0x0 0x0 0x0 0x0 0x0 
0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0>;
                         d-cache-sets = <0x100>;
                         d-cache-line-size = <0x40>;
                         d-cache-size = <0x10000>;
                         i-cache-sets = <0x100>;
                         i-cache-line-size = <0x40>;
                         i-cache-size = <0x10000>;
                         clock-frequency = <0x95aad1c0>;
                         device_type = "cpu";
                         enable-method = "psci";
                         compatible = "arm,cortex-a76";
                         reg = <0x0 0x0>;
                         next-level-cache = <0x2>;
                         cpu-idle-states = <0x3>;
                         clocks = <0x4 0x0>;
                         clock-names = "cpu";
                         phandle = <0x9>;

                         cache-controller-0 {
                                 cache-sets = <0x400>;
                                 cache-line-size = <0x40>;
                                 cache-size = <0x80000>;
                                 compatible = "cache";
                                 cache-unified;
                                 cache-level = <0x2>;
                                 next-level-cache = <0x5>;
                                 phandle = <0x2>;
                         };
                 };

Also, the values are actually populated by the boot loader from reading 
the CLIDR registers, and they yield the same results as yours, so this 
part looks good to me. We should consider decimal numbers however for a 
source level Device Tree (the output I provided is rendered via libfdt).




> +
> +		l2_cache_l1: l2-cache-l1 {
> +			compatible = "cache";
> +			cache-size = <0x80000>;
> +			cache-line-size = <128>;
> +			cache-sets = <1024>; // 512KiB(size)/64(line-size)=8192ways/8-way set
> +			cache-level = <2>;
> +			cache-unified;
> +			next-level-cache = <&l3_cache>;
> +		};
> +
> +		l2_cache_l2: l2-cache-l2 {
> +			compatible = "cache";
> +			cache-size = <0x80000>;
> +			cache-line-size = <128>;
> +			cache-sets = <1024>; // 512KiB(size)/64(line-size)=8192ways/8-way set
> +			cache-level = <2>;
> +			cache-unified;
> +			next-level-cache = <&l3_cache>;
> +		};
> +
> +		l2_cache_l3: l2-cache-l3 {
> +			compatible = "cache";
> +			cache-size = <0x80000>;
> +			cache-line-size = <128>;
> +			cache-sets = <1024>; // 512KiB(size)/64(line-size)=8192ways/8-way set
> +			cache-level = <2>;
> +			cache-unified;
> +			next-level-cache = <&l3_cache>;
> +		};
> +
> +		/* Source for cache-line-size and cache-sets:
> +		 * https://developer.arm.com/documentation/100453/0401/L3-cache?lang=en
> +		 * Source for cache-size:
> +		 * https://www.raspberrypi.com/documentation/computers/processors.html#bcm2712
> +		 */
> +		l3_cache: l3-cache {
> +			compatible = "cache";
> +			cache-size = <0x200000>;
> +			cache-line-size = <64>;
> +			cache-sets = <2048>; // 2MiB(size)/64(line-size)=32768ways/16-way set
> +			cache-level = <3>;
> +			cache-unified;
> +		};
> +	};
> +
> +	psci {
> +		method = "smc";
> +		compatible = "arm,psci-1.0", "arm,psci-0.2", "arm,psci";
> +		cpu_on = <0xc4000003>;
> +		cpu_suspend = <0xc4000001>;
> +		cpu_off = <0x84000002>;
> +	};
> +
> +	rmem: reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		atf@0 {
> +			reg = <0x0 0x0 0x80000>;
> +			no-map;
> +		};
> +
> +		cma: linux,cma {
> +			compatible = "shared-dma-pool";
> +			size = <0x4000000>; /* 64MB */
> +			reusable;
> +			linux,cma-default;
> +			alloc-ranges = <0x0 0x00000000 0x40000000>;
> +		};
> +	};
> +
> +	soc: soc@...c000000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		ranges     = <0x7c000000  0x10 0x7c000000  0x04000000>;
> +		/* 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>;
> +		};
> +
> +		uart0: 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>;
> +			status = "disabled";
> +		};

I see no reason for an interrupt controller to be disabled by default, 
if the interrupts are not used, we are not going to be consuming resources.

> +
> +		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.
> +			brcm,gpio-bank-widths = <17 6>;
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
> +					  IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
> +					  IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
> +					  IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
> +					  IRQ_TYPE_LEVEL_LOW)>;
> +		/* This only applies to the ARMv7 stub */
> +		arm,cpu-registers-not-fw-configured;

And the A76 cannot boot an AArch32 kernel at EL1, is that comment still 
relevant? It looks like there is an ARM trusted reserved region, can we 
assume that it will have done the configuration properly?
-- 
Florian


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ