Re: [PATCH 3/6] arm64: dts: broadcom: Add support for BCM2712

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 14/04/2024 00:14, Andrea della Porta wrote:
> Signed-off-by: Andrea della Porta <andrea.porta@xxxxxxxx>
> ---
>  arch/arm64/boot/dts/broadcom/Makefile         |   1 +
>  .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     | 313 +++++++
>  arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi |  81 ++
>  arch/arm64/boot/dts/broadcom/bcm2712.dtsi     | 841 ++++++++++++++++++
>  4 files changed, 1236 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
>  create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi
>  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..2ce180a54e5b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/pwm/pwm.h>
> +#include <dt-bindings/reset/raspberrypi,firmware-reset.h>
> +
> +#define spi0 _spi0
> +#define uart0 _uart0
> +
> +#include "bcm2712.dtsi"
> +
> +#undef spi0
> +#undef uart0
> +
> +/ {
> +	compatible = "raspberrypi,5-model-b", "brcm,bcm2712";

This patch did not pass basic tests. Like checkpatch.

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).


> +	model = "Raspberry Pi 5";
> +
> +	/* Will be filled by the bootloader */
> +	memory@0 {
> +		device_type = "memory";
> +		reg = <0 0 0x28000000>;
> +	};
> +
> +	leds: leds {
> +		compatible = "gpio-leds";
> +
> +		led_act: led-act {
> +			label = "ACT";
> +			gpios = <&gio_aon 9 GPIO_ACTIVE_LOW>;
> +			default-state = "off";
> +			linux,default-trigger = "mmc0";
> +		};
> +	};
> +
> +	sd_io_1v8_reg: sd_io_1v8_reg {

Don't push to us downstream code. Please fix it first and adjust to
match DTS coding style. Underscores are not allowed in node names.

> +		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>;

Aren't these two tupples?

> +		status = "okay";

Why? Where is it disabled?

> +	};
> +
> +	sd_vcc_reg: sd_vcc_reg {

Underscores...

> +		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>;
> +		status = "okay";

Why?

> +	};
> +
> +	wl_on_reg: wl_on_reg {
> +		compatible = "regulator-fixed";
> +		regulator-name = "wl-on-regulator";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		pinctrl-0 = <&wl_on_pins>;
> +		pinctrl-names = "default";
> +
> +		gpio = <&gio 28 GPIO_ACTIVE_HIGH>;
> +
> +		startup-delay-us = <150000>;
> +		enable-active-high;
> +	};
> +
> +	clocks: clocks {
> +	};

Drop, useless.

> +};
> +
> +// Add some labels to 2712 device
> +
> +// The system UART
> +uart10: &_uart0 { status = "okay"; };
> +
> +// The system SPI for the bootloader EEPROM
> +spi10: &_spi0 { status = "okay"; };

Use standard coding style. Look at other recent platforms how it is done.

&spi {
	foo;
};

> +
> +#include "bcm2712-rpi.dtsi"

This goes to the top.

I must say this DTS is terrible to read.

> +
> +/* SDIO1 is used to drive the SD card */
> +&sdio1 {
> +	pinctrl-0 = <&emmc_sd_pulls>, <&emmc_aon_cd_pins>;
> +	pinctrl-names = "default";
> +	vqmmc-supply = <&sd_io_1v8_reg>;
> +	vmmc-supply = <&sd_vcc_reg>;
> +	bus-width = <4>;
> +	sd-uhs-sdr50;
> +	sd-uhs-ddr50;
> +	sd-uhs-sdr104;
> +	cd-gpios = <&gio_aon 5 GPIO_ACTIVE_LOW>;
> +	//no-1-8-v;

Do not add dead code to the kernel.

> +	status = "okay";
> +};
> +
> +&pinctrl_aon {
> +	emmc_aon_cd_pins: emmc_aon_cd_pins {

Again, no underscores.

> +		function = "sd_card_g";
> +		pins = "aon_gpio5";
> +		bias-pull-up;
> +	};
> +
> +	/* Slight hack - only one PWM pin (status LED) is usable */
> +	aon_pwm_1pin: aon_pwm_1pin {
> +		function = "aon_pwm";
> +		pins = "aon_gpio9";
> +	};
> +};
> +
> +&pinctrl {
> +	pwr_button_pins: pwr_button_pins {
> +		function = "gpio";
> +		pins = "gpio20";
> +		bias-pull-up;
> +	};
> +
> +	wl_on_pins: wl_on_pins {
> +		function = "gpio";
> +		pins = "gpio28";
> +	};
> +
> +	bt_shutdown_pins: bt_shutdown_pins {
> +		function = "gpio";
> +		pins = "gpio29";
> +	};
> +
> +	emmc_sd_pulls: emmc_sd_pulls {
> +		pins = "emmc_cmd", "emmc_dat0", "emmc_dat1", "emmc_dat2", "emmc_dat3";
> +		bias-pull-up;
> +	};
> +};
> +
> +/ {

Why the heck this appears in the middle? This is top level section. I am
sorry, but this DTS looks really poor and not like existing coding
style. Please do not introduce some entirely different coding styles.


> +	chosen: chosen {
> +		bootargs = "reboot=w coherent_pool=1M 8250.nr_uarts=1 pci=pcie_bus_safe snd_bcm2835.enable_compat_alsa=0 snd_bcm2835.enable_hdmi=1";

Not a DTS properties. Drop entire bootargs.

> +		stdout-path = "serial10:115200n8";
> +	};
> +
> +	pwr_button {

Srsly...

> +		compatible = "gpio-keys";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pwr_button_pins>;
> +		status = "okay";

???

> +
> +		pwr_key: pwr {

OK, you definitely did not test it. The code looks worse and worse I
keep looking, so I will stop.

This did not pass basic internal review, checkpatch, basic tests.



...

> +
> +		// Pad bank0 out to 32 entries
> +		"", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> +
> +		"HDMI0_SCL", // AON_SGPIO_00
> +		"HDMI0_SDA", // AON_SGPIO_01
> +		"HDMI1_SCL", // AON_SGPIO_02
> +		"HDMI1_SDA", // AON_SGPIO_03
> +		"PMIC_SCL", // AON_SGPIO_04
> +		"PMIC_SDA"; // AON_SGPIO_05
> +};
> +
> +/ {
> +	aliases {

OK, now you are trolling us. It is third top-level node!

Limited review follows.

> +		blconfig = &blconfig;
> +		blpubkey = &blpubkey;
> +		console = &uart10;
> +		mailbox = &mailbox;
> +		mmc0 = &sdio1;
> +		uart10 = &uart10;
> +		serial10 = &uart10;
> +		gpio1 = &gio;
> +		gpio2 = &gio_aon;
> +		gpio3 = &pinctrl;
> +		gpio4 = &pinctrl_aon;
> +	};
> +
> +	__overrides__ {

?

Drop

> +		button_debounce = <&pwr_key>, "debounce-interval:0";
> +		random = <&random>, "status";
> +		sd_cqe = <&sdio1>, "supports-cqe?";
> +		suspend = <&pwr_key>, "linux,code:0=205";
> +		act_led_activelow = <&led_act>,"gpios:8";
> +		act_led_trigger = <&led_act>, "linux,default-trigger";
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi
> new file mode 100644
> index 000000000000..d04e39b9c0b6
> --- /dev/null
> +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi

What is this file for?

> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <dt-bindings/power/raspberrypi-power.h>
> +
> +&soc {
> +	firmware: firmware {
> +		compatible = "raspberrypi,bcm2835-firmware", "simple-mfd";


> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		mboxes = <&mailbox>;
> +		dma-ranges;
> +
> +		firmware_clocks: clocks {
> +			compatible = "raspberrypi,firmware-clocks";
> +			#clock-cells = <1>;
> +		};
> +
> +		reset: reset {
> +			compatible = "raspberrypi,firmware-reset";
> +			#reset-cells = <1>;
> +		};
> +	};
> +
> +	power: power {
> +		compatible = "raspberrypi,bcm2835-power";
> +		firmware = <&firmware>;
> +		#power-domain-cells = <1>;
> +	};
> +
> +	/* Define these notional regulators for use by overlays, etc. */
> +	vdd_3v3_reg: fixedregulator_3v3 {

W=2 warnings.

> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-name = "3v3";
> +	};
> +
> +	vdd_5v0_reg: fixedregulator_5v0 {

W=2 warnings.

> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-name = "5v0";
> +	};
> +};
> +
> +/ {
> +	__overrides__ {
> +		arm_freq;

NAK, drop.

> +	};
> +};
> +
> +&rmem {
> +	/*
> +	 * RPi5's co-processor will copy the board's bootloader configuration
> +	 * into memory for the OS to consume. It'll also update this node with
> +	 * its placement information.
> +	 */
> +	blconfig: nvram@0 {
> +		compatible = "raspberrypi,bootloader-config", "nvmem-rmem";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x0 0x0 0x0>;
> +		no-map;
> +		status = "disabled";
> +	};
> +	/*
> +	 * RPi5 will copy the binary public key blob (if present) from the bootloader
> +	 * into memory for use by the OS.
> +	 */
> +	blpubkey: nvram@1 {
> +		compatible = "raspberrypi,bootloader-public-key", "nvmem-rmem";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x0 0x0 0x0>;
> +		no-map;
> +		status = "disabled";
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
> new file mode 100644
> index 000000000000..fd5a19f68b49
> --- /dev/null
> +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
> @@ -0,0 +1,841 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/soc/bcm2835-pm.h>
> +#include <dt-bindings/phy/phy.h>
> +
> +/ {
> +	compatible = "brcm,bcm2712", "brcm,bcm2711";

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

> +	model = "BCM2712";

Drop



> +
> +	clk_27MHz: clk-27M {

No upperscore letters.

> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <27000000>;
> +		clock-output-names = "27MHz-clock";
> +	};
> +
> +	clk_108MHz: clk-108M {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <108000000>;
> +		clock-output-names = "108MHz-clock";
> +	};
> +
> +	soc: soc {
> +		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@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>;
> +		};
> +
> +		disp_intr: interrupt-controller@7c502000 {
> +			compatible = "brcm,bcm2711-l2-intc", "brcm,l2-intc";
> +			reg = <0x7c502000 0x30>;
> +			interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +			status = "disabled";
> +		};
> +
> +		dvp: clock@7c700000 {
> +			compatible = "brcm,brcm2711-dvp";
> +			reg = <0x7c700000 0x10>;
> +			clocks = <&clk_108MHz>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
> +
> +		/*
> +		 * This node is the provider for the enable-method for
> +		 * bringing up secondary cores.
> +		 */
> +		local_intc: local_intc@7cd00000 {

You really need to clean this up...


> +			compatible = "brcm,bcm2836-l1-intc";
> +			reg = <0x7cd00000 0x100>;
> +		};
> +
> +		uart0: 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";
> +		};
> +
> +		uart2: serial@7d001400 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x7d001400 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";
> +		};
> +
> +		uart5: serial@7d001a00 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x7d001a00 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";
> +		};
> +
> +		sdhost: mmc@7d002000 {
> +			compatible = "brcm,bcm2835-sdhost";
> +			reg = <0x7d002000 0x100>;
> +			//interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;

No dead code.


...

> +
> +		random: rng@7d208000 {
> +			compatible = "brcm,bcm2711-rng200";
> +			reg = <0x7d208000 0x28>;
> +			status = "okay";

Drop.

I just ignored the rest. Quality does not improve. This DTS is in very
poor shape and not suitable for mainline submission.

Please very carefully read DTS coding style and send DTS only after
fixing all automation errors (all! so checkpatch, W=1, dtbs_check W=1)
and after aligning this in 100% to DTS coding style.

Best regards,
Krzysztof